Closed jorgemd24 closed 2 weeks ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 64.7%. Comparing base (
5069f65
) to head (6ff5916
).
Thanks @puntope for reviewing this PR! I addressed your suggestions.
One thing I miss is the page parameter to get the results for a specific page. Not sure if its for an other PR.
Initially, I planned not to set the next page token in the endpoint response for the reasons mentioned in this comment: https://github.com/woocommerce/google-listings-and-ads/issues/1887#issuecomment-2231712265. However, after @mikkamp's comment, I realized it is possible to get the total number of results in the first query. Therefore, I implemented the following headers:
X-WP-Total
-> Total number across all pages.X-WP-TotalPages
-> Total pages.X-GLA-NextPageToken
-> The token to get the next page. (I used the GLA prefix to avoid future conflicts with this header name)With this, we can fetch the next page and store the page token in the client to navigate backwards. However, it seems that it is not possible to get the forward page tokens until you access the page, (for example jumping from page 1 to page 5) so we need to see if it is feasible to implement this in the front end and the back end.
I found this example that can be useful: https://developers.google.com/google-ads/api/samples/navigate-search-result-pages-caching-tokens but for now, I will leave that implementation/investigation for a different PR as the mobile team is interested in limiting the result to 1 to check if the merchant is active and I think this PR will help us to move forward to implement the pagination in the campaign table.
Changes proposed in this Pull Request:
Part of pcTzPl-2nS-p2
This PR allows setting the per_page parameter in the gla/ads/campaigns endpoint. Previously, we returned all the results, but now it is possible to limit the number of results.
Screenshots:
Detailed test instructions:
GET gla/ads/campaigns?per_page=X
and check that the number of results matches the per_page parameter.GET gla/ads/campaigns
and ensure that you get all the results.Additional details:
I had to replace iterateAllElements() with
getPage()->getIterator()
to avoid iterating through all the pages and retrieving all the results at once.Changelog entry