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

Migration Flow: Fix multiple stickers being added simultaneously when navigating to the "DIY" option #95612

Merged
merged 5 commits into from
Oct 31, 2024
Merged

Migration Flow: Fix multiple stickers being added simultaneously when navigating to the "DIY" option #95612

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
df6a9e0
Migration Flow: Fix race condition when adding the blog stickers
m1r0 Oct 23, 2024
682f844
Remove excessive fragment tag
m1r0 Oct 28, 2024
0252787
Fix loading state flickering
m1r0 Oct 28, 2024
a9fd24e
Fix tests
m1r0 Oct 28, 2024
eff54d4
Fix flickering after clicking an option
m1r0 Oct 28, 2024
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
14 changes: 12 additions & 2 deletions client/data/site-migration/use-update-migration-status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,13 +20,23 @@ export const useUpdateMigrationStatus = () => {
} ),
} );

const { mutate: updateMigrationStatusMutate, ...updateStatusMutationRest } = updateStatusMutation;
const {
mutate: updateMigrationStatusMutate,
mutateAsync: updateMigrationStatusMutateAsync,
...updateStatusMutationRest
} = updateStatusMutation;

const updateMigrationStatus = useCallback(
( targetBlogId: SiteId, statusSticker: string ) =>
updateMigrationStatusMutate( { targetBlogId, statusSticker } ),
[ updateMigrationStatusMutate ]
);

return { updateMigrationStatus, updateStatusMutationRest };
const updateMigrationStatusAsync = useCallback(
( targetBlogId: SiteId, statusSticker: string ) =>
updateMigrationStatusMutateAsync( { targetBlogId, statusSticker } ),
[ updateMigrationStatusMutateAsync ]
);

return { updateMigrationStatus, updateMigrationStatusAsync, updateStatusMutationRest };
};
29 changes: 22 additions & 7 deletions ...epper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import { StepContainer } from '@automattic/onboarding';
import { useTranslate } from 'i18n-calypso';
import { FC, useMemo } from 'react';
import { FC, useMemo, useState } from 'react';
Copy link
Contributor

Choose a reason for hiding this comment

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

The component SiteMigrationHowToMigrate is getting a little complex (big). I started extracting part of the logic to another hook here. I think it would be nice to base this PR there and also add the loading logic there, returning the loading state. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good!

I've updated the code but I'm a bit unsure if my implementation is aligned with your vision. 😅 Could you give it a look again?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes! That looks great!

Copy link
Contributor

@renatho renatho Oct 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

And I thought of another scenario where we could have the same race condition issue.
If the user is in a slow connection and they click on one of the options quickly it would be registering the "pending" sticker, and could try to register the "pending-{TYPE}" at the same time.

If we rebase this PR, we could also add the loader for this scenario (when creating the first "pending" sticker).

If we follow the suggestion of the other comment, I think it would assume this behavior automatically.

Copy link
Member Author

@m1r0 m1r0 Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think I've managed to fix this scenario but I'm facing an issue with the component loading first and then the loader appearing.

aAeTa2.mp4

Can I fix this without having an extra state in the component?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hm! I noticed that it's because this fetch depends on the siteId, and it's taking some time to be ready because it's coming from the store. I wonder if we could get it directly from the query string, so we would have this information immediately when the component is loaded to start already with the loader. WDYT?

Copy link
Member Author

@m1r0 m1r0 Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Could you take a look at this approach? I wasn't able to avoid the site loading because of this check. Do you think it's good enough? 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

This approach looks good to me!

But I just noticed another flickering after clicking on the options ("Do it for me", for example). It finishes the loading (the sticker creating that comes from the click), and displays the step again very quickly before calling the onSubmit.

Screen.Recording.2024-10-28.at.13.02.34.mov

I think we'll probably need an extra state here, similar to your previous approach. Maybe something that we set as loading after clicking and keep this state unchanged, so onSubmit is called when it's still in the loading state.

Copy link
Member Author

@m1r0 m1r0 Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Oh, I didn't noticed that one. I've fixed it here following your suggestion. Thank you for your patience!

import DocumentHead from 'calypso/components/data/document-head';
import FormattedHeader from 'calypso/components/formatted-header';
import { LoadingEllipsis } from 'calypso/components/loading-ellipsis';
import { useAnalyzeUrlQuery } from 'calypso/data/site-profiler/use-analyze-url-query';
import { useHostingProviderQuery } from 'calypso/data/site-profiler/use-hosting-provider-query';
import { HOW_TO_MIGRATE_OPTIONS } from 'calypso/landing/stepper/constants';
Expand Down Expand Up @@ -62,28 +63,42 @@ const SiteMigrationHowToMigrate: FC< Props > = ( props ) => {
urlData
);

const { setPendingMigration } = usePendingMigrationStatus( { onSubmit: navigation.submit } );
const { setPendingMigration, isLoading: isUpdatingMigrationStatus } = usePendingMigrationStatus( {
onSubmit: navigation.submit,
} );

const [ isSubmitting, setIsSubmitting ] = useState( false );
const handleClick = async ( value: string ) => {
setIsSubmitting( true );

try {
await setPendingMigration( value );
} finally {
setIsSubmitting( false );
}
};

const hostingProviderSlug = hostingProviderData?.hosting_provider?.slug;
const shouldDisplayHostIdentificationMessage =
hostingProviderSlug &&
hostingProviderSlug !== 'unknown' &&
hostingProviderSlug !== 'automattic';

