Budget screen logic #10709

JorgeMucientes · 2024-02-07T16:32:06Z

Part of: #10574

Description

This PR adds the following missing logic to the Blaze budget screen:

The main budget slider now replicates the behavior of the Blaze webview slider:
- Min and Max values are dynamically calculated based on the campaign duration
- The budget value is updated according to to the duration step, that is, if the duration is 7n days for example, the total budget value will be increased or decreased in steps of 7. This part is trickier than it looks. More on this below.
The duration bottom sheet now updates all the budget content once the slider is released, but not while drawing the slider.
Implemented updating the campaign start date
All the data from the Blaze budget screen is now passed back to the campaign preview screen.

The forecast updates are not part of the scope of this PR.

About the budget slider
Due to how the Composable slider works using a Float value to set the slider's position, whereas the budget value is an integer (that has to change in chunks/steps equal to the duration of the campaign), there's some flakiness to how slider responds. Between those 2 values there's some conversion from float to int and some rounding that causes the total budget value displayed to skip some steps. You'll see what I mean when scrolling back and forth in the budget slider. I tested different approaches (even using the steps property from the slider), but none worked perfectly. The current solution is the one that works best. Of course, any ideas for improvement are welcome. Its possible I might be missing something.

UPDATE: About the above. There are some real problems with the current slider implementation from this PR as explained in #10709 (comment). I fixed them in this other PR: #10739.

Testing instructions

Open Blaze campaign creation flow
Open edit budget screen
Play around with all the sliders and editable info
Click Update and check that all the update data is reflected in campaign preview screen.

Images/gif

demoBudgetScreen.mp4

When updating campaign duration we also need to update the budget slider range as well as the total budget value

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewViewModel.kt

WooCommerce/src/main/res/navigation/nav_graph_blaze_campaign_creation.xml

…reation.xml Co-authored-by: Ondrej Ruttkay <[email protected]>

wpmobilebot · 2024-02-07T18:52:16Z

📲 You can test the changes from this Pull Request in WooCommerce Android by scanning the QR code below to install the corresponding build.

	App Name	WooCommerce Android
	Flavor	Jalapeno
	Build Type	Debug
	Commit	`f2535b0`
	Direct Download	`woocommerce-prototype-build-pr10709-f2535b0.apk`

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewFragment.kt # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewViewModel.kt

dangermattic · 2024-02-08T00:36:52Z

	1 Warning
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.

Generated by 🚫 Danger

JorgeMucientes · 2024-02-08T00:38:11Z

...main/kotlin/com/woocommerce/android/ui/blaze/creation/budget/BlazeCampaignBudgetViewModel.kt

