Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

"last uid" is not respected when range and list of user IDs is provided. #10

Open
wants to merge 1 commit into
base: 7.x-1.x
Choose a base branch
from
Open

"last uid" is not respected when range and list of user IDs is provided. #10

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion message_subscribe.module
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,16 @@ function message_subscribe_send_message($entity_type, $entity, Message $message,
if ($subscribe_options['uids']) {
// We got a list of user IDs directly from the implementing module,
// However we need to adhere to the range.
$uids = $subscribe_options['range'] ? array_slice($subscribe_options['uids'], 0, $subscribe_options['range'], TRUE) : $subscribe_options['uids'];
// If 'last uid' is provided, we need to start from next 'uid' key in array.
if (!empty($subscribe_options['last uid'])) {
$index = array_search($subscribe_options['last uid'], array_keys($subscribe_options['uids'])) + 1;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@capuleto
This is OK assuming last uid is in the uids array,
but if not,
then $index = 1 as default.

@amitaibu
Can we live with that assumption ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we live with that assumption ?

You'll have to check, and I don't remember :)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@capuleto
Can you please add a check that determine if the last uid is in the uids array?
If it's not then just use same fallback (set $index = 0;) and add this scenario to the docs.

If you can add a test for that scenario as well it can be awesome :)

@amitaibu
This should cover it.

}
// Otherwise we start from the very beginning.
else {
$index = 0;
}

$uids = $subscribe_options['range'] ? array_slice($subscribe_options['uids'], $index, $subscribe_options['range'], TRUE) : $subscribe_options['uids'];
}

if (empty($uids) && !$uids = message_subscribe_get_subscribers($entity_type, $entity, $message, $subscribe_options, $context)) {
Expand Down
45 changes: 44 additions & 1 deletion message_subscribe.test
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,4 +521,47 @@ class MessageSubscribeQueueTest extends DrupalWebTestCase {
$this->cronRun();
$this->assertEqual($queue->numberOfItems(), 0, 'Message item 2 processed by cron.');
}
}

/**
* Test that if we get a list of user IDs directly from
* the implementing module, the messages are sent respecting
* the range value.
*/
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();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR. can we avoid running cron, and invoke the needed function ourself?


$captured_emails = variable_get('drupal_test_email_collector', array());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would be nicer not to rely on capturing the email, and get the values more directly.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the way done in core to test functionality that sends email.
Do you have any suggestion?

$this->assertEqual(2, count($captured_emails), 'The amount of recipients is the same as amount of emails captured according to "subscribe_options".');

$recipients = array();
foreach ($captured_emails as $captured_email) {
$recipients[] = $captured_email['to'];
}

$this->assertFalse(in_array($user1->mail, $recipients), 'User 1 mail is not in the recipient list as it was defined as "last uid".');
$this->assertTrue(in_array($user2->mail, $recipients), 'User 2 mail is in the recipient list.');
$this->assertTrue(in_array($user3->mail, $recipients), 'User 3 mail is in the recipient list.');
}

}