#8508 closed enhancement (fixed)
Paginate messages and recipients in a thread
Reported by: | espellcaste | Owned by: | 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 )
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)
Change History (22)
#5
@
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 :)
#8
@
3 years ago
- Summary changed from Paginate messages in a thread to Paginate messages and recipients in a thread
This ticket was mentioned in Slack in #buddypress by espellcaste. View the logs.
3 years ago
#12
@
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
@
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! :)
#16
@
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.
I agree it would be a nice improvement 👍