Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Dashboard campaigns Row per page not working #1887

Open
puntope opened this issue Feb 14, 2023 · 6 comments
Open

Dashboard campaigns Row per page not working #1887

puntope opened this issue Feb 14, 2023 · 6 comments
Labels
type: bug The issue is a confirmed bug.

Comments

@puntope
Copy link
Contributor

puntope commented Feb 14, 2023

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

Screen.Recording.2023-02-14.at.20.58.50.mov
@puntope puntope self-assigned this Feb 14, 2023
@puntope puntope added the type: bug The issue is a confirmed bug. label Feb 15, 2023
@puntope
Copy link
Contributor Author

puntope commented Feb 20, 2023

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.

@jorgemd24
Copy link
Contributor

While working on this PR #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.

@mikkamp
Copy link
Contributor

mikkamp commented Jul 17, 2024

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?

@jorgemd24
Copy link
Contributor

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 #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

@mikkamp
Copy link
Contributor

mikkamp commented Jul 18, 2024

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
Copy link
Contributor

mikkamp commented Dec 9, 2024

We did update the API endpoints to support pagination in #2472
However the UI was never updated to support the pagination.

Unfortunately pageSize has been removed since Google Ads API V17+, each request will always use a page size of 10,000 and return an error if we set it to anything else.

This means we'll have to revert the previous solution as we aren't ready yet to go for an interim solution where we can map between paginated requests and the results returned by the Google API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug The issue is a confirmed bug.
Projects
None yet
Development

No branches or pull requests

3 participants