+    fun onBudgetUpdated(sliderValue: Float) {
+        if (sliderValue.toInt().mod(viewState.value?.durationInDays!!) == 0) {
+            val dailySpending = sliderValue / viewState.value?.durationInDays!!
+            _viewState.value = _viewState.value?.copy(
+                totalBudget = sliderValue,
+                dailySpending = formatDailySpend(dailySpending)
+            )
+        }


This is the tricky part that handles updating the slider value and the budget value independently.

Ok, for some reason I just noticed that Total Spend value is not properly updated when the duration for the campaign has larger value.
For example, set the duration to something like 25 days and then proceed to play around with the budget slider. The amount won't get correctly refreshed, it will seem unresponsive.
I'll look again into fixing that.

codecov-commenter · 2024-02-08T01:23:04Z

Codecov Report

Attention: 70 lines in your changes are missing coverage. Please review.

Comparison is base (541c2d0) 41.44% compared to head (f2535b0) 41.41%.
Report is 4 commits behind head on trunk.

Files	Patch %	Lines
...ze/creation/budget/BlazeCampaignBudgetViewModel.kt	0.00%	52 Missing ⚠️
...n/preview/BlazeCampaignCreationPreviewViewModel.kt	0.00%	18 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff              @@
##              trunk   #10709      +/-   ##
============================================
- Coverage     41.44%   41.41%   -0.04%     
  Complexity     5017     5017              
============================================
  Files          1016     1016              
  Lines         58364    58414      +50     
  Branches       7803     7810       +7     
============================================
  Hits          24190    24190              
- Misses        32030    32080      +50     
  Partials       2144     2144

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewViewModel.kt

JorgeMucientes · 2024-02-09T12:34:41Z

Heads up @0nko, in case you are reviewing this or looking into the issues with the budget slider. I just want to let you know that I've implemented a different approach here: #10739, which fixes all the problems with the budget slider. I'll set the PR ready for review soon.

0nko

I've tested the PR with the slider fix and it works. I'll go ahead and merge this then.

JorgeMucientes added 10 commits February 6, 2024 15:46

Add logic to update budget slider values correctly

4a4d71e

When updating campaign duration we also need to update the budget slider range as well as the total budget value

Show daily spend as integer as decimals should always be 0

07c0bed

Refactor constant names

6786343

Update campaign duration dates section

91790b6

Enable updating campaign start date

2adf86a

Give edit campaign start date clickable feel

4be84aa

Give edit campaign start date clickable feel

3867944

Give edit campaign start date clickable feel

94d3795

Restore budget and duration state when opened from preview screen

ae039c2

Merge branch 'trunk' into issue/10574-budget-screen-logic

0ac35d9

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewViewModel.kt

JorgeMucientes added the feature: blaze Related to the Blaze project label Feb 7, 2024

0nko reviewed Feb 7, 2024

View reviewed changes

WooCommerce/src/main/res/navigation/nav_graph_blaze_campaign_creation.xml Outdated Show resolved Hide resolved

Update WooCommerce/src/main/res/navigation/nav_graph_blaze_campaign_c…

797639c

…reation.xml Co-authored-by: Ondrej Ruttkay <[email protected]>

JorgeMucientes added 3 commits February 8, 2024 00:27

Avoid refreshing all the UI while draging the slider

3d7a5a2

Use floats for slider values for smoother sliding

37f26f9

JorgeMucientes marked this pull request as ready for review February 8, 2024 00:35

JorgeMucientes requested review from hichamboushaba, hafizrahman and 0nko February 8, 2024 00:36

JorgeMucientes added the unit-tests-exemption label Feb 8, 2024

JorgeMucientes added this to the 17.3 milestone Feb 8, 2024

JorgeMucientes commented Feb 8, 2024

View reviewed changes

JorgeMucientes modified the milestones: 17.3, 17.4 Feb 9, 2024

JorgeMucientes mentioned this pull request Feb 9, 2024

Edit budget screen #10574

Closed

0nko self-assigned this Feb 9, 2024

Merge branch 'trunk' into issue/10574-budget-screen-logic

f2535b0

# Conflicts: # WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/creation/preview/BlazeCampaignCreationPreviewViewModel.kt

JorgeMucientes mentioned this pull request Feb 9, 2024

Fix budget slider responsiveness #10739

Merged

0nko approved these changes Feb 9, 2024

View reviewed changes

0nko merged commit e31d0dc into trunk Feb 9, 2024
16 checks passed

0nko deleted the issue/10574-budget-screen-logic branch February 9, 2024 14:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Budget screen logic #10709

Budget screen logic #10709

JorgeMucientes commented Feb 7, 2024 •

edited

Loading

wpmobilebot commented Feb 7, 2024 •

edited

Loading

dangermattic commented Feb 8, 2024 •

edited

Loading

JorgeMucientes Feb 8, 2024

JorgeMucientes Feb 8, 2024 •

edited

Loading

codecov-commenter commented Feb 8, 2024 •

edited

Loading

JorgeMucientes commented Feb 9, 2024

0nko left a comment

Budget screen logic #10709

Budget screen logic #10709

Conversation

JorgeMucientes commented Feb 7, 2024 • edited Loading

Description

Testing instructions

Images/gif

wpmobilebot commented Feb 7, 2024 • edited Loading

dangermattic commented Feb 8, 2024 • edited Loading

JorgeMucientes Feb 8, 2024

Choose a reason for hiding this comment

JorgeMucientes Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

codecov-commenter commented Feb 8, 2024 • edited Loading

Codecov Report

JorgeMucientes commented Feb 9, 2024

0nko left a comment

Choose a reason for hiding this comment

JorgeMucientes commented Feb 7, 2024 •

edited

Loading

wpmobilebot commented Feb 7, 2024 •

edited

Loading

dangermattic commented Feb 8, 2024 •

edited

Loading

JorgeMucientes Feb 8, 2024 •

edited

Loading

codecov-commenter commented Feb 8, 2024 •

edited

Loading