-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add GAM integration #124
Add GAM integration #124
Conversation
e2fba37
to
91c2062
Compare
724910d
to
c2b47e1
Compare
c2b47e1
to
64a7b96
Compare
2541e7b
to
9ad9c3b
Compare
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.
Newest changes and tests look good!
ef5a266
to
af73340
Compare
af73340
to
1fd5f75
Compare
In order to selectively test these features, I've changed it so they are enabled by a feature flag. See updated testing instructions. Could you please review again @dkoo ? |
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.
Great work! Ran into one minor PHP error, details below. Other than that, I experienced different (maybe better?) behavior at the final step:
- Change the network code in the options table (
_newspack_ads_service_google_ad_manager_network_code
) to something else. Observe a notice about the mismatch above ad units, and that the ad units editing and creation is unavailable in the UI
I changed the option value to some garbage string, then refreshed the Advertising wizard. After refreshing, the option value seemed to revert back to the correct GAM network code and the front-end was unaffected (I could still see all ad units, create new ones, edit existing ones, etc.).
@dkoo that's right, I forgot that the network code will be sychronised when the GAM data is loaded. To test network code mismatch, follow the updated step 12. in the PR description. |
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.
🙌
Experimental: NEWSPACK_GOOGLE_OAUTH_ENABLED, NEWSPACK_ADS_USE_GAM env variables have to be set, and the user added as the test user to (yet unpublished) Google Cloud Platform app. see: Automattic/newspack-ads#124
# [1.14.0-alpha.1](v1.13.1...v1.14.0-alpha.1) (2021-06-15) ### Features * add experimental GAM integration ([#124](#124)) ([ccd2c28](ccd2c28))
🎉 This PR is included in version 1.14.0-alpha.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.14.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
Changes proposed in this Pull Request:
The ad units created in GAM have to be manually recreated in Newspack, linking one with the other via the unique ad placement code. With changes here, the ad units are fetched from GAM and saved in the DB - in order not to query GAM on each page load.
The only downside is that the sync happens when the user edits the ad units, so the changes made in GAM web UI will not be reflected until the user logs in to the Advertising wizard of Newspack plugin. This is not a regression though - it was like that before.
How to test the changes in this Pull Request:
master
, set up some ad units to show (via global placements or the Ad block) - to ensure the update will not break existing ad unit placementsadd/gam-ad-units-in-ads-plugin
branch of the Newspack plugin (Handle GAM ad units from Ads plugin newspack-plugin#936)NEWSPACK_ADS_USE_GAM
env var to valuetrue
- this enables the GAM connectionNEWSPACK_GOOGLE_OAUTH_ENABLED
env var has to be set)master
are still displayedthe options table (edit: change value of_newspack_ads_service_google_ad_manager_network_code
)$user_network_code
inis_network_code_matched
method to something else. Observe a notice about the mismatch above ad units, and that the ad units editing and creation is unavailable in the UI* if properly configured - but in non-AMP context, an ad "surface" will be displayed anyway