-
Notifications
You must be signed in to change notification settings - Fork 299
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 unhappy paths for the Setup CTA Banner #8134
Comments
Hi @hussain-t, just letting you know I've moved this back to Backlog as the final in-progress changes to the design doc, relating to audience caching, will probably affect the AC for this one (we probably need to handle an audience sync error here). Please feel free to get preliminary AC ready in the meantime. |
Hi once again, @hussain-t! Again, as the audience caching aspect of the design doc has been sufficiently finalised, I've moved this back to AC. |
Hi @hussain-t, this IB is off to a good start. A few points:
|
Thanks for your valuable review, @techanvil.
This whole point is unnecessary. So, I went ahead and removed it.
I believe we can encapsulate this when necessary. As a general principle, we can extract reusable code when needed.
If it involves retrieving the value from the state and returning it, we can do this within the component itself, correct?
Do you mean setting a state in the Please correct me if I’m wrong. My understanding is that encapsulating these checks or creating a separate store might be an over-engineering solution at this stage. |
Thanks for your observations, @kelvinballoo.
We are using the common Regarding sizes, as mentioned earlier, we use the standard
That's expected due to the network blocking using the Tweak extension.
If you inspect, you'll see that it's a single sentence, not two. This follows the convention we use elsewhere for similar Insufficient Permissions Errors. You can review the AuthenticatedPermissionsModal stories for reference.
It's not applicable. I've updated the QAB. |
QA Update:
|
Hi @kelvinballoo and @wpdarren, thanks for raising this. The general design philosophy of Site Kit is to reuse generic components to achieve a consistent look and feel. So, reusing the The question about the line break on the "insufficient permissions" variant is related and I've asked Sigal to clarify that too. I don't think these issues need to hold this issue up, though - the current implementation is as I'd expect to see it, reusing Cc @hussain-t |
Thanks so much for the clarification, @techanvil ➕ |
On further note, the |
QA Update ✅This has been tested good:
Moving ticket to Approval. |
Hi @kelvinballoo, after some investigation, I can confirm that 5.1 is an issue. As a result, I've created a follow-up PR to fix it. I will get back to you for another pass shortly. Thanks!
|
…-follow-up Enhance/#8134 - Fix issue where closing the modal does not re-trigger it (follow-up)
Thanks @hussain-t, PR is now merged. Back to you @kelvinballoo for another pass. :) |
QA Update ✅This has been tested good after the latest fix:
Only issue found was about a hover effect which is not applicable here. Raised a ticket under #9007 Moving this ticket to Approval. |
Feature Description
Implement the unhappy paths for the Setup CTA Banner, making use of the Error Modal to display them. This includes handling errors returned by the OAuth flow.
See setup CTA banner > setup logic, OAuth errors and error modal in the design doc.
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
Update the
enableAudienceGroup
action inassets/js/modules/analytics-4/datastore/actions.js
:enableAudienceGroup
action to accept an object parameter that includesfailedSiteKitAudienceResourceNames
- A list of audiences that failed to be created by Site Kit.TODO
comments for error handling.TODO
comment with a return statement that returns an error object. For example, if there is an error in synchronizing available audiences, return the error object immediately.failedSiteKitAudienceResourceNames
is provided.failedSiteKitAudienceResourceNames
is provided, skip the creation of new audiences and proceed with the logic.failedSiteKitAudienceResourceNames
parameter.failedSiteKitAudienceResourceNames
is provided or use the default list of audiences to create.needsRetry
) to store the slugs of the audiences that failed to be created.failedSiteKitAudienceResourceNames
containing the slugs of the failed audiences.Update the
ModalDialog
component inassets/js/components/ModalDialog.js
:buttonLink
to handle opening a link in a new tab.buttonLink
is provided, update theSpinnerButton
component props to includehref
andtarget
attributes with thebuttonLink
and_blank
values, respectively.Create
AudienceErrorModal
component inassets/js/modules/analytics-4/components/audience-segmentation/dashboard
:hasOAuthError
,apiErrors
andonRetry
as props. Prop names can be adjusted as needed during implementation.ModalDialog
component to render the error modal.buttonLink
prop to theModalDialog
component for OAuth and insufficient permissions errors and other appropriate props.hasOAuthError
is true, render the OAuth error variant of the Error Modal. See the Figma design for details.apiErrors
contains an insufficient permissions error, render the insufficient permissions error variant of the Error Modal. See the Figma design for details.apiErrors
contains any other error, render the generic error variant of the Error Modal. See the Figma design for details.onRetry
prop when the Retry button is clicked within a callback function. The parent component should pass the appropriate retry function (i.e.,enableAudienceGroup
action with thefailedSiteKitAudienceResourceNames
parameter).Update the
AudienceSegmentationSetupCTAWidget
component inassets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js
:enableAudienceGroup
action response.{ error, response }
object from theenableAudienceGroup
action response within theonEnableGroups
callback function.enableAudienceGroup
action.enableAudienceGroup
action.getSetupErrorCode
selector.AudienceErrorModal
component with the appropriate props.onRetry
prop of theAudienceErrorModal
component to retry theenableAudienceGroup
action with the failed audience resource names.skipDefaultErrorNotifications
totrue
while dispatching thesetPermissionScopeError
action.Prevent Rendering Other Setup Errors in
assets/js/components/notifications/ErrorNotifications.js
andassets/js/components/notifications/SetupErrorNotification.js
:temporaryPersistedPermissionsError?.data?.skipDefaultErrorNotifications
is set.skipDefaultErrorNotifications
is set, do not render the error notifications. i.e., return null.ErrorNotifications
:site-kit-wp/assets/js/components/notifications/ErrorNotifications.js
Lines 56 to 61 in 17215a0
Test Coverage
AudienceErrorModal
component.AudienceErrorModal
component.enableAudienceGroup
action.AudienceSegmentationSetupCTAWidget
component.QA Brief
Prerequisites
audienceSegmentation
feature flag.Testing the Audience Error Modal Variants
There are three Audience Error Modal variants to test:
OAuth Error Variant
To simulate the OAuth error variant:
Prepare the Environment:
wp_googlesitekit_additional_auth_scopes
entry from thewp_usermeta
table if it exists. This entry contains thehttps://www.googleapis.com/auth/analytics.edit
scope.Trigger the OAuth Error:
Insufficient Permissions Error Variant
To simulate the Insufficient Permissions error variant:
Install and Configure Tweak Extension:
sync-audiences
request:.*/wp-json/google-site-kit/v1/modules/analytics-4/data/sync-audiences*
(.*)
POST
403
Trigger the Insufficient Permissions Error:
Generic Error Variant
To simulate the Generic Error variant, there are two scenarios:
Other API Errors:
Simulate Sync Available Audiences API Error:
sync-audiences
request:.*/wp-json/google-site-kit/v1/modules/analytics-4/data/sync-audiences*
(.*)
POST
500
Trigger the Generic Error:
2. Audience Creation Failure:* Simulate Audience Creation Failure:- Manually createnew-visitors
andreturning-visitors
audiences in the Analytics console.- Click on the Enable groups button in the Audience Segmentation setup CTA banner.- Ensure the Generic error variant of the Error Modal is displayed.- Delete thenew-visitors
andreturning-visitors
audiences in the Analytics console.- Click the Retry button to ensure the setup continues successfully.Changelog entry
The text was updated successfully, but these errors were encountered: