Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
5 tasks done
Tracked by #2509
joemcgill opened this issue Sep 9, 2024 · 8 comments
Open
5 tasks done
Tracked by #2509
Assignees

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Sep 9, 2024

Part of #2509

After connecting a Google account, we will provide the UI to allow the merchant to connenct to an existing Ads account.

image

Acceptance Criteria

  • When the Google account has 1 or more existing Ads accounts, show the Ads connection UI
  • If only one account exists, it should automatically be selected.
  • If multiple accouns exists, the user should be able to select which to connect.
  • The user can click the "connect" button to connect the selected Ads account.
  • If there is no MC account and one is being created, the card description should only reference MC (see designs)

Implementation Brief

  • Using the GoogleComboAccountCard created in Onboarding: Create New Google Combo Accounts Card #2566.
  • Check if the Google connected account has an Ads account connected.
    • This should check the hasGoogleAdsConnection prop returned by the useGoogleAdsAccount hook, but only after the Google account is connected, otherwise that property won't exist.
  • Create connect-ads/index.js in the same folder as GoogleComboAccountCard which will house the ConnectAds component.
  • It should follow the logic contained in js/src/components/google-ads-account-card/connect-ads/index.js to display the data (not the UI) and styled as per the designs.
    • Use the existing components, for e.g Section.Card.Body for the layout.
    • Rendering a select if there are multiple options or text if there's only one option is handled in Update AppSelectControl when only one account is available #2593.
    • Clicking the "Connect" button should follow the same logic as handleConnectClick in js/src/components/google-ads-account-card/connect-ads/index.js.
  • If there's no connected Google Ads account and the user has at least one Ad account, render ConnectAds within GoogleComboAccountCard.
    • To check if the user has at least an Ad account, use the useExistingGoogleAdsAccounts hook.
  • Additionally, if there is no Merchant Center account, GoogleComboAccountCard should automatically create one.
    • The logic for creating that account should be handled in Onboarding: Automatically kick off MC & Ads accounts creation #2567.
    • Update the description shown in GoogleComboAccountCard as per the designs indicating to the user that an account is being created for them.
    • The above should only happen when there is at least an Ad account but no MC account.
  • The implementation for the "Or, create a new Google Ads account" button is handled in Onboarding: Create new Ads account when there are existing accounts in Google Accounts Card #2603
  • Evaluate whether there are any race conditions when creating/connecting either MC or Ads accounts. In this case, one possible solution would be to disable the Ads connection section while the MC account is being created. This is highly unlikely but something to look for during implementation.

Test Coverage

  • E2E tests should be added to assert the main points mentioned in the AC.

Definition Questions

  • If there is only one Google Ads account, do we really need to prompt the user to connect it? Instead, can we automatically connect the single account? The user could always switch to a different Google Ads account later through Edit mode if needed.
  • When connecting a new MC account, a reclaim URL error may occur. In such cases, we need a new component similar to ReclaimUrlCard. However, we can't reuse the existing one (js/src/components/google-mc-account-card/reclaim-url-card/index.js) due to differences in the layout (e.g., logo, spacing, etc.).
  • If the creation of a new MC account is in progress, shall the Connect Ads section be disabled? One thing to consider is the UI when there's a new MC account creation in progress and the user clicks on the create a new Google ads account button.
@asvinb
Copy link
Collaborator

asvinb commented Sep 12, 2024

@joemcgill I've added the IB. Can you kindly review and let me know what you think please?

@joemcgill
Copy link
Collaborator Author

Thanks for adding the IB details, @asvinb. A few points of feedback, starting with your definition questions:

If there is only one Google Ads account, do we really need to prompt the user to connect it? Instead, can we automatically connect the single account? The user could always switch to a different Google Ads account later through Edit mode if needed.

Yes, while we could automatically connect the account, we wanted to give the user the opportunity to confirm that the existing account is the one they wanted to connect to this store. For example, they could have an existing account for some other purpose and would prefer to create a new one for this purpose.

When connecting a new MC account, a reclaim URL error may occur. In such cases, we need a new component similar to ReclaimUrlCard. However, we can't reuse the existing one (js/src/components/google-mc-account-card/reclaim-url-card/index.js) due to differences in the layout (e.g., logo, spacing, etc.).

Good observation. We probably need a new issue to handle this as a follow up to #2567, which hanldes this similarly to #2582.

If the creation of a new MC account is in progress, shall the Connect Ads section be disabled? One thing to consider is the UI when there's a new MC account creation in progress and the user clicks on the create a new Google ads account button.

I don't think we need to disable connecting an Ads account while the MC creation step in in process since currently these two accounts can be set up independently of each other.

Also, to clarify the AC, this issue should have no responsibility for creating or connecting an MC account and should only be responsible for providing the UI and front end logic required to connect an existing Ads account. The only related change that this issue should take care of is updating the text in the top of the card during creation so it no longer reads, "you don't already have an Ads account...", since that would not be true when this UI is being shown.

@joemcgill
Copy link
Collaborator Author

Also, wanted to point out for clarity that the requirements creating a new Ads account from the new connect component will be handled by #2603.

@asvinb
Copy link
Collaborator

asvinb commented Sep 16, 2024

@joemcgill
Thanks for your feedback. For the second point, about the reclaim URL UI, I feel we can reuse some of the implementation from #2604
For updating text in the top of the card during creation, I feel this should be done in #2567 where the automatic account creation kicks in. What do you think?

@joemcgill
Copy link
Collaborator Author

Thanks for your feedback. For the second point, about the reclaim URL UI, I feel we can reuse some of the implementation from #2604

Agreed. We don't necessarily need to include that in the scope of this ticket.

For updating text in the top of the card during creation, I feel this should be done in #2567 where the automatic account creation kicks in. What do you think?

I really want to keep that ticket focused on the initial account creation and not the two use cases where we are only creating one account, but we can create a separate follow-up issue to handle these cases if you think doing so here is too confusing. Mainly just want to make sure we remember to update that text.

Additionally I've made the following update:

This is done by checking googleAdsAccount.status === GOOGLE_ADS_ACCOUNT_STATUS.CONNECTED via the useGoogleAdsAccount hook.

This should check the hasGoogleAdsConnection prop returned by useGoogleAdsAccount() but only after the Google account is connected, otherwise that property won't exist.

Additionally, if there is no Merchant Center account, GoogleComboAccountCard should automatically create one

Most of this will already be handled in #2567, so we should only have to update the logic so that the text changes to indicate that we are automatically creating an MC account and not an MC and Ads account.

@joemcgill
Copy link
Collaborator Author

@eason9487 can you review the approach and flag any concerns you have prior to us starting this work? For this and other tasks that are a subtask of #2509, this only addresses a subset of requirements in order to keep the scope of each PR focused. It may be helpful to review the whole epic again if the way we've broken this up is unclear.

@joemcgill joemcgill added the needs feedback The issue/PR needs a response from any of the parties involved in the issue. label Sep 24, 2024
@eason9487
Copy link
Member

  • If the creation of a new MC account is in progress, shall the Connect Ads section be disabled? One thing to consider is the UI when there's a new MC account creation in progress and the user clicks on the create a new Google ads account button.

If there is a possible race condition on the DB/API data updates during linking Google Ads and Merchant accounts, it's suggested to disable the UI related to connecting or creating Google Ads account.

@asvinb
Copy link
Collaborator

asvinb commented Sep 26, 2024

@joemcgill @eason9487 IB has been added to look for any race conditions. This is unlikely but worth mentioning it in the IB.

@macka61 macka61 removed status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. needs feedback The issue/PR needs a response from any of the parties involved in the issue. labels Sep 26, 2024
@ankitrox ankitrox self-assigned this Oct 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants