Skip to:
Content

BuddyPress.org

Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#8697 closed enhancement (fixed)

BP REST API Messages endpoint - total messages per thread

Reported by: niftythree's profile niftythree Owned by: espellcaste's profile espellcaste
Milestone: 12.0.0 Priority: normal
Severity: minor Version:
Component: REST API Keywords: dev-reviewed has-patch has-unit-tests
Cc:

Description

Hi there,

We've noticed that currently there is no easy way to get the total number of individual messages per message thread when accessing messages from the BP REST API "messages" endpoint (as far as we are aware). In order to get the total count, you have to repeatedly call the endpoint and add up the number of messages returned for each thread. Because we're trying to access the API via a mobile app, this really slows down the message loading process. Would it possible to put in a total message count field as part of the message thread data returned by the API?

Thanks!

Attachments (2)

Screenshot 1.jpg (298.8 KB) - added by niftythree 3 years ago.
Screenshot 2.png (136.6 KB) - added by niftythree 3 years ago.

Download all attachments as: .zip

Change History (17)

#1 @espellcaste
3 years ago

  • Milestone changed from Awaiting Review to Under Consideration
  • Priority changed from normal to low
  • Severity changed from normal to minor

@niftythree I'd like to clarify the request.

If I understood the request correctly, you would like to get the total number of messages in a request/thread.

So if your app requests a thread which has 30 messages, and you make a request to show 10 messages per page/screen, you would like the 10 items but also wanna know the total amount of messages are available in this thread.

If that's the case, you can get this information not in the response of the request but in its header. See https://developer.wordpress.org/rest-api/using-the-rest-api/pagination/#pagination-parameters

To determine how many pages of data are available, the API returns two header fields with every paginated response:
X-WP-Total: the total number of records in the collection
X-WP-TotalPages: the total number of pages encompassing all available records

That means you can:

  • get the total number of thread messages using the X-WP-Total header.
  • get the total number of screens/pages using the X-WP-TotalPages header
  • And the total amount of messages per screen recipients_per_page, which is a parameter you can set as well.

See https://developer.buddypress.org/bp-rest-api/reference/private-messaging/#list-messages-threads for a list of pagination parameters which are specific to this endpoint.

#2 @niftythree
3 years ago

Hi @espellcaste,

Thank you for the reply.

To clarify, we’re using an API call to populate “Inbox/Starred/Outbox” pages where the message threads are displayed in a list, so at the moment, to get the total number of messages per thread we have to call the endpoint multiple times and add up the message counts, and then put this total against each respective thread (see screenshot for example).

Therefore, we don't need the number of messages in a specific thread, but for the list of message threads returned when calling the “messages” endpoint directly (i.e. “…/wp-json/buddypress/v1/messages”).

Thanks for sharing that information about the header however, it may come in handy in future! 🙂

#3 @espellcaste
3 years ago

  • Keywords needs-patch dev-reviewed added
  • Milestone changed from Under Consideration to 11.0.0
  • Owner set to espellcaste
  • Priority changed from low to normal
  • Status changed from new to assigned

Ok! I think I understand your request now. I'll introduce that. Thanks for clarifying. :)

This ticket was mentioned in PR #31 on buddypress/buddypress by renatonascalves.


2 years ago
#4

  • Keywords has-patch added; needs-patch removed

As titled.

In this first part, I'm making sure data is present in a performant way. The second part is changing get_messages and get_recipients to stop getting all data at once.

Currently, if a thread has 10k messages, get_messages will try to get all messages in a single SQL query. The same is true for get_recipients. This is a breaking change, so it bears more discussion.

Trac ticket: https://buddypress.trac.wordpress.org/ticket/8697
Trac ticket: https://buddypress.trac.wordpress.org/ticket/8696

renatonascalves commented on PR #31:


2 years ago
#5

@imath Yes!

Only running this when paginated is not good enough IMO. We are still querying ALL messages on the database (or in the cache) by default (regardless if the request is paginated or not) which is exactly what I'm trying to solve as part of this task.

This is a flaw in the design which I postponed solving when I introduced messages/recipients pagination but that ultimately should be solved regardless (at some point, why not now?!). The performance issue is more obvious when you have apps with threads with thousands of messages or/and recipients. (I've been doing some stress testing with the BP GraphQL extension.)

I'm aware this might be a breaking change and I'm open to options for how we can smooth this update to users. But doing those requests separately will be more performant (since they are cached) IF we get a smaller subset of messages. And since the data is not related to the current batch of messages, I can't see how we could get them without doing separate, cached, queries.

The second part of this task is to actually stop the default behavior from querying ALL messages /recipients by default and set a reasonable number. Allowing the option to get all messages/recipients, like posts_per_page = -1.

--

There is an argument to be made here that those two new messages|recipients_count MySQL queries should not be part of this class. We could move them to the BP REST API if desired. But if we follow through on this update, we'll need them regardless.

@imath commented on PR #31:


2 years ago
#6

@renatonascalves

Thanks for your explanations, here's what I suggest as it might be a breaking change as you wrote and we're ~8 days before 11.0.0-beta1:

  1. Make a first step about returning the total number of messages per thread to cover #BPTrac-8697 (which I'm not sure you did into this PR), if the messages of the thread are paginated or if a specific argument is passed eg: what we're doing here for the Activity API.
  2. Make another step about making sure to return the latest message of the thread, your self::get_latest_thread_message( $this->thread_id ); to cover #BPTrac-8696 in case the the messages of the thread are paginated, otherwise we already have it.
  3. Open a new ticket on BP Trac for the other performance improvements, and share data/benchmarks about it to explain/prove how 4 queries are taking less time than 1. Ideally, we'd need to commit these kind of improvements early during a development cycle so that we have enough time to test it. Moreover we should use these improvements into Core, eg: adding pagination into the Template packs (<- I can help on this part if needed).

What do you think ?

#7 @espellcaste
2 years ago

  • Milestone changed from 11.0.0 to Up Next

renatonascalves commented on PR #31:


2 years ago
#8

Sounds like an awesome plan! I'll bump related tickets to the next version so that we have more time to implement and test those changes. o/

This ticket was mentioned in PR #51 on buddypress/buddypress by renatonascalves.


2 years ago
#9

  • Keywords has-unit-tests added

renatonascalves commented on PR #51:


2 years ago
#10

We could also not take this approach at all and do the actual MySQL query in the BP REST API plugin. Here I'm assuming this new piece of data might be useful for those using or extending BP_Messages_Thread.

renatonascalves commented on PR #51:


2 years ago
#11

@imath Thanks for the cache reminder. I updated the code to use a different approach, including support for cache busting. :)

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


2 years ago

#13 @imath
2 years ago

  • Milestone changed from Up Next to 12.0.0

#14 @espellcaste
2 years ago

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

#15 @espellcaste
2 years ago

In 13403:

PHPUnit: fixes issue on the test_get_messages_total_count_cached unit test.

See #8697

Note: See TracTickets for help on using tickets.