[Vouchers] Fix tax included in price amount #11593

rioug · 2023-09-28T07:59:16Z

What? Why?

Closes [Vouchers] For tax included in price the tax amount is not updated after applying the voucher #11363

When tax are included in price, we store the tax part of the voucher in the voucher adjustment itself. The issue is, this value isn't displayed anywhere nor is it used to display the discount tax to the customer. This PR fix this.
In the case of tax excluded from price, we store the tax part of the voucher as its own adjustment, which are both displayed to the customer and in the backoffice, so we no fix is needed there.

What should we test?

As enterprise user set up a shop with a percentage voucher and a product with included tax.
As customer put that product in your cart.
Proceed to checkout step 2.
Select the percentage voucher and a payment method.
Proceed to step 3.
Calculate the numbers.

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Based on #11543, it will need to be merged first

When tax are included in price, voucher tax is stored in the voucher adjustment and not as its own adjustment. So we add a "fake adjustment" for display purposes.

mkllnk · 2023-10-02T05:12:04Z

app/helpers/admin/orders_helper.rb

+      adjustments_for_display = order.adjustments + order.all_adjustments.payment_fee.eligible
+
+      if VoucherAdjustmentsService.new(order).voucher_included_tax.negative?
+        adjustment = order.voucher_adjustments.first
+        adjustments_for_display << Spree::Adjustment.new(
+          label: I18n.t("admin.orders.edit.voucher_tax_included_in_price", label: adjustment.label),
+          amount: adjustment.included_tax
+        )
+      end
+
+      adjustments_for_display


The whole if block could have been somewhere else, e.g. the service and then this could be simpler:

Suggested change

adjustments_for_display = order.adjustments + order.all_adjustments.payment_fee.eligible

if VoucherAdjustmentsService.new(order).voucher_included_tax.negative?

adjustment = order.voucher_adjustments.first

adjustments_for_display << Spree::Adjustment.new(

label: I18n.t("admin.orders.edit.voucher_tax_included_in_price", label: adjustment.label),

amount: adjustment.included_tax

)

end

adjustments_for_display

voucher_service = VoucherAdjustmentsService.new(order)

order.adjustments +

order.all_adjustments.payment_fee.eligible +

voucher_service.included_tax_adjustment

Nice suggestion!

I'm uncomfortable with instantiating a new Adjustment here though; I think it could cause trouble (I can't think of any in the current circumstances, but circumstances could change). Or at least confusion.

The good thing is we don't need the model, just something like it. I suggest a struct would be enough to implement the label and amount values. As it's display-related logic, I would try to keep the logic here in the orders_helper, but perhaps in a private method.

dacook

Good fix.

I think it would be worth refactoring the helper though, what do you think?

dacook · 2023-10-03T04:49:18Z

app/helpers/admin/orders_helper.rb

+      adjustments_for_display = order.adjustments + order.all_adjustments.payment_fee.eligible
+
+      if VoucherAdjustmentsService.new(order).voucher_included_tax.negative?
+        adjustment = order.voucher_adjustments.first
+        adjustments_for_display << Spree::Adjustment.new(
+          label: I18n.t("admin.orders.edit.voucher_tax_included_in_price", label: adjustment.label),
+          amount: adjustment.included_tax
+        )
+      end
+
+      adjustments_for_display


Nice suggestion!

I'm uncomfortable with instantiating a new Adjustment here though; I think it could cause trouble (I can't think of any in the current circumstances, but circumstances could change). Or at least confusion.

The good thing is we don't need the model, just something like it. I suggest a struct would be enough to implement the label and amount values. As it's display-related logic, I would try to keep the logic here in the orders_helper, but perhaps in a private method.

dacook · 2023-10-03T05:03:53Z

spec/helpers/checkout_helper_spec.rb

+    subject(:display_checkout_tax_total) { helper.display_checkout_tax_total(order) }
+
+    let(:order) { instance_double(Spree::Order, total_tax: 123.45, currency: 'AUD') }
+    let(:service) { instance_double(VoucherAdjustmentsService, voucher_included_tax: ) }


I would have set up real data and used the real VoucherAdjustmentsService. What if the method VoucherAdjustmentsService#voucher_included_tax ~~go renamed one day~~, or returned a different value? This test would still pass.

But maybe that's just opinion :)

If it's because the test data is complicated to set up, then maybe we need to create a factory for it?

instance_double verifies that the methods are present. https://www.simplybusiness.co.uk/about-us/tech/2020/05/rspec-instance-doubles/

But in general I agree that mocking is to be avoided if possible.

Thanks, I didn't know that!

We are not creating a new adjustment here.

The whole concept is confusing. Maybe translators will find better versions.

dacook

Great, thanks for doing this.

I agree that it's still confusing using a Struct. But I like how it's a bit clearer that it's not an actual adjustment, so I suggest we proceed this way.

However it looks like the adjustments for display are in the wrong order now, can you please check?

dacook · 2023-10-12T04:25:44Z

app/helpers/admin/orders_helper.rb

+      order.adjustments +
+        voucher_included_tax_representations(order) +
+        order.all_adjustments.payment_fee.eligible


Should voucher_included_tax_representations(order) be the last one in the list? It looks like it was before.

I thought that that was looking odd. It seems clearer to me to put the voucher tax under the voucher.

config/locales/en.yml

dacook

👍

drummer83 · 2023-10-13T16:21:54Z

Hi @rioug,
Thanks for fixing this! 👍

I compared before and after staging and did some test shopping.

Before	After

Before:

After:

Before:

After:

I have also tested:

Checking out with multiple line items. ✔️
Shopping products excl. tax. ✔️
Using a voucher with a fixed rate. ✔️

All looking good! Excellent! 🎉
Merging! 🚀

Thanks again!

rioug self-assigned this Sep 28, 2023

rioug marked this pull request as ready for review September 28, 2023 08:57

rioug added 4 commits October 2, 2023 15:58

Take into account voucher tax part when displaying tax included in price

8254bd9

Add a fake adjustment voucher tax on admin order page

8d639c1

When tax are included in price, voucher tax is stored in the voucher adjustment and not as its own adjustment. So we add a "fake adjustment" for display purposes.

Fix system spec to take into account discounted tax

1a66f3d

Fix rubocop warning

7f2c1fe

mkllnk force-pushed the 11363-vouchers-fix-tax-included-in-price-amount branch from 8493792 to 7f2c1fe Compare October 2, 2023 04:58

mkllnk added the feature toggled These pull requests' changes are invisible by default and are grouped in release notes label Oct 2, 2023

mkllnk approved these changes Oct 2, 2023

View reviewed changes

mkllnk self-assigned this Oct 2, 2023

dacook self-requested a review October 3, 2023 04:24

dacook reviewed Oct 3, 2023

View reviewed changes

mkllnk added 3 commits October 12, 2023 14:17

Simplify helper with extracted method

74c8f06

Clarify adjustment data only for display

f83e78a

We are not creating a new adjustment here.

Trying to clarify voucher tax amount

f4fde0a

The whole concept is confusing. Maybe translators will find better versions.

mkllnk requested a review from dacook October 12, 2023 04:21

dacook requested changes Oct 12, 2023

View reviewed changes

dacook reviewed Oct 12, 2023

View reviewed changes

config/locales/en.yml Show resolved Hide resolved

Update spec

477ca39

mkllnk requested a review from dacook October 12, 2023 05:01

dacook approved these changes Oct 12, 2023

View reviewed changes

drummer83 self-assigned this Oct 13, 2023

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Oct 13, 2023

drummer83 merged commit d7b234c into openfoodfoundation:master Oct 13, 2023

drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Oct 13, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vouchers] Fix tax included in price amount #11593

[Vouchers] Fix tax included in price amount #11593

rioug commented Sep 28, 2023 •

edited by mkllnk

Loading

mkllnk Oct 2, 2023

dacook Oct 3, 2023

dacook left a comment

dacook Oct 3, 2023

dacook Oct 3, 2023 •

edited

Loading

mkllnk Oct 4, 2023

dacook Oct 4, 2023

dacook left a comment

dacook Oct 12, 2023

mkllnk Oct 12, 2023

dacook left a comment

drummer83 commented Oct 13, 2023

[Vouchers] Fix tax included in price amount #11593

[Vouchers] Fix tax included in price amount #11593

Conversation

rioug commented Sep 28, 2023 • edited by mkllnk Loading

What? Why?

What should we test?

Release notes

Dependencies

mkllnk Oct 2, 2023

Choose a reason for hiding this comment

dacook Oct 3, 2023

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook Oct 3, 2023

Choose a reason for hiding this comment

dacook Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

mkllnk Oct 4, 2023

Choose a reason for hiding this comment

dacook Oct 4, 2023

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook Oct 12, 2023

Choose a reason for hiding this comment

mkllnk Oct 12, 2023

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

drummer83 commented Oct 13, 2023

rioug commented Sep 28, 2023 •

edited by mkllnk

Loading

dacook Oct 3, 2023 •

edited

Loading