Implement the OAuth flow for the Setup CTA Banner

[techanvil]

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 https://github.com/google/site-kit-wp/issues/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

• If the user does not have the Analytics Admin Edit scope, then clicking on Enable groups in the Setup CTA should trigger the standard OAuth flow used in the plugin.
  • On successful completion, the user should be redirected back to the dashboard where the rest of the happy path implemented in #8131 should be followed.

Implementation Brief

• [ ] Update assets/js/modules/analytics-4/components/dashboard/AudienceSegmentation/AudienceSegmentationSetupCTAWidget.js
  • [ ] Add a callback function for Enable groups CTA:
    • Check if user does not has Analytics 4 Edit scope, to trigger the setPermissionScopeError for ANALYTICS_EDIT_SCOPE. You can see an example here
    • Append query argument notification with value, say audience_segmentation, to bypass authentication_success banner. You can see an example here
    • Set autoSubmit value on CORE_FORMS, by dispatching setValues action, and assign it to, say, AUDIENCE_SEGMENTATION_SETUP_FORM. You can see an example here
• [ ] Update the setup component implemented in #8131 to proceed with the setup step (when user has the ANALYTICS_EDIT_SCOPE) if query argument autoSubmit is true

Test Coverage

• Update jest tests to verify this scenario of jumping to the setup step after OAuth redirection. You can see example of mocking the redirect url here

QA Brief

Changelog entry

[tofumatt]

ACs sound good 👍🏻

[techanvil]

Hi @zutigrm, the IB is looking good, however I am not sure we should use the notification query param as the condition for reshowing the banner and continuing to the next step.

It's not an approach we have taken to date, rather we have a pattern of setting an autoSubmit form value, which is persisted via the store snapshot mechanism and restored on the next page load. For example:

https://github.com/google/site-kit-wp/blob/efdd75e4de73791a2e09eba73b2c4b6f57789304/assets/js/modules/analytics-4/components/common/AccountCreate/index.js#L151-L163

We then use the autoSubmit value in the conditions for restoring the previous UI state and continuing.

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 notification param may not be present in this scenario.

[zutigrm]

@techanvil Thank you for the feedback, I forgot about autoSubmit 🤦 . I updated the IB

[techanvil]

Thanks @zutigrm! IB LGTM. :white_check_mark:

[ankitrox]

Adding "Blocked by" #8707 because some logic inside the component AudienceSegmentationSetupCTAWidget will get changed in 8707 because of which tests for this issue needs to accommodate the change.

[techanvil]

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.

[ankitrox]

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 enableAudienceGroups action so that those are more of the integration in nature as per the convention.

Please note that I have made the use of muteFetch for reports endpoint as this is not really specific to the tests being performed and to keep the tests readable.

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

[techanvil]

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?

[ankitrox]

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

[techanvil]

No worries, thanks @ankitrox! As per my latest review this needs one more pass, please take a look.

[ankitrox]

@techanvil Thank you.

Tests updated as per suggestions.

[kelvinballoo]

QA Update ❌

I have tried to test this but it's not working as expected. After clicking on 'Enable groups', I get redirected to the OAuth flow but once I am back to the dashboard, the success message appears after a while but the enable group still has the spinning loading icon.

Video is attached below for reference:

https://github.com/google/site-kit-wp/assets/125576926/9ddf036b-77ca-4980-bca8-74bcce2126b3

[ankitrox]

Thanks @kelvinballoo ,

As per our slack conversation, we need the analytics property with write access (in order to create audiences in this case).

Can you please test it according to that and let me know if there is something you want me to do in this case?

Thanks

[kelvinballoo]

QA Update ⚠️

Noted on that @ankitrox

I have tested this happy flow with an analytics property with write access and it's looking good. Refer to below for the desktop flow. ✅

https://github.com/google/site-kit-wp/assets/125576926/e287f678-1673-4c44-93c3-06ce3a473331

It's worth noting that the success message box design on mobile doesn't align with Figma. @ankitrox , let me know if this needs another ticket or there is an existing ticket as it's most likely out of scope for this ticket. ⚠️

Mobile implementation (iPhone 15prox max):

Figma:

[kelvinballoo]

QA Update ✅

After latest conversations, the mobile issue is not part of this ticket. Hence, another ticket has been raised here: https://github.com/google/site-kit-wp/issues/8932

As for this ticket, it's good to go as the happy path for the setup is working properly. Video is attached for reference below.

Moving this ticket to Approval.

https://github.com/google/site-kit-wp/assets/125576926/e287f678-1673-4c44-93c3-06ce3a473331

[aaemnnosttv]

@techanvil we're requesting the edit scope here but that isn't mentioned in the interface, nor do we know for sure that it's needed, do we? We should only request the readonly scope as usual until we actually need the edit scope. When requesting the edit scope we always do so with a explanation as to why. Maybe this is coming in a later issue or was missed during UX but I just wanted to call it out. It's not blocking here.