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.

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

Refactor credentials from error handling #94682

Merged
merged 19 commits into from
Sep 22, 2024
Merged
Show file tree
Hide file tree
Changes from 16 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export const AccessMethodPicker: FC< Props > = ( { control } ) => {
<div className="site-migration-credentials__radio">
<Controller
control={ control }
name="howToAccessSite"
name="migrationType"
defaultValue="credentials"
render={ ( { field: { value, ...props } } ) => (
<FormRadio
Expand All @@ -36,7 +36,7 @@ export const AccessMethodPicker: FC< Props > = ( { control } ) => {
<div className="site-migration-credentials__radio">
<Controller
control={ control }
name="howToAccessSite"
name="migrationType"
defaultValue="backup"
render={ ( { field: { value, onBlur, ...props } } ) => (
<FormRadio
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import { ErrorMessage } from './error-message';

interface Props {
control: Control< CredentialsFormData >;
errors: any;
errors?: any;
importSiteQueryParam?: string | undefined | null;
}

Expand All @@ -35,18 +35,18 @@ export const SiteAddressField: React.FC< Props > = ( {

return (
<div className="site-migration-credentials__form-field">
<FormLabel htmlFor="site-address">{ translate( 'Site address' ) }</FormLabel>
<FormLabel htmlFor="from_url">{ translate( 'Site address' ) }</FormLabel>
<Controller
control={ control }
name="siteAddress"
name="from_url"
Copy link
Contributor

Choose a reason for hiding this comment

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

We are switching to snake case here, but backupFileLocation is in camel case. Having two different cases for the form fields in the same form looks a bit confusing to me. WDYT about sticking to one of the cases for all form fields instead?

rules={ {
required: translate( 'Please enter your WordPress site address.' ),
validate: validateSiteAddress,
} }
render={ ( { field } ) => (
<FormTextInput
id="site-address"
isError={ !! errors.siteAddress }
id="from_url"
isError={ !! errors?.from_url }
placeholder={ placeholder }
readOnly={ !! importSiteQueryParam }
disabled={ !! importSiteQueryParam }
Expand All @@ -55,7 +55,7 @@ export const SiteAddressField: React.FC< Props > = ( {
/>
) }
/>
<ErrorMessage error={ errors.siteAddress } />
<ErrorMessage error={ errors?.from_url } />
</div>
);
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import { useTranslate } from 'i18n-calypso';
import { useCallback, useMemo } from 'react';
import { FieldErrors } from 'react-hook-form';
import { ApiError, CredentialsFormData } from '../types';

// This function is used to map the error message to the correct field in the form.
// Backend is returning the errors related to backup files using 'from_url' key
// but we need to use 'backupFileLocation' to identify the field in the form.
const getFieldName = ( key: string, migrationType: string ) => {
return 'backup' === migrationType && key === 'from_url' ? 'backupFileLocation' : key;
};

// ** This hook is used to map the error messages to the form fields errors.
export const useFormErrorMapping = (
error?: ApiError | null,
variables?: CredentialsFormData | null
): FieldErrors< CredentialsFormData > | undefined => {
const translate = useTranslate();

const fieldMapping: Record< string, { type: string; message: string } | null > = useMemo(
() => ( {
from_url: { type: 'manual', message: translate( 'Enter a valid URL.' ) },
username: { type: 'manual', message: translate( 'Enter a valid username.' ) },
password: { type: 'manual', message: translate( 'Enter a valid password.' ) },
backupFileLocation: { type: 'manual', message: translate( 'Enter a valid URL.' ) },
} ),
[ translate ]
);

const getTranslatedMessage = useCallback(
( key: string ) => {
return (
fieldMapping[ key ] ?? {
type: 'manual',
message: translate( 'Invalid input, please check again' ),
}
);
},
[ fieldMapping, translate ]
);

const handleServerError = useCallback(
( error: ApiError, { migrationType }: CredentialsFormData ) => {
const { code, message, data } = error;

if ( code === 'rest_missing_callback_param' || ! code ) {
return {
root: {
type: 'manual',
message: translate( 'An error occurred while saving credentials.' ),
},
};
}

if ( code !== 'rest_invalid_param' || ! data?.params ) {
return { root: { type: 'manual', message } };
}

const invalidFields = Object.keys( data.params );

return invalidFields.reduce(
( errors, key ) => {
const fieldName = getFieldName( key, migrationType );
const message = getTranslatedMessage( key );

errors[ fieldName ] = message;
return errors;
},
{} as Record< string, { type: string; message: string } >
);
},
[ getTranslatedMessage, translate ]
);

return useMemo( () => {
if ( error && variables ) {
return handleServerError( error, variables ) as FieldErrors< CredentialsFormData >;
}
return undefined;
}, [ error, handleServerError, variables ] );
};
Original file line number Diff line number Diff line change
Expand Up @@ -128,21 +128,28 @@ describe( 'SiteMigrationCredentials', () => {
expect( wpcomRequest ).not.toHaveBeenCalled();
} );

it( 'shows errors on the required fields when the user does not fill the fields', async () => {
it( 'shows errors on the required fields when the user does not fill the fields when user select credentials option', async () => {
render();
await userEvent.click( continueButton() );
await userEvent.click( credentialsOption() );
expect( getByText( messages.urlError ) ).toBeVisible();
expect( getByText( messages.usernameError ) ).toBeVisible();
expect( getByText( messages.passwordError ) ).toBeVisible();
} );

expect( getByText( messages.urlError ) ).toBeInTheDocument();
expect( getByText( messages.usernameError ) ).toBeInTheDocument();
expect( getByText( messages.passwordError ) ).toBeInTheDocument();
it( 'shows errors on the required fields when the user does not fill the fields when user select backup option', async () => {
render();
await userEvent.click( backupOption() );
await userEvent.click( continueButton() );
expect( getByText( /Please enter a valid URL/ ) ).toBeVisible();
} );

it( 'shows error when user set invalid site address', async () => {
render();
await userEvent.type( siteAddressInput(), 'invalid-site-address' );
await userEvent.click( continueButton() );

expect( getByText( messages.noTLDError ) ).toBeInTheDocument();
expect( getByText( messages.noTLDError ) ).toBeVisible();
} );

it( 'fills the site address and disable it when the user already informed the site address on previous step', async () => {
Expand Down Expand Up @@ -218,6 +225,18 @@ describe( 'SiteMigrationCredentials', () => {
expect( submit ).not.toHaveBeenCalled();
} );

it( 'shows an generic error when server doesn`t return error', async () => {
const submit = jest.fn();
render( { navigation: { submit } } );

( wpcomRequest as jest.Mock ).mockRejectedValue( {} );

await fillAllFields();
await userEvent.click( continueButton() );

expect( getByText( /An error occurred while saving credentials./ ) ).toBeVisible();
} );

it( 'shows a notice when URL contains error=ticket-creation', async () => {
const submit = jest.fn();
const initialEntry = '/site-migration-credentials?error=ticket-creation';
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,12 @@
export interface CredentialsFormData {
from_url: string;
username: string;
password: string;
backupFileLocation: string;
migrationType: 'credentials' | 'backup';
notes: string;
}
export interface ApiFormData {
siteAddress: string;
username: string;
password: string;
Expand All @@ -7,14 +15,10 @@ export interface CredentialsFormData {
howToAccessSite: 'credentials' | 'backup';
}

export interface MigrationError {
body: {
code: string;
message: string;
data: {
status: number;
params?: Record< string, string >;
};
export interface ApiError {
code: string;
message: string;
data: {
params?: Record< string, string >;
};
status: number;
}
Original file line number Diff line number Diff line change
@@ -1,124 +1,66 @@
import { useTranslate } from 'i18n-calypso';
import { useEffect } from 'react';
import { useForm } from 'react-hook-form';
import { useQuery } from 'calypso/landing/stepper/hooks/use-query';
import { recordTracksEvent } from 'calypso/lib/analytics/tracks';
import { MigrationError, CredentialsFormData } from './types';
import { useFormErrorMapping } from './hooks/use-form-error-mapping';
import { CredentialsFormData } from './types';
import { useSiteMigrationCredentialsMutation } from './use-site-migration-credentials-mutation';

const mapApiError = ( error: any ) => {
return {
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 removed this response transformation to reduce complexity

body: {
code: error.code,
message: error.message,
data: error.data,
},
status: error.status,
};
};

export const useCredentialsForm = ( onSubmit: () => void ) => {
const translate = useTranslate();
const importSiteQueryParam = useQuery().get( 'from' ) || '';
const {
isPending,
mutate: requestAutomatedMigration,
error,
isSuccess,
variables,
} = useSiteMigrationCredentialsMutation();

const fieldMapping = {
from_url: {
fieldName: 'siteAddress',
errorMessage: translate( 'Enter a valid URL.' ),
},
username: {
fieldName: 'username',
errorMessage: translate( 'Enter a valid username.' ),
},
password: {
fieldName: 'password',
errorMessage: translate( 'Enter a valid password.' ),
},
migration_type: {
fieldName: 'howToAccessSite',
errorMessage: null,
},
notes: {
fieldName: 'notes',
errorMessage: null,
},
};

const setGlobalError = ( message?: string | null | undefined ) => {
// eslint-disable-next-line @typescript-eslint/no-use-before-define
setError( 'root', {
type: 'manual',
message: message ?? translate( 'An error occurred while saving credentials.' ),
} );
};

const handleMigrationError = ( err: MigrationError ) => {
let hasUnmappedFieldError = false;

if ( err.body?.code === 'rest_invalid_param' && err.body?.data?.params ) {
Object.entries( err.body.data.params ).forEach( ( [ key ] ) => {
const field = fieldMapping[ key as keyof typeof fieldMapping ];
const keyName =
// eslint-disable-next-line @typescript-eslint/no-use-before-define
'backup' === accessMethod && field?.fieldName === 'siteAddress'
? 'backupFileLocation'
: field?.fieldName;

if ( keyName ) {
const message = field?.errorMessage ?? translate( 'Invalid input, please check again' );
// eslint-disable-next-line @typescript-eslint/no-use-before-define
setError( keyName as keyof CredentialsFormData, { type: 'manual', message } );
} else if ( ! hasUnmappedFieldError ) {
hasUnmappedFieldError = true;
setGlobalError();
}
} );
} else {
setGlobalError( err.body?.message );
}
};

const { isPending, requestAutomatedMigration } = useSiteMigrationCredentialsMutation( {
onSuccess: () => {
recordTracksEvent( 'calypso_site_migration_automated_request_success' );
onSubmit();
},
onError: ( error: any ) => {
handleMigrationError( mapApiError( error ) );
recordTracksEvent( 'calypso_site_migration_automated_request_error' );
},
} );
const serverSideError = useFormErrorMapping( error, variables );

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All error logic is now parsed by this hook and returned on the serverSideError variable

const {
formState: { errors },
control,
handleSubmit,
watch,
setError,
clearErrors,
} = useForm< CredentialsFormData >( {
mode: 'onSubmit',
reValidateMode: 'onSubmit',
disabled: isPending,
defaultValues: {
siteAddress: importSiteQueryParam,
from_url: importSiteQueryParam,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The backend is waiting for a from_url, and the front is calling as siteAddress. It was causing constantly process of map values to show errors. I just removed it.

username: '',
password: '',
backupFileLocation: '',
notes: '',
howToAccessSite: 'credentials',
migrationType: 'credentials',
},
errors: serverSideError,
} );

const accessMethod = watch( 'migrationType' );

useEffect( () => {
if ( isSuccess ) {
recordTracksEvent( 'calypso_site_migration_automated_request_success' );
onSubmit();
}
}, [ isSuccess, onSubmit ] );

useEffect( () => {
if ( error ) {
recordTracksEvent( 'calypso_site_migration_automated_request_error' );
}
}, [ error ] );

useEffect( () => {
const { unsubscribe } = watch( () => {
clearErrors( 'root' );
} );
return () => unsubscribe();
}, [ watch, clearErrors ] );

const accessMethod = watch( 'howToAccessSite' );

const submitHandler = ( data: CredentialsFormData ) => {
requestAutomatedMigration( data );
};
Expand All @@ -131,7 +73,6 @@ export const useCredentialsForm = ( onSubmit: () => void ) => {
accessMethod,
isPending,
submitHandler,
setError,
importSiteQueryParam,
};
};
Loading
Loading