google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.25k stars 291 forks source link

Ensure the “Top content” metric doesn’t appear for view-only users if the custom dimension doesn’t exist #8175

Closed techanvil closed 1 month ago

techanvil commented 9 months ago

Feature Description

Ensure the “Top content” metric doesn’t appear for view-only users if the required custom dimension doesn’t exist.

See dashboard sharing in the design doc.


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

Acceptance criteria

Implementation Brief

Test Coverage

  1. Add a story for AudienceTile where the googlesitekit_post_type dimension is not present and is in view only context.

  2. Fix any other failing VR tests.

QA Brief

  1. Make sure audience segmentation is enabled and audiences tiles are visible in tiles.

  2. Use a GA4 property with the edit access to it. Archive the custom dimension googlesitekit_post_type in the GA4 property.

  3. Use the following snippet in the JS console to resync the available custom dimensions:

    googlesitekit.data.dispatch( 'modules/analytics-4' ).fetchSyncAvailableCustomDimensions();
  4. Login with the view only user and check the tile, you should not see the Top content by pageviews metric.

  5. Login with admin user and you should be able to see the Update button with the text

No data to show
Update Analytics to track metric

clicking on update will attempt to create the custom dimension, provided that the user has necessary permission for the property in GA.

Changelog entry

ivonac4 commented 5 months ago

@ankitrox can you add the estimate to this task? Thanks!

techanvil commented 5 months ago

Thanks @ankitrox, the IB LGTM. :white_check_mark:

ankitrox commented 2 months ago

This is ready for CR, but as it depends on #8130 , this will be moved to CR once 8130 is merged.

Actions to take before moving to CR.

  1. Merge develop into feature, push the changes.
  2. Make sure all tests and VRTs are passing.
  3. Make sure QA brief is up to date.
techanvil commented 1 month ago

Hi @ankitrox, I've merged the PR for this issue, but the QAB could use an update so I've assigned it to you in QA to make the amendment.

It's not necessary to use the mu-plugin you've provided to test this, and it's preferable to test things using a real-world user flow where possible, unless there's an exceptional reason not to.

In this case, we can test the issue by setting up Audience Segmentation using a property we've got edit access to and then archiving the custom dimension once it's created.

The property does need to be out of the gathering data state in order to set up the feature; it's reasonable to expect we should all have a property to hand that we can use for this sort of thing, but if needs be we can create a new property and then use the Tester plugin to provide mock report data in order to get the property out of the gathering data state.

image

I would expect QA engineers to have numerous existing properties to hand, though, so I wouldn't think this point is necessary to include in the QAB.

Please can you update the QAB accordingly?

ankitrox commented 1 month ago

Thank you @techanvil .

I have updated the QAB as per your instructions. It makes sense to use the GA4 property in real time. In the previous QAB I was expecting it to be tested with oi.ie analytics account.

Thanks for your feedback.

techanvil commented 1 month ago

Thanks @ankitrox! I added a point to the QAB as we need to resync the custom dimensions in order to test the scenarios.

kelvinballoo commented 1 month ago

QA Update ⚠️

@ankitrox can you add the steps for testing this AC point when the custom dimension exists but haven't been synced: In the edge case where the custom dimension does exist, but the available custom dimensions haven't been synced, the "Top content" metric should also not be shown. This is because the view-only user doesn't have permission to sync the available custom dimensions.

Items tested good: ✅

ankitrox commented 1 month ago

Thanks @kelvinballoo for testing this.

There's no need to test this edge case because this is similar to archiving the custom dimension where it will not exist in available dimensions in SK.

As view only user cannot sync the dimension, the custom dimension would not be present in SK's available dimensions.

kelvinballoo commented 1 month ago

QA Update ✅

Per latest conversion in slack, the edge case can be tested through the following steps:

Did as per the steps and the 'Top content metric' did not appear for the view-only user. ✅

Screenshot 2024-09-11 at 13 34 04

The main use-case was also verified good: ✅