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

Edit combo accounts #2660

Open
wants to merge 61 commits into
base: feature/2509-consolidate-google-account-cards
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
61 commits
Select commit Hold shift + click to select a range
b33484d
Add editing mode.
asvinb Oct 21, 2024
1b52069
Add placeholder text.
asvinb Oct 21, 2024
4c493a0
Merge branch 'feature/2509-consolidate-google-account-cards' into upd…
asvinb Oct 31, 2024
f63e731
REmove unused component.
asvinb Oct 31, 2024
78e18b9
Merge branch 'update/2597-connect-mc-account' into update/2605-edit-a…
asvinb Oct 31, 2024
e90aaca
Merge branch 'update/2597-connect-mc-account' into update/2605-edit-a…
asvinb Nov 5, 2024
b9f41b1
Merge branch 'update/2597-connect-mc-account' into update/2605-edit-a…
asvinb Nov 5, 2024
05580c3
Fix linting errors.
asvinb Nov 5, 2024
0a6c386
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
asvinb Nov 5, 2024
10cdd81
Merge branch 'update/2597-connect-mc-account' into update/2605-edit-a…
asvinb Nov 5, 2024
218149d
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
asvinb Nov 5, 2024
ad78ecb
Merge branch 'update/2582-claim-ads-account' into update/2605-edit-ac…
asvinb Nov 6, 2024
3b1dc9d
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
asvinb Nov 6, 2024
65e6571
Add E2E tests.
asvinb Nov 6, 2024
442b478
Merge branch 'feature/2509-consolidate-google-account-cards' into upd…
asvinb Nov 6, 2024
7ded84d
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
joemcgill Nov 6, 2024
36a13e4
Use actions prop.
asvinb Nov 7, 2024
80fd522
Refine E2E tests.
asvinb Nov 7, 2024
6de9cc0
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
asvinb Nov 7, 2024
1e8a618
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
asvinb Nov 8, 2024
846f975
Always hide conversion notice when clicking cancel.
asvinb Nov 8, 2024
fcf69f2
Remove autoadded code.
asvinb Nov 8, 2024
d0b30d2
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
asvinb Nov 8, 2024
328a264
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
asvinb Nov 8, 2024
0f0aa16
Update Connect to different Google account button text
joemcgill Nov 12, 2024
ba22a30
Eliminate border radius between combo account cards
joemcgill Nov 12, 2024
df93210
Revert title change in GoogleAdsAccountCard
joemcgill Nov 12, 2024
660ea2a
Rename onClick callback handlers
joemcgill Nov 12, 2024
3b9dbbb
Disconnect Ads when switching Google accounts
joemcgill Nov 12, 2024
9dbd087
Merge branch 'update/2603-create-ads-account' into update/2605-edit-a…
asvinb Nov 13, 2024
8260f12
Fix linting errors.
asvinb Nov 13, 2024
de1c98c
Remove unused imports.
asvinb Nov 13, 2024
1ace9be
Don't change import order
joemcgill Nov 13, 2024
c8f8be0
Improve card actions related to edit mode
joemcgill Nov 13, 2024
efb1bd3
Fix lint
joemcgill Nov 13, 2024
9795661
Fix inline docs
joemcgill Nov 13, 2024
0d63a35
Pass createMCAccount to useAutoCreateAdsMCAccounts
joemcgill Nov 14, 2024
3e46d07
Merge branch 'feature/2509-consolidate-google-account-cards' into upd…
joemcgill Nov 14, 2024
2ea19b8
Better card handling during auto-creation
joemcgill Nov 15, 2024
8d93a0e
E2E Add post auto-creation test
joemcgill Nov 15, 2024
08f96a7
Dont' show the edit button when showConnectMC is true and the Ads cla…
joemcgill Nov 15, 2024
1b0ad3e
Improve JSDoc
joemcgill Nov 15, 2024
392bdd7
Check existing accounts in the mc reclaim url card
joemcgill Nov 15, 2024
27a9627
Show MC Reclaim card after refresh
joemcgill Nov 15, 2024
767a129
Fix the issue that E2E test can't log in to wp-admin.
eason9487 Nov 11, 2024
109b724
Add external icon to Claim button
joemcgill Nov 17, 2024
17dab80
This may take a few moments, please wait…
joemcgill Nov 17, 2024
0348394
Only show Address Card once MC is ready
joemcgill Nov 17, 2024
ff47280
Reset connection flow if refreshed
joemcgill Nov 18, 2024
6a7d33e
Remove unnecessary ID from ReclaimUrlCard
joemcgill Nov 18, 2024
fa5a2d3
Omit createInterpolateElement
joemcgill Nov 18, 2024
4343210
Improve ConnectAds UI when connected account is not in existing accounts
joemcgill Nov 18, 2024
93d588e
Allow connected Ads account to be edited when claiming
joemcgill Nov 18, 2024
fd0ad0e
Improve edit mode when there are no existing accounts
joemcgill Nov 18, 2024
0e413b9
Remove unused import
joemcgill Nov 18, 2024
aef0d4a
Add edit button tests
joemcgill Nov 18, 2024
97465bc
Improve combo card border radius CSS
joemcgill Nov 18, 2024
d3d4c34
Lint fixes
joemcgill Nov 18, 2024
58820c1
Require an ID for hasGoogleMCConnection
joemcgill Nov 18, 2024
87049d8
Show disconnect button if there is 1 other existing account
joemcgill Nov 19, 2024
2b213f4
Consider the ConnectMCCard connected even when incomplete
joemcgill Nov 19, 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
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
@@ -1,15 +1,51 @@
/**
* External dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { getSetting } from '@woocommerce/settings'; // eslint-disable-line import/no-unresolved

/**
* Internal dependencies
*/
import AppSelectControl from '.~/components/app-select-control';
import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts';
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';

/**
* @param {Object} props The component props
* @return {JSX.Element} An enhanced AppSelectControl component.
*/
const AdsAccountSelectControl = ( props ) => {
const { existingAccounts } = useExistingGoogleAdsAccounts();
const { googleAdsAccount, hasGoogleAdsConnection } = useGoogleAdsAccount();

const accountIdExists = existingAccounts?.some(
( existingAccount ) => existingAccount.id === googleAdsAccount?.id
);

// If the account ID is not in the list of existing accounts, fake the select options by displaying the connected account ID only.
if ( ! accountIdExists && hasGoogleAdsConnection ) {
const domain = new URL( getSetting( 'homeUrl' ) ).host;

Check warning on line 28 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#L28

Added line #L28 was not covered by tests

return (

Check warning on line 30 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#L30

Added line #L30 was not covered by tests
<AppSelectControl
autoSelectFirstOption
nonInteractive
value={ googleAdsAccount.id }
options={ [
{
value: googleAdsAccount.id,
label: sprintf(
// translators: 1: account domain, 2: account ID.
__( '%1$s (%2$s)', 'google-listings-and-ads' ),
domain,
googleAdsAccount.id
),
},
] }
/>
);
}

const options = existingAccounts?.map( ( acc ) => ( {
value: acc.id,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,22 +1,10 @@
/**
* External dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import AccountCard, { APPEARANCE } from '.~/components/account-card';
import AppButton from '.~/components/app-button';
import ConnectedIconLabel from '.~/components/connected-icon-label';
import Section from '.~/wcdl/section';
import useSwitchGoogleAccount from './useSwitchGoogleAccount';

/**
* Clicking on the "connect to a different Google account" button.
*
* @event gla_google_account_connect_different_account_button_click
*/
import SwitchAccountButton from './switch-account-button';

/**
* Renders a Google account card UI with connected account information.
Expand All @@ -26,16 +14,12 @@ import useSwitchGoogleAccount from './useSwitchGoogleAccount';
* @param {{ email: string }} props.googleAccount A data payload object containing the user's Google account email.
* @param {JSX.Element} [props.helper] Helper content below the Google account email.
* @param {boolean} [props.hideAccountSwitch=false] Indicate whether hide the account switch block at the card footer.
*
* @fires gla_google_account_connect_different_account_button_click
*/
const ConnectedGoogleAccountCard = ( {
googleAccount,
helper,
hideAccountSwitch = false,
} ) => {
const [ handleSwitch, { loading } ] = useSwitchGoogleAccount();

return (
<AccountCard
appearance={ APPEARANCE.GOOGLE }
Expand All @@ -45,16 +29,7 @@ const ConnectedGoogleAccountCard = ( {
>
{ ! hideAccountSwitch && (
<Section.Card.Footer>
<AppButton
isLink
disabled={ loading }
text={ __(
'Or, connect to a different Google account',
'google-listings-and-ads'
) }
eventName="gla_google_account_connect_different_account_button_click"
onClick={ handleSwitch }
/>
<SwitchAccountButton />
</Section.Card.Footer>
) }
</AccountCard>
Expand Down
46 changes: 46 additions & 0 deletions js/src/components/google-account-card/switch-account-button.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
/**
* External dependencies
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import AppButton from '.~/components/app-button';
import useSwitchGoogleAccount from './useSwitchGoogleAccount';

/**
* Clicking on the "connect to a different Google account" button.
*
* @event gla_google_account_connect_different_account_button_click
*/

/**
* Renders a switch button that lets user connect with another Google account.
*
* @fires gla_google_account_connect_different_account_button_click
* @param {Object} props React props
* @param {string} [props.text="Or, connect to a different Google account"] Text to display on the button
*/
const SwitchAccountButton = ( {
text = __(
'Or, connect to a different Google account',
'google-listings-and-ads'
),
...restProps
} ) => {
const [ handleSwitch, { loading } ] = useSwitchGoogleAccount();
eason9487 marked this conversation as resolved.
Show resolved Hide resolved

return (
<AppButton
isLink
disabled={ loading }
text={ text }
eventName="gla_google_account_connect_different_account_button_click"
onClick={ handleSwitch }
{ ...restProps }
/>
);
};

export default SwitchAccountButton;
10 changes: 10 additions & 0 deletions js/src/components/google-account-card/useSwitchGoogleAccount.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,14 @@ const useSwitchGoogleAccount = () => {
method: 'DELETE',
} );

const [
fetchGoogleAdsDisconnect,
{ loading: loadingGoogleAdsDisconnect },
] = useApiFetchCallback( {
path: `${ API_NAMESPACE }/ads/connection`,
method: 'DELETE',
} );

/**
* Note: we are manually calling `DELETE /google/connect` instead of using
* `disconnectGoogleAccount` action from wp-data store
Expand Down Expand Up @@ -66,6 +74,7 @@ const useSwitchGoogleAccount = () => {

try {
await fetchGoogleMCDisconnect();
await fetchGoogleAdsDisconnect();
await fetchGoogleDisconnect();
const { url } = await fetchGoogleConnect();
window.location.href = url;
Expand All @@ -83,6 +92,7 @@ const useSwitchGoogleAccount = () => {

const loading =
loadingGoogleMCDisconnect ||
loadingGoogleAdsDisconnect ||
loadingGoogleDisconnect ||
loadingGoogleConnect ||
dataGoogleConnect;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import { Icon } from '@wordpress/components';
import { external as externalIcon } from '@wordpress/icons';

/**
* Internal dependencies
Expand Down Expand Up @@ -40,13 +42,13 @@ const ClaimAdsAccount = () => {
'google-listings-and-ads'
) }
</p>
<ClaimAccountButton
text={ __(
<ClaimAccountButton isPrimary>
{ __(
'Claim account in Google Ads',
'google-listings-and-ads'
) }
isPrimary
/>
<Icon icon={ externalIcon } size={ 20 } />
</ClaimAccountButton>
</div>
);
};
Expand Down
Copy link
Member

Choose a reason for hiding this comment

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

When there is no existing Google Ads account and the newly created one is waiting to be claimed, clicking on the creating button won't create a new one, but will go to the update account processing due to the useUpsertAdsAccount mechanism.

Kapture.2024-11-20.at.10.32.45.mp4

In the video, after click the button to create a new account

  • The status is "Connecting…"
  • The POST request to wc/gla/ads/accounts API has a { id: [account ID waiting for claiming] } payload
  • After the requests are completed, it's still the same account

Suggest in this case to make it either:

  • Can actually create a new account
  • Make it no option to create a new one or disconnect an incomplete one

Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import { __ } from '@wordpress/i18n';
*/
import AppButton from '.~/components/app-button';
import DisconnectAccount from '.~/components/google-ads-account-card/disconnect-account';
import useExistingGoogleAdsAccounts from '.~/hooks/useExistingGoogleAdsAccounts';

/**
* Footer component.
Expand All @@ -23,7 +24,9 @@ const ConnectAdsFooter = ( {
onCreateNewClick,
...restProps
} ) => {
if ( isConnected ) {
const { existingAccounts } = useExistingGoogleAdsAccounts();

if ( isConnected && existingAccounts.length > 0 ) {
return <DisconnectAccount />;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,19 +31,23 @@ const ConnectExistingAccount = ( { onCreateClick } ) => {
const { createNotice } = useDispatchCoreNotices();
const { fetchGoogleAdsAccountStatus } = useAppDispatch();
const isConnected = useGoogleAdsAccountReady();
const { googleAdsAccount, hasFinishedResolution, refetchGoogleAdsAccount } =
useGoogleAdsAccount();
const {
googleAdsAccount,
hasFinishedResolution,
hasGoogleAdsConnection,
refetchGoogleAdsAccount,
} = useGoogleAdsAccount();
const [ connectGoogleAdsAccount ] = useApiFetchCallback( {
path: '/wc/gla/ads/accounts',
method: 'POST',
data: { id: value },
} );

useEffect( () => {
if ( isConnected ) {
if ( hasGoogleAdsConnection ) {
setValue( googleAdsAccount.id );
}
}, [ googleAdsAccount, isConnected ] );
}, [ googleAdsAccount, hasGoogleAdsConnection ] );

const handleConnectClick = async () => {
if ( ! value ) {
Expand Down Expand Up @@ -86,7 +90,11 @@ const ConnectExistingAccount = ( { onCreateClick } ) => {
}

return (
<ConnectButton accountID={ value } onClick={ handleConnectClick } />
<ConnectButton
disabled={ hasGoogleAdsConnection }
accountID={ value }
onClick={ handleConnectClick }
/>
);
};

Expand All @@ -108,13 +116,13 @@ const ConnectExistingAccount = ( { onCreateClick } ) => {
value={ value }
onChange={ setValue }
autoSelectFirstOption
nonInteractive={ isConnected }
nonInteractive={ hasGoogleAdsConnection }
/>
}
actions={
<ConnectAdsFooter
disabled={ isLoading }
isConnected={ isConnected }
isConnected={ hasGoogleAdsConnection }
onCreateNewClick={ onCreateClick }
/>
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ const UpsertingAccount = ( { upsertingAction } ) => {
className="gla-google-combo-service-account-card--ads"
title={ title }
helper={ __(
'This may take a few minutes, please wait a moment…',
'This may take a few moments, please wait…',
'google-listings-and-ads'
) }
indicator={ <LoadingLabel text={ indicatorLabel } /> }
Expand Down
Loading