-
Notifications
You must be signed in to change notification settings - Fork 279
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
Implement the OAuth flow for the Setup CTA Banner #8132
Comments
ACs sound good 👍🏻 |
Hi @zutigrm, the IB is looking good, however I am not sure we should use the It's not an approach we have taken to date, rather we have a pattern of setting an site-kit-wp/assets/js/modules/analytics-4/components/common/AccountCreate/index.js Lines 151 to 163 in efdd75e
We then use the I'd suggest we take the same approach here, in part for consistency, but also because it will be useful for the unhappy path too. We can use it for showing the error modal - the |
@techanvil Thank you for the feedback, I forgot about |
Thanks @zutigrm! IB LGTM. ✅ |
Adding "Blocked by" #8707 because some logic inside the component |
Hey @ankitrox, I've left a few comments on the PR. With regard to the QAB - although it could be enough to give the QA team sufficient direction, it would be useful to provide a bit more detail. I've gone ahead and rewritten it a bit myself, please review the changes and make sure you are happy with them. Note that I've completely omitted the point about visiting https://myaccount.google.com/connections to remove permissions - we don't generally need to do that, in the scenario here simply disconnecting and reconnecting Site Kit is sufficient. |
Hi @techanvil , We can discuss about the use of mocks for the actions later when @aaemnnosttv is available. However; I have changed the tests to make use of assertions for Please note that I have made the use of Also, updated these tests so that this ticket can be moved ahead because there are some other tickets which are dependent on this one. Assigning this to you for re-review. Thank you |
Thanks @ankitrox! As per my review comment, I am not seeing the updated version of the tests. Maybe you need to push a recent change? |
@techanvil Sorry, that was my bad! I committed the changes, but missed to push them. I have pushed the changes and those should be available. Please refer to this commit where I have removed mock's implementation just to stick to our convention. |
No worries, thanks @ankitrox! As per my latest review this needs one more pass, please take a look. |
@techanvil Thank you. Tests updated as per suggestions. |
Feature Description
Implement the redirection to OAuth to grant the edit scope for the Setup CTA Banner.
Note that this issue only covers the happy path, the unhappy path will be implemented separately via #8134.
See setup CTA banner > setup logic in the design doc.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Enable groups
in the Setup CTA should trigger the standard OAuth flow used in the plugin.Implementation Brief
assets/js/modules/analytics-4/components/dashboard/AudienceSegmentation/AudienceSegmentationSetupCTAWidget.js
Enable groups
CTA:setPermissionScopeError
forANALYTICS_EDIT_SCOPE
. You can see an example herenotification
with value, sayaudience_segmentation
, to bypassauthentication_success
banner. You can see an example hereautoSubmit
value onCORE_FORMS
, by dispatchingsetValues
action, and assign it to, say,AUDIENCE_SEGMENTATION_SETUP_FORM
. You can see an example hereANALYTICS_EDIT_SCOPE
) if query argumentautoSubmit
istrue
Test Coverage
QA Brief
audienceSegmentation
feature flag enabled.Changelog entry
The text was updated successfully, but these errors were encountered: