-
Notifications
You must be signed in to change notification settings - Fork 21
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 billing setup page from the Ads setup flow #2577
base: feature/2459-campaign-creation-flow
Are you sure you want to change the base?
Remove the billing setup page from the Ads setup flow #2577
Conversation
@joemcgill Based on your comment on issue, I have not modified Jest tests here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kt-12 Thanks for the PR! Works well. I suggested a couple of improvements though. Can you kindly let me know what you think?
Also, let's update the tracking comments here:
google-listings-and-ads/js/src/setup-ads/ads-stepper/index.js
Lines 24 to 25 in 5b14c07
* @fires gla_setup_ads with `{ triggered_by: 'step1-continue-button' | 'step2-continue-button' , action: 'go-to-step2' | 'go-to-step3' }`. | |
* @fires gla_setup_ads with `{ triggered_by: 'stepper-step1-button' | 'stepper-step2-button', action: 'go-to-step1' | 'go-to-step2' }`. |
It should be:
* @fires gla_setup_ads with `{ triggered_by: 'step1-continue-button', action: 'go-to-step2' }`.
* @fires gla_setup_ads with `{ triggered_by: 'stepper-step1-button', action: 'go-to-step1'}`.
submitButtonText={ __( | ||
'Launch paid campaign', | ||
'google-listings-and-ads' | ||
) } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AdsCampaign
component is used in 2 flow. One setting up our first paid campaign flow (here). And another for the setting up other paid campaing.
Next is once the first campaign set up you see a button in the right hand side of dashboard- Add paid campaign
,
once you click that you see a different flow where AdsCampaign
is the first component in the flow.
submitButtonText
was introduced to make change to button text in a way it doesn't effect the other flow.
There was a problem hiding this 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 kindly check my comment about the default value for the submitButtonText
parameter please?
Done. I have addressed other open review comments too. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kt-12 I left some more comments. Can you check them out please?
Also why we have this error message for the PR?
Check failure on line 90 in js/src/setup-ads/ads-stepper/index.test.js
change text Co-authored-by: Asvin Balloo <[email protected]>
…ommerce/google-listings-and-ads into feature/2536-remove-billing
@kt-12 Can you please fix the linting and unit tests errors please? |
Unit test changes are not handled in this ticket. It is handled in a separate ticket 2579, which merges 2534, 2536 and 2535 |
Co-authored-by: Asvin Balloo <[email protected]>
@eason9487 This is ready for review please. |
Hi @kt-12 and @asvinb, I haven't looked into the code changes as the JS unit testing and e2e testing didn't pass.
Could you help with ensuring a PR passes all testing before passing its ticket to Woo CR stage? |
@joemcgill @asvinb I have updated e2e to support new flow. E2E is currently failing at form submit. While most of the flow worked fine clicking on Few observation:
I tried recconecting to jetpack again just before the click of button but it filed too. Curl extracted from E2E :-
|
…ommerce/google-listings-and-ads into feature/2536-remove-billing
Hi @eason9487, Sorry for the initial confusion. I have fixed e2e for this task and it did run without any failure (run). Jest changes will still be handled separately as we see some overlap within other sister tasks. I feel this is good for WooTeam review now, adding this back to Woo CR. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the work. The main concern is whether there is a better way to composite the continue button for the AdsCampaign
component rather than passing 5 props.
disabled={ | ||
! isValidForm || | ||
( trackingContext === 'setup-ads' && | ||
billingStatus?.status !== | ||
GOOGLE_ADS_BILLING_STATUS.APPROVED ) | ||
} | ||
loading={ isLoading } | ||
onClick={ onContinue } | ||
> | ||
{ __( 'Continue', 'google-listings-and-ads' ) } | ||
{ submitButtonText } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering this button now requires 5 props to render and its only internal dependency is on one of those conditions, its degree of customization makes it unsuitable to be part of this component. It's better to externalize it as a single prop such as continueButton
to composite this component.
<StepContentFooter>
{ continueButton }
</StepContentFooter>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eason9487 I believe this continue button is only temporarily going to need to handle this many conditions until #2535 is completed, to ensure that the current flow could still be tested properly. I'd like to avoid making any additional modifications to the AdsCampaign
component in this PR, which is really only meant to remove the billing step because it will be handled during step 3 once we've consolidated the form from the onboarding flow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-listings-and-ads/js/src/components/paid-ads/ads-campaign/ads-campaign.js
Lines 103 to 117 in 34c53d7
let continueButtonProps = { | |
text: __( 'Continue', 'google-listings-and-ads' ), | |
}; | |
if ( isOnboardingFlow ) { | |
continueButtonProps = { | |
'data-action': ACTION_COMPLETE, | |
text: __( 'Complete setup', 'google-listings-and-ads' ), | |
eventName: 'gla_onboarding_complete_with_paid_ads_button_click', | |
eventProps: { | |
budget: paidAds.amount, | |
audiences: countryCodes?.join( ',' ), | |
}, | |
}; | |
} |
google-listings-and-ads/js/src/components/paid-ads/ads-campaign/ads-campaign.js
Lines 176 to 182 in 34c53d7
<AppButton | |
isPrimary | |
disabled={ disabledComplete } | |
onClick={ handleCompleteClick } | |
loading={ completing === ACTION_COMPLETE } | |
{ ...continueButtonProps } | |
/> |
From the current revision of AdsCampaign
in #2623, I couldn't figure out how this button only temporarily needs to handle 5 props, because both PRs currently increase the complexity and conditional branching of this button to varying degrees.
All of these changes convinced me that both PRs should try to see if it's possible to move this button out of the AdsCampaign
component sooner rather than having to resolve more code conflicts and re-test all button scenarios when merging them with each other.
await setupBudgetPage.mockCampaignCreation( budget, {} ); | ||
|
||
await expect( | ||
page.getByText( | ||
'Great! You already have billing information saved for this' | ||
) | ||
).toBeVisible(); | ||
await setupBudgetPage.getLaunchPaidCampaignButton().click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wonder why the test was changed to not wait for the campaign creation request? (setupBudgetPage.mockCampaignCreationAndAdsSetupCompletion
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setupBudgetPage.mockCampaignCreationAndAdsSetupCompletion
was waiting indefinitely. We only needed the campaign to created to get the button clickable, hence the split.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see waiting indefinitely if it's used similarly to
google-listings-and-ads/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js
Lines 527 to 535 in 3d38d91
const campaignCreation = | |
setupBudgetPage.mockCampaignCreationAndAdsSetupCompletion( | |
'1', | |
[ 'US' ] | |
); | |
await page | |
.getByRole( 'button', { name: 'Launch paid campaign' } ) | |
.click(); | |
await campaignCreation; |
Without that check, the "It should show the campaign creation success message" test case can still pass after skipping the campaign creation by replacing the following line with return Promise.resolve()
:
return createAdsCampaign( amount, countryCodes ) |
This makes it not fulfill the purpose of the test case it describes.
I'm updating this PR to point to the new consolidated feature branch |
@eason9487 besides the changes requested for the continue button, I think this is ready for another look. |
/** | ||
* Extract budget recommendation value. | ||
* | ||
* @param {string} text | ||
* | ||
* @return {string} The budget recommendation value. | ||
*/ | ||
extractBudgetRecommendationValue( text ) { | ||
const match = text.match( /set a daily budget of (\d+)/ ); | ||
if ( match ) { | ||
return match[ 1 ]; | ||
} | ||
return ''; | ||
} | ||
|
||
/** | ||
* Register the responses when removing an ads audience. | ||
* | ||
* @return {Promise<import('@playwright/test').Response>} The response. | ||
*/ | ||
registerBudgetRecommendationResponse() { | ||
return this.page.waitForResponse( | ||
( response ) => | ||
response | ||
.url() | ||
.includes( '/gla/ads/campaigns/budget-recommendation' ) && | ||
response.status() === 200 | ||
); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like these two methods that had been removed from #2551 were added back when resolving code conflicts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed.
await setupBudgetPage.mockCampaignCreation( budget, {} ); | ||
|
||
await expect( | ||
page.getByText( | ||
'Great! You already have billing information saved for this' | ||
) | ||
).toBeVisible(); | ||
await setupBudgetPage.getLaunchPaidCampaignButton().click(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see waiting indefinitely if it's used similarly to
google-listings-and-ads/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js
Lines 527 to 535 in 3d38d91
const campaignCreation = | |
setupBudgetPage.mockCampaignCreationAndAdsSetupCompletion( | |
'1', | |
[ 'US' ] | |
); | |
await page | |
.getByRole( 'button', { name: 'Launch paid campaign' } ) | |
.click(); | |
await campaignCreation; |
Without that check, the "It should show the campaign creation success message" test case can still pass after skipping the campaign creation by replacing the following line with return Promise.resolve()
:
return createAdsCampaign( amount, countryCodes ) |
This makes it not fulfill the purpose of the test case it describes.
* Mock the campaign creation process. | ||
* | ||
* @param {string} budget The campaign budget. | ||
* @param {string} budget The campaign budget. | ||
* @param {Array} targetLocations The targeted locations. | ||
* @return {Promise<void>} | ||
*/ | ||
async mockCampaignCreationAndAdsSetupCompletion( budget, targetLocations ) { | ||
async mockCampaignCreation( budget, targetLocations ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Referring to #2577 (comment), although I don't think this method needs to be extracted, the name of this method doesn't match its implementation because it still includes the mocking of Ads setup completion.
google-listings-and-ads/tests/e2e/utils/pages/setup-ads/setup-budget.js
Lines 202 to 207 in fdd3aae
await this.fulfillRequest( | |
/\/wc\/gla\/ads\/setup\/complete\b/, | |
null, | |
200, | |
[ 'POST' ] | |
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the function. Test working with early function when await was moved to after the button click.
disabled={ | ||
! isValidForm || | ||
( trackingContext === 'setup-ads' && | ||
billingStatus?.status !== | ||
GOOGLE_ADS_BILLING_STATUS.APPROVED ) | ||
} | ||
loading={ isLoading } | ||
onClick={ onContinue } | ||
> | ||
{ __( 'Continue', 'google-listings-and-ads' ) } | ||
{ submitButtonText } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
google-listings-and-ads/js/src/components/paid-ads/ads-campaign/ads-campaign.js
Lines 103 to 117 in 34c53d7
let continueButtonProps = { | |
text: __( 'Continue', 'google-listings-and-ads' ), | |
}; | |
if ( isOnboardingFlow ) { | |
continueButtonProps = { | |
'data-action': ACTION_COMPLETE, | |
text: __( 'Complete setup', 'google-listings-and-ads' ), | |
eventName: 'gla_onboarding_complete_with_paid_ads_button_click', | |
eventProps: { | |
budget: paidAds.amount, | |
audiences: countryCodes?.join( ',' ), | |
}, | |
}; | |
} |
google-listings-and-ads/js/src/components/paid-ads/ads-campaign/ads-campaign.js
Lines 176 to 182 in 34c53d7
<AppButton | |
isPrimary | |
disabled={ disabledComplete } | |
onClick={ handleCompleteClick } | |
loading={ completing === ACTION_COMPLETE } | |
{ ...continueButtonProps } | |
/> |
From the current revision of AdsCampaign
in #2623, I couldn't figure out how this button only temporarily needs to handle 5 props, because both PRs currently increase the complexity and conditional branching of this button to varying degrees.
All of these changes convinced me that both PRs should try to see if it's possible to move this button out of the AdsCampaign
component sooner rather than having to resolve more code conflicts and re-test all button scenarios when merging them with each other.
Thank you, you were correct. awating after button is clicked did solve waiting indefinitely issue. I have also removed the newly introduced function. |
@eason9487 we have addressed your review comments and also moved continue button out of AdsCampaign. Moving it back to you for CR. |
Changes proposed in this Pull Request:
Closes #2536 .
Billing setup page should be removed and also
Launch Paid Campaign
should be possible from second step.Screenshots:
Detailed test instructions:
DELETE FROM `wp_options` WHERE `option_name` = 'gla_ads_setup_completed_at';
Add paid campaign
Set up your accounts
pageLaunch Paid Campaign
button which enables on addingDaily Average Cost
Additional details:
Changelog entry