Add minimum budget limit

[dsawardekar]

Changes proposed in this Pull Request:

Closes #2503

This PR updates the campaign flow to include validation of minimum daily limits. An error message is shown if the amount is less than the daily limit.

Screenshots:

With an insufficient amount:

With a sufficient amount:

Detailed test instructions:

1. On the final step of the onboarding flow, Click create a campaign
2. You should see the Set your budget section
3. In the Daily avg cost, enter 1 < value < 5
4. You should see the above error below the input (red styling TBD)

Additional details:

Update - Enforce the daily limit of 30% in setup your budget section

Changelog entry

Update - Adjust the minimum average daily cost of a campaign to 30% of the highest recommended value among audience countries.

[codecov]

Codecov Report

Attention: Patch coverage is 36.84211% with 48 lines in your changes missing coverage. Please review.

Project coverage is 61.6%. Comparing base (34c53d7) to head (ce9ad99).

Files with missing lines	Patch %	Lines
js/src/data/resolvers.js	0.0%	13 Missing and 3 partials ⚠️
...s/src/hooks/useValidateCampaignWithCountryCodes.js	50.0%	7 Missing ⚠️
js/src/utils/getHighestBudget.js	0.0%	5 Missing and 2 partials ⚠️
...s/paid-ads/ads-campaign/paid-ads-setup-sections.js	0.0%	4 Missing and 1 partial ⚠️
js/src/hooks/useFetchBudgetRecommendation.js	16.7%	5 Missing ⚠️
js/src/components/paid-ads/campaign-assets-form.js	66.7%	2 Missing ⚠️
js/src/data/selectors.js	33.3%	1 Missing and 1 partial ⚠️
js/src/data/utils.js	0.0%	2 Missing ⚠️
...-ads/budget-section/budget-recommendation/index.js	0.0%	1 Missing ⚠️
js/src/components/paid-ads/budget-section/index.js	50.0%	1 Missing ⚠️

Additional details and impacted files

@@                               Coverage Diff                                @@
##           update/2535-consolidate-ad-creation-ccf-merged   #2583     +/-   ##
================================================================================
+ Coverage                                            61.0%   61.6%   +0.5%     
================================================================================
  Files                                                 330     331      +1     
  Lines                                                5183    5233     +50     
  Branches                                             1254    1268     +14     
================================================================================
+ Hits                                                 3162    3221     +59     
+ Misses                                               1832    1820     -12     
- Partials                                              189     192      +3     

Flag	Coverage Δ
js-unit-tests	61.6% <36.8%> (+0.5%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
js/src/components/paid-ads/validateCampaign.js	100.0% <100.0%> (ø)
js/src/data/action-types.js	100.0% <ø> (ø)
js/src/data/reducer.js	83.8% <100.0%> (+0.3%)	⬆️
...-ads/budget-section/budget-recommendation/index.js	4.0% <0.0%> (ø)
js/src/components/paid-ads/budget-section/index.js	11.1% <50.0%> (+0.6%)	⬆️
js/src/components/paid-ads/campaign-assets-form.js	76.2% <66.7%> (-1.6%)	⬇️
js/src/data/selectors.js	48.0% <33.3%> (+0.6%)	⬆️
js/src/data/utils.js	94.1% <0.0%> (-3.8%)	⬇️
...s/paid-ads/ads-campaign/paid-ads-setup-sections.js	3.3% <0.0%> (+0.2%)	⬆️
js/src/hooks/useFetchBudgetRecommendation.js	16.7% <16.7%> (ø)
... and 3 more

... and 5 files with indirect coverage changes

[dsawardekar]

@asvinb We need a styling update to the error message shown when a minimum limit is shown to the user. The input box also needs to be updated when in that invalid input state. Can you please take a look? Thanks!

[dsawardekar]

@joemcgill As part of this work, I extracted the getHighestBudget into a utility.

• Should we update js/src/components/paid-ads/budget-section/budget-recommendation/index.js to use this utility
  • If yes, should those changes be part of this PR or a separate one

[joemcgill]

Thanks @dsawardekar. Re:

Should we update js/src/components/paid-ads/budget-section/budget-recommendation/index.js to use this utility

Let's not update the Paid Ads flow at this time. It should end up getting handled as part of #2535.

[dsawardekar]

@joemcgill This is ready for code review. Note that I needed to create a new mock helper for the budget recommendation, let me know if that needs to be refactored.

[asvinb]

@dsawardekar I left some comments on the PR. Can you kindly check please and let me know what you think?

[dsawardekar]

@asvinb I've updated the PR per the feedback. Can you take another look? thanks!

[asvinb]

@dsawardekar Thanks for the updates. I think it's very close to approval. Can you check my latest comments please? Thanks!

[joemcgill]

@asvinb I have one small tweak for this that I'd like to make, but am pre-approving and this can go to QA.

[asvinb]

@joemcgill @ankitguptaindia Raised a valid point for when using decimals which is in WooCommerce settings (Settings -> Currency Options -> Number of decimals). Checking @fblascogarma comment where we wish to round the number automatically, I think it'll add unnecessary complexity since we now have to rely on the number of decimals for the currency. I suggest we use don't round up the numbers and instead rely on the user settings for the decimal numbers. If the decimal place is set to 0, then the amount will be automatically rounded. Furthermore, I now agree with what Darshan originally suggested. If we change the error message from "greater than" to "at least", it'll simplify things as well. Let's say the minimum amount is 5, and the user inputs 5, depending on the decimal options set, we either need to tell the user that the minimum amount is 4 (if decimal places is set to 0) or 4.99 (if decimal places is set to 2). So we'll have additional logic if we use greater than. Whereas if we use at least, we just output the minimum amount, with the appropriate decimal places.
cc @ankitguptaindia

[joemcgill]

@asvinb I'm not sure I understand the issue we're running into with rounding. My understanding is that we are only rounding the suggested budget in order to set the minimum value, which is what should then be displayed to users as the minimum value in the error message. We should not be rounding the value that the user has input.

I think the text change you suggested makes sense and is more accurate since a value that is === to the minimum should be acceptable.

[eason9487]

Hi @joemcgill and @asvinb, I would like to chip in two questions about the budget validation.

1. While I was leaving Add minimum budget limit #2583 (comment), I thought it could be solved by adjusting the message to "Please make sure daily average cost is at least %s." and not changing any other logic, because adjusting the wording shouldn't be a concern. However, with subsequent code changes and discussions, I would like to confirm that the message must be strictly “Please make sure daily average cost is greater than %s.”?

2. Just noticed Campaign Creation: Add minimum budget limit #2503 (comment) saying:

   30% and rounding up for a nicer number.

   If the final decision still makes it a nicer integer as the minimum budget, then the previous Math.round() needs to be replaced with Math.ceil().
   

   google-listings-and-ads/js/src/components/paid-ads/validateCampaign.js

   Line 35 in 716581b

   const minAmount = Math.round( dailyBudget * BUDGET_MIN_PERCENT );

[asvinb]

@joemcgill @eason9487 Let me try to summarize the "issue" with the decimals (although it's not really an issue 😁 ).
The scenario below is assuming the minimum budget recommendation is 15 and we are changing the number of decimals in Woo settings:

With the number of decimals set to 0:

• Minimum value: 5 (rounded from 4.5)
• Anything less than 5, i.e 4 is considered invalid.
  

With the number of decimals set to 1:

• Minimum value: 4.5 (since we have the number of decimals set to 1)
• Anything less than 4.5, i.e 4.4 is considered invalid.
  

With the number of decimals set to 2:

• Minimum value: 4.50 (since we have the number of decimals set to 1)
• • Anything less than 4.50, i.e 4.49 is considered invalid.
    

So if we want to use the greater than text, then we must take into account the number of decimal places to do the calculation as well because it'll vary, from 4 to 4.49 (as in the examples above) for the same minimum amount. That's why I suggested we keep the at least since then we just show the minimum value out of the box without the need to calculate the highest invalid amount.
Now about the rounding, as you can see in the examples above, it'll automatically be rounded if the number of decimal places is set to 0. So we rely on Woo settings and the precision and formatAmount from the useAdsCurrency hook to automatically do that for us.

[joemcgill]

Thanks for the explanation, @asvinb. I think you're correct (if I'm understanding correctly) that using the store setting to control the decimal rounding likely makes sense.

What is still a bit confusing is how this ended up creating a situation like @ankitguptaindia reported here where the minimum budget error shows a number lower than the one the user has actually input. I'm not able to reproduce that error locally.

[asvinb]

@joemcgill Actually the error that @ankitguptaindia reported is no longer applicable because it's now fixed 😁 Previously, i.e when not taking into consideration the Woo number of decimals settings, then you could reproduce the issue. However now it's fixed. I changed the target branch to update/2535-consolidate-ad-creation-ccf-merged and fixed the conflicts and also simplified the code. Can you kindly take a look before I send to QA with all those changes? Thanks!

[joemcgill]

Thanks for explaining. This tested well for me. Thanks

[asvinb]

Thanks @joemcgill . Assigning to @ankitguptaindia for another round of QA.

[ankitguptaindia]

Thanks for the fixes, @asvinb. The issues have been resolved, and the minimum budget requirement message is now appearing correctly when the WooCommerce settings have different decimal configurations.

Quick Demo:

Recording.884.mp4

[eason9487]

Hi @asvinb @joemcgill

Before starting a new round of code reviews, could you help me clarify the two questions in #2583 (comment)?

Using the current 78fc8bb revision and replacing:

google-listings-and-ads/js/src/components/paid-ads/validateCampaign.js

Lines 37 to 39 in 78fc8bb

	const minAmount = ( dailyBudget * BUDGET_MIN_PERCENT ).toFixed(
	precision
	);

with:

const minAmount = Math.ceil( dailyBudget * BUDGET_MIN_PERCENT );

Apart from using the greater than wording, It works pretty well and fulfills the rest requirements without the semantic inaccuracies in the error message.

Kapture.2024-10-04.at.18.58.34.mp4

I have read all the relevant discussions in the PR. However, taking the decimals into account when calculating the minimum budget value still confuses me.

[asvinb]

Hello @eason9487
I think it's the word "rounding" which is causing some confusion here and what happens when the decimal places are set in the store settings and using Math.round. I think Math.ceil works well as well and we just need to align that we can use the at least instead of greater than (where we'll need to take the decimal points into account. For e.g if "greater than" was to be used, then 5.99 should be displayed if the decimal points setting is set to 2 decimals).

[joemcgill]

I think Math.ceil works well as well and we just need to align that we can use the at least instead of greater than

I agree and using "at least" is totally appropriate here and more accurate than the original requirements that I had written for the issue.

[eason9487]

Hi @asvinb

I think it's the word "rounding" which is causing some confusion here and what happens when the decimal places are set in the store settings and using Math.round. I think Math.ceil works well as well [...]

This question should not be related to the word "rounding" and it's not the cause of the confusion. I used Math.ceil in the example just because #2503 (comment) mentioned "rounding up".

[...] and we just need to align that we can use the at least instead of greater than (where we'll need to take the decimal points into account. For e.g if "greater than" was to be used, then 5.99 should be displayed if the decimal points setting is set to 2 decimals)

Yes, and the current implementation has already rephrased to at least. So my confusion is, what is the reason for still taking the decimals into account when there is no need?

[asvinb]

@eason9487 If we use Math.ceil, then there's no need to take any decimals into account.