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

Create pending sticker when loading the "how to migrate" step #95413

Merged
merged 4 commits into from
Oct 25, 2024
Merged

Create pending sticker when loading the "how to migrate" step #95413

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5b7c3ec
Create pending sticker when loading the step
renatho Oct 15, 2024
7023a65
Extract pending migration status hook
renatho Oct 15, 2024
970ce0c
Add tests
renatho Oct 21, 2024
4c81714
Update function name to not be opinionated
renatho Oct 23, 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
25 changes: 4 additions & 21 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
Expand Up @@ -3,16 +3,15 @@ import { useTranslate } from 'i18n-calypso';
import { FC, useMemo } from 'react';
import DocumentHead from 'calypso/components/data/document-head';
import FormattedHeader from 'calypso/components/formatted-header';
import { useUpdateMigrationStatus } from 'calypso/data/site-migration/use-update-migration-status';
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';
import { useQuery } from 'calypso/landing/stepper/hooks/use-query';
import { useSite } from 'calypso/landing/stepper/hooks/use-site';
import { recordTracksEvent } from 'calypso/lib/analytics/tracks';
import { usePresalesChat } from 'calypso/lib/presales-chat';
import useHostingProviderName from 'calypso/site-profiler/hooks/use-hosting-provider-name';
import FlowCard from '../components/flow-card';
import usePendingMigrationStatus from './use-pending-migration-status';
import type { StepProps } from '../../types';

import './style.scss';
Expand All @@ -26,7 +25,6 @@ const SiteMigrationHowToMigrate: FC< Props > = ( props ) => {
const { navigation, headerText } = props;

const translate = useTranslate();
const site = useSite();
const importSiteQueryParam = useQuery().get( 'from' ) || '';
usePresalesChat( 'wpcom' );

Expand Down Expand Up @@ -64,29 +62,14 @@ const SiteMigrationHowToMigrate: FC< Props > = ( props ) => {
urlData
);

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

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

const canInstallPlugins = site?.plan?.features?.active.find(
( feature ) => feature === 'install-plugins'
)
? true
: false;

const { updateMigrationStatus } = useUpdateMigrationStatus();

const handleClick = ( how: string ) => {
const destination = canInstallPlugins ? 'migrate' : 'upgrade';
if ( site?.ID ) {
const parsedHow = how === HOW_TO_MIGRATE_OPTIONS.DO_IT_MYSELF ? 'diy' : how;
updateMigrationStatus( site.ID, `migration-pending-${ parsedHow }` );
}
return navigation.submit?.( { how, destination } );
};

const stepContent = (
<>
<div className="how-to-migrate__list">
Expand All @@ -95,7 +78,7 @@ const SiteMigrationHowToMigrate: FC< Props > = ( props ) => {
key={ i }
title={ option.label }
text={ option.description }
onClick={ () => handleClick( option.value ) }
onClick={ () => setPendingMigration( option.value ) }
/>
) ) }
</div>
Expand Down
100 changes: 100 additions & 0 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
@@ -0,0 +1,100 @@
/**
* @jest-environment jsdom
*/
import { 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';
import SiteMigrationHowToMigrate from '../index';
import type { StepProps } from '../../../types';

const siteId = 1;

jest.mock( 'calypso/data/site-migration/use-update-migration-status', () => ( {
useUpdateMigrationStatus: jest.fn(),
} ) );

jest.mock( 'calypso/lib/presales-chat', () => ( {
usePresalesChat: jest.fn(),
} ) );

jest.mock( 'calypso/data/site-profiler/use-analyze-url-query', () => ( {
useAnalyzeUrlQuery: () => ( { data: {} } ),
} ) );

jest.mock( 'calypso/data/site-profiler/use-hosting-provider-query', () => ( {
useHostingProviderQuery: () => ( { data: {} } ),
} ) );

jest.mock( 'calypso/site-profiler/hooks/use-hosting-provider-name', () => jest.fn() );

jest.mock( 'calypso/landing/stepper/hooks/use-site', () => ( {
useSite: jest.fn( () => ( {
ID: siteId,
} ) ),
} ) );

const render = ( props?: Partial< StepProps >, renderOptions?: RenderStepOptions ) => {
const combinedProps = { ...mockStepProps( props ) };
return renderStep( <SiteMigrationHowToMigrate { ...combinedProps } />, renderOptions );
};

describe( 'SiteMigrationHowToMigrate', () => {
const mockSubmit = jest.fn();
let mockUpdateMigrationStatus;

beforeEach( () => {
mockUpdateMigrationStatus = jest.fn();
( useUpdateMigrationStatus as jest.Mock ).mockReturnValue( {
updateMigrationStatus: mockUpdateMigrationStatus,
} );
} );

it( 'should register pending migration status when the component is loaded', () => {
render( { navigation: { submit: mockSubmit } } );

expect( mockUpdateMigrationStatus ).toHaveBeenCalledWith( siteId, 'migration-pending' );
} );

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

const optionButton = getByText( 'Do it for me' );
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', () => {
const { getByText } = render( { navigation: { submit: mockSubmit } } );

const optionButton = getByText( "I'll do it myself" );
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', () => {
const { getByText } = render( { navigation: { submit: mockSubmit } } );

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

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

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

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

expect( mockSubmit ).toHaveBeenCalledWith( { destination: 'upgrade', how: 'myself' } );
} );
} );
45 changes: 45 additions & 0 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
@@ -0,0 +1,45 @@
import { useEffect } from 'react';
import { useUpdateMigrationStatus } from 'calypso/data/site-migration/use-update-migration-status';
import { HOW_TO_MIGRATE_OPTIONS } from 'calypso/landing/stepper/constants';
import { useSite } from 'calypso/landing/stepper/hooks/use-site';
import type { NavigationControls } from '../../types';

