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

Streamline campaign setup: Remove ads audience field from paid ads setup during onboarding #2551

Merged
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import { useState, useRef, useEffect } from '@wordpress/element';
import { Form } from '@woocommerce/components';

Expand All @@ -11,7 +10,6 @@ import { Form } from '@woocommerce/components';
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalCountryCodes';
import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus';
import AudienceSection from '.~/components/paid-ads/audience-section';
import BudgetSection from '.~/components/paid-ads/budget-section';
import BillingCard from '.~/components/paid-ads/billing-card';
import SpinnerCard from '.~/components/spinner-card';
Expand Down Expand Up @@ -45,23 +43,8 @@ const defaultPaidAds = {
* @param {Array<CountryCode>} targetAudience Country codes of selected target audience.
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
* @return {PaidAdsData} The resolved paid ads data.
*/
function resolveInitialPaidAds( paidAds, targetAudience ) {
eason9487 marked this conversation as resolved.
Show resolved Hide resolved
const nextPaidAds = { ...paidAds };

if ( targetAudience ) {
if ( nextPaidAds.countryCodes === defaultPaidAds.countryCodes ) {
// Replace the country codes with the loaded target audience only if the reference is
// the same as the default because the given country codes might be the restored ones.
nextPaidAds.countryCodes = targetAudience;
} else {
// The selected target audience may be changed back and forth during the onboarding flow.
// Remove countries if any don't exist in the latest state.
nextPaidAds.countryCodes = nextPaidAds.countryCodes.filter(
( code ) => targetAudience.includes( code )
);
}
}

function resolveInitialPaidAds( paidAds, targetAudience = [] ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

By setting an empty array as the default parameter, it seems targetAudience isn’t required, and we can create ads without specifying any countries.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks @jorgemd24 . Happy to revert that change but when I tested the "Continue" button to proceed to the next step was disabled if there were no countries selected. Screenshot below:
image

Also, in the validateCampaign function, there's a check for an empty array of countries.

if ( values.countryCodes.length === 0 ) {

Let me know what you think 😁

const nextPaidAds = { ...paidAds, countryCodes: targetAudience };
nextPaidAds.isValid = ! Object.keys( validateCampaign( nextPaidAds ) )
.length;

Expand Down Expand Up @@ -160,22 +143,12 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) {
disabledAudience || countryCodes.length === 0;

return (
<>
<AudienceSection
formProps={ formProps }
disabled={ disabledAudience }
countrySelectHelperText={ __(
'You can only choose from countries you’ve selected during product listings configuration.',
'google-listings-and-ads'
) }
/>
<BudgetSection
formProps={ formProps }
disabled={ disabledBudget }
>
{ ! disabledBudget && <BillingCard /> }
</BudgetSection>
</>
<BudgetSection
formProps={ formProps }
disabled={ disabledBudget }
>
{ ! disabledBudget && <BillingCard /> }
</BudgetSection>
);
} }
</Form>
Expand Down
88 changes: 0 additions & 88 deletions tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -11,13 +11,8 @@ import CompleteCampaign from '../../utils/pages/setup-mc/step-4-complete-campaig
import SetupAdsAccountPage from '../../utils/pages/setup-ads/setup-ads-accounts';
import {
checkFAQExpandable,
fillCountryInSearchBox,
getCountryInputSearchBoxContainer,
getCountryTagsFromInputSearchBoxContainer,
getFAQPanelTitle,
getFAQPanelRow,
getTreeSelectMenu,
removeCountryFromSearchBox,
checkBillingAdsPopup,
} from '../../utils/page';

Expand Down Expand Up @@ -183,17 +178,6 @@ test.describe( 'Complete your campaign', () => {
} );

test.describe( 'Setup up ads to a Google Ads account', () => {
test( 'should see "Ads audience" section is enabled', async () => {
const adsAudienceSection =
completeCampaign.getAdsAudienceSection();
await expect( adsAudienceSection ).toBeVisible();

// Confirm that the section title contains the correct text.
await expect(
adsAudienceSection.locator( 'h1' )
).toContainText( 'Ads audience' );
} );

test( 'should see "Set your budget" section is enabled', async () => {
const budgetSection = completeCampaign.getBudgetSection();
await expect( budgetSection ).toBeVisible();
Expand All @@ -219,78 +203,6 @@ test.describe( 'Complete your campaign', () => {
await completeCampaign.goto();
} );

test.describe( 'Select audience', () => {
test( 'should see only three country tags in country input search box', async () => {
const countrySearchBoxContainer =
getCountryInputSearchBoxContainer( page );
const countryTags =
getCountryTagsFromInputSearchBoxContainer( page );
await expect( countryTags ).toHaveCount( 3 );
await expect( countrySearchBoxContainer ).toContainText(
'United States'
);
await expect( countrySearchBoxContainer ).toContainText(
'Taiwan'
);
await expect( countrySearchBoxContainer ).toContainText(
'United Kingdom'
);
} );

test( 'should only allow searching for the same set of the countries selected in step 2, which is returned by target audience API', async () => {
const treeSelectMenu = getTreeSelectMenu( page );
joemcgill marked this conversation as resolved.
Show resolved Hide resolved

await fillCountryInSearchBox( page, 'United States' );
await expect( treeSelectMenu ).toBeVisible();

await fillCountryInSearchBox( page, 'United Kingdom' );
await expect( treeSelectMenu ).toBeVisible();

await fillCountryInSearchBox( page, 'Taiwan' );
await expect( treeSelectMenu ).toBeVisible();

await fillCountryInSearchBox( page, 'Japan' );
await expect( treeSelectMenu ).not.toBeVisible();

await fillCountryInSearchBox( page, 'Spain' );
await expect( treeSelectMenu ).not.toBeVisible();
} );

test( 'should see the budget recommendation value changed, and see the budget recommendation request is triggered when changing the ads audience', async () => {
let textContent = await setupBudgetPage
.getBudgetRecommendationTextRow()
joemcgill marked this conversation as resolved.
Show resolved Hide resolved
.textContent();

const textBeforeRemoveCountry =
setupBudgetPage.extractBudgetRecommendationValue(
joemcgill marked this conversation as resolved.
Show resolved Hide resolved
textContent
);

const responsePromise =
setupBudgetPage.registerBudgetRecommendationResponse();
joemcgill marked this conversation as resolved.
Show resolved Hide resolved

await removeCountryFromSearchBox(
page,
'United Kingdom (UK)'
);

await responsePromise;

textContent = await setupBudgetPage
.getBudgetRecommendationTextRow()
joemcgill marked this conversation as resolved.
Show resolved Hide resolved
.textContent();

const textAfterRemoveCountry =
setupBudgetPage.extractBudgetRecommendationValue(
joemcgill marked this conversation as resolved.
Show resolved Hide resolved
textContent
);

await expect( textBeforeRemoveCountry ).not.toBe(
textAfterRemoveCountry
);
} );
} );

test.describe( 'Set up budget', () => {
test( 'should see the low budget tip when the buget is set lower than the recommended value', async () => {
await setupBudgetPage.fillBudget( '1' );
Expand Down
11 changes: 0 additions & 11 deletions tests/e2e/utils/page.js
Original file line number Diff line number Diff line change
Expand Up @@ -80,17 +80,6 @@ export function getCountryInputSearchBox( page ) {
);
}

/**
* Get tree select menu.
*
* @param {import('@playwright/test').Page} page The current page.
*
* @return {import('@playwright/test').Locator} Get tree select menu.
*/
export function getTreeSelectMenu( page ) {
return page.locator( '.woocommerce-tree-select-control__main' );
}

/**
* Get tree item by country name.
*
Expand Down
39 changes: 0 additions & 39 deletions tests/e2e/utils/pages/setup-ads/setup-budget.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,15 +12,6 @@ export default class SetupBudget extends MockRequests {
this.page = page;
}

/**
* Get budget recommendation text row.
*
* @return {import('@playwright/test').Locator} The budget recommendation text row.
*/
getBudgetRecommendationTextRow() {
return this.page.locator( '.components-tip p > em > strong' );
}

/**
* Get budget input.
*
Expand Down Expand Up @@ -84,36 +75,6 @@ export default class SetupBudget extends MockRequests {
);
}

/**
* 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
);
}

/**
* Fill the budget.
*
Expand Down
9 changes: 0 additions & 9 deletions tests/e2e/utils/pages/setup-mc/step-4-complete-campaign.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,6 @@ export default class CompleteCampaign extends MockRequests {
return this.page.locator( '.wcdl-section' );
}

/**
* Get ads audience section.
*
* @return {import('@playwright/test').Locator} Get ads audience section.
*/
getAdsAudienceSection() {
return this.getSections().nth( 1 );
}

/**
* Get budget section.
*
Expand Down