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. #2595

Open
wants to merge 18 commits into
base: feature/2459-campaign-creation-flow
Choose a base branch
from

Conversation

kt-12
Copy link
Collaborator

@kt-12 kt-12 commented Sep 9, 2024

Changes proposed in this Pull Request:

Closes #2534.

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

Screenshots:

After removal of accounts setup page.
Screenshot 2024-09-09 at 09 11 26

Detailed test instructions:

  1. Delete gla_ads_setup_completed_at options.
  2. Go to dashboard, under Performance (Paid Campaigns) section click Add paid campaign
  3. You should not see the Setup accounts page as shown in the screenshot. Proceed with the other steps and see if you are able to create first paid campaign.

Additional details:

Changelog entry

Update - When the accounts have been connected before, skip their setup step during the Ads-onboarding.

@kt-12 kt-12 requested a review from joemcgill September 9, 2024 08:17
Copy link
Collaborator

@asvinb asvinb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kt-12 Can you please do the following:

  • Change the target branch for your PR to feature/2460-simplify-paid-ads-setup as per the IB.
  • Fix the linting errors.
  • Review the E2E tests. For e.g there's empty tests for Set up your accounts page in tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js

Copy link

codecov bot commented Sep 12, 2024

Codecov Report

Attention: Patch coverage is 57.14286% with 3 lines in your changes missing coverage. Please review.

Project coverage is 62.7%. Comparing base (315fce2) to head (c26a029).
Report is 114 commits behind head on develop.

Files with missing lines Patch % Lines
js/src/setup-ads/ads-stepper/index.js 57.1% 3 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##             develop   #2595      +/-   ##
============================================
- Coverage       65.0%   62.7%    -2.4%     
============================================
  Files            475     319     -156     
  Lines          17900    5080   -12820     
  Branches           0    1233    +1233     
============================================
- Hits           11640    3183    -8457     
+ Misses          6260    1724    -4536     
- Partials           0     173     +173     
Flag Coverage Δ
js-unit-tests 62.7% <57.1%> (?)
php-unit-tests ?

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

Files with missing lines Coverage Δ
js/src/setup-ads/ads-stepper/index.js 86.4% <57.1%> (ø)

... and 793 files with indirect coverage changes

js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
@kt-12 kt-12 requested a review from asvinb September 23, 2024 14:18
Copy link
Collaborator

@asvinb asvinb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes LGTM @kt-12

@ankitguptaindia
Copy link
Member

QA/Test Report-

Testing Environment -

  • WordPress: 6.6.2
  • Theme active on store: Twenty Twenty-Four Version: 1.2
  • WooCommerce - Version 9.3.2
  • PHP: 8.3
  • Web Server: Nginx
  • Browser: Chrome - Version 128
  • OS: macOS Sonoma 14.6.1

Test Results - Tested the functionally of the new changes. Working as described in scope. ✅

Functional Demo / Screencast -

GLNA.mp4

Next Step- Ready to Code Review(Woo)

Copy link
Member

@eason9487 eason9487 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition to the comments left, there are two other issues:

js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
js/src/setup-ads/ads-stepper/index.js Outdated Show resolved Hide resolved
@eason9487 eason9487 added changelog: none Skip changelog entry for this PR changelog: update Big changes to something that wasn't broken. and removed changelog: none Skip changelog entry for this PR labels Sep 25, 2024
@kt-12
Copy link
Collaborator Author

kt-12 commented Sep 30, 2024

@eason9487 I have addressed your review comment. I have also fixed E2E to reflect the current flow.

@joemcgill joemcgill changed the base branch from develop to feature/2460-simplify-paid-ads-setup September 30, 2024 18:03
@joemcgill joemcgill deleted the branch feature/2459-campaign-creation-flow September 30, 2024 19:27
@joemcgill joemcgill closed this Sep 30, 2024
@joemcgill joemcgill reopened this Sep 30, 2024
@joemcgill joemcgill changed the base branch from feature/2460-simplify-paid-ads-setup to feature/2459-campaign-creation-flow September 30, 2024 20:03
@joemcgill
Copy link
Collaborator

Sorry for the branch name confusion. We've consolidated feature/2460-simplify-paid-ads-setup into feature/2459-campaign-creation-flow. This should be correct now.

Copy link
Member

@ankitguptaindia ankitguptaindia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good and are working as expected. The accounts connection page has been removed from the Paid Ads setup screen. Both the paid ads campaign creation and the edit paid campaign functions are working fine. ✅

Comment on lines 43 to 45
if ( initHasAdsConnectionRef.current === null ) {
initHasAdsConnectionRef.current = hasGoogleAdsConnection;
}
Copy link
Member

@eason9487 eason9487 Oct 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just noticed this check is not enough to skip the first step. Without the following checks, the Google Ads account claiming and connection with Google Merchant Center account could be skipped.

// Ads is ready when we have a connection and verified and verified access.
// Billing is not required, and the 'link_merchant' step will be resolved
// when the MC the account is connected.
const isGoogleAdsReady =
hasGoogleAdsConnection &&
hasAccess &&
[ '', 'billing', 'link_merchant' ].includes( step );

Steps to reproduce:

Kapture.2024-10-04.at.17.44.59.mp4

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. We hadn't considered the claim state here.

@joemcgill
Copy link
Collaborator

@eason9487 I've addressed both concerns and was able to validate that the step remains visible whenever the account needs to be claimed. E2E tests are still passing after applying your suggestion. Can you look again?

const { hasAccess } = useGoogleAdsAccountStatus();
const initHasAdsConnectionRef = useRef( null );

const isGoogleAdsReady = hasGoogleAdsConnection && hasAccess;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking hasAccess only may still skip creating the conversion action of Google Ads account and connection to Google Merchant Center account. The step in ads/account-status API response may be stalled in 'conversion_action' status.

1

Steps to reproduce:

  1. Select to create new Google Ads account
  2. Open the pop-up window for claiming account but don't proceed to claim
  3. Close the webpage of Ads-onboarding flow
  4. Claim account via the pop-up window
  5. Open new webpage to visit the Ads-onboarding flow again
  6. The flow starts from the paid campaign setup step

Comment on lines +43 to +45
if ( ! googleAdsAccount ) {
return null;
}
Copy link
Member

@eason9487 eason9487 Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

useGoogleAdsAccount and useGoogleAdsAccountStatus are concurrent asynchronous requests. Only checking googleAdsAccount can't guarantee that the required data have been obtained to get the correct value of initHasAdsConnectionRef.current.

When useGoogleAdsAccount responds first and a Google ads account has been already fully connected, three steps will flash for a moment.

Kapture.2024-10-07.at.17.43.42.mp4

Also, when a Google Ads account is claimed but not yet fully connected such as #2595 (comment), three steps will flash for a moment as well.

Kapture.2024-10-07.at.17.36.12.mp4

The issues can be simulated by adding delays to the API request of useGoogleAdsAccountStatus.

import { __unstableAwaitPromise } from '@wordpress/data-controls';

export function* getGoogleAdsAccountStatus() {
	// Add this at the position needed a delay
	yield __unstableAwaitPromise( new Promise( ( resolve ) => setTimeout( resolve, 3000 ) ) );

	yield fetchGoogleAdsAccountStatus();
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog: update Big changes to something that wasn't broken.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove the accounts connection page from the Ads Setup flow
5 participants