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

Add the “Switch to GA4 Dashboard View” notification banner #6544

Closed
nfmohit opened this issue Feb 7, 2023 · 16 comments
Closed

Add the “Switch to GA4 Dashboard View” notification banner #6544

nfmohit opened this issue Feb 7, 2023 · 16 comments
Labels
Exp: SP P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@nfmohit
Copy link
Collaborator

nfmohit commented Feb 7, 2023

Feature Description

A new BannerNotification should be added that will prompt users to switch their sites to the GA4 dashboard view. It should be according to the Figma design.

To find out if this BannerNotification should be displayed, a shouldPromptGA4DashboardView selector should be added to the analytics-4 datastore. This selector should return true if GA4 is connected, isGA4DashboardView selector (being implemented in #6541) returns false, the above BannerNotification is not dismissed, and GA4 is not in a gathering data state and it has at least 3 days of data.

The BannerNotification should only be shown if the shouldPromptGA4DashboardView selector returns true, and the ga4Reporting feature flag is enabled.

Clicking the CTA should update the dashboardView Analytics module setting (being added in #6540) to GA4, thus switching the dashboard view to GA4.

Dismissing the banner should do so persistently.

Here is the relevant part in the design doc.

Here is a screenshot for visual aid:
image

This BannerNotification will later be removed as a part of #6549.


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

Acceptance criteria

  • A new BannerNotification should be created according to this Figma design.
    • Title: Display data from Google Analytics 4 on your dashboard
    • Description: Update your dashboard to show data from the new version of Analytics (Google Analytics 4) instead of the old version (Universal Analytics). Learn what’s new.
    • Clicking on the Update dashboard CTA should change the dashboardView Analytics module setting (being added in New Analytics module setting to store dashboard view #6540) to google-analytics-4.
    • Dismissing the BannerNotification using Maybe later should permanently dismiss it.
  • The analytics-4 datastore should have a new selector added that indicates if a user should be prompted to switch their site to the GA4 dashboard view, say shouldPromptGA4DashboardView(). This selector should return true if the following conditions are met:
    • GA4 is connected.
    • The isGA4DashboardView() selector in the analytics-4 datastore (being implemented in Add isGA4DashboardView selector #6541) returns false.
    • The GA4 property was created at least 3 days ago.
  • The above defined BannerNotification should be rendered in the client only if the above shouldPromptGA4DashboardView() selector returns true.

Implementation Brief

Create the Selector

Note: The shouldPromptGA4DashboardView selector should be added to the analytics datastore; similarly, we have added the isGA4DashboardView selector to the analytics datastore.

In assets/js/modules/analytics/datastore/settings.js:

  • Add a new selector shouldPromptGA4DashboardView(). It should return true if the following conditions are met:
    • GA4 is connected using the isModuleConnected() selector.
    • The isGA4DashboardView() selector of the analytics datastore returns false.
    • GA4 is not in a gathering data state using the isGatheringData() selector of the analytics-4 datastore.
    • And it has at least 3 days of data with the following logic:
      • Construct the args to be passed to the getReport selector of the analytics-4 datastore. The args should be:
        • startDate,
        • endDate,
        • metrics: [{ name: 'totalUsers' }]
        • dimensions: [{ name: 'date' }]
    • Get the report using the getReport selector of the analytics-4 datastore.
    • Return true if the property's createTime is greater than or equal to 3 days. Similar to the isGatheringData() selector that checks for 2 days.
  • Otherwise, return false.

Create the Notification Component

Create a new component, SwitchGA4DashboardViewNotification in assets/js/components/notifications:

  • It should render the BannerNotification component if the shouldPromptGA4DashboardView() selector of the analytics datastore returns true with the following props:
    • id: switch-ga4-dashboard-view
    • title: Display data from Google Analytics 4 on your dashboard
    • description: Update your dashboard to show data from the new version of Analytics (Google Analytics 4) instead of the old version (Universal Analytics)..
    • ctaComponent: Pass the SpinnerButton component with the following props:
      • onClick: A callback that dispatches the setDashboardView with google-analytics-4 and saveSettings actions of the analytics datastore.
      • children: Update dashboard
    • dismiss: Maybe later
    • WinImageSVG: assets/svg/graphics/ga4-success-green.svg
    • learnMoreLabel: Learn what’s new
    • learnMoreURL: Obtain it from the getDocumentationLinkURL selector of the core/site datastore with the ga4 as the slug.
    • The strings should be translated.
    • Add stories for the SwitchGA4DashboardViewNotification component.

Test Coverage

  • Add tests for the new selector shouldPromptGA4DashboardView() in assets/js/modules/analytics/datastore/settings.test.js.
  • Add basic tests for the new component SwitchGA4DashboardViewNotification.

QA Brief

  • Enable ga4Reporting and connect Analytics module with both UA and GA property.
  • Test with a GA4 property is in gathering data state (has no data and the property is less than 3 days old, this used to be 2 days old but was changed to align it with the banner display, see here) - the banner should not be displayed.
  • Test with a GA4 property with data - the banner should be shown as described in design and AC.
    • Clicking primary CTA will switch the dashboard to GA4 version and dismiss the banner.
    • Clicking dismiss will dismiss the banner permanently.
    • Clicking learn more will open the link defined in AC in a new tab.

Changelog entry

  • Add the “Switch to Google Analytics 4 Dashboard View” notification banner.
@tofumatt
Copy link
Collaborator

tofumatt commented Mar 7, 2023

ACs here are good 👍🏻

@techanvil
Copy link
Collaborator

techanvil commented Mar 16, 2023

Hey @hussain-t, good work so far on the IB here. I have a couple of comments:

  • It seems a bit strange to be adding a dismissable item to SwitchGA4DashboardViewNotification, when we could simply use the BannerNotication's built-in dismissal handling. I realise the AC specifies this dismissable item to be checked as part of the shouldPromptGA4DashboardView() selector, but I think it would be simpler to remove this aspect and let the BannerNotification take care of its own dismissal, in keeping with the general usage of this component.
  • That aside, the IB fulfills the AC, but there is a question about whether the use of rowCount is going to be the right approach, I've left a related comment on Ensure that GA4 widgets display correctly with real data (notably that the graphs handle missing/zero rows). #6623 and we should see how this resolves in case it calls for a change in this issue.

@techanvil techanvil assigned hussain-t and unassigned techanvil Mar 16, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Mar 16, 2023

I think we should use property creation date over days of row count, given we already need that date in #6572. I mentioned as much in #6623 (comment).

If that sounds good we should update the ACs here, let me know what you think @techanvil.

@hussain-t
Copy link
Collaborator

Thanks, @techanvil @tofumatt. Using the property's createTime seems reasonable. I have updated the IB accordingly. If that sounds good, please update the AC.

@hussain-t hussain-t assigned techanvil and unassigned hussain-t Mar 17, 2023
@techanvil
Copy link
Collaborator

Thanks @tofumatt and @hussain-t, using the creation time of the property SGTM. I've updated the AC and the IB also LGTM. :)

IB ✅

@nfmohit
Copy link
Collaborator Author

nfmohit commented Mar 20, 2023

Note, there was a mistake in the issue description about the requirement of isGA4DashboardView() selector being true, which actually should be false, as we want to show this Banner Notification if the site is not in the GA4 dashboard view. I have updated this including in ACs and IB.

Thank you @kuasha420 for spotting this!

@kuasha420
Copy link
Contributor

Thank @nfmohit for the quick amendment. As par our discussion, I'd like a bit more clarification on the last point of AC. The issue description says the Banner should only be displayed when GA4 is not in a gathering data state and it has at least 3 days of data. However, the gathering data state wasn't translated into AC (or retroactively removed) in favor of checking whether the GA4 property is at least 3 days old. I'm assuming from reading the comments is that this change was made while @hussain-t and @techanvil were finalizing the IB. This leaves a few caveats.

Currently in IB, we're getting report using the similar args as the hasZeroData ga4 selector but not doing anything with it. Instead the property age being at least 3 days old is determined by duplicating the logic from the selectDataAvailability / isGatheringData ga4 selector. This seems like a needless duplicated code which we may be able to streamline.

From discussion with @nfmohit, It seems that the intention is to only show the Banner if the GA4 property is not in gathering data state and has data (including zero data) in addition to property being at least 3 days old. This is very similar to the logic in the aforementioned selectDataAvailability / isGatheringData except that only accounts for property that are 2 days old.

I'm not super familiar with the implementation of ga4 isGatheringData selector but from memory, the 2 days were chosen somewhat arbitrarily. So, we have two options beside duplicating all the code in this shouldPromptGA4DashboardView selector:

  • Just use isGatheringData() selector as is, which will work but instead of 3 days, the property will need to be at least 2 days old for the banner to show.
  • Update the selectDataAvailability to change the property being less than 3 days old instead of 2 days. Then the AC will be met but theisGatheringData will require property older than 3 days.
  • Continue using the duplicated selectDataAvailability logic with updated date. This selector will eventually be removed, so it's not that big of a deal.

Please let me know what you think. Cheers.

@techanvil
Copy link
Collaborator

Hmm. Well, IIRC the decision for the GA4 "gathering data" state to include a check for the property to be under two days old was taken in a discussion with @marrrmarrr; I am not sure how the requirement for there to be at least three days of data to show this "Switch to GA4 Dashboard View" banner was arrived at but it's ultimately another product decision, so I think the best thing to do here would be to bring Mariya into the discussion and ask for her input on it - if we can synchronise these two similar but currently separate sounding requirements that would obviously make for a simpler implementation, but otherwise we'll need to implement with two separate checks.

@marrrmarrr
Copy link
Collaborator

@techanvil @kuasha420 IIRC the "3 days old" requirement for inviting users to switch over the dashboard to GA4 stats was arrived at by confirming what's the longest time it might take for stats to appear in GA (max 48hrs according to the official docs) and adding an extra day to that, just to be on the safe side. The idea is that if the user has recently added GA4, it would be a better experience if they can see at least some stats directly after they switch the dashboard.
We could probably synchronise to 3 days everywhere to simplify implementation, instead of having to do two separate checks (option 2 from what @kuasha420 was suggesting near the end of his last comment).

@techanvil
Copy link
Collaborator

Thanks @marrrmarrr - sounds good and would be a nice straightforward path conceptually and in terms of implementation.

@kuasha420
Copy link
Contributor

@marrrmarrr @techanvil Thank you both. I have gone ahead and updated the gathering data selector to 3 days. This made everything cleaner.

This is ready for CR.

Cheers.

@kuasha420 kuasha420 removed their assignment Mar 21, 2023
@techanvil techanvil assigned techanvil and kuasha420 and unassigned techanvil Mar 21, 2023
@kuasha420 kuasha420 assigned techanvil and unassigned kuasha420 Mar 22, 2023
@techanvil techanvil removed their assignment Mar 23, 2023
@wpdarren wpdarren self-assigned this Mar 24, 2023
@wpdarren
Copy link
Collaborator

wpdarren commented Mar 24, 2023

QA Update: ⚠️

@kuasha420 I have two observations.

  1. After clicking on the 'maybe later' link and then resetting Site Kit (or clear the application session storage) the banner reappears. The AC suggests that it should permanently disappear.

Is this expected?

  1. In the AC or QAB there's no mention of what the scenario should be if a site is in zero data state. As it stands, the banner does not appear with zero data. Should it? I suspect it should be the same as gathering state, but want to check.

@kuasha420
Copy link
Contributor

Hi @wpdarren,

Thanks for sharing your observation.

  1. This is fine, the banner is using our standard banner dismissal infrastructure and they don't persist beyond Site Kit reset.
  2. According to the description of the issue, the banner should only appear when the site is not in gathering data state and has at least 3 days of data. That and from reading the comments in the issue from Mariya, my understanding is that the intention here is to only offer the GA4 dashboard once it has some data so user doesn't go from UA with data to a barren dashboard. So, this also seems fine to me.

Cheers.

@kuasha420 kuasha420 removed their assignment Mar 27, 2023
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

Note: I have added a few issues to the bug bash board for the banner. The graphic and 'Learn what's new' link are not the same as figma but are known issues. Details on the Asana ticket.

  • With a new GA4 property that is in gathering data state the banner is not be displayed.
  • With a GA4 property and data the banner is shown as described in design and AC.
    • Clicking primary CTA switches the dashboard to GA4 version and dismisses the banner.
    • Clicking dismiss will dismiss the banner permanently.
    • Clicking learn more will open the link defined in AC in a new tab.
  • Tested the banner on supported browsers
  • Tested the banner on smaller screen sizes.

After clicking on the 'maybe later' link and then resetting Site Kit (or clear the application session storage) the banner reappears. The AC suggests that it should permanently disappear.

In the AC or QAB there's no mention of what the scenario should be if a site is in zero data state. As it stands, the banner does not appear with zero data.

Arafat confirmed that these are expected.

image

@aaemnnosttv
Copy link
Collaborator

Approval ⚠️

  • Dismissing the BannerNotification using Maybe later should permanently dismiss it.

This part of the AC is not implemented and the banner currently relies on the client side storage for the dismissal. This storage is limited to the current session and will be invalidated on the next login. If this is to be truly a permanent dismissal, it should use a dismissed item.

e.g.
image

Looking at the stories for the component, there is also some improper use there, while not critical would be good to fix up. The above detail is more important.

@nfmohit
Copy link
Collaborator Author

nfmohit commented Apr 7, 2023

This part of the AC is not implemented and the banner currently relies on the client side storage for the dismissal. This storage is limited to the current session and will be invalidated on the next login. If this is to be truly a permanent dismissal, it should use a dismissed item.

Based on our internal discussion, I have opened #6840 so that we can fine control when this banner notification should reappear after dismissal. Thank you @aaemnnosttv!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants