From f6c9f55bc934633d6c4f7604d632596bf321f7ed Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Thu, 5 Sep 2024 12:43:17 +0530 Subject: [PATCH 01/50] Updates validateCampaign with daily limit check --- js/src/components/paid-ads/validateCampaign.js | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index d2b2e60106..f5c6a3561c 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -30,6 +30,22 @@ const validateCampaign = ( values ) => { ); } + if ( + Number.isFinite( values.amount ) && + Number.isFinite( values.budget?.daily_budget ) + ) { + const { budget, budgetMin, amount } = values; + const { daily_budget: dailyBudget } = budget; + const minAmount = Math.round( dailyBudget * budgetMin, 2 ); + + if ( amount < minAmount ) { + errors.amount = __( + `Please make sure daily average cost is atleast ${ minAmount }.`, + 'google-listings-and-ads' + ); + } + } + return errors; }; From dbfb9726eb14334a1335c5664fd0eeb03ba4b401 Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Thu, 5 Sep 2024 12:45:34 +0530 Subject: [PATCH 02/50] Adds validateCampaign tests --- .../paid-ads/validateCampaign.test.js | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index afad100326..658320c291 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -81,4 +81,28 @@ describe( 'validateCampaign', () => { expect( errors ).toHaveProperty( 'amount' ); expect( errors.amount ).toMatchSnapshot(); } ); + + it( 'When a budget is provided and the amount is less than the minimum, should not pass', () => { + values.amount = 10; + values.budget = { + daily_budget: 100, + }; + values.budgetMin = 0.3; + + const errors = validateCampaign( values ); + + expect( errors ).toHaveProperty( 'amount' ); + expect( errors.amount ).toContain( 'atleast 30' ); + } ); + + it( 'When a budget is provided and the amount is greater than the minimum, should pass', () => { + values.amount = 35; + values.budget = { + daily_budget: 100, + }; + values.budgetMin = 0.3; + + const errors = validateCampaign( values ); + expect( errors ).not.toHaveProperty( 'amount' ); + } ); } ); From fb36ca623debf3859d3966c3dceead0c27614523 Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Thu, 5 Sep 2024 12:47:41 +0530 Subject: [PATCH 03/50] Adds getHighestBudget utility --- js/src/utils/getHighestBudget.js | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) create mode 100644 js/src/utils/getHighestBudget.js diff --git a/js/src/utils/getHighestBudget.js b/js/src/utils/getHighestBudget.js new file mode 100644 index 0000000000..e2fef429e9 --- /dev/null +++ b/js/src/utils/getHighestBudget.js @@ -0,0 +1,19 @@ +/* + * 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). + */ +export default function getHighestBudget( recommendations ) { + if ( ! recommendations ) { + return null; + } + + return recommendations.reduce( ( defender, challenger ) => { + if ( challenger.daily_budget > defender.daily_budget ) { + return challenger; + } + return defender; + } ); +} From 41956ca0cf411c4ae0182d5581f0118d7526e138 Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Thu, 5 Sep 2024 12:48:49 +0530 Subject: [PATCH 04/50] Adds daily limit data lookup to paid ads setup sections --- .../setup-paid-ads/paid-ads-setup-sections.js | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index 86c3739bf7..2e136eced5 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -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'; import AudienceSection from '.~/components/paid-ads/audience-section'; import BudgetSection from '.~/components/paid-ads/budget-section'; import BillingCard from '.~/components/paid-ads/billing-card'; @@ -19,6 +20,7 @@ import Section from '.~/wcdl/section'; import validateCampaign from '.~/components/paid-ads/validateCampaign'; import clientSession from './clientSession'; import { GOOGLE_ADS_BILLING_STATUS } from '.~/constants'; +import getHighestBudget from '.~/utils/getHighestBudget'; /** * @typedef { import(".~/data/actions").CountryCode } CountryCode @@ -91,6 +93,11 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) { return resolveInitialPaidAds( startingPaidAds, targetAudience ); } ); + const { data: budgetData } = useFetchBudgetRecommendationEffect( + paidAds.countryCodes + ); + const budget = getHighestBudget( budgetData?.recommendations ); + const isBillingCompleted = billingStatus?.status === GOOGLE_ADS_BILLING_STATUS.APPROVED; @@ -143,6 +150,8 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) { const initialValues = { countryCodes: paidAds.countryCodes, amount: paidAds.amount, + budget: budget, + budgetMin: 0.3, }; return ( From 9ef16c23bf037cad0ac37ea45487aa62901a3abc Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Thu, 5 Sep 2024 13:09:13 +0530 Subject: [PATCH 05/50] Fixes linter warnings --- js/src/components/paid-ads/validateCampaign.js | 12 ++++++++---- js/src/components/paid-ads/validateCampaign.test.js | 2 +- .../setup-paid-ads/paid-ads-setup-sections.js | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index f5c6a3561c..8919222214 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { __ } from '@wordpress/i18n'; +import { __, sprintf } from '@wordpress/i18n'; /** * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues @@ -39,9 +39,13 @@ const validateCampaign = ( values ) => { const minAmount = Math.round( dailyBudget * budgetMin, 2 ); if ( amount < minAmount ) { - errors.amount = __( - `Please make sure daily average cost is atleast ${ minAmount }.`, - 'google-listings-and-ads' + errors.amount = sprintf( + /* translators: %s: minimum daily budget */ + __( + 'Please make sure daily average cost is at least %s.', + 'google-listings-and-ads' + ), + minAmount ); } } diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index 658320c291..e79a9796c2 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -92,7 +92,7 @@ describe( 'validateCampaign', () => { const errors = validateCampaign( values ); expect( errors ).toHaveProperty( 'amount' ); - expect( errors.amount ).toContain( 'atleast 30' ); + expect( errors.amount ).toContain( 'at least 30' ); } ); it( 'When a budget is provided and the amount is greater than the minimum, should pass', () => { diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index 2e136eced5..9e59abf584 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -150,7 +150,7 @@ export default function PaidAdsSetupSections( { onStatesReceived } ) { const initialValues = { countryCodes: paidAds.countryCodes, amount: paidAds.amount, - budget: budget, + budget, budgetMin: 0.3, }; From 5741e6bbcbc3d28551019442022e2b582107c95f Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 6 Sep 2024 17:40:42 +0400 Subject: [PATCH 06/50] Style error message accordingly. --- js/src/components/app-input-control/index.scss | 2 ++ 1 file changed, 2 insertions(+) diff --git a/js/src/components/app-input-control/index.scss b/js/src/components/app-input-control/index.scss index 770d2ef41d..bd6f808a9a 100644 --- a/js/src/components/app-input-control/index.scss +++ b/js/src/components/app-input-control/index.scss @@ -29,11 +29,13 @@ color: $gray-700; } + &.has-error .components-input-control__backdrop, &--error-character-count .components-input-control .components-input-control__container .components-input-control__backdrop { border-color: $alert-red; box-shadow: none; } + &.has-error .components-base-control__help, &--error-character-count &__character-count { color: $alert-red; } From beb370030a50bd19326d694352f7a461f0669980 Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Wed, 11 Sep 2024 12:02:14 +0530 Subject: [PATCH 07/50] Adds e2e tests --- .../setup-mc/step-4-complete-campaign.test.js | 46 +++++++++++++++++++ .../e2e/utils/pages/setup-ads/setup-budget.js | 14 ++++++ 2 files changed, 60 insertions(+) diff --git a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js index a957d800f4..2337194d99 100644 --- a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js +++ b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js @@ -296,6 +296,52 @@ test.describe( 'Complete your campaign', () => { } ); } ); + test.describe( 'Validate budget percent', () => { + test.beforeAll( async () => { + await setupBudgetPage.mockBudgetRecommendation( { + currency: 'USD', + recommendations: [ + { + country: 'US', + daily_budget: 100, + }, + ], + } ); + } ); + + test( 'should see validation error if lower than the 30%', async () => { + await setupBudgetPage.fillBudget( '10' ); + await setupBudgetPage.getBudgetInput().blur(); + + const error = await page + .locator( '.components-base-control__help' ) + .textContent(); + await expect( error ).toBe( + 'Please make sure daily average cost is at least 30.' + ); + } ); + + test( 'should not see validation error if exactly 30%', async () => { + await setupBudgetPage.fillBudget( '30' ); + await setupBudgetPage.getBudgetInput().blur(); + + const error = await page.locator( + '.components-base-control__help' + ); + await expect( error ).not.toBeVisible(); + } ); + + test( 'should not see validation error if greater than 30%', async () => { + await setupBudgetPage.fillBudget( '40' ); + await setupBudgetPage.getBudgetInput().blur(); + + const error = await page.locator( + '.components-base-control__help' + ); + await expect( error ).not.toBeVisible(); + } ); + } ); + test.describe( 'Set up billing', () => { let newPage; diff --git a/tests/e2e/utils/pages/setup-ads/setup-budget.js b/tests/e2e/utils/pages/setup-ads/setup-budget.js index dca6843f64..43244bca01 100644 --- a/tests/e2e/utils/pages/setup-ads/setup-budget.js +++ b/tests/e2e/utils/pages/setup-ads/setup-budget.js @@ -187,6 +187,20 @@ export default class SetupBudget extends MockRequests { ); } + /** + * Mock the budget recommendation. + * + * @param {Object} payload The payload. + * @return {Promise} + */ + async mockBudgetRecommendation( payload ) { + await this.fulfillRequest( + /\/wc\/gla\/ads\/campaigns\/budget-recommendation\b/, + payload, + 200 + ); + } + /** * Mock the campaign creation process and the Ads setup completion. * From 1d41c1b6bab5de6bf979c71eb328dbdd944f39fe Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Fri, 13 Sep 2024 11:49:08 +0530 Subject: [PATCH 08/50] Uses form opts instead of values --- js/src/components/paid-ads/validateCampaign.js | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 172fcb60e4..fc4d6dcc24 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -11,9 +11,10 @@ import { __, sprintf } from '@wordpress/i18n'; * Validate campaign form. Accepts the form values object and returns errors object. * * @param {CampaignFormValues} values Campaign form values. + * @param {Object} opts Extra form options. * @return {Object} errors. */ -const validateCampaign = ( values ) => { +const validateCampaign = ( values, opts ) => { const errors = {}; if ( ! Number.isFinite( values.amount ) || values.amount <= 0 ) { @@ -24,10 +25,11 @@ const validateCampaign = ( values ) => { } if ( - Number.isFinite( values.amount ) && - Number.isFinite( values.budget?.daily_budget ) + Number.isFinite( values?.amount ) && + Number.isFinite( opts?.budget?.daily_budget ) ) { - const { budget, budgetMin, amount } = values; + const { amount } = values; + const { budget, budgetMin } = opts; const { daily_budget: dailyBudget } = budget; const minAmount = Math.round( dailyBudget * budgetMin, 2 ); From 585bbbd241fc0a0f836497821baaee4afe4d5589 Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Fri, 13 Sep 2024 11:49:56 +0530 Subject: [PATCH 09/50] Update countryCodes and validation callback --- .../setup-paid-ads/paid-ads-setup-sections.js | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index c1f2061143..8156afb13c 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -75,9 +75,7 @@ export default function PaidAdsSetupSections( { return resolveInitialPaidAds( startingPaidAds ); } ); - const { data: budgetData } = useFetchBudgetRecommendationEffect( - paidAds.countryCodes - ); + const { data: budgetData } = useFetchBudgetRecommendationEffect( countryCodes ); const budget = getHighestBudget( budgetData?.recommendations ); const isBillingCompleted = @@ -116,6 +114,9 @@ export default function PaidAdsSetupSections( { const initialValues = { amount: paidAds.amount, + }; + + const formOpts = { budget, budgetMin: 0.3, }; @@ -126,7 +127,7 @@ export default function PaidAdsSetupSections( { onChange={ ( _, values, isValid ) => { setPaidAds( { ...paidAds, ...values, isValid } ); } } - validate={ validateCampaign } + validate={ (formValues) => validateCampaign( formValues, formOpts ) } > { ( formProps ) => { return ( From f35ac9d6c36f356dae077f1aa199b0b219fa4e2f Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Fri, 13 Sep 2024 11:59:56 +0530 Subject: [PATCH 10/50] Updates unit tests --- .../paid-ads/validateCampaign.test.js | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index 65bd931f48..65ee9c5ddd 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -75,12 +75,15 @@ describe( 'validateCampaign', () => { it( 'When a budget is provided and the amount is less than the minimum, should not pass', () => { values.amount = 10; - values.budget = { - daily_budget: 100, + + const opts = { + budget: { + daily_budget: 100, + }, + budgetMin: 0.3, }; - values.budgetMin = 0.3; - const errors = validateCampaign( values ); + const errors = validateCampaign( values, opts ); expect( errors ).toHaveProperty( 'amount' ); expect( errors.amount ).toContain( 'at least 30' ); @@ -88,10 +91,13 @@ describe( 'validateCampaign', () => { it( 'When a budget is provided and the amount is greater than the minimum, should pass', () => { values.amount = 35; - values.budget = { - daily_budget: 100, + + const opts = { + budget: { + daily_budget: 100, + }, + budgetMin: 0.3, }; - values.budgetMin = 0.3; const errors = validateCampaign( values ); expect( errors ).not.toHaveProperty( 'amount' ); From 68044159131fa225e4f587514d27d2bda4402d22 Mon Sep 17 00:00:00 2001 From: Darshan Sawardekar Date: Fri, 13 Sep 2024 12:07:50 +0530 Subject: [PATCH 11/50] Fixes linter warnings --- js/src/components/paid-ads/validateCampaign.test.js | 2 +- .../setup-paid-ads/paid-ads-setup-sections.js | 7 +++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index 65ee9c5ddd..f63fd83a3d 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -99,7 +99,7 @@ describe( 'validateCampaign', () => { budgetMin: 0.3, }; - const errors = validateCampaign( values ); + const errors = validateCampaign( values, opts ); expect( errors ).not.toHaveProperty( 'amount' ); } ); } ); diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index 8156afb13c..ea463b77e3 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -75,7 +75,8 @@ export default function PaidAdsSetupSections( { return resolveInitialPaidAds( startingPaidAds ); } ); - const { data: budgetData } = useFetchBudgetRecommendationEffect( countryCodes ); + const { data: budgetData } = + useFetchBudgetRecommendationEffect( countryCodes ); const budget = getHighestBudget( budgetData?.recommendations ); const isBillingCompleted = @@ -127,7 +128,9 @@ export default function PaidAdsSetupSections( { onChange={ ( _, values, isValid ) => { setPaidAds( { ...paidAds, ...values, isValid } ); } } - validate={ (formValues) => validateCampaign( formValues, formOpts ) } + validate={ ( formValues ) => + validateCampaign( formValues, formOpts ) + } > { ( formProps ) => { return ( From d62bc02217507f14c7fba5086c5977e527015aa1 Mon Sep 17 00:00:00 2001 From: asvinb Date: Mon, 16 Sep 2024 18:00:06 +0400 Subject: [PATCH 12/50] Do not pass uneeded props. --- js/src/components/paid-ads/validateCampaign.js | 12 +++++++----- js/src/components/paid-ads/validateCampaign.test.js | 10 ++-------- .../setup-paid-ads/paid-ads-setup-sections.js | 3 +-- .../setup-stepper/setup-paid-ads/setup-paid-ads.js | 12 ++++++++---- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index fc4d6dcc24..8420bd607f 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -7,6 +7,9 @@ import { __, sprintf } from '@wordpress/i18n'; * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues */ +// Minimum budget percentage of daily budget. +const BUDGET_MIN_PERCENT = 0.3; + /** * Validate campaign form. Accepts the form values object and returns errors object. * @@ -26,18 +29,17 @@ const validateCampaign = ( values, opts ) => { if ( Number.isFinite( values?.amount ) && - Number.isFinite( opts?.budget?.daily_budget ) + Number.isFinite( opts?.dailyBudget ) ) { const { amount } = values; - const { budget, budgetMin } = opts; - const { daily_budget: dailyBudget } = budget; - const minAmount = Math.round( dailyBudget * budgetMin, 2 ); + const { dailyBudget } = opts; + const minAmount = Math.round( dailyBudget * BUDGET_MIN_PERCENT, 2 ); if ( amount < minAmount ) { errors.amount = sprintf( /* translators: %s: minimum daily budget */ __( - 'Please make sure daily average cost is at least %s.', + 'Please make sure daily average cost is greater than %s.', 'google-listings-and-ads' ), minAmount diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index f63fd83a3d..c03d4bd0ac 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -77,10 +77,7 @@ describe( 'validateCampaign', () => { values.amount = 10; const opts = { - budget: { - daily_budget: 100, - }, - budgetMin: 0.3, + dailyBudget: 100, }; const errors = validateCampaign( values, opts ); @@ -93,10 +90,7 @@ describe( 'validateCampaign', () => { values.amount = 35; const opts = { - budget: { - daily_budget: 100, - }, - budgetMin: 0.3, + dailyBudget: 100, }; const errors = validateCampaign( values, opts ); diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index ea463b77e3..c5081d20d8 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -118,8 +118,7 @@ export default function PaidAdsSetupSections( { }; const formOpts = { - budget, - budgetMin: 0.3, + dailyBudget: budget?.daily_budget, }; return ( diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js index cd5f77ac13..ed715e04f5 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js @@ -171,10 +171,14 @@ export default function SetupPaidAds() { - + + { countryCodes && ( + + ) } + { showSkipPaidAdsConfirmationModal && ( From 603d9c4c192278c0bc7d0e69cc4ecab0025d4a60 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 17 Sep 2024 21:03:53 +0400 Subject: [PATCH 13/50] Format currency for recommended budget. --- js/src/components/paid-ads/validateCampaign.js | 14 ++++++++------ .../components/paid-ads/validateCampaign.test.js | 8 +++++++- .../setup-paid-ads/paid-ads-setup-sections.js | 4 ++++ 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 8420bd607f..6f9d0cf1ea 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -7,7 +7,7 @@ import { __, sprintf } from '@wordpress/i18n'; * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues */ -// Minimum budget percentage of daily budget. +// Minimum percentage of the recommended daily budget. const BUDGET_MIN_PERCENT = 0.3; /** @@ -29,20 +29,22 @@ const validateCampaign = ( values, opts ) => { if ( Number.isFinite( values?.amount ) && - Number.isFinite( opts?.dailyBudget ) + Number.isFinite( opts?.dailyBudget ) && + typeof opts?.formatNumber === 'function' ) { const { amount } = values; - const { dailyBudget } = opts; + const { dailyBudget, currency, formatNumber } = opts; const minAmount = Math.round( dailyBudget * BUDGET_MIN_PERCENT, 2 ); if ( amount < minAmount ) { errors.amount = sprintf( - /* translators: %s: minimum daily budget */ + /* translators: %1$s: minimum daily budget, %2$s: currency */ __( - 'Please make sure daily average cost is greater than %s.', + 'Please make sure daily average cost is greater than %1$s %2$s.', 'google-listings-and-ads' ), - minAmount + formatNumber( minAmount ), + currency ); } } diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index c03d4bd0ac..2a18d3ffa0 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -74,23 +74,29 @@ describe( 'validateCampaign', () => { } ); it( 'When a budget is provided and the amount is less than the minimum, should not pass', () => { + const mockFormatNumber = jest.fn().mockReturnValue( '30' ); values.amount = 10; const opts = { dailyBudget: 100, + formatNumber: mockFormatNumber, + currency: 'MUR', }; const errors = validateCampaign( values, opts ); expect( errors ).toHaveProperty( 'amount' ); - expect( errors.amount ).toContain( 'at least 30' ); + expect( errors.amount ).toContain( 'is greater than 30 MUR' ); } ); it( 'When a budget is provided and the amount is greater than the minimum, should pass', () => { + const mockFormatNumber = jest.fn().mockReturnValue( '35' ); values.amount = 35; const opts = { dailyBudget: 100, + formatNumber: mockFormatNumber, + currency: 'MUR', }; const errors = validateCampaign( values, opts ); diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index c5081d20d8..f8557aa2d4 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -17,6 +17,7 @@ import validateCampaign from '.~/components/paid-ads/validateCampaign'; import clientSession from './clientSession'; import { GOOGLE_ADS_BILLING_STATUS } from '.~/constants'; import getHighestBudget from '.~/utils/getHighestBudget'; +import useStoreCurrency from '.~/hooks/useStoreCurrency'; /** * @typedef {import('.~/data/actions').CountryCode} CountryCode @@ -62,6 +63,7 @@ export default function PaidAdsSetupSections( { countryCodes, } ) { const { billingStatus } = useGoogleAdsAccountBillingStatus(); + const { code: currency, formatNumber } = useStoreCurrency(); const onStatesReceivedRef = useRef(); onStatesReceivedRef.current = onStatesReceived; @@ -119,6 +121,8 @@ export default function PaidAdsSetupSections( { const formOpts = { dailyBudget: budget?.daily_budget, + currency, + formatNumber, }; return ( From f73150b13722c3f7d484dfbdf848b441b63542c5 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 17 Sep 2024 21:05:49 +0400 Subject: [PATCH 14/50] Update E2E test. --- tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js index ee06dd1fea..69935b9157 100644 --- a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js +++ b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js @@ -229,7 +229,7 @@ test.describe( 'Complete your campaign', () => { .locator( '.components-base-control__help' ) .textContent(); await expect( error ).toBe( - 'Please make sure daily average cost is at least 30.' + 'Please make sure daily average cost is greater than 30.' ); } ); From f1fd7657df34e412e482da14c85b0e86ec8d3059 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 17 Sep 2024 21:45:49 +0400 Subject: [PATCH 15/50] Fix E2E test. --- tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js index 69935b9157..6ef81fecbd 100644 --- a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js +++ b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js @@ -229,7 +229,7 @@ test.describe( 'Complete your campaign', () => { .locator( '.components-base-control__help' ) .textContent(); await expect( error ).toBe( - 'Please make sure daily average cost is greater than 30.' + 'Please make sure daily average cost is greater than 30.00 USD.' ); } ); From 7adc092e41c8059d324d14bf527840b785cc4ff8 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 17 Sep 2024 22:16:24 +0400 Subject: [PATCH 16/50] Fix e2e test. --- tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js index 6ef81fecbd..f3e2dabed5 100644 --- a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js +++ b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js @@ -219,6 +219,8 @@ test.describe( 'Complete your campaign', () => { }, ], } ); + + await completeCampaign.goto(); } ); test( 'should see validation error if lower than the 30%', async () => { From 90e8d95cf35df8fd738ae7976fddd696865acc6a Mon Sep 17 00:00:00 2001 From: asvinb Date: Wed, 18 Sep 2024 20:32:33 +0400 Subject: [PATCH 17/50] Add useValidateCampaignWithCountryCodes hook. --- .../budget-recommendation/index.js | 2 +- .../paid-ads/campaign-assets-form.js | 15 ++++++- .../useFetchBudgetRecommendationEffect.js | 2 +- .../hooks/useHighestBudgetRecommendation.js | 31 ++++++++++++++ .../useValidateCampaignWithCountryCodes.js | 42 +++++++++++++++++++ .../setup-paid-ads/paid-ads-setup-sections.js | 21 ++-------- 6 files changed, 92 insertions(+), 21 deletions(-) rename js/src/{components/paid-ads/budget-section/budget-recommendation => hooks}/useFetchBudgetRecommendationEffect.js (92%) create mode 100644 js/src/hooks/useHighestBudgetRecommendation.js create mode 100644 js/src/hooks/useValidateCampaignWithCountryCodes.js diff --git a/js/src/components/paid-ads/budget-section/budget-recommendation/index.js b/js/src/components/paid-ads/budget-section/budget-recommendation/index.js index 2492685c36..25f400285a 100644 --- a/js/src/components/paid-ads/budget-section/budget-recommendation/index.js +++ b/js/src/components/paid-ads/budget-section/budget-recommendation/index.js @@ -10,7 +10,7 @@ import GridiconNoticeOutline from 'gridicons/dist/notice-outline'; * Internal dependencies */ import useCountryKeyNameMap from '.~/hooks/useCountryKeyNameMap'; -import useFetchBudgetRecommendationEffect from './useFetchBudgetRecommendationEffect'; +import useFetchBudgetRecommendationEffect from '.~/hooks/useFetchBudgetRecommendationEffect'; import './index.scss'; /* diff --git a/js/src/components/paid-ads/campaign-assets-form.js b/js/src/components/paid-ads/campaign-assets-form.js index b1578c2023..5e317e5d23 100644 --- a/js/src/components/paid-ads/campaign-assets-form.js +++ b/js/src/components/paid-ads/campaign-assets-form.js @@ -8,8 +8,8 @@ import { useState, useMemo } from '@wordpress/element'; */ import { ASSET_GROUP_KEY, ASSET_FORM_KEY } from '.~/constants'; import AdaptiveForm from '.~/components/adaptive-form'; -import validateCampaign from '.~/components/paid-ads/validateCampaign'; import validateAssetGroup from '.~/components/paid-ads/validateAssetGroup'; +import useValidateCampaignWithCountryCodes from '.~/hooks/useValidateCampaignWithCountryCodes'; /** * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues @@ -64,11 +64,13 @@ function convertAssetEntityGroupToFormValues( assetEntityGroup = {} ) { * @augments AdaptiveForm * @param {Object} props React props. * @param {CampaignFormValues} props.initialCampaign Initial campaign values. + * @param {Function} [props.onChange] Callback when the form values change. * @param {AssetEntityGroup} [props.assetEntityGroup] The asset entity group to be used in initializing the form values for editing. */ export default function CampaignAssetsForm( { initialCampaign, assetEntityGroup, + onChange, ...adaptiveFormProps } ) { const initialAssetGroup = useMemo( () => { @@ -77,6 +79,14 @@ export default function CampaignAssetsForm( { const [ baseAssetGroup, setBaseAssetGroup ] = useState( initialAssetGroup ); const [ hasImportedAssets, setHasImportedAssets ] = useState( false ); + const [ countryCodes, setCountryCodes ] = useState( [] ); + const { validateCampaignWithCountryCodes } = + useValidateCampaignWithCountryCodes( countryCodes ); + + const handleOnChange = ( _, values, arg ) => { + setCountryCodes( values.countryCodes ); + onChange( _, values, arg ); + }; const extendAdapter = ( formContext ) => { const assetGroupErrors = validateAssetGroup( formContext.values ); @@ -120,8 +130,9 @@ export default function CampaignAssetsForm( { ...initialCampaign, ...initialAssetGroup, } } - validate={ validateCampaign } + validate={ validateCampaignWithCountryCodes } extendAdapter={ extendAdapter } + onChange={ handleOnChange } { ...adaptiveFormProps } /> ); diff --git a/js/src/components/paid-ads/budget-section/budget-recommendation/useFetchBudgetRecommendationEffect.js b/js/src/hooks/useFetchBudgetRecommendationEffect.js similarity index 92% rename from js/src/components/paid-ads/budget-section/budget-recommendation/useFetchBudgetRecommendationEffect.js rename to js/src/hooks/useFetchBudgetRecommendationEffect.js index 6d63fad522..cd65322188 100644 --- a/js/src/components/paid-ads/budget-section/budget-recommendation/useFetchBudgetRecommendationEffect.js +++ b/js/src/hooks/useFetchBudgetRecommendationEffect.js @@ -7,7 +7,7 @@ import { addQueryArgs } from '@wordpress/url'; * Internal dependencies */ import { API_NAMESPACE } from '.~/data/constants'; -import useApiFetchEffect from '.~/hooks/useApiFetchEffect'; +import useApiFetchEffect from './useApiFetchEffect'; /** * @typedef { import(".~/data/actions").CountryCode } CountryCode diff --git a/js/src/hooks/useHighestBudgetRecommendation.js b/js/src/hooks/useHighestBudgetRecommendation.js new file mode 100644 index 0000000000..02b5835e79 --- /dev/null +++ b/js/src/hooks/useHighestBudgetRecommendation.js @@ -0,0 +1,31 @@ +/** + * Internal dependencies + */ +import useFetchBudgetRecommendationEffect from './useFetchBudgetRecommendationEffect'; +import getHighestBudget from '.~/utils/getHighestBudget'; +import useStoreCurrency from './useStoreCurrency'; + +/** + * @typedef { import(".~/data/actions").CountryCode } CountryCode + */ + +/** + * Fetch the highest budget recommendation for countries in a side effect. + * + * @param {Array} countryCodes Country code array. + * @return {Object} Budget recommendation. + */ +const useHighestBudgetRecommendation = ( countryCodes ) => { + const { code: currency, formatNumber } = useStoreCurrency(); + const { data: budgetData } = + useFetchBudgetRecommendationEffect( countryCodes ); + const budget = getHighestBudget( budgetData?.recommendations ); + + return { + dailyBudget: budget?.daily_budget, + currency, + formatNumber, + }; +}; + +export default useHighestBudgetRecommendation; diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js new file mode 100644 index 0000000000..9f0e49ab80 --- /dev/null +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -0,0 +1,42 @@ +/** + * Internal dependencies + */ +import useHighestBudgetRecommendation from './useHighestBudgetRecommendation'; +import validateCampaign from '.~/components/paid-ads/validateCampaign'; + +/** + * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues + */ + +/** + * @typedef { import(".~/data/actions").CountryCode } CountryCode + */ + +/** + * Validate campaign form. Accepts the form values object and returns errors object. + * + * @param {Array} countryCodes Country code array. + * @return {Object} errors. + */ +const useValidateCampaignWithCountryCodes = ( countryCodes ) => { + const { currency, dailyBudget, formatNumber } = + useHighestBudgetRecommendation( countryCodes ); + + /** + * Validate campaign form. Accepts the form values object and returns errors object. + * + * @param {CampaignFormValues} values Campaign form values. + * @return {Object} errors. + */ + const validateCampaignWithCountryCodes = ( values ) => { + return validateCampaign( values, { + currency, + dailyBudget, + formatNumber, + } ); + }; + + return { validateCampaignWithCountryCodes }; +}; + +export default useValidateCampaignWithCountryCodes; diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index f8557aa2d4..236c1fba5c 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -8,7 +8,6 @@ import { Form } from '@woocommerce/components'; * Internal dependencies */ import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus'; -import useFetchBudgetRecommendationEffect from '.~/components/paid-ads/budget-section/budget-recommendation/useFetchBudgetRecommendationEffect'; import BudgetSection from '.~/components/paid-ads/budget-section'; import BillingCard from '.~/components/paid-ads/billing-card'; import SpinnerCard from '.~/components/spinner-card'; @@ -16,8 +15,7 @@ import Section from '.~/wcdl/section'; import validateCampaign from '.~/components/paid-ads/validateCampaign'; import clientSession from './clientSession'; import { GOOGLE_ADS_BILLING_STATUS } from '.~/constants'; -import getHighestBudget from '.~/utils/getHighestBudget'; -import useStoreCurrency from '.~/hooks/useStoreCurrency'; +import useValidateCampaignWithCountryCodes from '.~/hooks/useValidateCampaignWithCountryCodes'; /** * @typedef {import('.~/data/actions').CountryCode} CountryCode @@ -62,8 +60,9 @@ export default function PaidAdsSetupSections( { onStatesReceived, countryCodes, } ) { + const { validateCampaignWithCountryCodes } = + useValidateCampaignWithCountryCodes( countryCodes ); const { billingStatus } = useGoogleAdsAccountBillingStatus(); - const { code: currency, formatNumber } = useStoreCurrency(); const onStatesReceivedRef = useRef(); onStatesReceivedRef.current = onStatesReceived; @@ -77,10 +76,6 @@ export default function PaidAdsSetupSections( { return resolveInitialPaidAds( startingPaidAds ); } ); - const { data: budgetData } = - useFetchBudgetRecommendationEffect( countryCodes ); - const budget = getHighestBudget( budgetData?.recommendations ); - const isBillingCompleted = billingStatus?.status === GOOGLE_ADS_BILLING_STATUS.APPROVED; @@ -119,21 +114,13 @@ export default function PaidAdsSetupSections( { amount: paidAds.amount, }; - const formOpts = { - dailyBudget: budget?.daily_budget, - currency, - formatNumber, - }; - return (
{ setPaidAds( { ...paidAds, ...values, isValid } ); } } - validate={ ( formValues ) => - validateCampaign( formValues, formOpts ) - } + validate={ validateCampaignWithCountryCodes } > { ( formProps ) => { return ( From 811461faee885b2e8cba55b292755d342a8fb98c Mon Sep 17 00:00:00 2001 From: asvinb Date: Wed, 18 Sep 2024 20:44:56 +0400 Subject: [PATCH 18/50] Fix unit test. --- .../paid-ads/campaign-assets-form.test.js | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/js/src/components/paid-ads/campaign-assets-form.test.js b/js/src/components/paid-ads/campaign-assets-form.test.js index 19339b09b7..c9f4faad6a 100644 --- a/js/src/components/paid-ads/campaign-assets-form.test.js +++ b/js/src/components/paid-ads/campaign-assets-form.test.js @@ -10,6 +10,34 @@ import userEvent from '@testing-library/user-event'; */ import CampaignAssetsForm from './campaign-assets-form'; +jest.mock( '@woocommerce/settings', () => ( { + getSetting: jest + .fn() + .mockName( "getSetting( 'currency' )" ) + .mockReturnValue( { + code: 'EUR', + symbol: '€', + precision: 2, + decimalSeparator: '.', + thousandSeparator: ',', + priceFormat: '%1$s %2$s', + } ), +} ) ); + +jest.mock( '.~/hooks/useApiFetchCallback', () => ( { + __esModule: true, + default: jest.fn().mockImplementation( () => { + return [ jest.fn(), null ]; + } ), +} ) ); + +jest.mock( '.~/hooks/useFetchBudgetRecommendationEffect', () => ( { + __esModule: true, + default: jest.fn().mockImplementation( () => { + return [ jest.fn(), null ]; + } ), +} ) ); + const alwaysValid = () => ( {} ); describe( 'CampaignAssetsForm', () => { From eb70d33b996f43eb48ce5da786d0c5de57ce8bdf Mon Sep 17 00:00:00 2001 From: asvinb Date: Thu, 19 Sep 2024 11:57:19 +0400 Subject: [PATCH 19/50] Do not trigger fetch if there are no countryCodes. --- .../useFetchBudgetRecommendationEffect.js | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/js/src/hooks/useFetchBudgetRecommendationEffect.js b/js/src/hooks/useFetchBudgetRecommendationEffect.js index cd65322188..b111992277 100644 --- a/js/src/hooks/useFetchBudgetRecommendationEffect.js +++ b/js/src/hooks/useFetchBudgetRecommendationEffect.js @@ -19,11 +19,19 @@ import useApiFetchEffect from './useApiFetchEffect'; * @param {Array} countryCodes Country code array. * @return {Object} Budget recommendation. */ -const useFetchBudgetRecommendationEffect = ( countryCodes ) => { - const url = `${ API_NAMESPACE }/ads/campaigns/budget-recommendation`; - const query = { country_codes: countryCodes }; - const path = addQueryArgs( url, query ); - return useApiFetchEffect( { path } ); +const useFetchBudgetRecommendationEffect = ( countryCodes = [] ) => { + let args; + + // If there are no country codes, undefined will be passed to useApiFetchEffect which won't trigger the fetch. + if ( countryCodes.length > 0 ) { + const url = `${ API_NAMESPACE }/ads/campaigns/budget-recommendation`; + const query = { country_codes: countryCodes }; + const path = addQueryArgs( url, query ); + + args = { path }; + } + + return useApiFetchEffect( args ); }; export default useFetchBudgetRecommendationEffect; From a502b7a8ccea2773ff0e12568fcee37b9e8cf0cf Mon Sep 17 00:00:00 2001 From: asvinb Date: Thu, 19 Sep 2024 18:53:57 +0400 Subject: [PATCH 20/50] Add loading property to know when the hook is ready. --- .../components/paid-ads/validateCampaign.js | 28 +++++---- .../hooks/useHighestBudgetRecommendation.js | 3 +- .../useValidateCampaignWithCountryCodes.js | 4 +- .../paid-ads-setup-sections/index.js | 54 +++++++++++++++++ .../paid-ads-setup-form.js} | 59 ++++++++----------- 5 files changed, 99 insertions(+), 49 deletions(-) create mode 100644 js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/index.js rename js/src/setup-mc/setup-stepper/setup-paid-ads/{paid-ads-setup-sections.js => paid-ads-setup-sections/paid-ads-setup-form.js} (71%) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 6f9d0cf1ea..efc9a0e304 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -21,10 +21,12 @@ const validateCampaign = ( values, opts ) => { const errors = {}; if ( ! Number.isFinite( values.amount ) || values.amount <= 0 ) { - errors.amount = __( - 'Please make sure daily average cost is greater than 0.', - 'google-listings-and-ads' - ); + return { + amount: __( + 'Please make sure daily average cost is greater than 0.', + 'google-listings-and-ads' + ), + }; } if ( @@ -37,15 +39,17 @@ const validateCampaign = ( values, opts ) => { const minAmount = Math.round( dailyBudget * BUDGET_MIN_PERCENT, 2 ); if ( amount < minAmount ) { - errors.amount = sprintf( - /* translators: %1$s: minimum daily budget, %2$s: currency */ - __( - 'Please make sure daily average cost is greater than %1$s %2$s.', - 'google-listings-and-ads' + return { + amount: sprintf( + /* translators: %1$s: minimum daily budget, %2$s: currency */ + __( + 'Please make sure daily average cost is greater than %1$s %2$s.', + 'google-listings-and-ads' + ), + formatNumber( minAmount ), + currency ), - formatNumber( minAmount ), - currency - ); + }; } } diff --git a/js/src/hooks/useHighestBudgetRecommendation.js b/js/src/hooks/useHighestBudgetRecommendation.js index 02b5835e79..7d1399c98e 100644 --- a/js/src/hooks/useHighestBudgetRecommendation.js +++ b/js/src/hooks/useHighestBudgetRecommendation.js @@ -17,7 +17,7 @@ import useStoreCurrency from './useStoreCurrency'; */ const useHighestBudgetRecommendation = ( countryCodes ) => { const { code: currency, formatNumber } = useStoreCurrency(); - const { data: budgetData } = + const { data: budgetData, loading } = useFetchBudgetRecommendationEffect( countryCodes ); const budget = getHighestBudget( budgetData?.recommendations ); @@ -25,6 +25,7 @@ const useHighestBudgetRecommendation = ( countryCodes ) => { dailyBudget: budget?.daily_budget, currency, formatNumber, + loading, }; }; diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 9f0e49ab80..ab9bc8aa57 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -19,7 +19,7 @@ import validateCampaign from '.~/components/paid-ads/validateCampaign'; * @return {Object} errors. */ const useValidateCampaignWithCountryCodes = ( countryCodes ) => { - const { currency, dailyBudget, formatNumber } = + const { currency, dailyBudget, formatNumber, loading } = useHighestBudgetRecommendation( countryCodes ); /** @@ -36,7 +36,7 @@ const useValidateCampaignWithCountryCodes = ( countryCodes ) => { } ); }; - return { validateCampaignWithCountryCodes }; + return { validateCampaignWithCountryCodes, loading }; }; export default useValidateCampaignWithCountryCodes; diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/index.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/index.js new file mode 100644 index 0000000000..28b8bacfd9 --- /dev/null +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/index.js @@ -0,0 +1,54 @@ +/** + * Internal dependencies + */ +import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus'; +import SpinnerCard from '.~/components/spinner-card'; +import Section from '.~/wcdl/section'; +import useValidateCampaignWithCountryCodes from '.~/hooks/useValidateCampaignWithCountryCodes'; +import PaidAdsSetupForm from './paid-ads-setup-form'; + +/** + * @typedef {import('.~/data/actions').CountryCode} CountryCode + */ + +/** + * @typedef {Object} PaidAdsData + * @property {number|undefined} amount Daily average cost of the paid ads campaign. + * @property {boolean} isValid Whether the campaign data are valid values. + * @property {boolean} isReady Whether the campaign data and the billing setting are ready for completing the paid ads setup. + */ + +/** + * Renders sections of Google Ads account, budget and billing for setting up the paid ads. + * Waits for the validate campaign with country codes to be loaded before rendering the form. + * + * @param {Object} props React props. + * @param {(onStatesReceived: PaidAdsData)=>void} props.onStatesReceived Callback to receive the data for setting up paid ads when initial and also when the budget and billing are updated. + * @param {Array|undefined} props.countryCodes Country codes for the campaign. + */ +export default function PaidAdsSetupSections( { + onStatesReceived, + countryCodes, +} ) { + const { + loading: loadingValidateCampaignWithCountryCodes, + validateCampaignWithCountryCodes, + } = useValidateCampaignWithCountryCodes( countryCodes ); + const { billingStatus } = useGoogleAdsAccountBillingStatus(); + + if ( ! billingStatus || loadingValidateCampaignWithCountryCodes ) { + return ( +
+ +
+ ); + } + + return ( + + ); +} diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/paid-ads-setup-form.js similarity index 71% rename from js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js rename to js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/paid-ads-setup-form.js index 236c1fba5c..8116678d56 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/paid-ads-setup-form.js @@ -10,12 +10,12 @@ import { Form } from '@woocommerce/components'; import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus'; import BudgetSection from '.~/components/paid-ads/budget-section'; import BillingCard from '.~/components/paid-ads/billing-card'; -import SpinnerCard from '.~/components/spinner-card'; -import Section from '.~/wcdl/section'; -import validateCampaign from '.~/components/paid-ads/validateCampaign'; -import clientSession from './clientSession'; +import clientSession from '../clientSession'; import { GOOGLE_ADS_BILLING_STATUS } from '.~/constants'; -import useValidateCampaignWithCountryCodes from '.~/hooks/useValidateCampaignWithCountryCodes'; + +/** + * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues + */ /** * @typedef {import('.~/data/actions').CountryCode} CountryCode @@ -34,39 +34,38 @@ const defaultPaidAds = { isReady: false, }; -/** - * Resolve the initial paid ads data from the given paid ads data. - * Parts of the resolved data are used in the `initialValues` prop of `Form` component. - * - * @param {PaidAdsData} paidAds The paid ads data as the base to be resolved with other states. - * @return {PaidAdsData} The resolved paid ads data. - */ -function resolveInitialPaidAds( paidAds ) { - const nextPaidAds = { ...paidAds }; - nextPaidAds.isValid = ! Object.keys( validateCampaign( nextPaidAds ) ) - .length; - - return nextPaidAds; -} - /** * Renders sections of Google Ads account, budget and billing for setting up the paid ads. * * @param {Object} props React props. - * @param {(onStatesReceived: PaidAdsData)=>void} props.onStatesReceived Callback to receive the data for setting up paid ads when initial and also when the budget and billing are updated. + * @param {(onStatesReceived: PaidAdsData) => void} props.onStatesReceived Callback to receive the data for setting up paid ads when initial and also when the budget and billing are updated. * @param {Array|undefined} props.countryCodes Country codes for the campaign. + * @param {(values: CampaignFormValues) => Object} props.validateCampaign Function to validate campaign form values. */ -export default function PaidAdsSetupSections( { +export default function PaidAdsSetupForm( { onStatesReceived, countryCodes, + validateCampaign, } ) { - const { validateCampaignWithCountryCodes } = - useValidateCampaignWithCountryCodes( countryCodes ); const { billingStatus } = useGoogleAdsAccountBillingStatus(); - const onStatesReceivedRef = useRef(); onStatesReceivedRef.current = onStatesReceived; + /** + * Resolve the initial paid ads data from the given paid ads data. + * Parts of the resolved data are used in the `initialValues` prop of `Form` component. + * + * @param {PaidAdsData} paidAds The paid ads data as the base to be resolved with other states. + * @return {PaidAdsData} The resolved paid ads data. + */ + function resolveInitialPaidAds( paidAds ) { + const nextPaidAds = { ...paidAds }; + nextPaidAds.isValid = ! Object.keys( validateCampaign( nextPaidAds ) ) + .length; + + return nextPaidAds; + } + const [ paidAds, setPaidAds ] = useState( () => { // Resolve the starting paid ads data with the campaign data stored in the client session. const startingPaidAds = { @@ -102,14 +101,6 @@ export default function PaidAdsSetupSections( { clientSession.setCampaign( nextPaidAds ); }, [ paidAds, isBillingCompleted ] ); - if ( ! billingStatus ) { - return ( -
- -
- ); - } - const initialValues = { amount: paidAds.amount, }; @@ -120,7 +111,7 @@ export default function PaidAdsSetupSections( { onChange={ ( _, values, isValid ) => { setPaidAds( { ...paidAds, ...values, isValid } ); } } - validate={ validateCampaignWithCountryCodes } + validate={ validateCampaign } > { ( formProps ) => { return ( From 34c58a9cc69fad7f152c3a8853e8cc386e3e5669 Mon Sep 17 00:00:00 2001 From: asvinb Date: Thu, 19 Sep 2024 19:02:11 +0400 Subject: [PATCH 21/50] Remove condition to display PaidAdsSetupSections. --- .../setup-stepper/setup-paid-ads/setup-paid-ads.js | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js index ed715e04f5..cd5f77ac13 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/setup-paid-ads.js @@ -171,14 +171,10 @@ export default function SetupPaidAds() { - - { countryCodes && ( - - ) } - + { showSkipPaidAdsConfirmationModal && ( From 4d80b454792ca48231e8e1cebaa1bf7ab8d4385e Mon Sep 17 00:00:00 2001 From: asvinb Date: Mon, 23 Sep 2024 22:50:24 +0400 Subject: [PATCH 22/50] Add getAdsBudgetRecommendations selector. --- .../paid-ads/campaign-assets-form.test.js | 7 --- js/src/components/types.js | 6 +++ js/src/data/action-types.js | 1 + js/src/data/actions.js | 25 +++++++++ js/src/data/reducer.js | 14 +++++ js/src/data/resolvers.js | 45 +++++++++++++++- js/src/data/selectors.js | 20 ++++++- js/src/data/utils.js | 19 +++++++ .../useFetchBudgetRecommendationEffect.js | 8 ++- .../hooks/useHighestBudgetRecommendation.js | 7 +-- ...tup-form.js => paid-ads-setup-sections.js} | 39 +++++++++----- .../paid-ads-setup-sections/index.js | 54 ------------------- 12 files changed, 165 insertions(+), 80 deletions(-) rename js/src/setup-mc/setup-stepper/setup-paid-ads/{paid-ads-setup-sections/paid-ads-setup-form.js => paid-ads-setup-sections.js} (75%) delete mode 100644 js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/index.js diff --git a/js/src/components/paid-ads/campaign-assets-form.test.js b/js/src/components/paid-ads/campaign-assets-form.test.js index c9f4faad6a..0b7afddda2 100644 --- a/js/src/components/paid-ads/campaign-assets-form.test.js +++ b/js/src/components/paid-ads/campaign-assets-form.test.js @@ -24,13 +24,6 @@ jest.mock( '@woocommerce/settings', () => ( { } ), } ) ); -jest.mock( '.~/hooks/useApiFetchCallback', () => ( { - __esModule: true, - default: jest.fn().mockImplementation( () => { - return [ jest.fn(), null ]; - } ), -} ) ); - jest.mock( '.~/hooks/useFetchBudgetRecommendationEffect', () => ( { __esModule: true, default: jest.fn().mockImplementation( () => { diff --git a/js/src/components/types.js b/js/src/components/types.js index fa7214953f..070663a099 100644 --- a/js/src/components/types.js +++ b/js/src/components/types.js @@ -8,6 +8,12 @@ * @property {number} amount The daily average cost amount. */ +/** + * @typedef {Object} ValidateCampaignOptions + * @property {string} Currency symbol to be used (e.g., 'USD'). + * @property {number} dailyBudget Daily budget for the campaign. + */ + /** * @typedef {Object} AssetGroupFormValues * @property {string | null} final_url - The page URL on merchant's website that people reach when they click the ad. diff --git a/js/src/data/action-types.js b/js/src/data/action-types.js index 96173fa461..18d72e0eec 100644 --- a/js/src/data/action-types.js +++ b/js/src/data/action-types.js @@ -50,6 +50,7 @@ const TYPES = { UPSERT_TOUR: 'UPSERT_TOUR', HYDRATE_PREFETCHED_DATA: 'HYDRATE_PREFETCHED_DATA', RECEIVE_GOOGLE_ADS_ACCOUNT_STATUS: 'RECEIVE_GOOGLE_ADS_ACCOUNT_STATUS', + RECEIVE_ADS_BUDGET_RECOMMENDATIONS: 'RECEIVE_ADS_BUDGET_RECOMMENDATIONS', }; export default TYPES; diff --git a/js/src/data/actions.js b/js/src/data/actions.js index 2b67eb9b33..a0ebd5fc29 100644 --- a/js/src/data/actions.js +++ b/js/src/data/actions.js @@ -1287,3 +1287,28 @@ export function* fetchGoogleAdsAccountStatus() { ); } } + +/** + * Action to receive ad budget recommendations. + * + * This function constructs an action object containing the type of the action, + * the provided country codes, currency, and recommendations. + * + * @param {string} countryCodesKey String identifying the country codes. + * @param {string} currency The currency code representing the currency of the recommendations. + * @param {Object} recommendations The budget recommendations object. + * @param {CountryCode} recommendations.country The country-specific recommendations. + * @param {number} recommendations.daily_budget The daily budget recommendation for the country. + */ +export function* receiveAdsBudgetRecommendations( + countryCodesKey, + currency, + recommendations +) { + return { + type: TYPES.RECEIVE_ADS_BUDGET_RECOMMENDATIONS, + countryCodesKey, + currency, + recommendations, + }; +} diff --git a/js/src/data/reducer.js b/js/src/data/reducer.js index 29fc5573dd..2b4f437d23 100644 --- a/js/src/data/reducer.js +++ b/js/src/data/reducer.js @@ -70,6 +70,7 @@ const DEFAULT_STATE = { inviteLink: null, step: null, }, + budgetRecommendations: {}, }, }; @@ -510,6 +511,19 @@ const reducer = ( state = DEFAULT_STATE, action ) => { .end(); } + case TYPES.RECEIVE_ADS_BUDGET_RECOMMENDATIONS: { + const { countryCodesKey, currency, recommendations } = action; + + return setIn( + state, + [ 'ads', 'budgetRecommendations', countryCodesKey ], + { + currency, + recommendations, + } + ); + } + // Page will be reloaded after all accounts have been disconnected, so no need to mutate state. case TYPES.DISCONNECT_ACCOUNTS_ALL: default: diff --git a/js/src/data/resolvers.js b/js/src/data/resolvers.js index 87d1d3b0da..c4e0a232ea 100644 --- a/js/src/data/resolvers.js +++ b/js/src/data/resolvers.js @@ -15,7 +15,7 @@ import { } from '.~/constants'; import TYPES from './action-types'; import { API_NAMESPACE } from './constants'; -import { getReportKey } from './utils'; +import { getReportKey, getCountryCodesKey } from './utils'; import { handleApiError } from '.~/utils/handleError'; import { adaptAdsCampaign, adaptAssetGroup } from './adapters'; import { fetchWithHeaders, awaitPromise } from './controls'; @@ -46,8 +46,15 @@ import { receiveMappingRules, receiveStoreCategories, receiveTour, + receiveAdsBudgetRecommendations, } from './actions'; +/** + * CountryCode + * + * @typedef {string} CountryCode Two-letter country code in ISO 3166-1 alpha-2 format. Example: 'US'. + */ + export function* getShippingRates() { yield fetchShippingRates(); } @@ -534,3 +541,39 @@ export function* getGoogleAdsAccountStatus() { getGoogleAdsAccountStatus.shouldInvalidate = ( action ) => { return action.type === TYPES.DISCONNECT_ACCOUNTS_GOOGLE_ADS; }; + +/** + * Fetch ad budget recommendations for the specified country codes. + * + * @param {Array} countryCodes An array of country codes for which to fetch budget recommendations. + */ +export function* getAdsBudgetRecommendations( countryCodes ) { + if ( ! countryCodes || ! countryCodes.length ) { + return; + } + + const countryCodesKey = getCountryCodesKey( countryCodes ); + const endpoint = `${ API_NAMESPACE }/ads/campaigns/budget-recommendation`; + const query = { country_codes: countryCodes }; + const path = addQueryArgs( endpoint, query ); + + try { + const response = yield apiFetch( { + path, + } ); + + yield receiveAdsBudgetRecommendations( + countryCodesKey, + response.currency, + response.recommendations + ); + } catch ( error ) { + handleApiError( + error, + __( + 'There was an error getting the budget recommendation.', + 'google-listings-and-ads' + ) + ); + } +} diff --git a/js/src/data/selectors.js b/js/src/data/selectors.js index fc6edb39f0..94a884df6b 100644 --- a/js/src/data/selectors.js +++ b/js/src/data/selectors.js @@ -8,7 +8,12 @@ import createSelector from 'rememo'; * Internal dependencies */ import { STORE_KEY } from './constants'; -import { getReportQuery, getReportKey, getPerformanceQuery } from './utils'; +import { + getReportQuery, + getReportKey, + getPerformanceQuery, + getCountryCodesKey, +} from './utils'; /** * @typedef {import('.~/data/actions').CountryCode} CountryCode @@ -410,3 +415,16 @@ export const getTour = ( state, tourId ) => { export const getGoogleAdsAccountStatus = ( state ) => { return state.ads.accountStatus; }; + +/** + * Retrieves ad budget recommendations for provided country codes. + * If no recommendations are found, it returns `null`. + * + * @param {Object} state The state + * @param {Array} countryCodes - An array of country code strings used to generate a unique key. + * @return {Object|null} The recommendations. It will be `null` if not yet fetched or fetched but doesn't exist. + */ +export const getAdsBudgetRecommendations = ( state, countryCodes ) => { + const key = getCountryCodesKey( countryCodes ); + return state.ads.budgetRecommendations[ key ] || null; +}; diff --git a/js/src/data/utils.js b/js/src/data/utils.js index 752c22efdf..f51650baf6 100644 --- a/js/src/data/utils.js +++ b/js/src/data/utils.js @@ -9,6 +9,10 @@ import { getCurrentDates } from '@woocommerce/date'; */ import round from '.~/utils/round'; +/** + * @typedef { import(".~/data/actions").CountryCode } CountryCode + */ + export const freeFields = [ 'clicks', 'impressions' ]; export const paidFields = [ 'sales', 'conversions', 'spend', ...freeFields ]; /** @@ -167,6 +171,21 @@ export function calculateDelta( value, base ) { return delta; } +/** + * Generates a unique key (slug) from an array of country codes. + * + * This function sorts the array of country codes alphabetically, + * joins them into a single string with underscore (`_`), and converts + * the result to lowercase. + * + * @param {Array} countryCodes - An array of country code strings. + * + * @return {string} A underscore-separated, lowercase string representing the sorted country codes. + */ +export function getCountryCodesKey( countryCodes ) { + return countryCodes.sort().join( '_' ).toLowerCase(); +} + /** * Report fields fetched from report API. * diff --git a/js/src/hooks/useFetchBudgetRecommendationEffect.js b/js/src/hooks/useFetchBudgetRecommendationEffect.js index b111992277..ad3d3a081e 100644 --- a/js/src/hooks/useFetchBudgetRecommendationEffect.js +++ b/js/src/hooks/useFetchBudgetRecommendationEffect.js @@ -16,8 +16,12 @@ import useApiFetchEffect from './useApiFetchEffect'; /** * Fetch the budget recommendation for a country in a side effect. * - * @param {Array} countryCodes Country code array. - * @return {Object} Budget recommendation. + * Although `countryCodes` is optional and defaults to an empty array, + * it eventually requires a non-empty array of country codes to return valid results. + * If `countryCodes` is empty, no fetch will be triggered, and undefined will be returned. + * + * @param {Array} [countryCodes=[]] An array of country codes. If empty, the fetch will not be triggered. + * @return {Object} Budget recommendation, or `undefined` if no valid country codes are provided. */ const useFetchBudgetRecommendationEffect = ( countryCodes = [] ) => { let args; diff --git a/js/src/hooks/useHighestBudgetRecommendation.js b/js/src/hooks/useHighestBudgetRecommendation.js index 7d1399c98e..f6d1752be3 100644 --- a/js/src/hooks/useHighestBudgetRecommendation.js +++ b/js/src/hooks/useHighestBudgetRecommendation.js @@ -3,7 +3,7 @@ */ import useFetchBudgetRecommendationEffect from './useFetchBudgetRecommendationEffect'; import getHighestBudget from '.~/utils/getHighestBudget'; -import useStoreCurrency from './useStoreCurrency'; +import useAdsCurrency from './useAdsCurrency'; /** * @typedef { import(".~/data/actions").CountryCode } CountryCode @@ -16,7 +16,8 @@ import useStoreCurrency from './useStoreCurrency'; * @return {Object} Budget recommendation. */ const useHighestBudgetRecommendation = ( countryCodes ) => { - const { code: currency, formatNumber } = useStoreCurrency(); + const { adsCurrencyConfig } = useAdsCurrency(); + const { code: currency } = adsCurrencyConfig; const { data: budgetData, loading } = useFetchBudgetRecommendationEffect( countryCodes ); const budget = getHighestBudget( budgetData?.recommendations ); @@ -24,8 +25,8 @@ const useHighestBudgetRecommendation = ( countryCodes ) => { return { dailyBudget: budget?.daily_budget, currency, - formatNumber, loading, + budgetData, }; }; diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/paid-ads-setup-form.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js similarity index 75% rename from js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/paid-ads-setup-form.js rename to js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index 8116678d56..89f5ce4ec3 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/paid-ads-setup-form.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -8,15 +8,14 @@ import { Form } from '@woocommerce/components'; * Internal dependencies */ import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus'; +import SpinnerCard from '.~/components/spinner-card'; +import Section from '.~/wcdl/section'; +import useValidateCampaignWithCountryCodes from '.~/hooks/useValidateCampaignWithCountryCodes'; import BudgetSection from '.~/components/paid-ads/budget-section'; import BillingCard from '.~/components/paid-ads/billing-card'; -import clientSession from '../clientSession'; +import clientSession from './clientSession'; import { GOOGLE_ADS_BILLING_STATUS } from '.~/constants'; -/** - * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues - */ - /** * @typedef {import('.~/data/actions').CountryCode} CountryCode */ @@ -36,18 +35,24 @@ const defaultPaidAds = { /** * Renders sections of Google Ads account, budget and billing for setting up the paid ads. + * Waits for the validate campaign with country codes to be loaded before rendering the form. * * @param {Object} props React props. - * @param {(onStatesReceived: PaidAdsData) => void} props.onStatesReceived Callback to receive the data for setting up paid ads when initial and also when the budget and billing are updated. + * @param {(onStatesReceived: PaidAdsData)=>void} props.onStatesReceived Callback to receive the data for setting up paid ads when initial and also when the budget and billing are updated. * @param {Array|undefined} props.countryCodes Country codes for the campaign. - * @param {(values: CampaignFormValues) => Object} props.validateCampaign Function to validate campaign form values. */ -export default function PaidAdsSetupForm( { +export default function PaidAdsSetupSections( { onStatesReceived, countryCodes, - validateCampaign, } ) { + const { + loading: loadingValidateCampaignWithCountryCodes, + validateCampaignWithCountryCodes, + budgetData, + } = useValidateCampaignWithCountryCodes( countryCodes ); + console.log( 'HELLO', useValidateCampaignWithCountryCodes( countryCodes ) ); const { billingStatus } = useGoogleAdsAccountBillingStatus(); + const onStatesReceivedRef = useRef(); onStatesReceivedRef.current = onStatesReceived; @@ -60,8 +65,9 @@ export default function PaidAdsSetupForm( { */ function resolveInitialPaidAds( paidAds ) { const nextPaidAds = { ...paidAds }; - nextPaidAds.isValid = ! Object.keys( validateCampaign( nextPaidAds ) ) - .length; + nextPaidAds.isValid = ! Object.keys( + validateCampaignWithCountryCodes( nextPaidAds ) + ).length; return nextPaidAds; } @@ -101,6 +107,14 @@ export default function PaidAdsSetupForm( { clientSession.setCampaign( nextPaidAds ); }, [ paidAds, isBillingCompleted ] ); + if ( ! billingStatus || loadingValidateCampaignWithCountryCodes ) { + return ( +
+ +
+ ); + } + const initialValues = { amount: paidAds.amount, }; @@ -111,13 +125,14 @@ export default function PaidAdsSetupForm( { onChange={ ( _, values, isValid ) => { setPaidAds( { ...paidAds, ...values, isValid } ); } } - validate={ validateCampaign } + validate={ validateCampaignWithCountryCodes } > { ( formProps ) => { return ( diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/index.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/index.js deleted file mode 100644 index 28b8bacfd9..0000000000 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections/index.js +++ /dev/null @@ -1,54 +0,0 @@ -/** - * Internal dependencies - */ -import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus'; -import SpinnerCard from '.~/components/spinner-card'; -import Section from '.~/wcdl/section'; -import useValidateCampaignWithCountryCodes from '.~/hooks/useValidateCampaignWithCountryCodes'; -import PaidAdsSetupForm from './paid-ads-setup-form'; - -/** - * @typedef {import('.~/data/actions').CountryCode} CountryCode - */ - -/** - * @typedef {Object} PaidAdsData - * @property {number|undefined} amount Daily average cost of the paid ads campaign. - * @property {boolean} isValid Whether the campaign data are valid values. - * @property {boolean} isReady Whether the campaign data and the billing setting are ready for completing the paid ads setup. - */ - -/** - * Renders sections of Google Ads account, budget and billing for setting up the paid ads. - * Waits for the validate campaign with country codes to be loaded before rendering the form. - * - * @param {Object} props React props. - * @param {(onStatesReceived: PaidAdsData)=>void} props.onStatesReceived Callback to receive the data for setting up paid ads when initial and also when the budget and billing are updated. - * @param {Array|undefined} props.countryCodes Country codes for the campaign. - */ -export default function PaidAdsSetupSections( { - onStatesReceived, - countryCodes, -} ) { - const { - loading: loadingValidateCampaignWithCountryCodes, - validateCampaignWithCountryCodes, - } = useValidateCampaignWithCountryCodes( countryCodes ); - const { billingStatus } = useGoogleAdsAccountBillingStatus(); - - if ( ! billingStatus || loadingValidateCampaignWithCountryCodes ) { - return ( -
- -
- ); - } - - return ( - - ); -} From 44d9eed9b9d77e0c2e2283d01bfdb872a0fe6ee3 Mon Sep 17 00:00:00 2001 From: asvinb Date: Mon, 23 Sep 2024 23:33:09 +0400 Subject: [PATCH 23/50] Update hooks. --- .../budget-recommendation/index.js | 3 +- .../paid-ads/campaign-assets-form.js | 3 +- .../components/paid-ads/validateCampaign.js | 15 ++++---- .../paid-ads/validateCampaign.test.js | 12 +++---- js/src/data/resolvers.js | 16 ++++++--- js/src/data/selectors.js | 2 +- js/src/data/test/reducer.test.js | 1 + js/src/hooks/useFetchBudgetRecommendation.js | 34 +++++++++++++++++++ .../hooks/useHighestBudgetRecommendation.js | 33 +++++++++++------- .../useValidateCampaignWithCountryCodes.js | 5 ++- .../setup-paid-ads/paid-ads-setup-sections.js | 1 - 11 files changed, 86 insertions(+), 39 deletions(-) create mode 100644 js/src/hooks/useFetchBudgetRecommendation.js diff --git a/js/src/components/paid-ads/budget-section/budget-recommendation/index.js b/js/src/components/paid-ads/budget-section/budget-recommendation/index.js index 25f400285a..9658e282f0 100644 --- a/js/src/components/paid-ads/budget-section/budget-recommendation/index.js +++ b/js/src/components/paid-ads/budget-section/budget-recommendation/index.js @@ -11,6 +11,7 @@ import GridiconNoticeOutline from 'gridicons/dist/notice-outline'; */ import useCountryKeyNameMap from '.~/hooks/useCountryKeyNameMap'; import useFetchBudgetRecommendationEffect from '.~/hooks/useFetchBudgetRecommendationEffect'; +import useFetchBudgetRecommendation from '.~/hooks/useFetchBudgetRecommendation'; import './index.scss'; /* @@ -51,7 +52,7 @@ function toRecommendationRange( isMultiple, ...values ) { const BudgetRecommendation = ( props ) => { const { countryCodes, dailyAverageCost = Infinity } = props; - const { data } = useFetchBudgetRecommendationEffect( countryCodes ); + const { data } = useFetchBudgetRecommendation( countryCodes ); const map = useCountryKeyNameMap(); if ( ! data ) { diff --git a/js/src/components/paid-ads/campaign-assets-form.js b/js/src/components/paid-ads/campaign-assets-form.js index 5e317e5d23..7a1befc0ed 100644 --- a/js/src/components/paid-ads/campaign-assets-form.js +++ b/js/src/components/paid-ads/campaign-assets-form.js @@ -2,6 +2,7 @@ * External dependencies */ import { useState, useMemo } from '@wordpress/element'; +import { noop } from 'lodash'; /** * Internal dependencies @@ -70,7 +71,7 @@ function convertAssetEntityGroupToFormValues( assetEntityGroup = {} ) { export default function CampaignAssetsForm( { initialCampaign, assetEntityGroup, - onChange, + onChange = noop, ...adaptiveFormProps } ) { const initialAssetGroup = useMemo( () => { diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index efc9a0e304..0b7ed15bcf 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -5,6 +5,7 @@ import { __, sprintf } from '@wordpress/i18n'; /** * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues + * @typedef {import('.~/components/types.js').ValidateCampaignOptions} ValidateCampaignOptions */ // Minimum percentage of the recommended daily budget. @@ -14,7 +15,7 @@ const BUDGET_MIN_PERCENT = 0.3; * Validate campaign form. Accepts the form values object and returns errors object. * * @param {CampaignFormValues} values Campaign form values. - * @param {Object} opts Extra form options. + * @param {ValidateCampaignOptions} opts Extra form options. * @return {Object} errors. */ const validateCampaign = ( values, opts ) => { @@ -31,23 +32,21 @@ const validateCampaign = ( values, opts ) => { if ( Number.isFinite( values?.amount ) && - Number.isFinite( opts?.dailyBudget ) && - typeof opts?.formatNumber === 'function' + Number.isFinite( opts?.dailyBudget ) ) { const { amount } = values; - const { dailyBudget, currency, formatNumber } = opts; + const { dailyBudget, formatAmount } = opts; const minAmount = Math.round( dailyBudget * BUDGET_MIN_PERCENT, 2 ); if ( amount < minAmount ) { return { amount: sprintf( - /* translators: %1$s: minimum daily budget, %2$s: currency */ + /* translators: %1$s: minimum daily budget */ __( - 'Please make sure daily average cost is greater than %1$s %2$s.', + 'Please make sure daily average cost is greater than %s.', 'google-listings-and-ads' ), - formatNumber( minAmount ), - currency + formatAmount( minAmount ) ), }; } diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index 2a18d3ffa0..e15f9c1ebb 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -74,29 +74,27 @@ describe( 'validateCampaign', () => { } ); it( 'When a budget is provided and the amount is less than the minimum, should not pass', () => { - const mockFormatNumber = jest.fn().mockReturnValue( '30' ); + const mockFormatAmount = jest.fn().mockReturnValue( 'Rs 30' ); values.amount = 10; const opts = { dailyBudget: 100, - formatNumber: mockFormatNumber, - currency: 'MUR', + formatAmount: mockFormatAmount, }; const errors = validateCampaign( values, opts ); expect( errors ).toHaveProperty( 'amount' ); - expect( errors.amount ).toContain( 'is greater than 30 MUR' ); + expect( errors.amount ).toContain( 'is greater than Rs 30' ); } ); it( 'When a budget is provided and the amount is greater than the minimum, should pass', () => { - const mockFormatNumber = jest.fn().mockReturnValue( '35' ); + const mockFormatAmount = jest.fn().mockReturnValue( 'Rs 35' ); values.amount = 35; const opts = { dailyBudget: 100, - formatNumber: mockFormatNumber, - currency: 'MUR', + formatAmount: mockFormatAmount, }; const errors = validateCampaign( values, opts ); diff --git a/js/src/data/resolvers.js b/js/src/data/resolvers.js index c4e0a232ea..9037251888 100644 --- a/js/src/data/resolvers.js +++ b/js/src/data/resolvers.js @@ -558,16 +558,24 @@ export function* getAdsBudgetRecommendations( countryCodes ) { const path = addQueryArgs( endpoint, query ); try { - const response = yield apiFetch( { + const { data } = yield fetchWithHeaders( { path, } ); yield receiveAdsBudgetRecommendations( countryCodesKey, - response.currency, - response.recommendations + data.currency, + data.recommendations ); - } catch ( error ) { + } catch ( response ) { + // Intentionally silence the specific in case the no budget recommendations are found from the API. + if ( response.status === 404 ) { + return; + } + + const bodyPromise = response?.json() || response?.text(); + const error = yield awaitPromise( bodyPromise ); + handleApiError( error, __( diff --git a/js/src/data/selectors.js b/js/src/data/selectors.js index 94a884df6b..ab4498159f 100644 --- a/js/src/data/selectors.js +++ b/js/src/data/selectors.js @@ -424,7 +424,7 @@ export const getGoogleAdsAccountStatus = ( state ) => { * @param {Array} countryCodes - An array of country code strings used to generate a unique key. * @return {Object|null} The recommendations. It will be `null` if not yet fetched or fetched but doesn't exist. */ -export const getAdsBudgetRecommendations = ( state, countryCodes ) => { +export const getAdsBudgetRecommendations = ( state, countryCodes = [] ) => { const key = getCountryCodesKey( countryCodes ); return state.ads.budgetRecommendations[ key ] || null; }; diff --git a/js/src/data/test/reducer.test.js b/js/src/data/test/reducer.test.js index 54e9ffdb0b..e3e56a3157 100644 --- a/js/src/data/test/reducer.test.js +++ b/js/src/data/test/reducer.test.js @@ -73,6 +73,7 @@ describe( 'reducer', () => { inviteLink: null, step: null, }, + budgetRecommendations: {}, }, } ); diff --git a/js/src/hooks/useFetchBudgetRecommendation.js b/js/src/hooks/useFetchBudgetRecommendation.js new file mode 100644 index 0000000000..5b1a4b1c2e --- /dev/null +++ b/js/src/hooks/useFetchBudgetRecommendation.js @@ -0,0 +1,34 @@ +/** + * External dependencies + */ +import { useSelect } from '@wordpress/data'; + +/** + * Internal dependencies + */ +import { STORE_KEY } from '.~/data/constants'; + +/** + * @typedef { import(".~/data/actions").CountryCode } CountryCode + */ + +/** + * Fetch the highest budget recommendation for countries in a side effect. + * + * @param {Array} countryCodes An array of country codes. If empty, the fetch will not be triggered. + * @return {Object} Budget recommendation. + */ +const useFetchBudgetRecommendation = ( countryCodes ) => { + return useSelect( ( select ) => { + const { getAdsBudgetRecommendations, isResolving } = + select( STORE_KEY ); + + const data = getAdsBudgetRecommendations( countryCodes ); + return { + loading: isResolving( 'getAdsBudgetRecommendations' ), + data, + }; + } ); +}; + +export default useFetchBudgetRecommendation; diff --git a/js/src/hooks/useHighestBudgetRecommendation.js b/js/src/hooks/useHighestBudgetRecommendation.js index f6d1752be3..fc54860ef4 100644 --- a/js/src/hooks/useHighestBudgetRecommendation.js +++ b/js/src/hooks/useHighestBudgetRecommendation.js @@ -1,7 +1,12 @@ +/** + * External dependencies + */ +import { useSelect } from '@wordpress/data'; + /** * Internal dependencies */ -import useFetchBudgetRecommendationEffect from './useFetchBudgetRecommendationEffect'; +import { STORE_KEY } from '.~/data/constants'; import getHighestBudget from '.~/utils/getHighestBudget'; import useAdsCurrency from './useAdsCurrency'; @@ -12,22 +17,24 @@ import useAdsCurrency from './useAdsCurrency'; /** * Fetch the highest budget recommendation for countries in a side effect. * - * @param {Array} countryCodes Country code array. + * @param {Array} countryCodes An array of country codes. If empty, the dailyBudget will be null. * @return {Object} Budget recommendation. */ const useHighestBudgetRecommendation = ( countryCodes ) => { - const { adsCurrencyConfig } = useAdsCurrency(); - const { code: currency } = adsCurrencyConfig; - const { data: budgetData, loading } = - useFetchBudgetRecommendationEffect( countryCodes ); - const budget = getHighestBudget( budgetData?.recommendations ); + const { formatAmount } = useAdsCurrency(); + + return useSelect( ( select ) => { + const { getAdsBudgetRecommendations, isResolving } = + select( STORE_KEY ); - return { - dailyBudget: budget?.daily_budget, - currency, - loading, - budgetData, - }; + const budgetData = getAdsBudgetRecommendations( countryCodes ); + const budget = getHighestBudget( budgetData?.recommendations ); + return { + dailyBudget: budget?.daily_budget, + formatAmount, + loading: isResolving( 'getAdsBudgetRecommendations' ), + }; + } ); }; export default useHighestBudgetRecommendation; diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index ab9bc8aa57..5e4a0ced22 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -19,7 +19,7 @@ import validateCampaign from '.~/components/paid-ads/validateCampaign'; * @return {Object} errors. */ const useValidateCampaignWithCountryCodes = ( countryCodes ) => { - const { currency, dailyBudget, formatNumber, loading } = + const { dailyBudget, formatAmount, loading } = useHighestBudgetRecommendation( countryCodes ); /** @@ -30,9 +30,8 @@ const useValidateCampaignWithCountryCodes = ( countryCodes ) => { */ const validateCampaignWithCountryCodes = ( values ) => { return validateCampaign( values, { - currency, dailyBudget, - formatNumber, + formatAmount, } ); }; diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index 89f5ce4ec3..beeda77ac8 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -50,7 +50,6 @@ export default function PaidAdsSetupSections( { validateCampaignWithCountryCodes, budgetData, } = useValidateCampaignWithCountryCodes( countryCodes ); - console.log( 'HELLO', useValidateCampaignWithCountryCodes( countryCodes ) ); const { billingStatus } = useGoogleAdsAccountBillingStatus(); const onStatesReceivedRef = useRef(); From 29b69208b56aeb1de661c0b8e5f55bda4342a9b1 Mon Sep 17 00:00:00 2001 From: asvinb Date: Mon, 23 Sep 2024 23:39:01 +0400 Subject: [PATCH 24/50] Add JSDocs. --- js/src/hooks/useValidateCampaignWithCountryCodes.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 5e4a0ced22..c7f91212a5 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -15,8 +15,10 @@ import validateCampaign from '.~/components/paid-ads/validateCampaign'; /** * Validate campaign form. Accepts the form values object and returns errors object. * - * @param {Array} countryCodes Country code array. - * @return {Object} errors. + * @param {Array} countryCodes Country code array. If not provided, the validate function will not take into account budget recommendations. + * @return {Object} An object containing the `validateCampaignWithCountryCodes` function and a `loading` state. + * @property {Function} validateCampaignWithCountryCodes A function to validate campaign form values. + * @property {boolean} loading A boolean indicating whether the budget recommendation data is being fetched. */ const useValidateCampaignWithCountryCodes = ( countryCodes ) => { const { dailyBudget, formatAmount, loading } = From a1a5cf44004c1bc46108005c74e2c86cb8699f42 Mon Sep 17 00:00:00 2001 From: asvinb Date: Mon, 23 Sep 2024 23:41:36 +0400 Subject: [PATCH 25/50] More descriptive JSDocs. --- js/src/hooks/useValidateCampaignWithCountryCodes.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index c7f91212a5..7db54b8b2b 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -28,7 +28,7 @@ const useValidateCampaignWithCountryCodes = ( countryCodes ) => { * Validate campaign form. Accepts the form values object and returns errors object. * * @param {CampaignFormValues} values Campaign form values. - * @return {Object} errors. + * @return {Object} An object containing any validation errors. If no errors, the object will be empty. */ const validateCampaignWithCountryCodes = ( values ) => { return validateCampaign( values, { From a7a2eba9634a7b29091278daa4e60bf82355e52f Mon Sep 17 00:00:00 2001 From: asvinb Date: Mon, 23 Sep 2024 23:43:16 +0400 Subject: [PATCH 26/50] Fix linting errors. --- .../paid-ads/budget-section/budget-recommendation/index.js | 1 - js/src/components/paid-ads/campaign-assets-form.test.js | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/js/src/components/paid-ads/budget-section/budget-recommendation/index.js b/js/src/components/paid-ads/budget-section/budget-recommendation/index.js index 9658e282f0..bdca6b0250 100644 --- a/js/src/components/paid-ads/budget-section/budget-recommendation/index.js +++ b/js/src/components/paid-ads/budget-section/budget-recommendation/index.js @@ -10,7 +10,6 @@ import GridiconNoticeOutline from 'gridicons/dist/notice-outline'; * Internal dependencies */ import useCountryKeyNameMap from '.~/hooks/useCountryKeyNameMap'; -import useFetchBudgetRecommendationEffect from '.~/hooks/useFetchBudgetRecommendationEffect'; import useFetchBudgetRecommendation from '.~/hooks/useFetchBudgetRecommendation'; import './index.scss'; diff --git a/js/src/components/paid-ads/campaign-assets-form.test.js b/js/src/components/paid-ads/campaign-assets-form.test.js index 0b7afddda2..acdcb11d35 100644 --- a/js/src/components/paid-ads/campaign-assets-form.test.js +++ b/js/src/components/paid-ads/campaign-assets-form.test.js @@ -24,7 +24,7 @@ jest.mock( '@woocommerce/settings', () => ( { } ), } ) ); -jest.mock( '.~/hooks/useFetchBudgetRecommendationEffect', () => ( { +jest.mock( '.~/hooks/useFetchBudgetRecommendation', () => ( { __esModule: true, default: jest.fn().mockImplementation( () => { return [ jest.fn(), null ]; From 684fd0289a2f718ec497e0a81719c01ca9d474d3 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 10:38:29 +0400 Subject: [PATCH 27/50] Apply change. --- js/src/components/paid-ads/validateCampaign.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 0b7ed15bcf..5e56018ea5 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -36,7 +36,7 @@ const validateCampaign = ( values, opts ) => { ) { const { amount } = values; const { dailyBudget, formatAmount } = opts; - const minAmount = Math.round( dailyBudget * BUDGET_MIN_PERCENT, 2 ); + const minAmount = Math.round( dailyBudget * BUDGET_MIN_PERCENT ); if ( amount < minAmount ) { return { From 447bff0750c11ae395b7b2d6d7b53c06e750067a Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 12:49:18 +0400 Subject: [PATCH 28/50] Add JSdocs for returned value. --- .../components/paid-ads/validateCampaign.js | 22 +++++++++---------- .../hooks/useHighestBudgetRecommendation.js | 8 +++++-- 2 files changed, 17 insertions(+), 13 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 5e56018ea5..803d544262 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -21,18 +21,9 @@ const BUDGET_MIN_PERCENT = 0.3; const validateCampaign = ( values, opts ) => { const errors = {}; - if ( ! Number.isFinite( values.amount ) || values.amount <= 0 ) { - return { - amount: __( - 'Please make sure daily average cost is greater than 0.', - 'google-listings-and-ads' - ), - }; - } - if ( - Number.isFinite( values?.amount ) && - Number.isFinite( opts?.dailyBudget ) + Number.isFinite( values.amount ) && + Number.isFinite( opts.dailyBudget ) ) { const { amount } = values; const { dailyBudget, formatAmount } = opts; @@ -52,6 +43,15 @@ const validateCampaign = ( values, opts ) => { } } + if ( ! Number.isFinite( values.amount ) || values.amount <= 0 ) { + return { + amount: __( + 'Please make sure daily average cost is greater than 0.', + 'google-listings-and-ads' + ), + }; + } + return errors; }; diff --git a/js/src/hooks/useHighestBudgetRecommendation.js b/js/src/hooks/useHighestBudgetRecommendation.js index fc54860ef4..2236eabb64 100644 --- a/js/src/hooks/useHighestBudgetRecommendation.js +++ b/js/src/hooks/useHighestBudgetRecommendation.js @@ -17,8 +17,11 @@ import useAdsCurrency from './useAdsCurrency'; /** * Fetch the highest budget recommendation for countries in a side effect. * - * @param {Array} countryCodes An array of country codes. If empty, the dailyBudget will be null. - * @return {Object} Budget recommendation. + * @param {Array} countryCodes An array of country codes. If empty, the dailyBudget will be undefined. + * @return {Object} An object containing the `dailyBudget` value, `formatAmount` function and a `loading` state. + * @property {number|null} dailyBudget The highest recommended daily budget. If no recommendations are available, this will be `undefined`. + * @property {boolean} loading A boolean indicating whether the budget recommendation data is being fetched. + * @property {Function} formatAmount A function to format the budget amount according to the currency settings. */ const useHighestBudgetRecommendation = ( countryCodes ) => { const { formatAmount } = useAdsCurrency(); @@ -29,6 +32,7 @@ const useHighestBudgetRecommendation = ( countryCodes ) => { const budgetData = getAdsBudgetRecommendations( countryCodes ); const budget = getHighestBudget( budgetData?.recommendations ); + return { dailyBudget: budget?.daily_budget, formatAmount, From c85bdf19298affbf5d152d275c03416b326ca82f Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 13:02:01 +0400 Subject: [PATCH 29/50] Fix JS errors. --- .../paid-ads/validateCampaign.test.js | 27 ++++++++++++------- .../setup-mc/step-4-complete-campaign.test.js | 2 +- 2 files changed, 18 insertions(+), 11 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index e15f9c1ebb..7fe2e9e7ba 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -9,6 +9,10 @@ import validateCampaign from './validateCampaign'; */ describe( 'validateCampaign', () => { let values; + const validateCampaignOptions = { + dailyBudget: undefined, + formatAmount: jest.mock(), + }; beforeEach( () => { // Initial values @@ -16,15 +20,18 @@ describe( 'validateCampaign', () => { } ); it( 'When all checks are passed, should return an empty object', () => { - const errors = validateCampaign( { - amount: 1, - } ); + const errors = validateCampaign( + { + amount: 1, + }, + validateCampaignOptions + ); expect( errors ).toStrictEqual( {} ); } ); it( 'should indicate multiple unpassed checks by setting properties in the returned object', () => { - const errors = validateCampaign( values ); + const errors = validateCampaign( values, validateCampaignOptions ); expect( errors ).toHaveProperty( 'amount' ); } ); @@ -33,25 +40,25 @@ describe( 'validateCampaign', () => { let errors; values.amount = ''; - errors = validateCampaign( values ); + errors = validateCampaign( values, validateCampaignOptions ); expect( errors ).toHaveProperty( 'amount' ); expect( errors.amount ).toMatchSnapshot(); values.amount = undefined; - errors = validateCampaign( values ); + errors = validateCampaign( values, validateCampaignOptions ); expect( errors ).toHaveProperty( 'amount' ); expect( errors.amount ).toMatchSnapshot(); values.amount = new Date(); - errors = validateCampaign( values ); + errors = validateCampaign( values, validateCampaignOptions ); expect( errors ).toHaveProperty( 'amount' ); expect( errors.amount ).toMatchSnapshot(); values.amount = NaN; - errors = validateCampaign( values ); + errors = validateCampaign( values, validateCampaignOptions ); expect( errors ).toHaveProperty( 'amount' ); expect( errors.amount ).toMatchSnapshot(); @@ -61,13 +68,13 @@ describe( 'validateCampaign', () => { let errors; values.amount = 0; - errors = validateCampaign( values ); + errors = validateCampaign( values, validateCampaignOptions ); expect( errors ).toHaveProperty( 'amount' ); expect( errors.amount ).toMatchSnapshot(); values.amount = -0.01; - errors = validateCampaign( values ); + errors = validateCampaign( values, validateCampaignOptions ); expect( errors ).toHaveProperty( 'amount' ); expect( errors.amount ).toMatchSnapshot(); diff --git a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js index 2cb6e9afa5..c7d737dcef 100644 --- a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js +++ b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js @@ -231,7 +231,7 @@ test.describe( 'Complete your campaign', () => { .locator( '.components-base-control__help' ) .textContent(); await expect( error ).toBe( - 'Please make sure daily average cost is greater than 30.00 USD.' + 'Please make sure daily average cost is greater than NT$30.00.' ); } ); From 74e0dd8905f802a66774465ed5e770fa30f91738 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 13:14:48 +0400 Subject: [PATCH 30/50] Fix e2e test. --- .../add-paid-campaigns.test.js | 35 +++++++++++++------ 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js index 805f77e798..dff72794f5 100644 --- a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js +++ b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js @@ -366,20 +366,33 @@ test.describe( 'Set up Ads account', () => { ); } ); - test( 'Set the budget', async () => { - budget = '0'; - await setupBudgetPage.fillBudget( budget ); + test.describe( 'Set the budget', async () => { + test( 'Continue button should be disabled if budget is 0', async () => { + budget = '0'; + await setupBudgetPage.fillBudget( budget ); - await expect( - page.getByRole( 'button', { name: 'Continue' } ) - ).toBeDisabled(); + await expect( + page.getByRole( 'button', { name: 'Continue' } ) + ).toBeDisabled(); + } ); - budget = '1'; - await setupBudgetPage.fillBudget( budget ); + test( 'Continue button should be disabled if budget is less than recommended value', async () => { + budget = '2'; + await setupBudgetPage.fillBudget( budget ); - await expect( - page.getByRole( 'button', { name: 'Continue' } ) - ).toBeEnabled(); + await expect( + page.getByRole( 'button', { name: 'Continue' } ) + ).toBeDisabled(); + } ); + + test( 'Continue button should be enabled if budget is above the recommended value', async () => { + budget = '5'; + await setupBudgetPage.fillBudget( budget ); + + await expect( + page.getByRole( 'button', { name: 'Continue' } ) + ).toBeEnabled(); + } ); } ); test( 'Budget Recommendation', async () => { From 6fc4a6b5f926838221cae56b69e500405d8906ab Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 17:09:33 +0400 Subject: [PATCH 31/50] Address CR feedback. --- js/src/components/types.js | 4 +-- js/src/data/actions.js | 32 +----------------- js/src/data/resolvers.js | 14 ++++---- js/src/data/selectors.js | 2 +- js/src/data/test/reducer.test.js | 33 +++++++++++++++++++ js/src/data/utils.js | 5 ++- js/src/hooks/useFetchBudgetRecommendation.js | 21 ++++++------ .../hooks/useHighestBudgetRecommendation.js | 29 +++++++++------- .../useValidateCampaignWithCountryCodes.js | 16 +++++---- .../setup-paid-ads/paid-ads-setup-sections.js | 6 ++-- 10 files changed, 87 insertions(+), 75 deletions(-) diff --git a/js/src/components/types.js b/js/src/components/types.js index 070663a099..844e737457 100644 --- a/js/src/components/types.js +++ b/js/src/components/types.js @@ -10,8 +10,8 @@ /** * @typedef {Object} ValidateCampaignOptions - * @property {string} Currency symbol to be used (e.g., 'USD'). - * @property {number} dailyBudget Daily budget for the campaign. + * @property {number | undefined} dailyBudget Daily budget for the campaign. + * @property {Function} formatAmount A function to format the budget amount according to the currency settings. */ /** diff --git a/js/src/data/actions.js b/js/src/data/actions.js index a0ebd5fc29..2846bee326 100644 --- a/js/src/data/actions.js +++ b/js/src/data/actions.js @@ -19,15 +19,10 @@ import { isWCIos, isWCAndroid } from '.~/utils/isMobileApp'; /** * @typedef {import('.~/data/types.js').AssetEntityGroupUpdateBody} AssetEntityGroupUpdateBody + * @typedef {import('.~/data/actions').CountryCode} CountryCode * @typedef {import('./selectors').Tour} Tour */ -/** - * CountryCode - * - * @typedef {string} CountryCode Two-letter country code in ISO 3166-1 alpha-2 format. Example: 'US'. - */ - /** * Individual shipping rate. * @@ -1287,28 +1282,3 @@ export function* fetchGoogleAdsAccountStatus() { ); } } - -/** - * Action to receive ad budget recommendations. - * - * This function constructs an action object containing the type of the action, - * the provided country codes, currency, and recommendations. - * - * @param {string} countryCodesKey String identifying the country codes. - * @param {string} currency The currency code representing the currency of the recommendations. - * @param {Object} recommendations The budget recommendations object. - * @param {CountryCode} recommendations.country The country-specific recommendations. - * @param {number} recommendations.daily_budget The daily budget recommendation for the country. - */ -export function* receiveAdsBudgetRecommendations( - countryCodesKey, - currency, - recommendations -) { - return { - type: TYPES.RECEIVE_ADS_BUDGET_RECOMMENDATIONS, - countryCodesKey, - currency, - recommendations, - }; -} diff --git a/js/src/data/resolvers.js b/js/src/data/resolvers.js index 9037251888..20646697b2 100644 --- a/js/src/data/resolvers.js +++ b/js/src/data/resolvers.js @@ -46,7 +46,6 @@ import { receiveMappingRules, receiveStoreCategories, receiveTour, - receiveAdsBudgetRecommendations, } from './actions'; /** @@ -545,7 +544,7 @@ getGoogleAdsAccountStatus.shouldInvalidate = ( action ) => { /** * Fetch ad budget recommendations for the specified country codes. * - * @param {Array} countryCodes An array of country codes for which to fetch budget recommendations. + * @param {Array} [countryCodes] An array of country codes for which to fetch budget recommendations. */ export function* getAdsBudgetRecommendations( countryCodes ) { if ( ! countryCodes || ! countryCodes.length ) { @@ -562,11 +561,14 @@ export function* getAdsBudgetRecommendations( countryCodes ) { path, } ); - yield receiveAdsBudgetRecommendations( + const { currency, recommendations } = data; + + return { + type: TYPES.RECEIVE_ADS_BUDGET_RECOMMENDATIONS, countryCodesKey, - data.currency, - data.recommendations - ); + currency, + recommendations, + }; } catch ( response ) { // Intentionally silence the specific in case the no budget recommendations are found from the API. if ( response.status === 404 ) { diff --git a/js/src/data/selectors.js b/js/src/data/selectors.js index ab4498159f..8abb0c266a 100644 --- a/js/src/data/selectors.js +++ b/js/src/data/selectors.js @@ -421,7 +421,7 @@ export const getGoogleAdsAccountStatus = ( state ) => { * If no recommendations are found, it returns `null`. * * @param {Object} state The state - * @param {Array} countryCodes - An array of country code strings used to generate a unique key. + * @param {Array} [countryCodes] - An array of country code strings used to generate a unique key. * @return {Object|null} The recommendations. It will be `null` if not yet fetched or fetched but doesn't exist. */ export const getAdsBudgetRecommendations = ( state, countryCodes = [] ) => { diff --git a/js/src/data/test/reducer.test.js b/js/src/data/test/reducer.test.js index e3e56a3157..9b6ece1f24 100644 --- a/js/src/data/test/reducer.test.js +++ b/js/src/data/test/reducer.test.js @@ -867,6 +867,39 @@ describe( 'reducer', () => { } ); } ); + describe( 'Ads Budget Recommendations', () => { + const path = 'ads.budgetRecommendations'; + + it( 'should receive a budget recommendation', () => { + const recommendation = { + countryCodesKey: 'mu_sg', + currency: 'MUR', + recommendations: [ + { + country: 'MU', + daily_budget: 15, + }, + { + country: 'SG', + daily_budget: 10, + }, + ], + }; + + const action = { + type: TYPES.RECEIVE_ADS_BUDGET_RECOMMENDATIONS, + ...recommendation, + }; + const state = reducer( prepareState(), action ); + + state.assertConsistentRef(); + expect( state ).toHaveProperty( `${ path }.mu_sg`, { + currency: recommendation.currency, + recommendations: recommendation.recommendations, + } ); + } ); + } ); + describe( 'Remaining actions simply update the data payload to the specific path of state and return the updated state', () => { // The readability is better than applying the formatting here. /* eslint-disable prettier/prettier */ diff --git a/js/src/data/utils.js b/js/src/data/utils.js index f51650baf6..3d132cb736 100644 --- a/js/src/data/utils.js +++ b/js/src/data/utils.js @@ -179,11 +179,10 @@ export function calculateDelta( value, base ) { * the result to lowercase. * * @param {Array} countryCodes - An array of country code strings. - * * @return {string} A underscore-separated, lowercase string representing the sorted country codes. */ -export function getCountryCodesKey( countryCodes ) { - return countryCodes.sort().join( '_' ).toLowerCase(); +export function getCountryCodesKey( countryCodes = [] ) { + return [ ...countryCodes ].sort().join( '_' ).toLowerCase(); } /** diff --git a/js/src/hooks/useFetchBudgetRecommendation.js b/js/src/hooks/useFetchBudgetRecommendation.js index 5b1a4b1c2e..636a47de43 100644 --- a/js/src/hooks/useFetchBudgetRecommendation.js +++ b/js/src/hooks/useFetchBudgetRecommendation.js @@ -15,20 +15,21 @@ import { STORE_KEY } from '.~/data/constants'; /** * Fetch the highest budget recommendation for countries in a side effect. * - * @param {Array} countryCodes An array of country codes. If empty, the fetch will not be triggered. + * @param {Array} [countryCodes] An array of country codes. If empty, the fetch will not be triggered. * @return {Object} Budget recommendation. */ const useFetchBudgetRecommendation = ( countryCodes ) => { - return useSelect( ( select ) => { - const { getAdsBudgetRecommendations, isResolving } = - select( STORE_KEY ); + return useSelect( + ( select ) => { + const { getAdsBudgetRecommendations } = select( STORE_KEY ); - const data = getAdsBudgetRecommendations( countryCodes ); - return { - loading: isResolving( 'getAdsBudgetRecommendations' ), - data, - }; - } ); + const data = getAdsBudgetRecommendations( countryCodes ); + return { + data, + }; + }, + [ countryCodes ] + ); }; export default useFetchBudgetRecommendation; diff --git a/js/src/hooks/useHighestBudgetRecommendation.js b/js/src/hooks/useHighestBudgetRecommendation.js index 2236eabb64..99e0b9e4c2 100644 --- a/js/src/hooks/useHighestBudgetRecommendation.js +++ b/js/src/hooks/useHighestBudgetRecommendation.js @@ -17,7 +17,7 @@ import useAdsCurrency from './useAdsCurrency'; /** * Fetch the highest budget recommendation for countries in a side effect. * - * @param {Array} countryCodes An array of country codes. If empty, the dailyBudget will be undefined. + * @param {Array} [countryCodes] An array of country codes. If empty, the dailyBudget will be undefined. * @return {Object} An object containing the `dailyBudget` value, `formatAmount` function and a `loading` state. * @property {number|null} dailyBudget The highest recommended daily budget. If no recommendations are available, this will be `undefined`. * @property {boolean} loading A boolean indicating whether the budget recommendation data is being fetched. @@ -26,19 +26,24 @@ import useAdsCurrency from './useAdsCurrency'; const useHighestBudgetRecommendation = ( countryCodes ) => { const { formatAmount } = useAdsCurrency(); - return useSelect( ( select ) => { - const { getAdsBudgetRecommendations, isResolving } = - select( STORE_KEY ); + return useSelect( + ( select ) => { + const { getAdsBudgetRecommendations, hasFinishedResolution } = + select( STORE_KEY ); - const budgetData = getAdsBudgetRecommendations( countryCodes ); - const budget = getHighestBudget( budgetData?.recommendations ); + const budgetData = getAdsBudgetRecommendations( countryCodes ); + const budget = getHighestBudget( budgetData?.recommendations ); - return { - dailyBudget: budget?.daily_budget, - formatAmount, - loading: isResolving( 'getAdsBudgetRecommendations' ), - }; - } ); + return { + dailyBudget: budget?.daily_budget, + formatAmount, + hasFinishedResolution: hasFinishedResolution( + 'getAdsBudgetRecommendations' + ), + }; + }, + [ countryCodes, formatAmount ] + ); }; export default useHighestBudgetRecommendation; diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 7db54b8b2b..69a80abf01 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -12,16 +12,20 @@ import validateCampaign from '.~/components/paid-ads/validateCampaign'; * @typedef { import(".~/data/actions").CountryCode } CountryCode */ +/** + * @typedef {Object} ValidateCampaignWithCountryCodesHook + * @property {(values: CampaignFormValues) => Object} validateCampaignWithCountryCodes A function to validate campaign form values. + * @property {boolean} hasFinishedResolution A boolean indicating whether the budget recommendation data has been resolved. + */ + /** * Validate campaign form. Accepts the form values object and returns errors object. * - * @param {Array} countryCodes Country code array. If not provided, the validate function will not take into account budget recommendations. - * @return {Object} An object containing the `validateCampaignWithCountryCodes` function and a `loading` state. - * @property {Function} validateCampaignWithCountryCodes A function to validate campaign form values. - * @property {boolean} loading A boolean indicating whether the budget recommendation data is being fetched. + * @param {Array} [countryCodes] Country code array. If not provided, the validate function will not take into account budget recommendations. + * @return {ValidateCampaignWithCountryCodesHook} An object containing the `validateCampaignWithCountryCodes` function and a `loading` state. */ const useValidateCampaignWithCountryCodes = ( countryCodes ) => { - const { dailyBudget, formatAmount, loading } = + const { dailyBudget, formatAmount, hasFinishedResolution } = useHighestBudgetRecommendation( countryCodes ); /** @@ -37,7 +41,7 @@ const useValidateCampaignWithCountryCodes = ( countryCodes ) => { } ); }; - return { validateCampaignWithCountryCodes, loading }; + return { validateCampaignWithCountryCodes, hasFinishedResolution }; }; export default useValidateCampaignWithCountryCodes; diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index beeda77ac8..ee41c83766 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -46,9 +46,8 @@ export default function PaidAdsSetupSections( { countryCodes, } ) { const { - loading: loadingValidateCampaignWithCountryCodes, + hasFinishedResolution: hasResolvedValidateCampaignWithCountryCodes, validateCampaignWithCountryCodes, - budgetData, } = useValidateCampaignWithCountryCodes( countryCodes ); const { billingStatus } = useGoogleAdsAccountBillingStatus(); @@ -106,7 +105,7 @@ export default function PaidAdsSetupSections( { clientSession.setCampaign( nextPaidAds ); }, [ paidAds, isBillingCompleted ] ); - if ( ! billingStatus || loadingValidateCampaignWithCountryCodes ) { + if ( ! billingStatus || ! hasResolvedValidateCampaignWithCountryCodes ) { return (
@@ -131,7 +130,6 @@ export default function PaidAdsSetupSections( { From 716581bbf1a165c55638c039e9aa84c3bcffb570 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 17:18:42 +0400 Subject: [PATCH 32/50] Add JSDocs. --- js/src/components/paid-ads/validateCampaign.js | 7 ++++++- js/src/components/types.js | 6 ------ js/src/hooks/useHighestBudgetRecommendation.js | 12 ++++++++---- 3 files changed, 14 insertions(+), 11 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 803d544262..239f4f5596 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -5,7 +5,12 @@ import { __, sprintf } from '@wordpress/i18n'; /** * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues - * @typedef {import('.~/components/types.js').ValidateCampaignOptions} ValidateCampaignOptions + */ + +/** + * @typedef {Object} ValidateCampaignOptions + * @property {number | undefined} dailyBudget Daily budget for the campaign. + * @property {Function} formatAmount A function to format the budget amount according to the currency settings. */ // Minimum percentage of the recommended daily budget. diff --git a/js/src/components/types.js b/js/src/components/types.js index 844e737457..fa7214953f 100644 --- a/js/src/components/types.js +++ b/js/src/components/types.js @@ -8,12 +8,6 @@ * @property {number} amount The daily average cost amount. */ -/** - * @typedef {Object} ValidateCampaignOptions - * @property {number | undefined} dailyBudget Daily budget for the campaign. - * @property {Function} formatAmount A function to format the budget amount according to the currency settings. - */ - /** * @typedef {Object} AssetGroupFormValues * @property {string | null} final_url - The page URL on merchant's website that people reach when they click the ad. diff --git a/js/src/hooks/useHighestBudgetRecommendation.js b/js/src/hooks/useHighestBudgetRecommendation.js index 99e0b9e4c2..8c29612a1d 100644 --- a/js/src/hooks/useHighestBudgetRecommendation.js +++ b/js/src/hooks/useHighestBudgetRecommendation.js @@ -14,14 +14,18 @@ import useAdsCurrency from './useAdsCurrency'; * @typedef { import(".~/data/actions").CountryCode } CountryCode */ +/** + * @typedef {Object} HighestBudgetRecommendationHook + * @property {number|undefined} dailyBudget The highest recommended daily budget. If no recommendations are available, this will be `undefined`. + * @property {boolean} hasFinishedResolution A boolean indicating whether the budget recommendation data has been fetched. + * @property {(amount: number) => string} formatAmount A function to format the budget amount according to the currency settings. + */ + /** * Fetch the highest budget recommendation for countries in a side effect. * * @param {Array} [countryCodes] An array of country codes. If empty, the dailyBudget will be undefined. - * @return {Object} An object containing the `dailyBudget` value, `formatAmount` function and a `loading` state. - * @property {number|null} dailyBudget The highest recommended daily budget. If no recommendations are available, this will be `undefined`. - * @property {boolean} loading A boolean indicating whether the budget recommendation data is being fetched. - * @property {Function} formatAmount A function to format the budget amount according to the currency settings. + * @return {HighestBudgetRecommendationHook} An object containing the `dailyBudget` value, `formatAmount` function and a `hasFinishedResolution` state. */ const useHighestBudgetRecommendation = ( countryCodes ) => { const { formatAmount } = useAdsCurrency(); From 2c4f863eacf62283a337ea2e803296d2950479af Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 21:58:23 +0400 Subject: [PATCH 33/50] Simplify hooks. --- .../paid-ads/campaign-assets-form.js | 34 ++++++-- .../components/paid-ads/validateCampaign.js | 2 +- .../hooks/useHighestBudgetRecommendation.js | 53 ----------- .../useValidateCampaignWithCountryCodes.js | 87 ++++++++++++++----- .../setup-paid-ads/paid-ads-setup-sections.js | 36 +++----- 5 files changed, 107 insertions(+), 105 deletions(-) delete mode 100644 js/src/hooks/useHighestBudgetRecommendation.js diff --git a/js/src/components/paid-ads/campaign-assets-form.js b/js/src/components/paid-ads/campaign-assets-form.js index 7a1befc0ed..01b6cbfe2e 100644 --- a/js/src/components/paid-ads/campaign-assets-form.js +++ b/js/src/components/paid-ads/campaign-assets-form.js @@ -1,7 +1,7 @@ /** * External dependencies */ -import { useState, useMemo } from '@wordpress/element'; +import { useState, useMemo, useEffect, useRef } from '@wordpress/element'; import { noop } from 'lodash'; /** @@ -74,21 +74,42 @@ export default function CampaignAssetsForm( { onChange = noop, ...adaptiveFormProps } ) { + const formRef = useRef(); const initialAssetGroup = useMemo( () => { return convertAssetEntityGroupToFormValues( assetEntityGroup ); }, [ assetEntityGroup ] ); - const [ baseAssetGroup, setBaseAssetGroup ] = useState( initialAssetGroup ); const [ hasImportedAssets, setHasImportedAssets ] = useState( false ); - const [ countryCodes, setCountryCodes ] = useState( [] ); - const { validateCampaignWithCountryCodes } = - useValidateCampaignWithCountryCodes( countryCodes ); + const { + validateCampaignWithCountryCodes, + dailyBudget, + refreshCountryCodes, + currencyCode, + } = useValidateCampaignWithCountryCodes(); + + // Grab the recommendations for the initial country codes. + useEffect( () => { + refreshCountryCodes( initialCampaign.countryCodes ); + }, [ initialCampaign.countryCodes, refreshCountryCodes ] ); const handleOnChange = ( _, values, arg ) => { - setCountryCodes( values.countryCodes ); onChange( _, values, arg ); + + // Whenever there's a change, update the country codes in the validation function. + refreshCountryCodes( values.countryCodes ); }; + useEffect( () => { + const { setValue } = formRef.current; + + // Trigger a form value change to refresh the validation function once again with the new budget values + // If the validation function and values do not change, then the validation will not be triggerred since the `validate` + // function uses useCallback and will not be re-created. + setValue( 'dailyBudget', dailyBudget ); + // Sometimes the currency code takes time to resolve and the budget data is already available. + setValue( 'currencyCode', currencyCode ); + }, [ dailyBudget, currencyCode ] ); + const extendAdapter = ( formContext ) => { const assetGroupErrors = validateAssetGroup( formContext.values ); const finalUrl = assetEntityGroup?.[ ASSET_GROUP_KEY.FINAL_URL ]; @@ -127,6 +148,7 @@ export default function CampaignAssetsForm( { return ( { if ( Number.isFinite( values.amount ) && - Number.isFinite( opts.dailyBudget ) + Number.isFinite( opts?.dailyBudget ) ) { const { amount } = values; const { dailyBudget, formatAmount } = opts; diff --git a/js/src/hooks/useHighestBudgetRecommendation.js b/js/src/hooks/useHighestBudgetRecommendation.js deleted file mode 100644 index 8c29612a1d..0000000000 --- a/js/src/hooks/useHighestBudgetRecommendation.js +++ /dev/null @@ -1,53 +0,0 @@ -/** - * External dependencies - */ -import { useSelect } from '@wordpress/data'; - -/** - * Internal dependencies - */ -import { STORE_KEY } from '.~/data/constants'; -import getHighestBudget from '.~/utils/getHighestBudget'; -import useAdsCurrency from './useAdsCurrency'; - -/** - * @typedef { import(".~/data/actions").CountryCode } CountryCode - */ - -/** - * @typedef {Object} HighestBudgetRecommendationHook - * @property {number|undefined} dailyBudget The highest recommended daily budget. If no recommendations are available, this will be `undefined`. - * @property {boolean} hasFinishedResolution A boolean indicating whether the budget recommendation data has been fetched. - * @property {(amount: number) => string} formatAmount A function to format the budget amount according to the currency settings. - */ - -/** - * Fetch the highest budget recommendation for countries in a side effect. - * - * @param {Array} [countryCodes] An array of country codes. If empty, the dailyBudget will be undefined. - * @return {HighestBudgetRecommendationHook} An object containing the `dailyBudget` value, `formatAmount` function and a `hasFinishedResolution` state. - */ -const useHighestBudgetRecommendation = ( countryCodes ) => { - const { formatAmount } = useAdsCurrency(); - - return useSelect( - ( select ) => { - const { getAdsBudgetRecommendations, hasFinishedResolution } = - select( STORE_KEY ); - - const budgetData = getAdsBudgetRecommendations( countryCodes ); - const budget = getHighestBudget( budgetData?.recommendations ); - - return { - dailyBudget: budget?.daily_budget, - formatAmount, - hasFinishedResolution: hasFinishedResolution( - 'getAdsBudgetRecommendations' - ), - }; - }, - [ countryCodes, formatAmount ] - ); -}; - -export default useHighestBudgetRecommendation; diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 69a80abf01..3d54223771 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -1,8 +1,16 @@ +/** + * External dependencies + */ +import { useSelect } from '@wordpress/data'; +import { useState, useEffect } from '@wordpress/element'; + /** * Internal dependencies */ -import useHighestBudgetRecommendation from './useHighestBudgetRecommendation'; +import { STORE_KEY } from '.~/data/constants'; +import useAdsCurrency from './useAdsCurrency'; import validateCampaign from '.~/components/paid-ads/validateCampaign'; +import getHighestBudget from '.~/utils/getHighestBudget'; /** * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues @@ -21,27 +29,66 @@ import validateCampaign from '.~/components/paid-ads/validateCampaign'; /** * Validate campaign form. Accepts the form values object and returns errors object. * - * @param {Array} [countryCodes] Country code array. If not provided, the validate function will not take into account budget recommendations. + * @param {Array} [initialCountryCodes] Country code array. If not provided, the validate function will not take into account budget recommendations. * @return {ValidateCampaignWithCountryCodesHook} An object containing the `validateCampaignWithCountryCodes` function and a `loading` state. */ -const useValidateCampaignWithCountryCodes = ( countryCodes ) => { - const { dailyBudget, formatAmount, hasFinishedResolution } = - useHighestBudgetRecommendation( countryCodes ); - - /** - * Validate campaign form. Accepts the form values object and returns errors object. - * - * @param {CampaignFormValues} values Campaign form values. - * @return {Object} An object containing any validation errors. If no errors, the object will be empty. - */ - const validateCampaignWithCountryCodes = ( values ) => { - return validateCampaign( values, { - dailyBudget, - formatAmount, - } ); - }; - - return { validateCampaignWithCountryCodes, hasFinishedResolution }; +const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { + const { + formatAmount, + adsCurrencyConfig: { code }, + } = useAdsCurrency(); + const [ countryCodes, setCountryCodes ] = useState( [] ); + + useEffect( () => { + setCountryCodes( initialCountryCodes ); + }, [ initialCountryCodes ] ); + + return useSelect( + ( select ) => { + // If no country codes are provided, return the default validateCampaign function. + if ( ! countryCodes?.length ) { + return { + validateCampaignWithCountryCodes: validateCampaign, + dailyBudget: null, + formatAmount, + setCountryCodes, + currencyCode: code, + hasFinishedResolution: true, + }; + } + + const { getAdsBudgetRecommendations, hasFinishedResolution } = + select( STORE_KEY ); + const budgetData = getAdsBudgetRecommendations( countryCodes ); + const budget = getHighestBudget( budgetData?.recommendations ); + + /** + * Validate campaign form. Accepts the form values object and returns errors object. + * + * @param {CampaignFormValues} values Campaign form values. + * @return {Object} An object containing any validation errors. If no errors, the object will be empty. + */ + const validateCampaignWithCountryCodes = ( values ) => { + return validateCampaign( values, { + dailyBudget: budget?.daily_budget, + formatAmount, + } ); + }; + + return { + validateCampaignWithCountryCodes, + dailyBudget: budget?.daily_budget, + formatAmount, + refreshCountryCodes: setCountryCodes, + currencyCode: code, + hasFinishedResolution: + hasFinishedResolution( 'getAdsBudgetRecommendations', [ + countryCodes, + ] ) && code, + }; + }, + [ countryCodes, formatAmount, code ] + ); }; export default useValidateCampaignWithCountryCodes; diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index ee41c83766..40f3111f11 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -45,38 +45,19 @@ export default function PaidAdsSetupSections( { onStatesReceived, countryCodes, } ) { - const { - hasFinishedResolution: hasResolvedValidateCampaignWithCountryCodes, - validateCampaignWithCountryCodes, - } = useValidateCampaignWithCountryCodes( countryCodes ); + const { validateCampaignWithCountryCodes, hasFinishedResolution } = + useValidateCampaignWithCountryCodes( countryCodes ); const { billingStatus } = useGoogleAdsAccountBillingStatus(); - const onStatesReceivedRef = useRef(); onStatesReceivedRef.current = onStatesReceived; - /** - * Resolve the initial paid ads data from the given paid ads data. - * Parts of the resolved data are used in the `initialValues` prop of `Form` component. - * - * @param {PaidAdsData} paidAds The paid ads data as the base to be resolved with other states. - * @return {PaidAdsData} The resolved paid ads data. - */ - function resolveInitialPaidAds( paidAds ) { - const nextPaidAds = { ...paidAds }; - nextPaidAds.isValid = ! Object.keys( - validateCampaignWithCountryCodes( nextPaidAds ) - ).length; - - return nextPaidAds; - } - const [ paidAds, setPaidAds ] = useState( () => { // Resolve the starting paid ads data with the campaign data stored in the client session. const startingPaidAds = { ...defaultPaidAds, ...clientSession.getCampaign(), }; - return resolveInitialPaidAds( startingPaidAds ); + return startingPaidAds; } ); const isBillingCompleted = @@ -97,15 +78,20 @@ export default function PaidAdsSetupSections( { For example, refresh page during onboarding flow after the billing setup is finished. */ useEffect( () => { + const isValid = ! Object.keys( + validateCampaignWithCountryCodes( paidAds ) + ).length; const nextPaidAds = { ...paidAds, - isReady: paidAds.isValid && isBillingCompleted, + isValid, + isReady: isValid && isBillingCompleted, }; + onStatesReceivedRef.current( nextPaidAds ); clientSession.setCampaign( nextPaidAds ); - }, [ paidAds, isBillingCompleted ] ); + }, [ paidAds, isBillingCompleted, validateCampaignWithCountryCodes ] ); - if ( ! billingStatus || ! hasResolvedValidateCampaignWithCountryCodes ) { + if ( ! billingStatus || ! hasFinishedResolution ) { return (
From 2d51b4a7bb40118f912690717fc57445b1ddac1e Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 22:03:49 +0400 Subject: [PATCH 34/50] Fix JS tests. --- js/src/components/paid-ads/campaign-assets-form.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/js/src/components/paid-ads/campaign-assets-form.js b/js/src/components/paid-ads/campaign-assets-form.js index 01b6cbfe2e..531c24825b 100644 --- a/js/src/components/paid-ads/campaign-assets-form.js +++ b/js/src/components/paid-ads/campaign-assets-form.js @@ -89,8 +89,10 @@ export default function CampaignAssetsForm( { // Grab the recommendations for the initial country codes. useEffect( () => { - refreshCountryCodes( initialCampaign.countryCodes ); - }, [ initialCampaign.countryCodes, refreshCountryCodes ] ); + if ( initialCampaign?.countryCodes ) { + refreshCountryCodes( initialCampaign.countryCodes ); + } + }, [ initialCampaign, refreshCountryCodes ] ); const handleOnChange = ( _, values, arg ) => { onChange( _, values, arg ); From ab6a39758f587e42e9eeebd4097a9c4a1eff6c5d Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 22:19:53 +0400 Subject: [PATCH 35/50] Rename function. --- js/src/hooks/useValidateCampaignWithCountryCodes.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 3d54223771..8df4af0aab 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -51,7 +51,7 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { validateCampaignWithCountryCodes: validateCampaign, dailyBudget: null, formatAmount, - setCountryCodes, + refreshCountryCodes: setCountryCodes, currencyCode: code, hasFinishedResolution: true, }; From edccd5725ec6289d74270c29f5954c84f95422b1 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 22:39:50 +0400 Subject: [PATCH 36/50] Delete unused file. --- .../useFetchBudgetRecommendationEffect.js | 41 ------------------- 1 file changed, 41 deletions(-) delete mode 100644 js/src/hooks/useFetchBudgetRecommendationEffect.js diff --git a/js/src/hooks/useFetchBudgetRecommendationEffect.js b/js/src/hooks/useFetchBudgetRecommendationEffect.js deleted file mode 100644 index ad3d3a081e..0000000000 --- a/js/src/hooks/useFetchBudgetRecommendationEffect.js +++ /dev/null @@ -1,41 +0,0 @@ -/** - * External dependencies - */ -import { addQueryArgs } from '@wordpress/url'; - -/** - * Internal dependencies - */ -import { API_NAMESPACE } from '.~/data/constants'; -import useApiFetchEffect from './useApiFetchEffect'; - -/** - * @typedef { import(".~/data/actions").CountryCode } CountryCode - */ - -/** - * Fetch the budget recommendation for a country in a side effect. - * - * Although `countryCodes` is optional and defaults to an empty array, - * it eventually requires a non-empty array of country codes to return valid results. - * If `countryCodes` is empty, no fetch will be triggered, and undefined will be returned. - * - * @param {Array} [countryCodes=[]] An array of country codes. If empty, the fetch will not be triggered. - * @return {Object} Budget recommendation, or `undefined` if no valid country codes are provided. - */ -const useFetchBudgetRecommendationEffect = ( countryCodes = [] ) => { - let args; - - // If there are no country codes, undefined will be passed to useApiFetchEffect which won't trigger the fetch. - if ( countryCodes.length > 0 ) { - const url = `${ API_NAMESPACE }/ads/campaigns/budget-recommendation`; - const query = { country_codes: countryCodes }; - const path = addQueryArgs( url, query ); - - args = { path }; - } - - return useApiFetchEffect( args ); -}; - -export default useFetchBudgetRecommendationEffect; From 7f7261a079a42fbb910047e46909f791a16639a2 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 22:57:10 +0400 Subject: [PATCH 37/50] Review comments. --- js/src/components/paid-ads/campaign-assets-form.js | 4 +++- js/src/data/actions.js | 7 ++++++- js/src/data/resolvers.js | 4 +--- js/src/hooks/useFetchBudgetRecommendation.js | 4 +--- js/src/hooks/useValidateCampaignWithCountryCodes.js | 12 ++++++++---- .../setup-paid-ads/paid-ads-setup-sections.js | 10 +++++----- 6 files changed, 24 insertions(+), 17 deletions(-) diff --git a/js/src/components/paid-ads/campaign-assets-form.js b/js/src/components/paid-ads/campaign-assets-form.js index 531c24825b..86b4f1d82d 100644 --- a/js/src/components/paid-ads/campaign-assets-form.js +++ b/js/src/components/paid-ads/campaign-assets-form.js @@ -87,7 +87,8 @@ export default function CampaignAssetsForm( { currencyCode, } = useValidateCampaignWithCountryCodes(); - // Grab the recommendations for the initial country codes. + // Grab the budget ecommendations for the initial country codes. + // refreshCountryCodes will trigger a new validation function with the new budget values. useEffect( () => { if ( initialCampaign?.countryCodes ) { refreshCountryCodes( initialCampaign.countryCodes ); @@ -108,6 +109,7 @@ export default function CampaignAssetsForm( { // If the validation function and values do not change, then the validation will not be triggerred since the `validate` // function uses useCallback and will not be re-created. setValue( 'dailyBudget', dailyBudget ); + // Sometimes the currency code takes time to resolve and the budget data is already available. setValue( 'currencyCode', currencyCode ); }, [ dailyBudget, currencyCode ] ); diff --git a/js/src/data/actions.js b/js/src/data/actions.js index 2846bee326..2b67eb9b33 100644 --- a/js/src/data/actions.js +++ b/js/src/data/actions.js @@ -19,10 +19,15 @@ import { isWCIos, isWCAndroid } from '.~/utils/isMobileApp'; /** * @typedef {import('.~/data/types.js').AssetEntityGroupUpdateBody} AssetEntityGroupUpdateBody - * @typedef {import('.~/data/actions').CountryCode} CountryCode * @typedef {import('./selectors').Tour} Tour */ +/** + * CountryCode + * + * @typedef {string} CountryCode Two-letter country code in ISO 3166-1 alpha-2 format. Example: 'US'. + */ + /** * Individual shipping rate. * diff --git a/js/src/data/resolvers.js b/js/src/data/resolvers.js index 20646697b2..e10a5f3686 100644 --- a/js/src/data/resolvers.js +++ b/js/src/data/resolvers.js @@ -49,9 +49,7 @@ import { } from './actions'; /** - * CountryCode - * - * @typedef {string} CountryCode Two-letter country code in ISO 3166-1 alpha-2 format. Example: 'US'. + * @typedef {import('.~/data/actions').CountryCode} CountryCode */ export function* getShippingRates() { diff --git a/js/src/hooks/useFetchBudgetRecommendation.js b/js/src/hooks/useFetchBudgetRecommendation.js index 636a47de43..81df485703 100644 --- a/js/src/hooks/useFetchBudgetRecommendation.js +++ b/js/src/hooks/useFetchBudgetRecommendation.js @@ -24,9 +24,7 @@ const useFetchBudgetRecommendation = ( countryCodes ) => { const { getAdsBudgetRecommendations } = select( STORE_KEY ); const data = getAdsBudgetRecommendations( countryCodes ); - return { - data, - }; + return { data }; }, [ countryCodes ] ); diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 8df4af0aab..407fc369b3 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -23,14 +23,18 @@ import getHighestBudget from '.~/utils/getHighestBudget'; /** * @typedef {Object} ValidateCampaignWithCountryCodesHook * @property {(values: CampaignFormValues) => Object} validateCampaignWithCountryCodes A function to validate campaign form values. - * @property {boolean} hasFinishedResolution A boolean indicating whether the budget recommendation data has been resolved. + * @property {number | undefined} dailyBudget The daily budget recommendation. + * @property {(number: string | number) => string} formatAmount A function to format an amount according to the user's currency settings. + * @property {(countryCodes: Array) => void} refreshCountryCodes A function to refresh the country codes. + * @property {string} currencyCode The currency code. + * @property {boolean} loaded A boolean indicating whether the budget recommendation data has been resolved and the code currency available. */ /** * Validate campaign form. Accepts the form values object and returns errors object. * * @param {Array} [initialCountryCodes] Country code array. If not provided, the validate function will not take into account budget recommendations. - * @return {ValidateCampaignWithCountryCodesHook} An object containing the `validateCampaignWithCountryCodes` function and a `loading` state. + * @return {ValidateCampaignWithCountryCodesHook} The validate campaign with country codes hook. */ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { const { @@ -53,7 +57,7 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { formatAmount, refreshCountryCodes: setCountryCodes, currencyCode: code, - hasFinishedResolution: true, + loaded: true, }; } @@ -81,7 +85,7 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { formatAmount, refreshCountryCodes: setCountryCodes, currencyCode: code, - hasFinishedResolution: + loaded: hasFinishedResolution( 'getAdsBudgetRecommendations', [ countryCodes, ] ) && code, diff --git a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js index 40f3111f11..20e0ca3717 100644 --- a/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js +++ b/js/src/setup-mc/setup-stepper/setup-paid-ads/paid-ads-setup-sections.js @@ -8,11 +8,11 @@ import { Form } from '@woocommerce/components'; * Internal dependencies */ import useGoogleAdsAccountBillingStatus from '.~/hooks/useGoogleAdsAccountBillingStatus'; +import BudgetSection from '.~/components/paid-ads/budget-section'; +import BillingCard from '.~/components/paid-ads/billing-card'; import SpinnerCard from '.~/components/spinner-card'; import Section from '.~/wcdl/section'; import useValidateCampaignWithCountryCodes from '.~/hooks/useValidateCampaignWithCountryCodes'; -import BudgetSection from '.~/components/paid-ads/budget-section'; -import BillingCard from '.~/components/paid-ads/billing-card'; import clientSession from './clientSession'; import { GOOGLE_ADS_BILLING_STATUS } from '.~/constants'; @@ -35,7 +35,7 @@ const defaultPaidAds = { /** * Renders sections of Google Ads account, budget and billing for setting up the paid ads. - * Waits for the validate campaign with country codes to be loaded before rendering the form. + * Waits for the validate campaign with country codes function to be loaded before rendering the form. * * @param {Object} props React props. * @param {(onStatesReceived: PaidAdsData)=>void} props.onStatesReceived Callback to receive the data for setting up paid ads when initial and also when the budget and billing are updated. @@ -45,7 +45,7 @@ export default function PaidAdsSetupSections( { onStatesReceived, countryCodes, } ) { - const { validateCampaignWithCountryCodes, hasFinishedResolution } = + const { validateCampaignWithCountryCodes, loaded } = useValidateCampaignWithCountryCodes( countryCodes ); const { billingStatus } = useGoogleAdsAccountBillingStatus(); const onStatesReceivedRef = useRef(); @@ -91,7 +91,7 @@ export default function PaidAdsSetupSections( { clientSession.setCampaign( nextPaidAds ); }, [ paidAds, isBillingCompleted, validateCampaignWithCountryCodes ] ); - if ( ! billingStatus || ! hasFinishedResolution ) { + if ( ! billingStatus || ! loaded ) { return (
From 330697cc2329febe0f121add117b38b316bbcc88 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 24 Sep 2024 23:19:28 +0400 Subject: [PATCH 38/50] Update minimum amount displayed to the user. --- js/src/components/paid-ads/validateCampaign.js | 2 +- js/src/components/paid-ads/validateCampaign.test.js | 13 +++++++++++++ .../add-paid-campaigns/add-paid-campaigns.test.js | 13 +++++++++++++ .../specs/setup-mc/step-4-complete-campaign.test.js | 2 +- 4 files changed, 28 insertions(+), 2 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 3c08b9b7e8..641068b29f 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -42,7 +42,7 @@ const validateCampaign = ( values, opts ) => { 'Please make sure daily average cost is greater than %s.', 'google-listings-and-ads' ), - formatAmount( minAmount ) + formatAmount( minAmount - 1 ) ), }; } diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index 7fe2e9e7ba..d92e87d712 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -95,6 +95,19 @@ describe( 'validateCampaign', () => { expect( errors.amount ).toContain( 'is greater than Rs 30' ); } ); + it( 'When a budget is provided and the amount is same than the minimum, should pass', () => { + const mockFormatAmount = jest.fn().mockReturnValue( 'Rs 30' ); + values.amount = 30; + + const opts = { + dailyBudget: 100, + formatAmount: mockFormatAmount, + }; + + const errors = validateCampaign( values, opts ); + expect( errors ).not.toHaveProperty( 'amount' ); + } ); + it( 'When a budget is provided and the amount is greater than the minimum, should pass', () => { const mockFormatAmount = jest.fn().mockReturnValue( 'Rs 35' ); values.amount = 35; diff --git a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js index dff72794f5..7ec5b8eeaf 100644 --- a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js +++ b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js @@ -2,6 +2,7 @@ * External dependencies */ import { expect, test } from '@playwright/test'; + /** * Internal dependencies */ @@ -393,6 +394,18 @@ test.describe( 'Set up Ads account', () => { page.getByRole( 'button', { name: 'Continue' } ) ).toBeEnabled(); } ); + + test( 'User is notified of the minimum value', async () => { + budget = '4'; + await setupBudgetPage.fillBudget( budget ); + await setupBudgetPage.getBudgetInput().blur(); + + await expect( + page.getByText( + 'Please make sure daily average cost is greater than €4.00.' + ) + ).toBeVisible(); + } ); } ); test( 'Budget Recommendation', async () => { diff --git a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js index c7d737dcef..b3ac690099 100644 --- a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js +++ b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js @@ -231,7 +231,7 @@ test.describe( 'Complete your campaign', () => { .locator( '.components-base-control__help' ) .textContent(); await expect( error ).toBe( - 'Please make sure daily average cost is greater than NT$30.00.' + 'Please make sure daily average cost is greater than NT$29.00.' ); } ); From 0d8c6bd9a285168c317b3de631b9f74eac02cf8c Mon Sep 17 00:00:00 2001 From: asvinb Date: Thu, 26 Sep 2024 12:29:06 +0400 Subject: [PATCH 39/50] Make sure country codes are loaded. --- .../useValidateCampaignWithCountryCodes.js | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 407fc369b3..cc135ab4d1 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -44,13 +44,16 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { const [ countryCodes, setCountryCodes ] = useState( [] ); useEffect( () => { - setCountryCodes( initialCountryCodes ); + if ( initialCountryCodes ) { + setCountryCodes( initialCountryCodes ); + } }, [ initialCountryCodes ] ); return useSelect( ( select ) => { // If no country codes are provided, return the default validateCampaign function. - if ( ! countryCodes?.length ) { + // countryCodes is initially empty when being set in state, hence the check for initialCountryCodes as well. + if ( ! countryCodes.length && ! initialCountryCodes?.length ) { return { validateCampaignWithCountryCodes: validateCampaign, dailyBudget: null, @@ -65,6 +68,12 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { select( STORE_KEY ); const budgetData = getAdsBudgetRecommendations( countryCodes ); const budget = getHighestBudget( budgetData?.recommendations ); + const loaded = + hasFinishedResolution( 'getAdsBudgetRecommendations', [ + countryCodes, + ] ) && + code && + countryCodes.length; // make sure the country codes are set in state before considering the data loaded /** * Validate campaign form. Accepts the form values object and returns errors object. @@ -85,13 +94,10 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { formatAmount, refreshCountryCodes: setCountryCodes, currencyCode: code, - loaded: - hasFinishedResolution( 'getAdsBudgetRecommendations', [ - countryCodes, - ] ) && code, + loaded, }; }, - [ countryCodes, formatAmount, code ] + [ countryCodes, formatAmount, code, initialCountryCodes ] ); }; From 5f2f291c81217027ad60797806771026e56416f7 Mon Sep 17 00:00:00 2001 From: asvinb Date: Thu, 26 Sep 2024 13:21:54 +0400 Subject: [PATCH 40/50] Fix e2e test. --- .../add-paid-campaigns.test.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js index 7ec5b8eeaf..e78a8193f0 100644 --- a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js +++ b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js @@ -386,15 +386,6 @@ test.describe( 'Set up Ads account', () => { ).toBeDisabled(); } ); - test( 'Continue button should be enabled if budget is above the recommended value', async () => { - budget = '5'; - await setupBudgetPage.fillBudget( budget ); - - await expect( - page.getByRole( 'button', { name: 'Continue' } ) - ).toBeEnabled(); - } ); - test( 'User is notified of the minimum value', async () => { budget = '4'; await setupBudgetPage.fillBudget( budget ); @@ -406,6 +397,15 @@ test.describe( 'Set up Ads account', () => { ) ).toBeVisible(); } ); + + test( 'Continue button should be enabled if budget is above the recommended value', async () => { + budget = '5'; + await setupBudgetPage.fillBudget( budget ); + + await expect( + page.getByRole( 'button', { name: 'Continue' } ) + ).toBeEnabled(); + } ); } ); test( 'Budget Recommendation', async () => { From 03b11679a80b80a013f40ca898c1a56e79422582 Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 27 Sep 2024 14:50:30 +0400 Subject: [PATCH 41/50] Take precision settings into consideration. --- js/src/components/paid-ads/validateCampaign.js | 11 +++++++---- js/src/hooks/useValidateCampaignWithCountryCodes.js | 8 ++++++-- 2 files changed, 13 insertions(+), 6 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 641068b29f..2662204568 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -31,18 +31,21 @@ const validateCampaign = ( values, opts ) => { Number.isFinite( opts?.dailyBudget ) ) { const { amount } = values; - const { dailyBudget, formatAmount } = opts; - const minAmount = Math.round( dailyBudget * BUDGET_MIN_PERCENT ); + const { dailyBudget, formatAmount, precision } = opts; + + const minAmount = parseFloat( + ( dailyBudget * BUDGET_MIN_PERCENT ).toFixed( precision ) + ); if ( amount < minAmount ) { return { amount: sprintf( /* translators: %1$s: minimum daily budget */ __( - 'Please make sure daily average cost is greater than %s.', + 'Please make sure daily average cost is at least %s.', 'google-listings-and-ads' ), - formatAmount( minAmount - 1 ) + formatAmount( minAmount ) ), }; } diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index cc135ab4d1..1ac8176277 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -25,6 +25,7 @@ import getHighestBudget from '.~/utils/getHighestBudget'; * @property {(values: CampaignFormValues) => Object} validateCampaignWithCountryCodes A function to validate campaign form values. * @property {number | undefined} dailyBudget The daily budget recommendation. * @property {(number: string | number) => string} formatAmount A function to format an amount according to the user's currency settings. + * @property {number} precision Number of decimal places after the decimal separator. * @property {(countryCodes: Array) => void} refreshCountryCodes A function to refresh the country codes. * @property {string} currencyCode The currency code. * @property {boolean} loaded A boolean indicating whether the budget recommendation data has been resolved and the code currency available. @@ -39,7 +40,7 @@ import getHighestBudget from '.~/utils/getHighestBudget'; const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { const { formatAmount, - adsCurrencyConfig: { code }, + adsCurrencyConfig: { code, precision }, } = useAdsCurrency(); const [ countryCodes, setCountryCodes ] = useState( [] ); @@ -60,6 +61,7 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { formatAmount, refreshCountryCodes: setCountryCodes, currencyCode: code, + precision, loaded: true, }; } @@ -85,6 +87,7 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { return validateCampaign( values, { dailyBudget: budget?.daily_budget, formatAmount, + precision, } ); }; @@ -92,12 +95,13 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { validateCampaignWithCountryCodes, dailyBudget: budget?.daily_budget, formatAmount, + precision, refreshCountryCodes: setCountryCodes, currencyCode: code, loaded, }; }, - [ countryCodes, formatAmount, code, initialCountryCodes ] + [ countryCodes, formatAmount, code, initialCountryCodes, precision ] ); }; From 7432760a3b5bb361811d05798e430c0544f32289 Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 27 Sep 2024 15:10:17 +0400 Subject: [PATCH 42/50] Fix e2e tests. --- js/src/components/paid-ads/validateCampaign.js | 2 +- js/src/components/paid-ads/validateCampaign.test.js | 2 +- tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js | 2 +- tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index 2662204568..a39222f296 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -42,7 +42,7 @@ const validateCampaign = ( values, opts ) => { amount: sprintf( /* translators: %1$s: minimum daily budget */ __( - 'Please make sure daily average cost is at least %s.', + 'Please make sure daily average cost is at least %s', 'google-listings-and-ads' ), formatAmount( minAmount ) diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index d92e87d712..a9ca4edb24 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -92,7 +92,7 @@ describe( 'validateCampaign', () => { const errors = validateCampaign( values, opts ); expect( errors ).toHaveProperty( 'amount' ); - expect( errors.amount ).toContain( 'is greater than Rs 30' ); + expect( errors.amount ).toContain( 'is at least Rs 30' ); } ); it( 'When a budget is provided and the amount is same than the minimum, should pass', () => { diff --git a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js index e78a8193f0..e0ccc7cadb 100644 --- a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js +++ b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js @@ -393,7 +393,7 @@ test.describe( 'Set up Ads account', () => { await expect( page.getByText( - 'Please make sure daily average cost is greater than €4.00.' + 'Please make sure daily average cost is at least €4.50' ) ).toBeVisible(); } ); diff --git a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js index b3ac690099..62bba6b668 100644 --- a/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js +++ b/tests/e2e/specs/setup-mc/step-4-complete-campaign.test.js @@ -231,7 +231,7 @@ test.describe( 'Complete your campaign', () => { .locator( '.components-base-control__help' ) .textContent(); await expect( error ).toBe( - 'Please make sure daily average cost is greater than NT$29.00.' + 'Please make sure daily average cost is at least NT$30.00' ); } ); From f23a52c8b87cf3afdeb3133ef520ed167cdd5966 Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 27 Sep 2024 15:29:54 +0400 Subject: [PATCH 43/50] Convert to float. --- .../components/paid-ads/validateCampaign.js | 7 ++++--- .../paid-ads/validateCampaign.test.js | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+), 3 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index a39222f296..b8c4f011a5 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -11,6 +11,7 @@ import { __, sprintf } from '@wordpress/i18n'; * @typedef {Object} ValidateCampaignOptions * @property {number | undefined} dailyBudget Daily budget for the campaign. * @property {Function} formatAmount A function to format the budget amount according to the currency settings. + * @property {number} precision Number of decimal for the amount. */ // Minimum percentage of the recommended daily budget. @@ -33,11 +34,11 @@ const validateCampaign = ( values, opts ) => { const { amount } = values; const { dailyBudget, formatAmount, precision } = opts; - const minAmount = parseFloat( - ( dailyBudget * BUDGET_MIN_PERCENT ).toFixed( precision ) + const minAmount = ( dailyBudget * BUDGET_MIN_PERCENT ).toFixed( + precision ); - if ( amount < minAmount ) { + if ( amount < parseFloat( minAmount ) ) { return { amount: sprintf( /* translators: %1$s: minimum daily budget */ diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index a9ca4edb24..103543ff42 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -12,6 +12,7 @@ describe( 'validateCampaign', () => { const validateCampaignOptions = { dailyBudget: undefined, formatAmount: jest.mock(), + precision: 0, }; beforeEach( () => { @@ -87,6 +88,7 @@ describe( 'validateCampaign', () => { const opts = { dailyBudget: 100, formatAmount: mockFormatAmount, + precision: 0, }; const errors = validateCampaign( values, opts ); @@ -120,4 +122,21 @@ describe( 'validateCampaign', () => { const errors = validateCampaign( values, opts ); expect( errors ).not.toHaveProperty( 'amount' ); } ); + + it( 'When precision is set, the correct amount should be displayed', () => { + const mockFormatAmount = jest + .fn() + .mockImplementation( ( amount ) => `Rs ${ amount }` ); + + values.amount = 10; + + const opts = { + dailyBudget: 100, + formatAmount: mockFormatAmount, + precision: 2, + }; + + const errors = validateCampaign( values, opts ); + expect( errors.amount ).toContain( 'is at least Rs 30.00' ); + } ); } ); From dfb1bc9dd38e0736a6e029b14c3f21a55d9dc29c Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 1 Oct 2024 19:18:47 +0400 Subject: [PATCH 44/50] Fix bad merge. --- .../add-paid-campaigns.test.js | 24 +------------------ 1 file changed, 1 insertion(+), 23 deletions(-) diff --git a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js index 030d1d7e96..bb8920ec12 100644 --- a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js +++ b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js @@ -353,34 +353,16 @@ test.describe( 'Set up Ads account', () => { } ); } ); -<<<<<<< HEAD - test( 'Audience should be United States', async () => { - const countrySearchBoxContainer = - getCountryInputSearchBoxContainer( page ); - const countryTags = - getCountryTagsFromInputSearchBoxContainer( page ); - await expect( countryTags ).toHaveCount( 1 ); - await expect( countrySearchBoxContainer ).toContainText( - 'United States' - ); - } ); - - test.describe( 'Set the budget', async () => { + test( 'Set the budget', async () => { test( 'Continue button should be disabled if budget is 0', async () => { budget = '0'; await setupBudgetPage.fillBudget( budget ); -======= - test( 'Set the budget', async () => { - budget = '0'; - await setupBudgetPage.fillBudget( budget ); ->>>>>>> update/2535-consolidate-ad-creation-ccf-merged await expect( page.getByRole( 'button', { name: 'Continue' } ) ).toBeDisabled(); } ); -<<<<<<< HEAD test( 'Continue button should be disabled if budget is less than recommended value', async () => { budget = '2'; await setupBudgetPage.fillBudget( budget ); @@ -410,10 +392,6 @@ test.describe( 'Set up Ads account', () => { page.getByRole( 'button', { name: 'Continue' } ) ).toBeEnabled(); } ); -======= - budget = '1'; - await setupBudgetPage.fillBudget( budget ); ->>>>>>> update/2535-consolidate-ad-creation-ccf-merged } ); test( 'Budget Recommendation', async () => { From c89ac46d27edcf046b0357435ff9f23e2d2777b8 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 1 Oct 2024 20:11:41 +0400 Subject: [PATCH 45/50] Simplify code where countryCodes is needed. --- .../ads-campaign/paid-ads-setup-sections.js | 9 ++----- .../paid-ads/budget-section/index.js | 10 +++---- .../paid-ads/campaign-assets-form.js | 27 +++---------------- js/src/components/types.js | 1 - .../useValidateCampaignWithCountryCodes.js | 27 +++++-------------- 5 files changed, 15 insertions(+), 59 deletions(-) diff --git a/js/src/components/paid-ads/ads-campaign/paid-ads-setup-sections.js b/js/src/components/paid-ads/ads-campaign/paid-ads-setup-sections.js index 54126edbc6..68247c7e73 100644 --- a/js/src/components/paid-ads/ads-campaign/paid-ads-setup-sections.js +++ b/js/src/components/paid-ads/ads-campaign/paid-ads-setup-sections.js @@ -46,20 +46,18 @@ const defaultPaidAds = { * * @param {Object} props React props. * @param {(onStatesReceived: PaidAdsData)=>void} props.onStatesReceived Callback to receive the data for setting up paid ads when initial and also when the budget and billing are updated. - * @param {Array|undefined} props.countryCodes Country codes for the campaign. * @param {Campaign} [props.campaign] Campaign data to be edited. If not provided, this component will show campaign creation UI. * @param {boolean} [props.showCampaignPreviewCard=false] Whether to show the campaign preview card. * @param {boolean} [props.loadCampaignFromClientSession=false] Whether to load the campaign data from the client session. */ export default function PaidAdsSetupSections( { onStatesReceived, - countryCodes, campaign, loadCampaignFromClientSession, showCampaignPreviewCard = false, } ) { const { validateCampaignWithCountryCodes, loaded } = - useValidateCampaignWithCountryCodes( countryCodes ); + useValidateCampaignWithCountryCodes(); const isCreation = ! campaign; const { billingStatus } = useGoogleAdsAccountBillingStatus(); const onStatesReceivedRef = useRef(); @@ -133,10 +131,7 @@ export default function PaidAdsSetupSections( { > { ( formProps ) => { return ( - + { showCampaignPreviewCard && } diff --git a/js/src/components/paid-ads/budget-section/index.js b/js/src/components/paid-ads/budget-section/index.js index 87f2b940fc..7f54a042d8 100644 --- a/js/src/components/paid-ads/budget-section/index.js +++ b/js/src/components/paid-ads/budget-section/index.js @@ -13,6 +13,7 @@ import './index.scss'; import BudgetRecommendation from './budget-recommendation'; import useGoogleAdsAccount from '.~/hooks/useGoogleAdsAccount'; import AppInputPriceControl from '.~/components/app-input-price-control'; +import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalCountryCodes'; /** * @typedef {import('.~/data/actions').CountryCode} CountryCode @@ -29,16 +30,11 @@ const nonInteractableProps = { * * @param {Object} props React props. * @param {Object} props.formProps Form props forwarded from `Form` component. - * @param {Array|undefined} props.countryCodes Country codes to fetch budget recommendations for. * @param {boolean} [props.disabled=false] Whether display the Card in disabled style. * @param {JSX.Element} [props.children] Extra content to be rendered under the card of budget inputs. */ -const BudgetSection = ( { - formProps, - countryCodes, - disabled = false, - children, -} ) => { +const BudgetSection = ( { formProps, disabled = false, children } ) => { + const { data: countryCodes } = useTargetAudienceFinalCountryCodes(); const { getInputProps, setValue, values } = formProps; const { amount } = values; const { googleAdsAccount } = useGoogleAdsAccount(); diff --git a/js/src/components/paid-ads/campaign-assets-form.js b/js/src/components/paid-ads/campaign-assets-form.js index ea694c8be3..a247a3923d 100644 --- a/js/src/components/paid-ads/campaign-assets-form.js +++ b/js/src/components/paid-ads/campaign-assets-form.js @@ -80,29 +80,8 @@ export default function CampaignAssetsForm( { }, [ assetEntityGroup ] ); const [ baseAssetGroup, setBaseAssetGroup ] = useState( initialAssetGroup ); const [ hasImportedAssets, setHasImportedAssets ] = useState( false ); - const { - validateCampaignWithCountryCodes, - dailyBudget, - refreshCountryCodes, - currencyCode, - } = useValidateCampaignWithCountryCodes(); - - // Grab the budget recommendations for the initial country codes. - // refreshCountryCodes will trigger a new validation function with the new budget values. - useEffect( () => { - if ( initialCampaign?.countryCodes ) { - refreshCountryCodes( initialCampaign.countryCodes ); - } - }, [ initialCampaign, refreshCountryCodes ] ); - - const handleOnChange = ( _, values, arg ) => { - onChange( _, values, arg ); - - // Whenever there's a change, update the country codes in the validation function. - if ( values.countryCodes ) { - refreshCountryCodes( values.countryCodes ); - } - }; + const { validateCampaignWithCountryCodes, dailyBudget, currencyCode } = + useValidateCampaignWithCountryCodes(); useEffect( () => { const { setValue } = formRef.current; @@ -163,7 +142,7 @@ export default function CampaignAssetsForm( { } } validate={ validateCampaignWithCountryCodes } extendAdapter={ extendAdapter } - onChange={ handleOnChange } + onChange={ onChange } { ...adaptiveFormProps } /> ); diff --git a/js/src/components/types.js b/js/src/components/types.js index fa7214953f..50259022b1 100644 --- a/js/src/components/types.js +++ b/js/src/components/types.js @@ -4,7 +4,6 @@ /** * @typedef {Object} CampaignFormValues - * @property {Array} countryCodes Selected country codes for the paid ads campaign. * @property {number} amount The daily average cost amount. */ diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 1ac8176277..b0f91a5c51 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -2,7 +2,6 @@ * External dependencies */ import { useSelect } from '@wordpress/data'; -import { useState, useEffect } from '@wordpress/element'; /** * Internal dependencies @@ -11,6 +10,7 @@ import { STORE_KEY } from '.~/data/constants'; import useAdsCurrency from './useAdsCurrency'; import validateCampaign from '.~/components/paid-ads/validateCampaign'; import getHighestBudget from '.~/utils/getHighestBudget'; +import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalCountryCodes'; /** * @typedef {import('.~/components/types.js').CampaignFormValues} CampaignFormValues @@ -26,7 +26,6 @@ import getHighestBudget from '.~/utils/getHighestBudget'; * @property {number | undefined} dailyBudget The daily budget recommendation. * @property {(number: string | number) => string} formatAmount A function to format an amount according to the user's currency settings. * @property {number} precision Number of decimal places after the decimal separator. - * @property {(countryCodes: Array) => void} refreshCountryCodes A function to refresh the country codes. * @property {string} currencyCode The currency code. * @property {boolean} loaded A boolean indicating whether the budget recommendation data has been resolved and the code currency available. */ @@ -34,32 +33,23 @@ import getHighestBudget from '.~/utils/getHighestBudget'; /** * Validate campaign form. Accepts the form values object and returns errors object. * - * @param {Array} [initialCountryCodes] Country code array. If not provided, the validate function will not take into account budget recommendations. * @return {ValidateCampaignWithCountryCodesHook} The validate campaign with country codes hook. */ -const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { +const useValidateCampaignWithCountryCodes = () => { + const { data: countryCodes } = useTargetAudienceFinalCountryCodes(); const { formatAmount, adsCurrencyConfig: { code, precision }, } = useAdsCurrency(); - const [ countryCodes, setCountryCodes ] = useState( [] ); - - useEffect( () => { - if ( initialCountryCodes ) { - setCountryCodes( initialCountryCodes ); - } - }, [ initialCountryCodes ] ); return useSelect( ( select ) => { - // If no country codes are provided, return the default validateCampaign function. - // countryCodes is initially empty when being set in state, hence the check for initialCountryCodes as well. - if ( ! countryCodes.length && ! initialCountryCodes?.length ) { + // If country codes are yet resolved, return the default validateCampaign function. + if ( ! countryCodes ) { return { validateCampaignWithCountryCodes: validateCampaign, dailyBudget: null, formatAmount, - refreshCountryCodes: setCountryCodes, currencyCode: code, precision, loaded: true, @@ -73,9 +63,7 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { const loaded = hasFinishedResolution( 'getAdsBudgetRecommendations', [ countryCodes, - ] ) && - code && - countryCodes.length; // make sure the country codes are set in state before considering the data loaded + ] ) && code; /** * Validate campaign form. Accepts the form values object and returns errors object. @@ -96,12 +84,11 @@ const useValidateCampaignWithCountryCodes = ( initialCountryCodes ) => { dailyBudget: budget?.daily_budget, formatAmount, precision, - refreshCountryCodes: setCountryCodes, currencyCode: code, loaded, }; }, - [ countryCodes, formatAmount, code, initialCountryCodes, precision ] + [ countryCodes, formatAmount, code, precision ] ); }; From 6d7a287ffb6053a2290aad30be3f0a75e6229119 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 1 Oct 2024 20:16:52 +0400 Subject: [PATCH 46/50] Remove unused props. --- js/src/components/paid-ads/campaign-assets-form.js | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/js/src/components/paid-ads/campaign-assets-form.js b/js/src/components/paid-ads/campaign-assets-form.js index a247a3923d..36659a95d0 100644 --- a/js/src/components/paid-ads/campaign-assets-form.js +++ b/js/src/components/paid-ads/campaign-assets-form.js @@ -2,7 +2,7 @@ * External dependencies */ import { useState, useMemo, useEffect, useRef } from '@wordpress/element'; -import { noop, isPlainObject } from 'lodash'; +import { isPlainObject } from 'lodash'; /** * Internal dependencies @@ -71,7 +71,6 @@ function convertAssetEntityGroupToFormValues( assetEntityGroup = {} ) { export default function CampaignAssetsForm( { initialCampaign, assetEntityGroup, - onChange = noop, ...adaptiveFormProps } ) { const formRef = useRef(); @@ -142,7 +141,6 @@ export default function CampaignAssetsForm( { } } validate={ validateCampaignWithCountryCodes } extendAdapter={ extendAdapter } - onChange={ onChange } { ...adaptiveFormProps } /> ); From 3ff8276be71c996227dd4fc0f6f3ebdc626aec61 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 1 Oct 2024 20:29:42 +0400 Subject: [PATCH 47/50] Fix loaded value --- .../paid-ads/campaign-assets-form.js | 19 +++++++++---------- .../useValidateCampaignWithCountryCodes.js | 2 +- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/js/src/components/paid-ads/campaign-assets-form.js b/js/src/components/paid-ads/campaign-assets-form.js index 36659a95d0..9e1c8fcea5 100644 --- a/js/src/components/paid-ads/campaign-assets-form.js +++ b/js/src/components/paid-ads/campaign-assets-form.js @@ -79,20 +79,19 @@ export default function CampaignAssetsForm( { }, [ assetEntityGroup ] ); const [ baseAssetGroup, setBaseAssetGroup ] = useState( initialAssetGroup ); const [ hasImportedAssets, setHasImportedAssets ] = useState( false ); - const { validateCampaignWithCountryCodes, dailyBudget, currencyCode } = + const { validateCampaignWithCountryCodes, dailyBudget, loaded } = useValidateCampaignWithCountryCodes(); useEffect( () => { - const { setValue } = formRef.current; + if ( loaded ) { + const { setValue } = formRef.current; - // Trigger a form value change to refresh the validation function once again with the new budget values - // If the validation function and values do not change, then the validation will not be triggerred since the `validate` - // function uses useCallback and will not be re-created. - setValue( 'dailyBudget', dailyBudget ); - - // Sometimes the currency code takes time to resolve and the budget data is already available. - setValue( 'currencyCode', currencyCode ); - }, [ dailyBudget, currencyCode ] ); + // Simulate a form value change to refresh the validation function once again with the new budget values + // If the validation function and values do not change, then the validation will not be triggerred since the `validate` + // function uses useCallback and will not be re-created. + setValue( 'dailyBudget', dailyBudget ); + } + }, [ dailyBudget, loaded ] ); const extendAdapter = ( formContext ) => { const assetGroupErrors = validateAssetGroup( formContext.values ); diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index b0f91a5c51..2ff2229766 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -52,7 +52,7 @@ const useValidateCampaignWithCountryCodes = () => { formatAmount, currencyCode: code, precision, - loaded: true, + loaded: false, }; } From 78fc8bbf555f9a2a3ac4236d15802c8c8e26ab43 Mon Sep 17 00:00:00 2001 From: asvinb Date: Tue, 1 Oct 2024 21:33:41 +0400 Subject: [PATCH 48/50] Fix e2e test --- .../add-paid-campaigns.test.js | 118 ++++++++---------- 1 file changed, 53 insertions(+), 65 deletions(-) diff --git a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js index bb8920ec12..9c48958816 100644 --- a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js +++ b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js @@ -352,53 +352,6 @@ test.describe( 'Set up Ads account', () => { await checkFAQExpandable( page ); } ); } ); - - test( 'Set the budget', async () => { - test( 'Continue button should be disabled if budget is 0', async () => { - budget = '0'; - await setupBudgetPage.fillBudget( budget ); - - await expect( - page.getByRole( 'button', { name: 'Continue' } ) - ).toBeDisabled(); - } ); - - test( 'Continue button should be disabled if budget is less than recommended value', async () => { - budget = '2'; - await setupBudgetPage.fillBudget( budget ); - - await expect( - page.getByRole( 'button', { name: 'Continue' } ) - ).toBeDisabled(); - } ); - - test( 'User is notified of the minimum value', async () => { - budget = '4'; - await setupBudgetPage.fillBudget( budget ); - await setupBudgetPage.getBudgetInput().blur(); - - await expect( - page.getByText( - 'Please make sure daily average cost is at least €4.50' - ) - ).toBeVisible(); - } ); - - test( 'Continue button should be enabled if budget is above the recommended value', async () => { - budget = '5'; - await setupBudgetPage.fillBudget( budget ); - - await expect( - page.getByRole( 'button', { name: 'Continue' } ) - ).toBeEnabled(); - } ); - } ); - - test( 'Budget Recommendation', async () => { - await expect( - page.getByText( 'set a daily budget of 15 USD' ) - ).toBeVisible(); - } ); } ); test.describe( 'Set up billing', () => { @@ -452,30 +405,65 @@ test.describe( 'Set up Ads account', () => { } ); test.describe( 'Create Ads with billing data already setup', () => { - test.beforeAll( async () => { - await setupBudgetPage.fulfillBillingStatusRequest( { - status: 'approved', + test.describe( 'Set the budget', async () => { + test( 'Continue button should be disabled if budget is 0', async () => { + //Reload the page + await page.reload(); + await setupBudgetPage.fulfillBillingStatusRequest( { + status: 'approved', + } ); + await page.waitForLoadState( LOAD_STATE.DOM_CONTENT_LOADED ); + + //Step 1 - Accounts are already set up. + await setupAdsAccounts.clickContinue(); + await page.waitForLoadState( LOAD_STATE.DOM_CONTENT_LOADED ); + + budget = '0'; + await setupBudgetPage.fillBudget( budget ); + + await expect( + page.getByRole( 'button', { name: 'Continue' } ) + ).toBeDisabled(); } ); - } ); - test( 'Launch paid campaign should be enabled', async () => { - //Reload the page - await page.reload(); - await page.waitForLoadState( LOAD_STATE.DOM_CONTENT_LOADED ); + test( 'Continue button should be disabled if budget is less than recommended value', async () => { + budget = '2'; + await setupBudgetPage.fillBudget( budget ); - //Step 1 - Accounts are already set up. - await setupAdsAccounts.clickContinue(); - await page.waitForLoadState( LOAD_STATE.DOM_CONTENT_LOADED ); + await expect( + page.getByRole( 'button', { name: 'Continue' } ) + ).toBeDisabled(); + } ); - //Step 2 - Fill the budget - await setupBudgetPage.fulfillBillingStatusRequest( { - status: 'approved', + test( 'User is notified of the minimum value', async () => { + budget = '4'; + await setupBudgetPage.fillBudget( budget ); + await setupBudgetPage.getBudgetInput().blur(); + + await expect( + page.getByText( + 'Please make sure daily average cost is at least €4.50' + ) + ).toBeVisible(); + } ); + + test( 'Continue button should be enabled if budget is above the recommended value', async () => { + budget = '5'; + await setupBudgetPage.fillBudget( budget ); + + await expect( + page.getByRole( 'button', { name: 'Continue' } ) + ).toBeEnabled(); } ); - await setupBudgetPage.fillBudget( '1' ); + } ); + test( 'Budget Recommendation', async () => { await expect( - page.getByRole( 'button', { name: 'Continue' } ) - ).toBeEnabled(); + page.getByText( 'set a daily budget of 15 USD' ) + ).toBeVisible(); + } ); + + test( 'Launch paid campaign should be enabled', async () => { await page.getByRole( 'button', { name: 'Continue' } ).click(); await page.waitForLoadState( LOAD_STATE.DOM_CONTENT_LOADED ); @@ -492,7 +480,7 @@ test.describe( 'Set up Ads account', () => { const campaignCreation = setupBudgetPage.mockCampaignCreationAndAdsSetupCompletion( - '1', + '5', [ 'US' ] ); await page From 3fb08323285e1f740097cacd11595d26f67ce1c7 Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 4 Oct 2024 18:31:22 +0400 Subject: [PATCH 49/50] Use Math.ceil. --- .../components/paid-ads/validateCampaign.js | 7 ++---- .../paid-ads/validateCampaign.test.js | 19 --------------- .../useValidateCampaignWithCountryCodes.js | 24 ++++++------------- 3 files changed, 9 insertions(+), 41 deletions(-) diff --git a/js/src/components/paid-ads/validateCampaign.js b/js/src/components/paid-ads/validateCampaign.js index b8c4f011a5..fe3d595c24 100644 --- a/js/src/components/paid-ads/validateCampaign.js +++ b/js/src/components/paid-ads/validateCampaign.js @@ -11,7 +11,6 @@ import { __, sprintf } from '@wordpress/i18n'; * @typedef {Object} ValidateCampaignOptions * @property {number | undefined} dailyBudget Daily budget for the campaign. * @property {Function} formatAmount A function to format the budget amount according to the currency settings. - * @property {number} precision Number of decimal for the amount. */ // Minimum percentage of the recommended daily budget. @@ -32,11 +31,9 @@ const validateCampaign = ( values, opts ) => { Number.isFinite( opts?.dailyBudget ) ) { const { amount } = values; - const { dailyBudget, formatAmount, precision } = opts; + const { dailyBudget, formatAmount } = opts; - const minAmount = ( dailyBudget * BUDGET_MIN_PERCENT ).toFixed( - precision - ); + const minAmount = Math.ceil( dailyBudget * BUDGET_MIN_PERCENT ); if ( amount < parseFloat( minAmount ) ) { return { diff --git a/js/src/components/paid-ads/validateCampaign.test.js b/js/src/components/paid-ads/validateCampaign.test.js index 103543ff42..a9ca4edb24 100644 --- a/js/src/components/paid-ads/validateCampaign.test.js +++ b/js/src/components/paid-ads/validateCampaign.test.js @@ -12,7 +12,6 @@ describe( 'validateCampaign', () => { const validateCampaignOptions = { dailyBudget: undefined, formatAmount: jest.mock(), - precision: 0, }; beforeEach( () => { @@ -88,7 +87,6 @@ describe( 'validateCampaign', () => { const opts = { dailyBudget: 100, formatAmount: mockFormatAmount, - precision: 0, }; const errors = validateCampaign( values, opts ); @@ -122,21 +120,4 @@ describe( 'validateCampaign', () => { const errors = validateCampaign( values, opts ); expect( errors ).not.toHaveProperty( 'amount' ); } ); - - it( 'When precision is set, the correct amount should be displayed', () => { - const mockFormatAmount = jest - .fn() - .mockImplementation( ( amount ) => `Rs ${ amount }` ); - - values.amount = 10; - - const opts = { - dailyBudget: 100, - formatAmount: mockFormatAmount, - precision: 2, - }; - - const errors = validateCampaign( values, opts ); - expect( errors.amount ).toContain( 'is at least Rs 30.00' ); - } ); } ); diff --git a/js/src/hooks/useValidateCampaignWithCountryCodes.js b/js/src/hooks/useValidateCampaignWithCountryCodes.js index 2ff2229766..d1c06b8f53 100644 --- a/js/src/hooks/useValidateCampaignWithCountryCodes.js +++ b/js/src/hooks/useValidateCampaignWithCountryCodes.js @@ -25,9 +25,7 @@ import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalC * @property {(values: CampaignFormValues) => Object} validateCampaignWithCountryCodes A function to validate campaign form values. * @property {number | undefined} dailyBudget The daily budget recommendation. * @property {(number: string | number) => string} formatAmount A function to format an amount according to the user's currency settings. - * @property {number} precision Number of decimal places after the decimal separator. - * @property {string} currencyCode The currency code. - * @property {boolean} loaded A boolean indicating whether the budget recommendation data has been resolved and the code currency available. + * @property {boolean} loaded A boolean indicating whether the budget recommendation data has been resolved. */ /** @@ -37,10 +35,7 @@ import useTargetAudienceFinalCountryCodes from '.~/hooks/useTargetAudienceFinalC */ const useValidateCampaignWithCountryCodes = () => { const { data: countryCodes } = useTargetAudienceFinalCountryCodes(); - const { - formatAmount, - adsCurrencyConfig: { code, precision }, - } = useAdsCurrency(); + const { formatAmount } = useAdsCurrency(); return useSelect( ( select ) => { @@ -50,8 +45,6 @@ const useValidateCampaignWithCountryCodes = () => { validateCampaignWithCountryCodes: validateCampaign, dailyBudget: null, formatAmount, - currencyCode: code, - precision, loaded: false, }; } @@ -60,10 +53,10 @@ const useValidateCampaignWithCountryCodes = () => { select( STORE_KEY ); const budgetData = getAdsBudgetRecommendations( countryCodes ); const budget = getHighestBudget( budgetData?.recommendations ); - const loaded = - hasFinishedResolution( 'getAdsBudgetRecommendations', [ - countryCodes, - ] ) && code; + const loaded = hasFinishedResolution( + 'getAdsBudgetRecommendations', + [ countryCodes ] + ); /** * Validate campaign form. Accepts the form values object and returns errors object. @@ -75,7 +68,6 @@ const useValidateCampaignWithCountryCodes = () => { return validateCampaign( values, { dailyBudget: budget?.daily_budget, formatAmount, - precision, } ); }; @@ -83,12 +75,10 @@ const useValidateCampaignWithCountryCodes = () => { validateCampaignWithCountryCodes, dailyBudget: budget?.daily_budget, formatAmount, - precision, - currencyCode: code, loaded, }; }, - [ countryCodes, formatAmount, code, precision ] + [ countryCodes, formatAmount ] ); }; From ce9ad995befa296ba3d1be4c8c469020191ff81c Mon Sep 17 00:00:00 2001 From: asvinb Date: Fri, 4 Oct 2024 19:53:08 +0400 Subject: [PATCH 50/50] Fix failing test. --- .../e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js index 9c48958816..f1e3e0d5de 100644 --- a/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js +++ b/tests/e2e/specs/add-paid-campaigns/add-paid-campaigns.test.js @@ -442,13 +442,13 @@ test.describe( 'Set up Ads account', () => { await expect( page.getByText( - 'Please make sure daily average cost is at least €4.50' + 'Please make sure daily average cost is at least €5.00' ) ).toBeVisible(); } ); test( 'Continue button should be enabled if budget is above the recommended value', async () => { - budget = '5'; + budget = '6'; await setupBudgetPage.fillBudget( budget ); await expect( @@ -480,7 +480,7 @@ test.describe( 'Set up Ads account', () => { const campaignCreation = setupBudgetPage.mockCampaignCreationAndAdsSetupCompletion( - '5', + '6', [ 'US' ] ); await page