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

Trigger the custom GA `view_notification` event only when the banner notifications are visible

Open hussain-t opened this issue 2 years ago • 1 comments

Bug Description

Currently, some banner notifications trigger the custom GA view_notification event using the useMount hook. However, the view_notification event will be triggered even if the banner notification is not displayed due to the respective component returning null if the data is unavailable.

The above event should be triggered if the respective banner notification is displayed.

Steps to reproduce

  1. Go to the Site Kit dashboard.
  2. Ensure the Google Analytics Debugger is enabled.
  3. Open the dev tools->console and filter by view_notification.
  4. Notice some GA events are triggered even though respective banner notifications aren't displayed. For example, mainDashboard_wp52-version-notification is triggered.

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

Acceptance criteria

  • All the banner notifications that trigger the custom GA view_notification event should be triggered if the respective banner notification is displayed. The following notification events should be checked to only send when the component actually renders and doesn't return null
    • https://github.com/google/site-kit-wp/blob/a7a6f45123b24b9c767ac62ae4dcd2c6c5a1bd6a/assets/js/components/activation/activation-app.js#L62
    • https://github.com/google/site-kit-wp/blob/a7a6f45123b24b9c767ac62ae4dcd2c6c5a1bd6a/assets/js/components/notifications/SetupSuccessBannerNotification.js#L99-L102
    • https://github.com/google/site-kit-wp/blob/a7a6f45123b24b9c767ac62ae4dcd2c6c5a1bd6a/assets/js/components/notifications/WPVersionBumpNotification.js#L53

Implementation Brief

  • We should trigger the trackEvent call within a useEffect instead of the useMount hook, conditionally on the same code path that causes the component to actually render. An example of how we have done this is in the ThankWithGoogleSupporterWallNotification component.

Test Coverage

QA Brief

Changelog entry

hussain-t avatar Oct 18 '22 18:10 hussain-t

ACs here are largely good, but I wanted to call out which components were actually affected. I'll move this to IB, and I can work on that myself 👍🏻

tofumatt avatar Oct 19 '22 14:10 tofumatt

Thanks, @tofumatt. Could you please add more information to IB? I think that each components have different circumstances when we can consider them as rendered, right? Let's write them down in IB.

eugene-manuilov avatar Oct 24 '22 11:10 eugene-manuilov

Thanks, @tofumatt. IB ✔️

eugene-manuilov avatar Oct 26 '22 10:10 eugene-manuilov

QA Update: ⚠️

@hussain-t I have a query based on this section of the QAB.

Install Site Kit via the plugin directory and activate it. Ensure the view_notification GA event for the mainDashboard event category is triggered only when the ActivationApp notification is displayed. (This notification will have the Congratulations, the Site Kit plugin is now activated. title).

When viewing the log the mainDashboard event is not showing and I am wondering if this is expected, since the Congratulations, the Site Kit plugin is now activated. title takes place on the plugins page. Please could you confirm this. This is what I see.

events-1

wpdarren avatar Oct 27 '22 23:10 wpdarren

@wpdarren, you're correct mainDashboard won't be available on the plugins page rather, it should be the activation category for the view_notification event.

hussain-t avatar Oct 28 '22 04:10 hussain-t

QA Update: ✅

Verified:

  • The view_notification GA event for the activation category is triggered only when the ActivationApp notification is displayed.
  • The view_notification GA event for the mainDashboard_wp52-version-notification event category is triggered only when the WPVersionBumpNotification is displayed. The following events are triggered only when the SetupSuccessBannerNotification is displayed: view_notification - mainDashboard_authentication-success-notification
    • complete_user_setup - mainDashboard_authentication-success-notification
    • complete_site_setup - mainDashboard_authentication-success-notification
Screenshots

image image image image image image

wpdarren avatar Oct 28 '22 10:10 wpdarren

⚠️ Approval

Not sure if this is a regression, but I still see the main authentication success view_notification event even when has been dismissed.

image

Also, it seems that view_notification events are triggered even when it isn't visible (i.e. hidden by CSS) due to the limit of only one shown at a time.

This can be seen by meeting the conditions for seeing the WPVersionBumpNotification, not dismissing it, and then visiting /wp-admin/admin.php?page=googlesitekit-dashboard&notification=authentication_success&slug=analytics to trigger the SetupSuccessBannerNotification. view_notification events are triggered for both right away even though only the success banner notification is visible.

cc: @hussain-t @tofumatt

It may be that this fulfills the AC as-is but we probably want to open a follow-up here so that view events are truly triggered when in-view (maybe using IntersectionObserver?)

aaemnnosttv avatar Nov 03 '22 21:11 aaemnnosttv

Good spot there @aaemnnosttv. I have looked into this and it's not a regression as I can see the same behaviour in the previous release. I've created a followup issue to address this: https://github.com/google/site-kit-wp/issues/6109

techanvil avatar Nov 04 '22 12:11 techanvil