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: Claim New Ads Accounts in the Google Combo Accounts Card #2582

Open
6 tasks done
Tracked by #2509
joemcgill opened this issue Sep 4, 2024 · 13 comments · Fixed by #2644
Open
6 tasks done
Tracked by #2509

Onboarding: Claim New Ads Accounts in the Google Combo Accounts Card #2582

joemcgill opened this issue Sep 4, 2024 · 13 comments · Fixed by #2644
Assignees

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Sep 4, 2024

Caution

This is blocked by #2567.

Part of #2509

As a follow-up to #2567, when a new Google Ads account is created, the account needs to be claimed before the connection process can be completed.

image

Clicking the "Claim account..." button will open the current pop-up flow for accepting a Google Ads account, and the card will show an updating state while the account is being accepted.

image

Once the Ads account has been accepted and the setup process has been completed, the following success state will be shown.

image

If we aren't able to confirm that the account was claimed when the user returns focus to the screen after entering the claim flow, we return to the original claim state shown in the first screenshot.

Designs: 🔗 Figma Onboarding

Acceptance Criteria

Note that the "Edit" link displayed in the screenshots is not in scope of this issue and will be handled separately.

  • When an Ads account is connected but not claimed, the "Claim your Google Ads" UI is shown
  • Clicking the "Claim Account..." button opens the pop-op flow to accept the invitation
  • Once focus returns to the onboarding flow, show the "Updating" UI and refetch the Google Ads account status from the API to see if the account has been claimed
  • Refreshing the page while the account is unclaimed, should still result
  • When the account is claimed from any source, set up conversions and show the success message
  • When the account is still unclaimed return the the original "Claim Account..." state

Implementation Brief

A new sub-component, ClaimAdsAccount will be created in the directory for the GoogleComboAccountsCard component to show the claim messaging and render the ClaimAccountButton button (can likely reuse the existing component from here). This component should support an isLoading prop to control whether the button is being shown or the "Updating..." UI.

The GoogleComboAccountsCard component will be responsible for checking whether the account needs to be claimed by checking the hasAccess value returned from useGoogleAdsAccountStatus() to determine whether this UI is visible. Additionally, once the account is claimed, the GoogleComboAccountsCard will need to call upsertAdsAccount to trigger the rest of the account setup process (reference). When checking the account status or updating the account, the GoogleComboAccountsCard can set isLoading:true.

Once the GoogleComboAccountsCard has confirmed that we have access to the account and the setup has been completed from useGoogleAdsAccountStatus(), i.e. hasAccess === true && step === "", the success notice should be shown instead.

Test Coverage

A new set of E2E test should be added to tests/e2e/specs/setup-mc/step-1-accounts.test.js confirming that the invitation flow is working per the AC.

Definition Questions

@joemcgill joemcgill self-assigned this Sep 4, 2024
@joemcgill joemcgill added the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Sep 4, 2024
@joemcgill
Copy link
Collaborator Author

@eason9487 or @asvinb I think this one is ready for review.

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

  1. image
  2. image
  3. image

In the first image, the Google Ads account status is created but not yet connected; In the second image, the status is being updated but not yet turned to connected.

The third image shows the UI state after clicking the Edit button, but I'm not sure if it will be able to show the UI state as expected after clicking that button in the state of the first two images.

@asvinb
Copy link
Collaborator

asvinb commented Sep 11, 2024

@eason9487 , @joemcgill Correct me if am wrong, but the "Edit" button will be displayed only when all the 3 accounts are connected 🤔
The brief looks good @joemcgill , except for one thing which is not clear in the designs. What happens to the "Claim Account in Google Ads" button once the user clicks on it? From the designs, it looks like there's the loading state which is rendered (Updating as label) which can be confusing when looking at the designs. We can keep the current implementation where we always show the claim button until the user has claimed his account.

@asvinb asvinb assigned joemcgill and unassigned asvinb Sep 11, 2024
@eason9487
Copy link
Member

Hi @asvinb

Correct me if am wrong, but the "Edit" button will be displayed only when all the 3 accounts are connected

It doesn't seem to have been designed this way from the current Figma file.

In the current implementation, each account card has a disconnect button once the connection is complete, and they don't need to wait for all three accounts to be connected before displaying it.

This makes sense because when a user just connects to a Google account and then wants to disconnect it, they shouldn't have to wait for all the other accounts to connect before they can do so.

@joemcgill
Copy link
Collaborator Author

@eason9487 and @asvinb, implementing the edit button will be handled in a separate issue since the requirement for that link are dependent on a few other pieces of UI.

The objective of this issue is only to handle part of the Ads connection flow that requires claiming a new Ads account so the connection can be completed. The result of this ticket will be to display the GoogleComboAccountCard in the state illustrated by the screenshot below, where all three accounts are connected and set up and the onboarding stepper can continue to step 2.

image

@joemcgill
Copy link
Collaborator Author

I've added more details about the edit link in #2605. The implementation brief is still needed, but hopefully there are enough details to explain the product vision.

@joemcgill
Copy link
Collaborator Author

With #2567 already in progress, this is nearly unblocked. @ankitrox had some alternate ideas about how we might implement this rather than using the upsertAdsAccount hook, which I'm happy for us to discuss. However, I think the general approach defined here still looks like a good starting point.

@eason9487 could you review this again with a focus only on the implementation of the claim accounts flow being added to this component and call out any additional concerns that we should account for before starting this work?

@eason9487
Copy link
Member

eason9487 commented Sep 25, 2024

image

As per the Compact and Semi-expanded states in #2605 and the images in this ticket, will the UI of claiming flow still be placed in the top card body section regardless of the state of the card?

@joemcgill
Copy link
Collaborator Author

@eason9487 good question, since the designs do not make that clear. For the MC accounts the reclaim URL flow can display in either the top card body OR the bottom card when expanded. It could make sense to do the same here, but that might make the top card logic more complex. Let me see if I can get feedback from @michaeleleder about this.

@joemcgill
Copy link
Collaborator Author

@eason9487 I've gotten confirmation from @fblascogarma that we will always display the claim component in the top card body, regardless. If the account is connected but not claimed, and the card is expanded, the unclaimed account will likely show in it's connected state, but the flow won't be able to be completed until after the account is claimed.

I don't think we need any changes to this issue to account for the expected behavior, but may need to review things again once #2603 is implemented. Do you have any other concerns with this issue before we mark it as ready for implementation?

@joemcgill
Copy link
Collaborator Author

Going to go ahead an move this into ToDo and we can discuss any concerns async.

@joemcgill joemcgill removed the needs feedback The issue/PR needs a response from any of the parties involved in the issue. label Oct 3, 2024
@joemcgill joemcgill removed the status: blocked The issue is blocked from progressing, waiting for another piece of work to be done. label Oct 3, 2024
@joemcgill
Copy link
Collaborator Author

This work should be based off the PR for #2567 but can be started now.

@eason9487 eason9487 assigned ankitrox and unassigned eason9487 Nov 7, 2024
@asvinb asvinb assigned joemcgill and unassigned ankitrox and ankitguptaindia Nov 7, 2024
@joemcgill joemcgill assigned eason9487 and unassigned joemcgill Nov 7, 2024
@eason9487 eason9487 assigned asvinb and unassigned eason9487 Nov 8, 2024
@asvinb asvinb assigned eason9487 and unassigned asvinb Nov 8, 2024
@eason9487 eason9487 assigned asvinb and unassigned eason9487 Nov 8, 2024
@asvinb asvinb assigned joemcgill and unassigned asvinb Nov 8, 2024
@joemcgill joemcgill assigned asvinb and unassigned joemcgill Nov 10, 2024
@eclarke1 eclarke1 assigned joemcgill and unassigned asvinb Nov 11, 2024
@joemcgill joemcgill assigned eason9487 and unassigned joemcgill Nov 11, 2024
@eason9487 eason9487 assigned asvinb and unassigned eason9487 Nov 12, 2024
@joemcgill joemcgill assigned eason9487 and unassigned asvinb Nov 12, 2024
@eason9487 eason9487 removed their assignment Nov 13, 2024
@joemcgill joemcgill assigned joemcgill and fblascogarma and unassigned joemcgill Nov 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment