Autosubscribe feature not respecting user settings for send_updates and send_comments
rmjiv - February 12, 2009 - 17:55
| Project: | Subscriptions |
| Version: | 6.x-1.x-dev |
| Component: | Code |
| Category: | feature request |
| Priority: | normal |
| Assigned: | rmjiv |
| Status: | needs review |
Description
I think the subscriptions_autosubscribe function in subscriptions.module has a bug. On line 401 of subscriptions.module we have the line:
subscriptions_write_subscription($module, $field, $value, -1, $user->uid, _subscriptions_get_setting('send_interval', $user), 1, 1);The problem is the two hardcoded 1's at the end of the line for send_updates and send_comments. Rather than being hardcoded to TRUE, shouldn't these values respect the user's settings from the subscriptions_user table?
#1
I think not.
What's the use of auto-subscribing if you don't get updates and comments?
#2
I may want updates but not comments. Or in my case, I want comments but not updates.
Our site has a restricted vocabulary that a group of moderators uses to tag user posts. This causes the update node hook to run and consequently generate subscription notifications for everybody who has autosubscribed. Not what we want. And on our site, normal users can't update their original post anyway, so subscribing to updates has no real meaning.
We can configure subscriptions to work the way we want by setting the user defaults for send_updates and send_updates_visibility to off, but if the auto-subscribe doesn't respect the flag, there is no way of accomplishing what we want.
Now maybe if both updates and comments are off then I agree that auto-subscribing has no real meaning and I'd probably just not create the subscription.
#3
I'm sure we'd get complaints that auto-subscribe was broken. No, that's not an viable strategy.
What you're asking for is an administrative setting to reduce auto-subscribe to either just comments or just updates instead of comments and updates. I'll accept a patch...
#4
That kind of setting would work for me.
I'd be happy with any of the these options:
1. Your suggestion, "an administrative setting to reduce auto-subscribe to either just comments or just updates instead of comments and updates."
2a. An administrative setting to have auto-subscribe use the subscriptions_user settings, not creating the subscription if both updates and comments are turned off.
2b. An administrative setting to have auto-subscribe use the subscriptions_user settings, creating the subscription with existing behavior if both updates and comments are turned off.
3. An administrative setting to have auto-subscribe use the defaults for users. Administrative form would warn the administrator if they enabled "auto-subscribe" but had notifications for both updates and comments disabled.
Which would you prefer? I'd be willing to do the work, although it'd probably be a week or two before I could get around to it.
#5
Auto-subscribe should "just work" if it's turned on, which means send updates (if there are updates at all) and send comments (if there are comments at all). Often there may never be updates or never be comments, depending on the site.
A user may well want to turn off updates and comments, but still turn on auto-subscribe — get posts by others, but no comments/updates to posts by others, unless she participates in a thread (which would auto-subscribe her to receive further notifications). In fact, I think that's the most useful setting.
Your situation where you do updates that are not supposed to be notified, but no other updates, is the same as having no updates at all, except you need a way to suppress the useless update notifications.
So, 2a is useless and calls for support requests, 2b is too difficult to explain, 3 disables my "most useful setting." That leaves us with 1.
Great, please assign yourself.
#6
Alright, one more question and then I'll do the work ;)
What about instead of what we described, creating an administrative feature to set the default value of the $node->subscriptions_notify flag in subscriptions_content.module? A default to FALSE would solve my problem and wouldn't affect auto-subscribe at all. If useful, there could different defaults for insert vs. update.
Thoughts?
#7
Yes, that would be a useful feature, too, and it may well have wider applicability. The default would need to be subscriptions_notify ON, of course, to avoid causing a flood of support issues. Being able to set this separately for inserts and updates would be a must. You'd use only the latter, but another site may very well use Subscriptions only for selected posts.
I think we'd even need a third setting: for unpublished posts. Subscriptions hasn't sent out notifications for those yet, and when they are published, it sends out New notifications, even though publishing a node is technically an update.
#8
Okay, that's what I'll implement. Starting now.
#9
Patch attached. I haven't rolled many, so let me know if I did anything wrong.
#10
Great!!
I have pretty much the same problem. I like my users to be auto subscribed when they post content or a comment, but would like to disable subscriptions when a user corrects grammar in the content.
Will this be implemented in the next release?
Subscribing :-)
Thanks!
#11
Things are rarely as easy as they look...
I've massaged this closer to where I want it, including information about its limitations, but there are still open issues:
1. When inserting, hook_nodeapi('prepare') is called with $node->status==1, so we get the wrong variable. Did you test this?
2. I'm still doubtful about what happens, when a node is created unpublished because its content type is set that way (a) and subsequently updated to be published (b). Which variable is used?
#12
Thanks for the feedback and looking at this.
1. I tested it for updates, I guess I didn't test it for inserts, sorry. I think the test should be:
<?php
if ( ($node->status) && ($node->nid) ) {
$node->subscriptions_notify = (boolean) variable_get('subscriptions_default_send_node_update_notifications', 1);
}
else if ($node->status) {
$node->subscriptions_notify = (boolean) variable_get('subscriptions_default_send_node_insert_notifications', 1);
}
?>
2. I didn't really handle this case. I'm not sure there is a right answer with the lack of a "publish" action for the nodeapi hook it's impossible to tell if the status flag changed for the node. Does the subscriptions_taxonomy module attempt anything when a node is updated with taxonomy terms?
Your changes seem good, but I realized something when I was reading the NOTE on line 43 of the patch. If the $node->subscriptions_notify isn't set (i.e. the user that edited/inserted the node doesn't have "administer nodes" permission), I think the behavior should be to use the default value.
<?php
if (!isset($node->subscriptions_notify)) {
if ( ($node->status) || ($node->nid) ) {
$node->subscriptions_notify = (boolean) variable_get('subscriptions_default_send_node_update_notifications', 1);
}
else {
$node->subscriptions_notify = (boolean) variable_get('subscriptions_default_send_node_insert_notifications', 1);
}
}
if ($node->subscriptions_notify) {
subscriptions_queue($event);
}
?>
would replace the existing block that reads:
<?phpif (!isset($node->subscriptions_notify) || $node->subscriptions_notify) {
subscriptions_queue($event);
}
?>
Or at least a way of enabling this behavior.
Thoughts?
#13
This patch takes care of my problem --