-
Notifications
You must be signed in to change notification settings - Fork 21
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
Update Google Ads API to v18 #2740
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2740 +/- ##
===========================================
+ Coverage 67.0% 67.0% +0.1%
+ Complexity 4692 4690 -2
===========================================
Files 479 479
Lines 19612 19601 -11
===========================================
+ Hits 13136 13141 +5
+ Misses 6476 6460 -16
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the Google Ads API. I ran the full smoke tests, created campaigns with/without assets, everything worked as expected.
Changes proposed in this Pull Request:
This PR update the Ads API to use V18. The composer packages were already updated in a previous PR.
Required changes:
V16 > V17
GoogleAdsService Search requests: Passing a page_size to GoogleAdsService.Search will result in a RequestError.PAGE_SIZE_NOT_SUPPORTED error.
V17 > V18
GoogleAdsService: The summary_row_setting and return_total_results_count fields in SearchGoogleAdsRequest are now part of the new SearchSettings object.
The original pagination support for campaigns was reverted from this PR, as the Google Ads API has removed the
pageSize
parameter and sets it to a fixed value of 10000 rows. For general use cases that's ok because of the associated costs most merchants won't be managing 1000's of campaigns.The initial reason why we did add pagination to the campaigns endpoint is to support the use case where the mobile app only wanted to fetch a single campaign. For this purpose I've left the
per_page
parameter in place to limit the API response. We still get the full set from the Ads API but we limit the results sent to the API. Because of this limitation it's not possible to fetch subsequent pages. We have a feature request open if we want to implement a full pagination support in the future.As a temporary measure the headers returning totals and pagination parameters was removed in this PR until we are ready to fully support pagination. I also added some CSS to remove the pagination from the list of campaigns in the dashboard, since this doesn't work.
Detailed test instructions:
rm -rf vendor && composer install
nvm use && npm start
Additional details:
pcTzPl-2zd-p2
Changelog entry