Refactor the GatheringDataNotification to use a newly refactored version of the BannerNotification component.

[jimmymadon]

Feature Description

This issue focuses on creating a new Notification component that will be the 'non-bloated' version of the BannerNotification component. This component will be composed using other smaller components and hooks for dismissal, event tracking, etc. as per the PoC branch.

---

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

Acceptance criteria

• Refactor the GatheringDataNotification to use a new component, that "composes" individual pieces of logic within <BannerNotification> using smaller components. This includes:
  • A hook that encapsulates tracking of all GA events for this notification.
  • A component for rendering CTA action buttons/links (other than dismissals).
  • A component that renders a dismissal action and encapsulates dismissal functionality which will dismiss the notification, fire any events on dismissal (including the GA tracking event) and expire the dismissal after a given time (one day for this particular notification). This should use the new datastore infrastructure.
  • A component that observes whether the component was actually viewed.

Implementation Brief

• Wait for Refactor GatheringDataNotification to use the new Notifications datastore #8976 to be merged and refer to the POC branch where mentioned.
• Within assets/js/components/notifications, create a new notification component folder. Within a new index file similar to within BannerNotification / PoC branch:
  • Export a new Notification component.
• Similar to the POC, create and use the two new hooks, useHasBeenViewed anduseNotificationEvents as well as the ViewedStateObserver component.
• Create the Dismiss and CTA components that have their own related props. Follow the pattern in the POC to built these as props by creating a getNotificationComponentProps() function.
• Within the Notification component:
  • the Dismiss component will take care of: dismiss and dismissExpires props and the onDismiss event within the component.
  • the CTA component will take care of: label and link props and the onCTAClick event within the component.
• The logic for the SVG image and styling of the component can be ported over from BannerNotification as is for now.
• Within the GatheringDataNotification, add the props for the new Notification, Dismiss and CTA components. Follow the pattern in the POC for the refactored ZeroDataNotification to simply pass the appropriate props and render the above components.

Test Coverage

• Add tests for the new Notification component.

QA Brief

• Test the Gathering Data Notification (and its variants with SC and Analytics on/off) thoroughly.
  • Test the GA tracking event for view-notification when the notification is rendered.
  • Click on the See more services button and ensure the link takes the user to the correct tab as before. Test the confirm-notification GA event is fired correctly. Ensure the notification is correctly dismissed - a new wp_googlesitekitpersistent_dismissed_items record will appear in wp_usermeta in the database.
  • Delete the record above and re-load the page to retest the notification. Click on the Maybe later button and ensure the notification is dismissed again in the database and the dismiss-notification GA event is correctly fired.
• Test when any other notification, say any other SetupSuccessBannerNotification is active - they should take precedence over the Gathering Data notification.

Changelog entry

• Refactor the GatheringDataNotification to use a newly refactored version of the BannerNotification component.

[eugene-manuilov]

AC ✔️

[eugene-manuilov]

IB ✔️

[mohitwp]

QA Update ❌

@jimmymadon

Question > I have noticed that on dev environment multiple 'view-notifications' event for even -category ""mainDashboard_gathering-data-notification" are getting trigger when notification render on main dashboard. On latest environment multiple events are not getting trigger.
Note : To check this either reset current site or create new site without analytics.

I checked this on entity dashboard and only 1 event is getting trigger on entity dashboard.

Main Dashboard

Entity dashboard

Issue > On clicking 'See more services' loader not appears and user navigate to settings>services tab after few seconds. On latest environment loader appears.

Dev environment

Recording.1310.mp4

Latest environment

Recording.1311.mp4

[mohitwp]

@jimmymadon I’d like to highlight that in the latest environment, notifications about data gathering or zero data do not get permanently dismissed and reappear if we open the dashboard in a new tab. Additionally, clicking on the 'See more Services' CTA or the 'Remind me later' CTA does not create a record of wp_googlesitekitpersistent_dismissed_items under wp_usermeta table in the database. Is this change part of a refactor? I wanted to flag this difference.

Recording.1313.mp4

[mohitwp]

@jimmymadon One more scenario -

1> Set up SK without Analytics.
2> On main dashboard SC is in gathering state notice appears.
3> Click on 'See more Services' or Maybe later CTA to dismiss CTA.
4> Now setup analytics and create new a/c.
5> After creating a/c user navigates to main dashboard.
6> Success notice appears. Click on 'OK".
7> Now gathering state notification for Analytics is not appearing because gathering data notification is already dismissed under database.

[jimmymadon]

@mohitwp Regarding Notification Dismissal

I think the Gathering Data Notification and Zero Data Notification should be dismissed for 24 hours and only appear again after this duration. Clicking on any CTA within should trigger the dismissal AFAIK. This wasn't the behaviour in the past since we cached the dismissal in browser storage instead of the database. But I would like @aaemnnosttv to confirm this here.

As far as GA event tracking and loading state is concerned, I'll try to create a follow up PR.

[aaemnnosttv]

@jimmymadon the issue here is that these notifications are essentially specific to the connected property but the dismissals aren't. So once the dismissal is set for one property, even if we change the property to another, it's perhaps expected that the notifications would be shown again because the context has changed. In this specific case, I don't think it's a big deal but ideally we should have these kinds of dismissals specific to the property. Can we see about addressing this in a follow-up?

[aaemnnosttv]

I missed @mohitwp 's previous comment about dismissals not happening in my last reply.

He's correct in that both actions (primary/secondary|dismiss) should dismiss the notification (not just secondary/dismiss) as you can see in the current implementation.

site-kit-wp/assets/js/components/notifications/BannerNotification/index.js

Lines 196 to 215 in 7c1e93d

	const handleCTAClick = async ( event ) => {
	event.persist();
	if ( isCTALink && ! event.defaultPrevented ) {
	event.preventDefault();
	}
	let dismissOnCTAClick = true;
	if ( onCTAClick ) {
	( { dismissOnCTAClick = true } =
	( await onCTAClick( event ) ) || {} );
	}
	if ( isDismissible && dismissOnCTAClick ) {
	await dismissNotification();
	}
	if ( isCTALink ) {
	navigateTo( ctaLink );
	}
	};

[jimmymadon]

@mohitwp

I’d like to highlight that in the latest environment, notifications about data gathering or zero data do not get permanently dismissed and reappear if we open the dashboard in a new tab.

This was definitely a bug previously which the refactor now "fixes".

Additionally, clicking on the 'See more Services' CTA or the 'Remind me later' CTA does not create a record of wp_googlesitekitpersistent_dismissed_items under wp_usermeta table in the database. Is this change part of a refactor?

Yes - it is. We now store dismissals in the DB instead of just in browser session storage.

One more scenrio .... Now gathering state notification for Analytics is not appearing because gathering data notification is already dismissed under database for SC.

This is the existing behaviour as well today (unless we open a new tab which is the bug that I have mentioned above). After discussing with @aaemnnosttv, this is fine and we don't have to change this. The notification should reappear again in 24 hours.

I have noticed that on dev environment multiple 'view-notifications' event for even -category ""mainDashboard_gathering-data-notification" are getting trigger when notification render on main dashboard.

This has been fixed in a follow up PR.

On clicking 'See more services' loader not appears and user navigate to settings>services tab after few seconds. On latest environment loader appears.

Unfortunately, the way we dismiss notifications now immediately removes it from the queue. We will have to rethink this properly. I have created a follow up PR that just improves the speed with which we navigate to the settings tab. This shouldn't block the release though I feel. I will discuss this with @eugene-manuilov or another engineer and either create a follow up PR quickly tomorrow or another issue for the next release.

c.c @aaemnnosttv

[mohitwp]

QA Update ✅

• Tested on main enviornment.
• Verified multiple 'view notification' events are not getting track for 'main-dashboard-gathering data notification'.
• Verified gathering data notification for both SC and Analytics module.
• Verified the GA tracking event for view-notification when the notification is rendered.
• Verified clicking on the See more services buttontakes the user to the correct tab as before.
• Verified the confirm-notification GA event is fired correctly.
• Verified Ensure the notification is correctly dismissed - a new wp_googlesitekitpersistent_dismissed_items record will appear in wp_usermeta in the database.
• Verified clicking on the Maybe later button dismissed the notification in the database and the dismiss-notification GA event is correctly fired.
• Verified when any other notification, any other SetupSuccessBannerNotification is active - they take precedence over the Gathering Data notification.

Note

1> For the loader issue. I created a new ticket - #9225
2> As detailed by @jimmymadon, once the notification is dismissed, it should reappear within 24 hours. I am creating test sites to check the behavior and the database after 24 hours to confirm this.

cc @wpdarren

[aaemnnosttv]

On clicking 'See more services' loader not appears and user navigate to settings>services tab after few seconds. On latest environment loader appears.

Unfortunately, the way we dismiss notifications now immediately removes it from the queue. We will have to rethink this properly. I have created a follow up PR that just improves the speed with which we navigate to the settings tab.

On closer inspection, the follow-up changes in #9222 here should be reverted as it risks breaking the tracked event + dismissal. I'm opening a quick PR to do this now.