Enhancement/8172 show view only users popup #9311
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Build files for 353fcc9 have been deleted. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Size Change: +840 B (+0.05%)
Total Size: 1.8 MB
| Filename | Size | Change |
|---|---|---|
./dist/assets/js/googlesitekit-activation-********************.js |
23.8 kB | +3 B (+0.01%) |
./dist/assets/js/googlesitekit-adminbar-********************.js |
34.5 kB | +16 B (+0.05%) |
./dist/assets/js/googlesitekit-api-********************.js |
9.97 kB | -1 B (-0.01%) |
./dist/assets/js/googlesitekit-datastore-forms-********************.js |
8.95 kB | -1 B (-0.01%) |
./dist/assets/js/googlesitekit-datastore-site-********************.js |
20.3 kB | -19 B (-0.09%) |
./dist/assets/js/googlesitekit-datastore-user-********************.js |
26.1 kB | -2 B (-0.01%) |
./dist/assets/js/googlesitekit-entity-dashboard-********************.js |
83.1 kB | +131 B (+0.16%) |
./dist/assets/js/googlesitekit-main-dashboard-********************.js |
155 kB | +152 B (+0.1%) |
./dist/assets/js/googlesitekit-modules-ads-********************.js |
32.8 kB | +11 B (+0.03%) |
./dist/assets/js/googlesitekit-modules-adsense-********************.js |
125 kB | -3 B (0%) |
./dist/assets/js/googlesitekit-modules-analytics-4-********************.js |
169 kB | +110 B (+0.07%) |
./dist/assets/js/googlesitekit-modules-********************.js |
22.1 kB | +1 B (0%) |
./dist/assets/js/googlesitekit-modules-pagespeed-insights-********************.js |
22.4 kB | +1 B (0%) |
./dist/assets/js/googlesitekit-modules-search-console-********************.js |
58.6 kB | -4 B (-0.01%) |
./dist/assets/js/googlesitekit-modules-tagmanager-********************.js |
32 kB | +1 B (0%) |
./dist/assets/js/googlesitekit-notifications-********************.js |
18.5 kB | -1 B (-0.01%) |
./dist/assets/js/googlesitekit-settings-********************.js |
83 kB | +2 B (0%) |
./dist/assets/js/googlesitekit-splash-********************.js |
89.4 kB | +17 B (+0.02%) |
./dist/assets/js/googlesitekit-user-input-********************.js |
63 kB | +11 B (+0.02%) |
./dist/assets/js/googlesitekit-vendor-********************.js |
321 kB | -1 B (0%) |
./dist/assets/js/googlesitekit-widgets-********************.js |
85.9 kB | +431 B (+0.5%) |
./dist/assets/js/googlesitekit-wp-dashboard-********************.js |
61.7 kB | -15 B (-0.02%) |
ℹ️ View Unchanged
| Filename | Size |
|---|---|
./dist/assets/css/googlesitekit-admin-css-********************.min.css |
57.5 kB |
./dist/assets/css/googlesitekit-adminbar-css-********************.min.css |
11.8 kB |
./dist/assets/css/googlesitekit-authorize-application-css-********************.min.css |
846 B |
./dist/assets/css/googlesitekit-wp-dashboard-css-********************.min.css |
8.2 kB |
./dist/assets/js/31-********************.js |
2.76 kB |
./dist/assets/js/32-********************.js |
2.25 kB |
./dist/assets/js/33-********************.js |
3.64 kB |
./dist/assets/js/34-********************.js |
935 B |
./dist/assets/js/35-********************.js |
892 B |
./dist/assets/js/36-********************.js |
3.12 kB |
./dist/assets/js/analytics-advanced-tracking-********************.js |
901 B |
./dist/assets/js/googlesitekit-ad-blocking-recovery-********************.js |
71.3 kB |
./dist/assets/js/googlesitekit-components-gm2-********************.js |
5.96 kB |
./dist/assets/js/googlesitekit-components-gm3-********************.js |
10.1 kB |
./dist/assets/js/googlesitekit-consent-mode-********************.js |
25.6 kB |
./dist/assets/js/googlesitekit-data-********************.js |
2.35 kB |
./dist/assets/js/googlesitekit-datastore-location-********************.js |
2.08 kB |
./dist/assets/js/googlesitekit-datastore-ui-********************.js |
9.91 kB |
./dist/assets/js/googlesitekit-events-provider-contact-form-7-********************.js |
646 B |
./dist/assets/js/googlesitekit-events-provider-easy-digital-downloads-********************.js |
624 B |
./dist/assets/js/googlesitekit-events-provider-mailchimp-********************.js |
630 B |
./dist/assets/js/googlesitekit-events-provider-ninja-forms-********************.js |
712 B |
./dist/assets/js/googlesitekit-events-provider-optin-monster-********************.js |
675 B |
./dist/assets/js/googlesitekit-events-provider-popup-maker-********************.js |
634 B |
./dist/assets/js/googlesitekit-events-provider-woocommerce-********************.js |
657 B |
./dist/assets/js/googlesitekit-events-provider-wpforms-********************.js |
633 B |
./dist/assets/js/googlesitekit-i18n-********************.js |
3.93 kB |
./dist/assets/js/googlesitekit-modules-reader-revenue-manager-********************.js |
21.7 kB |
./dist/assets/js/googlesitekit-polyfills-********************.js |
377 B |
./dist/assets/js/runtime-********************.js |
1.3 kB |
techanvil
requested changes
Sep 12, 2024
|
|
||
| await waitForRegistry(); | ||
|
|
||
| expect( container ).not.toBeEmptyDOMElement(); |
There was a problem hiding this comment.
This is not really testing much. I'd suggest doing a snapshot test:
Suggested change
| expect( container ).not.toBeEmptyDOMElement(); | |
| expect( container ).toMatchSnapshot(); |
When you examine the generated snapshot, you'll see that the only banner that gets rendered to the DOM is the AudienceSegmentationIntroductoryOverlayNotification.
This means that the test for PublicationApprovedOverlayNotification not being rendered below doesn't really prove much, because you don't have a case where it is rendered.
One way to tackle this is to update the conditions in this initial test so the PublicationApprovedOverlayNotification is also rendered. Or, you could add more test cases in which you explicitly test for each of PublicationApprovedOverlayNotification and AudienceSegmentationIntroductoryOverlayNotification being rendered.
You might also want to verify LinkAnalyticsAndAdSenseAccountsOverlayNotification and AnalyticsAndAdSenseAccountsDetectedAsLinkedOverlayNotification are also rendered. I don't want the scope to creep too much though, so would suggest doing this only if the issue is tracking well under the estimate.
Comment on lines
86
to
88
| expect( () => | ||
| getByText( 'Your Reader Revenue Manager publication is approved' ) | ||
| ).toThrow( /Unable to find an element with the text/ ); |
There was a problem hiding this comment.
We use queryByText() for cases like this:
Suggested change
| expect( () => | |
| getByText( 'Your Reader Revenue Manager publication is approved' ) | |
| ).toThrow( /Unable to find an element with the text/ ); | |
| expect( | |
| queryByText( 'Your Reader Revenue Manager publication is approved' ) | |
| ).not.toBeInTheDocument(); |
Comment on lines
109
to
114
| expect( () => | ||
| getByText( | ||
| 'You can now learn more about your site visitor groups by comparing different metrics' | ||
| ) | ||
| ).toThrow( /Unable to find an element with the text/ ); | ||
| } ); |
There was a problem hiding this comment.
As above:
Suggested change
| expect( () => | |
| getByText( | |
| 'You can now learn more about your site visitor groups by comparing different metrics' | |
| ) | |
| ).toThrow( /Unable to find an element with the text/ ); | |
| } ); | |
| expect( | |
| queryByText( | |
| 'You can now learn more about your site visitor groups by comparing different metrics' | |
| ) | |
| ).not.toBeInTheDocument(); |
Comment on lines
130
to
135
| expect( () => | ||
| getByText( | ||
| 'You can now learn more about your site visitor groups by comparing different metrics' | ||
| ) | ||
| ).toThrow( /Unable to find an element with the text/ ); | ||
| } ); |
There was a problem hiding this comment.
As above:
Suggested change
| expect( () => | |
| getByText( | |
| 'You can now learn more about your site visitor groups by comparing different metrics' | |
| ) | |
| ).toThrow( /Unable to find an element with the text/ ); | |
| } ); | |
| expect( | |
| queryByText( | |
| 'You can now learn more about your site visitor groups by comparing different metrics' | |
| ) | |
| ).not.toBeInTheDocument(); |
assets/js/util/scroll.js
Outdated
| @@ -31,7 +31,7 @@ import { finiteNumberOrZero } from './finite-number-or-zero'; | |||
| * @param {string} breakpoint The current breakpoint. | |||
| * @return {number} The offset to scroll to. | |||
| */ | |||
| export function getContextScrollTop( context, breakpoint ) { | |||
| export function getNavigationalScrollTop( context, breakpoint ) { | |||
There was a problem hiding this comment.
Please update the JSDoc to reflect this change. The description needs updating, and it should include a @since tag along these lines:
@since n.e.x.t Renamed from getContextScrollTop to getNavigationalScrollTop.
The context argument, and the variables named /context.*/ within the function should also be renamed.
For example this one could be called selector:
Suggested change
| export function getNavigationalScrollTop( context, breakpoint ) { | |
| export function getNavigationalScrollTop( selector, breakpoint ) { |
| } ); | ||
|
|
||
| expect( fetchMock ).toHaveFetched( dismissItemEndpoint ); | ||
| expect( getNavigationalScrollTopSpy ).toHaveBeenCalled(); |
There was a problem hiding this comment.
This is not such a useful test, as all it's doing it checking that a call to getNavigationScrollTopSpy() was made, rather than the behaviour we're interested in which is scrolling to the result of it.
I'd suggest checking that scrollTo() was called instead, with expected arguments. For example:
diff --git a/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationIntroductoryOverlayNotification.test.js b/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationIntroductoryOverlayNotification.test.js
index d818f28be7..0bf48624e0 100644
--- a/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationIntroductoryOverlayNotification.test.js
+++ b/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationIntroductoryOverlayNotification.test.js
@@ -40,6 +40,7 @@ const getNavigationalScrollTopSpy = jest.spyOn(
scrollUtils,
'getNavigationalScrollTop'
);
+const scrollToSpy = jest.spyOn( global, 'scrollTo' );
describe( 'AudienceSegmentationIntroductoryOverlayNotification', () => {
let registry;
@@ -90,7 +91,21 @@ describe( 'AudienceSegmentationIntroductoryOverlayNotification', () => {
expect( container ).toBeEmptyDOMElement();
} );
- it( 'should call getNavigationalScrollTopSpy and dismiss the notification when the notification is clicked', async () => {
+ it( 'should scroll to the traffic widget area and dismiss the notification when the notification is clicked', async () => {
+ getNavigationalScrollTopSpy.mockImplementation(
+ ( selector, breakpoint ) => {
+ if (
+ selector ===
+ '.googlesitekit-widget-area--mainDashboardTrafficAudienceSegmentation' &&
+ breakpoint === 'small'
+ ) {
+ return 12345;
+ }
+
+ return 0;
+ }
+ );
+
registry.dispatch( CORE_USER ).receiveGetDismissedItems( [] );
const { getByRole, waitForRegistry } = render(
@@ -115,6 +130,10 @@ describe( 'AudienceSegmentationIntroductoryOverlayNotification', () => {
} );
expect( fetchMock ).toHaveFetched( dismissItemEndpoint );
- expect( getNavigationalScrollTopSpy ).toHaveBeenCalled();
+
+ expect( scrollToSpy ).toHaveBeenCalledWith( {
+ top: 12345,
+ behavior: 'smooth',
+ } );
} );
} );
Comment on lines
67
to
68
| const widgetClass = | ||
| '.googlesitekit-widget-area--mainDashboardTrafficPrimary'; |
There was a problem hiding this comment.
I realised this is as per the IB, but in fact we should scroll to the audience segmentation widget area:
Suggested change
| const widgetClass = | |
| '.googlesitekit-widget-area--mainDashboardTrafficPrimary'; | |
| const widgetAreaClass = | |
| '.googlesitekit-widget-area--mainDashboardTrafficAudienceSegmentation'; |
| @@ -83,7 +100,7 @@ export default function AudienceSegmentationIntroductoryOverlayNotification() { | |||
| <Button | |||
| tertiary | |||
| disabled={ isDismissing } | |||
| onClick={ dismissNotification } | |||
| onClick={ scrollToWidgetAndDismissNotification } | |||
There was a problem hiding this comment.
The click handler is applied to the wrong button, this is the "Got it" button, it should be applied to the "Show me" button.
Comment on lines
37
to
39
| const audienceSegmentationSetupComplete = useSelect( ( select ) => | ||
| select( MODULES_ANALYTICS_4 ).getAudienceSegmentationSetupComplete() | ||
| ); |
There was a problem hiding this comment.
This results in the banner being shown for all users, whereas we only want to show it for secondary users i.e. not for the admin who initially set the feature up. We missed a few details in the IB.
In fact we are going to need to make a change to the getAudienceSegmentationSetupComplete() setting so that instead of simply storing a boolean, it should store the ID of the user who set the feature up, so we can then check here if the ID is a match for the current user. It's a bit of a pain but we should rename getAudienceSegmentationSetupComplete() to getAudienceSegmentationSetupCompletedBy() at the same time, to make it semantically clear.
So, please can you apply these changes to the setting, but also rename it to getAudienceSegmentationSetupCompletedBy(). You'll need to update a few tests as a result, it's unfortunate but we do need to do it here.
diff --git a/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js b/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js
index f7f41a78b0..4d8a407aa8 100644
--- a/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js
+++ b/assets/js/modules/analytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js
@@ -193,7 +193,7 @@ function AudienceSegmentationSetupCTAWidget( { Widget, WidgetNull } ) {
}
if (
- audienceSegmentationSetupComplete ||
+ audienceSegmentationSetupComplete !== null ||
configuredAudiences === undefined ||
configuredAudiences?.length ||
! analyticsIsDataAvailableOnLoad ||
diff --git a/assets/js/modules/analytics-4/datastore/audiences.js b/assets/js/modules/analytics-4/datastore/audiences.js
index a910222a32..2c40456f1a 100644
--- a/assets/js/modules/analytics-4/datastore/audiences.js
+++ b/assets/js/modules/analytics-4/datastore/audiences.js
@@ -522,8 +522,10 @@ const baseActions = {
return { error };
}
+ const userID = select( CORE_USER ).getID();
+
dispatch( MODULES_ANALYTICS_4 ).setAudienceSegmentationSetupComplete(
- true
+ userID
);
const { saveSettingsError } = yield commonActions.await(
diff --git a/assets/js/modules/analytics-4/index.js b/assets/js/modules/analytics-4/index.js
index 42833473f1..a7b5098c1a 100644
--- a/assets/js/modules/analytics-4/index.js
+++ b/assets/js/modules/analytics-4/index.js
@@ -175,7 +175,7 @@ export const registerWidgets = ( widgets ) => {
return (
availableAudiences?.length &&
configuredAudiences === null &&
- audienceSegmentationSetupComplete === true
+ audienceSegmentationSetupComplete !== null
);
},
},
diff --git a/includes/Modules/Analytics_4/Settings.php b/includes/Modules/Analytics_4/Settings.php
index 1531a67c77..3c74f3d8a4 100644
--- a/includes/Modules/Analytics_4/Settings.php
+++ b/includes/Modules/Analytics_4/Settings.php
@@ -107,7 +107,7 @@ class Settings extends Module_Settings implements Setting_With_Owned_Keys_Interf
'adsLinkedLastSyncedAt' => 0,
'availableAudiences' => null,
'availableAudiencesLastSyncedAt' => 0,
- 'audienceSegmentationSetupComplete' => false,
+ 'audienceSegmentationSetupComplete' => null,
'detectedEvents' => array(),
);
}
@@ -208,7 +208,9 @@ class Settings extends Module_Settings implements Setting_With_Owned_Keys_Interf
}
if ( isset( $option['audienceSegmentationSetupComplete'] ) ) {
- $option['audienceSegmentationSetupComplete'] = (bool) $option['audienceSegmentationSetupComplete'];
+ if ( ! is_int( $option['audienceSegmentationSetupComplete'] ) ) {
+ $option['audienceSegmentationSetupComplete'] = null;
+ }
}
}Once the above is done, you should update this condition as follows. However one more thing to consider is that it's starting to add quite a bit of logic specific to the AudienceSegmentationIntroductoryOverlayNotification component into OverlayNotificationsRenderer. So it might be preferable to move this condition into AudienceSegmentationIntroductoryOverlayNotification for cleaner encapsulation of its logic. That being the case you could remove the tests you've added for OverlayNotificationsRenderer.
Suggested change
| const audienceSegmentationSetupComplete = useSelect( ( select ) => | |
| select( MODULES_ANALYTICS_4 ).getAudienceSegmentationSetupComplete() | |
| ); | |
| const shouldShowAudienceSegmentationIntroductoryOverlay = useSelect( | |
| ( select ) => | |
| select( CORE_MODULES ).isModuleActive( 'analytics-4' ) && | |
| ( ! isViewOnly || | |
| select( CORE_USER ).canViewSharedModule( 'analytics-4' ) ) && | |
| select( | |
| MODULES_ANALYTICS_4 | |
| ).getAudienceSegmentationSetupCompletedBy() !== | |
| select( CORE_USER ).getID() | |
| ); |
assets/js/components/OverlayNotification/OverlayNotificationsRenderer.test.js
Outdated
Show resolved
Hide resolved
|
Size Change: +1.34 kB (+0.07%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
Thanks @techanvil for in-depth review and providing the suggestions. I have applied the recommended changes as per your suggestion and also removed the test file for Also, updated the QAB over the issue, so assigning you for another review. |
techanvil
requested changes
Sep 16, 2024
.../audience-segmentation/dashboard/AudienceSegmentationIntroductoryOverlayNotification.test.js
Outdated
Show resolved
Hide resolved
...nents/audience-segmentation/dashboard/AudienceSegmentationIntroductoryOverlayNotification.js
Outdated
Show resolved
Hide resolved
...nents/audience-segmentation/dashboard/AudienceSegmentationIntroductoryOverlayNotification.js
Outdated
Show resolved
Hide resolved
|
LGTM, nice one @ankitrox! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist