site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Implement the unhappy path for Selection Panel audience count retrieval (error notice above footer)

Open techanvil opened this issue 1 year ago • 11 comments

Feature Description

Implement the error states that can occur while resyncing the cached audience and retrieving the audience user counts for the Selection Panel.

See selection panel > error states in the design doc.


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

Acceptance criteria

  • When loading the Selection Panel, if an error occurs as a result of resyncing the cached audience list or retrieving the audience user counts:
    • The user counts should be displayed as dashes.
    • The list of audiences will be displayed on a best-effort basis, i.e. if the resync fails the existent cached state of the audiences will be used.
    • An error notice should be displayed above the footer. This should be implemented as per the Figma design.
    • If the error is a permissions error, the "insufficient permissions" variant of the error notice will be shown, including a Get help link and a Request access buttons.
    • Otherwise, the catch-all variant will be shown which just includes the Retry button.
      • Clicking the Retry button will reattempt the failed resync and/or re-request the user count report(s).

Implementation Brief

Note that this IB will need an update to reflect the audience caching related changes that have been made to the AC (i.e. the new AC points which mention resyncing the cached audiences).

  • [ ] Once #8158 and #8157 are merged.

Error Notice Component

  • [ ] Create an ErrorNotice component within the Audience Segmentation Selection Panel component directory with the following:
    • [ ] It should receive a list of errors as a prop.
    • [ ] Check for "insufficient permissions" errors using the isInsufficientPermissionsError helper function.
    • [ ] Render the Data loading failed copy if there are no permissions errors.
    • [ ] Render the ReportErrorActions component with the following props, which will render the error variant-specific buttons appropriately (See AudienceSegmentationErrorWidget):
      • moduleSlug - pass the analytics-4 module slug.
      • error - pass the errors.
      • hideRetryHelpLink - a boolean prop to hide the Get help link if the error is not a permissions error.
      • GetHelpLink - pass a custom GetHelpLink component that will render the Insufficient permissions, contact your administrator. Trouble getting access? copy with the Get help link similar to the AudienceTile/GetHelpLink.js component.
      • getHelpClassName - custom class name for the Get help link.
    • [ ] Adjust the styles that match the Figma design.

Displaying the Error Notice

  • [ ] In the Audience Segmentation Selection Panel's Footer component:
    • Resyncing cached audiences error:
      • [ ] If an error occurs while resyncing the cached audiences from the getAvailableAudiences selector.
      • [ ] Use the getErrorForSelector selector to determine if there are errors in the resyncing process.
      • [ ] If there are errors, render the ErrorNotice component and pass the errors as a prop.
    • Retrieving audience user counts error:
      • [ ] Check for getReport errors using the getErrorForSelector selector to determine if there are errors in the user counts.
      • [ ] Use the appropriate query args to determine the errors for the user counts.
      • [ ] If there are errors, render the ErrorNotice component and pass the errors as a prop.
      • [ ] If there are errors, display the user counts as dashes in the appropriate child component, which is being implemented in #8157.

Test Coverage

  • [ ] Add storybook stories for the ErrorNotice component.
  • [ ] Add storybook stories for the selection panel component with the error notice displayed.
  • [ ] Add basic test coverage for the ErrorNotice component.
  • [ ] Add test coverage for the audience selection panel component with the error notice displayed.

QA Brief

  • In Storybook, verify the new Modules > Analytics4 > Audience Segmentation > Dashboard > AudienceSelectionPanel > Insufficient permissions error, Audience sync error, and User count retrieval error story variants and ensure they are on par with the Figma designs.

    • The gap between the error notice and the learn more text is intentionally kept 20px (instead of 19px as shown in Figma). 20 pixels was the pre-defined spacing for the learn more link and it is maintained for consistency.
  • Set up Site Kit with the Analytics module, having a property that includes a few audiences for testing (any Site Kit audience shouldn't be in a partial data state, the case for Site Kit audiences with partial data states will be handled in #8923).

  • Enable the audienceSegmentation feature flag.

  • Open the audience selection panel.

  • Resync available audiences error:

    • To simulate an insufficient permissions error, try the following script in the browser console:
    onst error = {
    	code: 'test_error',
       message: 'Error message.',
    	data: { reason: 'insufficientPermissions' },
    ;
    
    ooglesitekit.data.dispatch( 'modules/analytics-4' ).receiveError( error, 'syncAvailableAudiences' );
    
    • To simulate a data loading failed error, try the following script in the browser console:
    onst error = {
    	code: 'test_error',
    	message: 'Error message.',
    	data: {},
    ;
    
    ooglesitekit.data.dispatch( 'modules/analytics-4' ).receiveError( error, 'syncAvailableAudiences' );
    
    • If you now click on the "Retry" button, you should be able to see in the "Network" tab that a request to sync-audiences is made.
  • User count retrieval error:

    • To simulate an insufficient permissions error, try the following script in the browser console:
    onst { getConfigurableAudiences, getAudiencesUserCountReportOptions } = googlesitekit.data.select( 'modules/analytics-4' );
    
    onst error = {
       code: 'test_error',
       message: 'Error message.',
       data: { reason: 'insufficientPermissions' },
    ;
    
    onst configurableAudiences = getConfigurableAudiences();
    
    onst reportOptions = getAudiencesUserCountReportOptions( configurableAudiences )
    
    ooglesitekit.data.dispatch( 'modules/analytics-4' ).receiveGetReport( false, { options: reportOptions } );
    ooglesitekit.data.dispatch( 'modules/analytics-4' ).receiveError( error, 'getReport', [ reportOptions ] );
    
    • To simulate a data loading failed error, try the following script in the browser console:
    onst { getConfigurableAudiences, getAudiencesUserCountReportOptions } = googlesitekit.data.select( 'modules/analytics-4' );
    
    onst error = {
       code: 'test_error',
       message: 'Error message.',
       data: {},
    ;
    
    onst configurableAudiences = getConfigurableAudiences();
    
    onst reportOptions = getAudiencesUserCountReportOptions( configurableAudiences )
    
    ooglesitekit.data.dispatch( 'modules/analytics-4' ).receiveGetReport( false, { options: reportOptions } );
    ooglesitekit.data.dispatch( 'modules/analytics-4' ).receiveError( error, 'getReport', [ reportOptions ] );
    
    • Now, clear your browser's session storage.
    • Now, if you click on "Retry", you should be able to see in the browser's network tab that a request to report.
    • In both cases above, the audience count should change to a "dash" (-).
  • Verify the behaviour of "Get help", "Request access", and "Retry" buttons according to ACs.

Changelog entry

  • If an error occurs while loading the Audience Selection Panel, show it an an error state, with a notice that allows the data loading to be retried.

techanvil avatar Jan 25 '24 11:01 techanvil

IB ✔️

eugene-manuilov avatar Mar 26 '24 14:03 eugene-manuilov

I've moved this back to Backlog as the final in-progress changes to the design doc, relating to audience caching, might affect the AC/IB for this one.

techanvil avatar Mar 28 '24 14:03 techanvil

The audience caching aspect of the design doc has been sufficiently finalised, I've updated the AC and moved this back to IB, as the existing IB will need an update.

Cc @hussain-t

techanvil avatar Apr 05 '24 14:04 techanvil

Thanks, @techanvil. I've updated the IB and assigned it to you for review.

hussain-t avatar May 02 '24 13:05 hussain-t

Thanks @hussain-t - let's keep this assigned to me, but don't forget that as co-lead on the epic @kuasha420 can also review the IBs so they can generally be left unassiged.

techanvil avatar May 02 '24 14:05 techanvil

Hi @hussain-t, thanks for drafting this IB. A few points:

  • It's worth noting that the IB for #8157 hasn't been finalised, and is subject to change (in fact it's due a big revision as the issue has been split into two now, with the initial refactor to be undertaken via #8652). One thing I think will change that is referenced here is the name AudienceSegmentationPanel, which I suspect we'll name AudienceSelectionPanel. It's also not entirely clear how the footer will be structured at this point. It's likely that the Audience Selection Panel will have its own footer component which wraps a generic one. We might be able to modify the footer, or it could be appropriate to render the error component above the footer. It's probably worth waiting until the IB for #8157 has been approved before we finalise this one, or another approach could be to make this one a bit less specific where we don't know the details yet.

* [ ] Mirror most of the logic from the `ReportErrorActions` component to determine the error variant and render the appropriate buttons.
  • I wonder if it's worth extracting the logic for reuse rather than copying it, it seems a shame to duplicate this logic if we can sensibly avoid it.

  • * [ ] Use the `totalUsers` metric and the `audienceResourceName` dimension as report args to query the `getReport` selector errors.
    
  • It's worth noting there will be additional report args, it's quite likely there will be dimensionFilters applied too. Maybe we can simply do without this point, it should be clear at implementation time that the report args need to be the same as those for the related report.

techanvil avatar May 03 '24 10:05 techanvil

Thanks for clarifying, @techanvil. It's good to have less specific information about the component names in IB and move it forward. I've updated the IB accordingly.

hussain-t avatar May 03 '24 13:05 hussain-t

Thanks @hussain-t! IB :white_check_mark:

techanvil avatar May 03 '24 13:05 techanvil

QA Update ⚠️

Tested this and I have a few items to raise and clarify:

ITEM 1: Padding around text on the side and above should be 16px instead of 14

Implementation: Screenshot 2024-07-02 at 17 05 23

Figma: Screenshot 2024-07-02 at 17 06 13

________________________________________________________________________________

ITEM 2: Currently, the 'Retry' and 'Request Access' CTAs are more like underlined text. I can't really tell from the design on the expectation but I feel that having a button would be more consistent with other parts of the site. Raising for review.

'Retry' is underlined. Screenshot 2024-07-02 at 17 08 32

ITEM 3: Currently, if I click on 'Request Access', I will be directed to the connected web data stream in the Analytics UI in a new tab. But there isn't anything really to 'Request Access'. The AC states : "button will open the URL for the connected web data stream in the Analytics UI, allowing the user to request access." but I can't see any modal to request access per se.

What should I request for 'allowing the user to request access'?

Refer to the video below:

https://github.com/google/site-kit-wp/assets/125576926/6796f176-669c-4d89-8775-7f2b2c2cc0b2

ITEM 4: Using the last script in the QAB and clicking on 'Retry' doesn't seem to yield any Report request in the Network tab. Nothing seems to happen.

https://github.com/google/site-kit-wp/assets/125576926/2a609efa-2e8e-4962-92e4-1c36f96d366d

kelvinballoo avatar Jul 02 '24 13:07 kelvinballoo

Hi @kelvinballoo . Thank you for sharing your observations. Please take a look at my comments below:

ITEM 1: Padding around text on the side and above should be 16px instead of 14

I am not sure which exact mock you're looking at, but I'm on the Figma link referenced in the AC, and I see that the vertical padding is 14px. Please see screenshot:

image

ITEM 2: Currently, the 'Retry' and 'Request Access' CTAs are more like underlined text. I can't really tell from the design on the expectation but I feel that having a button would be more consistent with other parts of the site. Raising for review.

According to the Figma designs, they appear as links and not buttons. See:

image

Links in Site Kit automatically have an underline when hovered or focused. I also don't think we'd be able to easily accommodate a button in such a minimal space. You may want to raise this internally with the UX team and open a follow-up issue for improvement if necessary.

ITEM 3: Currently, if I click on 'Request Access', I will be directed to the connected web data stream in the Analytics UI in a new tab. But there isn't anything really to 'Request Access'. The AC states : "button will open the URL for the connected web data stream in the Analytics UI, allowing the user to request access." but I can't see any modal to request access per se.

What should I request for 'allowing the user to request access'?

The respective action doesn't appear in the Analytics UI because you actually have access to the resource, but the script in the QAB makes Site Kit think that you don't, thus showing the error. You can try to reproduce this with an actual property that you don't have access to, but I don't think such an overhead will be necessary as insufficient permissions is just a generic error case we handle in most scenarios.

ITEM 4: Using the last script in the QAB and clicking on 'Retry' doesn't seem to yield any Report request in the Network tab. Nothing seems to happen.

I tested this and it appears that this will only work if any of the available Site Kit audience isn't in a partial data state. I've tested and it appears that in case of oi.ie, the Site Kit audiences are in a partial data state. This will be addressed as part of #8923.


Please let me know if you have any other questions, thank you!

nfmohit avatar Jul 03 '24 14:07 nfmohit

QA Update ✅

Thanks @nfmohit ,

ITEM 1 While I could see the relative distance as 16px from non-dev version of figma, once I got access to dev mode, I can now see it as 14px top and bottom. ✅

Screenshot 2024-07-05 at 17 01 48
____________________________________________________

ITEM 2: Thanks for the context. I noticed the underline and hence this is good to go. ✅


ITEM 3: Noted on this.


ITEM 4: Noted. Will test this under https://github.com/google/site-kit-wp/issues/8923.

3 error screenshots are below including with dash. Screenshot 2024-07-02 at 16 51 25 Screenshot 2024-07-02 at 16 54 45 Screenshot 2024-07-02 at 16 56 18

Moving ticket to approval.

kelvinballoo avatar Jul 05 '24 13:07 kelvinballoo