Skip to content

Commit

Permalink
Refactor credentials from error handling (#94682)
Browse files Browse the repository at this point in the history
* Fix typo on file name

* Improve credentials test suite

* Add scenario when there is an error with the file upload url

* Reorganize code to remove warnings

* Refactor form

* Remove errrors changes

* Remove errrors changes

* Reduce screen reference repetition

* Remove uncessary changes

* Remove uncessary changes

* Handle more error cases

* Add more test scenarios

* Fix type

* Fix linting

* Fix unit tests

---------

Co-authored-by: Imran Hossain <imranh920@gmail.com>
  • Loading branch information
gabrielcaires and Imran92 authored Sep 22, 2024
1 parent ff7cc13 commit e8a711e
Show file tree
Hide file tree
Showing 7 changed files with 177 additions and 119 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ export const AccessMethodPicker: FC< CredentialsFormFieldProps > = ( { control }
<div className="site-migration-credentials__radio">
<Controller
control={ control }
name="howToAccessSite"
name="migrationType"
defaultValue="credentials"
render={ ( { field: { value, ...props } } ) => (
<FormRadio
Expand All @@ -32,7 +32,7 @@ export const AccessMethodPicker: FC< CredentialsFormFieldProps > = ( { 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 @@ -34,20 +34,20 @@ export const SiteAddressField: React.FC< Props > = ( {

return (
<div className="site-migration-credentials__form-field">
<FormLabel htmlFor="site-address">
<FormLabel htmlFor="from_url">
{ isEnglishLocale ? translate( 'Current site address' ) : translate( 'Site address' ) }
</FormLabel>
<Controller
control={ control }
name="siteAddress"
name="from_url"
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 @@ -56,7 +56,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 @@ -214,10 +221,26 @@ describe( 'SiteMigrationCredentials', () => {
await fillAllFields();
await userEvent.click( continueButton() );

expect( getByText( /Error message from backend/ ) ).toBeVisible();
await waitFor( () => {
expect( getByText( /Error message from backend/ ) ).toBeVisible();
} );
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() );

await waitFor( () => {
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 All @@ -227,6 +250,9 @@ describe( 'SiteMigrationCredentials', () => {
const errorMessage = await findByText(
/We ran into a problem submitting your details. Please try again shortly./
);
expect( errorMessage ).toBeVisible();

await waitFor( () => {
expect( errorMessage ).toBeVisible();
} );
} );
} );
Original file line number Diff line number Diff line change
@@ -1,6 +1,15 @@
import { Control, FieldErrors } from 'react-hook-form';

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 @@ -9,6 +18,14 @@ export interface CredentialsFormData {
howToAccessSite: 'credentials' | 'backup';
}

export interface ApiError {
code: string;
message: string;
data: {
params?: Record< string, string >;
};
}

export interface CredentialsFormFieldProps {
control: Control< CredentialsFormData >;
errors?: FieldErrors< CredentialsFormData >;
Expand All @@ -23,5 +40,4 @@ export interface MigrationError {
params?: Record< string, string >;
};
};
status: number;
}
Loading

0 comments on commit e8a711e

Please sign in to comment.