Connect MC Account within combo card #2639

asvinb · 2024-10-03T17:55:05Z

Changes proposed in this Pull Request:

Closes #2597 .

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

Detailed test instructions:

Go through the onboarding process and make sure you have at least one MC account.
The card to connect your MC account should be displayed where you can connect a MC account
The user should also be able to create a new account or disconnect their MC account.
Designs: https://www.figma.com/design/fqR0EHi63lWahRcVTKCcba/Google-Listings-%26-Ads-v2.x?node-id=7447-22280&node-type=frame&t=tKJkit0BKc6985PH-0

Additional details:

There are no designs to reclaim a URL and existing components have been used as is.

Changelog entry

…te/2597-connect-mc-account

codecov · 2024-10-03T17:58:05Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.7%. Comparing base (4f5bb47) to head (17e7c84).

Additional details and impacted files

@@                               Coverage Diff                               @@
##           feature/2509-consolidate-google-account-cards   #2639     +/-   ##
===============================================================================
- Coverage                                           62.7%   62.7%   -0.0%     
===============================================================================
  Files                                                325     325             
  Lines                                               5162    5161      -1     
  Branches                                            1265    1265             
===============================================================================
- Hits                                                3239    3238      -1     
  Misses                                              1746    1746             
  Partials                                             177     177

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

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

Files with missing lines	Coverage Δ
js/src/hooks/useAutoCreateAdsMCAccounts.js	`93.6% <100.0%> (-0.1%)`	⬇️

…ate/2597-connect-mc-account

…te/2597-connect-mc-account

asvinb · 2024-10-08T12:35:39Z

@joemcgill Can you review PR please? The failing unit tests are from the parent branch.

joemcgill

Thanks @asvinb This is looking pretty good, but I've got a few questions/suggestions inline.

js/src/components/google-combo-account-card/connect-mc/connect-mc.js

joemcgill · 2024-10-08T21:44:33Z

js/src/components/google-combo-account-card/connect-mc/connect-mc.js

+	if ( ! isConnected && resultConnectMC.response?.status === 409 ) {
+		return (
+			<SwitchUrlCard
+				id={ resultConnectMC.error.id }
+				message={ resultConnectMC.error.message }
+				claimedUrl={ resultConnectMC.error.claimed_url }
+				newUrl={ resultConnectMC.error.new_url }
+				onSelectAnotherAccount={ resultConnectMC.reset }
+			/>
+		);
+	}
+
+	if (
+		! isConnected &&
+		( resultConnectMC.response?.status === 403 ||
+			resultCreateAccount.response?.status === 403 )
+	) {
+		return (
+			<ReclaimUrlCard
+				id={
+					resultConnectMC.error?.id || resultCreateAccount.error?.id
+				}
+				websiteUrl={
+					resultConnectMC.error?.website_url ||
+					resultCreateAccount.error?.website_url
+				}
+				onSwitchAccount={ () => {
+					resultConnectMC.reset();
+					resultCreateAccount.reset();
+				} }
+			/>
+		);
+	}
+
+	if (
+		! isConnected &&
+		( resultCreateAccount.loading ||
+			resultCreateAccount.response?.status === 503 )
+	) {
+		return (
+			<CreatingCard
+				retryAfter={ resultCreateAccount.error?.retry_after }
+				onRetry={ handleCreateAccount }
+			/>
+		);
+	}


This duplicates a lot of logic directly from js/src/components/google-mc-account-card/connect-mc/index.js. Curious if we could abstract some of this into a shared component and avoid the redundant isConnected check in the process?

@joemcgill I've introduced a new AccountConnectionStatus where we have this logic.
https://github.com/woocommerce/google-listings-and-ads/pull/2639/files#diff-398198aee87feb6f89983540431ac7e76d24a4d36793dd440d8758a287e190bbR1

However am not sure if there's an easier way to have the conditions to render that component:
https://github.com/woocommerce/google-listings-and-ads/pull/2639/files#diff-adf83b7ae2454ff433c2f50eb0f115c58b3a97776516764ef0ec88707353d177R45-R51

