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.

Sign up for GitHub

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

Enhance/#6932 - GA4 dashboard prompt banner notification #7026

Merged
merged 8 commits into from
May 16, 2023
Merged

Enhance/#6932 - GA4 dashboard prompt banner notification #7026

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
03823b0
Add GA4_DASHBOARD_VIEW_NOTIFICATION_ID constant.
hussain-t May 11, 2023
2abc24e
Dismiss notification in the banner.
hussain-t May 11, 2023
7af2c5c
Dismiss notification in the submitChanges.
hussain-t May 11, 2023
934d091
Merge branch 'develop' of github.com:google/site-kit-wp into enhance/…
hussain-t May 11, 2023
038e4ef
Use isItemDismissed selector and improvements.
hussain-t May 11, 2023
084f73b
Add tests for dismissing the ga4 dashboard view banner.
hussain-t May 11, 2023
ade237e
Refactor `submitChanges` to not violate complexity rule.
kuasha420 May 16, 2023
e6b0044
Update tests as per CR feedback.
kuasha420 May 16, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 19 additions & 4 deletions assets/js/components/notifications/SwitchGA4DashboardViewNotification.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,15 +30,25 @@ import { CORE_SITE } from '../../googlesitekit/datastore/site/constants';
import { CORE_UI } from '../../googlesitekit/datastore/ui/constants';
import {
DASHBOARD_VIEW_GA4,
GA4_DASHBOARD_VIEW_NOTIFICATION_ID,
MODULES_ANALYTICS,
} from '../../modules/analytics/datastore/constants';
import BannerNotification from './BannerNotification';
import { CORE_USER } from '../../googlesitekit/datastore/user/constants';
const { useDispatch, useSelect } = Data;

export default function SwitchGA4DashboardViewNotification() {
const shouldPromptGA4DashboardView = useSelect( ( select ) =>
select( MODULES_ANALYTICS ).shouldPromptGA4DashboardView()
);
const shouldPromptGA4DashboardView = useSelect( ( select ) => {
const isNotificationDismissed = select( CORE_USER ).isItemDismissed(
GA4_DASHBOARD_VIEW_NOTIFICATION_ID
);

if ( isNotificationDismissed ) {
return false;
}

return select( MODULES_ANALYTICS ).shouldPromptGA4DashboardView();
} );

const ga4DocumentationURL = useSelect( ( select ) =>
select( CORE_SITE ).getDocumentationLinkURL(
Expand All @@ -47,13 +57,15 @@ export default function SwitchGA4DashboardViewNotification() {
);

const { setValue } = useDispatch( CORE_UI );
const { dismissItem } = useDispatch( CORE_USER );
const { setDashboardView, saveSettings } = useDispatch( MODULES_ANALYTICS );
const handleCTAClick = () => {
setValue( 'forceInView', true );
setValue( 'showGA4ReportingTour', true );

setDashboardView( DASHBOARD_VIEW_GA4 );
saveSettings();
dismissItem( GA4_DASHBOARD_VIEW_NOTIFICATION_ID );

return { dismissOnCTAClick: false };
};
Expand All @@ -64,7 +76,7 @@ export default function SwitchGA4DashboardViewNotification() {

return (
<BannerNotification
id="switch-ga4-dashboard-view"
id={ GA4_DASHBOARD_VIEW_NOTIFICATION_ID }
title={ __(
'Display data from Google Analytics 4 on your dashboard',
'google-site-kit'
Expand All @@ -77,6 +89,9 @@ export default function SwitchGA4DashboardViewNotification() {
ctaLabel={ __( 'Update dashboard', 'google-site-kit' ) }
onCTAClick={ handleCTAClick }
dismiss={ __( 'Maybe later', 'google-site-kit' ) }
onDismiss={ () => {
dismissItem( GA4_DASHBOARD_VIEW_NOTIFICATION_ID );
} }
WinImageSVG={ () => <GA4SuccessGreenSVG /> }
learnMoreLabel={ __( 'Learn what’s new', 'google-site-kit' ) }
learnMoreURL={ ga4DocumentationURL }
Expand Down
2 changes: 2 additions & 0 deletions assets/js/modules/analytics/datastore/constants.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,5 @@ export const PROPERTY_TYPE_GA4 = 'ga4';

export const DASHBOARD_VIEW_GA4 = 'google-analytics-4';
export const DASHBOARD_VIEW_UA = 'universal-analytics';

export const GA4_DASHBOARD_VIEW_NOTIFICATION_ID = 'switch-ga4-dashboard-view';
15 changes: 15 additions & 0 deletions assets/js/modules/analytics/datastore/settings.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ import {
FORM_SETUP,
DASHBOARD_VIEW_GA4,
DASHBOARD_VIEW_UA,
GA4_DASHBOARD_VIEW_NOTIFICATION_ID,
} from './constants';
import { createStrictSelect } from '../../../googlesitekit/data/utils';
import { CORE_MODULES } from '../../../googlesitekit/modules/datastore/constants';
Expand Down Expand Up @@ -84,6 +85,7 @@ async function submitGA4Changes( { select, dispatch } ) {
return await dispatch( MODULES_ANALYTICS_4 ).submitChanges();
}

// eslint-disable-next-line complexity
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hussain-t, I really don't think we should be disabling this rule in any new scenarios. By disabling the rule it means we can keep adding more complexity to this function without any warning or check on it, which is a bit of a step backward.

Please can you instead refactor this function a bit, extracting some of it into new functions if needs be.

export async function submitChanges( registry ) {
const { select, dispatch } = registry;

Expand Down Expand Up @@ -178,6 +180,19 @@ export async function submitChanges( registry ) {
if ( ! select( CORE_USER ).isTourDismissed( ga4Reporting.slug ) ) {
dispatch( CORE_USER ).dismissTour( ga4Reporting.slug );
}

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
);
}
}

return {};
Expand Down
137 changes: 137 additions & 0 deletions assets/js/modules/analytics/datastore/settings.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ import {
PROFILE_CREATE,
DASHBOARD_VIEW_UA,
DASHBOARD_VIEW_GA4,
GA4_DASHBOARD_VIEW_NOTIFICATION_ID,
} from './constants';
import * as fixtures from './__fixtures__';
import {
Expand Down Expand Up @@ -134,6 +135,11 @@ describe( 'modules/analytics settings', () => {
registry
.dispatch( CORE_USER )
.receiveGetDismissedTours( [ ga4Reporting.slug ] );
registry
.dispatch( CORE_USER )
.receiveGetDismissedItems( [
GA4_DASHBOARD_VIEW_NOTIFICATION_ID,
] );
} );

it( 'dispatches createProperty if the "set up a new property" option is chosen', async () => {
Expand Down Expand Up @@ -621,6 +627,132 @@ describe( 'modules/analytics settings', () => {
} );
} );

describe( 'dismiss switch ga4 dashboard view notification', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should capitalise the initials in GA4 when using it in a sentence... I'd also make the following minor grammatical tweak:

Suggested change
describe( 'dismiss switch ga4 dashboard view notification', () => {
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 ga4 dashboard view notification if it is a UA dashboard view', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above:

Suggested change
it( 'should not dismiss the switch ga4 dashboard view notification if it is a UA dashboard view', async () => {
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();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seeing as we have to wait for resolvers to run in the successful test case below, we should probably add it here too, to ensure this test isn't giving us a false positive.

Suggested change
// 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 ga4 dashboard view notification if it is already dismissed', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above:

Suggested change
it( 'should not dismiss the switch ga4 dashboard view notification if it is already dismissed', async () => {
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();

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above:

Suggested change
// 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 ga4 dashboard view notification if it has not been dismissed yet', async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above:

Suggested change
it( 'should dismiss the switch ga4 dashboard view notification if it has not been dismissed yet', async () => {
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 );
} );
} );

describe( 'analytics-4', () => {
beforeEach( () => {
registry
Expand Down Expand Up @@ -768,6 +900,11 @@ describe( 'modules/analytics settings', () => {
registry
.dispatch( MODULES_ANALYTICS )
.receiveGetSettings( validSettings );
registry
.dispatch( CORE_USER )
.receiveGetDismissedItems( [
GA4_DASHBOARD_VIEW_NOTIFICATION_ID,
] );
expect(
registry.select( MODULES_ANALYTICS ).haveSettingsChanged()
).toBe( false );
Expand Down