google-listings-and-ads icon indicating copy to clipboard operation
google-listings-and-ads copied to clipboard

Dashboard campaigns Row per page not working

Open puntope opened this issue 2 years ago • 5 comments

Describe the bug:

While testing PMAX campaigns I realised I had more than 25 campaigns but paginator still shows 25 per page and showing my 35 campaigns.

Steps to reproduce:

  1. Create 26 campaigns
  2. See the dashboard showing 26 campaigns
  3. See rows per page showing 25

Expected behavior:

  • It only shows 25 campaigns and navigation for switching between pages
  • The selector allows to change campaigns per page

Actual behavior:

  • It shows all the campaigns
  • The rows per page selector is not working

Screenshots

https://user-images.githubusercontent.com/5908855/218848838-4cdc3648-f3c7-4b4c-a8b4-9f27bd8f94a6.mov

puntope avatar Feb 14 '23 20:02 puntope

After some investigation it seems like we need to implement the Google API pagination. Seems like it's not an easy fix since Google API Pagination works with PageToken links instead of OFFSET param. But in overview the steps to make it happen could be:

Backend:

  • Accept PageSize and PageToken in the /ads/campaigns endpoint
  • Grab the results from Google API using that params
  • Return the campaigns and also the NextPageToken

Frontend:

  • Add pageSize and pageToken in he API call
  • Modify the response handler to take the campaigns data and the nextPageToken
  • Map the pageTokens with page numbers for the forwards and backwards navigation
  • Modify the TableCard and add a custom paginator similar to AttributeMapping rules or ProductFeed Issues.

puntope avatar Feb 20 '23 07:02 puntope

While working on this PR https://github.com/woocommerce/google-listings-and-ads/pull/2472, I was also considering this issue. However, a downside of using pagination is that the pagination component requires the total number of results across all pages. Currently, Google doesn't offer an equivalent of COUNT as in MySQL; you have to fetch all the results. Thus, the benefit of pagination would be lost if we still have to retrieve all the campaigns.

jorgemd24 avatar Jul 16 '24 19:07 jorgemd24

The Ads API does allow you to return the total amount in the original query through using return_total_results_count. Is that something which we can use to handle the pagination? Here is an example of how we use that to query the total amount of accounts: https://github.com/Automattic/woocommerce-connect-server/blob/trunk/lib/google/index.js#L653-L661

If we are using the library it will be named slightly different, but this example shows how to enable it and get the total results: https://developers.google.com/google-ads/api/samples/get-all-disapproved-ads#php

Would that match our pagination structure?

mikkamp avatar Jul 17 '24 07:07 mikkamp

Thanks @mikkamp for your comment! Yes, I misread the description of return_total_results_count and thought it was returning the number of results for the current query.

I adjusted the PR https://github.com/woocommerce/google-listings-and-ads/pull/2472 to add some pagination headers that will allow us to get the total number of pages, total results, and the next page token. From the UI perspective, I think we could disable the Go to Page feature to keep it simple, as it seems that it's not straightforward to get the forward page token until you access the page (for example, jumping from page 1 to page 5). Here is an example: https://developers.google.com/google-ads/api/samples/navigate-search-result-pages-caching-tokens.

The backward tokens can be cached in the client, so the merchant can navigate backwards and one page forward (through the pages that we already loaded)

image

jorgemd24 avatar Jul 17 '24 18:07 jorgemd24

Thanks for the clarification. As seen in issue #811 we aren't very consistent in how we display / handle the pagination bar. We don't always have the "go to page" or "rows per page", so I think it makes sense to leave it out in this case.

Edit: I mean just leave the "go to page" out. The "rows per page" seems handy.

mikkamp avatar Jul 18 '24 07:07 mikkamp