Edit combo accounts #2660
Edit combo accounts #2660
Conversation
github-actions
bot
added
the
changelog: update
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more.
|
…ate/2605-edit-accounts
joemcgill
changed the base branch from
feature/2509-consolidate-google-account-cards
to
update/2603-create-ads-account
There was a problem hiding this comment.
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.
|
I forgot about the |
|
I've confirmed that inclusion of the 'Cancel' button with @fblascogarma and updated the issue description accordingly. |
|
While doing additional testing of this PR after updating with all of the upstream changes from the target branch, I noticed that when auto-creating accounts, that the MC account card would not be visible if it needed to be reclaimed. Part of the issue is that when a new MC account is created, the new account does not immediately get returned in the list of existing accounts due to what is likely a cache delay with that API. To account for this, I've updated the logic for when I also added another E2E test to confirm that the UI is displaying the correct flows for Reclaiming an MC account and Claiming a new Ads account after accounts are auto-created, since this important interaction was not being covered by our current set of tests. |
|
|
||
| const hasExistingGoogleAdsAccounts = existingGoogleAdsAccounts?.length > 0; | ||
| ( googleMCHasError || | ||
| hasGoogleMCConnection || |
There was a problem hiding this comment.
The hasGoogleMCConnection || seems to be redundant?
There was a problem hiding this comment.
I don't believe it is. Consider the case where a new account is initially created but is not yet claimed and is not yet returned in the list of existing accounts. In that case, we still need to show the ConnectMC card so that the reclaim flow can be shown.
There was a problem hiding this comment.
After cross-testing, it was realized that this is for a more extreme case. It's recommended to add code comment to note that the existence of hasGoogleMCConnection || condition is for the scenario with the following steps:
- Automatically create a Google Merchant Center account
- Reclaim URL is required
- The merchant interrupts the onboarding flow and then resumes it
- This is also the case for refreshing webpage
- The newly created account is not yet among the existing accounts
- The condition enables the merchant to resume the Google Merchant Center connection from the step of connecting the newly created account
js/src/components/google-combo-account-card/connected-google-combo-account-card.js
Outdated
Show resolved
Hide resolved
js/src/components/google-combo-account-card/connected-google-combo-account-card.js
Outdated
Show resolved
Hide resolved
js/src/components/google-mc-account-card/reclaim-url-card/index.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Eason <[email protected]>
|
I've made a few additional updates to address some small UI issues that were caught by @fblascogarma during UAT:
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:
|
|
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:
Given those challenges, and the fact that the current setup flow doesn't account for refreshing the page when the MC |
There was a problem hiding this comment.
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.
-
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 Edit combo 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
-
js/src/components/google-combo-account-card/claim-ads-account/claim-ads-account.js
Outdated
Show resolved
Hide resolved
|
I've made a few more adjustments that I think resolves the remaining issues:
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).
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)
This one is harder to reproduce, because it only seems to occur if an error happens while trying to complete the 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. |
There was a problem hiding this comment.
Apart from the two newly introduced issues, the issue in #2660 (comment) is not addressed.
js/src/components/google-combo-account-card/connect-ads/connect-ads-footer.js
Outdated
Show resolved
Hide resolved
|
@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.movHaving a connected account while there are no existing accounts can happen in two scenarios that we found:
|
There was a problem hiding this comment.
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/accountsAPI 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
Changes proposed in this Pull Request:
Closes #2605
Replace this with a good description of your changes & reasoning.
Screenshots:
Expanded mode:
Detailed test instructions:
Additional details:
Changelog entry