Unread count not set for message threads

[sjregan]

When a new Message Thread is populated within the populate() method, the unread_count property is not set.

This is because the array_slice() function added for ticket #8508 is missing the preserve_keys parameter.
​https://github.com/buddypress/buddypress/blob/master/src/bp-messages/classes/class-bp-messages-thread.php#L317

I'll make a pull request tomorrow.

[imath]

Hi @sjregan

Thanks for your report, I'll be happy to take a look at your PR 👍

[sjregan]

Unfortunately this one is a bit more complicated than I first thought and would appreciated some advice on how you would resolve this.

When a user is viewing their inbox, the BP_Messages_Box_Template class is used and BP_Messages_Thread::get_current_threads_for_user() is called:

​https://github.com/buddypress/buddypress/blob/master/src/bp-messages/classes/class-bp-messages-box-template.php#L173-L175

$threads = BP_Messages_Thread::get_current_threads_for_user(
    array(
        'user_id'             => $this->user_id,
        'box'                 => $this->box,
        'type'                => $this->type,
        'limit'               => $this->pag_num,
        'page'                => $this->pag_page,
        'search_terms'        => $this->search_terms,
        'meta_query'          => $r['meta_query'],
        'recipients_page'     => $r['recipients_page'],
        'recipients_per_page' => $r['recipients_per_page'],
        'messages_page'       => $r['messages_page'],
        'messages_per_page'   => $r['messages_per_page'],
    )
);

This passes the 'user_id' however, when fetching the threads themselves the 'user_id' is not passed on:

​https://github.com/buddypress/buddypress/blob/e0f5aaa4746fbbd9bee36fa810eaf67e103f1013/src/bp-messages/classes/class-bp-messages-thread.php#L755-L765

$threads[] = new BP_Messages_Thread(
    $thread_id,
    'ASC',
    array(
        'update_meta_cache'   => false,
        'recipients_page'     => $r['recipients_page'],
        'recipients_per_page' => $r['recipients_per_page'],
        'page'                => $r['messages_page'],
        'per_page'            => $r['messages_per_page'],
    )
);

I can add the 'user_id' to the arguments array and the current unit tests continue to pass.

The 'user_id' is required because of this:

​https://github.com/buddypress/buddypress/blob/master/src/bp-messages/classes/class-bp-messages-thread.php#L207-L209

if ( isset( $this->recipients[ $r['user_id'] ] ) ) {
    $this->unread_count = $this->recipients[ $r['user_id'] ]->unread_count;
}

This is where the little complication comes in, the get_recipients() method will slice the recipient array based on the 'recipients_per_page' and 'recipients_page' arguments:

​https://github.com/buddypress/buddypress/blob/master/src/bp-messages/classes/class-bp-messages-thread.php#L317

I can fix the array_slice() call to preserve the array keys, but there is a chance our user with ID 'user_id' has been sliced out of the array, so the unread_count will not be set. Also, the 'bp_messages_thread_get_recipients' filter results may have lost the recipient.

So when setting the unread count, we cannot rely on the recipient still being present within the array after the get_recipients() call.

The only solution I can think of is when the recipient is not present in the array, check the 'thread_recipients' cache for them, and if they are not there, query the database again.

We could cache/store the value within get_recipients() for retrieval after calling it, but that seems messy.

Any other suggestions?

This fixes the unread_count property not always being set on message thread object when getting message threads for a user, particularly when 'recipients_per_page' and 'recipients_page' are provided within arguments.

• BP_Tests_Messages_Template has been updated to catch potential failure.
• The user_id from arguments to \BP_Messages_Thread::get_current_threads_for_user() is now passed to new BP_Messages_Thread() for use when populating the thread.
• \BP_Messages_Thread::get_unread_count() method added and used within populate()

Trac ticket: ​https://buddypress.trac.wordpress.org/ticket/8750

[imath]

Hi @sjregan

Thanks a lot for your PR and sorry I haven't seen you question before. I must say, at first sight it's complex for me to get it. I'll try to create a unit test that simulate the issue you're reporting.

[imath]

Ok I got it, if you look at ​https://github.com/buddypress/buddypress/blob/e0f5aaa4746fbbd9bee36fa810eaf67e103f1013/src/bp-messages/classes/class-bp-messages-thread.php#L53 You'll see that the unread_count is designed to inform about the logged in user's unread count. It's set there ​https://github.com/buddypress/buddypress/blob/e0f5aaa4746fbbd9bee36fa810eaf67e103f1013/src/bp-messages/classes/class-bp-messages-thread.php#L163|L172

So I believe we should only try to set the unread_count into the BP_Messages_Thread->get_recipients() method if the user_id argument is sent to it (via $args param) which means it comes from the BP_Messages_Thread->populate() method.

While you're right about the fact preserving keys when using array_slice() was needed when this unread count was being set into BP_Messages_Thread->populate() (which means it's a regression we introduced in 10.0), I believe it might not be needed anymore 😅.

I've added a unit test to show the issue.

[sjregan]

Hi @imath, thanks for the response and the patch.

There's a few of things to note:

1. The BP_Messages_Thread::populate() method accepts $user_id in the arguments but defaults to the displayed or logged in user.

​https://github.com/buddypress/buddypress/blob/master/src/bp-messages/classes/class-bp-messages-thread.php#L149

2. The BP_Messages_Box_Template class also accepts the $user_id through its arguments and passes it to BP_Messages_Thread::get_current_threads_for_user(), however, BP_Messages_Thread::get_current_threads_for_user() does NOT pass on the $user_id to new BP_Messages_Thread().

​https://github.com/buddypress/buddypress/blob/master/src/bp-messages/classes/class-bp-messages-box-template.php#L146-L175

So, if you provide BP_Messages_Thread::get_current_threads_for_user() with a $user_id, you will not get the unread_count for that $user_id. You will get an unread count, just not the correct one for the $user_id you provided.

This is evident when using the REST API:
​https://github.com/buddypress/BP-REST/blob/master/includes/bp-messages/classes/class-bp-rest-messages-endpoint.php#L123

My PR included a test that captured this scenario and provided a fix. Here is a single commit containing just the test so you can apply it your patch and see the issue is not fully resolved:

​https://github.com/sjregan/buddypress/commit/0c930bf9dcd94dda72bdc13e9e83cfb3a92c6a3c

3. BP_Messages_Thread::get_recipients() includes the 'bp_messages_thread_get_recipients' filter. So even with the array_slice() issue fixed (and the missing $user_id parameter to the BP_Messages_Thread constructor), you cannot rely on the results of get_recipients() always including the $user_id recipient.

[imath]

@sjregan Sure, except that I'm setting the thread's unreat_count property before the array is sliced. In ​8750.2.patch I've just added another test for this scenario and included the user_id argument in ​https://github.com/buddypress/buddypress/blob/e0f5aaa4746fbbd9bee36fa810eaf67e103f1013/src/bp-messages/classes/class-bp-messages-thread.php#L758-L764

[sjregan]

@imath Ahh yes, my apologies. Thank you.

[imath]

In 13339:

BP Messages: make sure Threads unread count is consistent

[13102] introduced a regression considering the way we used to set a thread's unread count. This count is set using the list of recipient objects (which include an unread_count property for each user object) by checking for the requested user ID into this list to find the corresponding unread count.

Slicing this list into smaller parts to paginate recipients results introduced potential cases where the requested user ID wasn't found into this paginated list of recipients.

Before doing the slice operation, we need to set the requested user ID thread's unread count to be sure it's the right one and not a null value.

Props sjregan

Closes ​https://github.com/buddypress/buddypress/pull/26
See #8750 (trunk)

[imath]

In 13340:

BP Messages: make sure Threads unread count is consistent

[13102] introduced a regression considering the way we used to set a thread's unread count. This count is set using the list of recipient objects (which include an unread_count property for each user object) by checking for the requested user ID into this list to find the corresponding unread count.

Slicing this list into smaller parts to paginate recipients results introduced potential cases where the requested user ID wasn't found into this paginated list of recipients.

Before doing the slice operation, we need to set the requested user ID thread's unread count to be sure it's the right one and not a null value.

Props sjregan

Fixes #8750 (branch 10.0)