interface PendingMigrationStatusProps {
onSubmit?: Pick< NavigationControls, 'submit' >[ 'submit' ];
}

const usePendingMigrationStatus = ( { onSubmit }: PendingMigrationStatusProps ) => {
const site = useSite();
const siteId = site?.ID;

const canInstallPlugins = site?.plan?.features?.active.find(
( feature ) => feature === 'install-plugins'
)
? true
: false;

const { updateMigrationStatus } = useUpdateMigrationStatus();

// Register pending migration status when loading the step.
useEffect( () => {
if ( siteId ) {
updateMigrationStatus( siteId, 'migration-pending' );
}
}, [ siteId, updateMigrationStatus ] );

const setPendingMigration = ( 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 }` );
}

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

return { setPendingMigration };
};
Comment on lines +11 to +43
Copy link
Contributor

Choose a reason for hiding this comment

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

Hey folks! I stumbled upon this code today; I got really confused. I don't understand why we'd wrap the existing useUpdateMigrationStatus in another hook. Can you enlighten me on the reasoning for this? Even for me, who worked on this, I had difficulties finding where the status was being updated.

Especially since the hook is strictly tightened with the How to migrate step, as it won't work anywhere else.

I also disagree that the action on the How to migrate step is to "set a migration as pending", and that's the idea that it we give when we do:

onClick={ () => setPendingMigration( option.value ) }

For me, setting the pending state is a side effect of the submit action and not the other way around. I also feel that for future maintenance, removing the submit logic from the How to migrate step file just makes it harder to understand the code. We added a new layer for the onSubmit event, which by itself is already deep enough.

Copy link
Contributor

@gabrielcaires gabrielcaires Oct 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that it seems not necessary to have the submit inside the usePendingMigrationStatus`

Hey folks! I stumbled upon this code today; I got really confused. I don't understand why we'd wrap the existing useUpdateMigrationStatus in another hook.

Another point is that we are not waiting for the request to be finished before calling the onSubmit.

I don't understand why we'd wrap the existing useUpdateMigrationStatus in another hook.

I am ok with the extraction because it removes business logic from the original component and prevents some components from re-rendering. For example, useEffects inside hooks don't trigger component rendering.

But I am unaware of why we need to use 2 states to set migration as pending.

Copy link
Contributor

Choose a reason for hiding this comment

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

But I am unaware of why we need to use 2 states to set migration as pending.

I think the first one is to flag when the user gets to the How to migrate step so we can redirect them back to that stage. The migration-pending-{difm/diy} means that the user has already submitted the intent.

Copy link
Contributor

Choose a reason for hiding this comment

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

For example, useEffects inside hooks don't trigger component rendering.

In this case, would it trigger the render? I asked this because we are not setting any states that would justify that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand why we'd wrap the existing useUpdateMigrationStatus in another hook

The idea was to extract the portion of logic related to the sticker creation in the how to migrate. Otherwise, it would introduce a big complexity to the SiteMigrationHowToMigrate, which already has many other logic too. I thought having it in the same folder as the component would be enough, but maybe we could try to rename the hook to something else to avoid the confusion? I tried to think something to add the HowToMigrate in the name, but my ideas seem too verbose. Something like usePendingMigrationStatusHandlerForHowToMigrateStep?

I also disagree that the action on the How to migrate step is to "set a migration as pending", and that's the idea that it we give when we do:

onClick={ () => setPendingMigration( option.value ) }

For me, setting the pending state is a side effect of the submit action and not the other way around. I also feel that for future maintenance, removing the submit logic from the How to migrate step file just makes it harder to understand the code. We added a new layer for the onSubmit event, which by itself is already deep enough.

Hm! Looking at this again, I totally agree with you. Maybe we could refator it removing the onSubmit dependency from that and adding a clickHandler that will call the setPendingMigration and the navigation.submit?

Another point is that we are not waiting for the request to be finished before calling the onSubmit.

This is already being fixed as part of #95612
Also, notice that any refactor would be nice to do on top of this last PR to avoid conflicts. 👆 @m1r0, I think we could merge that, right?

I think the first one is to flag when the user gets to the How to migrate step so we can redirect them back to that stage. The migration-pending-{difm/diy} means that the user has already submitted the intent.

That's correct!

In this case, would it trigger the render? I asked this because we are not setting any states that would justify that.

That's less about it and more about the separation of concerns to simplify the component logic and avoid the components with hundreds of lines complex to maintain! 🤭

Copy link
Member

Choose a reason for hiding this comment

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

I think we could merge that, right?

Yep, I've merged it. 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, one important thing when moving the submit to the component, it needs to happen after the sticker is created. 😉

The #95612 does it fixing a race condition issue.


export default usePendingMigrationStatus;
Loading