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 full-tile error states for the Audience Tile #8147

Closed
31 tasks done
techanvil opened this issue Jan 24, 2024 · 25 comments
Closed
31 tasks done

Implement the full-tile error states for the Audience Tile #8147

techanvil opened this issue Jan 24, 2024 · 25 comments
Assignees
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 error handling for Audience Tiles for the cases where an error occurs when retrieving the audience data.

Note that this entails showing the Audience Tile Error when an individual tile is in the error state, or the Full Width Error Banner in the case where all audience tiles are in an error state.

See audience tiles > error states in the design doc.


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

Acceptance criteria

  • If an error occurs while retrieving the data for an audience, while other audiences in the selection have their data retrieved successfully:
    • The Audience Tile Error should be displayed instead of the Audience Tile for that audience, in the appropriate variant.
      • If a permission error is returned by one or more of the reports, the "insufficient permissions" variant of the error banner will be shown (see the AC for Add the Audience Tile Error (Storybook) #8228).
      • Otherwise, the catch-all variant will be shown. Clicking the Retry button will retry the failed report(s).
  • In the case where an error occurs while retrieving data for all of the selected audiences:
    • The Full Width Error Banner should be displayed in the appropriate variant.
      • If a permission error is returned by one or more of the reports, the "insufficient permissions" variant of the error banner will be shown (see the AC for Add the Full Width Error Banner (Storybook) #8230).
      • Otherwise, the catch-all variant will be shown. Clicking the Retry button will retry the failed report(s).
    • The Audience Tiles and Info Notice should not be displayed.
  • Note that error handling for syncing the audience selection will be handled separately, via Implement the “no audiences” banner error state #8190.

Implementation Brief

In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/AudienceTiles.js:

  • Retrieve all the following report errors using the getErrorForSelector by passing the appropriate report options:
    • For all tiles (these reports apply to all tiles and should be handled collectively):
      • reportOptions
      • totalPageviewsReportOptions
    • For individual tiles (these reports apply to individual tiles and should be checked per audienceResourceName):
      • topCitiesReportOptions
      • topContentReportOptions
      • topContentPageTitlesReportOptions
  • Create two local states to store errors:
    • individualTileErrors: To track errors for each specific audience tile, especially for the per-tile reports (topCitiesReportOptions, topContentReportOptions, topContentPageTitlesReportOptions).
    • allTilesError: To determine if errors occur in the reports that apply to all tiles collectively (reportOptions and totalPageviewsReportOptions), indicating that a full-width error banner should be displayed.
  • Use a useEffect hook to manage the error handling process:
    • Loop through the configuredAudiences array.
    • For each audienceResourceName, check if there are any errors associated with the per-tile reports (topCitiesReportOptions, topContentReportOptions, topContentPageTitlesReportOptions).
    • Collect and store any found errors in the individualTileErrors state, mapping each audienceResourceName to its corresponding errors.
    • Separately, check for errors in the collective reports (reportOptions and totalPageviewsReportOptions). If errors are found in these, set allTilesError to true.
    • After collecting all errors, if every configuredAudiences entry has associated errors for the per-tile reports, or if there are errors in the collective reports, set allTilesError to true.
  • In the rendering logic:
    • Conditionally render the AudienceSegmentationErrorWidget component if allTilesError is true, passing in all collected errors:
      • Use Object.values( individualTileErrors ).flat() to consolidate all errors into a single, flat array.
      • Combine these with any errors from the collective reports (reportOptions, totalPageviewsReportOptions) and pass this combined array to the AudienceSegmentationErrorWidget component as the errors prop.
    • If allTilesError is false, and if an error exists in individualTileErrors, render the AudienceTileError component within the Widget directly within AudienceTiles (inside visibleAudiences.map()), passing the errors prop.
    • If no error exists, render the AudienceTile component.
  • Lift the AudienceTileLoading logic into AudienceTiles:
    • Move the AudienceTileLoading logic into AudienceTiles (inside visibleAudiences.map()) and conditionally render AudienceTileLoading directly in AudienceTiles based on the loading state before checking for errors or rendering the AudienceTile component.

In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceTilesWidget/AudienceTile/index.js:

  • Remove the AudienceTileLoading component from the AudienceTile component as it will now be handled in AudienceTiles.
  • As we are now handing the loading state in AudienceTiles, remove the unnecessary props.

Hide the InfoNoticeWidget component when an error occurs:

In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationErrorWidget.js:

  • Within a useEffect hook, set the global state to hide the InfoNoticeWidget using the setValue action from the CORE_UI store.
  • Implement the handleRetry callback function to reset the state (using setValue from CORE_UI) to show the InfoNoticeWidget again once the errors are cleared.
  • Pass the handleRetry function to the onRetry prop of the ReportErrorActions component.

In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/InfoNoticeWidget/index.js:

  • Retrieve the global state value which was set in AudienceSegmentationErrorWidget to hide the InfoNoticeWidget component.
  • Do not render the InfoNoticeWidget component if the global state value is set to true.
  • Otherwise, render the InfoNoticeWidget component as usual.

Test Coverage

  • Add storybook stories for the individual and full-width error states in AudienceTilesWidget/index.stories.js and AudienceTile/index.stories.js.
  • Add test coverage for the individual and full-width error states in AudienceTilesWidget/index.test.js.
  • Fix any broken stories.
  • Fix any broken tests.

QA Brief

Note:

  • Make sure https://oi.ie is added as "Custom Site URL" in tester settings.

  • It may happen that the IDs given for Returning visitors and All visitors may get changed in case the audiences have been archived and re-created in Google Analytics. In this QAB, ID for Returning visitors is 8587716470 and All visitors is 2786548370 . It's better to run googlesitekit.data.select( 'modules/analytics-4' ).getAvailableAudiences(); command in browser and confirm the IDs for these audiences and replace with the correct one in case those are changed.

  1. Enable the audience segmentation feature and enable Visitor groups in Site Kit settings.

  2. Select Returning visitors and All visitors to show in audience tiles widget area from the selection panel.

  3. Using the Tweak Chrome extension, create a rule as follows.

Rule 1

  • URL: .*/wp-json/google-site-kit/v1/modules/analytics-4/data/report.*8587716470.*
  • Enable the use of regular expressions (.*)
  • HTTP Method: GET
  • Status code: 500
  • Response payload.
  {
    "code": 500,
    "message": "Some Error",
    "data": {
      "status": 500,
      "reason": "Some Reason"
    }
  }

Rule 2

  • URL: .*/wp-json/google-site-kit/v1/modules/analytics-4/data/report.*8587716470(?!.*2786548370).*
  • This will ensure to return error only when the data is requested for 8587716470 only and no other audience resource name (2786548370) is present in the request
  • Enable the use of regular expressions (.*)
  • HTTP Method: GET
  • Status code: 500
  • Response payload.
  {
    "code": 500,
    "message": "Some Error",
    "data": {
      "status": 500,
      "reason": "Some Reason"
    }
  }
  1. In Applications > Session storage (in chrome) search for report and clear all report data.

Single tile error

  • Enable rule 2 above in tweak and refresh the dashboard, make sure there are no reports in session storage so that request can be intercepted.

  • Notice that Returning visitors tile should show error.

  • All visitors tile should show the data properly.

All tiles error

  • Enable rule 1 above in tweak and refresh the dashboard, make sure there are no reports in session storage so that request can be intercepted.

  • Notice that full width error banner is shown and no tile will be shown.

  • Also, info notice banner Add the Info Notice (Storybook) #8137 is not shown when full width error banner is being displayed.

Changelog entry

  • Show an Audience Tile in an error state if an error occurred while retrieving its data; show a combined error state if all audiences have an error.
@techanvil techanvil added Module: Analytics Google Analytics module related issues P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jan 24, 2024
@techanvil
Copy link
Collaborator Author

Note, I've moved this to the backlog pending the addition of the full width error state as a GitHub issue, which this issue may depend on.

@techanvil techanvil changed the title Implement the full-tile error states for the Audience Tile Implement the error states for the Audience Tile Feb 6, 2024
@techanvil techanvil changed the title Implement the error states for the Audience Tile Implement the full-tile error states for the Audience Tile Feb 6, 2024
@techanvil
Copy link
Collaborator Author

The full width error state is now defined and added as a dependency for this issue, which is ready for AC.

@hussain-t hussain-t self-assigned this Mar 25, 2024
@techanvil
Copy link
Collaborator Author

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 again @hussain-t! 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 Jul 25, 2024
@techanvil techanvil removed their assignment Jul 25, 2024
@ivonac4 ivonac4 added the Next Up Issues to prioritize for definition label Aug 2, 2024
@hussain-t hussain-t self-assigned this Aug 5, 2024
@hussain-t hussain-t removed their assignment Aug 12, 2024
@techanvil techanvil self-assigned this Aug 13, 2024
@techanvil
Copy link
Collaborator Author

Thanks @hussain-t, nice work here so far on the IB. A couple of points:

  • Retrieve all the following report errors using the getErrorForSelector by passing the appropriate report options:
    • reportOptions
    • totalPageviewsReportOptions
    • topCitiesReportOptions
    • topContentReportOptions
    • topContentPageTitlesReportOptions

It's worth bearing in mind that reportOptions and totalPageviewsReportOptions are both for reports that apply to all tiles rather than per-tile. We should clarify this in the IB and consider how they will need to be handled differently to the other reports.

  • Modify the AudienceTile component to accept a new error prop.
  • Conditionally render the AudienceTileError component with in the Widget when an error is present.
  • Pass the error prop to the AudienceTileError component.

Hmm, I think it would be cleaner to conditionally render AudienceTileError directly in AudienceTiles rather than in AudienceTile. That way we can return early in the map over visibleAudiences rather than going through all the irrelevant logic of extracting the various data points (and running the various unneeded useSelect() calls in AudienceTile). We might want to lift the render of AudienceTileLoading out to AudienceTiles at some point for the same reason.

@techanvil techanvil assigned hussain-t and unassigned techanvil Aug 13, 2024
@hussain-t hussain-t assigned techanvil and unassigned hussain-t Aug 14, 2024
@hussain-t
Copy link
Collaborator

Thanks for the valuable feedback, @techanvil. I've updated the IB accordingly.

@techanvil
Copy link
Collaborator Author

Thanks @hussain-t! Nice work, that LGTM.

IB ✅

@techanvil techanvil removed their assignment Aug 14, 2024
@techanvil
Copy link
Collaborator Author

Hey @hussain-t, while working on the IB for #8190 I've noticed that one detail of the AC has been missed in the IB here.

  • In the case where an error occurs while retrieving data for all of the selected audiences:
    • ...
    • The Audience Tiles and Info Notice should not be displayed.

As per the above point, we need to ensure the Info Notice is not displayed when we are showing the Full Width Error Banner. Please can you update the IB accordingly?

@benbowler benbowler assigned benbowler and unassigned benbowler Aug 30, 2024
@nfmohit nfmohit assigned nfmohit and techanvil and unassigned nfmohit Sep 3, 2024
@techanvil techanvil assigned nfmohit and unassigned techanvil Sep 4, 2024
@nfmohit nfmohit removed their assignment Sep 5, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ⚠️

Thanks @techanvil
This has been verified good overall. Just need your confirming on ITEM 1.

ITEM 1 ⚠️
For item 1, I re-tested this and noticed that you have put the tiles in loading state. Agreed on that.
Just confirming as well that, it's fine for the 'Info notice' to appear below them, because it's part of the loading state?

Screenshot 2024-09-05 at 13 00 54

ITEM 2 ✅
Noted on that. I was able to do the proper change in the URL and I could interchange the positions.
Looking as expected.

Screenshot 2024-09-05 at 12 53 52

ITEM 3 ✅
The tiles on mobile and desktop are appearing as expected now.

2/3 tiles present: Screenshot 2024-09-05 at 12 48 01 Screenshot 2024-09-05 at 12 48 21

1 tile only present:
Screenshot 2024-09-05 at 12 46 44

Screenshot 2024-09-05 at 12 46 36

@techanvil
Copy link
Collaborator Author

ITEM 1 ⚠️ For item 1, I re-tested this and noticed that you have put the tiles in loading state. Agreed on that. Just confirming as well that, it's fine for the 'Info notice' to appear below them, because it's part of the loading state?

Screenshot 2024-09-05 at 13 00 54

Thanks @kelvinballoo, this is indeed expected for now. We do have an issue to only show the info notice when there is relevant data for it, which implies it won't show up while the tiles are in the loading state, but this is currently in the backlog for post-launch:
#8140

@techanvil techanvil removed their assignment Sep 5, 2024
@kelvinballoo
Copy link
Collaborator

QA Update ✅

Great, thanks for the confirmation @techanvil .
Moving this ticket to approval.


I was able to do the proper change in the URL and I could interchange the positions. ✅
Looking as expected.

Screenshot 2024-09-05 at 12 53 52

The tiles on mobile and desktop are appearing as expected. ✅

2/3 tiles present: Screenshot 2024-09-05 at 12 48 01 Screenshot 2024-09-05 at 12 48 21

1 tile only present:
Screenshot 2024-09-05 at 12 46 44

Screenshot 2024-09-05 at 12 46 36

The errors are showing up accordingly. ✅

Rule 2 active: Screenshot 2024-08-29 at 15 38 46

Rule 1 Active:
Screenshot 2024-08-29 at 15 43 50

@kelvinballoo kelvinballoo removed their assignment Sep 5, 2024
@techanvil techanvil assigned techanvil and unassigned techanvil Sep 5, 2024
@aaemnnosttv
Copy link
Collaborator

@kelvinballoo it looks like only the catch-all error case was tested here. Was the permissions error case also tested as mentioned in the AC?

@kelvinballoo
Copy link
Collaborator

@aaemnnosttv , apologies! Yes, only the catch-all error was tested. Missed out on the permissions error.

@ankitrox , could you update the QAB for how to test for the Permissions error please?

@kelvinballoo
Copy link
Collaborator

QA Update ✅

Permissions error variant tested by changing the tweak extension response to:

{
  "code": 403,
  "message": "Insufficient Permissions Test Error",
  "data": {
    "status": 403,
    "reason": "insufficientPermissions"
  }
}

The Permission error variant appeared accordingly and is looking good on desktop, mobile and tablet viewports.

  • All tiles error ✅

    Screenshot 2024-09-05 at 20 58 45 Screenshot 2024-09-05 at 20 59 00 Screenshot 2024-09-05 at 21 06 36
  • One tile error: ✅

    Screenshot 2024-09-05 at 21 01 00 Screenshot 2024-09-05 at 21 01 42 Screenshot 2024-09-05 at 21 01 56

The catch-all variant was tested good already ✅

1 tile error: Screenshot 2024-09-05 at 12 48 01 Screenshot 2024-09-05 at 12 48 21 Screenshot 2024-09-05 at 21 14 04

All tiles error:
Screenshot 2024-09-05 at 12 46 44

Screenshot 2024-09-05 at 12 46 36 Screenshot 2024-09-05 at 21 13 29

@aaemnnosttv
Copy link
Collaborator

Thanks @kelvinballoo !

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

8 participants