Enable page size in campaigns endpoint #2472
Conversation
github-actions
bot
added
the
changelog: update
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2472 +/- ##
============================================
+ Coverage 63.4% 64.7% +1.3%
- Complexity 0 4286 +4286
============================================
Files 321 459 +138
Lines 5027 16915 +11888
Branches 1219 0 -1219
============================================
+ Hits 3188 10941 +7753
- Misses 1672 5974 +4302
+ Partials 167 0 -167
Flags with carried forward coverage won't be shown. Click here to find out more.
|
jorgemd24
changed the title
src/API/Google/AdsCampaign.php
Outdated
| * @param bool $fetch_criterion Combine the campaign data with criterion data (default true). | ||
| * @param bool $exclude_removed Exclude removed campaigns (default true). | ||
| * @param bool $fetch_criterion Combine the campaign data with criterion data (default true). | ||
| * @param array $args Additional arguments. |
There was a problem hiding this comment.
| @@ -152,6 +152,67 @@ public function test_get_campaigns_converted_names() { | |||
| $this->assertEquals( 200, $response->get_status() ); | |||
| } | |||
|
|
|||
| public function test_get_campaigns_with_args() { | |||
There was a problem hiding this comment.
In the test added here where are we testing another page_size? For example page_size=1 to see if we don't get all the campaigns and just 1
There was a problem hiding this comment.
Thanks for the changes @jorgemd24
I left some minor comments but the per_page works as expected.
One thing I miss is the page parameter to get the results for a specific page. Not sure if its for an other PR.
|
Thanks @puntope for reviewing this PR! I addressed your suggestions.
Initially, I planned not to set the next page token in the endpoint response for the reasons mentioned in this comment: #1887 (comment). 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:
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. |
| * | ||
| * @return array | ||
| * @throws ExceptionWithResponseData When an ApiException is caught. | ||
| */ | ||
| public function get_campaigns( bool $exclude_removed = true, bool $fetch_criterion = true ): array { | ||
| public function get_campaigns( bool $exclude_removed = true, bool $fetch_criterion = true, $args = [], $return_pagination_params = false ): array { |
There was a problem hiding this comment.
What's the value of NOT returning pagination params? Even if they are not needed I assume they are not harmful. So maybe we can simplify this method by returning them always.
There was a problem hiding this comment.
@puntope, the reason for not always returning the pagination params is that we would need to refactor other parts of the code and tests that depend on this method:
- ..etc
Do you think it’s worth refactoring those parts even if they don't require pagination?
There was a problem hiding this comment.
Thx. @jorgemd24 I think we can go forward with this and evaluate the refactoring you mentioned
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=Xand check that the number of results matches the per_page parameter.GET gla/ads/campaignsand 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