const stepContent = (
<>
const stepContent =
isSubmitting || isUpdatingMigrationStatus ? (
<LoadingEllipsis className="how-to-migrate__loader" />
) : (
<div className="how-to-migrate__list">
{ options.map( ( option, i ) => (
<FlowCard
key={ i }
title={ option.label }
text={ option.description }
onClick={ () => setPendingMigration( option.value ) }
onClick={ () => handleClick( option.value ) }
/>
) ) }
</div>
</>
);
);

const platformText = shouldDisplayHostIdentificationMessage
? translate( 'Your WordPress site is hosted with %(hostingProviderName)s.', {
Expand Down
5 changes: 5 additions & 0 deletions ...pper/declarative-flow/internals/steps-repository/site-migration-how-to-migrate/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,9 @@
.how-to-migrate__description {
color: var(--studio-gray-60);
}

.how-to-migrate__loader {
display: block;
margin: 0 auto;
}
}
32 changes: 23 additions & 9 deletions .../declarative-flow/internals/steps-repository/site-migration-how-to-migrate/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* @jest-environment jsdom
*/
import { fireEvent } from '@testing-library/react';
import { act, fireEvent } from '@testing-library/react';
import React from 'react';
import { useUpdateMigrationStatus } from 'calypso/data/site-migration/use-update-migration-status';
import { RenderStepOptions, mockStepProps, renderStep } from '../../test/helpers';
Expand Down Expand Up @@ -47,6 +47,8 @@ describe( 'SiteMigrationHowToMigrate', () => {
mockUpdateMigrationStatus = jest.fn();
( useUpdateMigrationStatus as jest.Mock ).mockReturnValue( {
updateMigrationStatus: mockUpdateMigrationStatus,
updateMigrationStatusAsync: mockUpdateMigrationStatus,
updateStatusMutationRest: {},
} );
} );

Expand All @@ -56,44 +58,56 @@ describe( 'SiteMigrationHowToMigrate', () => {
expect( mockUpdateMigrationStatus ).toHaveBeenCalledWith( siteId, 'migration-pending' );
} );

it( 'should call updateMigrationStatus with correct value for DIFM option', () => {
it( 'should call updateMigrationStatus with correct value for DIFM option', async () => {
const { getByText } = render( { navigation: { submit: mockSubmit } } );

const optionButton = getByText( 'Do it for me' );
fireEvent.click( optionButton );

await act( async () => {
await fireEvent.click( optionButton );
} );

// Check the last call value
const lastCallValue =
mockUpdateMigrationStatus.mock.calls[ mockUpdateMigrationStatus.mock.calls.length - 1 ][ 1 ];
expect( lastCallValue ).toBe( 'migration-pending-difm' );
} );

it( 'should call updateMigrationStatus with correct value for DIY option', () => {
it( 'should call updateMigrationStatus with correct value for DIY option', async () => {
const { getByText } = render( { navigation: { submit: mockSubmit } } );

const optionButton = getByText( "I'll do it myself" );
fireEvent.click( optionButton );

await act( async () => {
await fireEvent.click( optionButton );
} );

// Check the last call value
const lastCallValue =
mockUpdateMigrationStatus.mock.calls[ mockUpdateMigrationStatus.mock.calls.length - 1 ][ 1 ];
expect( lastCallValue ).toBe( 'migration-pending-diy' );
} );

it( 'should call submit with correct value when DIFM option is clicked', () => {
it( 'should call submit with correct value when DIFM option is clicked', async () => {
const { getByText } = render( { navigation: { submit: mockSubmit } } );

const optionButton = getByText( 'Do it for me' );
fireEvent.click( optionButton );

await act( async () => {
await fireEvent.click( optionButton );
} );

expect( mockSubmit ).toHaveBeenCalledWith( { destination: 'upgrade', how: 'difm' } );
} );

it( 'should call submit with correct value for DIY option', () => {
it( 'should call submit with correct value for DIY option', async () => {
const { getByText } = render( { navigation: { submit: mockSubmit } } );

const optionButton = getByText( "I'll do it myself" );
fireEvent.click( optionButton );

await act( async () => {
await fireEvent.click( optionButton );
} );

expect( mockSubmit ).toHaveBeenCalledWith( { destination: 'upgrade', how: 'myself' } );
} );
Expand Down
17 changes: 13 additions & 4 deletions .../internals/steps-repository/site-migration-how-to-migrate/use-pending-migration-status.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,16 @@ const usePendingMigrationStatus = ( { onSubmit }: PendingMigrationStatusProps )
? true
: false;

const { updateMigrationStatus } = useUpdateMigrationStatus();
const {
updateMigrationStatus,
updateMigrationStatusAsync,
updateStatusMutationRest: {
isIdle: isMigrationStatusUpdateIdle,
isPending: isMigrationStatusUpdatePending,
},
} = useUpdateMigrationStatus();

const isLoading = isMigrationStatusUpdateIdle || isMigrationStatusUpdatePending;

// Register pending migration status when loading the step.
useEffect( () => {
Expand All @@ -27,19 +36,19 @@ const usePendingMigrationStatus = ( { onSubmit }: PendingMigrationStatusProps )
}
}, [ siteId, updateMigrationStatus ] );

const setPendingMigration = ( how: string ) => {
const setPendingMigration = async ( how: string ) => {
const destination = canInstallPlugins ? 'migrate' : 'upgrade';
if ( siteId ) {
const parsedHow = how === HOW_TO_MIGRATE_OPTIONS.DO_IT_MYSELF ? 'diy' : how;
updateMigrationStatus( siteId, `migration-pending-${ parsedHow }` );
await updateMigrationStatusAsync( siteId, `migration-pending-${ parsedHow }` );
}

if ( onSubmit ) {
return onSubmit( { how, destination } );
}
};

return { setPendingMigration };
return { setPendingMigration, isLoading };
};

export default usePendingMigrationStatus;
Loading