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

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

Closed
hussain-t opened this issue Oct 18, 2022 · 8 comments
Labels
P0 High priority Type: Bug Something isn't working

Comments

@hussain-t
Copy link
Collaborator

hussain-t commented Oct 18, 2022

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

Implementation Brief

Test Coverage

  • No new tests needed.

QA Brief

  • Install Site Kit via the plugin directory and activate it. Ensure the view_notification GA event for the activation category is triggered only when the ActivationApp notification is displayed. (This notification will have the Congratulations, the Site Kit plugin is now activated. title).
  • Ensure the view_notification GA event for the mainDashboard_wp52-version-notification event category is triggered only when the WPVersionBumpNotification is displayed.
  • Ensure 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

Changelog entry

  • Fix bug that could cause a notification view event to be sent even when the notification doesn't appear.
@hussain-t hussain-t added Type: Bug Something isn't working and removed Type: Bug Something isn't working labels Oct 19, 2022
@tofumatt tofumatt added the P0 High priority label Oct 19, 2022
@tofumatt
Copy link
Collaborator

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 tofumatt removed their assignment Oct 20, 2022
@eugene-manuilov eugene-manuilov self-assigned this Oct 24, 2022
@eugene-manuilov
Copy link
Collaborator

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
Copy link
Collaborator

Thanks, @tofumatt. IB ✔️

@wpdarren
Copy link
Collaborator

wpdarren commented Oct 27, 2022

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

@hussain-t
Copy link
Collaborator Author

hussain-t commented Oct 28, 2022

@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.

@wpdarren
Copy link
Collaborator

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 wpdarren removed their assignment Oct 28, 2022
@aaemnnosttv aaemnnosttv self-assigned this Nov 3, 2022
@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Nov 3, 2022

⚠️ 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?)

@techanvil
Copy link
Collaborator

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: #6109

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants