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.

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

Fix messages being sent to wrong or nonexistent users when sending to a user group #13796

Closed
wants to merge 1 commit into
base: 2.x
from

Conversation

Projects
4 participants
@BobRay
Contributor

BobRay commented Feb 26, 2018

Fixes bug with Message Create processor using the wrong recipient ID when processing usergroup messages.

What does it do?

Modified the method to get the 'member' field of the modUserGroupMember object rather than the 'id' field.

Why is it needed?

Messages to user groups are sent to wrong or non-existent users.

Related issue(s)/PR(s)

Issue #13795

Fix Bug in MessageCreate processor
Fixes bug with Message Create processor using the wrong recipient ID when processing usergroup messages.
@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Feb 27, 2018

Thanks @BobRay - could you perhaps explain the reasoning behind how this change fixes the reported bug? Not sure if I'm immediately seeing it...

@BobRay

This comment has been minimized.

Contributor

BobRay commented Feb 27, 2018

Sure, it's confusing because of the variable names. Both $users and $primaryKey are misleading names.

`public function getRecipients($users, $primaryKey = 'id') {
        /** @var xPDOSimpleObject $user */
        $recipients = array();
        foreach ($users as $user) {
           if ($user->get($primaryKey) == $this->modx->user->get('id')) {
                 continue;
           }
           -  $recipients[] = $user->get('id');
           + $recipients[] = $user->get($primaryKey);
        }
       return $recipients;
 }

The purpose of the function is to assemble an array of user IDs to send a message to.

When sending to a usergroup, the $users variable holds, not users, but modUserGroupMember objects. In that case $primaryKey is set to 'member', which is the field that holds the member's user ID.

Hardcoding 'id' in that line adds the ID of the modUserGroupMember object (not the user) to the $recipients array, so the message is stored in the DB, but the intended recipient can't see it because it has the wrong User ID.

BTW, a sanity check to make sure the recipient exists would be a good idea too. Otherwise, over time you could amass lots of messages that can't be removed cluttering up the DB.

@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Mar 2, 2018

Thanks Bob!

@Mark-H Mark-H added this to the v2.6.2 milestone Mar 2, 2018

@Mark-H Mark-H changed the title from Fix Bug in MessageCreate processor to Fix messages being sent to wrong or nonexistent users when sending to a user group Mar 2, 2018

@Mark-H

This comment has been minimized.

Collaborator

Mark-H commented Mar 2, 2018

I've marked this for the 2.6.2 milestone as it's a clear bug fix, but as the PR targets 2.x, it will probably need to be cherry picked.

@BobRay

This comment has been minimized.

Contributor

BobRay commented Mar 2, 2018

Sorry, I can do another one for the 2.6.x branch if that won't mess things up.

@gpsietzema gpsietzema added this to Testing in MODX3 Mar 20, 2018

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 26, 2018

Cherry-picked into 2.6.x branch as commit 66589f7.

@opengeek opengeek self-assigned this Mar 26, 2018

@OptimusCrime

This comment has been minimized.

Contributor

OptimusCrime commented Mar 26, 2018

Can this be closed, @opengeek ?

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 26, 2018

No; it will get marked as merged when this gets pushed to 2.x later today.

@OptimusCrime

This comment has been minimized.

Contributor

OptimusCrime commented Mar 26, 2018

Ah, okey, sorry. I thought you merged 2.6.x into 2.x

@opengeek

This comment has been minimized.

Member

opengeek commented Mar 26, 2018

Or perhaps it won't get marked as merged. Sigh. Anyway, it was merged. Closing.

@opengeek opengeek closed this Mar 26, 2018

@gpsietzema gpsietzema moved this from Testing to Done in MODX3 Apr 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment