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

Create “Top recent trending pages” key metric widget tile #7603

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

Create “Top recent trending pages” key metric widget tile #7603

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

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Sep 20, 2023

Feature Description

image
(screenshot for reference - see Figma for latest version)

A new key metric widget tile should be added for "Top recent trending pages".

See:

  1. Table of metrics in design doc.
  2. List of metrics in PRD.
  3. Figma designs.

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

Acceptance criteria

  • A new component should be created for the "Top recent trending pages" key metric widget tile and should be displayed within the Key Metrics Widget area if it is selected in the key metrics selection panel.
    • This metric tile should use the wrapper for tiles dependent on custom dimensions (being implemented in #7601).
    • When defining this metric tile in the KEY_METRICS_WIDGETS constant (located in assets/js/components/KeyMetrics/key-metrics-widgets.js at the time of writing), a new requiredCustomDimensions array property should be added to its definition. This array property will contain names of custom dimensions that the tile depends on. In this case, it should only be googlesitekit_post_date.
    • The component should look exactly as its Figma mock.
    • Its widget slug should be kmAnalyticsTopRecentTrendingPages (so that it is consistent with the design doc).
    • The metric tile title should be Top recent trending pages.
    • Its description tooltip should say Pages with the most pageviews published in the last 3 days.
    • This metric should be dependent on the analytics-4 module, so it should only be rendered in the key metrics widget area if GA4 is connected, otherwise, ConnectGA4CTATileWidget should be rendered (similar to existing GA4-dependent metric tiles). It should also not be shown to view-only users who do not have shared access to GA4.
  • The following data should be fetched from the Google Analytics Data API for the "last 3 days":
    • Metrics: screenPageViews
    • Dimensions: pagePath
    • Filter by: Date returned by the customEvent:googlesitekit_post_date custom dimension should be within the date range of the report request, i.e. the last 3 days.
    • Order by: screenPageViews descending
  • Values:
    • It should be a table pattern metric tile, with the page title showing up as the first column, and screenPageViews showing up as the second column.
  • The metrics & dimensions used in the reports for this tile should be allow-listed for view-only users.
  • This metric should be available for selection in the key metrics selection panel only if the following conditions are met:
    • newsKeyMetrics feature flag is enabled.
    • It is not a view-only user.
    • If it is a view-only user, the custom dimension that this metric tile depends on should be available for the connected GA property.

Implementation Brief

  • In assets/js/googlesitekit/datastore/user/constants.js:
    • Export a new constant for defining the new widget's slug: KM_ANALYTICS_TOP_RECENT_TRENDING_PAGES = 'kmAnalyticsTopRecentTrendingPages'.
    • Add this constant to the keyMetricsGA4Widgets array.
  • In assets/js/components/KeyMetrics/key-metrics-widgets.js:
    • Add a new item for the new widget to the KEY_METRICS_WIDGETS object if the newsKeyMetrics feature flag is enabled similar to existing widgets.
    • Its title & description should be according to the ACs.
    • Add a requiredCustomDimensions property that should be an array containing the googlesitekit_post_date string.
  • In assets/js/modules/analytics-4/components/widgets/, create a new TopRecentTrendingPages.js component. In this component, use TopCitiesWidget as a starting point, then make the following changes:
    • It should use the wrapper component introduced in #7601 so that it can handle the custom dimensions creation and relevant error & loading states.
    • Instead of obtaining the date range using the CORE_USER getDateRangeDates selector, it should use the last 3 days as the date range; i.e. an object such as:
      • startDate: Date of the 3rd previous day in YYYY-MM-DD format.
      • endDate: Yesterday's date in YYYY-MM-DD format.
    • Change relevant variable names accordingly.
    • Use the pagePath dimension instead of city.
    • Use the screenPageViews metric instead of totalUsers.
    • The report options object should have a dimensionFilters property that should filter the customEvent:googlesitekit_post_date dimension using the inListFilter filterType and verify that the dimension value is in a list of dates for the last 3 days in YYYYMMDD format.
    • The second column of the tile table should render the screenPageViews metric value (instead of the TopCitiesWidget totalUsers percentage).
    • The title and infoTooltip should be according to the ACs.
  • In assets/js/modules/analytics-4/index.js:
    • Register the new key metric widget the same as other key metric widgets. However, wrap the registration using the new newsKeyMetrics feature flag.
  • In includes/Modules/Analytics_4/Report/Request.php:
    • Update the validate_shared_dimensions method and add customEvent:googlesitekit_post_date to the list of valid dimensions.

Storybook

  • Add Storybook stories for the new widget for ready, zero, loading, and error states.

Test Coverage

  • Add VRT scenarios.

QA Brief

  • Setup Site Kit
  • Connect website that has some custom data for googlesitekit_post_date, tracked in the past 3 days
  • Enable ga4Reporting, userInput and newsKeyMetrics feature flags
  • Add Top recent trending pages metric tile
  • Verify it shows pages ordered by most views (if data is present)

Changelog entry

  • Add a "Top Recent Trending Pages" Key Metric tile.
@nfmohit nfmohit added Type: Enhancement Improvement of an existing feature P0 High priority labels Sep 20, 2023
@mxbclang mxbclang added P0 High priority and removed P0 High priority labels Sep 21, 2023
@nfmohit nfmohit self-assigned this Sep 25, 2023
@nfmohit nfmohit removed their assignment Sep 27, 2023
@techanvil techanvil self-assigned this Sep 28, 2023
@techanvil
Copy link
Collaborator

Hey @nfmohit, this is another well written AC, it does have a couple of implementation details that arguably should be in the IB (I'm thinking the stuff about widgetSlugs and requiredCustomDimensions), but I think it's manageable. Please do try to cut down on the IB details in future ACs though where possble - try to imagine yourself reading it from a QA perspective, determining how to verify the issue is behaving as expected. Remember, this will also have the added bonus of reducing the amount of detail us developers need to add to the QAB ;)


Actually, I was about to give it the ✅ but have noticed that the dimension for this widget is pageTitle in the Design Doc, but pagePath in this AC. I'm guessing the AC needs updating for this point, unless we've made a decision to change the dimension which hasn't been reflected in the doc...

@techanvil
Copy link
Collaborator

Update to the above: I've just spotted the change to use pagePath in #7579, so I guess the same applies here?

This could be a good candidate for the first entry in the Changes during engineering section in the DD :)

@nfmohit
Copy link
Collaborator Author

nfmohit commented Sep 28, 2023

Hey @nfmohit, this is another well written AC, it does have a couple of implementation details that arguably should be in the IB (I'm thinking the stuff about widgetSlugs and requiredCustomDimensions), but I think it's manageable. Please do try to cut down on the IB details in future ACs though where possble - try to imagine yourself reading it from a QA perspective, determining how to verify the issue is behaving as expected. Remember, this will also have the added bonus of reducing the amount of detail us developers need to add to the QAB ;)

Thank you for the detailed explanation, @techanvil. I agree, I shouldn't have been so verbose in these ACs and should've included the more technical choices to be made in the IB based on the design doc. I just didn't want the implementation to take a different turn than what we envisioned in the design doc to make sure we met the end goal when all the issues came together. I'll keep this in mind for future cases, and will include any preferred technical choices in the IB section. Thank you again!

Actually, I was about to give it the ✅ but have noticed that the dimension for this widget is pageTitle in the Design Doc, but pagePath in this AC. I'm guessing the AC needs updating for this point, unless we've made a decision to change the dimension which hasn't been reflected in the doc...
Update to the above: I've just spotted the change to use pagePath in #7579, so I guess the same applies here?

This could be a good candidate for the first entry in the Changes during engineering section in the DD :)

This is correct. I've also updated this across the design doc and included the change in the "Changes during engineering" section. Thank you for pointing out!

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

Thank you for the detailed explanation, @techanvil. I agree, I shouldn't have been so verbose in these ACs and should've included the more technical choices to be made in the IB based on the design doc. I just didn't want the implementation to take a different turn than what we envisioned in the design doc to make sure we met the end goal when all the issues came together. I'll keep this in mind for future cases, and will include any preferred technical choices in the IB section. Thank you again!

Thanks @nfmohit - that is completely understandable! As discussed it's valuable to keep the AC higher level but it's very worthwhile capturing relevant IB details up-front too if it's going to help the definition process (obviously, in the IB section as discussed). Thanks for taking this on board, and for the constructive conversation :)

This is correct. I've also updated this across the design doc and included the change in the "Changes during engineering" section. Thank you for pointing out!

Top stuff, nice one!

Happy to give this the old AC LGTM. ✅

@techanvil techanvil removed their assignment Sep 29, 2023
@nfmohit nfmohit assigned nfmohit and unassigned nfmohit Oct 3, 2023
@techanvil techanvil self-assigned this Oct 5, 2023
@techanvil
Copy link
Collaborator

IB ✅

@techanvil techanvil removed their assignment Oct 5, 2023
@jimmymadon
Copy link
Collaborator

@nfmohit Just flagging that this issue does not have an estimate. 🙂

@nfmohit
Copy link
Collaborator Author

nfmohit commented Oct 5, 2023

Whoops, I remember adding it 🤔 Thanks @jimmymadon !

@zutigrm zutigrm self-assigned this Oct 9, 2023
@zutigrm
Copy link
Collaborator

zutigrm commented Oct 9, 2023

Status update: Further execution pending for #7601

@zutigrm zutigrm mentioned this issue Oct 17, 2023
18 tasks
@tofumatt tofumatt removed their assignment Oct 18, 2023
@wpdarren wpdarren self-assigned this Oct 18, 2023
@techanvil
Copy link
Collaborator

Hey @zutigrm @wpdarren, I've just noticed that the various error state stories for this widget are not displaying properly - they are all displaying the loading state instead. It looks like this will need a followup PR to fix them.

@zutigrm it's a minor one but if you're creating a followup could you please take a look at this comment at the same time?

@aaemnnosttv
Copy link
Collaborator

I've opened a follow-up PR that corrects the states for error states, but it also addresses something that I would have flagged in the initial PR around the report options in that it was using new Date for today rather than deriving dates from the reference date.

@aaemnnosttv aaemnnosttv removed their assignment Oct 18, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Oct 18, 2023
@wpdarren wpdarren self-assigned this Oct 18, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Oct 26, 2023

QA Update: ❌

@zutigrm when I click on the link in the tile, nothing happens. I tested this on my live site that has data. Interestingly when I right click and select open in new tab, it does open and the Analytics screen loads successfully.

nkmw-7.mp4

@techanvil
Copy link
Collaborator

Back to you for another pass, @wpdarren.

@techanvil techanvil assigned wpdarren and unassigned techanvil Oct 27, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Oct 28, 2023

QA Update: ❌

@zutigrm @techanvil sadly, this still seems not to be working. Tried on Chrome and Safari. I refreshed the deverlop branch a few times and looking at Github actions, the zip was generated properly. I cleared cookies, also cleared local and session storage. Closed, and repoened browser, but no luck. Can you recreate this on your side?

nkmw-9.mp4

@wpdarren wpdarren assigned zutigrm and unassigned wpdarren Oct 28, 2023
@zutigrm
Copy link
Collaborator

zutigrm commented Oct 30, 2023

@wpdarren Thanks for checking this. I can't seem to replicate this, it is working in my testing, locally in develop, and on instawp for develop branch. Although, I switched to latest, and then back to develop, to ensure there is no some cache maybe. Here is the video from instawp:

Screen.Recording.2023-10-30.at.08.42.48.mov

@wpdarren Can you reproduce this still on your end? Maybe you can try also switching to latest and then back to develop to confirm

@wpdarren
Copy link
Collaborator

wpdarren commented Oct 30, 2023

QA Update: ✅

Verified:

  • A new component is created for the Top recent trending pages key metric widget tile and is displayed within the Key Metrics Widget area if it is selected in the key metrics selection panel.
  • The component looks exactly as its Figma mock.
  • Its widget slug is kmAnalyticsTopRecentTrendingPages
  • The metric tile title is Top recent trending pages.
  • Its description tooltip is Pages with the most pageviews published in the last 3 days.
    This metric is dependent on the analytics-4 module, and is only rendered in the key metrics widget area if GA4 is connected, otherwise, ConnectGA4CTATileWidget is rendered.
  • The component is a table pattern metric tile, with the page title showing up as the first column, and screenPageViews showing up as the second column.
  • The metrics & dimensions used in the reports for this tile are allow-listed for view-only users.
  • This metric are available for selection in the key metrics selection panel if the conditions in the AC are met
  • The issues with the links reported above is now fixed. The links open in a new window on the Analytics screen.
Screenshots

image
image
image
image

@wpdarren wpdarren removed their assignment Oct 30, 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