When you subscribe using comment_notify, it would be better for the subscription attached to a node, not a comment. Otherwise, if you participate in a conversation, it may be nearly impossible to unsubscribe, since you may be subscribed every time you comment.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

The underlying bug is:

1. Post 2 comments on a node where you subscribe to notifications on both
2. A new comment is posted, you get an e-mail
3. Click the unsubscribe link

Expected results:
The user never gets e-mails again from this thread.

Actual results:
One of the comments is unsubscribed, but the other remains subscribed.

The proposed solution from OP would also help solve #288726: Allow users to subscribe without posting a comment.

An alternate solution would be to replace the cid in the has with the nid.

nico24’s picture

Here's is a solution. Make your own module OWN_MODULE:

/**
 * Alters the existing filter path to this custom function
 *
 * @param unknown_type $callbacks
 */
function OWN_MODULE_menu_alter(&$callbacks) {
	$callbacks['comment_notify/disable/%']['page callback'] = 'OWN_MODULE_disable_page';
}

/**
 * Page callback to allow users to unsubscribe simply by visiting the page.
 */
function OWN_MODULE_disable_page($hash) {
	db_query("UPDATE {comment_notify} AS cn, (
	SELECT c.cid
	FROM {comments} c, (
		SELECT oc.nid, oc.uid
		FROM {comments} AS oc, {comment_notify} AS ocn
		WHERE oc.cid = ocn.cid
		AND ocn.notify_hash = '%s'
		) AS o
	WHERE o.nid = c.nid
	AND o.uid = c.uid
	) AS sc
SET cn.notify = 0
WHERE cn.cid = sc.cid", $hash);

  drupal_set_message(t('Your comment follow-up notification for this post was disabled. Thanks.'));
  return ' ';
}

For the people who do like hacking in thirdparty modules you can just change comment_notify.module line 202:
db_query("UPDATE {comment_notify} SET notify = 0 WHERE notify_hash = '%s'", $hash);
to:

	db_query("UPDATE {comment_notify} AS cn, (
	SELECT c.cid
	FROM {comments} c, (
		SELECT oc.nid, oc.uid
		FROM {comments} AS oc, {comment_notify} AS ocn
		WHERE oc.cid = ocn.cid
		AND ocn.notify_hash = '%s'
		) AS o
	WHERE o.nid = c.nid
	AND o.uid = c.uid
	) AS sc
SET cn.notify = 0
WHERE cn.cid = sc.cid", $hash);

I don't know about the performance of this fix, but it should be fine. Easier would be if the nid and uid also get saved in the comment_notify table. But for now this works.

edit: I didn't check all issues to see if there was already a solution. So sorry if this is double.

rfay’s picture

@nico24, thanks for your study of this problem. Please provide a patch that can improve this module and provide it for review here.

greggles’s picture

I think we should first disable the subscriptions for the specific hash.

Then, we can disable all subscriptions on that node that are "node level" subscriptions.

That should stick with what I consider to be expected behavior around the two subscription modes.

premanup’s picture

Status: Needs work » Needs review
FileSize
916 bytes

@nico24, thanks for your approach, performance is good, this is a patch for #2
worked quite well for me at first glance. Will report if I come accross something strange.

Status: Active » Needs work

The last submitted patch, comment_notify_unsubscribe_should_be_by_node.patch, failed testing.

premanup’s picture

Status: Needs review » Needs work
FileSize
886 bytes

second try

premanup’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, comment_notify_539214_1.patch, failed testing.

greggles’s picture

Version: 6.x-1.2 » 7.x-1.x-dev

Let's fix in 7.x first and then backport.

greggles’s picture

I just wanted to share that there is another issue - #1404990: More informative confirmation when unsubscribing which came about for a different reason but has a potential solution to this issue.

burgs’s picture

Status: Needs work » Needs review
FileSize
1001 bytes

A patch is attached that gets all the comment ids (cid) for the node in question, and disables notifications on all of those. It's an extra query, but at least it's SQL99 compliant and now D7. I'm sure it could use a config flag at some point for people who have nested comments and complex requirements, but I presume that most people will want it to work this way. It should apply nicely to 7.1 and/or 7.x-dev.

Henke001’s picture

Would it be possible to get this for Drupal 6 and node comments? Some of my forum users are getting a bit upset by not being able to unsubscribe. Thanks in advance!

greggles’s picture

Title: Subscribe/Unsubscribe should be by node, not by comment » Subscribe/Unsubscribe should be by node, not by comment if the subscription is to all comments
FileSize
2.58 KB

Thanks, burgs! This seems like decent progress, but this seems like it would give unexpected results for someone who used threading and subscribed to different sub-threads.

I'm changing the title to reflect that goal. New patch attached - anyone up for a review?

@Henke001 I appreciate your enthusiasm, but the best thing to move it forward is a patch review.

greggles’s picture

Status: Needs review » Fixed

Ok, now fixed http://drupalcode.org/project/comment_notify.git/commit/37be47c to get some testing of this code in the 7.x-1.x-dev branch.

Thanks, burgs!

burgs’s picture

Status: Fixed » Closed (fixed)

Thanks greggles! That's working really well now :)

greggles’s picture

Status: Closed (fixed) » Fixed

Cool, thanks burgs. Note that this should stay in a fixed state and gets closed automatically after 2 weeks. That delay helps people know what has changed recently.

Status: Fixed » Closed (fixed)

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