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 70.00000% with 6 lines in your changes missing coverage. Please review.

Project coverage is 63.1%. Comparing base (1626f8a) to head (9dd2744).
Report is 71 commits behind head on feature/2459-campaign-creation-flow.

Files with missing lines	Patch %	Lines
js/src/pages/edit-paid-ads-campaign/index.js	0.0%	4 Missing and 1 partial ⚠️
js/src/components/paid-ads/campaign-assets-form.js	66.7%	1 Missing ⚠️

Additional details and impacted files

@@                          Coverage Diff                          @@
##           feature/2459-campaign-creation-flow   #2583     +/-   ##
=====================================================================
+ Coverage                                 62.6%   63.1%   +0.5%     
=====================================================================
  Files                                      329     329             
  Lines                                     5166    5183     +17     
  Branches                                  1254    1262      +8     
=====================================================================
+ Hits                                      3232    3268     +36     
+ Misses                                    1752    1735     -17     
+ Partials                                   182     180      -2     

Flag	Coverage Δ
js-unit-tests	63.1% <70.0%> (+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/pages/create-paid-ads-campaign/index.js	10.0% <ø> (ø)
js/src/setup-ads/ads-stepper/setup-paid-ads.js	62.1% <ø> (ø)
js/src/components/paid-ads/campaign-assets-form.js	76.9% <66.7%> (-0.9%)	⬇️
js/src/pages/edit-paid-ads-campaign/index.js	8.6% <0.0%> (-0.6%)	⬇️

... and 6 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.

[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.

[eason9487]

Consider this PR is based on the update/2535-consolidate-ad-creation-ccf-merged branch, and the branch's PR #2623 may have several code changes that conflict with this PR, and even need some rewriting. Therefore, I'm going to hold off on reviewing this PR until the code review for #2623 is done first and the possible code conflicts are resolved.

[eason9487]

Thanks for the work. LGTM.