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

Enhancement/9889 Refactor RRM Setup CTA Banner #9938

Open
wants to merge 46 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
46 commits
Select commit Hold shift + click to select a range
f5388ef
Remove rendering of WidgetNull.
jimmymadon Dec 23, 2024
5fa92aa
Wrap RRM notification registration with a feature flag check.
jimmymadon Dec 23, 2024
f3f92c2
Register RRM Setup CTA notification to the setup CTAs queue.
jimmymadon Dec 23, 2024
64e66e5
Remove direct rendering of RRM Setup CTA banner.
jimmymadon Dec 23, 2024
7118da0
Move canActivateModule check to notification registration.
jimmymadon Dec 23, 2024
3c2169d
Refactor the RRM Setup CTA Component to use the new notification comp…
jimmymadon Dec 23, 2024
1a24942
Add propTypes and remove HOC.
jimmymadon Dec 23, 2024
1bfffcb
Remove duplicated confirm GA tracking event.
jimmymadon Dec 23, 2024
5f61c3a
Remove redundant dismissal logic and GA tracking view event.
jimmymadon Dec 23, 2024
accf8e2
Remove all common dismissal logic.
jimmymadon Dec 23, 2024
5aec9ef
Fix incorrect title.
jimmymadon Dec 23, 2024
79a4bb7
Add support to dismiss and redisplay a notification.
jimmymadon Dec 23, 2024
20cc27e
Check prompts and dismissed items to determine notification dismissal…
jimmymadon Dec 23, 2024
007a69d
Use new notification prompt dismissal in RRM CTA component.
jimmymadon Dec 23, 2024
12ccc05
Prevent notification from showing after tooltip is rendered.
jimmymadon Dec 23, 2024
51fbdb1
Fix styles to match the legacy component.
jimmymadon Dec 23, 2024
48a16cd
Add the dismissRetries option to notification registration.
jimmymadon Dec 23, 2024
d10597f
Use dismissRetries property from notification property instead of pas…
jimmymadon Dec 23, 2024
7c12db9
Move dismissRetries for RRM setup banner from react component to noti…
jimmymadon Dec 23, 2024
91eed68
Only check for prompts if dismissRetries exists as a property.
jimmymadon Dec 23, 2024
7465e69
Add a default value to dismissExpires to prevent undefined checks.
jimmymadon Dec 23, 2024
010e78b
Improve notification dismissed selecter docs.
jimmymadon Dec 23, 2024
1570bae
Add a selector to fetch if a notification is on its final retry.
jimmymadon Dec 23, 2024
2775ad5
Add support for multiple dismiss labels.
jimmymadon Dec 23, 2024
e4c4f28
Fix undefined map select errors.
jimmymadon Dec 23, 2024
cec66b7
Move multiple dismiss labels out of common component.
jimmymadon Dec 26, 2024
af84a83
Refactor breakpoint SVG rendering for RRM banner.
jimmymadon Dec 26, 2024
a0a4c64
Refactor breakpoint SVG rendering for FPM banner.
jimmymadon Dec 26, 2024
3d2b8f3
Rename selector.
jimmymadon Dec 26, 2024
80bd3d5
Check if notification isDismissible before checking number of retries.
jimmymadon Dec 26, 2024
a17cb06
Provide FPM notification data for isDismissed check.
jimmymadon Dec 28, 2024
06b4c47
Provide FPM Notification data for isDismissed check in Ads settings.
jimmymadon Dec 28, 2024
45015c3
Separate out section of tests for isNotificationDismissed using dismi…
jimmymadon Dec 28, 2024
42f8b1b
Add tests for the isItemDismissed selector when using prompts.
jimmymadon Dec 28, 2024
7fc96bd
Merge branch 'develop' into enhancement/9889-refactor-rrm-setup-cta-b…
jimmymadon Jan 2, 2025
9217fed
Update rendering of RRM notification component in JS tests.
jimmymadon Jan 2, 2025
f6ac09f
Remove redundant trackEvent tests for RRM setup notification.
jimmymadon Jan 2, 2025
fa4d52d
Update test description.
jimmymadon Jan 2, 2025
ac4fe55
Refactor RRM notification registration to use an object of notificati…
jimmymadon Jan 2, 2025
dd8277f
Fix onCTAClick test.
jimmymadon Jan 2, 2025
0d23768
Prevent triggering survey when component is not rendered.
jimmymadon Jan 3, 2025
523073e
Fix all remaining tests for the RRM setup banner component.
jimmymadon Jan 3, 2025
1d99a46
Remove unnecessary use of view context when rendering the setup compo…
jimmymadon Jan 3, 2025
0fe3b7d
Update dismissed key to use notification ID.
jimmymadon Jan 3, 2025
99f56bc
Update storybook.
jimmymadon Jan 3, 2025
49527b5
Fix JSON response in test.
jimmymadon Jan 3, 2025
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
10 changes: 0 additions & 10 deletions assets/js/components/DashboardMainApp.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,6 @@ import {
AudienceSegmentationSetupCTAWidget,
AudienceSelectionPanel,
} from '../modules/analytics-4/components/audience-segmentation/dashboard';
import ReaderRevenueManagerSetupCTABanner from '../modules/reader-revenue-manager/components/dashboard/ReaderRevenueManagerSetupCTABanner';
import EntitySearchInput from './EntitySearchInput';
import DateRangeSelector from './DateRangeSelector';
import HelpMenu from './help/HelpMenu';
Expand Down Expand Up @@ -88,7 +87,6 @@ import {

export default function DashboardMainApp() {
const audienceSegmentationEnabled = useFeature( 'audienceSegmentation' );
const readerRevenueManagerEnabled = useFeature( 'rrmModule' );

const [ showSurveyPortal, setShowSurveyPortal ] = useState( false );

Expand Down Expand Up @@ -276,14 +274,6 @@ export default function DashboardMainApp() {
</Fragment>
) }

{ ! viewOnlyDashboard && (
<Fragment>
{ readerRevenueManagerEnabled && (
<ReaderRevenueManagerSetupCTABanner />
) }
</Fragment>
) }

<Notifications
areaSlug={ NOTIFICATION_AREAS.BANNERS_BELOW_NAV }
groupID={ NOTIFICATION_GROUPS.SETUP_CTAS }
Expand Down
15 changes: 4 additions & 11 deletions assets/js/components/notifications/FirstPartyModeSetupBanner.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,16 +129,9 @@ export default function FirstPartyModeSetupBanner( { id, Notification } ) {
return null;
}

const getBannerSVG = () => {
if ( breakpoint === BREAKPOINT_SMALL ) {
return FPMSetupCTASVGMobile;
}

if ( breakpoint === BREAKPOINT_TABLET ) {
return FPMSetupCTASVGTablet;
}

return FPMSetupCTASVGDesktop;
const breakpointSVGMap = {
[ BREAKPOINT_SMALL ]: FPMSetupCTASVGMobile,
[ BREAKPOINT_TABLET ]: FPMSetupCTASVGTablet,
};

return (
Expand Down Expand Up @@ -180,7 +173,7 @@ export default function FirstPartyModeSetupBanner( { id, Notification } ) {
} }
/>
}
SVG={ getBannerSVG() }
SVG={ breakpointSVGMap[ breakpoint ] || FPMSetupCTASVGDesktop }
/>
</Notification>
);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,10 @@ export default function NotificationWithSVG( {
</Cell>
<Cell
alignBottom
className={ `googlesitekit-setup-cta-banner__svg-wrapper--${ id }` }
className={ classNames(
'googlesitekit-setup-cta-banner__svg-wrapper',
`googlesitekit-setup-cta-banner__svg-wrapper--${ id }`
) }
{ ...svgSizeProps }
>
<SVG />
Expand Down
80 changes: 76 additions & 4 deletions assets/js/googlesitekit/notifications/datastore/notifications.js
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,11 @@ export const actions = {
* @param {WPComponent} [settings.Component] React component used to display the contents of this notification.
* @param {number} [settings.priority] Notification's priority for ordering (lower number is higher priority, like WordPress hooks). Ideally in increments of 10. Default 10.
* @param {string} [settings.areaSlug] The slug of the area where the notification should be rendered, e.g. notification-area-banners-above-nav.
* @param {string} [settings.groupID] The ID of the group of notifications that should be rendered in their own individual queue.
* @param {string} [settings.groupID] Optional. The ID of the group of notifications that should be rendered in their own individual queue.
* @param {Array.<string>} [settings.viewContexts] Array of Site Kit contexts, e.g. VIEW_CONTEXT_MAIN_DASHBOARD.
* @param {Function} [settings.checkRequirements] Optional. Callback function to determine if the notification should be queued.
* @param {boolean} [settings.isDismissible] Flag to check if the notification should be queued and is not dismissed.
* @param {boolean} [settings.isDismissible] Optional. Flag to check if the notification should be queued and is not dismissed.
* @param {number} [settings.dismissRetries] Optional. An integer number denoting how many times a notification should be shown again on dismissal.
* @return {Object} Redux-style action.
*/
registerNotification(
Expand All @@ -80,6 +81,7 @@ export const actions = {
viewContexts,
checkRequirements,
isDismissible,
dismissRetries = 0,
}
) {
invariant(
Expand Down Expand Up @@ -117,6 +119,7 @@ export const actions = {
viewContexts,
checkRequirements,
isDismissible,
dismissRetries,
},
},
type: REGISTER_NOTIFICATION,
Expand Down Expand Up @@ -226,6 +229,25 @@ export const actions = {
return;
}

// Use prompts if a notification should be shown again until it
// is dismissed for a certain number of retries.
if ( notification.dismissRetries > 0 ) {
const dismissCount = registry
.select( CORE_USER )
.getPromptDismissCount( id );

const expirationInSeconds =
dismissCount < notification.dismissRetries
? expiresInSeconds
: 0;

return yield commonActions.await(
registry.dispatch( CORE_USER ).dismissPrompt( id, {
expiresInSeconds: expirationInSeconds,
} )
);
}

return yield commonActions.await(
registry
.dispatch( CORE_USER )
Expand Down Expand Up @@ -407,8 +429,8 @@ export const selectors = {
/**
* Determines whether a notification is dismissed or not.
*
* Currently, this selector simply forwards the call to the dismissed items API.
* We can potentially add more notification-specific logic here in the future.
* If the notification should appear again for a certain number of times after dismissal,
* then we store them as prompts. So we check for dismissed prompts instead of dismissed items.
*
* @since 1.132.0
*
Expand All @@ -418,9 +440,59 @@ export const selectors = {
*/
isNotificationDismissed: createRegistrySelector(
( select ) => ( state, id ) => {
const notification =
select( CORE_NOTIFICATIONS ).getNotification( id );

if ( notification === undefined ) {
return undefined;
}

if ( notification.dismissRetries > 0 ) {
return select( CORE_USER ).isPromptDismissed( id );
}

return select( CORE_USER ).isItemDismissed( id );
}
),
/**
* Determines whether a notification that can reappear again for a fixed number of times
* on dismissal is at its final appearance.
*
* @since n.e.x.t
*
* @param {Object} state Data store's state.
* @param {string} id Notification id.
* @return {(boolean|undefined)} TRUE if notification is on its final retry, otherwise FALSE, `undefined` if not resolved yet.
*/
isNotificationDismissalFinal: createRegistrySelector(
( select ) => ( state, id ) => {
const notification =
select( CORE_NOTIFICATIONS ).getNotification( id );

if ( notification === undefined ) {
return undefined;
}

invariant(
notification.isDismissible,
'Notification should be dismissible to check if a notification is on its final dismissal.'
);

// If a notification does not have retries, it always will be on its final render.
if ( notification.dismissRetries === 0 ) {
return true;
}

const dismissCount =
select( CORE_USER ).getPromptDismissCount( id );

if ( dismissCount >= notification.dismissRetries ) {
return true;
}

return false;
}
),
};

export default {
Expand Down
119 changes: 99 additions & 20 deletions assets/js/googlesitekit/notifications/datastore/notifications.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
*/
import {
createTestRegistry,
provideNotifications,
untilResolved,
} from '../../../../../tests/js/utils';
import { render } from '../../../../../tests/js/test-utils';
Expand All @@ -42,6 +43,9 @@ describe( 'core/notifications Notifications', () => {
const fetchDismissItem = new RegExp(
'^/google-site-kit/v1/core/user/data/dismiss-item'
);
const fetchGetDismissedPrompts = new RegExp(
'^/google-site-kit/v1/core/user/data/dismissed-prompts'
);

let registry;
let store;
Expand Down Expand Up @@ -576,30 +580,105 @@ describe( 'core/notifications Notifications', () => {
} );

describe( 'isNotificationDismissed', () => {
let isNotificationDismissed;
beforeEach( () => {
( { isNotificationDismissed } =
registry.select( CORE_NOTIFICATIONS ) );
} );
describe( 'when using dismissed items', () => {
let isNotificationDismissed;
beforeEach( () => {
// Register the Gathering Data Notification as a test
provideNotifications( registry );

it( 'should return undefined if getDismissedItems selector is not resolved yet', async () => {
fetchMock.getOnce( fetchGetDismissedItems, { body: [] } );
expect( isNotificationDismissed( 'foo' ) ).toBeUndefined();
await untilResolved( registry, CORE_USER ).getDismissedItems();
} );
( { isNotificationDismissed } =
registry.select( CORE_NOTIFICATIONS ) );
} );

it( 'should return TRUE if the notification is dismissed', () => {
registry
.dispatch( CORE_USER )
.receiveGetDismissedItems( [ 'foo', 'bar' ] );
expect( isNotificationDismissed( 'foo' ) ).toBe( true );
it( 'should return undefined if getDismissedItems selector is not resolved yet', async () => {
fetchMock.getOnce( fetchGetDismissedItems, { body: [] } );
expect(
isNotificationDismissed( 'gathering-data-notification' )
).toBeUndefined();
await untilResolved(
registry,
CORE_USER
).getDismissedItems();
} );

it( 'should return TRUE if the notification is dismissed', () => {
registry
.dispatch( CORE_USER )
.receiveGetDismissedItems( [
'gathering-data-notification',
'some-other-notification',
] );
expect(
isNotificationDismissed( 'gathering-data-notification' )
).toBe( true );
} );

it( 'should return FALSE if the notification is not dismissed', () => {
registry
.dispatch( CORE_USER )
.receiveGetDismissedItems( [ 'foo', 'bar' ] );
expect(
isNotificationDismissed( 'gathering-data-notification' )
).toBe( false );
} );
} );
describe( 'when using dismissed prompts', () => {
let isNotificationDismissed;
beforeEach( () => {
provideNotifications( registry, {
'test-notification-using-prompts': {
Component: () => {},
areaSlug: NOTIFICATION_AREAS.BANNERS_ABOVE_NAV,
viewContexts: [ VIEW_CONTEXT_MAIN_DASHBOARD ],
priority: 11,
checkRequirements: () => true,
isDismissible: false,
dismissRetries: 1,
},
} );

( { isNotificationDismissed } =
registry.select( CORE_NOTIFICATIONS ) );
} );

it( 'should return undefined if getDismissedPrompts selector is not resolved yet', async () => {
fetchMock.getOnce( fetchGetDismissedPrompts, { body: [] } );
expect(
isNotificationDismissed(
'test-notification-using-prompts'
)
).toBeUndefined();
await untilResolved(
registry,
CORE_USER
).getDismissedPrompts();
} );

it( 'should return TRUE if the notification is dismissed', () => {
registry.dispatch( CORE_USER ).receiveGetDismissedPrompts( {
'test-notification-using-prompts': {
expires: 0,
count: 1,
},
'some-other-notification': { expires: 0, count: 2 },
} );
expect(
isNotificationDismissed(
'test-notification-using-prompts'
)
).toBe( true );
} );

it( 'should return FALSE if the notification is not dismissed', () => {
registry
.dispatch( CORE_USER )
.receiveGetDismissedItems( [ 'foo', 'bar' ] );
expect( isNotificationDismissed( 'baz' ) ).toBe( false );
it( 'should return FALSE if the notification is not dismissed', () => {
registry
.dispatch( CORE_USER )
.receiveGetDismissedPrompts( [ 'foo', 'bar' ] );
expect(
isNotificationDismissed(
'test-notification-using-prompts'
)
).toBe( false );
} );
} );
} );
} );
Expand Down
11 changes: 11 additions & 0 deletions assets/js/modules/ads/datastore/settings.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,17 @@ describe( 'modules/ads settings', () => {
FPM_SETUP_CTA_BANNER_NOTIFICATION,
] );

provideNotifications(
registry,
{
[ FPM_SETUP_CTA_BANNER_NOTIFICATION ]:
DEFAULT_NOTIFICATIONS[
FPM_SETUP_CTA_BANNER_NOTIFICATION
],
},
{ overwrite: true }
);

fetchMock.postOnce( settingsEndpoint, ( url, opts ) => ( {
body: JSON.parse( opts.body )?.data,
status: 200,
Expand Down
11 changes: 11 additions & 0 deletions assets/js/modules/analytics-4/datastore/settings.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -518,6 +518,17 @@ describe( 'modules/analytics-4 settings', () => {
FPM_SETUP_CTA_BANNER_NOTIFICATION,
] );

provideNotifications(
registry,
{
[ FPM_SETUP_CTA_BANNER_NOTIFICATION ]:
DEFAULT_NOTIFICATIONS[
FPM_SETUP_CTA_BANNER_NOTIFICATION
],
},
{ overwrite: true }
);

const validSettings = {
accountID: fixtures.createProperty._accountID,
propertyID: fixtures.createProperty._id,
Expand Down
Loading
Loading