Show the Introductory Popup / Banner to view-only users and secondary admins

[techanvil]

Feature Description

Show the Introductory Popup / Banner to view-only users and secondary admins to introduce the feature once it has been set up.

See dashboard sharing and secondary admins in the design doc.

---

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

Acceptance criteria

• Given that the Audience Segmentation feature has been set up by the primary admin (i.e. an admin has enabled the feature via the Setup CTA Banner, or the Setup CTA on the Settings screen):
  • When a secondary admin, or a view-only user subsequently visits the dashboard, the Introductory Popup / Banner should be displayed (see Add the Introductory Popup / Banner (Storybook) #8171).
  • Clicking on the Popup / Banner's Show me CTA will scroll to the Audiences Widget Area, and permanently dismiss the Popup / Banner.
  • Clicking on the Popup / Banner's Got it CTA will permanently dismiss the Popup / Banner.

Implementation Brief

• Rendering the banner/popup is handled here. It will be visible when the Audience Segmentation feature is enabled.

  • Update the condition to also check if the Audience Segmentation feature has already been setup. This can be checked with the flag audienceSegmentationSetupCompleted in Analytics settings being introduced in Implement the secondary user setup #8130.

• In assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationIntroductoryOverlayNotification.js

  • Import getContextScrollTop from assets/js/util/scroll.js.
  • Add a new handler scrollToWidgetAndDismissNotification for scrolling and dismissing the notification.
    • This function should call the dismissNotification.
    • Pass context as googlesitekit-widget-area--mainDashboardTrafficPrimary and breakpoint using useBreakpoint hook to getContextScrollTop.

• As the getContextScrollTop is now being used to get the position for widgets and widget areas as well as widget context elements, rename this function to getNavigationalScrollTop and update all its usages indicating it's a general purpose utility for use in scrolling to an area which is within the remit of the dashboard navigation, below the header.

Test Coverage

• Add a test for AudienceSegmentationIntroductoryOverlayNotification component to test the scroll behaviour.
• Add a test for AudienceSegmentationIntroductoryOverlayNotification component to test the dismissal behaviour.
• Update story for AudienceSegmentationIntroductoryOverlayNotification to include a new scenario dependent on audienceSegmentationSetupCompleted flag.

QA Brief

Changelog entry

[techanvil]

Hi @ankitrox, thanks for drafting this IB. Here are a few points:

• Rendering the banner/popup is already been taken care here. It will be visible when the audience segmentation feature is enabled.

• Actually, while this is true in itself, the banner should not be shown purely based on whether the feature flag is enabled. In fact the banner should only be shown under the conditions described in the AC, i.e. once the feature has been setup by a primary admin. I imagine we can add a check for audienceSegmentationSetupComplete (being introduced via Implement the secondary user setup #8130) in the conditions for rendering the banner (either in OverlayNotificationsRenderer, or to potentially return null in AudienceSegmentationIntroductoryOverlayNotification).
• Further to the above I've added Implement the secondary user setup #8130 as a dependency to this issue.

---

• As to the rest of the IB - there's no need to add a scrollToAudienceWidgetArea parameter and modify dismissNotification. Instead, we should simply follow the example of AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification and provide a separate click handler for the Show me CTA with a similar implementation:

site-kit-wp/assets/js/components/OverlayNotification/AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification.js

Lines 182 to 201 in 51452f6

	const scrollToWidgetAndDismissNotification = ( event ) => {
	event.preventDefault();
	dismissNotification();
	setTimeout( () => {
	const widgetClass =
	'.googlesitekit-widget--adsenseTopEarningPagesGA4';
	global.history.replaceState(
	{},
	'',
	`#${ ANCHOR_ID_MONETIZATION }`
	);
	global.scrollTo( {
	top: getContextScrollTop( widgetClass, breakpoint ),
	behavior: 'smooth',
	} );
	}, 50 );
	};

• We should pass .googlesitekit-widget-area--mainDashboardTrafficAudienceSegmentation rather than googlesitekit-widget-area--mainDashboardTrafficPrimary to getContextScrollTop() to ensure we scroll to the Audiences Widget Area rather than the top of the Traffic context.
• Incidentally seeing as getContextScrollTop() is now being used to get the position for widgets and widget areas as well as widget context elements, we should rename the function. I'd suggest a new name of getNavigationalScrollTop() indicating it's a general purpose utility for use in scrolling to an area which is within the remit of the dashboard navigation, below the header.

[ankitrox]

Hi @techanvil ,

Thanks for reviewing the IB. I've addressed the feedback points mentioned in the comments and updated the IB.

Assigning this to you for re-review.

[techanvil]

Thanks @ankitrox. The main body of the IB is looking good (although it needed a tweak for formatting, please review). I have though realised the Test Coverage section is written in a way that would encourage more spying on implementation details.

We don't really want to test whether getContextScrollTop() has been called as that's not verifying the actual behaviour of the component. If anything a spy and assertion on global.scrollTo() would be more appropriate here, that being the public DOM API call that would be the clearest indication we've actually scrolled or not.

Also, although it does look like we're in the process of setting a precedent of testing whether an action has been dispatched by spying on it, we should still try to stick with our more usual approach where we can. As such the more conventional approach would be to mock the dismiss-item endpoint and assert the DOM after interacting with the component.

For example here are a couple of existing tests for dismissing a UI item:

site-kit-wp/assets/js/components/OverlayNotification/AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification.test.js

Lines 436 to 466 in 817317d

	it( 'clicking the `Show me` button dismisses the notification', async () => {
	provideAnalytics4MockReport( registry, reportOptions );
	fetchMock.getOnce( fetchGetDismissedItemsRegExp, { body: [] } );
	fetchMock.postOnce( fetchDismissItemRegExp, {
	body: [ ANALYTICS_ADSENSE_LINKED_OVERLAY_NOTIFICATION ],
	} );
	const { container, getByRole, waitForRegistry } = render(
	<AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification />,
	{
	registry,
	viewContext: VIEW_CONTEXT_MAIN_DASHBOARD,
	}
	);
	await waitForRegistry();
	expect( container ).toHaveTextContent(
	'Data is now available for the pages that earn the most AdSense revenue'
	);
	act( () => {
	fireEvent.click( getByRole( 'button', { name: /show me/i } ) );
	} );
	await waitForRegistry();
	expect( container ).not.toHaveTextContent(
	'Data is now available for the pages that earn the most AdSense revenue'
	);
	} );

site-kit-wp/assets/js/modules/adsense/components/dashboard/AdSenseConnectCTAWidget.test.js

Lines 113 to 125 in 817317d

	it( 'should close the modal on clicking the dismiss button', async () => {
	// eslint-disable-next-line require-await
	await act( async () => {
	fireEvent.click(
	document.querySelector(
	'.googlesitekit-tooltip-buttons > button'
	)
	);
	} );
	expect(
	document.querySelector( '.googlesitekit-tour-tooltip' )
	).not.toBeInTheDocument();
	} );

To be honest, we don't usually specify the test coverage in an IB in such detail. I would suggest simply specifying that we provide test coverage for the relevant behaviour, and leave the details to implementation time.

[ankitrox]

Hi @techanvil
Thanks for the feedback. I agree that details in the test coverage section is not required, so I updated the section just to mention which tests will be required to cover the feature.

Assigning this to you to re-review the test coverage section.

Thanks again!

[techanvil]

That's great, thanks @ankitrox. The IB LGTM! ✅