Message Subscribe sends emails regardless of context

That was a bug. Fixed.

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

Indeed, the prefix name is hardcoded.

Patches are welcome :)

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

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

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.

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.

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):

  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.

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.

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.

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

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 ;-)

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

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.

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

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.

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.

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:

/**
 * 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?

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.

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();

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.

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

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.

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

#52 looks better to me, but we're still not grabbing the email specific notifications. Thats a commons specific issue, we can tackle that over there.

Resubmitting this patch, it should apply correctly.

After extensive testing Baring Devin's work on the other tests in #2271317: Tests currently failing I think its safe to say that the patch in #55 fixes all of our issues.

Thanks japerry. Would you have time to add a simpleTest to prevent regressions?

Just a little warning !

In the patch (55) he use de column content_type and content_id in both flag API 2 and 1 but the column name have changed between these two versions. In the API 2 content_type is entity_type and content_id is entity_id.

here is my patch.

Hi folks, any chance someone writes a simpleTest for this?

Feel free to open a PR in the GitHub repo instead of here
#2362663: Move issue queue to Github

A reroll of #53 now that we don't have to support Flag 2.x anymore.

For D7 this seems to be a duplicate of #2126689: Email Notifications sent regardless of email checkboxes, revisited. bug in message_subscribe_email and for D8 it has been committed in this issue.

Nevermind. It appears this patch is still useful, since the patch in the related issue only works if two flags are created. If the module Message Subscribe Email Frequency is enabled, only one flag would normally be necessary.