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

Add GAM integration #124

Merged
merged 36 commits into from
Jun 15, 2021
Merged

Add GAM integration #124

merged 36 commits into from
Jun 15, 2021

Conversation

adekbadek
Copy link
Member

@adekbadek adekbadek commented Apr 16, 2021

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:

  1. On 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 placements
  2. Switch to this branch, and use add/gam-ad-units-in-ads-plugin branch of the Newspack plugin (Handle GAM ad units from Ads plugin newspack-plugin#936)
  3. Do not authenticate via Google yet (or revoke access)
  4. Visit the site, verify that the ads work as before
  5. Visit the Advertising wizard - observe that legacy ad units are listed and editable
  6. Set NEWSPACK_ADS_USE_GAM env var to value true - this enables the GAM connection
  7. Authenticate Newspack to use Google data via Newspack dashboard (NEWSPACK_GOOGLE_OAUTH_ENABLED env var has to be set)
  8. Observe that now the ad units like displays both legacy ad units, and GAM ad units
  9. Create, edit, then archive a GAM ad unit - observe the changes are reflected in GAM web UI at each step
  10. Assign an ad unit to a global placement or insert via the Ad block - observe the ad unit displaying an ad*
  11. Observe the ad units set up while on master are still displayed
  12. Change the network code in the options table (_newspack_ads_service_google_ad_manager_network_code) edit: change value of $user_network_code in is_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

Copy link
Contributor

@dkoo dkoo left a 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!

@adekbadek
Copy link
Member Author

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 ?

Copy link
Contributor

@dkoo dkoo left a 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:

  1. 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.).

@adekbadek
Copy link
Member Author

Other than that, I experienced different (maybe better?) behavior at the final step:

@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.

@adekbadek adekbadek requested a review from dkoo June 10, 2021 12:00
Copy link
Contributor

@dkoo dkoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙌

@adekbadek adekbadek merged commit ccd2c28 into master Jun 15, 2021
adekbadek added a commit to Automattic/newspack-plugin that referenced this pull request Jun 15, 2021
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
@adekbadek adekbadek deleted the add/gam-ad-units branch June 15, 2021 09:53
matticbot pushed a commit that referenced this pull request Jun 15, 2021
# [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))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.14.0-alpha.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

matticbot pushed a commit that referenced this pull request Jun 15, 2021
# [1.14.0](v1.13.1...v1.14.0) (2021-06-15)

### Bug Fixes

* run composer install on release tasks ([#135](#135)) ([fbf1dbc](fbf1dbc))

### Features

* add experimental GAM integration ([#124](#124)) ([ccd2c28](ccd2c28))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.14.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants