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

Consolidate accounts onboarding: Automatically preselect a Google Ads account when there is only one, as well as adjust the UI presentation #2608

Merged
36 changes: 36 additions & 0 deletions js/src/components/ads-account-select-control/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
/**
* Internal dependencies
*/
import AppSelectControl from '.~/components/app-select-control';
import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts';

/**
* @param {Object} props The component props
* @param {string} [props.value] The selected value. IF no value is defined, then the first option is selected and onChange function is triggered.
* @param {Function} [props.onChange] Callback when the select value changes.
* @return {JSX.Element} An enhanced AppSelectControl component.
*/
const AdsAccountSelectControl = ( {
value,
onChange = () => {},

Check warning on line 15 in js/src/components/ads-account-select-control/index.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/ads-account-select-control/index.js#L15

Added line #L15 was not covered by tests
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
...props
} ) => {
const { existingAccounts: accounts = [] } = useExistingGoogleAdsAccounts();
eason9487 marked this conversation as resolved.
Show resolved Hide resolved

const options = accounts.map( ( acc ) => ( {
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
value: acc.id,
label: `${ acc.name } (${ acc.id })`,
} ) );

return (
<AppSelectControl
options={ options }
onChange={ onChange }
value={ value }
autoSelectFirstOption
{ ...props }
/>
);
};

export default AdsAccountSelectControl;
49 changes: 45 additions & 4 deletions js/src/components/app-select-control/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
* External dependencies
*/
import { SelectControl } from '@wordpress/components';
import { useEffect } from '@wordpress/element';
import classNames from 'classnames';

/**
Expand All @@ -16,14 +17,54 @@
* it will be added to the container div's `className`,
* so that you can further control its style.
*
* @param {*} props
* @param {*} props The component props.
* @param {boolean} [props.autoSelectFirstOption=false] Whether the automatically select the first option.
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
*/
const AppSelectControl = ( props ) => {
const { className, ...rest } = props;
const {
className,
options,
onChange,
value,
autoSelectFirstOption = false,
...rest
} = props;
const hasSingleValueStyle = autoSelectFirstOption && options?.length === 1;

useEffect( () => {
// Triggers the onChange event to set the initial value for the select
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
if (
autoSelectFirstOption &&
options?.length > 0 &&
value === undefined
) {
onChange( options[ 0 ].value );
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
}
}, [ autoSelectFirstOption, onChange, options, value ] );
eason9487 marked this conversation as resolved.
Show resolved Hide resolved

let selectProps = {
options,
value,
onChange,
...rest,
};

if ( hasSingleValueStyle ) {
selectProps = {

Check warning on line 53 in js/src/components/app-select-control/index.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/app-select-control/index.js#L53

Added line #L53 was not covered by tests
...selectProps,
suffix: ' ',
tabIndex: '-1',
readOnly: true,
};
}

return (
<div className={ classNames( 'app-select-control', className ) }>
<SelectControl { ...rest } />
<div
className={ classNames( 'app-select-control', className, {
'app-select-control--has-single-value': hasSingleValueStyle,
} ) }
>
<SelectControl { ...selectProps } />
</div>
);
};
Expand Down
4 changes: 4 additions & 0 deletions js/src/components/app-select-control/index.scss
asvinb marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,7 @@
margin-bottom: 0;
}
}

.app-select-control--has-single-value {
pointer-events: none;
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import useApiFetchCallback from '.~/hooks/useApiFetchCallback';
import useDispatchCoreNotices from '.~/hooks/useDispatchCoreNotices';
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import useEventPropertiesFilter from '.~/hooks/useEventPropertiesFilter';
import AdsAccountSelectControl from './ads-account-select-control';
import AdsAccountSelectControl from '.~/components/ads-account-select-control';
import { useAppDispatch } from '.~/data';
import { FILTER_ONBOARDING } from '.~/utils/tracks';
import './index.scss';
Expand Down Expand Up @@ -95,7 +95,7 @@ const ConnectAds = ( props ) => {
<Section.Card.Body>
<Subsection.Title>
{ __(
'Select an existing account',
'Connect to an existing account',
'google-listings-and-ads'
) }
</Subsection.Title>
Expand All @@ -120,7 +120,6 @@ const ConnectAds = ( props ) => {
) }
<ContentButtonLayout>
<AdsAccountSelectControl
accounts={ accounts }
value={ value }
onChange={ setValue }
/>
Expand Down
30 changes: 14 additions & 16 deletions js/src/components/google-ads-account-card/connect-ads/index.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import { useAppDispatch } from '.~/data';
import { FILTER_ONBOARDING } from '.~/utils/tracks';
import expectComponentToRecordEventWithFilteredProperties from '.~/tests/expectComponentToRecordEventWithFilteredProperties';
import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts';

jest.mock( '.~/hooks/useApiFetchCallback', () =>
jest.fn().mockName( 'useApiFetchCallback' )
Expand All @@ -24,6 +25,10 @@ jest.mock( '.~/hooks/useGoogleAdsAccount', () =>
jest.fn().mockName( 'useGoogleAdsAccount' )
);

jest.mock( '.~/hooks/useExistingGoogleAdsAccounts', () =>
jest.fn().mockName( 'useExistingGoogleAdsAccounts' )
);

jest.mock( '.~/data', () => ( {
...jest.requireActual( '.~/data' ),
useAppDispatch: jest.fn(),
Expand Down Expand Up @@ -64,6 +69,10 @@ describe( 'ConnectAds', () => {
.mockName( 'refetchGoogleAdsAccount' ),
} );

useExistingGoogleAdsAccounts.mockReturnValue( {
existingAccounts: accounts,
} );

fetchGoogleAdsAccountStatus = jest
.fn()
.mockName( 'fetchGoogleAdsAccountStatus' );
Expand All @@ -76,11 +85,7 @@ describe( 'ConnectAds', () => {

it( 'should render the given accounts in a selection', () => {
render( <ConnectAds accounts={ accounts } /> );

expect( screen.getByRole( 'combobox' ) ).toBeInTheDocument();
expect(
screen.getByRole( 'option', { name: 'Select one' } )
).toBeInTheDocument();
expect(
screen.getByRole( 'option', { name: 'Account A (1)' } )
).toBeInTheDocument();
Expand Down Expand Up @@ -115,22 +120,15 @@ describe( 'ConnectAds', () => {
expect( onCreateNew ).toHaveBeenCalledTimes( 1 );
} );

it( 'should disable the "Connect" button when no account is selected', async () => {
const user = userEvent.setup();
it( 'should disable the "Connect" button when there are no accounts', async () => {
useExistingGoogleAdsAccounts.mockReturnValue( {
existingAccounts: [],
} );

render( <ConnectAds accounts={ accounts } /> );
const combobox = screen.getByRole( 'combobox' );
render( <ConnectAds accounts={ [] } /> );
const button = getConnectButton();

expect( button ).toBeDisabled();

await user.selectOptions( combobox, '1' );

expect( button ).toBeEnabled();

await user.selectOptions( combobox, '' );

expect( button ).toBeDisabled();
} );

it( 'should render a connecting state and disable the button to switch to account creation after clicking the "Connect" button', async () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ const ConnectMC = () => {
<Section.Card.Body>
<Subsection.Title>
{ __(
'Select an existing account',
'Connect to an existing account',
'google-listings-and-ads'
) }
</Subsection.Title>
Expand Down
9 changes: 1 addition & 8 deletions js/src/components/merchant-center-select-control/index.js
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
* External dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { useEffect } from '@wordpress/element';

/**
* Internal dependencies
Expand Down Expand Up @@ -38,18 +37,12 @@ const MerchantCenterSelectControl = ( {
return a.label.localeCompare( b.label );
} );

useEffect( () => {
// Triggers the onChange event in order to pre-select the initial value
if ( value === undefined ) {
onChange( options[ 0 ]?.value );
}
}, [ options, onChange, value ] );

return (
<AppSelectControl
options={ options }
onChange={ onChange }
value={ value }
autoSelectFirstOption
{ ...props }
/>
);
Expand Down
8 changes: 0 additions & 8 deletions tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -246,18 +246,10 @@ test.describe( 'Set up Ads account', () => {
test( 'Select one existing account', async () => {
const adsAccountSelected = `${ ADS_ACCOUNTS[ 1 ].id }`;

await expect(
setupAdsAccounts.getConnectAdsButton()
).toBeDisabled();

await setupAdsAccounts.selectAnExistingAdsAccount(
adsAccountSelected
);

await expect(
setupAdsAccounts.getConnectAdsButton()
).toBeEnabled();

//Intercept Ads connection request
const connectAdsAccountRequest =
setupAdsAccounts.registerConnectAdsAccountRequests(
Expand Down