Disallow entering discounts higher than price #14083
Conversation
| func updateDiscountDisallowedState(_ value: Bool) { | ||
| DispatchQueue.main.async { | ||
| self.discountValueIsDisallowed = value | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure how good practice is this, since we generally send the values directly to the publisher, but I don't see a specific problem with it either. Initially I attempted to use combine to observe changes on the price amount and enable/disable appropriately, but that changes the current implementation enough to open a new can of worms in several places.
There was a problem hiding this comment.
There's nothing wrong with it, but as mentioned above, you shouldn't need it if you do the validation in response to user input, rather than in response to reading the value.
|
|
iamgabrielma
added
type: task
| /// Captures state when a discount value should be disallowed, for example, | ||
| /// when the discount entered is higher than the total price of a product. | ||
| /// | ||
| @Published private(set) var discountValueIsDisallowed: Bool = false |
There was a problem hiding this comment.
I'd suggest either naming this specific to the reason for why it's disallowed, or making it an enum that explains why with code, rather than a bool.
I think at the moment it's only used for the discount being too high, so let's call it
| @Published private(set) var discountValueIsDisallowed: Bool = false | |
| @Published private(set) var discountExceedsProductPrice: Bool = false |
There was a problem hiding this comment.
Updated! 1771281
| if priceAfterDiscount.compare(NSDecimalNumber.zero) == .orderedAscending { | ||
| updateDiscountDisallowedState(true) | ||
| return currencyFormatter.formatAmount(priceAfterDiscount) ?? "" | ||
| } | ||
| updateDiscountDisallowedState(false) |
There was a problem hiding this comment.
Inside a function named calculatePriceAfterDiscount, I'd consider this a side-effect.
Since calculatePriceAfterDiscount is used directly from a view for display, it really ought to be a var, ideally a @Published var, but at least a computed. I know that's pre-existing, but we should avoid adding a side effect here.
It's potentially confusing that we check whether the input is valid as a response to displaying the discount, rather than as a response to the user entering the discount.
A better trigger might be FeeOrDiscountLineDetailsViewModel.updateAmount(_:) and updatePercentage, which happens when the onChangeText for the input field is called. This would also remove the need for the async block too.
If you move it there, you should probably rename the functions to avoid it being a side effect, e.g. amountTextChanged and percentageTextChanged, but it's much more of a nitpick at that point.
A complicating factor is that the base price is currently passed in to FeeOrDiscountLineDetailsViewModel from the view, as an argument to calculatePriceAfterDiscount, which is weird – but it's also passed in when the view model is created, as baseAmountForPercentage. That's just the base price, with a re-name, I see no reason that we can't use it for calculating an amount discount as well.
If you do all of that, you can have a much simpler function to check the validity:
func checkDiscountValidity() {
let priceAfterDiscount = baseAmount - finalAmountDecimal
discountValueIsDisallowed = priceAfterDiscount < 0
}
Then just call that from your text changed functions.
You're working with some unnecessarily convoluted code here, but I think the PR as it stands makes it more complex, and we don't need to do that.
I've only had a fairly short poke at this, but it appears to work. If I'm missing some detail, apologies and just let me know!
There was a problem hiding this comment.
Thanks for the guidance! Appreciated 💟
I had three main issues here that made it trickier-than-expected:
- The -unnecessary- complexity of it all
- Being forced to use a function with the price property passed in, directly from the view, for the updates.
- I was unable to see where
updateAmountorupdatePercentagewere called. Using both "Show Callers" and the "Call Hierarchy" showed nothing, I see now that this was because these are not directly invoked, but passed through the closure via.onChange(), most likely to facilitate decoupling, so we do not need to pass a viewmodel in, but this made it harder to follow its actual usage.
By removing the constraint of (2) as you mentioned, and just using the value we pass in the initializer everything became much simpler, and the tests have become also more understandable now: We pass an initial amount, we update the amount with a discount, and we assert if that's allowed or not.
Main update was done on 1771281 , I've also updated the property name on 9dda8ea , and made it a computed var on 6e3c39b
I think there's still some oddities to be addressed in this feature (like how much we need a separation between baseAmount and initialTotal, or if we still need lineType at all) but that's for another day :D
| func updateDiscountDisallowedState(_ value: Bool) { | ||
| DispatchQueue.main.async { | ||
| self.discountValueIsDisallowed = value | ||
| } | ||
| } |
There was a problem hiding this comment.
There's nothing wrong with it, but as mentioned above, you shouldn't need it if you do the validation in response to user input, rather than in response to reading the value.
By using the existing `baseAmountForPercentage` that we pass to the VM in the init, we can remove the requirement to pass the `price` to the function from the view, which simplifies both the calculation of the discount, as well as validating when a discount shouldn’t be allowed.
| } | ||
| let discount = NSDecimalNumber(decimal: finalAmountDecimal) | ||
| let priceAfterDiscount = price.subtracting(discount) | ||
| var recalculateFormattedPriceAfterDiscount: String { |
There was a problem hiding this comment.
| var recalculateFormattedPriceAfterDiscount: String { | |
| var formattedPriceAfterDiscount: String { |
Strange to have a verb in a property name, it makes it seem like a function
There was a problem hiding this comment.
You're right, updated: 795fcff
WooCommerce/Classes/ViewRelated/Orders/Order Creation/ProductsSection/ProductDiscountView.swift
Outdated
Show resolved
Hide resolved
...CommerceTests/ViewRelated/Orders/Order Creation/FeeOrDiscountLineDetailsViewModelTests.swift
Outdated
Show resolved
Hide resolved
...CommerceTests/ViewRelated/Orders/Order Creation/FeeOrDiscountLineDetailsViewModelTests.swift
Outdated
Show resolved
Hide resolved
...CommerceTests/ViewRelated/Orders/Order Creation/FeeOrDiscountLineDetailsViewModelTests.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Thanks for the changes, the code looks a lot better. I've suggested a rename in comments.
There is a bug I've found, minor but worth a quick check while you have context – if you enter an invalid discount, switch to the other type ($/%), enter a valid discount there, then switch back to the first type, it will let you add the previously-entered invalid discount.
Repro
- Create an order and add a product to it
- Expand the product and enter a discount greater than its price
- Change to % discount – observe that the validation message is not cleared, even though the discount shows as $0
- Type any percentage <= 100% – observe that the validation message is cleared
- Change to $ discount – observe that the previous amount is still there
- Tap Add
- You have just discounted by more than the price.
This is happening because we only check the validity when we type something – we could perhaps also do that check when we change the type.
Discount validity is only checked when the input field changes, but is not checked if a merchant happens to switch between discount types (fixed vs percentual), which can lead to invalid state when a discount is modified in one view, and then they switch type, as discount validity won’t be checked again until the input is updated.
Good catch, you're right on: when we switch between fixed and percentage discounts we recreate a new view instance of I've initially tried to fix it in the view model when we perform the discount line type switch, but this ends crashing the app due what seems to be a massive call stack to the computed variable from view recreation. Instead I've just added a call for discount validity check when the view appears, which would happen only once each time we switch between discount types. Fixed here: b94b567
|
Closes: #14080
Description
There's a discrepancy between platforms regarding how we allow discounts to be applied to products, in iOS we allow discounts higher than the product price, while Android does not.
With this PR we get closer to Android by not allowing these, displaying an error message when a merchant attempts to do so, and disallowing the
Addbutton of working until is resolved.There are further UI and logic improvements that can be done in this screen, but I have left them outside this PR to keep a reduced scope (eg: The "remove discount" button appears when there are no discounts saved yet).
Note: There seems to be a lot of unnecessary complexity in these discount-related screens, most likely because we haven't updated the logic from when we stopped handling cases that no longer exist (fees). At some point would be good to separate "discounts" from "legacy fees", and improve the general design of how we apply these.
Testing
Orders>+>+ Add product> expand the product and tap on+ Add discountAddbutton is disabledRELEASE-NOTES.txtif necessary.Reviewer (or Author, in the case of optional code reviews):
Please make sure these conditions are met before approving the PR, or request changes if the PR needs improvement: