Delayed mail delivery / spam prevention

Thank you for the well thought-out feature request. I clearly see the need.

Previewing the notifications is not feasible. For example, comments are always merged with pending new or update notifications, so spam and non-spam could be in the same preview. For the same reason not all users get the same notifications in the same context, e.g. some users may already have received the new notification and will get the comment separately, and others with larger (or offset) Send Intervals would get them merged.

I'd rather see posts and comments getting links like
-- release notifications (where notifications are held)
-- hold notifications (where notifications are auto-released)
with the former going to a confirmation form with a checkbox like
[ ] auto-release notifications for this user

Is a notification moderation queue form needed?

Is auditing of moderator actions needed? history of poster qualifications?

I've been thinking about this and I found that my original idea was a bit too simple for various reasons, e.g. it doesn't provide a way to get items off the queue in all cases, and it's cumbersome to use because it involves reloads. Whatever interface it is, it will appear below every post and every comment that has notifications in the queue, so it really ought to work well!

You recently mentioned on the dev list that you're getting into JS/AJAX — maybe we can do something together here. I'm thinking of a dynamic interface like the following:

Notifications: discard | 0 | HELD | 0 | release auto-release

Clicking discard would turn it into

Notifications:            | 1 | DISCARDED | 0 |

Clicking release or auto-release (the latter would need an additional permission) instead would turn it into either of

Notifications:    recall | 0 | RELEASED | 1 | auto-release
Notifications:    recall | 0 | AUTO-RELEASED | 1 |

A second post would then show up (depending on how the first one was treated) as either of

Notifications: discard | 1 | HELD | 0 | release auto-release
Notifications: discard | 0 | HELD | 1 | release auto-release
Notifications:   recall | 0 | AUTO-RELEASED | 2 |

Clicking on recall would change it to

Notifications:            | 1 | RECALLED | 2 |

If we could do this in place, without a reload, that'd be pretty neat!

So, can you write me a widget that fills a <div> with HTML, including a few links that perform some actions and refill the <div>? It'll have to work as multiple instances on the same page and needs to have an anchor and a fall-back non-JS interface. Keeping a constant width in the first column would be a nice touch.

I am certainly not an ajax guru.

But you are way ahead of me. So, if you'll take care of the UI, I'll take care of the implementation.

Designing the details of the UI helps me find the use cases and thus the requirements for the inner workings.

The entries in {subscriptions_queue} are by recipient and by node/comment. When someone creates (or updates) a node or comment, then every matching subscription puts an entry into the queue for each subscribed user.

cron consumes the entries in FIFO manner. For every entry, when it loads a node, the queue is automatically scanned for comment entries for that node and user. From this information a notification is assembled (node + comments, for this user) and sent, and the comment entries as well as all other queue items with the same load_function and load_args (and user) are removed. The actual implementation is a little more involved, but these are the essential parts.

So, in the current implementation,
queue item U10/C1 is picked up, N loaded, queue items U10/C1..4 fetched, comments loaded, notification assembled and sent, and the queue items U10/C1..4 are removed.

Then
queue item U11/C2 is picked up, N loaded, queue items U10/C2..4 fetched, comments loaded, notification assembled and sent, and the queue items U11/C2..4 are removed. U11/C1 had already been removed during the previous cron run according to your set-up.

***

For the new implementation we have to decide whether moderators receive advance notification of unreleased posts. I tend to say no. Notification moderation needs to be organized so that it's done in a timely manner, and I think it can only work well when enough people patrol the site frequently enough.

The trust state needs to be stored with the author, i.e. in {subscriptions_user}. Before items (nodes as well as comments) are inserted into the queue they need to be checked against the trust state of the author. The trust state would be one of (AUTO-RELEASE, HOLD, and maybe DISCARD) and translates directly into initial actions taken when content is created. The count of good and bad posts would have to be stored there as well for efficient access. We'd have
U1: AUTO-RELEASE
U2: HOLD
U3: HOLD
U4: AUTO-RELEASE

To store the moderation state, we need a new table {subscriptions_moderate}:
nid,cid: primary key (cid=0 for the node)
state: RECALLED/DISCARDED/HELD

Items not in the table would be RELEASED (or AUTO-RELEASED, depending on the author).

At creation time, C1 and C4 would be queued and C2 and C3 entered into {subscriptions_moderate} as HELD. So, C1 and C4 would be notified as usual and C2 and C3 not. When the moderator releases C2 and C3, then they are entered into the queue and notified as usual. If he auto-releases C2, then {subscriptions_user} is updated and all held content of U2 is queued.

We might sort the comments that are assembled into one notification try to preserve chronological order as well as possible, but we can't avoid C4 being notified before C2 and C3, if cron runs in the meantime.

When the moderator discards C2, it's updated in {subscriptions_moderate} only. When he releases and recalls it, then queue entries are removed also. Notifications that have already gone out can't really be recalled, of course, but "recall" describes the intent.

It wouldn't make sense to notify comments for a node before that node's notifications are released. Thus all comments would need to be implicitly put on hold (and blocked from moderation) as long as the node needs moderation. When the node is released, its comments could be dealt with as if they had just been created.

Is user U10 notified of C1 and C4, but not of C2 and C3?
Assuming everything is ok, is he notified of C2 and C3 after the moderation process?
Or is he notified of C1 only?
Or not at all, until the comments in this nodes are moderated?
Is U11 notified of anything at all?

yes
yes
no
no
yes, C4 right away, and C2 and C3 after release

How do you tag internally which change (new comment) has been notified to whom?

No. As explained above, the queue works as a ToNotify list at the most granular (and redundant!) level. Every processed notification removes the items that would lead to duplicate notifications (to the same user). So this essentially takes care of itself. This design is due to chx, btw.

Do you use timestamps: check the last time the users were sent notifications, and see what content has been added since that time?

The timestamps are only used to delay processing, if a Send Interval > ASAP was selected. This automatically has the effect, that multiple comments may accumulate in the queue, and when the time has come, they'll be merged.

Before dealing with the details of the UI, can you explain the current internal process?
What changes would be required in the internal process for this feature to work?

Good enough? This requires very little changes in the current code. A hook_queue_subscription() is probably all that's needed, so that a module can filter events before they are entered into the queue. Just about everything else can be handled externally.

There's plenty of data for administrative interfaces (moderation queue, etc.), and if anything else is needed, we can easily add a column somewhere.

Does this make sense? Have I missed anything?

***

Back to the UI:
The problem is that we need such a widget for every node and comment, which means there could be tens of them on a page, and if the same author has made multiple comments, then it would be nice to have some synchronization among that author's widgets. Reloading the page would automatically take care of that, but reloading the page for each and every comment just won't cut it. We need something better, and not just for D6.

Having all these widgets visible all of the time is not very attractive either, so if we could hide them behind a JS link (unless moderation is needed) and only pop them up on request, that would be really nice.

This could become an add-on module of its own or an optional component of the Subscriptions distribution. If you want to take a stab at it on your own we'll certainly help. Otherwise I can do the internal stuff, but I'd need you to do a usable JS UI. And I'd appreciate help with whatever administrative interface is needed.

P.S. I'll be away for a week starting Saturday and pretty busy in the week after that, so I won't be able to do much in the next few days, but I see the need to implement it and will work on it.

Hmm, i do not readily see the need for a new table, why not just add a hold flag to the subscriptions_queue table. The mail module could check that only hold = 0 mails are sent. A subscriptions_moderation module can update this flag accordingly. (subscriptions_content_node_load and subscriptions_content_comment_load tells us which entries are nodes / comments). We could add a module_invoke to subscriptions_queue calling subscriptions_moderation_queue which would conviniently deliver a 0 for us if the module is not on. This is backend and it's fairly easy. About the UI, no idea.

An alter like hook also works, pretty much the same as a dedicated module invoke.

Storing the HELD state in {subscriptions_queue} would mean that you can't display moderation controls on comments, because retrieving the state would be very inefficient. chx sees no need for that, just a moderation queue with a list of subjects and links to the nodes/comments. If that is sufficient, then we can save a lot of effort.

Since we have to add 'hold' to {subscriptions_queue} (default 0), we may as well add 'moderate' to {subscriptions_user} (again default 0). The defaults ensure that everything runs normally if subscriptions_moderate is not enabled.

The benefit of having a permission would be that core could be used for administering it when you initially set this up. Later on, having a column in {subscriptions_user} seems much simpler and more compact, but at some point you'll probably want to know, which of your users have auto-release, so the role-based approach has the advantage again.

Besides, the admin might choose to give this perm to a few other roles (maybe even most roles) from the start.

So I prefer the first approach, too.

You'll have to provide some instructions and suggest a good (short) name for the role, ideally right on the setting page, so that admins who go there will know what to do.

No problem, I'm not leaving. :-)

I see this more like

 function subscriptions_queue($event) {
   global $user;
   $event += array(
     'uid' => $user->uid,
     'load_args' => '',
   );
+  $moderate = module_invoke_all('subscriptions_moderation_queue', $event);
+  $moderate = !empty($moderate);
   if (is_array($event['load_args'])) {
    $event['load_args'] = serialize($event['load_args']);
   }

Then subscriptions_moderation or any other interested module can implement that hook and return a 1 to put on moderation the notifications that result from any event.

The rest will be pretty much as you prepared it (except that some of the AND moderate = 0's aren't needed).

I hope to have the next beta ready on Wednesday, including these changes (unless we decide something else, see below).

First, I misunderstood the meaning of the column 'author_uid' in {subscriptions_queue}. I thought I could use it to select all entries by the same author, but now I see that most of the time, the field has the value -1, and only the author's uid in specific circumstances.

'author_uid' in {subscriptions_queue} is just a copy of 'author_uid' in {subscriptions}, which is either -1 (for any author) or a uid (for author-specific subscriptions). You'll have to load the node or comment to get the author, but you need to do that anyway, don't you?

Also, now that I understand a bit more the module, I see that a single comment / node will create dozens or more rows in the table, one per recipient and subscription method.

Yes, that's part of what I tried to explain in #5.

I am currently left wondering:
A - how to sort the list, group the rows that correspond to the same piece of content, and then group the pieces of content per actual author.

Group by load_function and load_args. That's what subscriptions_mail() does to avoid sending duplicates. I think you're interested only in the 'subscriptions_content_node_load' and 'subscriptions_content_comment_load' load functions, and their load_args is nothing but nid and cid, respectively.

B- how to present this to the moderator (we are back to the UI problem discussed above. Knowing better the module, salvis foresaw the problem that I figure out only now).

I don't think it's that bad. You can create the moderation list directly from the nids and cids, or if you want to get fancy, you can group them by author uids.

So, we need to make a decission as this point:

C- Either carry on in this direction (see attached files), and find a way to go through the list of items to moderate in {subscriptions_queue} in a logical manner.

D- Or, decide that everything in {subscriptions_queue} is already pre-moderated (i.e. don't add the 'moderate' field). Thus, every new piece of content that requires moderation is first entered in different table. When the moderator goes through the items in this new table (better organized for this specific purpose), the {subscriptions_queue} is populated, i.e. hours after the fact.
I've got the feeling that this would be too complicated overall...

So far I see no obstacles to C. Until now we only deal with normal nodes and comments that can be fully assembled with 'subscriptions_content_node_load' and 'subscriptions_content_comment_load', and I assume that subscriptions_og will continue that tradition. I'm not sure what we may see in that area in the future, but we should be able to deal with it when it gets here.

D is also a possibility; this would mean snatching and serializing the event in hook_subscriptions_moderation_queue() and feeding it back into subscriptions_queue() upon approval.

Now that I think about it, I like this even better, because you'd be able to extract and store whatever information you want, right at the source (only for the events that do need moderation, of course), and Subscriptions would not be impacted at all, i.e. not have any held rows in {subscriptions_queue}.

We might even rename the hook to just hook_subscriptions_queue() — this would open up other interesting possibilities, including feeding back altered events immediately in a recursive call.

Or we might implement a hook_subscriptions_queue_alter() and allow unsetting the event, as chx suggested in #7. This might be more in line with expected behavior in Drupal.

What do you (or anyone) think of this?

After some thought I am siding with salvis on D. Let's do that. Less overhead for subscriptions itself.

The only thing special about an alter hook is that you take your parameter(s) by reference and you're allowed to change the object of interest. (It's an array in our case.) IOW, you define your hook function as

function subscriptions_moderation_subscriptions_queue_alter(&$event)

and you can change the $event as you like (as long as you don't break Subscriptions, of course). Other alter hooks might not like this, but here we specifically allow you to even set the $event = null, and Subscriptions will just forget about it.

So, you get an $event, which includes absolutely all the information that Subscriptions has (including $node and possibly $comment). If it does not need moderation, you just let it pass. Otherwise serialize($event) into a LONGTEXT database field, along with any other information that will be useful for the moderation queue, and set $event = null.

When the moderator releases the notification, then you simply call subscriptions_queue(unserialize($event)) and Subscriptions will never know what happened in between. You're completely free to implement whatever you want, and your only point of contact with the rest of Subscriptions is the $event array.

Obviously, when you call subscriptions_queue() yourself, your hook will also be called again, and you should let the $event pass this time. You could for example add an element like $event['subscriptions_moderation'] = 1, so that you can recognize the situation.

The next beta release will have hook_subscriptions_queue_alter(), but you can put it in yourself and start exploring this route right away:

function subscriptions_queue($event) {
  global $user;
  $event += array(
    'uid' => $user->uid,
    'load_args' => '',
  );
+  
+  foreach (module_implements('subscriptions_queue_alter') as $module) {
+    $function = $module .'_subscriptions_queue_alter';
+    $function($event);
+    if (empty($event)) {
+      return;  // $event was cleared, never mind.
+    }
+  }
+  
  if (is_array($event['load_args'])) {
    $event['load_args'] = serialize($event['load_args']);
  }

No other change is needed.

The alter hook is in BETA11.

@beginner: Have you had a chance to look at this?

function moderate_mailings_uninstall() {
  if (db_table_exists('subscriptions')) {
    db_query("DROP TABLE {subscriptions}");
  }
}

Lurkers: Don't try to install (and especially uninstall) this!!!

The moderate_mailings_uninstall() function will destroy your Subscriptions installation!

@beginner: Maybe you want to call it something like subscriptions_moderate, to make it clear that it's a Subscriptions add-on?

theme_comment_admin_overview()

brilliant - and delighted to see this. in an effort to make it even more user-centric and directly benefit from user activity, would it be possible to incorporate the d6 implementation of the "flag content" module such that any "flagged content" is automatically removed from any subscription output?

this would be a great way to allow active site users to police content violations (particular in school sites or youth-oriented sites) and make sure that no flagged content goes out to subscribers of a node/item until the item has been unflagged...

just a thought. turns users 'optionally' into part of the clean-up for the spam problem...

okay, i follow you. perhaps a feature request (for flag content - not subscriptions - would be to change usertype if flagged, basically revert the person to 'untrusted' until a flagged item is unflagged)

as for d6, just saw this: http://ftp.drupal.org/files/projects/subscriptions-6.x-1.x-dev.tar.gz (direct link - or go to modules, filter by d6)

I'm sorry about the confusion re: 6.x-1.x-dev. I was educated in http://drupal.org/node/231966 that the MAIN releases are supposed to be converted to -dev nodes and I needed this done for some other modules and thought it would be a good occasion to have subscriptions-MAIN converted, too, not realizing that this would change the versions tags here on the site...

There is no D6 version anywhere yet, not even on my local disk. Please ignore 6.x-1.x-dev, it's the same as the latest BETA for Drupal 5!

thanks salvis - i almost put it up and likely woulda been bombarding the support forums if it bugged! you may want to add that note above to the project page because when it showed up yesterday, probably tons of people downloaded it...

@beginner: Thank you for your work! I'm sorry this 6.x mess erupted in your thread...

I haven't had time to take a look yet, but I will. However, moderate_mailings.install.txt is still unchanged.

Lurkers: Don't try to install this yet — #27 will still trash Subscriptions when you uninstall it!!!

First of all, let's decide whether it's a generic or a content-only solution. Never forget that subscription is a generic framework, you might subscribe to private messages for example. This solution is currently content focused.

Second, all this fiddling with roles is not necessary. You provide a permission , check the permission with user_access and that's it. Let core handle the rest.

i also agree (with #39 right above) - seems that moderation should be a separate module and could possibly grow to support other areas of notification (like other site messaging tools, notifications etc)...or maybe i'm missing something...perhaps include with subscriptions project page as 'suggested other modules: moderate mailings' so that people will clearly see it, but then on moderate mailings list specific dependencies and so on...that way people can isolate the workflow around moderation and hammer it out independent of larger 'subscriptions' module issues.

I thought I have valid concerns -- I am not against this "bloody" thing, I just would like to see a few things fixed. I have re-read the code and the user-role is fine, after all, sorry -- you do check the permission with user_access as I asked where it's appropriate. On the same line, we have empty($event['moderate_mailings_done'] how is this supposed to work...? Who fills this key?

Next up, this is minor but you can see in for eg. watchdog that we use auto_increment / serial instead of db_next_id when we do not care about the value of the id. Speaking of serial, a postgresql install would be nice.

After you unserialize, you are using content instead of data, $content['node'] ought to be $data->node, $content['comment'] ought to be $data->comment.

There is concern that still stands: subscriptions is a generic framework, you might subscribe to private messages for example. This solution is currently content focused. It is fine that it only deals with content, we have subscriptions_content.module too. But, then this module needs to be renamed and some sorts of checking added so it won't try to process anything but content.

Thanks for the hard work.

Thank you for the fixes and for all your work.

I think limiting the module to content (nodes and comments) is just fine at this point, but I'll let chx answer the question on the best method to differentiate content from other subscription items.

It's hard to come up with an appropriate name — at first I thought, "Subscriptions" needed to be part of the name, but maybe not. I keep coming back to moderate_content_mailings ("Content Mail Moderation"). chx?

Is there a reason for not using l() in moderate_mailings_admin_settings_form()? The link is wrong.

Please add the dependency on subscriptions_content to the info file.

What's wrong with l()? I'm using it on admin/settings/subscriptions, and it works just fine.

What this module does? Moderates content mailings. Hence, moderate_content_mailing. That one is good. Checking for node/comment, just check whether $event['node'] exists.

One more thought — sorry this comes in pieces...

The menu item keeps bothering me a bit:

# ...
# Site information
# Site maintenance
# Subscriptions
# Subscriptions (spam deterrent moderation)

This is only used once in the entire lifetime of the site and it takes up a lot of menu real estate. Could it either become a submenu item under "Subscriptions" or a subtab on admin/settings/subscriptions? As long as we don't have any other add-ins competing for the same name, I'd be fine with having a subtab "Moderation" with a heavy weight.

However, this would mean changing the admin/settings/moderate_mailings path to admin/settings/subscriptions/moderation.

Ah, you've changed the menu item in #50, but it's even longer now:

# Moderate content mailing (spam deterrent moderation)

It takes four lines now. I don't think we can do that...

Here's a grammar question: my strings use the term "subscriptions notifications" (= multiple mailings caused by multiple subscriptions, two times plural form). You use "subscription mailings" — should the first word be plural or singular?

At this point, Subscriptions only supports mail as medium, but the design is such that subscriptions_mail.module could be replaced by some other module. That's why I'm using "notifications" rather than "mailings," and your module is independent of subscriptions_mail, so it should follow the same terminology.

This means the module should be renamed again, to moderate_content_notifications. I realize that you've only just renamed it and I should have noticed that before, so I'll do the renaming for you...

Here are the resulting files. We're planning to release RC1 tomorrow evening (European time) — let me know before then if you need any last-minute changes.

You can find it

• on admin/by-module
• at Administer ›› Site configuration ›› Subscriptions ›› Content notifications
• via the link in the message on admin/content/moderate_content_notifications

The links you mention are one-time-use indeed, but they're part of Drupal core and thus privileged. We want people who install Drupal for the first time to find all the Site configuration pages, and in a logical sequence.

However, I don't think us lowly contrib developers are supposed to litter the menu with monster entries...

moderate_content_notifications is included in RC1. Thank you, beginner!

I'd still like to get some feedback, if possible...

Ok, 2.0 is out and moderate_content_notifications along with it.

@beginner: Please keep an eye on the moderate_content_notifications subqueue.

Thanks again!