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.

Sign up for GitHub

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

Jump to bottom

Monetization: Add MGID support #13368

Merged
merged 7 commits into from Jun 19, 2023
Merged

Monetization: Add MGID support #13368

merged 7 commits into from Jun 19, 2023

Conversation

leprik
Copy link
Contributor

@leprik leprik commented Jun 13, 2023 •
edited

Context

MGID is now support Web Story ads and we want to add to WP plugin.
https://amp.dev/documentation/guides-and-tutorials/develop/advertise_amp_stories

Summary

MGID integration

Relevant Technical Choices

Source code is similar to Ad Manager

To-do

Not applicable.

User-facing changes

image

Testing Instructions

  1. On the Settings screen, see the "Monetization" section
  2. Select "MGID" and enter Widget ID value in the same format as in the example
    (Other formats should cause warnings / red borders)
  3. Save changes and view a story on the frontend. Verify that there's an tag in the page source for MGID

Reviews

Does this PR have a security-related impact?

No.

Does this PR change what data or activity we track or use?

No.

Does this PR have a legal-related impact?

No.

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

swissspidy
swissspidy reviewed Jun 13, 2023
View reviewed changes
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Sweet, thank you! I was not aware of this new support. At first glance this all looks good. Just a few minor comments.

includes/Settings.php Show resolved Hide resolved
includes/Mgid.php Outdated Show resolved Hide resolved
includes/Mgid.php Outdated Show resolved Hide resolved
includes/Mgid.php Outdated Show resolved Hide resolved
includes/Settings.php Show resolved Hide resolved
@leprik leprik force-pushed the try/mgid branch 3 times, most recently from 234b3ee to b56ea36 Compare June 13, 2023 15:44
@leprik leprik requested a review from swissspidy June 13, 2023 16:27
swissspidy
swissspidy reviewed Jun 14, 2023
View reviewed changes
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

Getting there! My suggestions should help fix the tests. Just a few formatting issues in in addition to that. Try running Prettier with npm run lint:js:fix

packages/wp-dashboard/src/components/editorSettings/test/editorSettings.js Outdated Show resolved Hide resolved
includes/Mgid.php Show resolved Hide resolved
tests/phpunit/integration/tests/Mgid.php Show resolved Hide resolved
packages/wp-dashboard/src/components/editorSettings/adManagement/mgid/index.js Outdated Show resolved Hide resolved
packages/wp-dashboard/src/components/editorSettings/adManagement/test/adManagement.js Outdated Show resolved Hide resolved
@swissspidy swissspidy added this to the 1.33.0 milestone Jun 14, 2023
@leprik
Copy link
Contributor Author

leprik commented Jun 14, 2023

Tried my best to fix all linter and unit tests errors. Let's run pipeline checks again.

@swissspidy
Copy link
Collaborator

Looks like all the relevant tests are passing now 🎉 The remaining ones are known to be flakey, so can be ignored.

swissspidy
swissspidy reviewed Jun 14, 2023
View reviewed changes
includes/Settings.php Outdated Show resolved Hide resolved
@leprik leprik force-pushed the try/mgid branch 2 times, most recently from 801a5f4 to cb3133c Compare June 19, 2023 08:21
@leprik
Copy link
Contributor Author

leprik commented Jun 19, 2023

@swissspidy Can you re-check latest update?

@leprik leprik requested a review from swissspidy June 19, 2023 11:22
AnuragVasanwala
AnuragVasanwala approved these changes Jun 19, 2023
View reviewed changes
Copy link
Collaborator

@AnuragVasanwala AnuragVasanwala left a comment

Choose a reason for hiding this comment

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

Minor nitpick, otherwise LGTM!

tests/phpunit/integration/tests/Settings.php Outdated Show resolved Hide resolved
packages/wp-dashboard/src/components/editorSettings/adManagement/index.js Show resolved Hide resolved
packages/wp-dashboard/src/components/editorSettings/adManagement/mgid/index.js Show resolved Hide resolved
@leprik
Copy link
Contributor Author

leprik commented Jun 19, 2023

@swissspidy @AnuragVasanwala can we come back to string? Now all changes from string to number produce many issues and workarounds to work with default null (undefined in JS) value.

@swissspidy
Copy link
Collaborator

Let me take a look 👍

@AnuragVasanwala
Copy link
Collaborator

@swissspidy @AnuragVasanwala can we come back to string? Now all changes from string to number produce many issues and workarounds to work with default null (undefined in JS) value.

@leprik Absolutely, if it is causing issues, please revert back to the string.

@AnuragVasanwala
Copy link
Collaborator

AnuragVasanwala commented Jun 19, 2023 •
edited

@leprik I have only acknowledged for the changes highlighted by my review:

Let's wait for the @swissspidy review on the changes highlighted by him earlier related to the latest commit 72df86f.

swissspidy
swissspidy approved these changes Jun 19, 2023
View reviewed changes
Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

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

As long as a string works fine with MGID to display ads, that sounds good to me :-)

@swissspidy swissspidy changed the title Settings: MGID support Monetization: Add MGID support Jun 19, 2023
@swissspidy swissspidy added Type: Enhancement New feature or improvement of an existing feature Group: Settings labels Jun 19, 2023
@swissspidy swissspidy merged commit d4c9d04 into GoogleForCreators:main Jun 19, 2023
51 checks passed
@leprik leprik deleted the try/mgid branch September 1, 2023 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@swissspidy swissspidy swissspidy approved these changes

@AnuragVasanwala AnuragVasanwala AnuragVasanwala approved these changes

Assignees
No one assigned
Labels
Group: Settings Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Milestone
1.33.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@leprik @swissspidy @AnuragVasanwala