Consolidate accounts onboarding: Claim Google Ads account via the combo card

[ankitrox]

Changes proposed in this Pull Request:

Closes #2582

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

Screenshots:

Detailed test instructions:

1. Once the MC and Ads accounts are created for the user, you should be able to see the ads accounts claim card.
2. Click on the Claim your Google Ads account button, it will open the pop-up window to accept the invitation.
3. If the invitation has been accepted and pop-up is closed, there would be a Updating... spinner in place of Claim your Google Ads account button and eventually claim card will go away and in place of that green colored notice will be shown as shown in screenshot.
4. If the invitation has not been accepted, claim card will stay.

Additional details:

Changelog entry

[codecov]

Codecov Report

Attention: Patch coverage is 14.28571% with 6 lines in your changes missing coverage. Please review.

Project coverage is 62.7%. Comparing base (900d45d) to head (6c93d64).
Report is 64 commits behind head on feature/2509-consolidate-google-account-cards.

Files with missing lines	Patch %	Lines
js/src/utils/showAdsConversionNotice.js	0.0%	4 Missing ⚠️
...ts/google-ads-account-card/claim-account-button.js	50.0%	1 Missing ⚠️
...google-ads-account-card/google-ads-account-card.js	0.0%	1 Missing ⚠️

Additional details and impacted files

@@                               Coverage Diff                               @@
##           feature/2509-consolidate-google-account-cards   #2644     +/-   ##
===============================================================================
- Coverage                                           62.7%   62.7%   -0.0%     
===============================================================================
  Files                                                325     326      +1     
  Lines                                               5161    5166      +5     
  Branches                                            1265    1266      +1     
===============================================================================
+ Hits                                                3238    3239      +1     
- Misses                                              1746    1751      +5     
+ Partials                                             177     176      -1     

Flag	Coverage Δ
js-unit-tests	62.7% <14.3%> (-<0.1%)	⬇️

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

Files with missing lines	Coverage Δ
...ts/google-ads-account-card/claim-account-button.js	90.9% <50.0%> (-9.1%)	⬇️
...google-ads-account-card/google-ads-account-card.js	73.9% <0.0%> (+3.1%)	⬆️
js/src/utils/showAdsConversionNotice.js	0.0% <0.0%> (ø)

[asvinb]

@ankitrox Any idea why we cannot use the ClaimAccountButton from js/src/components/google-ads-account-card/claim-account-button.js?

[asvinb]

@ankitrox I think we should be able to do the same thing by passing the loading prop to AppButton along with the text prop.

[asvinb]

@ankitrox Can we use the same logic in js/src/components/google-ads-account-card/claim-account/index.js

const { fetchGoogleAdsAccountStatus } = useAppDispatch();
useWindowFocusCallbackIntervalEffect( fetchGoogleAdsAccountStatus, 30 );

[asvinb]

@ankitrox There won't be the need to invalidate the data when we'll use the suggested approach above, i.e using useWindowFocusCallbackIntervalEffect

[asvinb]

@ankitrox I don't think we should check whether there's a claim in progress to upsert an Ads account. I think hasAccess and step === 'conversion_action' should be enough:
useEffect( () => {
if ( hasAccess === true && step === 'conversion_action' ) {
upsertAdsAccount();
}
}, [ hasAccess, upsertAdsAccount, step ] );

[ankitrox]

This has been changed to

useEffect( () => {
if ( hasAccess === true && step === 'conversion_action' ) {
	upsertAdsAccount();
}
}, [ hasAccess, step, upsertAdsAccount ] );

[asvinb]

@ankitrox I think the condition to render ClaimAdsAccount should be in ConnectedGoogleComboAccountCard
and we can check as follows:

const shouldClaimGoogleAdsAccount = Boolean(
		googleAdsAccount.id && hasAccess === false
	);

Thus we won't need to track for any button click. Side note, we don't need to check for ! hasFinishedResolution since hasAccess will be falsy if that's not the case.

[asvinb]

@ankitrox The only thing we need from onClick is to have an internal state for the loading feature. We don't need to track whether it was clicked or not.

[asvinb]

@ankitrox Unnecessary fragment.

Suggested change

<>

[asvinb]

@ankitrox Can we not render AccountCreationDescription directly? Why a function? 🤔

<AccountCreationDescription
	isCreatingWhichAccount={ isCreatingWhichAccount }
/>

[asvinb]

@ankitrox Check my previous comments. Let's add the condition to display ClaimAdsAccount

[joemcgill]

This looks very close. One thing that I think we can do to improve this is to do something similar to what the useAutoCheckBillingStatusEffect() hook does here and return early from this callback once we no longer need to check on the claim status of the account.

Here's a quick example:

diff --git a/js/src/components/google-combo-account-card/claim-ads-account/claim-ads-account.js b/js/src/components/google-combo-account-card/claim-ads-account/claim-ads-account.js
index 547ef9927..fb22d1f02 100644
--- a/js/src/components/google-combo-account-card/claim-ads-account/claim-ads-account.js
+++ b/js/src/components/google-combo-account-card/claim-ads-account/claim-ads-account.js
@@ -29,11 +29,19 @@ const ClaimAdsAccount = () => {
 	const { hasAccess, step } = useGoogleAdsAccountStatus();
 	const [ upsertAdsAccount ] = useUpsertAdsAccount();
 
+	const shouldClaimGoogleAdsAccount = Boolean(
+		googleAdsAccount.id && hasAccess === false
+	);
+
 	const checkUpdatedAdsAccountStatus = useCallback( async () => {
+		if ( ! shouldClaimGoogleAdsAccount ) {
+			return;
+		}
+
 		setUpdating( true );
 		await fetchGoogleAdsAccountStatus();
 		setUpdating( false );
-	}, [ fetchGoogleAdsAccountStatus ] );
+	}, [ fetchGoogleAdsAccountStatus, hasAccess ] );
 
 	useWindowFocusCallbackIntervalEffect( checkUpdatedAdsAccountStatus, 30 );
 
@@ -43,9 +51,7 @@ const ClaimAdsAccount = () => {
 		}
 	}, [ hasAccess, step, upsertAdsAccount ] );
 
-	const shouldClaimGoogleAdsAccount = Boolean(
-		googleAdsAccount.id && hasAccess === false
-	);
+
 
 	if ( ! shouldClaimGoogleAdsAccount ) {
 		return null;

[joemcgill]

An alternative way we could avoid needing to poll the ads/account-status API is to move the shouldClaimGoogleAdsAccount logic to this component, and only render this component if shouldClaimGoogleAdsAccount is true.

[ankitrox]

Changed it as per this comment

[ankitrox]

Thanks for reviewing it @joemcgill.

I have added ConnectedAccountsActions component which will be displayed with helper prop of the AccountCard. This will enable us to simplify the styling as can be seen in this change.

Also, this component will hold the business logic like whether or not to show the other components like claim ads account or conversion measurement notice so that the other components like ClaimAdsAccount and ConversionMeasurementNotice will be more UI focused which enables them to be easily testable.

Over to you for another review.

[joemcgill]

The latest updates here are looking good to me.

I tried looking for a better way handle the Claim Ads E2E test so that the successful claim could be checked without a page refresh, but waiting for the useWindowFocusCallbackIntervalEffect to run makes that test take too long, so I modified it again to just confirm that the AccountCard shows the 'Connected' message in 3e2d8ee with additional cleanup in 0f38c5e.

I also cherry-picked the changes from 80764f0 that fixes some flakiness with the account creation tests.

[eason9487]

Found an issue that was not introduced by this PR but must be raised.

If a merchant doesn't authorize all required scopes when connecting Google account, the automatically created process may get stuck and can not continue. Refreshing webpage also remains in the same stuck state.

The following video demos the issue when the Google Ads scope ('https://www.googleapis.com/auth/adwords') is not selected.

Kapture.2024-11-07.at.15.42.10.mp4

The original scope.glaRequired checking logic is no longer applicable to the current implementation. The conditions below must check both scope.gmcRequired and scope.adsRequired.

google-listings-and-ads/js/src/components/google-combo-account-card/index.js

Lines 21 to 25 in 0f38c5e

	if ( isConnected && scope.glaRequired ) {
	return <ConnectedGoogleComboAccountCard />;
	}
	if ( isConnected && ! scope.glaRequired ) {

[joemcgill]

@eason9487 I've left some feedback about one of the questions you raised, but I think it's ready otherwise.

[joemcgill]

Happy to remove the logic to conditionally show the conversion notice, but I don't think the changes are quite right yet. Left two suggestions inline.

[joemcgill]

Happy for us to remove the useEffect logic that only shows this when the conversion action has first been set up, but if we're using similar logic to the GoogleAdsAccountCard we should make it consistent, i.e.

Suggested change

	const showConversionMeasurementNotice =
	( googleAdsAccount?.status === GOOGLE_ADS_ACCOUNT_STATUS.CONNECTED ||
	googleAdsAccount?.step === 'billing' ) &&
	! shouldClaimGoogleAdsAccount;
	const showConversionMeasurementNotice =
	( googleAdsAccount?.status === GOOGLE_ADS_ACCOUNT_STATUS.CONNECTED ||
	googleAdsAccount?.step === 'link_merchant' ) &&
	! shouldClaimGoogleAdsAccount;

Also, thought it doesn't hurt things, I don't think the ! shouldClaimGoogleAdsAccount is necessary, because in neither condition above would shouldClaimGoogleAdsAccount resolve to true.

[joemcgill]

I've handled this in ac3eef9 and moved this logic to a utility function to avoid inconsistent logic.

[joemcgill]

Using the inviteLink value as part of the loading value is problematic, because it is possible for the inviteLink to be an empty string, (ie. inviteLink: ""). The case of the inviteLink being empty seems to be related to a strange race condition.

When no account is connected, the initial state for account-status will be:

{
    "has_access": false,
    "step": "",
    "invite_link": ""
}

To create a new account, we call upsertAdsAccount(), which internally makes a direct POST request (bypassing actions altogether), followed by refetching the Ads account and the new Ads account status.

This is where a race condition can happen because the store will still have the initial account-status with an invite_link: "" and has_access: false but the Ads account will be resolved, causing this logic to be true:

const shouldClaimGoogleAdsAccount = Boolean(
	googleAdsAccount?.id && hasAccess === false
);

I think the safest thing to do is to either invalidate the account-status data whenever we're creating a new account, or add a condition to shouldClaimGoogleAdsAccount that waits for the upsertAdsAccount() loading value to be false. Here's an example:

diff --git a/js/src/components/google-ads-account-card/claim-account-button.js b/js/src/components/google-ads-account-card/claim-account-button.js
index e9afa0a78..b53ded9c9 100644
--- a/js/src/components/google-ads-account-card/claim-account-button.js
+++ b/js/src/components/google-ads-account-card/claim-account-button.js
@@ -45,7 +45,7 @@ const ClaimAccountButton = ( { onClick = noop, loading, ...restProps } ) => {
 	return (
 		<AppButton
 			{ ...restProps }
-			loading={ loading || ! inviteLink }
+			loading={ loading }
 			eventName="gla_open_ads_account_claim_invitation_button_click"
 			eventProps={ getEventProps() }
 			onClick={ handleClaimAccountClick }
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 ab2a4eab8..536db2912 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
@@ -48,7 +48,7 @@ const ConnectedGoogleComboAccountCard = () => {
 	const { invalidateResolution } = useAppDispatch();
 	const { googleAdsAccount } = useGoogleAdsAccount();
 	const { hasAccess, step } = useGoogleAdsAccountStatus();
-	const [ upsertAdsAccount, { action } ] = useUpsertAdsAccount();
+	const [ upsertAdsAccount, { action, loading } ] = useUpsertAdsAccount();
 
 	const finalizeAdsAccountCreation =
 		hasAccess === true && step === 'conversion_action';
@@ -73,7 +73,7 @@ const ConnectedGoogleComboAccountCard = () => {
 	// @TODO: edit mode implementation in 2605
 	const editMode = false;
 	const shouldClaimGoogleAdsAccount = Boolean(
-		googleAdsAccount?.id && hasAccess === false
+		! loading && googleAdsAccount?.id && hasAccess === false
 	);
 
 	const hasExistingGoogleMCAccounts = existingGoogleMCAccounts?.length > 0;

[joemcgill]

Handled in 43c5ab9 and 6e2b00d.

[joemcgill]

@eason9487 this is ready for another review, though it may be easier to test everything if we go ahead and merge #2651 into this PR, since testing that the inviteLink issues are working properly require a new Ads account to be created via the UI, so let us know if you want to review these together.

A summary of the latest updates:

1. Based on these observations the logic for showing the claim component has been updated in 43c5ab9 so it's not shown until the accounts-status is refreshed.
2. Even with ☝🏻 addressed, it's technically possible for the API response to include an empty URL, so I've chosen not to show the button when there is no URL to send them to and only show the claim message instead in 6e2b00d
3. To keep the logic in sync for showing the Ads Conversion setup notice, I've created a new utility function in ac3eef9
4. To avoid the button getting continually stuck on "Waiting.." if the merchant closes the popup without claiming, I've added a retry limit in Consolidate accounts onboarding: Claim Google Ads account via the combo card #2644 (comment), after which the button will be shown again.

Let me know your thoughts.

[eason9487]

The retry mechanism is less than ideal, but if showing a waiting indicator is necessary, then the current implementation can be kept for now.

The naming of useRef variable and redundant loading forwards still need to be resolved.