Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Dave Reid’s picture

Version: 7.x-1.0-alpha1 » 7.x-1.x-dev
Category: support » bug
Status: Active » Needs review
Issue tags: +token
FileSize
831 bytes

Oops. Small bug in the token code...

greggles’s picture

Status: Needs review » Fixed

Now fixed - http://drupal.org/cvs?commit=445084

Thanks, Dave.

VladGh’s picture

Status: Fixed » Needs review

I am sorry but it's still not working. The emails received are still like this:

You can stop receiving emails when someone replies to this post by
visiting the following address [comment:unsubscribe-url]

greggles’s picture

VladGh, how did you update the code? Or did you apply the patch directly?

VladGh’s picture

First I just replaced $cid with $comment->cid by hand. But then, because it was still not working, I replaced the entire 7.x-1.0-alpha1 release with 7.x-1.x-dev. All steps were accompanied by drush cc all and cleared cache in Firefox.

greggles’s picture

Status: Needs review » Needs work

Well, if you fixed it by hand that should have worked. The tarball might not have been re-rolled by the time you tested it.

Looking at the code I'm a bit stumped but don't have a setup to test/debug right now.

VladGh’s picture

I added return $replacements; at the end of comment_notify_tokens function and now the link is rendered correctly, but in the preliminary tests it does not actually removes the user's email from the database. Meaning I get the following message "Your comment follow-up notification for this post was disabled. Thanks." but I still receive new comment notifications.

Dave Reid’s picture

Status: Needs work » Needs review
FileSize
490 bytes

Agh!

Dave Reid’s picture

Priority: Normal » Major
David D’s picture

Version: 7.x-1.x-dev » 7.x-1.0-alpha1

I'm still just getting the unlinked [comment:unsubscribe-url] token text in my email. I'm running D 7.0.

mermentau’s picture

I'm still seeing this with the latest 7.x-1.x-dev (Nov. 23, 2010). Running Drupal 7.0

Dave Reid’s picture

Version: 7.x-1.0-alpha1 » 7.x-1.x-dev

Please test the patch in comment #8.

mermentau’s picture

Patch works for me.

VladGh’s picture

I am sorry but I have to return to the same problem as #7. With the latest 7.x-1.x-dev, the unsubscribe link is rendered correctly in the emails, but the functionality is broken for unauthenticated users. The guest users can subscribe and they receive all the emails as intended, but when clicking on the unsubscribe link, and receiving the successful removed message, they still receive ulterior emails.

VladGh’s picture

I see that the notify field in the comment_notify table is changing from 1 or 2 to 0 correctly in the database, but some how it's lost either in the _get_watchers in the .inc file or at foreach ($watchers as $alert) (line 467 in the .module)

codeforkjeff’s picture

Patch in #8 applied to feb 25 dev version works for me. Should go into git.

grendzy’s picture

Status: Needs review » Reviewed & tested by the community

looks good.

VladGh’s picture

I am sorry but the functionality is still impaired. Like I said in #7 and #15 guest users cannot unsubscribe, even if they clicked the link and got the success message.

grendzy’s picture

Status: Reviewed & tested by the community » Needs work

whoops, you're right. I only tested the token itself, not the unsubscribe function.

codeforkjeff’s picture

Oops, VladGh is right.

The problem is that the hash in the unsubscribe-url refers to the new comment made, NOT to the comment that asked for notifications. That's why it's not working: it's actually unsubscribing the wrong comment.

Attached is a patch.

My patched fork of comment_notify is also here: https://github.com/codeforkjeff/comment_notify

codeforkjeff’s picture

My patch in #20 introduced an index error. Please apply this one on top of it, to get rid of the error message.

Sorry about that.

jcw’s picture

Confirmed - patches #20 + #21 solved it for me.

jcw’s picture

Eh, wait... still a problem: I posted a comment (as jcw), and it was sent out to someone else starting off as "Hi jcw," i.e. addressed to me i.s.o. him. The default text sent out starts off with "Hi [user:name],". I'm the author, someone else commented, and then I replied. Could this be another case where $watcher and $comment need to be adjusted in the code?

codeforkjeff’s picture

jcw: please see this issue: http://drupal.org/node/1030022

I'll post a patch to that issue shortly, would appreciate it if you had any feedback. Thanks!

jcw’s picture

Yep - that patch works perfectly. Thx!

Thomas Bosviel’s picture

Confirmed - patches #8 + #20 + #21 fixed it.

JayaAmbika’s picture

I am using comment_notify7.x-1.0 alpha version and i am also getting the same problem with unsubscribe url. i tried with the patches above but still not working.

JayaAmbika’s picture

Now its working

Dave Reid’s picture

Now, can someone please roll all the patches together?

ibrahim ali’s picture

I hope the author or someone else can bring all these patches and fixes of all errors together in a new update. This module is really nice and it will be a waste not to update it. It looks the last update was in february 25. I am not sure if the author is following up these threads!

Thomas Bosviel’s picture

Status: Needs work » Needs review
FileSize
1.38 KB

Patch based on dev release (master branch). Patches #8, #20 and #21 included.

Dave Reid’s picture

Status: Needs review » Needs work

Actually we need to re-approach this. We need a new token type 'comment-subscribed' that is a 'comment' token type. We want the user to be able to enter custom e-mail texxt and not have the token invalidated. Patch incoming.

Anticosti’s picture

Subscribing...

geerlingguy’s picture

Submarine.

illogicz’s picture

Subroutine

mgifford’s picture

Just ran into this error. Dave, just wanted to say I'm happy to test the patch when it comes out. I got:

You can stop receiving emails when someone replies to this post,
by going to [comment:unsubscribe-url]

in an email from comment notify today. Not a mission critical thing, but a nice bug to work out before it lands on a clients desk.

Anyways, thanks!

rodrigoaguilera’s picture

Subaru

Dave Reid’s picture

Revised patch that should address the following issues:
1. Only 'comment' tokens should be the only token type used in the notifications. Enforces this on token replacement and provides an update function to fix most existing e-mail subject/body strings.
2. Provides a new 'comment-subscribed' token type to reference the commenter that is being notified of a new comment.
3. Fixes user account object and language handling in _comment_notify_mailalert().
4. Provides a re-usable comment_notify_get_unsubscribe_url() for use with tokens.

Needs to be throughly tested.

Dave Reid’s picture

Assigned: Unassigned » Dave Reid
mgifford’s picture

Version: 7.x-1.x-dev » 7.x-1.0-alpha1

I applied this to 7.x-1.0-alpha1 and it all worked until patching file comment_notify.tokens.inc
Hunk #1 succeeded at 10 (offset 1 line).
Hunk #2 FAILED at 49.

I haven't received any notifications yet, but will let you know.

mgifford’s picture

Status: Needs work » Needs review

should have done this earlier.

Status: Needs review » Needs work

The last submitted patch, 952072-commentnotify-token-fixes.patch, failed testing.

Dave Reid’s picture

Version: 7.x-1.0-alpha1 » 7.x-1.x-dev
Status: Needs work » Needs review

Ah, make sure to patch against 7.x-1.x-dev. I've tested it applies cleanly.

rwohleb’s picture

The patch applied cleanly for me. It seems to work as expected so far.

Dave Reid’s picture

This time without a serious logic error in the update function...

mgifford’s picture

I tried that patch #47, but haven't gotten an email yet to confirm. It applied nicely.

guillaumev’s picture

Status: Needs review » Reviewed & tested by the community

Tried patch #47, did the update and can confirm that it works. I would love to see it committed, as well as this one tested :-)

sam_squarewave’s picture

Would love to this this module working! Getting the same error message with the latest version of Drupal 7.

berenddeboer’s picture

Yes, it would be nice to have a wrapup of all patches reported by people so this module is actually usable out of the box.

greggles’s picture

Title: [comment:unsubscribe-url] is not working » [comment:unsubscribe-url] is not working (and a few other token issues)
Status: Reviewed & tested by the community » Fixed

Thanks for the work and reviews, everyone.

It's nearly time for a new release, I think.

http://drupalcode.org/project/comment_notify.git/commit/f7b5a4d

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Rewarp’s picture

Status: Closed (fixed) » Needs work

Hi. I just installed this module and after configuring the module I got the following message after saving the settings:

The Default mail text for sending out notifications to commenters is using the following invalid tokens: [comment-subscribed:author], [comment-subscribed:unsubscribe-url].

I am very new to Drupal so I have no idea how to fix this.

greggles’s picture

Status: Needs work » Closed (fixed)

@Rewarp - I don't get that error and it seems nobody else is. Maybe make sure you're using the most recent version?

RobKoberg’s picture

Hi, I got the same error as #54 after enabling comment_notify and going directly to configure it:

The Default mail text for sending out notifications to commenters is using the following invalid tokens: [comment-subscribed:author], [comment-subscribed:unsubscribe-url].

The problem was that I had not enabled the permission "Subscribe to comment notifications". Without enabling the perm first, the tokens do not show for comment-subscribed:*

Pafla’s picture

Status: Closed (fixed) » Active

Hi

I just downloaded 7.x-1.x-dev from 2012-08-15 +
7.x-1.1 from 2012-06-13
And tried both but the error is still there

I use Token 7.x-1.3

Permissions is set for "Subscribe to comment notifications" to all except anonymous

The Default mail text for sending out notifications to commenters is using the following invalid tokens: [comment-subscribed:author], [comment-subscribed:unsubscribe-url].

davidwhthomas’s picture

I had the same issue as Pafla in #57 and it was solved by clearing all caches at

Admin > Performance > Clear all caches

to make the new tokens available on the config form.

DT

kscheirer’s picture

Assigned: Dave Reid » Unassigned
Priority: Major » Normal
Status: Active » Fixed

Solution in #58. Don't reopen this issue, make a new one if you're still having trouble.

Status: Fixed » Closed (fixed)
Issue tags: -token

Automatically closed -- issue fixed for 2 weeks with no activity.

tgeller’s picture

Issue summary: View changes

I had this problem, but believe I've found the answer. It's so simple, you'll slap yourself.

The original poster *can't* unsubscribe from individual threads. This is by design, and well-documented.

But if you're like me, you tried to be thorough and threw an unsubcribe token in there anyway. That token won't be converted to a link.

So: Go back to admin/config/people/comment_notify and remove the unsubscribe token from the "Default mail text for sending out the notifications to node authors" section, and be happy.

If it fails to work in the "Default mail text for sending out notifications to commenters" section, however, there's a real problem.