woocommerce / google-listings-and-ads

Sync your store with Google to list products for free, run paid ads, and track performance straight from your store dashboard.
https://woo.com/products/google-listings-and-ads/
GNU General Public License v3.0
47 stars 21 forks source link

Onboarding: Automatically kick off MC & Ads accounts creation #2567

Open joemcgill opened 3 months ago

joemcgill commented 3 months ago

As part of #2509, when a merchant connects a Google account that doesn't already have associated Google Merchant Center or Google Ads accounts, we'll streamline the process by creating the accounts for them and then prompt them to claim them.

Image

Account creation copy:

You don’t have Merchant Center nor Google Ads accounts, so we’re creating them for you.

Merchant Center is required to sync products so they show on Google. Google Ads is required to set up conversion measurement for your store.

Once the accounts are created and accepted, the Google card will be shown as connected, and all three accounts will be displayed on the card as shown in the following screenshot.

Image

[!NOTE] Handling the Ads account acceptance from this flow will be handled in a follow-up issue

Acceptance Criteria

Implementation Brief

[!NOTE] This work should be branched from feature/2566-google-combo-card (if not already merged) and a PR should be created against the feature/2590-consolidate-google-account-cards branch.

The GoogleAccountCard is the wrapper for all the connection states currently. Once the Google Account is connected, it renders the ConnectedGoogleAccountCard component. The GoogleAccountCard component will need to be enhanced so it has a prop to optionally handle the account creation for Ads and MC accounts, since it is also used in the ReconnectGoogleAccount component here. When that prop is true it will render a new ConnectedGoogleComboCard component, since the ConnectedGoogleAccountCard component is also used as a standalone component on the settings screen and also on the first page of the current ads setup flow (though, this may get removed in #2534).

The new ConnectedGoogleComboCard component will need to use the useExistingGoogleMCAccounts() and useExistingGoogleAdsAccounts() hooks to see if there are no existing accounts and kick off account creation. If so, the useCreateMCAccount() and useUpsertAdsAccount() hooks to create both accounts. Initially, we're going to ignore the need to reclaim and MC account or accept an Ads account, so the UI should be able to be updated as soon as we have a googleMCAccount.id and googleAdsAccount.it, respectively.

Test Coverage

Definition Questions

  1. ~An alternative to adding a prop to the GoogleAccountCard to optionally display this combined flow would be to fork it to a new GoogleComboAccountsCard component that duplicates much of the logic for the initial connection of the Google account and showing the RequestFullAccessGoogleAccountCard component. Is there a preference?~
    • This will be handled in #2566.
  2. There's an opportunity to streamline a lot of the REST API calls that are currently required to create these accounts so that we can get all of the status we need about the current state of accounts (e.g., does the MC account need to be reclaimed, does the Ads account need to be accepted, is billing set up, etc). Should we pursue prioritizing those changes prior to getting too far into these UI changes?

Testing Instructions

While testing this issue, please note that it assumes following points.

A. That site is not already claimed by any other Google Merchant Center account, so that there won't be any 403 error. As this scenario will be handled separately in #2597 B. The Google account we are trying to connect does not have existing MC and Ads account. A custom plugin code provided in PR description can be used to emulate this behaviour for existing accounts.

joemcgill commented 3 months ago

@mikkamp and @eason9487 while several details are still being worked out about all of the different states that will need to be shown in the Consolidated Google Accounts card, I think we can do this to start to get the initial structure in place for the simplest use case. I'd appreciate your feedback on this approach and specifically the questions listed under "Definition Questions" above. If you need more context, #2509 outlines the overarching goal of these changes.

mikkamp commented 2 months ago

There's an opportunity to streamline a lot of the REST API calls that are currently required to create these accounts so that we can get all of the status we need about the current state of accounts (e.g., does the MC account need to be reclaimed, does the Ads account need to be accepted, is billing set up, etc). Should we pursue prioritizing those changes prior to getting too far into these UI changes?

I would suggest to create those in parallel to the existing endpoints, and then clean things up once the UI has fully switched over.

I find it a little hard to suggest what the technical implementation should look like as this issue seems to describe the happy path going from "no accounts found" to "accounts successfully created/claimed". There are a lot of in between steps / states that are currently being handled for each account, some of which require interactions.

Are the account creations going to be fired off in parallel, with possible next steps on each account showing? Or is the account creation going to be handled sequentially, much like is done now.

eason9487 commented 2 months ago

Regard of Implementation Brief

Once the Google Account is connected, it renders the ConnectedGoogleAccountCard component. The GoogleAccountCard component will need to be enhanced so it has a prop to optionally handle the account creation for Ads and MC accounts, [...]

Since GoogleAccountCard already contains a lot of processing logic, it is not recommended to incorporate GoogleComboAccountsCard. The approach mentioned in the first question of the Definition Questions should be more appropriate. The case of duplicate logic may not be too significant, and it should be possible to prioritize the adjustment of components by reusing them.

Initially, we're going to ignore the need to reclaim and MC account or accept an Ads account, so the UI should be able to be updated as soon as we have a googleMCAccount.id and googleAdsAccount.it, respectively.

If there is no particular reason, it's recommended that the Google Merchant Center account (re)claiming and Google Ads account acceptance be included in the scope of this ticket. Because the definition of "connected completion" for these accounts also include completion of these processes, breaking them down into other tickets might require writing additional temporary logic and tests that would be removed by another PR. Including them will also allow for early clarification of every detail that needs to be addressed.

Regard of Definition Questions

2. There's an opportunity to streamline a lot of the REST API calls that are currently required to create these accounts so that we can get all of the status we need about the current state of accounts (e.g., does the MC account need to be reclaimed, does the Ads account need to be accepted, is billing set up, etc). Should we pursue prioritizing those changes prior to getting too far into these UI changes?

My intuition is that this may not be necessary. This could be considered after all the main implementation of #2509 has been completed. There is a lot of branching logic, detail states, interrupt resuming, and error handling in the whole process. The possibility of streamlining it is unclear at this point, and it probably won't make the frontend UI state management any easier.

Other suggestions

It may be difficult to have a more precise discussion before defining how the following features are going to be presented in terms of UI and flow.

joemcgill commented 2 months ago

Thanks for the detailed feedback, @eason9487. We'll plan to implement this new flow via a separate GoogleComboAccountsCard and let the current GoogleAccountCard component only be responsible for a connection to a Google account.

It may be difficult to have a more precise discussion before defining how the following features are going to be presented in terms of UI and flow.

Totally agree with you. The designs that @michaeleleder are working on in this Figma are getting closer to being finalized, at which point the remainder of the implementation plan can be defined.

One thing that I can see wasn't clear in my initial write up here is that we're planning on breaking this issue into several sub-tasks so we can make all of the updates needed in smaller, focused PRs. So, what I meant by "initially, we're going to ignore the need to reclaim and MC account or accept an Ads account" is that we may ignore these states in the first PRs, while we're getting the basic architecture of the components and UI set up, but then we'll add handling for those states in follow-up PRs as this feature is being implemented. I mainly wanted to set the expectation early that initial sub-tasks and their PRs related to this work will not fulfill all of the requirements of the current flow, but at the end of this feature, all should be accounted for.

joemcgill commented 2 months ago

I've updated #2566 so that it accounts for creating the new GoogleComboAccountsCard as suggested. This work is blocked until after the base component has been created.

joemcgill commented 2 months ago

@eason9487 I've updated this issue now that the design flows are starting to get close to completion and have created #2582 to handle the Google Ads claim flow. The reason I'd like to split this into separate tasks is that there are many additional scenarios that need to be handled by this new GoogleComboAccountsCard, which will be blocked until the initial connection flow is set up.

Since the Claim Ads Account flow has several details that need to be worked through, separating that work to a separate task will allow for work on additional scenarios to be started in parallel.

ankitrox commented 1 month ago

@asvinb Assigning this to you for CR. There are few fixes for the issues reported by @ankitguptaindia during QA and some adjustments to E2E test cases.