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

Update Budget Setup Card. #2552

Open
wants to merge 21 commits into
base: feature/2459-campaign-creation-flow
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
* Internal dependencies
*/
import useCountryKeyNameMap from '.~/hooks/useCountryKeyNameMap';
import useFetchBudgetRecommendationEffect from './useFetchBudgetRecommendationEffect';
import './index.scss';

/*
Expand All @@ -34,12 +33,12 @@
const template = isMultiple
? // translators: it's a range of recommended budget amount. 1: the value of the budget, 2: the currency of amount.
__(
'Google will optimize your ads to maximize performance across the country/s you select.<br /><em>Tip: Most merchants targeting similar countries <strong>set a daily budget of %1$f %2$s</strong></em>',
'We recommend running campaigns at least 1 month so it can learn to optimize for your business.<br /><em>Tip: Most merchants targeting similar countries <strong>set a daily budget of %1$f %2$s</strong></em>',
'google-listings-and-ads'
)
: // translators: it's a range of recommended budget amount. 1: the value of the budget, 2: the currency of amount 3: a country name selected by the merchant.
__(
'Google will optimize your ads to maximize performance across the country/s you select.<br /><em>Tip: Most merchants targeting <strong>%3$s set a daily budget of %1$f %2$s</strong></em>',
'We recommend running campaigns at least 1 month so it can learn to optimize for your business.<br /><em>Tip: Most merchants targeting <strong>%3$s set a daily budget of %1$f %2$s</strong></em>',
'google-listings-and-ads'
);

Expand All @@ -50,17 +49,19 @@
}

const BudgetRecommendation = ( props ) => {
const { countryCodes, dailyAverageCost = Infinity } = props;
const { data } = useFetchBudgetRecommendationEffect( countryCodes );
const { countryCodes, dailyAverageCost = Infinity, data } = props;
const map = useCountryKeyNameMap();

if ( ! data ) {
return null;
}

const { currency, recommendations } = data;
const { daily_budget: dailyBudget, country } =
getHighestBudget( recommendations );
const { daily_budget: dailyBudget, country } = getHighestBudget(

Check warning on line 60 in js/src/components/paid-ads/budget-section/budget-recommendation/index.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/budget-section/budget-recommendation/index.js#L60

Added line #L60 was not covered by tests
recommendations.filter( ( recommendation ) =>
joemcgill marked this conversation as resolved.
Show resolved Hide resolved
countryCodes.includes( recommendation.country )

Check warning on line 62 in js/src/components/paid-ads/budget-section/budget-recommendation/index.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/budget-section/budget-recommendation/index.js#L62

Added line #L62 was not covered by tests
)
Copy link
Member

Choose a reason for hiding this comment

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

I'm curious about in which case it needs this filter?

);

const countryName = map[ country ];
const recommendationRange = toRecommendationRange(
Expand Down
4 changes: 3 additions & 1 deletion js/src/components/paid-ads/budget-section/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
*/
const BudgetSection = ( { formProps, disabled = false, children } ) => {
const { getInputProps, setValue, values } = formProps;
const { countryCodes, amount } = values;
const { countryCodes, amount, recommendationData } = values;

Check warning on line 33 in js/src/components/paid-ads/budget-section/index.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/budget-section/index.js#L33

Added line #L33 was not covered by tests
const { googleAdsAccount } = useGoogleAdsAccount();
const monthlyMaxEstimated = getMonthlyMaxEstimated( amount );
// Display the currency code that will be used by Google Ads, but still use the store's currency formatting settings.
Expand Down Expand Up @@ -93,6 +93,8 @@
<BudgetRecommendation
countryCodes={ countryCodes }
dailyAverageCost={ amount }
currency={ currency }
Copy link
Member

Choose a reason for hiding this comment

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

BudgetRecommendation component doesn't use the currency prop.

data={ recommendationData }
/>
) }
</Section.Card.Body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import { Form } from '@woocommerce/components';
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalCountryCodes';
import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus';
import useFetchBudgetRecommendationEffect from '.~/components/paid-ads/budget-section/budget-recommendation/useFetchBudgetRecommendationEffect';
Copy link
Member

Choose a reason for hiding this comment

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

What I understand is that the same logic of setting the default budget will then be applied to all ad creation forms, so moving the useFetchBudgetRecommendationEffect hook to the .~/hooks directory would be more appropriate.

import AudienceSection from '.~/components/paid-ads/audience-section';
import BudgetSection from '.~/components/paid-ads/budget-section';
import BillingCard from '.~/components/paid-ads/billing-card';
Expand All @@ -33,10 +34,28 @@ import { GOOGLE_ADS_BILLING_STATUS } from '.~/constants';
const defaultPaidAds = {
amount: 0,
countryCodes: [],
recommendations: [],
isValid: false,
isReady: false,
recommendationData: null,
};

/*
* If a merchant selects more than one country, the budget recommendation
* takes the highest country out from the selected countries.
*
* For example, a merchant selected Brunei (20 USD) and Croatia (15 USD),
* then the budget recommendation should be (20 USD).
*/
function getHighestBudget( recommendations ) {
return recommendations.reduce( ( defender, challenger ) => {
if ( challenger.daily_budget > defender.daily_budget ) {
return challenger;
}
return defender;
} );
}
Copy link
Member

Choose a reason for hiding this comment

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

This function is completely the same as:

/*
* If a merchant selects more than one country, the budget recommendation
* takes the highest country out from the selected countries.
*
* For example, a merchant selected Brunei (20 USD) and Croatia (15 USD),
* then the budget recommendation should be (20 USD).
*/
function getHighestBudget( recommendations ) {
return recommendations.reduce( ( defender, challenger ) => {
if ( challenger.daily_budget > defender.daily_budget ) {
return challenger;
}
return defender;
} );
}

Suggest considering whether it can be integrated into the useFetchBudgetRecommendationEffect hook or extracted as a util to eliminate duplication.


/**
* Resolve the initial paid ads data from the given paid ads data with the loaded target audience.
* Parts of the resolved data are used in the `initialValues` prop of `Form` component.
Expand Down Expand Up @@ -78,6 +97,14 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) {
const { hasGoogleAdsConnection } = useGoogleAdsAccount();
const { data: targetAudience } = useTargetAudienceFinalCountryCodes();
const { billingStatus } = useGoogleAdsAccountBillingStatus();
const { data: recommendationData } =
useFetchBudgetRecommendationEffect( targetAudience );
Copy link
Member

Choose a reason for hiding this comment

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

It may cause API to fail with a 400 bad request error.

image

Steps:

  1. Proceed to onboarding step 4
  2. Refresh webpage

const { recommendations } = recommendationData || {};

const { daily_budget: dailyBudget } =
recommendations !== undefined
? getHighestBudget( recommendations )
: {};

const onStatesReceivedRef = useRef();
onStatesReceivedRef.current = onStatesReceived;
Expand Down Expand Up @@ -127,10 +154,15 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) {
- https://github.com/woocommerce/google-listings-and-ads/blob/5b6522ca10ad75556e6b2de7c120cc712aab70b1/js/src/components/free-listings/setup-free-listings/index.js#L172-L186
*/
useEffect( () => {
setPaidAds( ( currentPaidAds ) =>
resolveInitialPaidAds( currentPaidAds, targetAudience )
);
}, [ targetAudience ] );
setPaidAds( ( currentPaidAds ) => {
currentPaidAds = {
...currentPaidAds,
amount: dailyBudget ? dailyBudget : currentPaidAds.amount,
recommendationData,
};
return resolveInitialPaidAds( currentPaidAds, targetAudience );
} );
}, [ targetAudience, dailyBudget, recommendationData ] );

if ( ! targetAudience || ! billingStatus ) {
return (
Expand All @@ -143,6 +175,7 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) {
const initialValues = {
countryCodes: paidAds.countryCodes,
amount: paidAds.amount,
recommendationData: paidAds.recommendationData,
Copy link
Member

Choose a reason for hiding this comment

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

The recommendationData is not used as the form value, so passing it around this way is not ideal.

};

return (
Expand Down
43 changes: 37 additions & 6 deletions tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,24 @@ test.describe( 'Complete your campaign', () => {
[ 'GET' ]
),

completeCampaign.fulfillBudgetRecommendations( {
currency: 'USD',
recommendations: [
{
country: 'US',
daily_budget: 10,
},
{
country: 'TW',
daily_budget: 8,
},
{
country: 'GB',
daily_budget: 20,
},
],
} ),

// The following mocks are requests will happen after completing the onboarding
completeCampaign.mockSuccessfulSettingsSyncRequest(),

Expand Down Expand Up @@ -266,16 +284,11 @@ test.describe( 'Complete your campaign', () => {
textContent
);

const responsePromise =
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we no longer need to wait for this response after the country setting is changed? It looks like when the country values change, the text that reads "Tip: Most merchants targeting similar countries set a daily budget of %d USD" does change, so we should probably ensure that is still being tested.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ankitrox I'm still unsure why we need to remove this. Could you clarify?

setupBudgetPage.registerBudgetRecommendationResponse();

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

await responsePromise;

textContent = await setupBudgetPage
.getBudgetRecommendationTextRow()
.textContent();
Expand All @@ -289,9 +302,25 @@ test.describe( 'Complete your campaign', () => {
textAfterRemoveCountry
);
} );

test( 'should have the tip text "We recommend running campaigns at least 1 month so it can learn to optimize for your business."', async () => {
const tipText =
setupBudgetPage.getBudgetRecommendationTip();
await expect( tipText ).toContainText(
'We recommend running campaigns at least 1 month so it can learn to optimize for your business.'
);
} );
} );

test.describe( 'Set up budget', () => {
test( '"Daily average cost" input should have highest value set', async () => {
const dailyAverageCostInput =
setupBudgetPage.getBudgetInput();
await expect( dailyAverageCostInput ).toHaveValue(
'20.00'
);
} );

test( 'should see the low budget tip when the buget is set lower than the recommended value', async () => {
await setupBudgetPage.fillBudget( '1' );
const lowBudgetTip = setupBudgetPage.getLowerBudgetTip();
Expand Down Expand Up @@ -425,7 +454,9 @@ test.describe( 'Complete your campaign', () => {
} );

test( 'should also see the url contains product-feed', async () => {
expect( page.url() ).toMatch( /path=%2Fgoogle%2Fproduct-feed/ );
await expect( page.url() ).toMatch(
/path=%2Fgoogle%2Fproduct-feed/
);
} );

test( 'should see buttons on Dashboard for Google Ads onboarding', async () => {
Expand Down
15 changes: 15 additions & 0 deletions tests/e2e/utils/mock-requests.js
Original file line number Diff line number Diff line change
Expand Up @@ -375,6 +375,21 @@ export default class MockRequests {
);
}

/**
* Fulfill the budget recommendations request.
*
* @param {Object} payload
* @return {Promise<void>}
*/
async fulfillBudgetRecommendations( payload ) {
await this.fulfillRequest(
/\/wc\/gla\/ads\/campaigns\/budget-recommendation\b/,
payload,
200,
[ 'GET' ]
);
}

/**
* Mock the request to connect Jetpack
*
Expand Down
11 changes: 11 additions & 0 deletions tests/e2e/utils/pages/setup-ads/setup-budget.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,17 @@ export default class SetupBudget extends MockRequests {
this.page = page;
}

/**
* Get budget recommendation tip section.
*
* @return {import('@playwright/test').Locator} The budget recommendation text row.
Copy link
Member

Choose a reason for hiding this comment

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

This JSDoc description is not quite consistent.

*/
getBudgetRecommendationTip() {
return this.page.locator(
'.gla-budget-recommendation > .components-tip'
);
}

/**
* Get budget recommendation text row.
*
Expand Down