Posted by jide on October 31, 2012 at 3:39pm
5 followers
| Project: | Message-subscribe |
| Version: | 7.x-1.x-dev |
| Component: | Code |
| Category: | bug report |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
Issue Summary
I fail to understand the role of "notifiers" in message_subscribe_email and how they interact with the way messages are sent.
From what I've seen, message_subscribe_process_message always sends emails, whether there is an "email" notifier in the $uids options or not.
Maybe some (pseudo) code would help :
/**
* Implements hook_entity_insert().
*/
function coopnet_subscribe_entity_insert($entity, $type) {
//...
$message = message_create('coopnet_notifications_message', array('arguments' => $arguments), user_load($entity->uid));
message_subscribe_process_message($type, $entity, $message, array('email' => array()), array('save message' => FALSE), $context);
}Although the user 4003 has not flagged my content with an "email_*" flag related to my content, email is sent anyway.
In message_subscribe_message_subscribe_get_subscribers, the $uids array looks like this :
array (
//...
3521 =>
array (
'notifiers' =>
array (
'email' => 'email',
),
'flags' =>
array (
0 => 'follow_user',
),
),
4003 =>
array (
'notifiers' =>
array (
),
'flags' =>
array (
0 => 'follow_user',
),
),
)But user 4003 is sent an email.
Am I missing something here ?
Comments
#1
That was a bug. Fixed.
#2
There is still a problem with message_subscribe_email_message_subscribe_get_subscribers_alter though :
/**
* Implements hook_message_subscribe_get_subscribers_alter().
*
* Filters out subscribes to show only email subscribers.
*/
function message_subscribe_email_message_subscribe_get_subscribers_alter(&$uids, $values) {
if (empty($uids)) {
// Nobody is subscribed to the content.
return;
}
$flags = message_subscribe_email_flag_get_flags();
$flag_ids = array();
foreach ($flags as $flag) {
$flag_ids[] = $flag->fid;
}
$result = db_select('flag_content', 'f')
->fields('f', array('uid'))
->condition('fid', $flag_ids, 'IN')
->condition('uid', array_keys($uids), 'IN')
->groupBy('uid')
->execute()
->fetchAll();
foreach ($result as $row) {
// Add 'email' to the list of notifiers.
$uids[$row->uid]['notifiers']['email'] = 'email';
}
}
This means :
"Get all "email_*" flags, then query all users that are in $uids and have at least one "email_*" flag, and add the notifier key to these uids."
But if a user has a random "email_*" flag on *something*, they will have the email notifier, whether the entities in context are concerned or not.
#3
And here is a patch.
#4
Hmmmm, sorry, I did not get it first.
#5
In fact comment #2 still stands. User will always be notified if he has at least one "email_*" flag.
#6
> But if a user has a random "email_*" flag on *something*
Indeed, the prefix name is hardcoded.
#7
That's not the problem here :)
Code in message_subscribe_email_message_subscribe_get_subscribers_alter() is wrong, any user who subscribed via a subscribe flag (and consequently is in $uids array) AND has *any* email flag will be notified, which we don't want.
#8
Patches are welcome :)
#9
I have to test this again, but #3 may be right finally.
#10
I confirm that the patch solves the problem.
#11
I have applied the patch in #3 but I'm still getting messages, although the email flag is not set. Any ideas?
#12
@Chipie: Are you sure no related entity is flagged ? Not the author, a term or the OG group for example ?
#13
@Jide: The entity is flagged:
array (86 =>
array (
'notifiers' =>
array (
'email' => 'email',
),
'flags' =>
array (
0 => 'commons_follow_node',
),
),
)
But the flag is not an "email*" flag. If I delete $uids before and only add the uids with "emails*" flags with your patch, it works as expected.
#14
@jide,
I'd love it if you could add a simpletest, so we make sure there will not be a regression. You can look at the existing tests for examples.
#15
I tested #3 but it didn't prevent users without an email_node flagging on a node from receiving updates about that node.
#16
Here's a revised patch. It takes into account the EFQ changes from #3, but also changes the default value of the
message_subscribe_default_notifiersvariable to an empty array.I believe that was the intention of the issue per the title and commit from comment #1, but apparently email is still set as the default notifier in the latest from the 7.x-1.x branch.
I tested by having one user with a subscription on a node and toggling the email subscription of another user, and emails were/were not sent in the appropriate cases in my testing.
#17
@ezra-g, any chance for #14 ? :)
#18
#16 works for me!