Consolidate accounts onboarding: Add edit mode to the combo card to enable it to disconnect accounts

[asvinb]

Changes proposed in this Pull Request:

Closes #2605

Replace this with a good description of your changes & reasoning.

Screenshots:

Expanded mode:

Detailed test instructions:

1. Go to the onboarding flow.
2. Once there's a connected Google account, the Edit button is displayed
3. User should be able to connect to another Google account when clicking the Edit button
4. The Google Ads card and MC cards are displayed where users can edit their respective connections.

Additional details:

Changelog entry

[codecov]

Codecov Report

Attention: Patch coverage is 50.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 61.5%. Comparing base (64085a0) to head (68fa52b).
Report is 65 commits behind head on feature/2509-consolidate-google-account-cards.

Files with missing lines	Patch %	Lines
...src/components/ads-account-select-control/index.js	66.7%	2 Missing ⚠️
js/src/hooks/useGoogleMCAccount.js	0.0%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@                               Coverage Diff                               @@
##           feature/2509-consolidate-google-account-cards   #2660     +/-   ##
===============================================================================
- Coverage                                           61.5%   61.5%   -0.0%     
===============================================================================
  Files                                                320     320             
  Lines                                               4934    4941      +7     
  Branches                                            1207    1210      +3     
===============================================================================
+ Hits                                                3036    3040      +4     
- Misses                                              1730    1732      +2     
- Partials                                             168     169      +1     

Flag	Coverage Δ
js-unit-tests	61.5% <50.0%> (-<0.1%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
...src/components/ads-account-select-control/index.js	80.0% <66.7%> (-20.0%)	⬇️
js/src/hooks/useGoogleMCAccount.js	12.5% <0.0%> (-0.8%)	⬇️

---- 🚨 Try these New Features:

• Flaky Tests Detection - Detect and resolve failed and flaky tests
• JS Bundle Analysis - Avoid shipping oversized bundles

[joemcgill]

Initial thoughts after reviewing this:

First off, since you've already merged the update/2603-create-ads-account for testing (which I agree is a good idea), I've updated this PR to merge into that branch and merged upstream changes so it's easier to see what changes are being introduced specifically by this PR, and to make sure commits unrelated to this PR aren't merged to the feature branch before they're ready. Please double check and make sure I didn't miss anything in my merge commit, since there were conflicts.

For the Edit button, I think it makes more sense to pass this to the new actions prop that we introduced for the ConnectAds and ConnectMC components rather than as part of the description.

For consistency with other actions, these could also use the isTertiary button style so they match the actions elsewhere in this card.

One other thing I noticed while testing this is that once you click the Edit action, there is no way to toggle this back off, and connecting all accounts also don't automatically toggle edit mode (nor do I think it should, since that's a bit confusing). Instead, I think we could add a second action when the card is in edit mode to "Cancel" edit mode and go back to a collapsed or semi-collapsed state.

I've taken a pass at making these changes for you to check out.

diff --git a/js/src/components/google-account-card/switch-account-button.js b/js/src/components/google-account-card/switch-account-button.js
index 73c9712bd..8e0dbb4e0 100644
--- a/js/src/components/google-account-card/switch-account-button.js
+++ b/js/src/components/google-account-card/switch-account-button.js
@@ -27,11 +27,13 @@ const SwitchAccountButton = ( {
 		'Or, connect to a different Google account',
 		'google-listings-and-ads'
 	),
+	...restProps
 } ) => {
 	const [ handleSwitch, { loading } ] = useSwitchGoogleAccount();
 
 	return (
 		<AppButton
+			{ ...restProps }
 			isLink
 			disabled={ loading }
 			text={ text }
diff --git a/js/src/components/google-combo-account-card/connected-google-combo-account-card.js b/js/src/components/google-combo-account-card/connected-google-combo-account-card.js
index 2d7d360bf..0acc9bd53 100644
--- a/js/src/components/google-combo-account-card/connected-google-combo-account-card.js
+++ b/js/src/components/google-combo-account-card/connected-google-combo-account-card.js
@@ -95,7 +95,7 @@ const ConnectedGoogleComboAccountCard = () => {
 	}
 
 	const handleEditClick = () => {
-		setEditMode( true );
+		setEditMode( ! editMode );
 	};
 
 	const hasExistingGoogleMCAccounts = existingGoogleMCAccounts?.length > 0;
@@ -118,40 +118,44 @@ const ConnectedGoogleComboAccountCard = () => {
 		( Boolean( creatingWhich ) && ! shouldClaimGoogleAdsAccount ) ||
 		( ! showConnectAds && finalizeAdsAccountCreation );
 
+	const cardActions = (
+		<div className="gla-google-combo-account-card__description-actions">
+			{ editMode ? (
+				<>
+					<SwitchAccountButton
+						isTertiary
+						text={ __(
+							'Connect to a different Google account',
+							'google-listings-and-ads'
+						) }
+					/>
+					<AppButton
+						isTertiary
+						key="cancel"
+						onClick={ handleEditClick }
+					>
+						{ __( 'Cancel', 'google-listings-and-ads' ) }
+					</AppButton>
+				</>
+			) : (
+				<AppButton
+					isTertiary
+					text={ __( 'Edit', 'google-listings-and-ads' ) }
+					onClick={ handleEditClick }
+				/>
+			) }
+		</div>
+	);
+
 	return (
 		<div>
 			<AccountCard
 				appearance={ APPEARANCE.GOOGLE }
 				alignIcon="top"
 				className="gla-google-combo-account-card gla-google-combo-account-card--connected gla-google-combo-service-account-card--google"
-				description={
-					<>
-						{ text || <AccountDetails /> }
-
-						<div className="gla-google-combo-account-card__description-actions">
-							{ ! editMode && (
-								<AppButton
-									isLink
-									text={ __(
-										'Edit',
-										'google-listings-and-ads'
-									) }
-									onClick={ handleEditClick }
-								/>
-							) }
-
-							{ editMode && (
-								<SwitchAccountButton
-									text={ __(
-										'Connect to a different Google account',
-										'google-listings-and-ads'
-									) }
-								/>
-							) }
-						</div>
-					</>
-				}
+				description={ text || <AccountDetails /> }
 				helper={ subText }
+				actions={ cardActions }
 				indicator={ <Indicator showSpinner={ showSpinner } /> }
 			>
 				<ConnectedAdsAccountsActions
diff --git a/js/src/components/google-combo-account-card/connected-google-combo-account-card.scss b/js/src/components/google-combo-account-card/connected-google-combo-account-card.scss
index c085e78a4..89cb10ce0 100644
--- a/js/src/components/google-combo-account-card/connected-google-combo-account-card.scss
+++ b/js/src/components/google-combo-account-card/connected-google-combo-account-card.scss
@@ -20,8 +20,4 @@
 			margin-bottom: calc((var(--large-gap)) / 2);
 		}
 	}
-
-	.gla-google-combo-account-card__description-actions {
-		margin-top: $grid-unit-10;
-	}
 }
diff --git a/js/src/components/google-combo-account-card/index.scss b/js/src/components/google-combo-account-card/index.scss
index 0f59a5c38..ddc4bf5f7 100644
--- a/js/src/components/google-combo-account-card/index.scss
+++ b/js/src/components/google-combo-account-card/index.scss
@@ -1,5 +1,5 @@
 .gla-google-combo-account-card {
-	.gla-account-card__actions .app-button {
+	.gla-account-card__actions .app-button:first-child {
 		margin-left: calc(var(--main-gap) / -2);
 	}
 }

Finally, it looks like E2E tests are failing in this branch (even before my upstream merge) but I've not gotten to investigate what's happening there.

Update: ☝🏻 these are actually running fine, so was probably a problem locally or some additional flakiness we've yet to debug.

[asvinb]

I forgot about the actions prop 🤦 I've updated the PR, can you kindly check again please?

[joemcgill]

Left some suggestions for improving the E2E tests. Otherwise, this is looking good.

[joemcgill]

I've confirmed that inclusion of the 'Cancel' button with @fblascogarma and updated the issue description accordingly.

[joemcgill]

E2E Changes look good! Thanks

[joemcgill]

I've made a few additional updates to address some small UI issues that were caught by @fblascogarma during UAT:

• 109b724 – Add and external icon to the "Claim account in Google Ads" button.
• 17dab80 – Tweak wording when an Ads account is being created.

And also changed the logic for showing the address card in 0348394, so that it only shows once MC is ready, as pointed out in this comment:

This condition may need an adjustment since if refreshing the webpage during the reclaim URL state, it will show the store address card.

[joemcgill]

I realized during more testing that attempting to keep the MC Reclaim flow visible when someone refreshes leads to a number of additional issues, including:

• The Switch account button in the ReclaimUrlCard expects to use the result.reset() method returned from useConnectMCAccount() or useCreateMCAccount() hooks.
• The ReclaimUrlCard also displays the ID and URL based on the response from the gla/mc/accounts API, which would need to be replaced.

Given those challenges, and the fact that the current setup flow doesn't account for refreshing the page when the MC ReclaimUrlCard is shown, I've opted to revert those changes and modify the AccountCard in ff47280 so it shows the original connection flow if the page is refreshed while the Reclaim is pending.

[eason9487]

After more cross-testing in different scenarios, it turns out that the current edit mode is still a bit awkward compared to the original. For example:

• If I would like to disconnect a created but not claimed Google Ads account, the options are:

  • Claim it first and then disconnect it
  • Disconnect Google account and redo all account connections again

  

• The situation where the newly created Google Merchant Center account won't be listed in existing accounts for a while might lead to a status that can not continue the onboarding.

  • See Consolidate accounts onboarding: Add edit mode to the combo card to enable it to disconnect accounts #2660 (comment)

• If an error occurs during the (automatic) creation of Google Merchant Center account, it may be stuck in a situation where the account can not be connected nor disconnected.

  • This issue is similar to Consolidate accounts onboarding: Add edit mode to the combo card to enable it to disconnect accounts #2660 (review) and it might be more about the API design issue. Because when that error occurs, the connection states could be:

    {
      "id": 0,
      "status": "incomplete",
      "step": "set_id"
    }

  • The following video was tested with a non-publicly accessible website, it shows some issues:

    • The Edit button didn't work
    • After the webpage was refreshed, error occurred - "There was an error loading Google Merchant Center contact information."
    • After the webpage was refreshed, the Connect button didn't work
    • After the webpage was refreshed, it couldn't disconnect the incomplete Google Merchant Center account unless disconnecting Google account
    Kapture.2024-11-18.at.16.49.22.mp4

[joemcgill]

I've made a few more adjustments that I think resolves the remaining issues:

If I would like to disconnect a created but not claimed Google Ads account, the options are

Now, when a new Ads account is awaiting a claim, you can click "Edit" to change the connection (93d588e). If there are not other existing accounts, the connected but not claimed account will be shown in the UI similar to how the ConnectMCCard handles this situation (93d588e).

The situation where the newly created Google Merchant Center account won't be listed in existing accounts for a while might lead to a status that can not continue the onboarding.

Now, for both MC and Ads accounts, if in edit mode and there are no existing accounts to change to, we show the "Create a new ... account" button so that the action makes sense and so we don't leave the user in a situation where we disconnect from the current account with no other account to connect to (fd0ad0e)

If an error occurs during the (automatic) creation of Google Merchant Center account, it may be stuck in a situation where the account can not be connected nor disconnected.

This one is harder to reproduce, because it only seems to occur if an error happens while trying to complete the set_id setup step for MC accounts. In this case, the status will be incomplete and the id will not be set, i.e. id: 0. The useGoogleMCAccount() hook, which is used in many places to determine whether we have an MC connection doesn't take the ID into account for the value of hasGoogleMCConnection, so I've made that a requirement in 58820c1. This also avoids the issue described in #2660 (comment) where the MC card is shown without a selector.

In addition to these changes, I've added some more E2E tests to cover the new edit button considerations and improved the way the border-radius for the combo card was being handled since some UI was not being styled appropriately.

[eason9487]

Apart from the two newly introduced issues, the issue in #2660 (comment) is not addressed.

[joemcgill]

@eason9487 for the two cases where we are showing the "Create new account" button I've modified this logic slightly in 87049d8 to only show the "Create new account" button only if there are no existing accounts. Otherwise, disconnecting the currently connected account leaves the UI in an unrecoverable state because there will be no other accounts to connect to. Here's a video of showing an example when disconnecting a newly created MC account when there are no other accounts to connect to.

Screen.Recording.2024-11-19.at.7.11.13.AM.mov

Having a connected account while there are no existing accounts can happen in two scenarios that we found:

1. For MC accounts, the existing accounts query from the Google API doesn't seem to refresh immediately after a new account is created. So if a new account is automatically created, there will be no existing accounts right away. In this state, if someone disconnects and then refreshes the page, it will trigger an additional automatic account creation, which is undesirable. In this scenario, giving them the option to create a new account is the only action they could take.

2. For new Ads accounts, the account will not be listed in existing accounts until after the account is claimed. If someone decides to edit their accounts rather than complete the claim, and disconnect, there will be no other accounts to connect to.