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

Request scope only if it's needed #9840

Merged
merged 27 commits into from
Feb 21, 2025
Merged

Request scope only if it's needed #9840

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
7a33fe2
Check if scope is needed before requesting it.
nfmohit Dec 8, 2024
4218d13
Add test coverage.
nfmohit Dec 8, 2024
2afedfb
Fetch available audiences and custom dimensions as part of the hook c…
nfmohit Dec 9, 2024
6583661
Merge branch 'develop' into enhancement/#9595-groups-edit-scope.
nfmohit Jan 13, 2025
1ecbfe9
Merge branch 'develop' into enhancement/#9595-groups-edit-scope.
techanvil Feb 17, 2025
33bccad
Restructuring `onEnableGroups()` / `enableAudienceGroups()` to check …
techanvil Feb 17, 2025
57996b5
Update `needsAnalytics4EditScope()` to check whether the required cus…
techanvil Feb 17, 2025
1395ca8
Only sync audiences/custom dimensions once per audience group enabling.
techanvil Feb 17, 2025
89ddb91
Revert unneeded export of `requiredAudienceSlugs`.
techanvil Feb 17, 2025
644bf77
Revert property name change.
techanvil Feb 17, 2025
2acc27d
Fix `needsAnalytics4EditScope()` return value.
techanvil Feb 17, 2025
7c18dd4
Prevent `onEnableGroups()` from triggering unexpectedly.
techanvil Feb 19, 2025
bc4f6c2
Add JSDoc for new actions.
techanvil Feb 19, 2025
4c301e3
Remove redundant check for failed audiences.
techanvil Feb 19, 2025
159ed82
Remove stale comment.
techanvil Feb 19, 2025
55ed491
Remove unnecessary property rename.
techanvil Feb 19, 2025
6e69a3a
Rename variable.
techanvil Feb 19, 2025
707ffd8
Revert unneeded changes.
techanvil Feb 19, 2025
d65d097
Fix `SettingsCardVisitorGroups/SetupCTA` test.
techanvil Feb 19, 2025
30da750
Assert that the `sync-custom-dimensions` endpoint was requested.
techanvil Feb 19, 2025
43d819c
Fix `enableAudienceGroup()` tests.
techanvil Feb 19, 2025
22842ca
Fix `AudienceSegmentationSetupCTAWidget` test.
techanvil Feb 19, 2025
1bde374
Update `getSelectionFromExistingAudiences()` to be an async function …
techanvil Feb 20, 2025
5547592
Update `retrieveInitialAudienceSelection()` to be an async function r…
techanvil Feb 20, 2025
92e2e15
Rename `needsAnalytics4EditScope()` -> `determineNeedForAnalytics4Edi…
techanvil Feb 20, 2025
488b22d
Add a comment to clarify why we don't set isSaving back to false when…
techanvil Feb 20, 2025
9a03b2a
Clarify the `isSaving` comment.
techanvil Feb 20, 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
34 changes: 18 additions & 16 deletions ...lytics-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,24 +104,26 @@ function AudienceSegmentationSetupCTAWidget( { id, Notification } ) {

const { dismissItem, dismissPrompt } = useDispatch( CORE_USER );

const onSuccess = useCallback( () => {
invalidateResolution( 'getQueuedNotifications', [
viewContext,
NOTIFICATION_GROUPS.DEFAULT,
] );
Comment on lines +108 to +111
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why invalidate the default queue? I'd expect this should invalidate the setup CTA resolution, but I'm not entirely sure why it would invalidate this for either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I believe this was introduced as part of the banner notification refactor as a way to show the CTA immediately. We do have an issue in IB to address this in a more elegant way: #9453

Copy link
Collaborator

@aaemnnosttv aaemnnosttv Feb 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes; what I mean is that this invalidates the default group, but the CTA is registered with other setup CTAs in their respective group:

site-kit-wp/assets/js/modules/analytics-4/index.js

Lines 730 to 734 in 9a03b2a

[ AUDIENCE_SEGMENTATION_SETUP_CTA_NOTIFICATION ]: {
Component: AudienceSegmentationSetupCTAWidget,
priority: 10,
areaSlug: NOTIFICATION_AREAS.BANNERS_BELOW_NAV,
groupID: NOTIFICATION_GROUPS.SETUP_CTAS,

Edit: actually, this is about showing the setup success banner right away – which is in the default group – so that makes sense. I missed that detail in your response initially 👍

dismissPrompt( id, {
expiresInSeconds: 0,
} );
// Dismiss success notification in settings.
dismissItem( SETTINGS_VISITOR_GROUPS_SETUP_SUCCESS_NOTIFICATION );
}, [ dismissItem, dismissPrompt, id, invalidateResolution, viewContext ] );

const onError = useCallback( () => {
setShowErrorModal( true );
}, [ setShowErrorModal ] );

const { apiErrors, failedAudiences, isSaving, onEnableGroups } =
useEnableAudienceGroup( {
onSuccess: () => {
invalidateResolution( 'getQueuedNotifications', [
viewContext,
NOTIFICATION_GROUPS.DEFAULT,
] );
dismissPrompt( id, {
expiresInSeconds: 0,
} );
// Dismiss success notification in settings.
dismissItem(
SETTINGS_VISITOR_GROUPS_SETUP_SUCCESS_NOTIFICATION
);
},
onError: () => {
setShowErrorModal( true );
},
onSuccess,
onError,
} );

const { clearPermissionScopeError } = useDispatch( CORE_USER );
Expand Down
3 changes: 3 additions & 0 deletions ...s-4/components/audience-segmentation/dashboard/AudienceSegmentationSetupCTAWidget.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ import {
} from '../../../../../../../tests/js/test-utils';
import {
createTestRegistry,
freezeFetch,
muteFetch,
provideModules,
provideSiteInfo,
Expand Down Expand Up @@ -898,6 +899,8 @@ describe( 'AudienceSegmentationSetupCTAWidget', () => {
it( 'should track an event when the Retry button is clicked', () => {
mockTrackEvent.mockClear();

freezeFetch( syncAvailableAudiencesEndpoint );

act( () => {
fireEvent.click(
getByRole( 'button', { name: /retry/i } )
Expand Down
22 changes: 12 additions & 10 deletions ...alytics-4/components/audience-segmentation/settings/SettingsCardVisitorGroups/SetupCTA.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
/**
* WordPress dependencies
*/
import { useState } from '@wordpress/element';
import { useCallback, useState } from '@wordpress/element';
import { __ } from '@wordpress/i18n';

/**
Expand All @@ -46,18 +46,20 @@ export default function SetupCTA() {

const { dismissNotification } = useDispatch( CORE_NOTIFICATIONS );

const onSuccess = useCallback( () => {
// Dismiss success notification in dashboard.
dismissNotification( AUDIENCE_SEGMENTATION_SETUP_SUCCESS_NOTIFICATION );
}, [ dismissNotification ] );

const onError = useCallback( () => {
setShowErrorModal( true );
}, [ setShowErrorModal ] );

const { apiErrors, failedAudiences, isSaving, onEnableGroups } =
useEnableAudienceGroup( {
redirectURL: global.location.href,
onSuccess: () => {
// Dismiss success notification in dashboard.
dismissNotification(
AUDIENCE_SEGMENTATION_SETUP_SUCCESS_NOTIFICATION
);
},
onError: () => {
setShowErrorModal( true );
},
onSuccess,
onError,
} );

const setupErrorCode = useSelect( ( select ) =>
Expand Down
34 changes: 25 additions & 9 deletions ...cs-4/components/audience-segmentation/settings/SettingsCardVisitorGroups/SetupCTA.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,9 @@ describe( 'SettingsCardVisitorGroups SetupCTA', () => {
const syncAvailableAudiencesEndpoint = new RegExp(
'^/google-site-kit/v1/modules/analytics-4/data/sync-audiences'
);
const syncAvailableCustomDimensionsEndpoint = new RegExp(
'^/google-site-kit/v1/modules/analytics-4/data/sync-custom-dimensions'
);

beforeEach( () => {
registry = createTestRegistry();
Expand Down Expand Up @@ -157,6 +160,9 @@ describe( 'SettingsCardVisitorGroups SetupCTA', () => {
);

expect( fetchMock ).toHaveFetched( syncAvailableAudiencesEndpoint );
expect( fetchMock ).toHaveFetched(
syncAvailableCustomDimensionsEndpoint
);

await act( waitForDefaultTimeouts );
} );
Expand Down Expand Up @@ -185,6 +191,16 @@ describe( 'SettingsCardVisitorGroups SetupCTA', () => {

describe( 'AudienceErrorModal', () => {
it( 'should show the OAuth error modal when the required scopes are not granted', async () => {
fetchMock.postOnce( syncAvailableAudiencesEndpoint, {
body: [],
status: 200,
} );

fetchMock.postOnce( syncAvailableCustomDimensionsEndpoint, {
body: [],
status: 200,
} );

provideSiteInfo( registry, {
setupErrorCode: 'access_denied',
} );
Expand Down Expand Up @@ -221,13 +237,13 @@ describe( 'SettingsCardVisitorGroups SetupCTA', () => {
);
} );

// Allow the `trackEvent()` promise to resolve.
await waitForDefaultTimeouts();

// Verify the error is an OAuth error variant.
expect(
getByText( /Analytics update failed/i )
).toBeInTheDocument();
// Wait for the error modal to be displayed.
await waitFor( () => {
// Verify the error is an OAuth error variant.
expect(
getByText( /Analytics update failed/i )
).toBeInTheDocument();
} );

// Verify the "Get help" link is displayed.
expect( getByText( /get help/i ) ).toBeInTheDocument();
Expand All @@ -252,7 +268,7 @@ describe( 'SettingsCardVisitorGroups SetupCTA', () => {
data: { reason: ERROR_REASON_INSUFFICIENT_PERMISSIONS },
};

fetchMock.post( syncAvailableAudiencesEndpoint, {
fetchMock.postOnce( syncAvailableAudiencesEndpoint, {
body: errorResponse,
status: 500,
} );
Expand Down Expand Up @@ -292,7 +308,7 @@ describe( 'SettingsCardVisitorGroups SetupCTA', () => {
data: { status: 500 },
};

fetchMock.post( syncAvailableAudiencesEndpoint, {
fetchMock.postOnce( syncAvailableAudiencesEndpoint, {
body: errorResponse,
status: 500,
} );
Expand Down
Loading
Loading