This is related to the new feature from #2088441: Don't send messages to the author.

The logic check in message_subscribe_get_subscribers() is incorrect in relation to the text on the UI.

Problem description
The text of the checkbox in the UI indicates that you should check the checkbox in order for message authors to be notified of their own post. However the check in message_subscribe_get_subscribers() is set to skip the user if the checkbox is checked.

The simpletests for this feature also have the logic backwards in that they are setting the notify_own_actions to TRUE when checking for an exclusion and setting to FALSE when checking for inclusion.

Possible Solutions
We can either adjust the text on the UI to say "Do not notify author of their own submissions" or we can alter the logic in the module and the simple tests.

If we reverse the logic (line 217 of message_subscribe.module), then the following lines in the simpletests will also need to be updated to account for the default state being that users are not notified of their own submissions (due to the tests assuming the default is that users get notified of their own actions):

  • MessageSubscribeSubscribersTest->testGetSubscribers() Line 256
  • MessageSubscribeSubscribersTest->testGetSubscribersExcludeSelf() Line 308
  • MessageSubscribeSubscribersTest->testGetSubscribersExcludeSelf() Line 331
  • MessageSubscribeSubscribersTest->testEntityAccess() Line 353
  • MessageSubscribeSubscribersTest->testEntityAccess() Line 357

I'm happy to submit a patch using either approach (change the UI or change the logic).

CommentFileSizeAuthor
#18 message_subscribe-notify_own_actions-2149181-18.patch5.36 KBRoySegall
#18 innerdiff-18.txt1.54 KBRoySegall
#13 message_subscribe-notify_own_actions-2149181-13.patch3.8 KBRoySegall
#4 message_subscribe-notify_own_actions-2149181-4.patch4.08 KBshawn_smiley
#2 message_subscribe-notify_own_actions-2149181-2.patch2.94 KBshawn_smiley
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Yes, let's fix the logic in code, following the logic of the variable name -- message_subscribe_notify_own_actions

shawn_smiley’s picture

shawn_smiley CreditAttribution: shawn_smiley commented
Status: Active » Needs review
FileSize
message_subscribe-notify_own_actions-2149181-2.patch2.94 KB

Attached is a patch which resolves the logic issue.

Changes included in this patch:

  • Changed the check in message_subscribe.module to skip the user if the notify_own_actions checkbox is not checked
  • Updated the simple tests to account for the default state being that users are not notified of their own actions.

Status: Needs review » Needs work

The last submitted patch, 2: message_subscribe-notify_own_actions-2149181-2.patch, failed testing.

shawn_smiley’s picture

shawn_smiley CreditAttribution: shawn_smiley commented
Status: Needs work » Needs review
FileSize
message_subscribe-notify_own_actions-2149181-4.patch4.08 KB

Updated the previous patch to include adjustments to the message_subscribe_email simpletests.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Thanks, @RoySegall will review it.

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented

I tested the patch and it's working as expected.

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented
Status: Needs review » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)

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

loze’s picture

loze CreditAttribution: loze commented

There seems to still be an error in the logic.

Shouldn't we be checking the uid of the message and not the subscribed entity?

if (!$notify_own_actions && $message->uid == $uid) {

instead of

if (!$notify_own_actions && $entity->uid == $uid) {

this makes it work as I would expect, however I may be interpreting the intended behavior wrongly.

I would think that we want the behavior to be: don't send an notification to a user that triggered the generating of that message.

not: don't send a notification to the author of the subscribed entity.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

@RoySegall, also I've noticed that the property name, unlike the other properties, is with underscores -- so please fix that as-well.

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented

@loze - the user of the entity and the user that trigger the message isn't the same one? Even when the admin created node on the behalf of another user the message will create for that user.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

I think we can rename the property to notify message owner, so it will be more clear, and in that case check $message->uid

makes sense?

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented
Status: Closed (fixed) » Needs review
FileSize
message_subscribe-notify_own_actions-2149181-13.patch3.8 KB

I added a patch with the renaming.

Status: Needs review » Needs work

The last submitted patch, 13: message_subscribe-notify_own_actions-2149181-13.patch, failed testing.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Lets get it fixed, so we can roll RC2, and then do a full release...

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented

It seems that OG is installed during the simple test. Even using module_enable(array('og'))

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented

Update: when using drupal_static_reset(); in the top of the function the test passed.

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented
Status: Needs work » Needs review
FileSize
innerdiff-18.txt1.54 KB
message_subscribe-notify_own_actions-2149181-18.patch5.36 KB

For some reason the problem with OG solved and i managed to pass the tests on my local machine. Let's see the tests.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented
Status: Needs review » Fixed

Committed, thanks.

Status: Fixed » Closed (fixed)

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