Kickoff MC and Ads account creation #2618

ankitrox · 2024-09-18T15:26:07Z

Changes proposed in this Pull Request:

Closes #2567 .

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

Screenshots:

Detailed test instructions

Create a new site and setup the GLA plugin (Make sure you are testing on the feature branch if this is not merged).
Go to Marketing > Google for Woocommerce and start onboarding process.
Connect to the fresh Google account. This account should not have any existing merchant center accounts or Google ads account.
As soon as user sign-in flow is completed and user lands back to onboarding screen, there should be the card status changed to the one which should read You don’t have Merchant Center nor Google Ads accounts, so we’re creating them for you. as per the Figma designs.
Once Merchant Center account and Ads account is created, card state will be changed this. Which will display Merchant Center ID and Google Ads ID (for 1-2 seconds ads ID may be seen as zero because ads request gets resolved later).
Note that there would be some error notices popping up regarding ads account connection and ads connection status, but this can be ignored for this issue and will be handled in Onboarding: Claim New Ads Accounts in the Google Combo Accounts Card #2582

Use custom plugin to test with existing account.

As it is very inconvenient to test with the fresh account everytime; especially in the scenario where Google may not allow more accounts to be created and asks for new mobile number (where we are out of phone numbers). Following method can help us to test with existing Google account.

Following plugin mimics the scenario such that for the first request to MC or Ads, it mocks the request such that there are no MC and Ads account.

<?php
/**
 * Plugin name: 2567 Test
 * Description: A plugin to provide REST API helper utilities.
 */

add_filter('rest_pre_dispatch', function($result, $server, $request) {
    $method = $request->get_method();
    $route  = $request->get_route();

    if ( 'POST' === $method ) {
        switch ($route) {
            case '/wc/gla/ads/accounts':
                $result = [
                    'message' => 'Account must be accepted before completing setup.',
                    'has_access' => false,
                    'step' => 'account_access',
                    'invite_link' => 'https://ads.google.com/nav/startacceptinvite?ivid=202079690&ocid=6741519560&eivid=ARZPQJ79CnAPjRUtJ1N2kGxycD2IjlRsgCNVRdiDRtvIyQRvkslrzx0',
                ];
                break;
            case '/wc/gla/mc/accounts':
                $result = [
                    'id'         => 5432178,
                    'name'       => null,
                    'subaccount' => null,
                    'domain'     => null,
                ];
                break;
        }
    }

    return $result;
}, 10, 3);

/**
 * Filter the response before it is returned to the client.
 * 
 * @param WP_HTTP_Response $response Result to send to the client. Usually a WP_REST_Response.
 * @param WP_REST_Server $server Server instance.
 * @param WP_REST_Request $request Request used to generate the response.
 */
add_filter('rest_post_dispatch', function($response, $server, $request) {

    // Mock accounts endpoints.
    if( '/wc/gla/ads/accounts' === $request->get_route() ) {
        $method = $request->get_method();

        if ( 'POST' === $method ) {
            $response->set_status( 428 );
        }

        if ( 'GET' === $method ) {
            $ads_request_number = get_option('gla_ads_account_request_number', 0);
            update_option('gla_ads_account_request_number', $ads_request_number + 1);

            $data = [];

            if ( $ads_request_number > 0 ) {
               $data[] = [
                'id'   => 78787878,
                'name' => 'gla.instawp.xyz',
               ];
            }

            $response->set_data( $data );
        }
    }

    if( '/wc/gla/mc/accounts' === $request->get_route() ) {
        $method = $request->get_method();

        if ( 'GET' === $method ) {
            $mc_request_number = get_option('gla_mc_account_request_number', 0);
            update_option('gla_mc_account_request_number', $mc_request_number + 1);

            $data = [];
            if ($mc_request_number > 0) {
                $data[] = [
                    'id'         => 5432178,
                    'name'       => null,
                    'subaccount' => null,
                    'domain'     => null,
                ];
            }

            $response->set_data( $data );
        }
    }

    // Mock connections endpoints.
    if( '/wc/gla/ads/connection' === $request->get_route() ) {
        $method = $request->get_method();
        if ( 'GET' === $method ) {
            $ads_request_number = get_option('gla_ads_connection_request_number', 0);
            update_option('gla_ads_connection_request_number', $ads_request_number + 1);

            $data = [
                'id'         => 0,
                'currency'   => 'USD',
                'status'     => 'disconnected',
                'symbol'     => '$',
            ];

            if ($ads_request_number > 0) {
                $data['id']     = 78787878;
                $data['status'] = 'incomplete';
                $data['step']   = 'account_access';
            }

            $response->set_data( $data );
        }
    }

    if( '/wc/gla/mc/connection' === $request->get_route() ) {
        $method = $request->get_method();
        if ( 'GET' === $method ) {
            $ads_request_number = get_option('gla_mc_connection_request_number', 0);
            update_option('gla_mc_connection_request_number', $ads_request_number + 1);

            $data = [
                'id'         => 0,
                'name'       => null,
                'subaccount' => null,
                'domain'     => null,
            ];

            if ($ads_request_number > 0) {
                $data['id']     = 5432178;
                $data['status'] = 'incomplete';
                $data['step']   = 'claim';
            }

            $response->set_data( $data );
        }
    }

    return $response;
}, 10, 3);

add_filter( 'woocommerce_gla_ads_billing_setup_status', function( $status ) {
	return 'approved';
} );

Once the test is run, you will need to clear following options from *_options table.

gla_ads_account_request_number
gla_mc_account_request_number
gla_ads_connection_request_number
gla_mc_connection_request_number

These options can also be cleared using following npm command in local setup.

npm run wp-env run cli wp option delete gla_ads_account_request_number && npm run wp-env run cli wp option delete gla_mc_account_request_number && npm run wp-env run cli wp option delete gla_ads_connection_request_number && npm run wp-env run cli wp option delete gla_mc_connection_request_number

Once above command ran successfully, the test can again be performed with given steps.

Additional details:

Changelog entry

Add - During onboarding, automatically create Google Merchant Center or Google Ads accounts when the connected Google account doesn't have a respective existing one.

codecov · 2024-09-18T15:30:28Z

Codecov Report

Attention: Patch coverage is 80.64516% with 12 lines in your changes missing coverage. Please review.

Please upload report for BASE (feature/2509-consolidate-google-account-cards@7d31e5c). Learn more about missing BASE report.
Report is 94 commits behind head on feature/2509-consolidate-google-account-cards.

Files with missing lines	Patch %	Lines
js/src/hooks/useGoogleMCAccount.js	12.5%	6 Missing and 1 partial ⚠️
js/src/hooks/useAutoCreateAdsMCAccounts.js	93.8%	3 Missing ⚠️
js/src/hooks/useExistingGoogleAdsAccounts.js	0.0%	2 Missing ⚠️

Additional details and impacted files

@@                               Coverage Diff                               @@
##             feature/2509-consolidate-google-account-cards   #2618   +/-   ##
===============================================================================
  Coverage                                                 ?   62.7%           
===============================================================================
  Files                                                    ?     324           
  Lines                                                    ?    5153           
  Branches                                                 ?    1259           
===============================================================================
  Hits                                                     ?    3232           
  Misses                                                   ?    1744           
  Partials                                                 ?     177

Flag	Coverage Δ
js-unit-tests	`62.7% <80.6%> (?)`

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

Files with missing lines	Coverage Δ
.../components/google-combo-account-card/constants.js	`100.0% <100.0%> (ø)`
js/src/constants.js	`100.0% <100.0%> (ø)`
js/src/hooks/useCreateMCAccount.js	`6.7% <ø> (ø)`
js/src/hooks/useExistingGoogleAdsAccounts.js	`16.7% <0.0%> (ø)`
js/src/hooks/useAutoCreateAdsMCAccounts.js	`93.8% <93.8%> (ø)`
js/src/hooks/useGoogleMCAccount.js	`13.3% <12.5%> (ø)`

Multiple API calls.

asvinb

@ankitrox I left a few comments on the PR. Can you kindly check them out please? Also most importantly I think we should implement the useCreateAccounts hook in such a way that when we need to implement the other features, we can easily tweak/update it. Right now the hook is too rigid 😁 I suggest the following changes:

The hook returns additional properties:
- description: for the You don’t have Merchant Center nor Google Ads accounts, so we’re creating them for you. text
- helperText: for the Merchant Center is required to sync products so they show on Google. Google Ads is required to set up conversion measurement for your store.
  - The text above will be set when we need to create both accounts (which is the case for this ticket).
- adsAccountCreated: boolean to indicate that an ads account has been created
- mcAccountCreated : boolean to indicate that MC account has been created
  - From the 2 properties above, we can deduce accountsCreated.
- isCreatingAdsAccount: boolean to indicate that ads account creation is in progress.
- isCreatingMCAccount: boolean to indicate that MC account creation is in progress.
  - From the 2 properties above, we can deduce isCreatingAccounts

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

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

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

js/src/hooks/useCreateAccounts.js

js/src/components/google-combo-account-card/account-creation-description.js

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

js/src/hooks/useAutoCreateAdsMCAccounts.js

joemcgill · 2024-09-30T19:35:01Z

We had a typo in the feature branch for #2509. I've updated the branch and am changing the base of this PR to merge into the correct branch now.

asvinb

@ankitrox I left some comments. Can you kindly check them out and let me know what you think please?
Thanks!

js/src/components/google-combo-account-card/account-creation-description.js

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

js/src/hooks/useAutoCreateAdsMCAccounts.js

joemcgill

I've given this a look and given a bit of feedback inline. I do think it's important to resolve the hook dependencies conversation here before sending this to QA. There also seems to be some ESLint and Unit test issues to resolve before this is ready.

js/src/components/google-combo-account-card/account-creation-description.js

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

js/src/hooks/useExistingGoogleAdsAccounts.js

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

joemcgill

Nice updates! Some small suggestions about the new useGoogleMCAccountReady() hook inline

js/src/hooks/useGoogleMCAccountReady.js

js/src/setup-mc/setup-stepper/setup-accounts/index.js

This removes the unnecessary `SetupAdsAccountPage` from the E2E tests, since they were only being used for mocking and both `SetupAdsAccountPage` and `SetUpAccountsPage` extend the same mocking library.

joemcgill

This is looking much better to me, and i've been able to test several of the different connected states. I think we can improve some of the E2E test structure some more as the rest of the component gets filled out.

@eason9487 can you give this another look?

eason9487

I didn't view the code changes related to e2e testing in detail since they were continuously pushed commits while I was reviewing.

Due to #2618 (comment) and the following UI errors in one of the e2e test cases, I guess the e2e testing probably still needs to be adjusted accordingly, so I would like to submit the current feedback for this round.

js/src/setup-mc/setup-stepper/setup-accounts/index.js

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

js/src/hooks/useGoogleMCAccountReady.js

js/src/hooks/useGoogleMCAccount.js

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

js/src/hooks/useAutoCreateAdsMCAccounts.test.js

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

joemcgill · 2024-10-24T19:57:19Z

js/src/hooks/useGoogleAdsAccountReady.js

-	);
+		[ '', 'billing', 'link_merchant' ].includes( step );
+
+	return isReady !== null ? isReady : null;


The ternary here seems redundant because it means:

If isReady is !== null, return isReady
If isReady is === null, return null

This can be simplified to:

Suggested change

return isReady !== null ? isReady : null;

return isReady;

However, it seems this all be simplified to a boolean return value by wrapping the expression in parenthesis like this (and updating the docblock):

const isReady = ( hasGoogleAdsConnection && hasAccess && [ '', 'billing', 'link_merchant' ].includes( step ) );

@ankitrox I see you updated this slightly in 7f20af7 but still think the suggestions here would be better, unless there is some reason to retain the null return value?

@joemcgill This has been addressed here:

google-listings-and-ads/js/src/hooks/useGoogleAdsAccountReady.js

Lines 24 to 26 in 9137ef6

if ( ! adsAccountResolved || ! adsAccountStatusResolved ) {

return null;

}

Also:

google-listings-and-ads/js/src/hooks/useGoogleAdsAccountReady.js

Lines 28 to 32 in 9137ef6

return (

hasGoogleAdsConnection &&

hasAccess &&

[ '', 'billing', 'link_merchant' ].includes( step )

);

joemcgill · 2024-10-24T19:59:33Z

js/src/hooks/useGoogleMCAccount.js

@@ -60,6 +64,12 @@ const useGoogleMCAccount = () => {
 				GOOGLE_MC_ACCOUNT_STATUS.INCOMPLETE,
 			].includes( acc?.status );

+			const isReady =
+				hasGoogleMCConnection &&


This can be omitted since you are checking for the same account statuses below

Suggested change

hasGoogleMCConnection &&

Upvote this.

It has been removed:

google-listings-and-ads/js/src/hooks/useGoogleMCAccount.js

Lines 67 to 70 in 9137ef6

const isReady =

acc?.status === GOOGLE_MC_ACCOUNT_STATUS.CONNECTED ||

( acc?.status === GOOGLE_MC_ACCOUNT_STATUS.INCOMPLETE &&

acc?.step === 'link_ads' );

joemcgill · 2024-10-24T20:11:51Z

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

+				await setUpAccountsPage.fulfillAdsAccountStatus( {
+					has_access: true,
+					invite_link: '',
+					step: 'link_merchant',


This is very similar to the response from mockAdsStatusClaimed() except for the step that is returned. This matches what the API returns from the ads/account-status endpoint:

However, when the the Ads account is in this state, the ads/connections response would look something like this:

Making these two responses consistent would make for a more robust test, I think.

joemcgill · 2024-10-24T20:42:51Z

I've left a bit of feedback, but would appreciate if @eason9487 could give the latest iteration another look.

js/src/hooks/useGoogleAdsAccountReady.js

eason9487 · 2024-10-25T03:59:30Z

js/src/hooks/useGoogleMCAccount.js

@@ -60,6 +64,12 @@ const useGoogleMCAccount = () => {
 				GOOGLE_MC_ACCOUNT_STATUS.INCOMPLETE,
 			].includes( acc?.status );

+			const isReady =
+				hasGoogleMCConnection &&


Upvote this.

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

tests/e2e/utils/mock-requests.js

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

tests/e2e/utils/mock-requests.js

joemcgill · 2024-10-25T15:08:56Z

tests/e2e/utils/mock-requests.js

@@ -11,31 +11,50 @@ export default class MockRequests {
 		this.page = page;
 	}

+	fulfillTimes( times ) {


This needs a docblock

This has been updated.

joemcgill · 2024-10-25T15:14:48Z

tests/e2e/utils/mock-requests.js

@@ -129,7 +148,6 @@ export default class MockRequests {
 	 * Fulfill the MC connection request.
 	 *
 	 * @param {Object} payload
-	 * @return {Promise<void>}


I don't think this return docs should be removed here or for fulfillAdsConnection

Reverted this one.

joemcgill · 2024-10-25T15:14:58Z

tests/e2e/utils/mock-requests.js

@@ -189,7 +207,6 @@ export default class MockRequests {
 	 * Fulfill the Ads Connection request.
 	 *
 	 * @param {Object} payload
-	 * @return {Promise<void>}


Keep (see above)

Reverted this one.

ankitrox · 2024-10-25T15:44:52Z

Thanks for your valuable suggestion on E2E @eason9487. I have addressed these changes and left response to your comments. Assigning this to you for another pass.

eason9487

Thanks for the work. LGTM.

Left a few comments and I believe it won't need another round of code reviews. I'll be approving in advance.

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

tests/e2e/utils/mock-requests.js

ankitrox · 2024-10-28T06:37:02Z

Thanks for your review and valuable suggestions @eason9487 .

I've made the changes and merging the PR.

Initial structure.

1535369

ankitrox changed the base branch from develop to feature/2458-streamline-onboarding September 18, 2024 15:27

ankitrox changed the base branch from feature/2458-streamline-onboarding to feature/2458-consolidate-google-account-cards September 18, 2024 15:29

ankitrox changed the base branch from feature/2458-consolidate-google-account-cards to feature/2590-consolidate-google-account-cards September 18, 2024 15:40

ankitrox added 14 commits September 19, 2024 16:20

Use hooks to determine existing accounts.

0d7965c

Fix lint.

9e9b662

Fix: CreateAccounts component.

c5023b6

Multiple API calls.

Remove redundant props.

d41d825

Remove redundant code.

41e6d04

Introduce hook.

6917337

Fix: Flicker in component due to resolution state.

d1b4c77

Update account creation checks.

b997713

Update name for resolved checks property.

d293f15

E2E tests.

9ef6c13

Move hook to hooks directory.

767a906

Add JS tests for useCreateAccounts hook.

2de300a

Add indicator style.

8239b47

Fix css lint error.

2e16bf4

asvinb requested changes Sep 26, 2024

View reviewed changes

asvinb reviewed Sep 26, 2024

View reviewed changes

js/src/hooks/useCreateAccounts.js Outdated Show resolved Hide resolved

asvinb requested changes Sep 26, 2024

View reviewed changes

js/src/hooks/useCreateAccounts.js Outdated Show resolved Hide resolved

js/src/hooks/useCreateAccounts.js Outdated Show resolved Hide resolved

Refactor hook to handle either of the account creation.

32b47d5

asvinb requested changes Sep 27, 2024

View reviewed changes

CR feedback - round 2.

f55fd1d

joemcgill changed the base branch from feature/2590-consolidate-google-account-cards to feature/2509-consolidate-google-account-cards September 30, 2024 19:34

asvinb requested changes Oct 1, 2024

View reviewed changes

Add CR feedback changes.

95fe0cb

joemcgill requested changes Oct 2, 2024

View reviewed changes

joemcgill reviewed Oct 23, 2024

View reviewed changes

js/src/components/google-combo-account-card/indicator.js Outdated Show resolved Hide resolved

ankitrox added 2 commits October 23, 2024 23:40

Add useGoogleMCAccountReady hook.

7a9854b

Update account details rendering logic.

2859cf5

joemcgill reviewed Oct 23, 2024

View reviewed changes

js/src/hooks/useGoogleMCAccountReady.js Outdated Show resolved Hide resolved

js/src/setup-mc/setup-stepper/setup-accounts/index.js Outdated Show resolved Hide resolved

joemcgill and others added 3 commits October 23, 2024 14:34

Tweak useGoogleMCAccountReady hook

7a24be5

Remove unused constant

e62562f

Clean up E2E tests for account creation

bd1f3e3

This removes the unnecessary `SetupAdsAccountPage` from the E2E tests, since they were only being used for mocking and both `SetupAdsAccountPage` and `SetUpAccountsPage` extend the same mocking library.

joemcgill approved these changes Oct 23, 2024

View reviewed changes

ankitrox added 2 commits October 24, 2024 12:02

Fix: E2E test.

56a2e15

E2E tests changs for new hook.

599c2d2

eason9487 requested changes Oct 24, 2024

View reviewed changes

ankitrox added 4 commits October 24, 2024 20:14

CR feedback.

f3f63fe

Remove redundant code.

72434e2

Fix: JS lint.

e00c6e3

Fix: condition in hook.

7f20af7

joemcgill requested changes Oct 24, 2024

View reviewed changes

joemcgill requested a review from eason9487 October 24, 2024 20:43

eason9487 reviewed Oct 25, 2024

View reviewed changes

ankitrox added 3 commits October 25, 2024 19:10

E2E improvements.

d363edc

Use E2E mockAdsStatusClaimed function

4b8badb

Use mockAdsAccountConnected utility.

9137ef6

joemcgill reviewed Oct 25, 2024

View reviewed changes

Update docblocks.

8d381c4

eason9487 approved these changes Oct 28, 2024

View reviewed changes

tests/e2e/specs/setup-mc/step-1-accounts.test.js Outdated Show resolved Hide resolved

tests/e2e/utils/mock-requests.js Outdated Show resolved Hide resolved

tests/e2e/utils/mock-requests.js Outdated Show resolved Hide resolved

Update JSDoc.

8ab1be0

ankitrox merged commit 7ceffde into feature/2509-consolidate-google-account-cards Oct 28, 2024
7 of 8 checks passed

ankitrox deleted the feature/2567-kickoff-mc-ads-account-creation branch October 28, 2024 06:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kickoff MC and Ads account creation #2618

Kickoff MC and Ads account creation #2618

ankitrox commented Sep 18, 2024 •

edited by eason9487

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

asvinb left a comment

joemcgill commented Sep 30, 2024

asvinb left a comment

joemcgill left a comment

joemcgill left a comment

joemcgill left a comment

eason9487 left a comment •

edited

Loading

joemcgill Oct 24, 2024

joemcgill Oct 24, 2024

ankitrox Oct 25, 2024

ankitrox Oct 25, 2024

joemcgill Oct 24, 2024

eason9487 Oct 25, 2024

ankitrox Oct 25, 2024

joemcgill Oct 24, 2024

joemcgill commented Oct 24, 2024

eason9487 Oct 25, 2024

joemcgill Oct 25, 2024

ankitrox Oct 25, 2024

joemcgill Oct 25, 2024

ankitrox Oct 25, 2024

joemcgill Oct 25, 2024

ankitrox Oct 25, 2024

ankitrox commented Oct 25, 2024

eason9487 left a comment

ankitrox commented Oct 28, 2024

	if ( ! adsAccountResolved \|\| ! adsAccountStatusResolved ) {
	return null;
	}

	return (
	hasGoogleAdsConnection &&
	hasAccess &&
	[ '', 'billing', 'link_merchant' ].includes( step )
	);

	const isReady =
	acc?.status === GOOGLE_MC_ACCOUNT_STATUS.CONNECTED \|\|
	( acc?.status === GOOGLE_MC_ACCOUNT_STATUS.INCOMPLETE &&
	acc?.step === 'link_ads' );

Kickoff MC and Ads account creation #2618

Kickoff MC and Ads account creation #2618

Conversation

ankitrox commented Sep 18, 2024 • edited by eason9487 Loading

Changes proposed in this Pull Request:

Screenshots:

Detailed test instructions

Use custom plugin to test with existing account.

Additional details:

Changelog entry

codecov bot commented Sep 18, 2024 • edited Loading

Codecov Report

asvinb left a comment

Choose a reason for hiding this comment

joemcgill commented Sep 30, 2024

asvinb left a comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

joemcgill left a comment

Choose a reason for hiding this comment

eason9487 left a comment • edited Loading

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

joemcgill commented Oct 24, 2024

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

ankitrox commented Oct 25, 2024

eason9487 left a comment

Choose a reason for hiding this comment

ankitrox commented Oct 28, 2024

ankitrox commented Sep 18, 2024 •

edited by eason9487

Loading

codecov bot commented Sep 18, 2024 •

edited

Loading

eason9487 left a comment •

edited

Loading