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 30 commits into
base: update/2535-consolidate-ad-creation-ccf-merged
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
a57bf14
Update Budget Setup Card.
ankitrox Aug 22, 2024
273d02b
Fix: e2e tests.
ankitrox Aug 22, 2024
9c30663
Add e2e coverage.
ankitrox Aug 22, 2024
103ae13
CR feedback - round 1.
ankitrox Aug 27, 2024
096c086
Fix: Step change shows no notice.
ankitrox Aug 30, 2024
76328ea
Fix: network error for budget recommendation.
ankitrox Sep 10, 2024
8d9358d
Remove redundant code.
ankitrox Sep 10, 2024
ea68ee8
Update js doc.
ankitrox Sep 10, 2024
dcc6f73
Resolve conflicts and fix tests.
ankitrox Sep 10, 2024
ac68177
Fix: lint issues.
ankitrox Sep 10, 2024
052783e
Remove unused code.
ankitrox Sep 10, 2024
c2fbf1a
Resolve conflict with base branch.
ankitrox Sep 12, 2024
61d73eb
use fetch budget hook in paid-ads-setup-sections.
ankitrox Sep 12, 2024
f15bfb9
Remove redundant code.
ankitrox Sep 12, 2024
85f5b35
Display spinner till data gets loaded.
ankitrox Sep 12, 2024
566476d
Add hook for code resuse.
ankitrox Sep 16, 2024
dc7a4ac
Use hook.
ankitrox Sep 16, 2024
9ddfef4
Stick to old UI/UX.
ankitrox Sep 16, 2024
4af37a3
Remove redundant code.
ankitrox Sep 16, 2024
1b92ecb
Pass recommendations prop.
ankitrox Sep 16, 2024
ab2db99
Remove redundant code.
ankitrox Sep 16, 2024
983438b
Merge branch 'update/2535-consolidate-ad-creation-ccf-merged' into up…
asvinb Oct 2, 2024
eb18178
Try not to use the useBudgetRecommendationData hook.
asvinb Oct 2, 2024
35716f0
Add useFetchBudgetRecommendation hook.
asvinb Oct 2, 2024
0cafdb2
Make use of new hook and add loading indicator.
asvinb Oct 2, 2024
79ccefa
Remove unused hook.
asvinb Oct 2, 2024
fbde5f4
Move clientSession.
asvinb Oct 2, 2024
3ea1c12
Restore complete campaign data from client session.
asvinb Oct 2, 2024
f6f7ac3
Merge pull request #2637 from woocommerce/update/2502-budget-setup-ca…
joemcgill Oct 2, 2024
7aaa968
Set minimum amount to always be the recommended budget.
asvinb Oct 7, 2024
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
12 changes: 11 additions & 1 deletion js/src/components/paid-ads/ads-campaign.js
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
/* eslint-disable react-hooks/exhaustive-deps */
joemcgill marked this conversation as resolved.
Show resolved Hide resolved
/**
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import { createInterpolateElement } from '@wordpress/element';
import { createInterpolateElement, useEffect } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -17,6 +18,7 @@
import BudgetSection from './budget-section';
import { CampaignPreviewCard } from './campaign-preview';
import FaqsSection from './faqs-section';
import clientSession from '.~/setup-mc/setup-stepper/setup-paid-ads/clientSession';

/**
* @typedef {import('.~/data/actions').Campaign} Campaign
Expand Down Expand Up @@ -55,6 +57,14 @@
'google-listings-and-ads'
);

// Set the amount from client session if it is available.
useEffect( () => {
asvinb marked this conversation as resolved.
Show resolved Hide resolved
const sessionData = clientSession.getCampaign();

Check warning on line 62 in js/src/components/paid-ads/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign.js#L61-L62

Added lines #L61 - L62 were not covered by tests
if ( sessionData?.amount !== undefined ) {
formContext.setValue( 'amount', sessionData.amount );

Check warning on line 64 in js/src/components/paid-ads/ads-campaign.js

View check run for this annotation

Codecov / codecov/patch

js/src/components/paid-ads/ads-campaign.js#L64

Added line #L64 was not covered by tests
}
}, [] ); // eslint-disable-line react-hooks/exhaustive-deps

return (
<StepContent>
<StepContentHeader
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,17 @@
* External dependencies
*/
import { __, sprintf } from '@wordpress/i18n';
import { createInterpolateElement } from '@wordpress/element';
import { createInterpolateElement, useEffect } from '@wordpress/element';
import { Tip } from '@wordpress/components';
import GridiconNoticeOutline from 'gridicons/dist/notice-outline';

/**
* Internal dependencies
*/
import useCountryKeyNameMap from '.~/hooks/useCountryKeyNameMap';
import useFetchBudgetRecommendationEffect from './useFetchBudgetRecommendationEffect';
import './index.scss';
import useFetchBudgetRecommendationEffect from '.~/hooks/useFetchBudgetRecommendationEffect';
import clientSession from '.~/setup-mc/setup-stepper/setup-paid-ads/clientSession';

/*
* If a merchant selects more than one country, the budget recommendation
Expand All @@ -21,25 +22,30 @@
* 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 recommendations.reduce(
( defender, challenger ) => {

Check warning on line 26 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#L25-L26

Added lines #L25 - L26 were not covered by tests
if ( challenger.daily_budget > defender.daily_budget ) {
return challenger;

Check warning on line 28 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#L28

Added line #L28 was not covered by tests
}
return defender;

Check warning on line 30 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#L30

Added line #L30 was not covered by tests
},
{
daily_budget: 0,
}
return defender;
} );
);
}

function toRecommendationRange( isMultiple, ...values ) {
const conversionMap = { strong: <strong />, em: <em />, br: <br /> };
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,18 +56,42 @@
}

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

Check warning on line 61 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#L61

Added line #L61 was not covered by tests
setRecommendedBudget,
setRecommendationsLoaded,
} = props;

Check warning on line 64 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#L64

Added line #L64 was not covered by tests

const map = useCountryKeyNameMap();
const { data, loading } =
useFetchBudgetRecommendationEffect( countryCodes );

Check warning on line 68 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#L68

Added line #L68 was not covered by tests

if ( ! data ) {
return null;
}
const recommendationsLoaded = ! loading;

Check warning on line 70 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#L70

Added line #L70 was not covered by tests

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

useEffect( () => {

Check warning on line 76 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#L76

Added line #L76 was not covered by tests
if ( recommendationsLoaded ) {
const sessionData = clientSession.getCampaign();

Check warning on line 78 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#L78

Added line #L78 was not covered by tests
const sessionAmount =
sessionData?.amount !== undefined
? dailyAverageCost
: dailyBudget;

Check warning on line 82 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#L81-L82

Added lines #L81 - L82 were not covered by tests

setRecommendationsLoaded( true );
setRecommendedBudget( sessionAmount );

Check warning on line 85 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#L84-L85

Added lines #L84 - L85 were not covered by tests
}
}, [
dailyAverageCost,
dailyBudget,
recommendationsLoaded,
setRecommendationsLoaded,
setRecommendedBudget,
] );

const countryName = map[ country ];
const recommendationRange = toRecommendationRange(
recommendations.length > 1,
Expand Down
49 changes: 39 additions & 10 deletions js/src/components/paid-ads/budget-section/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import { __ } from '@wordpress/i18n';
import { useEffect, useRef } from '@wordpress/element';
import { useEffect, useRef, useState } from '@wordpress/element';

/**
* Internal dependencies
Expand All @@ -13,6 +13,7 @@
import BudgetRecommendation from './budget-recommendation';
import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount';
import AppInputPriceControl from '.~/components/app-input-price-control';
import clientSession from '.~/setup-mc/setup-stepper/setup-paid-ads/clientSession';

const nonInteractableProps = {
noPointerEvents: true,
Expand Down Expand Up @@ -42,16 +43,40 @@
const setValueRef = useRef();
setValueRef.current = setValue;

/**
* In addition to the initial value setting during initialization, when `disabled` changes
* - from false to true, then clear filled amount to `undefined` for showing a blank <input>.
* - from true to false, then reset amount to the initial value passed from the consumer side.
*/
const initialAmountRef = useRef( amount );
const [ recommendedBudget, setRecommendedBudget ] = useState( null );

