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

Credential Form: Show error message on the correct language + cleanup #95688

Merged
merged 3 commits into from
Oct 31, 2024
Merged

Credential Form: Show error message on the correct language + cleanup #95688

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
136cefc
Remove phrase variants and english checkings
gabrielcaires Oct 24, 2024
9f25cab
Add locale flag to the ticket creation request
gabrielcaires Oct 24, 2024
9a44c67
Fix tests
gabrielcaires Oct 24, 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
7 changes: 2 additions & 5 deletions ...w/internals/steps-repository/site-migration-credentials/components/site-address-field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { FormLabel } from '@automattic/components';
import { useHasEnTranslation, useIsEnglishLocale } from '@automattic/i18n-utils';
import { useHasEnTranslation } from '@automattic/i18n-utils';
import { useTranslate } from 'i18n-calypso';
import { Controller } from 'react-hook-form';
import getValidationMessage from 'calypso/blocks/import/capture/url-validation-message-helper';
Expand All @@ -10,7 +10,6 @@ import { ErrorMessage } from './error-message';

export const SiteAddressField: React.FC< CredentialsFormFieldProps > = ( { control, errors } ) => {
const translate = useTranslate();
const isEnglishLocale = useIsEnglishLocale();
const hasEnTranslation = useHasEnTranslation();

const validateSiteAddress = ( siteAddress: string ) => {
Expand All @@ -26,9 +25,7 @@ export const SiteAddressField: React.FC< CredentialsFormFieldProps > = ( { contr

return (
<div className="site-migration-credentials__form-field">
<FormLabel htmlFor="from_url">
{ isEnglishLocale ? translate( 'Current site address' ) : translate( 'Site address' ) }
</FormLabel>
<FormLabel htmlFor="from_url">{ translate( 'Current site address' ) }</FormLabel>
<Controller
control={ control }
name="from_url"
Expand Down
12 changes: 3 additions & 9 deletions ...low/internals/steps-repository/site-migration-credentials/hooks/use-form-error-mapping.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { useIsEnglishLocale } from '@automattic/i18n-utils';
import { useTranslate } from 'i18n-calypso';
import { useCallback, useMemo } from 'react';
import { FieldErrors } from 'react-hook-form';
Expand All @@ -19,7 +18,6 @@ export const useFormErrorMapping = (
siteInfo?: UrlData | undefined
): FieldErrors< CredentialsFormData > | undefined => {
const translate = useTranslate();
const isEnglishLocale = useIsEnglishLocale();

const fieldMapping: Record< string, { type: string; message: string } | null > = useMemo(
() => ( {
Expand All @@ -32,7 +30,7 @@ export const useFormErrorMapping = (
);

const getCredentialsErrorMessage = useCallback(
( errorCode: any ) => {
( errorCode: number | undefined ) => {
switch ( errorCode ) {
case 404:
return {
Expand Down Expand Up @@ -79,7 +77,6 @@ export const useFormErrorMapping = (
},
};
}
return undefined;
},
[ translate ]
);
Expand All @@ -96,10 +93,7 @@ export const useFormErrorMapping = (
};
}

if (
isEnglishLocale &&
code === 'automated_migration_tools_login_and_get_cookies_test_failed'
) {
if ( code === 'automated_migration_tools_login_and_get_cookies_test_failed' ) {
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Super nitpicky, but we should reverse this conditional so the constant is on the left-hand side ("Yoda" conditions as per WP coding standards)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @sixhours.
I just double-checked if we are adopting the WP pattern, and I saw we just have
164 instances of the Yoda style
1494 instances of the regular style.

I run a search using the following patterns:
Regular style

=== '

Yoda style

' ===

The javascript WordPress code standards also contain examples of "regular" style usage.
https://developer.wordpress.org/coding-standards/wordpress-coding-standards/javascript/

So, I think the Yoda style is more applicable to PHP

root: {
type: 'special',
Expand Down Expand Up @@ -128,7 +122,7 @@ export const useFormErrorMapping = (
{} as Record< string, { type: string; message: string } >
);
},
[ getTranslatedMessage, translate, getCredentialsErrorMessage, isEnglishLocale ]
[ getTranslatedMessage, translate, getCredentialsErrorMessage ]
);

return useMemo( () => {
Expand Down
9 changes: 6 additions & 3 deletions ...als/steps-repository/site-migration-credentials/hooks/use-request-automated-migration.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { useLocale } from '@automattic/i18n-utils';
import { useMutation, UseMutationOptions } from '@tanstack/react-query';
import wpcomRequest from 'wpcom-proxy-request';
import { ApiError, CredentialsFormData } from '../types';
Expand All @@ -19,10 +20,11 @@ interface AutomatedMigration {

const requestAutomatedMigration = async (
siteSlug: string,
payload: AutomatedMigration
payload: AutomatedMigration,
locale: string
): Promise< AutomatedMigrationAPIResponse > => {
return wpcomRequest( {
path: `sites/${ siteSlug }/automated-migration`,
path: `sites/${ siteSlug }/automated-migration?_locale=${ locale }`,
apiNamespace: 'wpcom/v2/',
apiVersion: '2',
method: 'POST',
Expand All @@ -34,6 +36,7 @@ export const useRequestAutomatedMigration = (
siteSlug?: string | null,
options: UseMutationOptions< AutomatedMigrationAPIResponse, ApiError, CredentialsFormData > = {}
) => {
const locale = useLocale();
return useMutation< AutomatedMigrationAPIResponse, ApiError, CredentialsFormData >( {
mutationFn: ( {
from_url,
Expand Down Expand Up @@ -70,7 +73,7 @@ export const useRequestAutomatedMigration = (
};
}

return requestAutomatedMigration( siteSlug, body );
return requestAutomatedMigration( siteSlug, body, locale );
},
...options,
} );
Expand Down
28 changes: 5 additions & 23 deletions .../stepper/declarative-flow/internals/steps-repository/site-migration-credentials/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { useIsEnglishLocale } from '@automattic/i18n-utils';
import { StepContainer } from '@automattic/onboarding';
import { useTranslate } from 'i18n-calypso';
import { UrlData } from 'calypso/blocks/import/types';
Expand Down Expand Up @@ -27,7 +26,6 @@ const getAction = ( siteInfo?: UrlData ) => {

const SiteMigrationCredentials: Step = function ( { navigation } ) {
const translate = useTranslate();
const isEnglishLocale = useIsEnglishLocale();

const handleSubmit = ( siteInfo?: UrlData | undefined ) => {
const action = getAction( siteInfo );
Expand All @@ -42,13 +40,7 @@ const SiteMigrationCredentials: Step = function ( { navigation } ) {

return (
<>
<DocumentHead
title={
isEnglishLocale
? translate( 'Tell us about your WordPress site' )
: translate( 'Tell us about your site' )
}
/>
<DocumentHead title={ translate( 'Tell us about your WordPress site' ) } />
<StepContainer
stepName="site-migration-credentials"
flowName="site-migration"
Expand All @@ -59,20 +51,10 @@ const SiteMigrationCredentials: Step = function ( { navigation } ) {
formattedHeader={
<FormattedHeader
id="site-migration-credentials-header"
headerText={
isEnglishLocale
? translate( 'Tell us about your WordPress site' )
: translate( 'Tell us about your site' )
}
subHeaderText={
isEnglishLocale
? translate(
'Please share the following details to access your site and start your migration to WordPress.com.'
)
: translate(
'Please share the following details to access your site and start your migration.'
)
}
headerText={ translate( 'Tell us about your WordPress site' ) }
subHeaderText={ translate(
'Please share the following details to access your site and start your migration to WordPress.com.'
) }
align="center"
/>
}
Expand Down
8 changes: 4 additions & 4 deletions ...per/declarative-flow/internals/steps-repository/site-migration-credentials/test/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ const fillNoteField = async () => {
};

const requestPayload = {
path: 'sites/site-url.wordpress.com/automated-migration',
path: 'sites/site-url.wordpress.com/automated-migration?_locale=en',
apiNamespace: 'wpcom/v2/',
apiVersion: '2',
method: 'POST',
Expand Down Expand Up @@ -120,7 +120,7 @@ describe( 'SiteMigrationCredentials', () => {
await userEvent.click( continueButton() );

expect( wpcomRequest ).toHaveBeenCalledWith( {
path: 'sites/site-url.wordpress.com/automated-migration',
path: 'sites/site-url.wordpress.com/automated-migration?_locale=en',
apiNamespace: 'wpcom/v2/',
apiVersion: '2',
method: 'POST',
Expand Down Expand Up @@ -164,7 +164,7 @@ describe( 'SiteMigrationCredentials', () => {
await userEvent.click( continueButton() );

expect( wpcomRequest ).toHaveBeenCalledWith( {
path: 'sites/site-url.wordpress.com/automated-migration',
path: 'sites/site-url.wordpress.com/automated-migration?_locale=en',
apiNamespace: 'wpcom/v2/',
apiVersion: '2',
method: 'POST',
Expand Down Expand Up @@ -199,7 +199,7 @@ describe( 'SiteMigrationCredentials', () => {

//TODO: Ideally we should use nock to mock the request, but it is not working with the current implementation due to wpcomRequest usage that is well captured by nock.
expect( wpcomRequest ).toHaveBeenCalledWith( {
path: 'sites/site-url.wordpress.com/automated-migration',
path: 'sites/site-url.wordpress.com/automated-migration?_locale=en',
apiNamespace: 'wpcom/v2/',
apiVersion: '2',
method: 'POST',
Expand Down
Loading