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 ?

Files: 
CommentFileSizeAuthor
#52 email-notifiers-1828184.patch2.27 KBmpaler
#44 group-context-1828184-44.patch6.27 KBDevin Carlson
FAILED: [[SimpleTest]]: [MySQL] 57 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#43 demo.zip1.52 KBDevin Carlson
#43 acquia_drupal.sql_.gz303.89 KBDevin Carlson
#38 group-context-1828184-38.patch5.06 KBDevin Carlson
PASSED: [[SimpleTest]]: [MySQL] 55 pass(es).
[ View ]
#31 1828184-remove-email-default-notifier-31.patch5.11 KBAmitaibu
PASSED: [[SimpleTest]]: [MySQL] 45 pass(es).
[ View ]
#31 31-interdiff.txt4.51 KBAmitaibu
#29 1828184-remove-email-default-notifier-29-test-only.patch4.19 KBDevin Carlson
FAILED: [[SimpleTest]]: [MySQL] 41 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#28 Screenshot_7_30_13_5_10_PM.png186.62 KBjaperry
#24 1828184-remove-email-default-notifier-23.patch1.76 KBezra-g
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1828184-remove-email-default-notifier-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#22 Screenshot_7_26_13_2_29_PM.png219.57 KBjaperry
#16 email-notifiers-1828184-15.patch2.38 KBezra-g
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email-notifiers-1828184-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 email-notifiers-1828184-3.patch1.69 KBjide
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email-notifiers-1828184-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Title:Clarify the role of notifiersRemove "email" as default notifier
Component:Documentation» Code
Category:support» bug
Status:Active» Fixed

That was a bug. Fixed.

Status:Fixed» Needs work

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.

Status:Needs work» Needs review
StatusFileSize
new1.69 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email-notifiers-1828184-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

And here is a patch.

Status:Needs review» Fixed

Hmmmm, sorry, I did not get it first.

Status:Fixed» Needs work

In fact comment #2 still stands. User will always be notified if he has at least one "email_*" flag.

> But if a user has a random "email_*" flag on *something*

Indeed, the prefix name is hardcoded.

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.

Patches are welcome :)

Status:Needs work» Needs review

I have to test this again, but #3 may be right finally.

I confirm that the patch solves the problem.

I have applied the patch in #3 but I'm still getting messages, although the email flag is not set. Any ideas?

@Chipie: Are you sure no related entity is flagged ? Not the author, a term or the OG group for example ?

@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.

@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.

I tested #3 but it didn't prevent users without an email_node flagging on a node from receiving updates about that node.

StatusFileSize
new2.38 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch email-notifiers-1828184-15.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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_notifiers variable 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.

@ezra-g, any chance for #14 ? :)

#16 works for me!

#16: email-notifiers-1828184-15.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, email-notifiers-1828184-15.patch, failed testing.

Here is my current state with this issue.

Background
Commons once worked with message subscribe and message subscribe email. It would allow people to subscribe to issues without needing emails being sent. You checked the 'email' button, which would flag a piece of content for a user, and the email would send.

When this patch came out, it did work, but around Alpha 5, the api was changed, causing this patch to fail. Subsequent changes to message subscribe, including adding flag 3 support, has made porting this patch even harder. Commons 3 does not use flag 3 yet, and the flag 2 support that was added to message subscribe doesn't actually work.

Current debugging issues
Right now I can get the admin user to recieve an email without any patches with Alpha 5. I get flag errors with HEAD (7/25/2013) so I haven't been able to test that. The uids that get gathered only show one user, even if multiple users have the flag working.

Next steps
Devin is going to work on getting a virgin copy of message subscribe running without commons (flag 3), I'm working on continuing debugging and stepping through every user id (flag 2). Hopefully from there we can get a patch out, where Devin is writing tests to make sure it functions as described.

StatusFileSize
new219.57 KB

Further debugging is showing that regular users cannot access newly created nodes within a group because of an access denied error, caused by the following lines of code (alpha5):

<?php
 
foreach (module_implements('message_subscribe_get_subscribers') as $module) {
   
$function = $module . '_message_subscribe_get_subscribers';
   
dpm($function);
   
$result = $function($message, $subscribe_options, $context);
   
dpm("Result");
   
dpm($result);
    foreach (
$result as $uid => $values) {
      if (!empty(
$subscribe_options['entity access'])) {
       
$account = user_load($uid);
        if (!
entity_access('view', $entity_type, $entity, $account)) {
         
// User doesn't have access to view the entity.
         
dpm("USER $account->uid CANNOT VIEW ACCESS $entity_type");
         
dpm($entity);
          continue;
        }
      }
     
$uids[$uid] = $values;
    }
  }
 
dpm($values);
?>

Shows like below:

User with uid=4 can access the page fine, and when I comment as that user (or admin) on the new node (50), both get sent emails. When I disable the send emails flag, it still sends it.

So there are two issues here... First a node access issue which denies non-administrators from getting sent a message because entity_access returns false. Second, an issue within message subscribe where users are getting sent emails because message subscribe module doesn't really follow the 'notifiers' key, it just uses whatever uids are set within the array.

Status:Needs work» Needs review

Second, an issue within message subscribe where users are getting sent emails because message subscribe module doesn't really follow the 'notifiers' key, it just uses whatever uids are set within the array.

This sounds like the same cause described in #2:

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.

Great to see some agreement there :).

I had just enough time in this instance to do a blind re-roll of #16 to re-introduce the entity type context filtering to message_subscribe_email_message_subscribe_get_subscribers_alter() that was updated in #1819778: Support flag 3.x, but sadly not to do functional testing. However, since there's some consensus that this lack of context filtering may be the root cause of the issue here, I've attached for functional testing. Of course, a new simpletest would be even better.

For folks testing with Commons and therefore Flag 7.x-2x, we should check to see if #2052431: Undefined index errors with latest Flag stable 2.x has any effect on the functionality here.

Status:Needs review» Needs work
StatusFileSize
new1.76 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1828184-remove-email-default-notifier-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Now with actual patch. Note, this is actually "needs work" because it's missing the added conditions on the EFQ in message_subscribe_email_message_subscribe_get_subscribers_alter() introduced in #16.

Also, it definitely seems like we should postpone testing on this with Flag 7.x-2.x until we know #2052431: Undefined index errors with latest Flag stable 2.x, isn't affecting the functionality here (or, we could just test with Flag 7.x-2.x by temporarily rolling that issue back.

I'd still want to keep the email as default notifier. The bug seems to be around the fact that when you remove that option, it still send an email.

Title:Remove "email" as default notifierMessage Subscribe sends emails regardless of context

Retitling based on the causes quoted in 23.

The first issue (entity access) in #22 is being taken care of with #1918666: Entity Access support

StatusFileSize
new186.62 KB

so I got a few other issues like the flag and og_user_access issues fixed today but ended today with this fun bit.. the patch above definitely needs some work ;-)

Status:Needs work» Needs review
StatusFileSize
new4.19 KB
FAILED: [[SimpleTest]]: [MySQL] 41 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I haven't found a solution to the problem(s) mentioned in the issue (yet) but I have been able to duplicate them. To try and help anyone else who might be working on this, I've attached a test-only patch for Message subscribe email, based on the Message subscribe test, which:

  1. Creates two users
  2. Has both users place a subscribe flag on the same node
  3. Has each user place an email flag on separate entities: User #1 flags the node mentioned above and User 2 flags User 1
  4. Send a message to the node's subscribers

The test checks to see if the $uids returned from message_subscribe_get_subscribers() match the expected results (after removing email from the list of default notifiers) and that only one email is sent (to User 1).

Status:Needs review» Needs work

The last submitted patch, 1828184-remove-email-default-notifier-29-test-only.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new4.51 KB
new5.11 KB
PASSED: [[SimpleTest]]: [MySQL] 45 pass(es).
[ View ]

A nasty problem, is one that has several hidden bugs ;)

@Devin Carlson, forgot to mention kudos for providing a simpleTest!

Please confirm the fix, and I'll commit.

Status:Needs review» Reviewed & tested by the community

I tested #31 and can confirm that it fixes all of the issues that I ran into. :)

Status:Reviewed & tested by the community» Needs work

This still needs work. I'm fairly certain that #1 is not fixed. Is there a commit reference for the first comment?

Steps to reproduce:

2 Users:
Admin (has admin access)
UserA (authenticated user)
Both have Send emails by default enabled in message subscribe.

Create an open (Anyone can post) group as admin --> admin should get email, userA should not
Create a node inside group as UserA --> UserA should get email, admin should get email
Have userA follow group.
Go to UserA's notification settings. Turn off 'send emails by default', and uncheck Send Email for the group.
Create a node inside group as admin --> admin should get email, userA should not -- but does.
Create a comment inside the node admin just created --> admin should get email, userA should not -- but does.

This was tested using commons. I'm now retracing my steps with core D7 and Message Subscribe and OG.

Committed #31 as it fixed existing bugs.

Status:Needs work» Needs review
StatusFileSize
new5.06 KB
PASSED: [[SimpleTest]]: [MySQL] 55 pass(es).
[ View ]

Attached is a tests-only patch that should demonstrate the problem(s) that japerry encountered in #36 where users who follow a group and sign up for email notifications still receive emails after later opting out of email notifications.

It looks like the issue is caused by message_subscribe_email_message_subscribe_get_subscribers_alter() adding the email notifier to users who do not have the appropriate email_* flag.

I'm now investigating japerry's fix at http://drupal.org/node/2001702#comment-7664607.

Status:Needs review» Needs work

Note that you'll have to run the tests from #38 locally to see the fails as the tests require OG, which the testbot does not include (since it is not a dependency of Message Subscribe) and thus skips the tests.

I was trying to resolve the un-passing tests from #38, but I seem to be misunderstanding something:

+++ b/message_subscribe_email/message_subscribe_email.testundefined
@@ -138,6 +185,81 @@ class MessageSubscribeEmailSubscribersTest extends MessageSubscribeEmailTestHelp
+    flag('unflag', 'email_og', $this->node->nid, $this->user2);

What's being un-flagged is the group content node and not the group itself. Is that on purpose?

If the above is replaced with the following than the tests pass.
  flag('unflag', 'email_og', $this->group->nid, $this->user2);

@itamar Good catch! Unfortunately, I haven't had any more luck with duplicating the issue through tests.

I can manually duplicate the problem found in Commons by doing the follow:

Create a fresh install of D7 using the Standard profile and install Message Subscribe Email and OG.
Set the message_subscribe_default_notifiers variable to an empty array.
Create a new content type (ie "Group") and specify that it should be treated as a group by OG.
Create a new content type (ie "Post") and specify that it should be treated as group content by OG.
Enabling the subscribe_node, email_node, subscribe_og and email_og flags (*_node flags are assigned to the Post content type, *_og flags are assigned to the Group content type). Flags can have the "Display in entity links" option set for easy flagging through the UI.
Create a new message type (ie "test").
Implement hook_node_insert() somewhere (ie in a custom module or in one of the message_subscribe_* modules) and have it send a message to subscribers on node creation. For example:

<?php
/**
* Implements hook_node_insert().
*/
function example_node_insert($node) {
 
$message = message_create('test');
 
message_subscribe_send_message('node', $node, $message);
}
?>

As the administrator, create a new group.
Create a new user account.
Have the new user join the group and set their state to "active".
Have the administrator subscribe to the group and subscribe to emails for the group.
Have the new user subscribe to the group.
As the administrator, create a new Post (node) in the group.
Two emails are sent (one to the administrator and one to the new user) but only one should have been sent (to the administrator).

I'm not sure where the issue lies or if it is a configuration problem. I'll also try to cleanup and post the tests I've worked on.

@Devin Carlson
Thanks for the detailed explanation, but I still have troubles reproducing the problem; When I follow your steps, I get no emails sent at all.
Could you upload the database of the installation you described?

BTW, which version of flag did you use?

StatusFileSize
new303.89 KB
new1.52 KB

It sounds like it might be a configuration issue. I'm using Flag 7.x-3.0.

I attached a database dump of a demo site. You'll need the latest stable releases of all of the message subscribe email dependencies (ctools, entity, entity reference, flag, message, message notify, message subscribe, og, views and views bulk operations) and a demo module (that implements hook_node_insert()) which I also attached.

The demo site has two user accounts:

User name: Administrator
Password: admin

Username: Test
Password: test

You should be able to duplicate the problem by posting a new node inside of the only group on the site, which is named Group, and checking to see if only a single email was sent.

Note: I think drupal.org renames .sql files, so you'll probably need to rename the uploaded file's extension from .sql_.gz to .sql.gz.

StatusFileSize
new6.27 KB
FAILED: [[SimpleTest]]: [MySQL] 57 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

An updated patch to add a test which includes the change from #40. It also adds the second user to the group as described in #41 and adds a testing module which implements hook_node_insert().

I've also found a potential issue with Message on line 238 of message.message.inc where Message attempts to find the information for all field instances on the message type but it doesn't account for the case where the message type does not have a category. This will cause a couple of warning when running the tests which can be avoided by doing a if (!empty($message_type->category)) around the foreach();

Status:Needs work» Fixed

With the recent commits to this issue, we found that editing the Message Subscribe config to remove "email" from the default notifiers resolves the remaining issues here. Marking as fixed.

Status:Fixed» Closed (fixed)

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

3: email-notifiers-1828184-3.patch queued for re-testing.

The last submitted patch, 3: email-notifiers-1828184-3.patch, failed testing.

Issue summary:View changes

Can anyone explain why the patch in #3 isn't included in the current codebase? I can confirm that that patch fixed my problem with the latest release of Commons (7.x-3.9) where users were being emailed even though the email flag wasn't set. I have a related case open over in Commons. https://drupal.org/node/2210829

I could re-roll the patch if needed.

Status:Closed (fixed)» Needs review

Status:Needs review» Needs work

The last submitted patch, 44: group-context-1828184-44.patch, failed testing.

Version:7.x-1.x-dev» 7.x-1.0-rc1
StatusFileSize
new2.27 KB

Submitting a re-roll of #3 against 7.x-1.0-rc1.