Check warning on line 46 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#L46

Added line #L46 was not covered by tests
const [ recommendationsLoaded, setRecommendationsLoaded ] =
useState( false );

Check warning on line 48 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#L48

Added line #L48 was not covered by tests

useEffect( () => {
const nextAmount = disabled ? undefined : initialAmountRef.current;
setValueRef.current( 'amount', nextAmount );
}, [ disabled ] );
if ( ! recommendationsLoaded || recommendedBudget === null ) {
return;

Check warning on line 52 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#L52

Added line #L52 was not covered by tests
}

const sessionData = clientSession.getCampaign();

Check warning on line 55 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#L55

Added line #L55 was not covered by tests

let sessionAmount;
if ( sessionData?.amount === undefined ) {
sessionAmount = recommendedBudget;
} else {
sessionAmount = amount;

Check warning on line 61 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#L59-L61

Added lines #L59 - L61 were not covered by tests
}

if ( disabled ) {
sessionAmount = undefined;

Check warning on line 65 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#L65

Added line #L65 was not covered by tests
}

clientSession.setCampaign( {

Check warning on line 68 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#L68

Added line #L68 was not covered by tests
amount: sessionAmount,
countryCodes,
} );
setValueRef.current( 'amount', sessionAmount );

Check warning on line 72 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#L72

Added line #L72 was not covered by tests
}, [
amount,
countryCodes,
disabled,
recommendationsLoaded,
recommendedBudget,
] );

return (
<div className="gla-budget-section">
Expand Down Expand Up @@ -93,6 +118,10 @@
<BudgetRecommendation
countryCodes={ countryCodes }
dailyAverageCost={ amount }
setRecommendedBudget={ setRecommendedBudget }
setRecommendationsLoaded={
setRecommendationsLoaded
}
/>
) }
</Section.Card.Body>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) {
isReady: paidAds.isValid && isBillingCompleted,
};
onStatesReceivedRef.current( nextPaidAds );
clientSession.setCampaign( nextPaidAds );
}, [ paidAds, isBillingCompleted ] );

/*
Expand Down
56 changes: 51 additions & 5 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 @@ -256,7 +274,7 @@ test.describe( 'Complete your campaign', () => {
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 () => {
test( 'should see the budget recommendation value changed when changing the ads audience', async () => {
let textContent = await setupBudgetPage
.getBudgetRecommendationTextRow()
.textContent();
Expand All @@ -266,16 +284,26 @@ 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 Author

Choose a reason for hiding this comment

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

@joemcgill This is no longer require because we are fulfilling the budget recommendation here, so we no longer have to wait to capture the budget recommendation response.

Those assertions will still be tested with the new fulfilment.

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@joemcgill Sorry for missing this one.

Initially when we were removing country from (or updating anything in Select country/s textbox), a request to budget recommendation endpoint was triggered which we were awaiting to get fulfilled in setupBudgetPage.registerBudgetRecommendationResponse()

Now, as per our new implementation, that request will no longer trigger because we are using the countries that have been selected in step 2 which acts as a superset for step 4 data.

Please let me know if this helps or in case of any other query.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ankitrox Can you check @joemcgill 's comment please? Not sure about this one.

setupBudgetPage.registerBudgetRecommendationResponse();
// Mock response when UK is removed from the target audience.
completeCampaign.fulfillBudgetRecommendations( {
currency: 'USD',
recommendations: [
{
country: 'US',
daily_budget: 10,
},
{
country: 'TW',
daily_budget: 8,
},
],
} );

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

await responsePromise;

textContent = await setupBudgetPage
.getBudgetRecommendationTextRow()
.textContent();
Expand All @@ -288,10 +316,28 @@ test.describe( 'Complete your campaign', () => {
await expect( textBeforeRemoveCountry ).not.toBe(
textAfterRemoveCountry
);

await expect( textAfterRemoveCountry ).toBe( '10' );
} );

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
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 tip.
*/
getBudgetRecommendationTip() {
return this.page.locator(
'.gla-budget-recommendation > .components-tip'
);
}

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