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
28 changes: 28 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,28 @@
/**
* Internal dependencies
*/
import AppSelectControl from '.~/components/app-select-control';
import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts';

/**
* @param {Object} props The component props
* @return {JSX.Element} An enhanced AppSelectControl component.
*/
const AdsAccountSelectControl = ( props ) => {
const { existingAccounts: accounts = [] } = useExistingGoogleAdsAccounts();
eason9487 marked this conversation as resolved.
Show resolved Hide resolved

const options = accounts?.map( ( acc ) => ( {
value: acc.id,
label: `${ acc.name } (${ acc.id })`,
} ) );

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

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

/**
* Internal dependencies
Expand All @@ -12,18 +14,60 @@
/**
* Renders a `@wordpress/components`'s `SelectControl` with margin-bottom removed.
*
* If you provide `className` via props,
* 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 {Array} props.options Array of options for the select dropdown. Each option should be an object containing `label` and `value` properties.
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
* @param {string} [props.className] Additional classname to further control the style of the component.
* @param {Function} [props.onChange=noop] Callback function triggered when the selected value changes. Receives the new value as an argument.
* @param {string} [props.value] The currently selected value. This component should be used as a controlled component. A special case is that after mounting, when `autoSelectFirstOption` is true and `value` is undefined, it tries to call back `onChange` once to select the first option so that the `value` can be consistent with the `<select>` element's own value.
* @param {boolean} [props.autoSelectFirstOption=false] If true, automatically triggers the onChange callback with the first option as value when no value is provided. If only one option is available, the select control is also changed to non-interactive.
* @param {*} [props.rest] Additional props passed to the `SelectControl` component.
*/
const AppSelectControl = ( props ) => {
const { className, ...rest } = props;
const {
options = [],

Check warning on line 27 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#L27

Added line #L27 was not covered by tests
className,
onChange = noop,
value,
autoSelectFirstOption = false,
...rest
} = props;
const shouldAutoSelectOnceRef = useRef( autoSelectFirstOption === true );

useEffect( () => {
if ( ! shouldAutoSelectOnceRef.current ) {
return;
}

if ( value === undefined && options.length ) {
shouldAutoSelectOnceRef.current = false;
onChange( options[ 0 ].value );
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
}
}, [ value, options, onChange ] );

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

const hasSingleValueStyle = autoSelectFirstOption && options?.length === 1;
if ( hasSingleValueStyle ) {
selectProps = {

Check warning on line 56 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#L56

Added line #L56 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
23 changes: 4 additions & 19 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 All @@ -12,17 +11,11 @@ import AppSelectControl from '.~/components/app-select-control';

/**
* @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 MerchantCenterSelectControl = ( {
value,
onChange = () => {},
...props
} ) => {
const MerchantCenterSelectControl = ( props ) => {
const { data: existingAccounts = [] } = useExistingGoogleMCAccounts();
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
const options = existingAccounts.map( ( acc ) => {
const options = existingAccounts?.map( ( acc ) => {
return {
value: acc.id,
label: sprintf(
Expand All @@ -34,22 +27,14 @@ const MerchantCenterSelectControl = ( {
),
};
} );
options.sort( ( a, b ) => {
options?.sort( ( a, b ) => {
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