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

Implement the unhappy paths for the Setup CTA Banner #8134

Closed
41 tasks done
techanvil opened this issue Jan 24, 2024 · 20 comments
Closed
41 tasks done

Implement the unhappy paths for the Setup CTA Banner #8134

techanvil opened this issue Jan 24, 2024 · 20 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 24, 2024

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

  • Having clicked on Enable groups in the Setup CTA Banner, if an error occurs, whether it's an error returned from the OAuth flow, or from an API call made to create the audiences and/or custom dimension, the Error Modal should be displayed (see Update the Modal Dialog component for error and confirmation actions (Storybook) #8110).
    • If an error is returned from the OAuth flow:
      • The OAuth error variant of the Error Modal should be displayed.
      • It should follow the Figma design, with the following copy:
        • Title: Analytics update failed
        • Description: Setup was interrupted because you did not grant the necessary permissions. Get help
      • The Retry button should take the user back to the OAuth flow to retry the setup from that point.
      • The Cancel button should close the modal.
      • The default OAuth error notice that Site Kit usually displays when the OAuth flow returns an error should not be shown.
    • If a permission error is returned by one of the API calls:
      • The "insufficient permissions" variant of the Error Modal should be displayed.
      • It should follow the Figma design, with the following copy:
        • Title: Insufficient permissions
        • Description: You’ll need to contact your administrator. Trouble getting access? Get help
      • The Request access button should open the URL for the currently connected web data stream in the Analytics UI, allowing the user to request access.
      • The Cancel button should close the modal.
    • If any other error is returned by one of the API calls:
      • The generic error variant of the Error Modal should be displayed.
      • It should follow the Figma design, with the following copy:
        • Title: Failed to set up visitor groups
        • Description: Oops! Something went wrong. Retry enabling groups.
      • The Retry button should close the modal, retry the failed API call(s) and continue setup from that point.
      • The Cancel button should close the modal.
  • The Get help links should open the Analytics support page in a new tab, scrolled to the insufficient permissions section.

  • Note that all copy should be verified with Figma as the source of truth.

Implementation Brief

Update the enableAudienceGroup action in assets/js/modules/analytics-4/datastore/actions.js:

  • Update Function (Action) Signature
    • Modify the enableAudienceGroup action to accept an object parameter that includes failedSiteKitAudienceResourceNames - A list of audiences that failed to be created by Site Kit.
  • Return Error Objects
    • Identify all the places in the action where there are TODO comments for error handling.
    • Replace each 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.
  • Handle Site Kit-Created Audience Failures
    • Add a conditional check at the beginning of the audience creation logic to see if failedSiteKitAudienceResourceNames is provided.
    • If failedSiteKitAudienceResourceNames is provided, skip the creation of new audiences and proceed with the logic.
  • Update Audience Creation Logic
    • Modify the existing audience creation logic to handle retries by using the failedSiteKitAudienceResourceNames parameter.
    • Define the audiences to be created based on whether failedSiteKitAudienceResourceNames is provided or use the default list of audiences to create.
  • Handle Creation Results
    • After attempting to create the audiences, track which audiences need to be retried if their creation fails.
    • Use an array (e.g., needsRetry) to store the slugs of the audiences that failed to be created.
    • If any audiences need to be retried, return an object with failedSiteKitAudienceResourceNames containing the slugs of the failed audiences.
  • Syncing the available audiences, updating the configured audiences, and saving the audience settings should be kept as is.
  • Check this commit for reference on a POC branch as a starting point.

Update the ModalDialog component in assets/js/components/ModalDialog.js:

  • Add a new prop buttonLink to handle opening a link in a new tab.
  • If buttonLink is provided, update the SpinnerButton component props to include href and target attributes with the buttonLink and _blank values, respectively.

