Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#8508 closed enhancement (fixed)

Paginate messages and recipients in a thread

Reported by: espellcaste's profile espellcaste Owned by: espellcaste's profile espellcaste
Milestone: 10.0.0 Priority: high
Severity: normal Version: 1.0
Component: Messages Keywords: has-patch has-unit-tests dev-reviewed commit
Cc:

Description (last modified by espellcaste)

Currently, the BP_Messages_Thread class grabs all messages created for a thread. So if a thread has 1k messages, it tries to grab all of them and caches them, but there is no way to paginate those messages.

I suggest we refactor that part of the class to not cache the messages but to set a default number of messages, like 10, and add a parameter to paginate the rest of the messages.

Attachments (3)

8508-1.diff (22.8 KB) - added by espellcaste 3 years ago.
8508-2.diff (25.8 KB) - added by espellcaste 3 years ago.
8508.2.suggestion.patch (904 bytes) - added by imath 3 years ago.

Download all attachments as: .zip

Change History (22)

#1 @espellcaste
3 years ago

  • Description modified (diff)

#2 @espellcaste
3 years ago

  • Owner set to espellcaste
  • Status changed from assigned to accepted

#3 @espellcaste
3 years ago

  • Milestone changed from Awaiting Review to 9.0.0

#4 @imath
3 years ago

I agree it would be a nice improvement 👍

#5 @imath
3 years ago

  • Milestone changed from 9.0.0 to Up Next

We won't have time for 9.0.0 unfortunately, so let's work on it early for BP 10.0.0 :)

#6 @espellcaste
3 years ago

  • Milestone changed from Up Next to 10.0.0

#7 @espellcaste
3 years ago

  • Keywords needs-unit-tests added

#8 @espellcaste
3 years ago

  • Summary changed from Paginate messages in a thread to Paginate messages and recipients in a thread

#9 @espellcaste
3 years ago

  • Keywords has-patch added; needs-patch removed

@espellcaste
3 years ago

#10 @espellcaste
3 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.


3 years ago

#12 @imath
3 years ago

  • Keywords dev-reviewed added
  • Version changed from 8.0.0 to 1.0

Thanks a lot for your work about this improvement.

1) I'm unsure we should paginate the recipients list the way you did it as messages_check_thread_access() ends up getting all recipients of the thread to check the current user can access it. I believe paginating recipients using an array_slice( $recipients, $offset, $length ) of the cached list is a better option.
2) Is this the only step or will there be another one to add default values for the new arguments to the bp_thread_has_messages() function? It's not required (I believe) to pass these arguments, but it can help developers to save some time finding these 😉.
3) Small suggestion, I think the inline comment to explain why the messages are not cached would be better if it was phrased the other way around as you're checking ! empty(), eg: // If 'per_page' is not empty, this is a paginated request and messages should not be fetched from the cache.

Otherwise I confirm unit tests are Ok (thanks for building these 😍) and I haven't seen any regression into the BP Nouveau Messages UI.

#13 @espellcaste
3 years ago

@imath

1 - I reviewed other query classes and it seems almost all of them use bp_core_get_incremented_cache even when querying a paginated page. So your suggestion makes sense. I'll work on a update.

Just a note that the example you provided would get the recipients from cache instead of directly querying the database.

2 - Good catch. I'll update those with the new parameters.

3 - I'll update as suggested.

Tks for the feedback. Very useful! :)

@espellcaste
3 years ago

#14 @espellcaste
3 years ago

@imath Could you check again?

#15 @imath
3 years ago

Sure, I will do asap 👌

#16 @imath
3 years ago

  • Keywords commit added

@espellcaste 8508-2.diff looks good to me. After applying your patch, I just added an optional suggestion to save a foreach loop (see 8508.2.suggestion.patch). Thanks again for this improvement.

#17 @espellcaste
3 years ago

Thank you! Will proceed with the commit (including your patch). :)

#18 @espellcaste
3 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 13102:

Adding support to the offset pagination to Messages and Recipients in a thread.

Previously, the BP_Messages_Thread query class would grab ALL messages created for a thread and ALL its recipients. This commit introduces support to the offset pagination
allowing one to paginate both messages and recipients using parameters with a specific limit and offset.

Props imath
Fixes #8508

#19 @espellcaste
3 years ago

Related: #8597

Last edited 3 years ago by espellcaste (previous) (diff)
Note: See TracTickets for help on using tickets.