joemcgill · 2024-10-08T22:05:33Z

js/src/components/google-combo-account-card/connected-google-combo-account-card.js

+				indicator={ getIndicator() }
+			/>
+
+			<ConnectMC />


Rather than making this a sibling component of the AccountCard above, I would have expected ConnectMC to return a Section.Card.Body component that is passed as a child of the AccountCard above.

@joemcgill We synced in Slack about that. We'll have a group of cars, so it'll be siblings.

…te/2597-connect-mc-account

joemcgill

Left a few questions, but no blockers. Marking this as approved and ready for QA/WooCR

joemcgill · 2024-10-26T20:22:20Z

js/src/components/google-combo-account-card/connect-mc/connect-mc-body.js

+			{ ! isConnected && ! isConnecting && (
+				<AppButton
+					isSecondary
+					disabled={ ! value }


In what circumstance would this be visible and disabled?

@joemcgill This will be visible only when we need to connect a new account. I removed the disabled prop since we will always have a value set.

joemcgill · 2024-10-26T20:32:22Z

js/src/components/google-combo-account-card/connect-mc/connect-mc.js

+	useEffect( () => {
+		if ( isGoogleMCReady ) {
+			setAccountID( googleMCAccount.id );
+		}
+	}, [ googleMCAccount, isGoogleMCReady ] );


Not sure I understand why we need to save the MC account separately as component state here rather than using googleMcaccount.id directly?

@joemcgill We need to save the value set from the select in case the user needs to connect to a different acccount.

joemcgill · 2024-10-26T20:40:37Z

js/src/components/google-combo-account-card/connect-mc/merchant-center-select.js

+		( existingAccount ) => existingAccount.id === googleMCAccount.id
+	);
+
+	if ( ! accountIdExists && isConnected ) {


This is a clever workaround. I assume this is for a newly created account that has not yet shown up in the API response for existing accounts? If so, adding that context to the inline doc would be helpful.

@joemcgill I added it at the start of the component where we explain why we have this intermediary component:
https://github.com/woocommerce/google-listings-and-ads/pull/2639/files#diff-38d86396eccec4e8e7e5b37e8de9cf25e28fb0a2536261feb312adfe9c6689aeR15-R24

joemcgill · 2024-10-26T20:44:04Z

js/src/components/google-mc-account-card/connect-mc/account-connection-status.js

+	resultCreateAccount,
+	onRetry,
+} ) => {
+	if ( resultConnectMC.response?.status === 409 ) {


Not a blocker, but I think using raw response status codes like this is not very readable. Curious if we could create a utility to map these to more human friendly, or at least add inline comments that makes it easy to understand what a 409 indicates.

I added some inline comments, just describing what are the error statuses. More details can be found in the utils function: https://github.com/woocommerce/google-listings-and-ads/pull/2639/files#diff-289f2d84147885af5105af45f5b616e4057a4490f0fe42b5cf487ca942fdfcefR6-R7

…ate/2597-connect-mc-account

ankitguptaindia · 2024-10-28T18:00:02Z

QA Report- ✅

Testing Environment -

WordPress: 6.6.2
Theme active on store: Twenty Twenty-Four Version: 1.2
WooCommerce - Version 9.3.3
PHP: 8.3
Web Server: Nginx
Browser: Chrome - Version 130
OS: macOS Sonoma 14.6.1

Test Results: Tested multiple scenarios related to MC account creation and connection with the store, including cases where Google accounts have multiple MC accounts, a single MC account, connection when the URL is already claimed, connection when the URL doesn’t need to be claimed, and creating a new MC account from the options. All cases were found to be working.

Functional Demo / Screencast

Connection when Google account has only single MC account:

when.account.has.only.One.MC.account.avaaible.mp4

Creating New account when Google Account has single MC account:

creeatung.new.account.mp4

Creating account when Google Accounts has multiple MC accounts:

Creating.new.account.when.user.has.already.multiple.MC.accounts.mp4

Connection with existing MC account when Google has multiple MC accounts:

Connecting.when.multiple.MC.accounts.availble.mp4

…ctor 2597: Refactor ConnectMC with updated AccountCard

…ate/2597-connect-mc-account

asvinb · 2024-10-31T13:16:31Z

js/src/components/google-combo-account-card/connected-google-combo-account-card.js

@@ -39,7 +56,7 @@ const ConnectedGoogleComboAccountCard = () => {
 			<AccountCard
 				appearance={ APPEARANCE.GOOGLE }
 				alignIcon="top"
-				className="gla-google-combo-account-card--connected"
+				className="gla-google-combo-account-card gla-google-combo-account-card--connected"


Adding the gla-google-combo-account-card to be consistent with the other cards.

eason9487 · 2024-11-01T06:29:31Z

js/src/components/google-combo-account-card/connect-mc/merchant-center-select.js

+const MerchantCenterSelect = ( { isConnected, ...rest } ) => {
+	const { data: existingAccounts } = useExistingGoogleMCAccounts();
+	const { googleMCAccount } = useGoogleMCAccount();
+	const domain = new URL( getSetting( 'homeUrl' ) ).host;


How about moving this variable into the below if block?

Sure thing! Updated @eason9487

eason9487 · 2024-11-01T07:21:10Z

js/src/data/resolvers.js

+getExistingGoogleMCAccounts.shouldInvalidate = ( action ) => {
+	return action.type === TYPES.RECEIVE_ACCOUNTS_GOOGLE_MC;
+};


Not sure if this invalidation is trying to refetch the existing accounts so that it can have the newly created account. If so, after the following findings, maybe this could be removed since it usually won't work.

google-listings-and-ads/js/src/components/google-combo-account-card/connect-mc/merchant-center-select.js

Lines 20 to 22 in 8fba3e4

* we found that when a URL is reclaimed for Merchant Center (MC), the Google API does not

* return the newly reclaimed account immediately in the list provided by the useExistingGoogleMCAccounts hook,

* even though the data in the store is invalidated. In that case, thus we end up having an account ID

eason9487 · 2024-11-01T08:29:35Z

tests/e2e/specs/setup-mc/step-1-accounts.test.js

 				await setUpAccountsPage.mockMCConnected();
 				await setUpAccountsPage.mockAdsAccountConnected();

+				await setUpAccountsPage.mockMCHasAccounts();
+				await setUpAccountsPage.mockMCConnected();


Duplicated await setUpAccountsPage.mockMCConnected();

Thanks! Removed.

eason9487 · 2024-11-01T08:42:32Z

tests/e2e/specs/setup-mc/step-1-accounts.test.js

+				const createAccountButton =
+					setUpAccountsPage.getMCCreateAccountButtonFromPage();
+				await createAccountButton.click();
+				await page.waitForLoadState( LOAD_STATE.DOM_CONTENT_LOADED );


Why it needs to wait for load state?

Turns out we don't need it. Removed.

eason9487 · 2024-11-01T08:42:41Z

tests/e2e/specs/setup-mc/step-1-accounts.test.js

+					await page.waitForLoadState(
+						LOAD_STATE.DOM_CONTENT_LOADED
+					);


Why it needs to wait for load state?

Reviewed and removed unnecessary wait for load state.

eason9487 · 2024-11-01T09:11:24Z

tests/e2e/specs/setup-mc/step-1-accounts.test.js

+					// Mock Jetpack as connected.
+					setUpAccountsPage.mockJetpackConnected(),
+
+					// Mock google as connected.
+					setUpAccountsPage.mockGoogleConnected(
+						'[email protected]'
+					),


These mocks seem to be redundant as the upper test.beforeAll block has set the same mocks and they won't invalidated when entering here.

google-listings-and-ads/tests/e2e/specs/setup-mc/step-1-accounts.test.js

Lines 297 to 304 in 8fba3e4

test.describe( 'Connect Merchant Center account', () => {

test.beforeAll( async () => {

await Promise.all( [

// Mock Jetpack as connected.

setUpAccountsPage.mockJetpackConnected(),

// Mock google as connected.

setUpAccountsPage.mockGoogleConnected( '[email protected]' ),

eason9487 · 2024-11-01T09:18:16Z

tests/e2e/utils/pages/setup-mc/step-1-set-up-accounts.js

+		return this.getMCAccountCard().locator(
+			'select[id*="inspector-select-control"]'
+		);


Suggested change

return this.getMCAccountCard().locator(

'select[id*="inspector-select-control"]'

);

return this.getMCAccountCard().getByRole( 'combobox' );

eason9487 · 2024-11-01T09:27:23Z

tests/e2e/specs/setup-mc/step-1-accounts.test.js

+						// Mock Jetpack as connected.
+						setUpAccountsPage.mockJetpackConnected(),
+
+						// Mock google as connected.
+						setUpAccountsPage.mockGoogleConnected(
+							'[email protected]'
+						),
+
+						// Mock Google Ads as connected.
+						setUpAccountsPage.mockAdsAccountConnected(),
+						setUpAccountsPage.mockAdsStatusClaimed(),


These mocks seem to be redundant.

👍 Removed.

eason9487 · 2024-11-01T09:28:13Z

tests/e2e/specs/setup-mc/step-1-accounts.test.js

+				test( 'should see see a modal to ensure the intention of creating a new account', async () => {
+					// Click 'Or, create a new Merchant Center account'
+					const mcFooterButton =
+						setUpAccountsPage.getMCCardFooterButton();
+					await mcFooterButton.click();
+					await page.waitForLoadState(
+						LOAD_STATE.DOM_CONTENT_LOADED
+					);
+
+					const modalHeader = setUpAccountsPage.getModalHeader();
+					await expect( modalHeader ).toContainText(
+						'Create Google Merchant Center Account'
+					);
+
+					const modalCheckbox = setUpAccountsPage.getModalCheckbox();
+					await expect( modalCheckbox ).toBeEnabled();
+
+					const modalPrimaryButton =
+						setUpAccountsPage.getModalPrimaryButton();
+					await expect( modalPrimaryButton ).toContainText(
+						'Create account'
+					);
+					await expect( modalPrimaryButton ).toBeDisabled();
+
+					// Select the checkbox, the button should be enabled.
+					await modalCheckbox.click();
+					await expect( modalPrimaryButton ).toBeEnabled();
+				} );


Looks like the same test is already in place.

google-listings-and-ads/tests/e2e/specs/setup-mc/step-1-accounts.test.js

Lines 342 to 364 in 8fba3e4

test( 'click "Create account" button should see the modal of confirmation of creating account', async () => {

// Click the create account button

const createAccountButton =

setUpAccountsPage.getMCCreateAccountButtonFromPage();

await createAccountButton.click();

await page.waitForLoadState( LOAD_STATE.DOM_CONTENT_LOADED );

const modalHeader = setUpAccountsPage.getModalHeader();

await expect( modalHeader ).toContainText(

'Create Google Merchant Center Account'

);

const modalCheckbox = setUpAccountsPage.getModalCheckbox();

await expect( modalCheckbox ).toBeEnabled();

const createAccountButtonFromModal =

setUpAccountsPage.getMCCreateAccountButtonFromModal();

await expect( createAccountButtonFromModal ).toBeDisabled();

// Click the checkbox of accepting ToS, the create account button will be enabled.

await modalCheckbox.click();

await expect( createAccountButtonFromModal ).toBeEnabled();

} );

Test removed @eason9487

eason9487 · 2024-11-01T09:28:50Z

tests/e2e/utils/pages/setup-mc/step-1-set-up-accounts.js

+		return this.page
+			.locator( '.components-card-body', {
+				has: this.page.locator( '.gla-account-card__title', {
+					hasText: 'Google',
+				} ),
+			} )
+			.first();


May add a CSS class for this as well like the getMCAccountCard method below.

eason9487

Most of the original components under the js/src/components/google-mc-account-card directory are only used for the Onboarding accounts step, except for ConnectedGoogleMCAccountCard which is shared by the Setting page. Given this, extracting some code into shared components or functions and creating another ConnectedMC component would make for unnecessary redundant code and reduce maintainability.

This PR should be able to be done based on the original existing code and minimize code changes to optimally leverage codes that have been run for 2-3 years.

Suggest exploring possibilities in this direction:

Adjust the google-mc-account-card/connect-mc/index.js to fulfill the new UI and functionality rather than creating google-combo-account-card/connect-mc/connect-mc.js
Revert the extraction of AccountConnectionStatus and hasAccountConnectionIssue
ConnectedGoogleMCAccountCard will only be used on the Settings page and its hideAccountSwitch prop will no longer be needed
- Remove code related to hideAccountSwitch
- Rename to specify that this component is dedicated to the Settings page

asvinb added 3 commits October 3, 2024 20:28

Save WIP.

874ef00

Merge branch 'feature/2567-kickoff-mc-ads-account-creation' into upda…

1ead7f9

…te/2597-connect-mc-account

Extract DisconnectAccount button.

4428639

github-actions bot added the changelog: update Big changes to something that wasn't broken. label Oct 3, 2024

asvinb added 15 commits October 4, 2024 12:15

Add ConnectAccountCard component.

bebfaed

Add ConnectMCBody and ConnectMCFooter component.

872787e

Save WIP.

9475ec2

Check for non connected state.

3965234

Add preConditionReady check.

8a4ba4c

Revert dev temp changes.

8b850fd

Save E2E tests,

176a4a8

Merge branch 'feature/2509-consolidate-google-account-cards' into upd…

b4b4347

…ate/2597-connect-mc-account

Save WIP.

e9626f0

Merge branch 'feature/2567-kickoff-mc-ads-account-creation' into upda…

57c4693

…te/2597-connect-mc-account

Review class names.

e86de9b

Merge branch 'feature/2567-kickoff-mc-ads-account-creation' into upda…

9ad469a

…te/2597-connect-mc-account

Fix e2e tests.

75723e2

Merge branch 'feature/2567-kickoff-mc-ads-account-creation' into upda…

9923f70

…te/2597-connect-mc-account

Use mock functions available.

406f028

asvinb marked this pull request as ready for review October 8, 2024 12:35

joemcgill linked an issue Oct 8, 2024 that may be closed by this pull request

Onboarding: Connect to existing MC account in Google Accounts Card #2597

Open

5 tasks

joemcgill requested changes Oct 8, 2024

View reviewed changes

asvinb added 6 commits October 9, 2024 15:49

Merge branch 'feature/2567-kickoff-mc-ads-account-creation' into upda…

26ec13a

…te/2597-connect-mc-account

Address CR feedback.

acedda6

Cast googleMCAccount?.id to a boolean.

13d8d2b

Add description when connecting extra account.

9e8649b

Fix card when reclaiming URL.

2981067

Fix E2E tests.

e03f3c3

Merge branch 'feature/2567-kickoff-mc-ads-account-creation' into upda…

c117be7

…te/2597-connect-mc-account

joemcgill linked an issue Oct 25, 2024 that may be closed by this pull request

Onboarding: Create new MC account when there are existing accounts in Google Accounts Card #2604

Open

7 tasks

joemcgill approved these changes Oct 26, 2024

View reviewed changes

joemcgill requested a review from eason9487 October 26, 2024 20:51

Base automatically changed from feature/2567-kickoff-mc-ads-account-creation to feature/2509-consolidate-google-account-cards October 28, 2024 06:50

asvinb added 2 commits October 28, 2024 12:47

Merge branch 'feature/2509-consolidate-google-account-cards' into upd…

fa72d4e

…ate/2597-connect-mc-account

Add JS Docs.

7a4e40d

ankitguptaindia self-requested a review October 28, 2024 18:00

ankitguptaindia approved these changes Oct 28, 2024

View reviewed changes

joemcgill mentioned this pull request Oct 30, 2024

Onboarding: Connect to existing Ads account in Google Accounts Card #2640

Merged

joemcgill added 2 commits October 30, 2024 15:16

Update AccountCard component

53c80c5

Incorporate new AccountCard into ConnectMC

a9ea1b1

joemcgill mentioned this pull request Oct 30, 2024

2597: Refactor ConnectMC with updated AccountCard #2657

Merged

joemcgill and others added 11 commits October 30, 2024 16:34

Update docblocks to AccountCard

cb667fd

JSLint fix

14c653c

Delete ConnectAccountCard

a9c8839

Remove todo

a8b4f8c

Add missing styles.

9c55a39

Use correct button variant.

c40800b

Merge pull request #2657 from woocommerce/update/2597-connect-mc-refa…

7a757a2

…ctor 2597: Refactor ConnectMC with updated AccountCard

Sync changes.

cc3e804

Merge branch 'feature/2509-consolidate-google-account-cards' into upd…

8c4cd7a

…ate/2597-connect-mc-account

Update JSDocs.

a3040fe

Fix E2E tests.

e75a930

asvinb commented Oct 31, 2024

View reviewed changes

Remove unused functions.

8fba3e4

eason9487 reviewed Nov 1, 2024

View reviewed changes

Address CR feedback.

17e7c84

	* we found that when a URL is reclaimed for Merchant Center (MC), the Google API does not
	* return the newly reclaimed account immediately in the list provided by the useExistingGoogleMCAccounts hook,
	* even though the data in the store is invalidated. In that case, thus we end up having an account ID

	test.describe( 'Connect Merchant Center account', () => {
	test.beforeAll( async () => {
	await Promise.all( [
	// Mock Jetpack as connected.
	setUpAccountsPage.mockJetpackConnected(),

	// Mock google as connected.
	setUpAccountsPage.mockGoogleConnected( '[email protected]' ),

	test( 'click "Create account" button should see the modal of confirmation of creating account', async () => {
	// Click the create account button
	const createAccountButton =
	setUpAccountsPage.getMCCreateAccountButtonFromPage();
	await createAccountButton.click();
	await page.waitForLoadState( LOAD_STATE.DOM_CONTENT_LOADED );

	const modalHeader = setUpAccountsPage.getModalHeader();
	await expect( modalHeader ).toContainText(
	'Create Google Merchant Center Account'
	);

	const modalCheckbox = setUpAccountsPage.getModalCheckbox();
	await expect( modalCheckbox ).toBeEnabled();

	const createAccountButtonFromModal =
	setUpAccountsPage.getMCCreateAccountButtonFromModal();
	await expect( createAccountButtonFromModal ).toBeDisabled();

	// Click the checkbox of accepting ToS, the create account button will be enabled.
	await modalCheckbox.click();
	await expect( createAccountButtonFromModal ).toBeEnabled();
	} );

Connect MC Account within combo card #2639

Are you sure you want to change the base?

Connect MC Account within combo card #2639

Conversation

asvinb commented Oct 3, 2024 • edited by eason9487 Loading

Changes proposed in this Pull Request:

Detailed test instructions:

Additional details:

Changelog entry

codecov bot commented Oct 3, 2024 • edited Loading

Codecov Report

asvinb commented Oct 8, 2024

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ankitguptaindia commented Oct 28, 2024

QA Report- ✅

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eason9487 left a comment • edited Loading

Choose a reason for hiding this comment

asvinb commented Oct 3, 2024 •

edited by eason9487

Loading

codecov bot commented Oct 3, 2024 •

edited

Loading

eason9487 left a comment •

edited

Loading