Create AudienceErrorModal component in assets/js/modules/analytics-4/components/audience-segmentation/dashboard:

  • It should receive hasOAuthError, apiErrors and onRetry as props. Prop names can be adjusted as needed during implementation.
  • Use the ModalDialog component to render the error modal.
  • Pass the buttonLink prop to the ModalDialog component for OAuth and insufficient permissions errors and other appropriate props.
  • OAuth error variant:
  • Insufficient permissions error variant:
    • If apiErrors contains an insufficient permissions error, render the insufficient permissions error variant of the Error Modal. See the Figma design for details.
  • Generic error variant:
    • If apiErrors contains any other error, render the generic error variant of the Error Modal. See the Figma design for details.
    • Call the 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 the failedSiteKitAudienceResourceNames parameter).
  • See ReportErrorActions for an example of how to determine the insufficient permissions error and generic error variants.
  • Figma designs should be the source of truth for copy and design details.

Update the AudienceSegmentationSetupCTAWidget component in assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js:

  • Create a component state to store the API errors.
  • Create a component state to store the failed Site Kit audience resource names from the enableAudienceGroup action response.
  • Get the { error, response } object from the enableAudienceGroup action response within the onEnableGroups callback function.
  • Set the error state based on the response from the enableAudienceGroup action.
  • Set the failed Site Kit audience resource names state based on the response from the enableAudienceGroup action.
  • Check if there is an OAuth error using the getSetupErrorCode selector.
  • If there is an OAuth error or API error, render the AudienceErrorModal component with the appropriate props.
  • Pass a callback function to the onRetry prop of the AudienceErrorModal component to retry the enableAudienceGroup action with the failed audience resource names.
  • Set Skip Error Notification Flag:
    • When triggering the OAuth flow, pass skipDefaultErrorNotifications to true while dispatching the setPermissionScopeError action.

Prevent Rendering Other Setup Errors in assets/js/components/notifications/ErrorNotifications.js and assets/js/components/notifications/SetupErrorNotification.js:

  • Modify the ErrorNotifications and SetupErrorNotification components to check if temporaryPersistedPermissionsError?.data?.skipDefaultErrorNotifications is set.
  • If skipDefaultErrorNotifications is set, do not render the error notifications. i.e., return null.
  • See how the temporary persisted permissions error is retrieved in the ErrorNotifications:
    const temporaryPersistedPermissionsError = useSelect( ( select ) =>
    select( CORE_FORMS ).getValue(
    FORM_TEMPORARY_PERSIST_PERMISSION_ERROR,
    'permissionsError'
    )
    );

Test Coverage

  • Add storybook stories for all the error variants for the AudienceErrorModal component.
  • Add test coverage for all the error variants for the AudienceErrorModal component.
  • Add test coverage for the changes made to the enableAudienceGroup action.
  • Fix any broken stories in the AudienceSegmentationSetupCTAWidget component.
  • Fix any failing tests.

QA Brief

Prerequisites

  1. Setup Analytics and enable the audienceSegmentation feature flag.
  2. Ensure no audiences have been created or configured.
  3. Verify the Audience Segmentation setup CTA banner is displayed.

Testing the Audience Error Modal Variants

There are three Audience Error Modal variants to test:

  1. OAuth error variant.
  2. Insufficient permissions error variant.
  3. Generic error variant.

OAuth Error Variant

To simulate the OAuth error variant:

  1. Prepare the Environment:

    • Delete the wp_googlesitekit_additional_auth_scopes entry from the wp_usermeta table if it exists. This entry contains the https://www.googleapis.com/auth/analytics.edit scope.
  2. Trigger the OAuth Error:

    • Click on the Enable groups button in the Audience Segmentation setup CTA banner.
    • In the OAuth flow, when prompted to grant permissions, cancel the flow.
    • Ensure the OAuth error variant of the Error Modal is displayed.
    • Click the Retry button to trigger the OAuth flow again.
    • Complete the OAuth flow and grant permissions.
    • Ensure the setup continues successfully after granting permissions.

Insufficient Permissions Error Variant

To simulate the Insufficient Permissions error variant:

  1. Install and Configure Tweak Extension:

    • Install the Tweak Chrome extension.
    • Add a new rule to block the sync-audiences request:
      • URL: .*/wp-json/google-site-kit/v1/modules/analytics-4/data/sync-audiences*
      • Enable the use of regular expression (.*)
      • HTTP Method: POST
      • Status code: 403
      • Response payload:
        {
          "code": 403,
          "message": "Insufficient Permissions Test Error",
          "data": {
            "status": 403,
            "reason": "insufficientPermissions"
          }
        }
  2. Trigger the Insufficient Permissions Error:

    • Click on the Enable groups button in the Audience Segmentation setup CTA banner.
    • Ensure the Insufficient Permissions error variant of the Error Modal is displayed.
    • Click on the Request access button and verify the Analytics console opens in a new tab.

Generic Error Variant

To simulate the Generic Error variant, there are two scenarios:

  1. Other API Errors:

    • Simulate Sync Available Audiences API Error:

      • Add a new rule in the Tweak extension to block the sync-audiences request:
        • URL: .*/wp-json/google-site-kit/v1/modules/analytics-4/data/sync-audiences*
        • Enable the use of regular expression (.*)
        • HTTP Method: POST
        • Status code: 500
        • Response payload:
          {
            "code": "internal_server_error",
            "message": "Internal server error",
            "data": { "status": 500 }
          }
    • Trigger the Generic Error:

      • Click on the Enable groups button in the Audience Segmentation setup CTA banner.
      • Ensure the Generic error variant of the Error Modal is displayed.
      • Disable the rule to unblock the request in the Tweak extension.
      • Click the Retry button to ensure the setup continues successfully.

2. Audience Creation Failure:

* Simulate Audience Creation Failure:
- Manually create new-visitors and returning-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 the new-visitors and returning-visitors audiences in the Analytics console.
- Click the Retry button to ensure the setup continues successfully.

Changelog entry

  • Handle errors in the Audience Segmentation setup flow, showing an error modal allowing the setup to be retried, or relevant permissions to be requested.
@techanvil
Copy link
Collaborator Author

techanvil commented Mar 28, 2024

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.

@techanvil
Copy link
Collaborator Author

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.

@ivonac4 ivonac4 added the Team M Issues for Squad 2 label Apr 9, 2024
@techanvil techanvil assigned techanvil and unassigned hussain-t and techanvil Apr 11, 2024
@ivonac4 ivonac4 added Next Up Issues to prioritize for definition Sp Wk 2 Issues to be completed in the second week of the assigned sprint labels May 1, 2024
@hussain-t hussain-t self-assigned this May 2, 2024
@ivonac4 ivonac4 removed Sp Wk 2 Issues to be completed in the second week of the assigned sprint Next Up Issues to prioritize for definition labels May 15, 2024
@hussain-t hussain-t removed their assignment May 23, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label May 24, 2024
@techanvil techanvil self-assigned this May 30, 2024
@techanvil
Copy link
Collaborator Author

Hi @hussain-t, this IB is off to a good start. A few points:

  • I realise the name siteKitCreatedAudienceFailures is taken from the POC, but I would suggest a slightly different name. How about failedSiteKitAudienceResourceNames? The current name suggests the variable contains "failures", i.e. error objects or something like that, rather than the audience names.

  • See ReportErrorActions to determine the insufficient permissions error and generic error variants
  • A minor point but this should say something like "See ReportErrorActions for an example of how to determine the insufficient permissions error and generic error variants" and it would be nice to provide a permalink to the example.

  • Update the enableAudienceGroup action call to pass the siteKitCreatedAudienceFailures parameter.
  • This doesn't sound right in isolation. The current dispatch of enableAudienceGroup() is in response to the setup CTA click. We need a way of triggering it in response to clicking the Retry CTA in the error modal.

  • I am not sure about the relevance/accuracy of this point. The implementation of Implement the OAuth flow for the Setup CTA Banner #8132 will dispatch setPermissionScopeError() in order to trigger the OAuth flow rather than dispatching it when there is an OAuth error. Maybe the point needs to be rephrased?

Prevent Rendering Other Setup Errors in assets/js/components/notifications/ErrorNotifications.js and assets/js/components/notifications/SetupErrorNotification.js

  • Modify the ErrorNotifications and SetupErrorNotification components to check if the autoSubmit property is set for the AUDIENCE_SEGMENTATION_SETUP_FORM key of the CORE_FORMS store. Setting the store is being implemented in Implement the OAuth flow for the Setup CTA Banner #8132.
  • If the autoSubmit property is set, do not render the error notifications. i.e., return null as want to show the AudienceErrorModal component.
  • It would be nice to abstract this a little to future proof it - there will be other scenarios where we want to handle an OAuth error inline and suppress these default handler components. We could consider:
    • Encapsulating the check for the autoSubmit in a selector, which we could then add more conditions to as they arise.
    • Using a separate piece of Redux state as a flag for suppressing these components and toggling this flag in the components which are aware of the autoSubmit.
  • Adding a selector is probably a more straightforward approach, but please see what you think.

@techanvil techanvil assigned hussain-t and unassigned techanvil May 30, 2024
@hussain-t
Copy link
Collaborator

Thanks for your valuable review, @techanvil.

I am not sure about the relevance/accuracy of this point. The implementation of #8132 will dispatch setPermissionScopeError() in order to trigger the OAuth flow rather than dispatching it when there is an OAuth error. Maybe the point needs to be rephrased?

This whole point is unnecessary. So, I went ahead and removed it.

It would be nice to abstract this a little to future proof it - there will be other scenarios where we want to handle an OAuth error inline and suppress these default handler components.

I believe we can encapsulate this when necessary. As a general principle, we can extract reusable code when needed.

Encapsulating the check for the autoSubmit in a selector, which we could then add more conditions to as they arise.

If it involves retrieving the value from the state and returning it, we can do this within the component itself, correct?

Using a separate piece of Redux state as a flag for suppressing these components and toggling this flag in the components which are aware of the autoSubmit.

Do you mean setting a state in the core/forms store or creating a separate partial store with its own setter and getter? If it’s the latter, it might be a bit overkill. Instead, we could simply use the getValue selector inside the components.

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.

@kelvinballoo
Copy link
Collaborator

QA Update ❌

I have tested this and I have a few items to raise:

ITEM 1:
Trigger the OAuth Error:
When the modal appears, the 'Get Help' is highlighted already. Notice the dotted box around it.
By right it should not be focused on.

Screenshot 2024-07-10 at 14 50 45
____________________________________________________________

ITEM 2:
Trigger the OAuth Error:
The modal size is not correct.
It should have width of 386px and not 502px.

Figma: 386 x 224 px Screenshot 2024-07-09 at 21 36 09

Implementation: 502 x 224px
Screenshot 2024-07-09 at 21 36 41


ITEM 3:
Insufficient Permissions Error Variant
The comments from item 1 and 2 are applicable for this variant as well.>
On top of that, I'd like to add that the second sentence should be on a new line.

Figma: Screenshot 2024-07-09 at 21 24 22

Implementation:
Screenshot 2024-07-09 at 21 21 21


ITEM 4:
Generic Error Variant
Size of modal should be 402 instead of 422

Figma: 402 px wide Screenshot 2024-07-10 at 14 26 04

Implementation: 422 px wide
Screenshot 2024-07-10 at 14 26 13


ITEM 5:
Generic Error Variant

  1. Clicking outside of modal makes enable disabled. Video is attached for reference.
  2. With the Tweak extension on, it seems like clicking on 'Retry' doesn't do much. Is that expected?
https://github.com/google/site-kit-wp/assets/125576926/66891b54-8565-4dca-bb1f-0f98b37546a5
____________________________________________________________

ITEM 6:
Generic Error Variant
When checking the scenario 2 for Audience Creation Failure, is that still applicable?
I believe after the changes done through this slack conversation, this is no longer applicable. Can you confirm please?


ITEM 7:
Generic Error Variant
When the error module popped up, I can see the grey background on the cancel button.
Based on figma, there is no background there (unless maybe upon hover?)

Figma: Screenshot 2024-07-10 at 16 00 50

Implementation:
Screenshot 2024-07-10 at 15 57 10

@hussain-t
Copy link
Collaborator

Thanks for your observations, @kelvinballoo.

ITEM 1:
Trigger the OAuth Error:
When the modal appears, the 'Get Help' is highlighted already. Notice the dotted box around it.
By right it should not be focused on.

ITEM 2:
Trigger the OAuth Error:
The modal size is not correct.
It should have width of 386px and not 502px.

ITEM 4:
Generic Error Variant
Size of modal should be 402 instead of 422

ITEM 7:
Generic Error Variant
When the error module popped up, I can see the grey background on the cancel button.
Based on figma, there is no background there (unless maybe upon hover?)

We are using the common ModalDialog component. It seems buttons and links are focused on the modal component, which is consistent across all our models. For more details, explore the ModalDialog stories. If needed, feel free to create a separate ticket.

Regarding sizes, as mentioned earlier, we use the standard ModalDialog component, which adjusts to the content size. You can see examples in the ModalDialog stories.

ITEM 5:
Generic Error Variant
Clicking outside of modal makes enable disabled. Video is attached for reference.
With the Tweak extension on, it seems like clicking on 'Retry' doesn't do much. Is that expected?

That's expected due to the network blocking using the Tweak extension.

ITEM 3:
Insufficient Permissions Error Variant
The comments from item 1 and 2 are applicable for this variant as well.>
On top of that, I'd like to add that the second sentence should be on a new line.

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.

ITEM 6:
Generic Error Variant
When checking the scenario 2 for Audience Creation Failure, is that still applicable?
I believe after the changes done through this slack conversation, this is no longer applicable. Can you confirm please?

It's not applicable. I've updated the QAB.

@wpdarren
Copy link
Collaborator

QA Update: ⚠️

I have flagged my concerns about these UX/UI differences as @kelvinballoo has compared these with figma designs. These designs are either incorrect, or, we need to make some changes in this ticket. Its important that Figma design are accurate as we will be uding these for bug bashing.

@hussain-t is going to sync with @techanvil about this.

@techanvil
Copy link
Collaborator Author

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 ModalDialog component, which takes its width from its content, does seem like the right way to go here. However, I realise that results in a discrepancy with Figma. I have therefore asked @sigal-teller to comment on this particular case in a thread on Figma.

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 ModalDialog in the same way we are elsewhere in the plugin, as specced in the IB. I'd suggest this can pass QA on the above points and we can raise a followup issue to address anything that may come out of Sigal's replies on Figma.

Cc @hussain-t

@hussain-t
Copy link
Collaborator

Thanks so much for the clarification, @techanvil

@hussain-t
Copy link
Collaborator

hussain-t commented Jul 11, 2024

On further note, the ModelDialog component was modified as part of #8110, as mentioned in the AC.

@hussain-t hussain-t removed their assignment Jul 11, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

This has been tested good:

  • The OAuth Error is triggered under the circumstances highlighted.

    Screenshot 2024-07-10 at 15 01 04
  • If a permission error is returned by one of the API calls, the "insufficient permissions" variant of the Error Modal is displayed.

    Screenshot 2024-07-09 at 21 28 35
  • If any other error is returned by one of the API calls, the generic error variant of the Error Modal is displayed.

    Screenshot 2024-07-10 at 15 57 10
  • The Get help links to the Analytics support page in a new tab, scrolled to the insufficient permissions section.

    Screenshot 2024-07-10 at 14 51 50

Moving ticket to Approval.

@hussain-t
Copy link
Collaborator

hussain-t commented Jul 11, 2024

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!

ITEM 5:
Generic Error Variant

  1. Clicking outside of modal makes enable disabled. Video is attached for reference.

kuasha420 added a commit that referenced this issue Jul 11, 2024
…-follow-up

Enhance/#8134 - Fix issue where closing the modal does not re-trigger it (follow-up)
@kuasha420
Copy link
Contributor

Thanks @hussain-t, PR is now merged. Back to you @kelvinballoo for another pass. :)

@kelvinballoo
Copy link
Collaborator

QA Update ✅

This has been tested good after the latest fix:

  • The OAuth Error is triggered under the circumstances highlighted.

    Screenshot 2024-07-10 at 15 01 04
  • If a permission error is returned by one of the API calls, the "insufficient permissions" variant of the Error Modal is displayed.

    Screenshot 2024-07-09 at 21 28 35
  • If any other error is returned by one of the API calls, the generic error variant of the Error Modal is displayed.

    Screenshot 2024-07-10 at 15 57 10
  • The Get help links to the Analytics support page in a new tab, scrolled to the insufficient permissions section.

    Screenshot 2024-07-10 at 14 51 50

Only issue found was about a hover effect which is not applicable here. Raised a ticket under #9007

Moving this ticket to Approval.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Team M Issues for Squad 2 Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants