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

Remove the accounts connection page from the Ads Setup flow #2534

Open
2 tasks
Tracked by #2459
joemcgill opened this issue Aug 16, 2024 · 9 comments · Fixed by #2595
Open
2 tasks
Tracked by #2459

Remove the accounts connection page from the Ads Setup flow #2534

joemcgill opened this issue Aug 16, 2024 · 9 comments · Fixed by #2595

Comments

@joemcgill
Copy link
Collaborator

joemcgill commented Aug 16, 2024

Part of #2460

When a merchant is setting up a new ads campaign from the plugin dashboard, they enter the "Set up paid campaign" flow, which includes individual steps for connecting a Google Ads account, configuring a campaign, and completing their billing setup.

image

Now that connecting an Ads account is a requirement of the initial onboarding process, we will remove the first step of this flow entirely, as it's no longer necessary. We'll still need to allow for an Ads account to be connected in this flow if one was disconnected from the settings screen (see this comment), but that flow will be consolidated into step 2 as a follow up to #2535.

Acceptance Criteria

  • The "Setup your accounts" step is removed from the "Set up paid campaign" flow.
  • The flow can still be completed to successfully to add a campaign if Ads is connected.

Out of scope: addressing the use case where an Ads account was disconnected after onboarding.

Implementation Brief

The PR should get merged into the feature/2460-simplify-paid-ads-setup branch.

The component that defines the Set up paid campaign flow is the AdsStepper located at /js/src/setup-ads/ads-stepper/index.js. The first object in the array passed to the steps prop can be removed and any unused code cleaned up. The keys associated with each remaining step objects and the handleCreateCampaignContinue() function will need to be updated.

The SetupAccounts component defined in js/src/setup-ads/ads-stepper/setup-accounts/index.js can be deleted, along with any dependencies that are no longer in use after it has been removed.

Test Coverage

  • The jest tests in js/src/setup-ads/ads-stepper/index.test.js should not be updated in this PR, and can be updated as a separate follow up task when Remove the billing setup page from the Ads setup flow #2536 is completed.
  • The E2E tests in tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js cover this flow, but should be refactored since they focus on setting up a Google Ads account.
    • Rename the top level test from "Set up Ads account" to "Set up paid campaign".
    • All the tests related to the accounts setup can be removed (ref).
    • All unused utilities after these changes should be removed.

Definition Questions

Is there a way to confirm that the Ad setup was successful without actually creating a paid ad, since most billing is being spoofed during development?

As long as the Ads account is not suspended, the campaign should be created if billing is approved (see these instructions for working locally without billing).

@joemcgill
Copy link
Collaborator Author

joemcgill commented Aug 22, 2024

@eason9487 could you give this a review and also provide any guidance you have about how to test this without setting up billing? Also, I'm curious what the best set of updates are for the Jest tests in js/src/setup-ads/ads-stepper/index.test.js. It seems these are mostly to test the behavior of the stepper and not the specific steps. Given the simplification described in #2460, is this test still necessary? If so, what assertions would be helpful?

@eason9487
Copy link
Member

Is there a way to confirm that the Ad setup was successful without actually creating a paid ad, since most billing is being spoofed during development?

Usually it doesn't need to bypass the creation of paid ads in development, but instead bypasses the billing settings. Because the paid ads will never go live or be charged since the billing isn't fully complete.

[...] how to test this without setting up billing?

This hack should be able to skip the billing setup.

Also, I'm curious what the best set of updates are for the Jest tests in js/src/setup-ads/ads-stepper/index.test.js. It seems these are mostly to test the behavior of the stepper and not the specific steps.

In the relatively early days of development, we wrote almost no jest tests. It's only in the last year or two that we've had more time, but still haven't had the resources to make up for better coverage.

Given the simplification described in #2460, is this test still necessary? If so, what assertions would be helpful?

Writing tests is not yet mandatory, but it would be great to at least write tests for the modified scope when possible.

@eason9487
Copy link
Member

I'm a little suspicious that the accounts connection step can be removed since the Google Ads account can be disconnected individually on the Settings page. I couldn't figure out how users can reconnect once the step is removed.

image

@joemcgill
Copy link
Collaborator Author

I'm a little suspicious that the accounts connection step can be removed since the Google Ads account can be disconnected individually on the Settings page. I couldn't figure out how users can reconnect once the step is removed.

That's a good point, @eason9487. I'm not sure that someone disconnecting from the settings page had been considered. I think we have a few options. We could optionally show the account connection page only when an ads account isn't connected. We could also update the Google Ads card on the settings screen so that a merchant would be able to reconnect their account from there. @fblascogarma what do you think would be best?

@fblascogarma
Copy link
Collaborator

Hi @joemcgill and @eason9487 , we're talking about the use case: merchant disconnects Google Ads accounts in Settings page (post onboarding), right?

In that case, onboarding has finished, so it doesn't impact the onboarding flow UX. However, merchants should be able to connect/create a Google Ads account in the Setting page if for some reason they disconnected it before.

Let me know if you have follow up questions.

@fblascogarma
Copy link
Collaborator

If you're talking about the case: merchants want to create ads in post onboarding, they should've already done Ads Account connection. If for some reason they disconnected it, once they start the ads creation they should be prompted to create/link Ads account in order to create ads.

@joemcgill
Copy link
Collaborator Author

If you're talking about the case: merchants want to create ads in post onboarding, they should've already done Ads Account connection. If for some reason they disconnected it, once they start the ads creation they should be prompted to create/link Ads account in order to create ads.

Allowing an Ads account to be connected if it was disconnected from the setting screen is exactly the scenario we were talking about. After additional discussion with @fblascogarma earlier today, we will remove this first step and add the option to connect an Ads account when disconnected in a follow-up issue. Currently, the idea is that we will add an Ads connection card to the new consolidated campaign creation page (#2535) when necessary. The campaign creation step of the onboarding previously included this UI, but it was removed for #2215.

@joemcgill
Copy link
Collaborator Author

Per this comment, we will optionally show this first accounts connection step if for some reason the Ads account has been disconnected prior to creating a new campaign. We're determining whether this will be handled in the current PR, or if we'll do so as a follow-up task.

@kt-12
Copy link
Collaborator

kt-12 commented Sep 16, 2024

@joemcgill This is stuck in small flow issue. Once we connect to an account, page sets the account and reloads the page. Reloading leads to the second flow with just 2 steps as we have the account now.

Continue flow or anything related to continue is not reached.
One way is we can remove continue but it will be lead to a UX difference, might be beat the purpose of having continue.

Image

@kt-12 kt-12 assigned joemcgill and unassigned kt-12 Sep 16, 2024
@joemcgill joemcgill assigned joemcgill and kt-12 and unassigned eason9487 Oct 10, 2024
@kt-12 kt-12 assigned eason9487 and unassigned joemcgill and kt-12 Oct 10, 2024
@eason9487 eason9487 assigned kt-12 and unassigned eason9487 Oct 11, 2024
@kt-12 kt-12 assigned ankitguptaindia and unassigned kt-12 Oct 14, 2024
@kt-12 kt-12 assigned joemcgill and unassigned ankitguptaindia and kt-12 Oct 17, 2024
@joemcgill joemcgill assigned eason9487 and unassigned joemcgill Oct 17, 2024
@eason9487 eason9487 assigned kt-12 and unassigned eason9487 Oct 18, 2024
@kt-12 kt-12 assigned eason9487 and unassigned kt-12 Oct 18, 2024
@eason9487 eason9487 assigned kt-12 and unassigned eason9487 Oct 18, 2024
@kt-12 kt-12 assigned eason9487 and unassigned kt-12 Oct 21, 2024
@eason9487 eason9487 removed their assignment Oct 22, 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

Successfully merging a pull request may close this issue.

6 participants