Skip to content

Commit

Permalink
CR feedback.
Browse files Browse the repository at this point in the history
  • Loading branch information
ankitrox committed Oct 24, 2024
1 parent 599c2d2 commit f3f63fe
Show file tree
Hide file tree
Showing 8 changed files with 40 additions and 45 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ import { useEffect, useState } from '@wordpress/element';
*/
import AccountCard, { APPEARANCE } from '../account-card';
import AccountDetails from './account-details';
import AppSpinner from '../app-spinner';
import Indicator from './indicator';
import getAccountCreationTexts from './getAccountCreationTexts';
import SpinnerCard from '.~/components/spinner-card';
import useAutoCreateAdsMCAccounts from '.~/hooks/useAutoCreateAdsMCAccounts';
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import useGoogleMCAccount from '.~/hooks/useGoogleMCAccount';
Expand Down Expand Up @@ -54,7 +54,7 @@ const ConnectedGoogleComboAccountCard = () => {
! hasFinishedResolutionForCurrentAdsAccount ||
! hasFinishedResolutionForCurrentMCAccount )
) {
return <AccountCard description={ <AppSpinner /> } />;
return <SpinnerCard />;
}

return (
Expand Down
4 changes: 2 additions & 2 deletions js/src/components/google-combo-account-card/indicator.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import { __ } from '@wordpress/i18n';
import ConnectedIconLabel from '.~/components/connected-icon-label';
import LoadingLabel from '.~/components/loading-label/loading-label';
import useGoogleAdsAccountReady from '.~/hooks/useGoogleAdsAccountReady';
import useGoogleMCAccountReady from '.~/hooks/useGoogleMCAccountReady';
import useGoogleMCAccount from '.~/hooks/useGoogleMCAccount';

/**
* Account creation indicator.
Expand All @@ -20,7 +20,7 @@ import useGoogleMCAccountReady from '.~/hooks/useGoogleMCAccountReady';
*/
const Indicator = ( { showSpinner } ) => {
const isGoogleAdsConnected = useGoogleAdsAccountReady();
const isGoogleMCConnected = useGoogleMCAccountReady();
const { isReady: isGoogleMCConnected } = useGoogleMCAccount();

if ( showSpinner ) {
return (
Expand Down
6 changes: 1 addition & 5 deletions js/src/hooks/useAutoCreateAdsMCAccounts.js
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ const useShouldCreateMCAccount = () => {

/**
* @typedef {Object} AutoCreateAdsMCAccountsData
* @property {boolean} accountsCreated - Whether the accounts have been successfully created.
* @property {boolean} hasDetermined - Whether the checks to determine if accounts should be created are finished.
* @property {('ads'|'mc'|'both'|null)} creatingWhich - Which accounts are being created ('ads', 'mc', 'both'), or `null` if none.
*/
Expand All @@ -71,7 +70,6 @@ const useShouldCreateMCAccount = () => {
*/
const useAutoCreateAdsMCAccounts = () => {
const lockedRef = useRef( false );
// Create separate states.
const [ creatingWhich, setCreatingWhich ] = useState( null );
const [ hasDetermined, setDetermined ] = useState( false );

Expand Down Expand Up @@ -112,14 +110,12 @@ const useAutoCreateAdsMCAccounts = () => {
if ( which === CREATING_BOTH_ACCOUNTS ) {
await handleCreateAccount();
await upsertAdsAccount();
setCreatingWhich( null );
} else if ( which === CREATING_MC_ACCOUNT ) {
await handleCreateAccount();
setCreatingWhich( null );
} else if ( which === CREATING_ADS_ACCOUNT ) {
await upsertAdsAccount();
setCreatingWhich( null );
}
setCreatingWhich( null );
};

handleCreateAccountCallback();
Expand Down
17 changes: 9 additions & 8 deletions js/src/hooks/useAutoCreateAdsMCAccounts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,9 +66,11 @@ describe( 'useAutoCreateAdsMCAccounts hook', () => {
const { result } = renderHook( () => useAutoCreateAdsMCAccounts() );

await act( async () => {
// It should create both accounts.
expect( result.current.creatingWhich ).toBe( 'both' );
} );

expect( handleCreateAccount ).toHaveBeenCalledTimes( 1 );
expect( upsertAdsAccount ).toHaveBeenCalledTimes( 1 );
} );

it( 'should create only the Merchant Center account', async () => {
Expand All @@ -83,11 +85,12 @@ describe( 'useAutoCreateAdsMCAccounts hook', () => {
} );

const { result } = renderHook( () => useAutoCreateAdsMCAccounts() );
// It should create only the Merchant Center account.
await act( async () => {
// It should create both accounts.
expect( result.current.creatingWhich ).toBe( 'mc' );
} );

expect( handleCreateAccount ).toHaveBeenCalledTimes( 1 );
expect( upsertAdsAccount ).toHaveBeenCalledTimes( 0 );
} );

it( 'should create only the Google Ads account', async () => {
Expand All @@ -102,16 +105,17 @@ describe( 'useAutoCreateAdsMCAccounts hook', () => {
} );

const { result } = renderHook( () => useAutoCreateAdsMCAccounts() );
// It should create only the Google Ads account.
await act( async () => {
expect( result.current.creatingWhich ).toBe( 'ads' );
} );

expect( handleCreateAccount ).toHaveBeenCalledTimes( 0 );
expect( upsertAdsAccount ).toHaveBeenCalledTimes( 1 );
} );
} );

describe( 'Existing accounts', () => {
beforeEach( () => {
// Existing Ads and MC accounts.
useExistingGoogleAdsAccounts.mockReturnValue( {
hasFinishedResolution: true,
existingAccounts: [
Expand All @@ -136,10 +140,7 @@ describe( 'useAutoCreateAdsMCAccounts hook', () => {
it( 'should not create accounts if they already exist', () => {
const { result } = renderHook( () => useAutoCreateAdsMCAccounts() );

// It should not create any accounts.
expect( result.current.creatingWhich ).toBe( null );

// make sure functions are not called.
expect( handleCreateAccount ).not.toHaveBeenCalled();
expect( upsertAdsAccount ).not.toHaveBeenCalled();
} );
Expand Down
16 changes: 10 additions & 6 deletions js/src/hooks/useGoogleAdsAccountReady.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,18 +4,22 @@
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import useGoogleAdsAccountStatus from '.~/hooks/useGoogleAdsAccountStatus';

/**
* Hook to check if the Google Ads account is ready.
* This is used to determine if the user can proceed to the next step.
*
* @return {boolean|null} Whether the Google Ads account is ready. `null` if the state is not yet determined.
*/
const useGoogleAdsAccountReady = () => {
const { hasGoogleAdsConnection } = useGoogleAdsAccount();
const { hasAccess, step } = useGoogleAdsAccountStatus();

// Ads is ready when we have a connection and verified and verified access.
// Billing is not required, and the 'link_merchant' step will be resolved
// when the MC the account is connected.
return (
const isReady =
hasGoogleAdsConnection &&
hasAccess &&
[ '', 'billing', 'link_merchant' ].includes( step )
);
[ '', 'billing', 'link_merchant' ].includes( step );

return isReady !== null ? isReady : null;
};

export default useGoogleAdsAccountReady;
11 changes: 11 additions & 0 deletions js/src/hooks/useGoogleMCAccount.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ import { GOOGLE_MC_ACCOUNT_STATUS } from '.~/constants';
* @property {boolean} isResolving Whether resolution is in progress.
* @property {boolean} hasFinishedResolution Whether resolution has completed.
* @property {boolean} isPreconditionReady Whether the precondition of continued connection processing is fulfilled.
* @property {boolean} hasGoogleMCConnection Whether the user has a Google Merchant Center account connection established.
* @property {boolean} isReady Whether the user has a Google Merchant Center account is in connected state.
*/

const googleMCAccountSelector = 'getGoogleMCAccount';
Expand Down Expand Up @@ -46,6 +48,8 @@ const useGoogleMCAccount = () => {
// has not been granted necessary access permissions for Google Merchant Center, then
// the precondition doesn't meet.
isPreconditionReady: false,
hasGoogleMCConnection: false,
isReady: false,
};
}

Expand All @@ -60,6 +64,12 @@ const useGoogleMCAccount = () => {
GOOGLE_MC_ACCOUNT_STATUS.INCOMPLETE,
].includes( acc?.status );

const isReady =
hasGoogleMCConnection &&
( acc.status === GOOGLE_MC_ACCOUNT_STATUS.CONNECTED ||
( acc.status === GOOGLE_MC_ACCOUNT_STATUS.INCOMPLETE &&
acc?.step === 'link_ads' ) );

Check warning on line 71 in js/src/hooks/useGoogleMCAccount.js

View check run for this annotation

Codecov / codecov/patch

js/src/hooks/useGoogleMCAccount.js#L69-L71

Added lines #L69 - L71 were not covered by tests

return {
googleMCAccount: acc,
isResolving: isResolvingGoogleMCAccount,
Expand All @@ -68,6 +78,7 @@ const useGoogleMCAccount = () => {
),
isPreconditionReady: true,
hasGoogleMCConnection,
isReady,
};
},
[
Expand Down
18 changes: 0 additions & 18 deletions js/src/hooks/useGoogleMCAccountReady.js

This file was deleted.

9 changes: 5 additions & 4 deletions js/src/setup-mc/setup-stepper/setup-accounts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import Faqs from './faqs';
import './index.scss';
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import useGoogleAdsAccountReady from '.~/hooks/useGoogleAdsAccountReady';
import useGoogleMCAccountReady from '.~/hooks/useGoogleMCAccountReady';

/**
* Renders the disclaimer of Comparison Shopping Service (CSS).
Expand Down Expand Up @@ -84,11 +83,13 @@ const SetupAccounts = ( props ) => {
const { onContinue = () => {} } = props;
const { jetpack } = useJetpackAccount();
const { google, scope } = useGoogleAccount();
const { googleMCAccount, isPreconditionReady: isGMCPreconditionReady } =
useGoogleMCAccount();
const {
googleMCAccount,
isPreconditionReady: isGMCPreconditionReady,
isReady: isGoogleMCReady,
} = useGoogleMCAccount();
const { hasFinishedResolution } = useGoogleAdsAccount();
const isGoogleAdsReady = useGoogleAdsAccountReady();
const isGoogleMCReady = useGoogleMCAccountReady();

/**
* When jetpack is loading, or when google account is loading,
Expand Down

0 comments on commit f3f63fe

Please sign in to comment.