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

Fix/10036-duplicate-survey-triggers #10167

Open
wants to merge 21 commits into
base: develop
Choose a base branch
from
Open

Fix/10036-duplicate-survey-triggers #10167

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
8591467
Update survey trigger with concurrency lock and move timeout to trigg…
aaemnnosttv Feb 4, 2025
586dd87
Update tests for triggerSurvey and remove setSurveyTimeout.
aaemnnosttv Feb 4, 2025
dd5eab7
Update survey-trigger REST handler, remove survey-timeout endpoint.
aaemnnosttv Feb 4, 2025
75a34cf
Update tests for surveys controller.
aaemnnosttv Feb 4, 2025
08e5b12
Extract selectors once.
aaemnnosttv Feb 4, 2025
c3ad87f
Update survey endpoint mock helper.
aaemnnosttv Feb 5, 2025
8881208
Await triggerSurvey in action.
aaemnnosttv Feb 5, 2025
6ec40fa
Conditionally pass ttl in trigger fetch.
aaemnnosttv Feb 5, 2025
3d9934c
Update change metrics link tests.
aaemnnosttv Feb 5, 2025
3f0d205
Fix ABR setup tests.
aaemnnosttv Feb 5, 2025
cf1ea2f
Fix ABR setup success banner test.
aaemnnosttv Feb 5, 2025
3f009db
Fix settings card key metrics test.
aaemnnosttv Feb 5, 2025
dc72e93
Fix ABR setup CTA notice test.
aaemnnosttv Feb 5, 2025
3ef43f0
Fix KeyMetricsSetupCTAWidget test.
aaemnnosttv Feb 5, 2025
93ccdb1
Await survey trigger and remove timeout wait.
aaemnnosttv Feb 5, 2025
662355f
Fix unstable GA submitChanges test.
aaemnnosttv Feb 5, 2025
9498e57
Merge branch 'develop' into fix/10036-duplicate-survey-triggers.
aaemnnosttv Feb 5, 2025
abfaf7f
Remove references to removed survey-timeout endpoint.
aaemnnosttv Feb 5, 2025
b42a18d
Update surveys controller to test added timeout via trigger endpoint.
aaemnnosttv Feb 5, 2025
bd4dd63
Add fallback value for timeouts in case of error.
aaemnnosttv Feb 5, 2025
256306d
Add ttl to survey-trigger endpoint args schema.
aaemnnosttv Feb 5, 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
37 changes: 18 additions & 19 deletions assets/js/components/KeyMetrics/ChangeMetricsLink.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,16 +25,15 @@ import {
provideUserAuthentication,
provideUserInfo,
render,
waitFor,
} from '../../../../tests/js/test-utils';
import {
mockSurveyEndpoints,
surveyTimeoutEndpoint,
surveyTriggerEndpoint,
} from '../../../../tests/js/mock-survey-endpoints';
import { CORE_UI } from '../../googlesitekit/datastore/ui/constants';
import { KEY_METRICS_SELECTION_PANEL_OPENED_KEY } from './constants';
import {
CORE_USER,
KM_ANALYTICS_LEAST_ENGAGING_PAGES,
KM_ANALYTICS_MOST_ENGAGING_PAGES,
} from '../../googlesitekit/datastore/user/constants';
Expand Down Expand Up @@ -129,10 +128,14 @@ describe( 'ChangeMetricsLink', () => {
provideSiteInfo( registry, { keyMetricsSetupCompletedBy: 1 } );
mockSurveyEndpoints();

render( <ChangeMetricsLink />, { registry } );
const { waitForRegistry } = render( <ChangeMetricsLink />, {
registry,
} );
await waitForRegistry();

await waitFor( () =>
expect( fetchMock ).toHaveFetched( surveyTriggerEndpoint, {
expect( fetchMock ).toHaveFetched(
surveyTriggerEndpoint,
expect.objectContaining( {
body: {
data: { triggerID: 'view_kmw' },
},
Expand All @@ -149,33 +152,29 @@ describe( 'ChangeMetricsLink', () => {
} );
provideSiteInfo( registry, { keyMetricsSetupCompletedBy: 1 } );
provideUserInfo( registry, { id: 1 } );
registry.dispatch( CORE_USER ).receiveGetSurveyTimeouts( [] );
fetchMock.post( surveyTriggerEndpoint, {
status: 200,
body: {},
} );

fetchMock.post( surveyTimeoutEndpoint, {
status: 200,
body: {},
} );

fetchMock.getOnce( surveyTimeoutEndpoint, {
status: 200,
body: {},
const { waitForRegistry } = render( <ChangeMetricsLink />, {
registry,
} );

render( <ChangeMetricsLink />, { registry } );
await waitForRegistry();

await waitFor( () =>
expect( fetchMock ).toHaveFetched( surveyTriggerEndpoint, {
expect( fetchMock ).toHaveFetched(
surveyTriggerEndpoint,
expect.objectContaining( {
body: {
data: { triggerID: 'view_kmw' },
},
} )
);

await waitFor( () =>
expect( fetchMock ).toHaveFetched( surveyTriggerEndpoint, {
expect( fetchMock ).toHaveFetched(
surveyTriggerEndpoint,
expect.objectContaining( {
body: {
data: { triggerID: 'view_kmw_setup_completed' },
},
Expand Down
13 changes: 8 additions & 5 deletions assets/js/components/KeyMetrics/KeyMetricsSetupCTAWidget.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,11 +261,14 @@ describe( 'KeyMetricsSetupCTAWidget', () => {

// Should also trigger a survey view.
await waitFor( () =>
expect( fetchMock ).toHaveFetched( surveyTriggerEndpoint, {
body: {
data: { triggerID: 'view_kmw_setup_cta' },
},
} )
expect( fetchMock ).toHaveFetched(
surveyTriggerEndpoint,
expect.objectContaining( {
body: {
data: { triggerID: 'view_kmw_setup_cta' },
},
} )
)
);
} );

Expand Down
29 changes: 15 additions & 14 deletions assets/js/components/notifications/AdBlockingRecoverySetupSuccessBannerNotification.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,6 @@
* limitations under the License.
*/

/**
* External dependencies
*/
import { useIntersection as mockUseIntersection } from 'react-use';

/**
* Internal dependencies
*/
Expand All @@ -30,6 +25,7 @@ import {
fireEvent,
provideModules,
provideSiteInfo,
provideUserAuthentication,
render,
waitFor,
} from '../../../../tests/js/test-utils';
Expand All @@ -48,10 +44,11 @@ import AdBlockingRecoverySetupSuccessBannerNotification from './AdBlockingRecove
const mockTrackEvent = jest.spyOn( tracking, 'trackEvent' );
mockTrackEvent.mockImplementation( () => Promise.resolve() );

jest.mock( 'react-use' );

mockUseIntersection.mockImplementation( () => ( {
isIntersecting: true,
jest.mock( 'react-use', () => ( {
...jest.requireActual( 'react-use' ),
useIntersection: () => ( {
isIntersecting: true,
} ),
Comment on lines +47 to +51
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not sure how this wasn't broken before. The addition of requireActual here is something we do elsewhere and is necessary to preserve the normal function of other hooks from the package. Without this, the test was failing with useMountedState is not a function.

} ) );

describe( 'AdBlockingRecoverySetupSuccessBannerNotification', () => {
Expand All @@ -68,6 +65,7 @@ describe( 'AdBlockingRecoverySetupSuccessBannerNotification', () => {
connected: true,
},
] );
provideUserAuthentication( registry );

registry.dispatch( MODULES_ADSENSE ).setSettings( {
accountID: 'pub-123456',
Expand Down Expand Up @@ -139,11 +137,14 @@ describe( 'AdBlockingRecoverySetupSuccessBannerNotification', () => {

// The survey trigger endpoint should be called.
await waitFor( () =>
expect( fetchMock ).toHaveFetched( surveyTriggerEndpoint, {
body: {
data: { triggerID: 'abr_setup_completed' },
},
} )
expect( fetchMock ).toHaveFetched(
surveyTriggerEndpoint,
expect.objectContaining( {
body: {
data: { triggerID: 'abr_setup_completed' },
},
} )
)
);
} );
} );
11 changes: 0 additions & 11 deletions assets/js/components/settings/SettingsCardKeyMetrics.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,11 +14,6 @@
* limitations under the License.
*/

/**
* External dependencies
*/
import fetchMock from 'fetch-mock';

/**
* Internal dependencies
*/
Expand Down Expand Up @@ -91,12 +86,6 @@ export default {
'^/google-site-kit/v1/core/user/data/survey-trigger'
)
);
fetchMock.postOnce(
new RegExp(
'^/google-site-kit/v1/core/user/data/survey-timeout'
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This endpoint has been entirely removed.

),
{}
);
muteFetch(
new RegExp(
'^/google-site-kit/v1/core/user/data/survey-timeouts'
Expand Down
13 changes: 8 additions & 5 deletions assets/js/components/settings/SettingsCardKeyMetrics.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,14 @@ describe( 'SettingsCardKeyMetrics', () => {
} );

await waitFor( () =>
expect( fetchMock ).toHaveFetched( surveyTriggerEndpoint, {
body: {
data: { triggerID: 'view_kmw_setup_cta' },
},
} )
expect( fetchMock ).toHaveFetched(
surveyTriggerEndpoint,
expect.objectContaining( {
body: {
data: { triggerID: 'view_kmw_setup_cta' },
},
} )
)
);
} );

Expand Down
4 changes: 3 additions & 1 deletion assets/js/googlesitekit/datastore/site/first-party-mode.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ const baseActions = {
);

if ( results?.response?.isEnabled ) {
dispatch( CORE_USER ).triggerSurvey( 'fpm_setup_completed' );
yield commonActions.await(
dispatch( CORE_USER ).triggerSurvey( 'fpm_setup_completed' )
);
Comment on lines +121 to +123
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While this seems unrelated, it fixes stability issues in other tests where submitChanges is awaited where additional arbitrary waits were necessary.

}

return results;
Expand Down
Loading