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

Update Ads module to conditionally require adwords scope #8565

Closed
1 task
aaemnnosttv opened this issue Apr 15, 2024 · 2 comments
Closed
1 task

Update Ads module to conditionally require adwords scope #8565

aaemnnosttv opened this issue Apr 15, 2024 · 2 comments
Labels
Module: Ads Google Ads module related issues P0 High priority PHP Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Apr 15, 2024

Feature Description

The PAX application requires an OAuth token with the Adwords scope (https://www.googleapis.com/auth/adwords) granted.

Since this is only relevant to request + keep when using PAX, it should be requested on-demand and then treated as a required scope so long as Ads is connected via PAX.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The https://www.googleapis.com/auth/adwords scope should be conditionally required by the Ads module when the adsPax feature is enabled
  • Once the scope has been granted, it should continue to be required for the current user
  • Once the extCustomerID is set, it should be generally required by the module

Implementation Brief

  • Update the Ads module to include the Module_With_Scopes interface + use the Module_With_Scopes_Trait trait
    • Add the register_scopes_hook to register
    • Implement the get_scopes method as follows:
    • By default it should not require any, and return an empty list
    • If the adsPax feature is enabled, conditionally return the adwords scope if the scope is already granted, or extCustomerID is set/valid

Test Coverage

  • Add test coverage for the conditional behavior using googlesitekit_auth_scopes filter for assertions

QA Brief

  • Setup Site Kit with proxy plugin
  • Enable adsPax feature flag
  • Force the adwords permission popup with following JS snippet:
googlesitekit.data.dispatch("core/user").setPermissionScopeError({
  code: "missing_required_scopes",
  message: "Additional permissions are required.",
  data: {
    status: 403,
    scopes: ["https://www.googleapis.com/auth/adwords"],
    skipModal: false,
    redirectURL:
      "/wp-admin/admin.php?page=googlesitekit-dashboard",
  },
});
  • After OAuth redirection, go back to Site Kit dashbaord and paste this JS snippet: googlesitekit.data.select('core/user').getGrantedScopes(), verify you see adwords scope in the outputted list

Changelog entry

  • Add feature that requests AdsWords scope when required.
@aaemnnosttv aaemnnosttv added P0 High priority Type: Enhancement Improvement of an existing feature Module: Ads Google Ads module related issues labels Apr 15, 2024
@bethanylang bethanylang added Squad 1 (Team S) Issues for Squad 1 Next Up Issues to prioritize for definition labels Apr 16, 2024
@10upsimon 10upsimon assigned 10upsimon and unassigned 10upsimon Apr 18, 2024
@tofumatt tofumatt self-assigned this Apr 18, 2024
@tofumatt
Copy link
Collaborator

ACs here look good, moving to IB 👍🏻

@tofumatt tofumatt removed their assignment Apr 18, 2024
@aaemnnosttv aaemnnosttv self-assigned this Apr 19, 2024
@aaemnnosttv aaemnnosttv added PHP QA: Eng Requires specialized QA by an engineer labels Apr 19, 2024
@aaemnnosttv aaemnnosttv removed their assignment Apr 20, 2024
@eclarke1 eclarke1 removed the Next Up Issues to prioritize for definition label Apr 22, 2024
@zutigrm zutigrm self-assigned this Apr 23, 2024
@zutigrm zutigrm removed their assignment Apr 23, 2024
@10upsimon 10upsimon assigned 10upsimon and zutigrm and unassigned 10upsimon Apr 24, 2024
@zutigrm zutigrm assigned 10upsimon and unassigned zutigrm Apr 24, 2024
@10upsimon 10upsimon removed their assignment Apr 24, 2024
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Apr 24, 2024
@wpdarren wpdarren removed the QA: Eng Requires specialized QA by an engineer label Apr 25, 2024
@mohitwp
Copy link
Collaborator

mohitwp commented Apr 28, 2024

QA Update ✅

  • Tested on dev environment using service staging proxy plugin.
  • Verified that Additional scrope permission modal appeared successfully.
  • Verified that user is able to complete OAuth Flow.
  • Verified that after completing OAuth flow user redirects to main dashboard.
  • Verified that when we paste this JS snippet:googlesitekit.data.select('core/user').getGrantedScopes(), then adwords scope in the outputted list.
  • I ran code snippet multiple times and verified that Once the scope has been granted, it should continue to be required for the current user.

image

image

Recording.892.mp4

@mohitwp mohitwp removed their assignment Apr 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Ads Google Ads module related issues P0 High priority PHP Squad 1 (Team S) Issues for Squad 1 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants