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 ability to create custom dimensions from metric tiles #7601

Closed
nfmohit opened this issue Sep 20, 2023 · 19 comments
Closed

Implement ability to create custom dimensions from metric tiles #7601

nfmohit opened this issue Sep 20, 2023 · 19 comments
Labels
P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Sep 20, 2023

Feature Description

A metric tile dependent on custom dimensions should use a new wrapper/utility that goes on to create the custom dimension(s) it depends on.

See:

  1. Relevant section in the design doc.
  2. Figma designs for error states.

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

Acceptance criteria

  • A new wrapper component should be created for metric tiles that depend on custom dimensions that should act like the following:
    • If the required custom dimensions are available:
      • The wrapper should let the metric tile make the appropriate report request and display the report accordingly.
      • If the report request fails with an INVALID_ARGUMENT error.status, the locally persisted list of available custom dimensions might not be up to date and that should be updated. This update should be triggered only once so that multiple metric tiles do not attempt to update this at the same time.
    • If the required custom dimensions are not available:
      • The metric tile should show a loading state if the custom dimensions are being created in the background.
      • The metric tile should show an error state if the custom dimension creation for the current tile fails:
        • If the error occurs due to the user not having sufficient permissions for the GA property, the metric tile should show an error state for “Insufficient permissions” according to the Figma designs:
          • Heading: Insufficient permissions
          • Text: You’ll need to contact your administrator. Learn more (link) <br> Permissions updated? Retry (link)
          • Clicking on Learn more should take the user to the Site Kit support URL with an error_id of analytics-4_insufficient_permissions.
          • Clicking on Retry should attempt to create the custom dimensions again.
          • Screenshot for reference:
            image
        • If the error occurs due to something generic, the metric tile should show an error state with a generic error message according to the Figma designs:
          • Heading: Analytics update failed.
          • Text: Retry didn't work? Get help (link)
          • CTA: Retry
          • Clicking on Get help should take the user to the Site Kit support URL with an error_id of the encountered error code.
          • Clicking on Retry should attempt to create the custom dimensions again.
          • Screenshot for reference:
            image
      • Otherwise, it should be checked if the user has the Analytics “edit” scope.
        • If the user doesn’t have the Analytics “edit” scope:
          • The metric tile should show an error state according to the Figma designs:
            • Heading: No data to show
            • Text: Update Analytics to track metric
            • CTA: Update
            • Clicking on Update should direct the user to the OAuth flow to grant the Analytics “edit” scope in such a way so that Site Kit automatically creates the custom dimensions after they return. This behaviour should already be partially implemented in #7599 and the same should be adopted here as well.
        • If the user has the Analytics “edit” scope (or they just granted it) and Site Kit doesn't automatically create the custom dimensions, the metric tile should show an error state according to the Figma designs:
          • Heading: No data to show
          • Text: Update Analytics to track metric
          • CTA: Update
          • Clicking on Update should trigger the creation of custom dimensions.
          • Screenshot for reference:
            image
    • Until an error scenario, a gathering/zero data state, or a report is displayed, the metric tile should show a consistent loading state. It should be ensured that there are no flickers between different states.
  • This wrapper should be a drop-in component so that it automatically handles the creation of custom dimensions and relevant error scenarios. The issues implementing the metric tiles (#7603, #7605, #7607) should be able to use this wrapper effortlessly and not have to worry about creating the custom dimensions and handling errors.

Implementation Brief

  • Create assets/js/components/KeyMetrics/CustomDimensionTileWrapper
    • Add CustomDimensionErrorActions.js
      • Use this markup as starting point -
        <span className="googlesitekit-error-retry-text">
        { createInterpolateElement(
        __(
        'Retry didn’t work? <HelpLink />',
        'google-site-kit'
        ),
        {
        HelpLink: (
        <Link
        href={ errorTroubleshootingLinkURL }
        external
        >
        { __( 'Get help', 'google-site-kit' ) }
        </Link>
        ),
        }
        ) }
        </span>
      • Include props to handle different variations of content and link/callback function by adapting different output of the referenced code.
      • When learn more is passed it should render text and link/callback fn as defined in AC for Insufficient permissions tile, as well as prop for inline retry defined on the same tile
      • When get help is present it should render text and link defined in AC for generic error
      • Button callback function and label can be more dynamic to adapt per need when passed as a props
    • Add main error component CustomDimensionError.js
      • Use assets/js/components/KeyMetrics/MetricTileError/index.js as starting point
      • Move current title value to the variable so it can be adapted to different messages
      • Create callback functions to handle granting the EDIT_SCOPE permission (Example in #7599), and to create custom dimensions by dispatching createCustomDimensions action introduced in #7598
      • Use CustomDimensionErrorActions component for content output
      • You can use variables to determine which components will be rendered in CustomDimensionErrorActions, and which callback to use for links/button
      • User having/or not having EDIT_SCOPE should modify error output with content defined in design and AC for that scenario
      • In case of hasInsufficientPermissionsError title and content should match Insufficient permissions tile in AC
    • Add main tile wrapper component index.js
      • Include props to accept required custom dimensions and reportOptions
      • You can use hasCustomDimensions selector to determine if custom dimensions are available
      • Check for potential errors with getErrorForSelector on the report made by child component, using reportOptions prop
      • In useMount or useEffect hook, do early check if error.status is INVALID_ARGUMENT and if  getAvailableCustomDimensions is not fetching data.
        • And In that case dispatch syncAvailableCustomDimensions action, which will in turn create missing custom dimensions and sync the availableCustomDimensions state. Just implement isSyncing selector, and setIsSyncing controller, in that store to lock syncAvailableCustomDimensions action by returning early if it isSyncing, to prevent multiple calls at the same time.
      • Check if metrics are being created, to induce the loading state. You can use isFetchingCreateCustomDimension, or perhaps hasFinishedResolution on the getReport. And to unify it with child component loading, you can add additional loading prop and combine it in this check
      • On loading return MetricTileLoader component, with necessary wrappers to keep the styling, you can get them from MetricTileNumeric component for example.
      • On error that is not due to INVALID_ARGUMENT, return CustomDimensionError component
      • Otherwise children should be returned

Here is a summary of the component hierarchy for clarity:

  1. Custom dimensions don't exist and are not being created: CustomDimensionTileWrapper > CustomDimensionError component (with prompt to update Analytics).
  2. Custom dimensions being created: CustomDimensionTileWrapper > MetricTileLoader.
  3. Custom dimensions creation failed: CustomDimensionTileWrapper > CustomDimensionError component (with relevant error).
  4. Custom dimensions exist: CustomDimensionTileWrapper > Child Tile component.

Storybook

  • Add Storybook stories for the CustomDimensionError component including its various variants.

Test Coverage

  • Add JS tests for the wrapper component.
  • Add VRT scenarios for added Storybook stories.

QA Brief

  • Set up Site Kit and connect the Analytics module.
  • Enable the newsKeyMetrics feature flag.
  • In the appearing KMW banner notification, choose to pick up your own metrics and select the "Top recent trending pages", "Top categories by pageviews", and "Most popular authors by pageviews" metrics to show in your dashboard.
  • Upon saving your selection, you should be directed to the OAuth flow. Cancel the OAuth flow to return to the SK dashboard.
  • In the selected metric tiles, verify that you see an error state with a prompt to update your Analytics property.
  • Verify that clicking on "Update" makes the tiles go on a loading state, creates the custom dimensions in the background (you can observe network requests in developer tools) and the tiles are displayed without an error state.

Verify insufficient permissions error state

  • Follow the above steps, but connect an Analytics property to Site Kit on which you do not have "Editor" or "Administrator" access.
  • Upon clicking on "Update" in the error state, the tiles should go on a loading state and attempt to create the custom dimensions in the background.
  • Since your account doesn't have at least an "editor" access to the GA property, verify that the request fails and you see an error tile with "Insufficient permissions". Verify that the "Learn more" and "Retry" links work as expected.

Verify generic error state

  • Follow the first set of steps.
  • Just before clicking the "Update" CTA in the error state, use the browser developer tools and simulate an "Offline" network status.
  • Click on "Update" and verify that a different error state appears that says "Analytics update fails".
  • Verify that the "Learn more" link and "Retry" CTA works as expected.

Verify scenario where custom dimension is deleted after creation

  • Select the Top categories by pageviews metric tile to be visible in the KMW area.
  • Complete custom dimension setup and wait for the report request to be successful.
  • Go to analytics.google.com -> Admin. Select your connected property and go to Custom definitions. Archive the 'googlesitekit_post_categories` custom dimension.
  • In the SK dashboard, clear session storage and reload the page.
  • In the network tab of developer tools, verify that the report request for this tile fails with a badRequest error reason. Once that fails, SK should make another request to sync the custom dimensions and then show the missing dimensions error state ("Update Analytics to track metric") in the tile.
  1. Clicking on "Update" should create the custom dimension again and re-fetch the report request.

Changelog entry

  • Allow creation of custom Analytics dimensions via Key Metric tiles.
@techanvil
Copy link
Collaborator

Hey @nfmohit, this AC is well written but again, it's going into IB territory where it doesn't really need to.

  • Arguably, the point about syncCustomDimensions could be written in a more behaviourally descriptive way (with a note in the IB to capture the implementation detail), but it's a minor point and we can skip that if you want.
  • However, I think the points that describe the autoSubmit behaviour would be better off being removed from here, and rather the AC can refer to the fact this behaviour should already be partially implemented via Update metrics selection panel to initiate creation of custom dimensions #7599, the implementation side of which can be subsequently referred to in the IB.
  • Relating to the previous point, when it comes to the behaviour upon returning from OAuth, there seems to be a bit of a crossover between this issue as described and 7599, as the AC in this one currently states that "the metric tile should trigger creation of the custom dimensions automatically", whereas this should already be taken care of via "the Key Metrics Widget area should create the custom dimensions automatically" in 7599, if I've understood things correctly. Does this indicate that 7599 should in fact specify that the individual tiles should create the custom dimensions? Or, maybe that detail can be left to the IB while this issue can simply refer back to 7599 as mentioned above.
  • A minor grammatical point, relating to the first two sub-points in the AC: The custom dimensions are dependencies, not dependants, of the tiles. So, rather than describing them as "the dependent custom dimensions", it would be more correct to say for example "the required custom dimensions".

@techanvil techanvil assigned nfmohit and unassigned techanvil Sep 27, 2023
@nfmohit
Copy link
Collaborator Author

nfmohit commented Sep 28, 2023

Thank you for the kind review, @techanvil. I've addressed your feedback as the following:

Arguably, the point about syncCustomDimensions could be written in a more behaviourally descriptive way (with a note in the IB to capture the implementation detail), but it's a minor point and we can skip that if you want.

This has been updated.

However, I think the points that describe the autoSubmit behaviour would be better off being removed from here, and rather the AC can refer to the fact this behaviour should already be partially implemented via #7599, the implementation side of which can be subsequently referred to in the IB.

Updated as well.

Relating to the previous point, when it comes to the behaviour upon returning from OAuth, there seems to be a bit of a crossover between this issue as described and 7599, as the AC in this one currently states that "the metric tile should trigger creation of the custom dimensions automatically", whereas this should already be taken care of via "the Key Metrics Widget area should create the custom dimensions automatically" in 7599, if I've understood things correctly. Does this indicate that 7599 should in fact specify that the individual tiles should create the custom dimensions? Or, maybe that detail can be left to the IB while this issue can simply refer back to 7599 as mentioned above.

You're correct, this was a mistake. This should actually rely on the key metrics widget area for custom dimensions creation if Site Kit is supposed to create them automatically. If Sit Kit is not supposed to create them automatically, the error state CTA should trigger it. I've updated the section.

A minor grammatical point, relating to the first two sub-points in the AC: The custom dimensions are dependencies, not dependants, of the tiles. So, rather than describing them as "the dependent custom dimensions", it would be more correct to say for example "the required custom dimensions".

Thank you for pointing out, updated.

Please let me know if this looks good, thank you!

@techanvil
Copy link
Collaborator

Thanks @nfmohit, that looks great!

AC LGTM ✅

@aaemnnosttv
Copy link
Collaborator

Thanks @zutigrm! A few general points about the IB as it relates to the guidelines which I encourage you to review as a refresher.

  • The IB as a whole is a bit verbose and could be simplified to read a bit more concisely. For example, we don't need to prescribe creating a folder explicitly, we can simply add this to the path of a new component we're describing. E.g. components/KeyMetrics/CustomDimensionsMetricTileError/ErrorActions
  • The IB shouldn't duplicate text that is defined in the AC since this can get out of sync during definition.
  • We want to keep the IB to be more of a summary of the approach, so it should be limited in the level of detail it includes to only that which is necessary. Things like names of internal functions and variables should be omitted as well as ternary operators or other specific code details. The IB should describe the approach rather than how to arrive at some code that sounds like it has already been written. It's a bit hard to follow as-is.
  • Regarding the code references, please always use GitHub "permalinks" to ensure they always reference the same source. The current links use the branch and line numbers only, which will change as the source updates making it easy to get out of sync.

  • Create a new CustomDimensionsMetricTileError folder

This directory probably isn't necessary as we can add the ErrorActions under the CustomDimensionTileWrapper since this component should only be relevant for metric tiles that depend on our custom dimensions.

  • Create new ErrorActions.js for component which will handle the error text and button actions

AFAIU this would only be used for errors related to custom dimension actions, correct? If so, let's name this accordingly as I believe we'd still be using ReportErrorActions within the metric tile itself for handling report errors. Perhaps CustomDimensionErrorActions.

  • Dispatch invalidateResolution on analytics-4 store for getAvailableCustomDimensions, so it can trigger re-fetch once, on first tile re-render

This may be insufficient as most resolvers have a condition to return early if there is already data in state for it which we will still likely want to keep/implement for consistency, but also to minimize the need for fetch mocks in tests. One way around this could be to have an additional flag in state that we can enable when we want to do this to bypass this guard in the resolver. Using a resolver for this is a good idea though because it solves the issue of ensuring it's only done once, regardless of how many custom dimensions error.


Question about the AC for @nfmohit

If the required custom dimensions are not available:

  • The metric tile should show a loading state if the custom dimensions are being created in the background.

Is this issue intended to implement the background creation mentioned here? If not, where is this being handled?

@aaemnnosttv aaemnnosttv assigned zutigrm and unassigned aaemnnosttv Oct 3, 2023
@zutigrm
Copy link
Collaborator

zutigrm commented Oct 4, 2023

@aaemnnosttv Thank you for the feedback.

Dispatch invalidateResolution on analytics-4 store for getAvailableCustomDimensions, so it can trigger re-fetch once, on first tile re-render
This may be insufficient as most resolvers have a condition to return early if there is already data in state for it which we will still likely want to keep/implement for consistency, but also to minimize the need for fetch mocks in tests. One way around this could be to have an additional flag in state that we can enable when we want to do this to bypass this guard in the resolver. Using a resolver for this is a good idea though because it solves the issue of ensuring it's only done once, regardless of how many custom dimensions error.

What about using the receiveGetAvailableCustomDimensions with value of undefined to reset the state, and then invalidate the resolver?

Is this issue intended to implement the background creation mentioned here? If not, where is this being handled?

To chip in here, creation of metrics infra and it's actions is being added in #7598. createCustomDimensions will be used to create custom metrics, and until it is resolved, by my understanding it will be creating the metrics. That's why I suggested waiting for isFetchingCreateCustomDimension to determine if metrics are still being created.

@nfmohit
Copy link
Collaborator Author

nfmohit commented Oct 4, 2023

Question about the AC for @nfmohit

If the required custom dimensions are not available:

  • The metric tile should show a loading state if the custom dimensions are being created in the background.

Is this issue intended to implement the background creation mentioned here? If not, where is this being handled?

Hi @aaemnnosttv. Triggering the creation of custom dimensions is a part of #7599. However, it is also notable that the error state introduced in this current issue will also trigger automatic creation of custom dimensions.

@zutigrm zutigrm assigned aaemnnosttv and unassigned zutigrm Oct 4, 2023
@mxbclang
Copy link

@wpdarren Just chatted with @aaemnnosttv about this one and it seems like there are actually issues here on both the service side and the plugin side.

For the service, this link was added to the Error States MVP Mapping (Get help) sheet as opposed to the Documentation URLs (Learn more) sheet. This was 100% on me as I had asked @jamesozzie to make this change. James, I'll follow up with an Asana task for you to get this added to Documentation URLs (Learn more) instead.

For the plugin, the URL that's being generated for the Learn more link is https://sitekit.withgoogle.com/support?error=The%20caller%20does%20not%20have%20permission, which is not the correct structure. Because there is no page at that URL, you're being directed to the fallback URL of https://sitekit.withgoogle.com/documentation/troubleshooting/using-troubleshooting-mode/, which is also not particularly helpful. I'll open up a new Asana task to get this fallback URL changed.

But the tl;dr here is that there is, in fact, an issue in the plugin in terms of how this URL is being served that will need to be investigated and fixed.

Ongoing Slack thread about all of this here! I'm offline beginning tomorrow but Evan is aware of this issue and can help as needed. Thanks!

@mxbclang
Copy link

Update: @jamesozzie has been asked to update the Learn more sheet in this Asana task.

@wpdarren Depending on timing, this may not be fixed on the service side by the time a fix is in place on the plugin side for the URL structure issue.

@aaemnnosttv Can you please clarify what the correct URL structure should be, so that even if the correct URL isn't being served from the service, Darren can confirm that the fix is correct on the plugin side? Thanks!

@aaemnnosttv
Copy link
Collaborator

For the service, this link was added to the Error States MVP Mapping (Get help) sheet as opposed to the Documentation URLs (Learn more) sheet. This was 100% on me as I had asked @jamesozzie to make this change. James, I'll follow up with an Asana task for you to get this added to Documentation URLs (Learn more) instead.

I just looked into this a bit more closely and so long as the intention is to use the same invalid permissions error that we have now, then the support URL was added to the correct sheet (as an error). The plugin needs to be updated to rewrite the error as we're doing for report errors to provide the proper code which will be used as the error_id which isn't currently in place.

if ( isInsufficientPermissionsError( err ) ) {
err.code = `${ moduleSlug }_insufficient_permissions`;
}

const helpLink = useSelect( ( select ) =>
select( CORE_SITE ).getErrorTroubleshootingLinkURL( error )
);

Can you please clarify what the correct URL structure should be, so that even if the correct URL isn't being served from the service, Darren can confirm that the fix is correct on the plugin side?

It should be

https://site-kit-dev.appspot.com/support/?error_id=analytics-4_insufficient_permissions

Sending this back for a follow-up 👍

@wpdarren
Copy link
Collaborator

wpdarren commented Nov 1, 2023

QA Update: ❌

Update: after a brief conversation with Nahid on Slack, it appears that there's an issue with gathering data states on the tiles and the error message when custom dimensions are deleted after creation.

@nfmohit an observation on the final section: Verify scenario where custom dimension is deleted after creation
In the QAB it mentions:

In the network tab of developer tools, verify that the report request for this tile fails with a badRequest error reason. Once that fails, SK should make another request to sync the custom dimensions and then show the missing dimensions error state ("Update Analytics to track metric") in the tile.

I see the badRequest error reason in the network tab but do not see the Update Analytics to track metric error in the tile. I cleared session storage and also cleared cookies and logged back in but no error message appears. I just see the gathering state which is was it was after the initial oAuth.

Any ideas why this could be?

image
image

@wpdarren wpdarren assigned nfmohit and unassigned wpdarren Nov 1, 2023
@nfmohit nfmohit closed this as completed Nov 1, 2023
@nfmohit nfmohit reopened this Nov 1, 2023
@nfmohit nfmohit assigned aaemnnosttv and unassigned nfmohit Nov 1, 2023
@aaemnnosttv
Copy link
Collaborator

@wpdarren & @nfmohit from what I can see, the sync in response to the missing custom dimension error in the main report works as expected on the current main. The problem observed in the report above which is attempted to be worked around in @nfmohit 's PR above is specific to the case where we do not yet know if the custom dimension has data available or not (i.e is still gathering data) which makes it quite an edge case, although somewhat easy for us to encounter since we're going through the motions rather unrealistically quick. The report which fails with the expected error is the gathering data check, not the "main" report request that triggers the sync as defined in this issue. I still think we should address it but I don't feel that this should block this issue, as it's more about the effect of #7638.

Sending this back to QA as is in case there is anything else to check here, and I'll open a new issue about the remaining work for this to work properly with dimensions that are still gathering data.

@wpdarren
Copy link
Collaborator

wpdarren commented Nov 6, 2023

QA Update: ✅

Verified:

Please note that I went through the ACs and can confirm that all of the error states work as expected, other than the issue reported above about the gathering state.

  • I see an error state with a prompt to update your Analytics property when following the QAB instructions.
  • Clicking on "Update" makes the tiles go on a loading state, creates the custom dimensions in the background. This was also observed in network requests in developer tools. The tiles are displayed without an error state.

Insufficient permissions error state

  • Upon clicking on "Update" in the error state, the tiles go on a loading state and attempt to create the custom dimensions in the background. Since my account doesn't have at least an "editor" access to the GA property, the request fails and you see an error tile with "Insufficient permissions". The "Learn more" and "Retry" links work as expected.

Generic error state

  • The "Learn more" link and "Retry" CTA works as expected.

Custom dimension is deleted after creation

  • In the network tab of developer tools, the report request for this tile fails with a badRequest error reason.
  • Once that fails, SK shows the missing dimensions error state Update Analytics to track metric in the tile.
  • Clicking on "Update" creates the custom dimension again and re-fetch the report request.
Screenshots

image
image
image
image
image
image

@wpdarren wpdarren removed their assignment Nov 6, 2023
nfmohit added a commit that referenced this issue Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

8 participants