Vouchers part 1 #11002

Matt-Yorkley · 2023-06-13T21:59:20Z

What? Why?

Addresses a couple of things related to checkout processing and vouchers. This was previously combined with some other changes, but I've split them into two smaller chunks so it's easier to review. This is part one.

Partially fixes #10857, should be fully fixed by the next PR (#11003).

Review notes

Hopefully the commits are pretty well explained by their names but here's some extra notes:

Removed an Angular loading indicator which in some cases was causing the screen to be blocked with a dark background and little loading animation in cases where it shouldn't be there, mostly on the order completed page. This was previously used by Angular but isn't needed any more.
The customer details controller was doing a weird thing where it was handling applying taxes on admin adjustments, but that shouldn't be the responsibility of that controller, so its moved out to #create_tax_charge! where all other tax handling is done.
Handling of the issue where taxes on an order need to be recalculated if the order's address changes was being done solely by the customer details controller and that's now done at the model level, because it should apply to any context and not just that one controller.
Taxes on enterprise fees don't get applied until the order is at least in payment state, eg it's passed cart, address, and delivery states. This is in line with how all tax handling works since split checkout was introduced.
The way that re-calculating voucher amounts based on the order total was being handled was a bit off in certain cases where a voucher was already present, as it was using the order amount with discounts applied instead of the order amount before discounts. Eg: if the order total is 20 and a voucher for -5 is present and the order total changes to 22, when recalculating the voucher amount it should be using the pre-discount total of 22 and not the discounted total of 22 - 5 (17) when making calculations.

What should we test?

If an order has progressed to at least payment state then submitting a change to it's address should recalculate tax correctly if the change of address would change it's tax category. Note: which address gets used for tax purposes depends on an admin configuration setting.
Some of the issues outlined in [Vouchers] Errors when/from iterating between /summary and /cart page #10857 around errors when recalculating a voucher amount on an order should be resolved but maybe it's best to test that along with the next PR Vouchers part 2 #11003 (and it's still behind a feature toggle here)?

Release notes

Changelog Category: Technical changes

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

…ransition

This should be done at the model level

mkllnk

Nice. Looking great! ✨

mkllnk · 2023-06-15T23:35:45Z

app/views/layouts/darkswarm.html.haml

@@ -46,7 +46,6 @@
          = yield

          #footer
-    %loading


mkllnk · 2023-06-15T23:43:07Z

spec/controllers/spree/admin/orders_controller_spec.rb

@@ -57,7 +57,7 @@

        it "updates fees and taxes and redirects to order details page" do
          expect(order).to receive(:recreate_all_fees!)
-          expect(order).to receive(:create_tax_charge!)
+          expect(order).to receive(:create_tax_charge!).at_least :once


So now it's called multiple times? I would have thought that the transition happens only once.

I puzzled over this a bit, but it looks like this happens in relation to what the test setup is doing as opposed to what happens in the test itself. Basically before the test even starts (or when the order object is called and lazy-instantiated) the process of created a completed order (:completed_order_with_totals) is running through various bits of logic in a sequence like adding line items and creating shipments and setting the shipping address, etc etc, and some of that is doing things that are triggering tax-related callbacks because the order total is changing or the tax address is changing?

rioug

Looks good 👍

rioug · 2023-06-16T01:26:17Z

app/controllers/split_checkout_controller.rb

@@ -204,6 +203,7 @@ def shipping_method_ship_address_not_required?

  def process_voucher
    if add_voucher
+      VoucherAdjustmentsService.calculate(@order)


As long as taxes don't change after this it should be fine. If I followed correctly taxes are recalculated if address changes, and also after the order move to the "payment" step, which all happen before we can reach this code, is that correct ?

app/controllers/split_checkout_controller.rb

app/models/spree/order.rb

dacook

Great, looks good to me too! I have a suggestion for bonus points though ;)

dacook · 2023-06-16T02:08:11Z

spec/system/consumer/split_checkout_spec.rb

@@ -1114,11 +1114,8 @@
        let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor, amount: 6) }

        before do
-          # Add voucher to the order


These comments might seem superfluous to some, but as a first-time reader of the code, I find them a helpful narration :)

drummer83 · 2023-06-23T18:26:52Z

What should we test?

If an order has progressed to at least payment state then submitting a change to it's address should recalculate tax correctly if the change of address would change it's tax category. Note: which address gets used for tax purposes depends on an admin configuration setting.

Ok, someone needs to help me out here.

If the delivery address changes, then a different tax rate may apply. The tax category (of the product) remains unchanged. This is probably what is meant here, right?
Recalculating the tax due to a delivery address change is working before this PR already. What exactly has changed?
I am not aware of such admin configuration setting to select which address gets used. I know that tax rates and tax categories are set in admin configuration. The tax category of the product is selected on the edit page of the product. The tax rate is selected according to the delivery address.

In summary I can't find anything that's not working - but I also didn't see any changes at all. So maybe we are good to merge this. Or please provide some more details for testing, @Matt-Yorkley. Or do you have any insights, @filipefurtad0?

Thanks!

rioug · 2023-06-25T23:52:22Z

If the delivery address changes, then a different tax rate may apply. The tax category (of the product) remains unchanged. This is probably what is meant here, right?

Correct

Recalculating the tax due to a delivery address change is working before this PR already. What exactly has changed?

The code that takes care of that has been moved around, but it is expected to work as before. So If you didn't find anything broken that means we are all good.

I am not aware of such admin configuration setting to select which address gets used. I know that tax rates and tax categories are set in admin configuration. The tax category of the product is selected on the edit page of the product. The tax rate is selected according to the delivery address.

I am not sure about this one.

rioug · 2023-06-27T10:25:07Z

@Matt-Yorkley can you clarify this ? see Konrad question above

Note: which address gets used for tax purposes depends on an admin configuration setting.

Matt-Yorkley · 2023-06-28T09:18:55Z

The code that takes care of that has been moved around, but it is expected to work as before. So If you didn't find anything broken that means we are all good.

That's right 👌

Note: which address gets used for tax purposes depends on an admin configuration setting.

Hmmm that's strange, I thought there was an option to set the value of Spree::Config[:tax_using_ship_address] in the configuration page but can't find it now. Maybe it got removed at some point?

Anyway, I think we can merge?

Matt-Yorkley added 2 commits June 13, 2023 17:35

Remove angular loading element in darkwarm layout

bf912ae

Move tax charge logic out of checkout controller and update payment t…

074eb4b

…ransition

Matt-Yorkley self-assigned this Jun 13, 2023

Matt-Yorkley changed the title ~~Vouchers~~ Vouchers part 1 Jun 13, 2023

Matt-Yorkley added 14 commits June 14, 2023 10:19

Tidy up checkout tax spec

a0d0f8f

Reorder callback definitions

65b6a75

Move taxing of admin adjustments out of customer details controller

13a22c5

Extract before_payment_state? method

a0f23fc

Reapply taxes at model level if order address changes

c5dfecb

This should be done at the model level

Don't try to cancel shipments that might already be cancelled

06c9697

Reload order after cancelling it in tests that check shipment states

837e581

Don't tax fees before payment state

6fa3811

Update adjustment spec to use orders past delivery state

ef09492

Fix test setup in BOM spec

11382a5

Fix Rubocop complaint

52806a3

Use order total excluding discounts in voucher calculations

93df70c

Improve voucher #calculate tests

1a744de

Remove voucher processing from checkout edit action

202b004

Matt-Yorkley force-pushed the voucher-prep branch from 9eeed29 to 202b004 Compare June 14, 2023 10:24

Matt-Yorkley mentioned this pull request Jun 14, 2023

Vouchers part 2 #11003

Closed

Matt-Yorkley marked this pull request as ready for review June 15, 2023 10:44

mkllnk approved these changes Jun 16, 2023

View reviewed changes

dacook self-requested a review June 16, 2023 01:21

rioug approved these changes Jun 16, 2023

View reviewed changes

dacook reviewed Jun 16, 2023

View reviewed changes

app/controllers/split_checkout_controller.rb Outdated Show resolved Hide resolved

app/models/spree/order.rb Show resolved Hide resolved

app/models/spree/order.rb Show resolved Hide resolved

app/models/spree/order.rb Show resolved Hide resolved

dacook approved these changes Jun 16, 2023

View reviewed changes

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Jun 16, 2023

drummer83 self-assigned this Jun 21, 2023

drummer83 added the pr-staged-uk staging.openfoodnetwork.org.uk label Jun 21, 2023

drummer83 mentioned this pull request Jun 21, 2023

Error 422 on /admin/products #11095

Closed

drummer83 added blocked pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-uk staging.openfoodnetwork.org.uk blocked pr-staged-au staging.openfoodnetwork.org.au labels Jun 21, 2023

drummer83 removed their assignment Jun 21, 2023

filipefurtad0 mentioned this pull request Jun 21, 2023

[staging-FR] Snail on checkout #11099

Open

drummer83 self-assigned this Jun 23, 2023

drummer83 added pr-staged-uk staging.openfoodnetwork.org.uk and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jun 23, 2023

drummer83 removed their assignment Jun 23, 2023

drummer83 added feedback-needed and removed pr-staged-uk staging.openfoodnetwork.org.uk labels Jun 23, 2023

rioug added the priority We focus on this issue right now label Jun 27, 2023

Matt-Yorkley merged commit a0a1f8f into master Jun 28, 2023

Matt-Yorkley deleted the voucher-prep branch June 28, 2023 10:30

Matt-Yorkley mentioned this pull request Jun 28, 2023

Vouchers part 2 #11135

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vouchers part 1 #11002

Vouchers part 1 #11002

Matt-Yorkley commented Jun 13, 2023 •

edited

Loading

mkllnk left a comment

mkllnk Jun 15, 2023

mkllnk Jun 15, 2023

Matt-Yorkley Jun 16, 2023 •

edited

Loading

rioug left a comment

rioug Jun 16, 2023

dacook left a comment

dacook Jun 16, 2023

drummer83 commented Jun 23, 2023 •

edited

Loading

What should we test?

rioug commented Jun 25, 2023

rioug commented Jun 27, 2023

Matt-Yorkley commented Jun 28, 2023

Vouchers part 1 #11002

Vouchers part 1 #11002

Conversation

Matt-Yorkley commented Jun 13, 2023 • edited Loading

What? Why?

Review notes

What should we test?

Release notes

mkllnk left a comment

Choose a reason for hiding this comment

mkllnk Jun 15, 2023

Choose a reason for hiding this comment

mkllnk Jun 15, 2023

Choose a reason for hiding this comment

Matt-Yorkley Jun 16, 2023 • edited Loading

Choose a reason for hiding this comment

rioug left a comment

Choose a reason for hiding this comment

rioug Jun 16, 2023

Choose a reason for hiding this comment

dacook left a comment

Choose a reason for hiding this comment

dacook Jun 16, 2023

Choose a reason for hiding this comment

drummer83 commented Jun 23, 2023 • edited Loading

What should we test?

rioug commented Jun 25, 2023

rioug commented Jun 27, 2023

Matt-Yorkley commented Jun 28, 2023

Matt-Yorkley commented Jun 13, 2023 •

edited

Loading

Matt-Yorkley Jun 16, 2023 •

edited

Loading

drummer83 commented Jun 23, 2023 •

edited

Loading