Control when "Switch to GA4 Dashboard View" banner notification reappears after dismissal

[nfmohit]

Feature Description

#6544 added a banner notification that prompts the user to switch their site to the GA4 dashboard view. When this banner notification is dismissed, it is only dismissed on the client side and will re-appear on the next login.

Based on this approval comment, this Figma comment thread and the conversation in Slack here, we need to fine control when this banner notification should re-appear.

To achieve this, we can use a dismissed item in the backend, and set an expiry based on when we want it to reappear.

Update: As per #6932, the banner notification is now being persistently dismissed.

---

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

Acceptance criteria

• Clicking on Maybe later in the SwitchGA4DashboardViewNotification banner notification should no longer permanently dismiss the notification. Instead, the dismissal should have an expiry of 24 hours.
• However, the notification should still be permanently dismissed when the Analytics dashboard view is switched to GA4 (either from the banner or Analytics settings), so that if in case the site switches back to the UA dashboard view, the user isn't shown the notification again.

Implementation Brief

• In assets/js/components/notifications/SwitchGA4DashboardViewNotification.js:
  • Update the handleDismiss function so that the dismissItem action inside is dispatched with an expiry of 24 hours.

Test Coverage

• No tests need to be added/updated.

QA Brief

Changelog entry

[nfmohit]

Hi @marrrmarrr. I'm almost done defining the ACs for this issue. I just wanted to check with you if you had a recommendation on the duration after which the banner notification should reappear when a user clicks on Maybe later. Thank you!

CC: @aaemnnosttv

[marrrmarrr]

@nfmohit I think given that after this Friday their dashboard will basically slowly go stale, it's ok to show this more frequently - so how about either after 1 day or at their next login, whichever comes later?

[aaemnnosttv]

@marrrmarrr the dismissal uses time so it would be more complicated to use a login.

@nfmohit regarding the AC it's important that the dismissal is still persistent, just not permanent, which is what I think you meant but we still need to use dismissed items (it can't be client-side only).

[nfmohit]

Thank you, @marrrmarrr & @aaemnnosttv.

I have updated the ACs for the dismissal to expire after 24 hours, as it would be complicated to use a login with our dismissed-items infrastructure. I have also clarified the persistence versus permanence bit. Let me know what you think. Thanks!

[tofumatt]

ACs here are 👍🏻

[aaemnnosttv]

Thanks @nfmohit, one suggestion for you

In assets/js/components/settings/SettingsActiveModule/Footer.js:

• Update the handleConfirm function, specifically inside the check to see if the slug is analytics.

Can we do this instead within the submitChanges action for Analytics? We should be able to use a selector like hasSettingChanged in combination with the current value before saving to know if dashboardView was changed to GA4 and if so, we can set a flag to dismiss permanently if: the item is not yet dismissed, and settings saved successfully.

This way we avoid adding more module-specific coupling to a shared component.

LMKWYT

[nfmohit]

Thank you for pointing this out, @aaemnnosttv, and I absolutely agree.

I went to check on the submitChanges action for Analytics, and found out that this bit was actually already implemented as part of a former issue. See:

site-kit-wp/assets/js/modules/analytics/datastore/settings.js

Lines 180 to 191 in 238155d

	await registry
	.__experimentalResolveSelect( CORE_USER )
	.getDismissedItems();
	if (
	! select( CORE_USER ).isItemDismissed(
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID
	)
	) {
	dispatch( CORE_USER ).dismissItem(
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID
	);
	}

As a result, I've removed this part from the IB, and reduced the estimate.

Let me know what you think, thank you!

[aaemnnosttv]

Thanks @nfmohit ! That's great to see. We might want to spend some time to make sure that the existing implementation is working correctly with an additional test or two. We might need to resolve select dismissed tours beforehand like we're doing with dismissed items in the snippet you linked, otherwise the synchronous select may not be accurate.

Let's make sure to review this, but the current estimate seems like plenty of time for doing this as well.

[nfmohit]

Thank you for raising the valid concerns, @aaemnnosttv!

We might want to spend some time to make sure that the existing implementation is working correctly with an additional test or two. We might need to resolve select dismissed tours beforehand like we're doing with dismissed items in the snippet you linked, otherwise the synchronous select may not be accurate.

I gave this a try to verify it works according to the ACs, and it does seem to do so. One catch is that if a user dismisses the SwitchGA4DashboardViewNotification, our change proposed in the IB dismisses it for 24 hours.

Then, within those 24 hours, if they switch to the GA4 dashboard view, the notification is not dismissed again as isItemDismissed returns true. Do you think this is a valid concern? If so, we could introduce a new selector, e.g. isItemDismissedPermanently or such to tackle such a scenario. Please let me know what you think.

About tests, the item dismissal seems to have been covered quite extensively. See:

site-kit-wp/assets/js/modules/analytics/datastore/settings.test.js

Lines 630 to 760 in afbdd0d

	describe( 'dismiss the switch to GA4 dashboard view notification', () => {
	const fetchDismissItemRegExp = new RegExp(
	'^/google-site-kit/v1/core/user/data/dismiss-item'
	);
	it( 'should not dismiss the switch to GA4 dashboard view notification when setting the dashboard view to UA', async () => {
	registry
	.dispatch( CORE_USER )
	.receiveGetDismissedItems( [] );
	registry
	.dispatch( MODULES_ANALYTICS )
	.setSettings( validSettings );
	fetchMock.postOnce( gaSettingsEndpoint, {
	body: validSettings,
	} );
	expect(
	registry.select( MODULES_ANALYTICS ).getDashboardView()
	).toBe( DASHBOARD_VIEW_UA );
	expect(
	registry
	.select( CORE_USER )
	.isItemDismissed(
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID
	)
	).toBe( false );
	await registry
	.dispatch( MODULES_ANALYTICS )
	.submitChanges();
	// Wait for resolvers to run.
	await waitForDefaultTimeouts();
	expect(
	registry
	.select( CORE_USER )
	.isItemDismissed(
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID
	)
	).toBe( false );
	} );
	it( 'should not dismiss the switch to GA4 dashboard view notification if it is already dismissed', async () => {
	registry
	.dispatch( CORE_USER )
	.receiveGetDismissedItems( [
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID,
	] );
	registry.dispatch( MODULES_ANALYTICS ).setSettings( {
	...validSettings,
	dashboardView: DASHBOARD_VIEW_GA4,
	} );
	fetchMock.postOnce( gaSettingsEndpoint, {
	body: validSettings,
	} );
	expect(
	registry.select( MODULES_ANALYTICS ).getDashboardView()
	).toBe( DASHBOARD_VIEW_GA4 );
	expect(
	registry
	.select( CORE_USER )
	.isItemDismissed(
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID
	)
	).toBe( true );
	await registry
	.dispatch( MODULES_ANALYTICS )
	.submitChanges();
	// Wait for resolvers to run.
	await waitForDefaultTimeouts();
	expect(
	registry
	.select( CORE_USER )
	.isItemDismissed(
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID
	)
	).toBe( true );
	expect( fetchMock ).not.toHaveFetched(
	fetchDismissItemRegExp
	);
	} );
	it( 'should dismiss the switch to GA4 dashboard view notification if it has not been dismissed yet when setting the dashboard view to GA4', async () => {
	registry
	.dispatch( CORE_USER )
	.receiveGetDismissedItems( [] );
	registry.dispatch( MODULES_ANALYTICS ).setSettings( {
	...validSettings,
	dashboardView: DASHBOARD_VIEW_GA4,
	} );
	fetchMock.postOnce( gaSettingsEndpoint, {
	body: validSettings,
	} );
	fetchMock.post( fetchDismissItemRegExp, {
	body: [ GA4_DASHBOARD_VIEW_NOTIFICATION_ID ],
	} );
	expect(
	registry
	.select( CORE_USER )
	.isItemDismissed(
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID
	)
	).toBe( false );
	await registry
	.dispatch( MODULES_ANALYTICS )
	.submitChanges();
	// Wait for resolvers to run.
	await waitForDefaultTimeouts();
	expect( fetchMock ).toHaveFetched( fetchDismissItemRegExp );
	expect(
	registry
	.select( CORE_USER )
	.isItemDismissed(
	GA4_DASHBOARD_VIEW_NOTIFICATION_ID
	)
	).toBe( true );
	} );
	} );

Do you think any more tests would help?

Thank you!

[aaemnnosttv]

@nfmohit I'm not sure if this issue is worth addressing anymore since most people have probably already switched to GA4 already but also, the dismissal is currently permanent so if we wanted to change it, it would only benefit people who haven't dismissed it already. We could work around that by using a new key for the dismissed item, but I'm not sure we need to nag people to switch to GA4 since there are already many notices on the dashboard that UA data is stale and stopped collecting data. Eventually, people will be moved to the GA4 version of the dashboard automatically.

@marrrmarrr if it's okay with you, I think we can close this issue and leave things as they are now?

[marrrmarrr]

👀 👍 to me

[mxbclang]

Closing per @marrrmarrr's approval above!