Do not send subscription email for unpublished content

I mark this critical since a spammer can quickly ruin user experience. There is a duplicate: http://drupal.org/node/141685

Looks like this needs to be solved by adding extra checks to subscriptions_comment. Seems to me correct handling might ultimately require validation of hooks other than just "insert" and "update" (for example "publish" and "unpublish"). Here is the 4.7 documentation of the hook:
http://api.drupal.org/api/4.7/function/hook_comment

The check should probably go right after node unpublishing check in subscriptions_comment:

      $cobj = _comment_load($cid);
      if($cobj->status != 1) return;  // ignore deactivated comments

The issue should be fixed in 4.7 branch as well - this is where I am doing my testing.

subscribe. +1

OK, I got this tested and found another bug in the original code. subscriptions_comment uses 1 as a constant denoting a published comment, while the actual value is 0 (yes, counterintuitive). So this should be the updated if statement:

  if ($op == 'insert' || ($op == 'update' && $comment->status == COMMENT_PUBLISHED)) { // ignore deactivated comments

and this should be the updated check that goes right after $nobj check:

      $cobj = _comment_load($cid);
      if($cobj->status != COMMENT_PUBLISHED) return;  // ignore deactivated comments

This has been tested and is ready to commit...

subscribing

Subscribing.
- is there no -dev version that contains all the small but "critical" bugs reported in April and May?

eh, bugfixes, I mean, heh

This bug has caused us to discontinue use of the Subscriptions module. If there is a fix, could someone please direct me to it?

@ryness: Read above for the manual fix for 4.7.

As for 5.1, I just rolled out updated patch that also correctly handles the case when comment is moderated and later published for example by admin. The new logic looks only at the comment's state in the database. This has been tested and is ready to go.

I have independently arrived at exactly the same patch as dkruglyak just above; wish I'd seen it first; could have saved myself some trouble!

However, I believe I found a place that needs to be patched which dkruglyak missed. A major bugfix provided by this patch is to pass the $comment['uid'] instead of $user->uid since passing the latter utterly breaks functionality when admins use moderated comments. But the same bug exists in the subscriptions_autosubscribe function, so I've made up this additional patch, which I suggest you apply after applying the one by dkruglyak above.

All it does is change subscriptions_add($nid, $user->uid, 'node'); to subscriptions_add($nid, $uid, 'node'); in the subscriptions_autosubscribe function.

Hi,

I am puzzled by the dates in this bug. Has this been committed to the Subscriptions codebase? Is there a stable release that implements the changes?

Merc.

I have no idea. This was before chx and I rewrote Subscriptions from scratch in the fall of 2007, and there's very little left of 5.x-1.x-dev.

Subscriptions 5.x-2.x+ does not send out notifications for unpublished content.

Hi,

Fantastic. Thanks a lot.

Merc.

I am having the opposite problem, for certain roles I need to treat them as "admin" and allow them to receive the subscribe messages. But unfortunately because they aren't admin they aren't getting the notices, because the default state of the node type I am allowing people to submit is unpublished.

If there was a way to determine if a role, or roles were treated as "admins" to the subscription module that would make this a bit more extensible. For now I am looking for a work around, because I have to forward the messages to the person/persons for them to see them currently.

Thanks!

Hmm, no, sorry, I don't see any way to do this in a generic way. You'd have to patch Subscriptions for your purpose. If this person does not have the 'administer nodes' permission, you probably don't want to make all unpublished content available to them, so you'll need further logic.

Well I am also using the "override node publishing options", as well as "view unpublished content", to allow this user to view, and then publish specific content, that they can edit.

I don't want the user to be able to see any nodes except nodes they own, or subscribe to. Right now only admins and certain rules can even see subscription options, and if they get any options in their profile they can't subscribe to any content. Very granular, but very powerful.

The issue is that if I "allow the user to administer nodes", they can then edit any node, which is not the functionality I am implementing.

In the future I may add more roles or users that have very specific control over specific content types, and therefore should only get notifications about those nodes they can edit, and publish.

Is there a way you can do some tie in with the "view unpublished content" module to do a check and see if the user in question can view that unpublished content. If the admin (me) doesn't have this module installed, it defaults to it's current behavior.

Seems really roundabout, but it gives a setting and control, and it's not "hacked" or hard coded into the module, and it also doesn't just do a blanket, let all users get subscriptions notifications for unpublished content.

Is there a way you can do some tie in with the "view unpublished content" module to do a check and see if the user in question can view that unpublished content.

No. http://drupal.org/project/view_unpublished is not an established module and there would have to be much stronger support (start with an actual release as well as a D6 version) before I'd even consider this.

Such tie-in should not be necessary in the first place, if the unpublished_content authors did their homework. Patching contrib modules all over the place is not the right solution to infrastructure issues.

BTW:

I am having the opposite problem [...]

You should have created a new issue rather than sneaking into a closed thread discussing a different issue, then you'd be able to move this over to the view_unpublished queue now.