add condition on warning_forfeit_remaining_amount note #11694
Conversation
prateek0411999
force-pushed
the
bugfix/11666-warning_forfeit_remaining_amount
branch
from
fde4a6d to
0656838
Compare
| create(:voucher_percentage_rate, code: 'percent_code', | ||
| enterprise: percent_order.distributor, amount: 20) | ||
| } | ||
| let(:adjustment) { flat_voucher.create_adjustment(flat_voucher.code, order) } |
There was a problem hiding this comment.
As far as I can see this is not used, could you remove this line ?
app/models/voucher.rb
Outdated
|
|
||
| def type_flat? | ||
| type == 'Vouchers::FlatRate' | ||
| end |
There was a problem hiding this comment.
Voucher is meant to be an abstract class, ie it is meant to be subclassed to implement a "concrete voucher" in our case flat rate and percentage voucher (I should have added some comment around that to make it clearer). That said, Voucher shouldn't know what type of voucher are going to exist. By adding this method you created a dependency on Vouchers::FlatRate meaning it is implying that Vouchers::FlatRate exists. In general it's better to avoid this.
A simpler solution is to use is_a? method in the view.
To be fair in our code base, we do have an explicit dependency on Vouchers::FlatRate, as it is the default type in the vouchers table, meaning we actually need Vouchers::FlatRate to be defined :
Line 1086 in 9f426ad
| @@ -11,7 +11,7 @@ | |||
| = link_to t("split_checkout.step2.voucher.remove_code"), voucher_adjustment_path(id: voucher_adjustment.id), method: "delete", data: { confirm: t("split_checkout.step2.voucher.confirm_delete") } | |||
|
|
|||
| - # This might not be true, ie payment method including a fee which wouldn't be covered by voucher or tax implication raising total to be bigger than the voucher amount ? | |||
| - if voucher_adjustment.originator.amount > order.pre_discount_total | |||
| - if ((voucher_adjustment.originator.amount > order.pre_discount_total) && voucher_adjustment.originator.type_flat?) | |||
There was a problem hiding this comment.
We don't need the parenthesis here. And as mentioned in the comment above, the simpler solution is to use is_a?
| - if ((voucher_adjustment.originator.amount > order.pre_discount_total) && voucher_adjustment.originator.type_flat?) | |
| - if voucher_adjustment.originator.amount > order.pre_discount_total && voucher_adjustment.originator.is_a?(Vouchers::FlatRate) |
| let(:flat_order) { create(:order_with_distributor, total: 10) } | ||
| let(:percent_order) { create(:order_with_distributor, total: 10) } |
There was a problem hiding this comment.
Each test should be able to be run independently, ie not depend on each other. This means you could just use the same order for both your test and it will work. Not a blocker though.
|
@rioug thanks for reviewing the MR, will push the necessary changes shortly. |
prateek0411999
force-pushed
the
bugfix/11666-warning_forfeit_remaining_amount
branch
from
0656838 to
a113b5b
Compare
There was a problem hiding this comment.
Looks good now ! 👍
I can see that you rewrote your git history. Whilst it's a good thing to keep the git history clean, it's better no to do it once your code has been reviewed. It results in lost context, other reviewer won't be able to see the process that got us to the current solution.
|
@dacook thanks. |
|
Hey @prateek0411999 , welcome 🎉 Thanks for the PR, and the spec! So, the warning is displayed if the order total is less than the voucher value (regardless of being flat or not). Indeed, this way, we can see the warning, for a percentage voucher:
After staging this PR: We can see that the warning is not displayed now, within the same scenario. Regression test - It still is displayed for flat amount vouchers Thanks for this improvement! |
What? Why?
Resolves the issue where the **warning_forfeit_remaining_amount note ** ->(Note: if your order total is less than your voucher you may not be able to spend the remaining value.') was still visible even when the percentage voucher was applied.
What should we test?
Changelog Category: