Message subscribe does not use "last uid" at all when the list of user IDs is provided and range is enabled in subscribe options.

CommentFileSizeAuthor
#28 2256899-27-message-subscribe-last-uid-range-user-ids-provided.patch3.54 KBcapuleto
#26 only-tests-2256899-13-message-subscribe-last-uid-range-user-ids-provided.patch1.95 KBcapuleto
#13 interdiff.txt1.52 KBcapuleto
#13 2256899-13-message-subscribe-last-uid-range-user-ids-provided.patch3.54 KBcapuleto
#5 interdiff-2256899-3-5.txt1.83 KBcapuleto
#5 2256899-5-message-subscribe-last-uid-range-user-ids-provided.patch3.02 KBcapuleto
#3 interdiff-2256899-1-3.txt1.89 KBcapuleto
#3 2256899-3-message-subscribe-last-uid-range-user-ids-provided.patch3.06 KBcapuleto
#1 2256899-message-subscribe-last-uid-range-user-ids-provided.patch1.17 KBcapuleto
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

capuleto’s picture

capuleto CreditAttribution: capuleto commented
FileSize
2256899-message-subscribe-last-uid-range-user-ids-provided.patch1.17 KB
amitaibu’s picture

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

Thanks, Setting correct status.
Can you add a simpletest to this as-well?

capuleto’s picture

capuleto CreditAttribution: capuleto commented
FileSize
2256899-3-message-subscribe-last-uid-range-user-ids-provided.patch3.06 KB
interdiff-2256899-1-3.txt1.89 KB

Thanks for the feedback.
I've just added a test for the issue.

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented 
  1. +++ b/message_subscribe.test
@@ -471,4 +471,47 @@ class MessageSubscribeQueueTest extends DrupalWebTestCase {
+   * Test that if we get a list of user IDs directly from the implementing module,

    Don't pass to 80 line characters. 

  2. +++ b/message_subscribe.test
@@ -471,4 +471,47 @@ class MessageSubscribeQueueTest extends DrupalWebTestCase {
+   * @see https://drupal.org/node/2256899

    No need for the issue URL. 

  3. +++ b/message_subscribe.test
@@ -471,4 +471,47 @@ class MessageSubscribeQueueTest extends DrupalWebTestCase {
+    $this->assertTrue(in_array($user1->mail, $recipients), 'user1 mail is in the recipient list');

    user1 => User 1

  4. +++ b/message_subscribe.test
@@ -471,4 +471,47 @@ class MessageSubscribeQueueTest extends DrupalWebTestCase {
+    $this->assertTrue(in_array($user2->mail, $recipients), 'user2 mail is in the recipient list');

    Forgot a dot(.) in the end.

capuleto’s picture

capuleto CreditAttribution: capuleto commented
FileSize
2256899-5-message-subscribe-last-uid-range-user-ids-provided.patch3.02 KB
interdiff-2256899-3-5.txt1.83 KB

Applied suggested changes.

capuleto’s picture

capuleto CreditAttribution: capuleto commented
capuleto’s picture

capuleto CreditAttribution: capuleto commented
capuleto’s picture

capuleto CreditAttribution: capuleto commented

@RoySegall.. Is it the last patch ok?

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented

@capuleto I'll try to have a look on this tomorrow and run the simple test on my local computer due to the infinity queued of the simpletest(#2255301: Simple test are queued for ever.)

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented

I don't see any use of the last UID in the simple test when passing the options to the message_subscribe_send_message.

RoySegall’s picture

RoySegall CreditAttribution: RoySegall commented
Status: Needs review » Needs work
capuleto’s picture

capuleto CreditAttribution: capuleto commented

@RoySegall You could run the tests without applying the patch to "message_subscribe_send_message".. and you will see that " testProvidedUserIdsAreSplitAccordingToRangeValue" fires more than the 3 expected e-mails as the issue is the internal use of "last uid" when the list of user ids are provided along with range.

capuleto’s picture

capuleto CreditAttribution: capuleto commented
Status: Needs work » Needs review
FileSize
2256899-13-message-subscribe-last-uid-range-user-ids-provided.patch3.54 KB
interdiff.txt1.52 KB

@RoySegall I've updated the test to include "last uid" to subscribe_options array.

rjacobs’s picture

rjacobs CreditAttribution: rjacobs commented

Would someone be able to provide a bit more context for this issue? What are the related symptoms? What I can gather (via a scan of the code) is that this bug leads to an incorrect uid being associated with the first message of a batch/range whenever that range of uids to process in exceeded within message_subscribe_send_message()? Does this mean that this bug is only an issue when the queue is used (i.e. message_subscribe_use_queue = TRUE)?

We have uncovered a case where some subscribers seem to get the incorrect mid associated with some of their notifications, and I'm trying to see if it may be linked to this issue. For example, we may have a user subscribed to 5 entities, and an event takes place that simultaneously triggers a message associated with all 5 of those entities. In this case the 5 messages will be generated correctly (each with a distinct mid and related field data) and the user will receive 5 notifications (as expected). The problem is that the 5 notifications received are not distinct, and several appear in duplicate (e.g. we would expect 5 notifications each associated with one of the 5 messages, but instead we see 2 dup notifications associated with message 1, and 3 dup notifications associated with message 5). In our instance message_subscribe_use_queue is set to FALSE.

So this is clearly representative of some kind of mismatch between mid and uid data processed in message_subscribe_send_message(), and passed to message_notify_send_message(), but it's really hard to tell if it could be tied to this issue or something else entirely.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Is this relared? #2299083: Users receive duplicate notifications when queue is enabled

capuleto’s picture

capuleto CreditAttribution: capuleto commented

Hi @Amitaibu and @rjacobs.
This issue is covered by the attached test

+++ b/message_subscribe.test
@@ -471,4 +471,47 @@ class MessageSubscribeQueueTest extends DrupalWebTestCase {
+  function testProvidedUserIdsAreSplitAccordingToRangeValue() {
+
+    $user1 = $this->drupalCreateUser();
+    $user2 = $this->drupalCreateUser();
+    $user3 = $this->drupalCreateUser();
+
+    $message = message_create('foo', array());
+
+    $subscribe_options = array(
+      'uids' => array(
+        $user1->uid => array('notifiers' => array('email')),
+        $user2->uid => array('notifiers' => array('email')),
+        $user3->uid => array('notifiers' => array('email')),
+      ),
+      'skip context' => TRUE,
+      'range' => 1,
+      'last uid' => $user1->uid,
+    );
+
+    message_subscribe_send_message('node', $this->node, $message, array(), $subscribe_options);
+
+    // Run cron.
+    $this->cronRun();

We found out that if we provide the uids and range in the subscribe_options array, the queue job didn't respect the "last uid" stored internally.
Meaning that the subsequent job queues will start from the very fist element of the "uids" array if the current job did not manage to send to all provided users.
In our case it was sending the same email several times to the same user, but every message sent was triggered by a different job queue.

The last submitted patch, 1: 2256899-message-subscribe-last-uid-range-user-ids-provided.patch, failed testing.

The last submitted patch, 3: 2256899-3-message-subscribe-last-uid-range-user-ids-provided.patch, failed testing.

The last submitted patch, 5: 2256899-5-message-subscribe-last-uid-range-user-ids-provided.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2256899-13-message-subscribe-last-uid-range-user-ids-provided.patch, failed testing.

capuleto queued 13: 2256899-13-message-subscribe-last-uid-range-user-ids-provided.patch for re-testing.

capuleto’s picture

capuleto CreditAttribution: capuleto commented
Status: Needs work » Needs review

@Amitaibu and @RoySegall.. the testbot seems to be able to run the tests..
I've re-queued the latest one..

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

@capuleto, is this related #2299083: Users receive duplicate notifications when queue is enabled?

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

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

capuleto queued 13: 2256899-13-message-subscribe-last-uid-range-user-ids-provided.patch for re-testing.

capuleto’s picture

capuleto CreditAttribution: capuleto at Eksponent commented
FileSize
only-tests-2256899-13-message-subscribe-last-uid-range-user-ids-provided.patch1.95 KB

Only tests patch.

Status: Needs review » Needs work

The last submitted patch, 26: only-tests-2256899-13-message-subscribe-last-uid-range-user-ids-provided.patch, failed testing.

capuleto’s picture

capuleto CreditAttribution: capuleto at Eksponent commented
FileSize
2256899-27-message-subscribe-last-uid-range-user-ids-provided.patch3.54 KB

Patch and tests.

capuleto’s picture

capuleto CreditAttribution: capuleto at Eksponent commented
Status: Needs work » Needs review

Trigger tests.

oturpin’s picture

oturpin CreditAttribution: oturpin commented

Hi All,

Got a question on this patch: Would it be possible that the $index equals the last member of the uids array ? (ie the former loop over the uids reached the last member of array having just "range" members)
In that case, the queue would be re-initialized with the full list of uids from the message_subscribe_get_subscribers ?

Thx for feedback

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

Merged, thanks.

  • amitaibu committed 34c74c9 on 7.x-1.x authored by capuleto 
    Issue #2256899 by capuleto: "last uid" is not respected when range and...
amitaibu’s picture

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

Status: Fixed » Closed (fixed)

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

jelo’s picture

jelo CreditAttribution: jelo commented

I realize that this issue is closed, but for anyone looking for solutions to the duplicate message issue it may be helpful to know that this patch does not seem to fix the problem. amitaibu asked several times if this issue is related to #2299083. Even with this patch implemented I am still experiencing the problem that a number of users receive multiple duplicate notifications when queue is enabled... Just had a case where the notification was sent 7 times within 15 minutes. So, the two issues seem to be distinct.