From 04bbea53e125a86f993b33687212b4cb6c19980a Mon Sep 17 00:00:00 2001
From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com>
Date: Fri, 2 Jun 2023 00:58:06 +0100
Subject: [PATCH 01/24] Move voucher processing out of checkout controller

---
 spec/requests/voucher_adjustments_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb
index 8c93fb3f69..5260996a4b 100644
--- a/spec/requests/voucher_adjustments_spec.rb
+++ b/spec/requests/voucher_adjustments_spec.rb
@@ -83,7 +83,7 @@ describe VoucherAdjustmentsController, type: :request do
       expect(response).to be_successful
     end
 
-    context "when adjustment doesn't exits" do
+    context "when adjustment doesn't exist" do
       it "does nothing" do
         delete "/voucher_adjustments/-1"
 

From 60c0c54eb29303093dae0d4797d5785ed917bede Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 26 Jun 2023 14:26:23 +1000
Subject: [PATCH 02/24] Update create_adjustment to create with an amount of 0

Amount calculation is handled by VoucherAdjustmentService.calculate
---
 app/models/voucher.rb       | 7 +++----
 spec/models/voucher_spec.rb | 4 ++--
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/app/models/voucher.rb b/app/models/voucher.rb
index 1199ad0820..60bf1b8cf2 100644
--- a/app/models/voucher.rb
+++ b/app/models/voucher.rb
@@ -21,12 +21,11 @@ class Voucher < ApplicationRecord
   # but vouchers have complicated calculation so we can't easily use Spree::Calculator. We keep
   # the same method to stay as consistent as possible.
   #
-  # Creates a new voucher adjustment for the given order
+  # Creates a new voucher adjustment for the given order with an amount of 0
+  # The amount will be calculated via VoucherAdjustmentsService.calculate
   def create_adjustment(label, order)
-    amount = compute_amount(order)
-
     adjustment_attributes = {
-      amount: amount,
+      amount: 0,
       originator: self,
       order: order,
       label: label,
diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb
index a41ec65812..0c62a81496 100644
--- a/spec/models/voucher_spec.rb
+++ b/spec/models/voucher_spec.rb
@@ -45,8 +45,8 @@ describe Voucher do
     let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 25) }
     let(:order) { create(:order_with_line_items, line_items_count: 3, distributor: enterprise) }
 
-    it 'includes the full voucher amount' do
-      expect(adjustment.amount.to_f).to eq(-25.0)
+    it 'includes an amount of 0' do
+      expect(adjustment.amount.to_f).to eq(0.0)
     end
 
     it 'has no included_tax' do

From 87790b29ae06abf0c226a6e641accb2bedc162a9 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 26 Jun 2023 14:30:10 +1000
Subject: [PATCH 03/24] Fix VoucherAdjustmentsService.calculate so we can call
 it mutiple time

It now uses `order.pre_discount_total` to make any calculation, that
means you can call it multiple time and you will still get the same
adjustment amount, included_tax and tax adjustment when needed. This
is assuming nothing changed on the order.
---
 app/services/voucher_adjustments_service.rb   | 24 ++++++-----
 .../voucher_adjustments_service_spec.rb       | 40 ++++++++++---------
 2 files changed, 34 insertions(+), 30 deletions(-)

diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb
index ccadb8eb75..aadc4bf91c 100644
--- a/app/services/voucher_adjustments_service.rb
+++ b/app/services/voucher_adjustments_service.rb
@@ -10,7 +10,7 @@ class VoucherAdjustmentsService
     # We only support one voucher per order right now, we could just loop on voucher_adjustments
     adjustment = order.voucher_adjustments.first
 
-    # Recalculate value
+    # Calculate value
     amount = adjustment.originator.compute_amount(order)
 
     # It is quite possible to have an order with both tax included in and tax excluded from price.
@@ -23,33 +23,35 @@ class VoucherAdjustmentsService
       handle_tax_included_in_price(order, amount)
     else
       adjustment.amount = amount
+      adjustment.save
     end
-
-    # Move to closed state
-    adjustment.close
   end
 
   def self.handle_tax_excluded_from_price(order, amount)
-    voucher_rate = amount / order.total
+    voucher_rate = amount / order.pre_discount_total
+
+    adjustment = order.voucher_adjustments.first
 
     # Adding the voucher tax part
     tax_amount = voucher_rate * order.additional_tax_total
 
-    adjustment = order.voucher_adjustments.first
     adjustment_attributes = {
-      amount: tax_amount,
       originator: adjustment.originator,
       order: order,
       label: "Tax #{adjustment.label}",
       mandatory: false,
-      state: 'closed',
+      state: 'open',
       tax_category: nil,
       included_tax: 0
     }
-    order.adjustments.create(adjustment_attributes)
+
+    # Update the amount if tax adjustment already exist, create if not
+    tax_adjustment = order.adjustments.find_or_initialize_by(adjustment_attributes)
+    tax_adjustment.amount = tax_amount
+    tax_adjustment.save
 
     # Update the adjustment amount
-    amount = voucher_rate * (order.total - order.additional_tax_total)
+    amount = voucher_rate * (order.pre_discount_total - order.additional_tax_total)
 
     adjustment.update_columns(
       amount: amount,
@@ -58,7 +60,7 @@ class VoucherAdjustmentsService
   end
 
   def self.handle_tax_included_in_price(order, amount)
-    voucher_rate = amount / order.total
+    voucher_rate = amount / order.pre_discount_total
     included_tax = voucher_rate * order.included_tax_total
 
     # Update Adjustment
diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb
index fa2c1bb951..e84286c3b7 100644
--- a/spec/services/voucher_adjustments_service_spec.rb
+++ b/spec/services/voucher_adjustments_service_spec.rb
@@ -22,7 +22,7 @@ describe VoucherAdjustmentsService do
       end
     end
 
-    context 'with price included in order price' do
+    context 'with tax included in order price' do
       subject { order.voucher_adjustments.first }
 
       let(:order) do
@@ -51,18 +51,14 @@ describe VoucherAdjustmentsService do
 
       it 'updates the adjustment included_tax' do
         # voucher_rate = amount / order.total
-        # -10 / 150 = -0.066666667
+        # -10 / 160 = -0.0625
         # included_tax = voucher_rate * order.included_tax_total
-        # -0.66666666 * 10 = -0.67
-        expect(subject.included_tax.to_f).to eq(-0.67)
-      end
-
-      it 'moves the adjustment state to closed' do
-        expect(subject.state).to eq('closed')
+        # -0.0625 * 10 = -0.625
+        expect(subject.included_tax.to_f).to eq(-0.63)
       end
     end
 
-    context 'with price not included in order price' do
+    context 'with tax not included in order price' do
       let(:order) do
         create(
           :order_with_taxes,
@@ -87,28 +83,34 @@ describe VoucherAdjustmentsService do
         VoucherAdjustmentsService.calculate(order)
       end
 
-      it 'includes amount withou tax' do
+      it 'includes amount without tax' do
         adjustment = order.voucher_adjustments.first
         # voucher_rate = amount / order.total
-        # -10 / 161 = -0.062111801
+        # -10 / 171 = -0.058479532
         # amount = voucher_rate * (order.total - order.additional_tax_total)
-        # -0.062111801 * (161 -11) = -9.32
-        expect(adjustment.amount.to_f).to eq(-9.32)
+        # -0.058479532 * (171 -11) = -9.36
+        expect(adjustment.amount.to_f).to eq(-9.36)
       end
 
       it 'creates a tax adjustment' do
         # voucher_rate = amount / order.total
-        # -10 / 161 = -0.062111801
+        # -10 / 171 = -0.058479532
         # amount = voucher_rate * order.additional_tax_total
-        # -0.0585 * 11 = -0.68
+        # -0.058479532 * 11 = -0.64
         tax_adjustment = order.voucher_adjustments.second
-        expect(tax_adjustment.amount.to_f).to eq(-0.68)
+        expect(tax_adjustment.amount.to_f).to eq(-0.64)
         expect(tax_adjustment.label).to match("Tax")
       end
 
-      it 'moves the adjustment state to closed' do
-        adjustment = order.voucher_adjustments.first
-        expect(adjustment.state).to eq('closed')
+      context "when re calculating" do
+        it "updates the tax adjustment" do
+          order.update_columns(item_total: 200)
+          tax_adjustment = order.voucher_adjustments.second
+
+          expect do
+            VoucherAdjustmentsService.calculate(order)
+          end.to change { tax_adjustment.reload.amount }
+        end
       end
     end
 

From 366cca7984a086b992dc59a08a50877d2ede1bc7 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 26 Jun 2023 14:43:25 +1000
Subject: [PATCH 04/24] Prevent voucher adjustment from bein updated when
 update is called

---
 .../app/services/order_management/order/updater.rb           | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb
index fc727d7e35..47d17f9877 100644
--- a/engines/order_management/app/services/order_management/order/updater.rb
+++ b/engines/order_management/app/services/order_management/order/updater.rb
@@ -137,7 +137,10 @@ module OrderManagement
       end
 
       def update_all_adjustments
-        order.all_adjustments.reload.each(&:update_adjustment!)
+        # Voucher are modelled as a Spree::Adjustment but  they don't behave like all the other
+        # adjustments, so we don't want voucher adjustment to be updated here.
+        # Calculation are handled by VoucherAdjustmentsService.calculate
+        order.all_adjustments.non_voucher.reload.each(&:update_adjustment!)
       end
 
       def before_save_hook

From ca7dcb82b831ecd942d53f91b5767ecce532a57d Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 26 Jun 2023 14:48:22 +1000
Subject: [PATCH 05/24] Apply voucher after transitionning to the confirmation
 step

Testing that VoucherAdjustmentsService.calculate has been called after a
transition doens't work, skipping test for now.
---
 app/models/spree/order/checkout.rb | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb
index 3575dc1ae0..030df13964 100644
--- a/app/models/spree/order/checkout.rb
+++ b/app/models/spree/order/checkout.rb
@@ -81,6 +81,12 @@ module Spree
                 order.create_tax_charge!
                 order.update_totals_and_states
               end
+
+              after_transition to: :confirmation do |order|
+                VoucherAdjustmentsService.calculate(order)
+                order.update_totals_and_states
+              end
+
               after_transition to: :complete, do: :finalize!
               after_transition to: :resumed,  do: :after_resume
               after_transition to: :canceled, do: :after_cancel

From edabc56b5cf3c4c1d471d3582172b6708aca39c0 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 26 Jun 2023 14:51:01 +1000
Subject: [PATCH 06/24] Add voucher calculation after updating an order

This is to make sure the voucher are recalculated when a customer
update the order content from the `/cart` page. This is tested
with system test
---
 app/controllers/spree/orders_controller.rb | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb
index 01779e11cb..57b4983fde 100644
--- a/app/controllers/spree/orders_controller.rb
+++ b/app/controllers/spree/orders_controller.rb
@@ -69,6 +69,10 @@ module Spree
       if @order.contents.update_cart(order_params)
         @order.recreate_all_fees! # Enterprise fees on line items and on the order itself
 
+        # Re apply the voucher
+        VoucherAdjustmentsService.calculate(@order)
+        @order.update_totals_and_states
+
         if @order.complete?
           @order.update_payment_fees!
           @order.create_tax_charge!

From a8062e951a52dee5a42e7b12a5999e7f6eee8a5a Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 26 Jun 2023 14:54:20 +1000
Subject: [PATCH 07/24] Add a scenario to make sure voucher adjustment a
 recalculated

This is specificly for voucher with a tax component, but it will
also become relevant for percentage based voucher.
---
 .../consumer/split_checkout_tax_incl_spec.rb  | 71 +++++++++++++++++--
 1 file changed, 66 insertions(+), 5 deletions(-)

diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb
index 6663c6a970..2cb1828830 100644
--- a/spec/system/consumer/split_checkout_tax_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb
@@ -49,7 +49,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
   let!(:order_within_zone) {
     create(:order, order_cycle: order_cycle, distributor: distributor, user: user_within_zone,
                    bill_address: address_within_zone, ship_address: address_within_zone,
-                   state: "cart", line_items: [create(:line_item, variant: variant_with_tax)])
+                   state: "cart", line_items: [create(:line_item, variant: variant_with_tax, quantity: 1)])
   }
   let!(:order_outside_zone) {
     create(:order, order_cycle: order_cycle, distributor: distributor, user: user_outside_zone,
@@ -139,11 +139,65 @@ describe "As a consumer, I want to see adjustment breakdown" do
           end
 
           # DB check
-          order_within_zone.reload
-          voucher_adjustment = order_within_zone.voucher_adjustments.first
+          assert_db_voucher_adjustment(-10.00, -1.15)
+        end
 
-          expect(voucher_adjustment.amount.to_f).to eq(-10)
-          expect(voucher_adjustment.included_tax.to_f).to eq(-1.15)
+        describe "moving between summary to summary via edit cart" do
+          let!(:voucher) do
+            create(:voucher, code: 'good_code', enterprise: distributor, amount: 15)
+          end
+
+          it "recalculate the tax component properly" do
+            visit checkout_step_path(:details)
+            proceed_to_payment
+
+            # add Voucher
+            fill_in "Enter voucher code", with: voucher.code
+            click_button("Apply")
+
+            proceed_to_summary
+
+            assert_db_voucher_adjustment(-10.00, -1.15)
+
+            # Click on edit link
+            within "div", text: /Order details/ do
+              # It's a bit brittle, but the scoping doesn't seem to work
+              all(".summary-edit").last.click
+            end
+
+            # Update quantity
+            within ".cart-item-quantity" do
+              input = find(".line_item_quantity")
+              input.send_keys :up
+            end
+
+            click_button("Update")
+
+            # Check adjustment has been recalculated
+            assert_db_voucher_adjustment(-15.00, -1.73)
+
+            within "#cart-container" do
+              click_link("Checkout")
+            end
+
+            # Go back to payment step
+            proceed_to_payment
+
+            # Check voucher is still there
+            expect(page).to have_content("$15.00 Voucher")
+
+            # Go to summary
+            proceed_to_summary
+
+            # Check voucher value
+            within ".summary-right" do
+              expect(page).to have_content "good_code"
+              expect(page).to have_content "-15"
+            end
+
+            # Check adjustment has been recalculated, we are not expecting any changes here
+            assert_db_voucher_adjustment(-15.00, -1.73)
+          end
         end
       end
     end
@@ -190,4 +244,11 @@ describe "As a consumer, I want to see adjustment breakdown" do
     expect(order_outside_zone.included_tax_total).to eq(0.0)
     expect(order_outside_zone.additional_tax_total).to eq(0.0)
   end
+
+  def assert_db_voucher_adjustment(amount, tax_amount)
+    adjustment = order_within_zone.voucher_adjustments.first
+    #adjustment.reload
+    expect(adjustment.amount.to_f).to eq(amount)
+    expect(adjustment.included_tax.to_f).to eq(tax_amount)
+  end
 end

From 789ce39ff4bbe5e2378987b89ade7621b54da595 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 26 Jun 2023 15:51:12 +1000
Subject: [PATCH 08/24] Fixing Rubocop errors

---
 spec/system/consumer/split_checkout_tax_incl_spec.rb | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb
index 2cb1828830..fec1b08098 100644
--- a/spec/system/consumer/split_checkout_tax_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb
@@ -47,9 +47,12 @@ describe "As a consumer, I want to see adjustment breakdown" do
                             calculator: Calculator::FlatRate.new(preferred_amount: 0.00))
   }
   let!(:order_within_zone) {
-    create(:order, order_cycle: order_cycle, distributor: distributor, user: user_within_zone,
-                   bill_address: address_within_zone, ship_address: address_within_zone,
-                   state: "cart", line_items: [create(:line_item, variant: variant_with_tax, quantity: 1)])
+    create(
+      :order,
+      order_cycle: order_cycle, distributor: distributor, user: user_within_zone,
+      bill_address: address_within_zone, ship_address: address_within_zone,
+      state: "cart", line_items: [create(:line_item, variant: variant_with_tax, quantity: 1)]
+    )
   }
   let!(:order_outside_zone) {
     create(:order, order_cycle: order_cycle, distributor: distributor, user: user_outside_zone,
@@ -247,7 +250,6 @@ describe "As a consumer, I want to see adjustment breakdown" do
 
   def assert_db_voucher_adjustment(amount, tax_amount)
     adjustment = order_within_zone.voucher_adjustments.first
-    #adjustment.reload
     expect(adjustment.amount.to_f).to eq(amount)
     expect(adjustment.included_tax.to_f).to eq(tax_amount)
   end

From 751056ba39f79fc4ca8465a02b05023eda6b851b Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 30 Jun 2023 11:12:58 +1000
Subject: [PATCH 09/24] As per review comment, spell out voucher code when
 entering a voucher

Plus, update other related spec to be consistent
---
 spec/system/consumer/split_checkout_spec.rb              | 4 ++--
 spec/system/consumer/split_checkout_tax_incl_spec.rb     | 4 ++--
 spec/system/consumer/split_checkout_tax_not_incl_spec.rb | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb
index 148f2a212c..2f957238d6 100644
--- a/spec/system/consumer/split_checkout_spec.rb
+++ b/spec/system/consumer/split_checkout_spec.rb
@@ -735,7 +735,7 @@ describe "As a consumer, I want to checkout my order" do
           describe "adding voucher to the order" do
             shared_examples "adding voucher to the order" do
               before do
-                fill_in "Enter voucher code", with: voucher.code
+                fill_in "Enter voucher code", with: "some_code"
                 click_button("Apply")
               end
 
@@ -756,7 +756,7 @@ describe "As a consumer, I want to checkout my order" do
               it_behaves_like "adding voucher to the order"
 
               it "shows a warning message" do
-                fill_in "Enter voucher code", with: voucher.code
+                fill_in "Enter voucher code", with: "some_code"
                 click_button("Apply")
 
                 expect(page).to have_content(
diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb
index fec1b08098..3b8275ee4e 100644
--- a/spec/system/consumer/split_checkout_tax_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb
@@ -123,7 +123,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
           click_button "Next - Payment method"
 
           # add Voucher
-          fill_in "Enter voucher code", with: voucher.code
+          fill_in "Enter voucher code", with: "some_code"
           click_button("Apply")
 
           # Choose payment
@@ -155,7 +155,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
             proceed_to_payment
 
             # add Voucher
-            fill_in "Enter voucher code", with: voucher.code
+            fill_in "Enter voucher code", with: "good_code"
             click_button("Apply")
 
             proceed_to_summary
diff --git a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
index 046a6ac483..3f44c4328f 100644
--- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
@@ -129,7 +129,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
           click_button "Next - Payment method"
 
           # add Voucher
-          fill_in "Enter voucher code", with: voucher.code
+          fill_in "Enter voucher code", with: "some_code"
           click_button("Apply")
           expect(page).to have_selector ".voucher-added"
 

From 41271192e1f179f0a2c4af109b031992a873f70c Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 30 Jun 2023 11:34:06 +1000
Subject: [PATCH 10/24] Per review, Makes quantity change more explicit

---
 spec/system/consumer/split_checkout_tax_incl_spec.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb
index 3b8275ee4e..44e85ad113 100644
--- a/spec/system/consumer/split_checkout_tax_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb
@@ -170,8 +170,7 @@ describe "As a consumer, I want to see adjustment breakdown" do
 
             # Update quantity
             within ".cart-item-quantity" do
-              input = find(".line_item_quantity")
-              input.send_keys :up
+              fill_in "order_line_items_attributes_0_quantity", with: "2"
             end
 
             click_button("Update")

From 3d9542fec24203d87bd14338a6c2129cb06ceb00 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 30 Jun 2023 11:44:13 +1000
Subject: [PATCH 11/24] Per review, rename amount to adjustment_amount

It was confusing as I was re assigning a variable given as a parameter
---
 app/services/voucher_adjustments_service.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb
index aadc4bf91c..3b0201d0af 100644
--- a/app/services/voucher_adjustments_service.rb
+++ b/app/services/voucher_adjustments_service.rb
@@ -51,10 +51,10 @@ class VoucherAdjustmentsService
     tax_adjustment.save
 
     # Update the adjustment amount
-    amount = voucher_rate * (order.pre_discount_total - order.additional_tax_total)
+    adjustment_amount = voucher_rate * (order.pre_discount_total - order.additional_tax_total)
 
     adjustment.update_columns(
-      amount: amount,
+      amount: adjustment_amount,
       updated_at: Time.zone.now
     )
   end

From 5a59396a41181934e1acae2b9c04db0861e87d9e Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 30 Jun 2023 12:00:47 +1000
Subject: [PATCH 12/24] Remove call to VoucherAdjustmentService when creating a
 voucher

Voucher will be automatically applied when the order transition to
the confirmation step. No need to do the work twice.
---
 app/controllers/voucher_adjustments_controller.rb | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb
index c34570269c..9b6a2bac26 100644
--- a/app/controllers/voucher_adjustments_controller.rb
+++ b/app/controllers/voucher_adjustments_controller.rb
@@ -5,9 +5,6 @@ class VoucherAdjustmentsController < BaseController
 
   def create
     if add_voucher
-      VoucherAdjustmentsService.calculate(@order)
-      @order.update_totals_and_states
-
       update_payment_section
     elsif @order.errors.present?
       render_error

From 0e0850ef49a3d85beca764021dd29c1234be58b9 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 30 Jun 2023 14:22:42 +1000
Subject: [PATCH 13/24] Add specs to cover re calculation

It is important that the calculated voucher adjustments don't change
if they are recalculated and it is equally important that they are
updated if the order has changed
---
 .../voucher_adjustments_service_spec.rb       | 67 +++++++++++++++++--
 1 file changed, 60 insertions(+), 7 deletions(-)

diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb
index e84286c3b7..7cc56e0b81 100644
--- a/spec/services/voucher_adjustments_service_spec.rb
+++ b/spec/services/voucher_adjustments_service_spec.rb
@@ -56,6 +56,38 @@ describe VoucherAdjustmentsService do
         # -0.0625 * 10 = -0.625
         expect(subject.included_tax.to_f).to eq(-0.63)
       end
+
+      context "when re calculating" do
+        it "does not update the adjustment amount" do
+          expect do
+            VoucherAdjustmentsService.calculate(order)
+          end.not_to change { subject.reload.amount }
+        end
+
+        it "does not update the tax adjustment" do
+          expect do
+            VoucherAdjustmentsService.calculate(order)
+          end.not_to change { subject.reload.included_tax }
+        end
+
+        context "when the order changed" do
+          before do
+            order.update_columns(item_total: 200)
+          end
+
+          it "does update the adjustment amount" do
+            expect do
+              VoucherAdjustmentsService.calculate(order)
+            end.not_to change { subject.reload.amount }
+          end
+
+          it "updates the tax adjustment" do
+            expect do
+              VoucherAdjustmentsService.calculate(order)
+            end.to change { subject.reload.included_tax }
+          end
+        end
+      end
     end
 
     context 'with tax not included in order price' do
@@ -70,6 +102,8 @@ describe VoucherAdjustmentsService do
           tax_rate_name: "Tax 1"
         )
       end
+      let(:adjustment) { order.voucher_adjustments.first }
+      let(:tax_adjustment) { order.voucher_adjustments.second }
 
       before do
         # create adjustment before tax are set
@@ -84,7 +118,6 @@ describe VoucherAdjustmentsService do
       end
 
       it 'includes amount without tax' do
-        adjustment = order.voucher_adjustments.first
         # voucher_rate = amount / order.total
         # -10 / 171 = -0.058479532
         # amount = voucher_rate * (order.total - order.additional_tax_total)
@@ -97,19 +130,39 @@ describe VoucherAdjustmentsService do
         # -10 / 171 = -0.058479532
         # amount = voucher_rate * order.additional_tax_total
         # -0.058479532 * 11 = -0.64
-        tax_adjustment = order.voucher_adjustments.second
         expect(tax_adjustment.amount.to_f).to eq(-0.64)
         expect(tax_adjustment.label).to match("Tax")
       end
 
       context "when re calculating" do
-        it "updates the tax adjustment" do
-          order.update_columns(item_total: 200)
-          tax_adjustment = order.voucher_adjustments.second
-
+        it "does not update the adjustment amount" do
           expect do
             VoucherAdjustmentsService.calculate(order)
-          end.to change { tax_adjustment.reload.amount }
+          end.not_to change { adjustment.reload.amount }
+        end
+
+        it "does not update the tax adjustment" do
+          expect do
+            VoucherAdjustmentsService.calculate(order)
+          end.not_to change { tax_adjustment.reload.amount }
+        end
+
+        context "when the order changed" do
+          before do
+            order.update_columns(item_total: 200)
+          end
+
+          it "updates the adjustment amount" do
+            expect do
+              VoucherAdjustmentsService.calculate(order)
+            end.to change { adjustment.reload.amount }
+          end
+
+          it "updates the tax adjustment" do
+            expect do
+              VoucherAdjustmentsService.calculate(order)
+            end.to change { tax_adjustment.reload.amount }
+          end
         end
       end
     end

From a584f9aaecd2e9d174af50c43b49d9a7b379150f Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 18 Jul 2023 14:39:01 +1000
Subject: [PATCH 14/24] Refactor VoucherAdjustmentsService

It's more inline with existing coding style
---
 app/controllers/spree/orders_controller.rb    |  2 +-
 app/models/spree/order/checkout.rb            |  2 +-
 app/models/voucher.rb                         |  2 +-
 app/services/voucher_adjustments_service.rb   | 48 ++++++++++---------
 .../voucher_adjustments_service_spec.rb       | 28 +++++------
 spec/system/consumer/split_checkout_spec.rb   |  2 +-
 6 files changed, 44 insertions(+), 40 deletions(-)

diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb
index 57b4983fde..24895ecf1a 100644
--- a/app/controllers/spree/orders_controller.rb
+++ b/app/controllers/spree/orders_controller.rb
@@ -70,7 +70,7 @@ module Spree
         @order.recreate_all_fees! # Enterprise fees on line items and on the order itself
 
         # Re apply the voucher
-        VoucherAdjustmentsService.calculate(@order)
+        VoucherAdjustmentsService.new(@order).calculate
         @order.update_totals_and_states
 
         if @order.complete?
diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb
index 030df13964..20eb10da04 100644
--- a/app/models/spree/order/checkout.rb
+++ b/app/models/spree/order/checkout.rb
@@ -83,7 +83,7 @@ module Spree
               end
 
               after_transition to: :confirmation do |order|
-                VoucherAdjustmentsService.calculate(order)
+                VoucherAdjustmentsService.new(order).calculate
                 order.update_totals_and_states
               end
 
diff --git a/app/models/voucher.rb b/app/models/voucher.rb
index 60bf1b8cf2..f74fa6101c 100644
--- a/app/models/voucher.rb
+++ b/app/models/voucher.rb
@@ -22,7 +22,7 @@ class Voucher < ApplicationRecord
   # the same method to stay as consistent as possible.
   #
   # Creates a new voucher adjustment for the given order with an amount of 0
-  # The amount will be calculated via VoucherAdjustmentsService.calculate
+  # The amount will be calculated via VoucherAdjustmentsService#calculate
   def create_adjustment(label, order)
     adjustment_attributes = {
       amount: 0,
diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb
index 3b0201d0af..3d23f8e11e 100644
--- a/app/services/voucher_adjustments_service.rb
+++ b/app/services/voucher_adjustments_service.rb
@@ -1,43 +1,49 @@
 # frozen_string_literal: true
 
 class VoucherAdjustmentsService
-  def self.calculate(order)
-    return if order.nil?
+  def initialize(order)
+    @order = order
+  end
+
+  def calculate
+    return if @order.nil?
 
     # Find open Voucher Adjustment
-    return if order.voucher_adjustments.empty?
+    return if @order.voucher_adjustments.empty?
 
     # We only support one voucher per order right now, we could just loop on voucher_adjustments
-    adjustment = order.voucher_adjustments.first
+    adjustment = @order.voucher_adjustments.first
 
     # Calculate value
-    amount = adjustment.originator.compute_amount(order)
+    amount = adjustment.originator.compute_amount(@order)
 
     # It is quite possible to have an order with both tax included in and tax excluded from price.
     # We should be able to caculate the relevant amount apply the current calculation.
     #
     # For now we just assume it is either all tax included in price or all tax excluded from price.
-    if order.additional_tax_total.positive?
-      handle_tax_excluded_from_price(order, amount)
-    elsif order.included_tax_total.positive?
-      handle_tax_included_in_price(order, amount)
+    if @order.additional_tax_total.positive?
+      handle_tax_excluded_from_price(amount)
+    elsif @order.included_tax_total.positive?
+      handle_tax_included_in_price(amount)
     else
       adjustment.amount = amount
       adjustment.save
     end
   end
 
-  def self.handle_tax_excluded_from_price(order, amount)
-    voucher_rate = amount / order.pre_discount_total
+  private
 
-    adjustment = order.voucher_adjustments.first
+  def handle_tax_excluded_from_price(amount)
+    voucher_rate = amount / @order.pre_discount_total
+
+    adjustment = @order.voucher_adjustments.first
 
     # Adding the voucher tax part
-    tax_amount = voucher_rate * order.additional_tax_total
+    tax_amount = voucher_rate * @order.additional_tax_total
 
     adjustment_attributes = {
       originator: adjustment.originator,
-      order: order,
+      order: @order,
       label: "Tax #{adjustment.label}",
       mandatory: false,
       state: 'open',
@@ -46,12 +52,12 @@ class VoucherAdjustmentsService
     }
 
     # Update the amount if tax adjustment already exist, create if not
-    tax_adjustment = order.adjustments.find_or_initialize_by(adjustment_attributes)
+    tax_adjustment = @order.adjustments.find_or_initialize_by(adjustment_attributes)
     tax_adjustment.amount = tax_amount
     tax_adjustment.save
 
     # Update the adjustment amount
-    adjustment_amount = voucher_rate * (order.pre_discount_total - order.additional_tax_total)
+    adjustment_amount = voucher_rate * (@order.pre_discount_total - @order.additional_tax_total)
 
     adjustment.update_columns(
       amount: adjustment_amount,
@@ -59,12 +65,12 @@ class VoucherAdjustmentsService
     )
   end
 
-  def self.handle_tax_included_in_price(order, amount)
-    voucher_rate = amount / order.pre_discount_total
-    included_tax = voucher_rate * order.included_tax_total
+  def handle_tax_included_in_price(amount)
+    voucher_rate = amount / @order.pre_discount_total
+    included_tax = voucher_rate * @order.included_tax_total
 
     # Update Adjustment
-    adjustment = order.voucher_adjustments.first
+    adjustment = @order.voucher_adjustments.first
 
     return unless amount != adjustment.amount || included_tax != 0
 
@@ -74,6 +80,4 @@ class VoucherAdjustmentsService
       updated_at: Time.zone.now
     )
   end
-
-  private_class_method :handle_tax_included_in_price, :handle_tax_excluded_from_price
 end
diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb
index 7cc56e0b81..0d61de9121 100644
--- a/spec/services/voucher_adjustments_service_spec.rb
+++ b/spec/services/voucher_adjustments_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe VoucherAdjustmentsService do
-  describe '.calculate' do
+  describe '#calculate' do
     let(:enterprise) { build(:enterprise) }
     let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) }
 
@@ -16,7 +16,7 @@ describe VoucherAdjustmentsService do
         voucher.create_adjustment(voucher.code, order)
         order.update_columns(item_total: 6)
 
-        VoucherAdjustmentsService.calculate(order)
+        VoucherAdjustmentsService.new(order).calculate
 
         expect(subject.amount.to_f).to eq(-6.0)
       end
@@ -46,7 +46,7 @@ describe VoucherAdjustmentsService do
         order.update_shipping_fees!
         order.update_order!
 
-        VoucherAdjustmentsService.calculate(order)
+        VoucherAdjustmentsService.new(order).calculate
       end
 
       it 'updates the adjustment included_tax' do
@@ -60,13 +60,13 @@ describe VoucherAdjustmentsService do
       context "when re calculating" do
         it "does not update the adjustment amount" do
           expect do
-            VoucherAdjustmentsService.calculate(order)
+            VoucherAdjustmentsService.new(order).calculate
           end.not_to change { subject.reload.amount }
         end
 
         it "does not update the tax adjustment" do
           expect do
-            VoucherAdjustmentsService.calculate(order)
+            VoucherAdjustmentsService.new(order).calculate()
           end.not_to change { subject.reload.included_tax }
         end
 
@@ -77,13 +77,13 @@ describe VoucherAdjustmentsService do
 
           it "does update the adjustment amount" do
             expect do
-              VoucherAdjustmentsService.calculate(order)
+              VoucherAdjustmentsService.new(order).calculate
             end.not_to change { subject.reload.amount }
           end
 
           it "updates the tax adjustment" do
             expect do
-              VoucherAdjustmentsService.calculate(order)
+              VoucherAdjustmentsService.new(order).calculate
             end.to change { subject.reload.included_tax }
           end
         end
@@ -114,7 +114,7 @@ describe VoucherAdjustmentsService do
         order.update_shipping_fees!
         order.update_order!
 
-        VoucherAdjustmentsService.calculate(order)
+        VoucherAdjustmentsService.new(order).calculate
       end
 
       it 'includes amount without tax' do
@@ -137,13 +137,13 @@ describe VoucherAdjustmentsService do
       context "when re calculating" do
         it "does not update the adjustment amount" do
           expect do
-            VoucherAdjustmentsService.calculate(order)
+            VoucherAdjustmentsService.new(order).calculate
           end.not_to change { adjustment.reload.amount }
         end
 
         it "does not update the tax adjustment" do
           expect do
-            VoucherAdjustmentsService.calculate(order)
+            VoucherAdjustmentsService.new(order).calculate
           end.not_to change { tax_adjustment.reload.amount }
         end
 
@@ -154,13 +154,13 @@ describe VoucherAdjustmentsService do
 
           it "updates the adjustment amount" do
             expect do
-              VoucherAdjustmentsService.calculate(order)
+              VoucherAdjustmentsService.new(order).calculate
             end.to change { adjustment.reload.amount }
           end
 
           it "updates the tax adjustment" do
             expect do
-              VoucherAdjustmentsService.calculate(order)
+              VoucherAdjustmentsService.new(order).calculate
             end.to change { tax_adjustment.reload.amount }
           end
         end
@@ -169,7 +169,7 @@ describe VoucherAdjustmentsService do
 
     context 'when no order given' do
       it "doesn't blow up" do
-        expect { VoucherAdjustmentsService.calculate(nil) }.to_not raise_error
+        expect { VoucherAdjustmentsService.new(nil).calculate }.to_not raise_error
       end
     end
 
@@ -177,7 +177,7 @@ describe VoucherAdjustmentsService do
       let(:order) { create(:order_with_line_items, line_items_count: 1, distributor: enterprise) }
 
       it "doesn't blow up" do
-        expect { VoucherAdjustmentsService.calculate(order) }.to_not raise_error
+        expect { VoucherAdjustmentsService.new(order).calculate }.to_not raise_error
       end
     end
   end
diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb
index 2f957238d6..8d19772eda 100644
--- a/spec/system/consumer/split_checkout_spec.rb
+++ b/spec/system/consumer/split_checkout_spec.rb
@@ -1119,7 +1119,7 @@ describe "As a consumer, I want to checkout my order" do
         before do
           voucher.create_adjustment(voucher.code, order)
           order.update_totals
-          VoucherAdjustmentsService.calculate(order)
+          VoucherAdjustmentsService.new(order).calculate
 
           visit checkout_step_path(:summary)
         end

From a5b2bc62939ce15389761003d1c7d8b9f362258f Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 18 Jul 2023 15:16:21 +1000
Subject: [PATCH 15/24] Per review, Refactor VoucherAdjustmentsService

Change #calculate to #update and clean up internal code
---
 app/controllers/spree/orders_controller.rb    |  2 +-
 app/models/spree/order/checkout.rb            |  2 +-
 app/models/voucher.rb                         |  2 +-
 app/services/voucher_adjustments_service.rb   | 22 +++++++++------
 .../voucher_adjustments_service_spec.rb       | 28 +++++++++----------
 spec/system/consumer/split_checkout_spec.rb   |  2 +-
 6 files changed, 31 insertions(+), 27 deletions(-)

diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb
index 24895ecf1a..501e6605dd 100644
--- a/app/controllers/spree/orders_controller.rb
+++ b/app/controllers/spree/orders_controller.rb
@@ -70,7 +70,7 @@ module Spree
         @order.recreate_all_fees! # Enterprise fees on line items and on the order itself
 
         # Re apply the voucher
-        VoucherAdjustmentsService.new(@order).calculate
+        VoucherAdjustmentsService.new(@order).update
         @order.update_totals_and_states
 
         if @order.complete?
diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb
index 20eb10da04..8d5ec88b5c 100644
--- a/app/models/spree/order/checkout.rb
+++ b/app/models/spree/order/checkout.rb
@@ -83,7 +83,7 @@ module Spree
               end
 
               after_transition to: :confirmation do |order|
-                VoucherAdjustmentsService.new(order).calculate
+                VoucherAdjustmentsService.new(order).update
                 order.update_totals_and_states
               end
 
diff --git a/app/models/voucher.rb b/app/models/voucher.rb
index f74fa6101c..1551c6807b 100644
--- a/app/models/voucher.rb
+++ b/app/models/voucher.rb
@@ -22,7 +22,7 @@ class Voucher < ApplicationRecord
   # the same method to stay as consistent as possible.
   #
   # Creates a new voucher adjustment for the given order with an amount of 0
-  # The amount will be calculated via VoucherAdjustmentsService#calculate
+  # The amount will be calculated via VoucherAdjustmentsService#update
   def create_adjustment(label, order)
     adjustment_attributes = {
       amount: 0,
diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb
index 3d23f8e11e..355058b9d6 100644
--- a/app/services/voucher_adjustments_service.rb
+++ b/app/services/voucher_adjustments_service.rb
@@ -5,7 +5,7 @@ class VoucherAdjustmentsService
     @order = order
   end
 
-  def calculate
+  def update
     return if @order.nil?
 
     # Find open Voucher Adjustment
@@ -41,6 +41,18 @@ class VoucherAdjustmentsService
     # Adding the voucher tax part
     tax_amount = voucher_rate * @order.additional_tax_total
 
+    update_tax_adjustment_for(adjustment, tax_amount)
+
+    # Update the adjustment amount
+    adjustment_amount = voucher_rate * (@order.pre_discount_total - @order.additional_tax_total)
+
+    adjustment.update_columns(
+      amount: adjustment_amount,
+      updated_at: Time.zone.now
+    )
+  end
+
+  def update_tax_adjustment_for(adjustment, tax_amount)
     adjustment_attributes = {
       originator: adjustment.originator,
       order: @order,
@@ -55,14 +67,6 @@ class VoucherAdjustmentsService
     tax_adjustment = @order.adjustments.find_or_initialize_by(adjustment_attributes)
     tax_adjustment.amount = tax_amount
     tax_adjustment.save
-
-    # Update the adjustment amount
-    adjustment_amount = voucher_rate * (@order.pre_discount_total - @order.additional_tax_total)
-
-    adjustment.update_columns(
-      amount: adjustment_amount,
-      updated_at: Time.zone.now
-    )
   end
 
   def handle_tax_included_in_price(amount)
diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb
index 0d61de9121..b83a18ab80 100644
--- a/spec/services/voucher_adjustments_service_spec.rb
+++ b/spec/services/voucher_adjustments_service_spec.rb
@@ -3,7 +3,7 @@
 require 'spec_helper'
 
 describe VoucherAdjustmentsService do
-  describe '#calculate' do
+  describe '#update' do
     let(:enterprise) { build(:enterprise) }
     let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) }
 
@@ -16,7 +16,7 @@ describe VoucherAdjustmentsService do
         voucher.create_adjustment(voucher.code, order)
         order.update_columns(item_total: 6)
 
-        VoucherAdjustmentsService.new(order).calculate
+        VoucherAdjustmentsService.new(order).update
 
         expect(subject.amount.to_f).to eq(-6.0)
       end
@@ -46,7 +46,7 @@ describe VoucherAdjustmentsService do
         order.update_shipping_fees!
         order.update_order!
 
-        VoucherAdjustmentsService.new(order).calculate
+        VoucherAdjustmentsService.new(order).update
       end
 
       it 'updates the adjustment included_tax' do
@@ -60,13 +60,13 @@ describe VoucherAdjustmentsService do
       context "when re calculating" do
         it "does not update the adjustment amount" do
           expect do
-            VoucherAdjustmentsService.new(order).calculate
+            VoucherAdjustmentsService.new(order).update
           end.not_to change { subject.reload.amount }
         end
 
         it "does not update the tax adjustment" do
           expect do
-            VoucherAdjustmentsService.new(order).calculate()
+            VoucherAdjustmentsService.new(order).update
           end.not_to change { subject.reload.included_tax }
         end
 
@@ -77,13 +77,13 @@ describe VoucherAdjustmentsService do
 
           it "does update the adjustment amount" do
             expect do
-              VoucherAdjustmentsService.new(order).calculate
+              VoucherAdjustmentsService.new(order).update
             end.not_to change { subject.reload.amount }
           end
 
           it "updates the tax adjustment" do
             expect do
-              VoucherAdjustmentsService.new(order).calculate
+              VoucherAdjustmentsService.new(order).update
             end.to change { subject.reload.included_tax }
           end
         end
@@ -114,7 +114,7 @@ describe VoucherAdjustmentsService do
         order.update_shipping_fees!
         order.update_order!
 
-        VoucherAdjustmentsService.new(order).calculate
+        VoucherAdjustmentsService.new(order).update
       end
 
       it 'includes amount without tax' do
@@ -137,13 +137,13 @@ describe VoucherAdjustmentsService do
       context "when re calculating" do
         it "does not update the adjustment amount" do
           expect do
-            VoucherAdjustmentsService.new(order).calculate
+            VoucherAdjustmentsService.new(order).update
           end.not_to change { adjustment.reload.amount }
         end
 
         it "does not update the tax adjustment" do
           expect do
-            VoucherAdjustmentsService.new(order).calculate
+            VoucherAdjustmentsService.new(order).update
           end.not_to change { tax_adjustment.reload.amount }
         end
 
@@ -154,13 +154,13 @@ describe VoucherAdjustmentsService do
 
           it "updates the adjustment amount" do
             expect do
-              VoucherAdjustmentsService.new(order).calculate
+              VoucherAdjustmentsService.new(order).update
             end.to change { adjustment.reload.amount }
           end
 
           it "updates the tax adjustment" do
             expect do
-              VoucherAdjustmentsService.new(order).calculate
+              VoucherAdjustmentsService.new(order).update
             end.to change { tax_adjustment.reload.amount }
           end
         end
@@ -169,7 +169,7 @@ describe VoucherAdjustmentsService do
 
     context 'when no order given' do
       it "doesn't blow up" do
-        expect { VoucherAdjustmentsService.new(nil).calculate }.to_not raise_error
+        expect { VoucherAdjustmentsService.new(nil).update }.to_not raise_error
       end
     end
 
@@ -177,7 +177,7 @@ describe VoucherAdjustmentsService do
       let(:order) { create(:order_with_line_items, line_items_count: 1, distributor: enterprise) }
 
       it "doesn't blow up" do
-        expect { VoucherAdjustmentsService.new(order).calculate }.to_not raise_error
+        expect { VoucherAdjustmentsService.new(order).update }.to_not raise_error
       end
     end
   end
diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb
index 8d19772eda..f2c563bf52 100644
--- a/spec/system/consumer/split_checkout_spec.rb
+++ b/spec/system/consumer/split_checkout_spec.rb
@@ -1119,7 +1119,7 @@ describe "As a consumer, I want to checkout my order" do
         before do
           voucher.create_adjustment(voucher.code, order)
           order.update_totals
-          VoucherAdjustmentsService.new(order).calculate
+          VoucherAdjustmentsService.new(order).update
 
           visit checkout_step_path(:summary)
         end

From 895f534afa4bdc891b22744188b5e00edcda3929 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 27 Jul 2023 16:02:21 +1000
Subject: [PATCH 16/24] Remove unnecessary extra page load

---
 spec/system/consumer/split_checkout_spec.rb | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb
index f2c563bf52..384586689b 100644
--- a/spec/system/consumer/split_checkout_spec.rb
+++ b/spec/system/consumer/split_checkout_spec.rb
@@ -724,15 +724,16 @@ describe "As a consumer, I want to checkout my order" do
             create(:voucher, code: 'some_code', enterprise: distributor, amount: 15)
           end
 
-          before do
-            visit checkout_step_path(:payment)
-          end
-
           it "shows voucher input" do
+            visit checkout_step_path(:payment)
             expect(page).to have_content "Apply voucher"
           end
 
           describe "adding voucher to the order" do
+            before do
+              visit checkout_step_path(:payment)
+            end
+
             shared_examples "adding voucher to the order" do
               before do
                 fill_in "Enter voucher code", with: "some_code"
@@ -779,7 +780,6 @@ describe "As a consumer, I want to checkout my order" do
           describe "removing voucher from order" do
             before do
               voucher.create_adjustment(voucher.code, order)
-              # Reload the page so we pickup the voucher
               visit checkout_step_path(:payment)
             end
 

From be1a72743a91a692df41d599f586e061681d3b81 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 27 Jul 2023 16:04:25 +1000
Subject: [PATCH 17/24] Pending spec: vouchers should not require payment

---
 spec/system/consumer/split_checkout_spec.rb | 39 +++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb
index 384586689b..5460aad7ce 100644
--- a/spec/system/consumer/split_checkout_spec.rb
+++ b/spec/system/consumer/split_checkout_spec.rb
@@ -726,6 +726,7 @@ describe "As a consumer, I want to checkout my order" do
 
           it "shows voucher input" do
             visit checkout_step_path(:payment)
+            expect(page).to have_field "Enter voucher code"
             expect(page).to have_content "Apply voucher"
           end
 
@@ -765,6 +766,17 @@ describe "As a consumer, I want to checkout my order" do
                   "you may not be able to spend the remaining value."
                 )
               end
+
+              it "proceeds without requiring payment" do
+                fill_in "Enter voucher code", with: "some_code"
+                click_button("Apply")
+
+                pending
+                expect(page).to have_content "No payment required"
+                click_button "Next - Order summary"
+                # Expect to be on the Order Summary page
+                expect(page).to have_content "Delivery details"
+              end
             end
 
             context "voucher doesn't exist" do
@@ -781,19 +793,42 @@ describe "As a consumer, I want to checkout my order" do
             before do
               voucher.create_adjustment(voucher.code, order)
               visit checkout_step_path(:payment)
-            end
 
-            it "removes voucher" do
               accept_confirm "Are you sure you want to remove the voucher?" do
                 click_on "Remove code"
               end
+            end
 
+            it "removes voucher" do
               within '#voucher-section' do
                 expect(page).to have_button("Apply", disabled: true)
+                expect(page).to have_field "Enter voucher code" # Currently no confirmation msg
               end
 
               expect(order.voucher_adjustments.length).to eq(0)
             end
+
+            it "can re-enter a voucher" do
+              # Re-enter a voucher code
+              fill_in "Enter voucher code", with: "some_code"
+              click_button("Apply")
+
+              expect(page).to have_content("$15.00 Voucher")
+              expect(order.reload.voucher_adjustments.length).to eq(1)
+              pending
+              expect(page).to have_content "No payment required"
+
+              click_button "Next - Order summary"
+              # Expect to be on the Order Summary page
+              expect(page).to have_content "Delivery details"
+            end
+
+            it "can proceed with payment" do
+              choose "Payment with Fee"
+              click_button "Next - Order summary"
+              # Expect to be on the Order Summary page
+              expect(page).to have_content "Delivery details"
+            end
           end
         end
       end

From ef62fb885db855651f44797a9655e5dc63dd4368 Mon Sep 17 00:00:00 2001
From: David Cook <david@redcliffs.net>
Date: Thu, 27 Jul 2023 16:35:43 +1000
Subject: [PATCH 18/24] Check if record actually saved

I'm not sure if it will ever make a difference, but I think this makes more sense.
---
 app/controllers/voucher_adjustments_controller.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb
index 9b6a2bac26..329ca05712 100644
--- a/app/controllers/voucher_adjustments_controller.rb
+++ b/app/controllers/voucher_adjustments_controller.rb
@@ -38,7 +38,7 @@ class VoucherAdjustmentsController < BaseController
 
     adjustment = voucher.create_adjustment(voucher.code, @order)
 
-    unless adjustment.valid?
+    unless adjustment.persisted?
       @order.errors.add(:voucher_code, I18n.t('split_checkout.errors.add_voucher_error'))
       adjustment.errors.each { |error| @order.errors.import(error) }
       return false

From f35001dacc5369e7909d6ecc14ee51e28be41510 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 28 Jul 2023 14:27:27 +1000
Subject: [PATCH 19/24] Update voucher adjustment and order total when adding a
 voucher

This is needed so we can check if payment is needed and display
no payment required when no payment needed
---
 app/controllers/voucher_adjustments_controller.rb | 3 +++
 spec/system/consumer/split_checkout_spec.rb       | 3 +--
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb
index 329ca05712..32e27ed333 100644
--- a/app/controllers/voucher_adjustments_controller.rb
+++ b/app/controllers/voucher_adjustments_controller.rb
@@ -44,6 +44,9 @@ class VoucherAdjustmentsController < BaseController
       return false
     end
 
+    VoucherAdjustmentsService.new(@order).update
+    @order.update_totals_and_states
+
     true
   end
 
diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb
index 5460aad7ce..1048cdd4df 100644
--- a/spec/system/consumer/split_checkout_spec.rb
+++ b/spec/system/consumer/split_checkout_spec.rb
@@ -771,7 +771,6 @@ describe "As a consumer, I want to checkout my order" do
                 fill_in "Enter voucher code", with: "some_code"
                 click_button("Apply")
 
-                pending
                 expect(page).to have_content "No payment required"
                 click_button "Next - Order summary"
                 # Expect to be on the Order Summary page
@@ -815,7 +814,7 @@ describe "As a consumer, I want to checkout my order" do
 
               expect(page).to have_content("$15.00 Voucher")
               expect(order.reload.voucher_adjustments.length).to eq(1)
-              pending
+
               expect(page).to have_content "No payment required"
 
               click_button "Next - Order summary"

From 33ef8def084fad25aabfd823f65077b4c62621cf Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 28 Jul 2023 14:52:25 +1000
Subject: [PATCH 20/24] Update order total after removing a voucher

We need to do this to make sure when display the payment method
again.
Plus spec
---
 app/controllers/voucher_adjustments_controller.rb |  3 +++
 spec/system/consumer/split_checkout_spec.rb       | 15 ++++++++++-----
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb
index 32e27ed333..162a1c89d6 100644
--- a/app/controllers/voucher_adjustments_controller.rb
+++ b/app/controllers/voucher_adjustments_controller.rb
@@ -14,6 +14,9 @@ class VoucherAdjustmentsController < BaseController
   def destroy
     @order.voucher_adjustments.find_by(id: params[:id])&.destroy
 
+    # Update order to make sure we display the appropriate payment method
+    @order.update_totals_and_states
+
     update_payment_section
   end
 
diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb
index 1048cdd4df..ffaaaff8b9 100644
--- a/spec/system/consumer/split_checkout_spec.rb
+++ b/spec/system/consumer/split_checkout_spec.rb
@@ -790,7 +790,8 @@ describe "As a consumer, I want to checkout my order" do
 
           describe "removing voucher from order" do
             before do
-              voucher.create_adjustment(voucher.code, order)
+              add_voucher_to_order(voucher, order)
+
               visit checkout_step_path(:payment)
 
               accept_confirm "Are you sure you want to remove the voucher?" do
@@ -804,6 +805,7 @@ describe "As a consumer, I want to checkout my order" do
                 expect(page).to have_field "Enter voucher code" # Currently no confirmation msg
               end
 
+              expect(page).not_to have_content "No payment required"
               expect(order.voucher_adjustments.length).to eq(0)
             end
 
@@ -823,7 +825,6 @@ describe "As a consumer, I want to checkout my order" do
             end
 
             it "can proceed with payment" do
-              choose "Payment with Fee"
               click_button "Next - Order summary"
               # Expect to be on the Order Summary page
               expect(page).to have_content "Delivery details"
@@ -1151,9 +1152,7 @@ describe "As a consumer, I want to checkout my order" do
         let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor, amount: 6) }
 
         before do
-          voucher.create_adjustment(voucher.code, order)
-          order.update_totals
-          VoucherAdjustmentsService.new(order).update
+          add_voucher_to_order(voucher, order)
 
           visit checkout_step_path(:summary)
         end
@@ -1207,4 +1206,10 @@ describe "As a consumer, I want to checkout my order" do
       end
     end
   end
+
+  def add_voucher_to_order(voucher, order)
+    voucher.create_adjustment(voucher.code, order)
+    VoucherAdjustmentsService.new(order).update
+    order.update_totals_and_states
+  end
 end

From 089d2b9c840c587652cd0ae9869cada57748d8f2 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 31 Jul 2023 12:05:52 +1000
Subject: [PATCH 21/24] Clear any existing payment and payment fees when adding
 a voucher

If a user come back to checkout step 2 from step 3, the order
will have payment and possibly payment fee existing. This can
interfere with voucher calculation, so we clear them.
The user can then select a payment method again if needed
---
 .../voucher_adjustments_controller.rb         |  4 +++
 spec/requests/voucher_adjustments_spec.rb     | 25 ++++++++++++++++++-
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb
index 162a1c89d6..c55df05063 100644
--- a/app/controllers/voucher_adjustments_controller.rb
+++ b/app/controllers/voucher_adjustments_controller.rb
@@ -47,6 +47,10 @@ class VoucherAdjustmentsController < BaseController
       return false
     end
 
+    # Clear payments and payment_fee, to not affect voucher adjustment calculation
+    @order.all_adjustments.payment_fee.destroy_all
+    @order.payments.clear
+
     VoucherAdjustmentsService.new(@order).update
     @order.update_totals_and_states
 
diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb
index 5260996a4b..1bd92a1a94 100644
--- a/spec/requests/voucher_adjustments_spec.rb
+++ b/spec/requests/voucher_adjustments_spec.rb
@@ -8,7 +8,7 @@ describe VoucherAdjustmentsController, type: :request do
   let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) }
   let(:order_cycle) { create(:order_cycle, distributors: [distributor]) }
   let(:exchange) { order_cycle.exchanges.outgoing.first }
-  let(:order) do
+  let!(:order) do
     create(
       :order_with_line_items,
       line_items_count: 1,
@@ -66,6 +66,29 @@ describe VoucherAdjustmentsController, type: :request do
         )
       end
     end
+
+    context "when the order has a payment and payment feed" do
+      let(:payment_method) { create(:payment_method, calculator: calculator) }
+      let(:calculator) do
+        ::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10)
+      end
+
+      before do
+        create(:payment, order: order, payment_method: payment_method, amount: order.total)
+      end
+
+      it "removes existing payments" do
+        expect do
+          post "/voucher_adjustments", params: params
+        end.to change { order.reload.payments.count }.from(1).to(0)
+      end
+
+      it "removes existing payment fees" do
+        expect do
+          post "/voucher_adjustments", params: params
+        end.to change { order.reload.all_adjustments.payment_fee.count }.from(1).to(0)
+      end
+    end
   end
 
   describe "DELETE voucher_adjustments/:id" do

From 2857930263ed2055a6afd85aa062fb5b4204e414 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 31 Jul 2023 14:06:36 +1000
Subject: [PATCH 22/24] Fix voucher adjustment request spec

Error messages have been updated to be more user friendly. This spec
was missed somehow.
---
 spec/requests/voucher_adjustments_spec.rb | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb
index 1bd92a1a94..f3bcbf575b 100644
--- a/spec/requests/voucher_adjustments_spec.rb
+++ b/spec/requests/voucher_adjustments_spec.rb
@@ -61,9 +61,7 @@ describe VoucherAdjustmentsController, type: :request do
         post "/voucher_adjustments", params: params
 
         expect(response).to be_unprocessable
-        expect(flash[:error]).to match(
-          "There was an error while adding the voucher and Label can't be blank"
-        )
+        expect(flash[:error]).to match("Voucher code There was an error while adding the voucher")
       end
     end
 

From 85adb9f3450211908f9ed5ff9c50564720dd5dfa Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 31 Jul 2023 15:19:04 +1000
Subject: [PATCH 23/24] Fix rubocop warning

---
 app/controllers/voucher_adjustments_controller.rb | 10 +++++++---
 spec/requests/voucher_adjustments_spec.rb         |  4 +---
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb
index c55df05063..152097f68c 100644
--- a/app/controllers/voucher_adjustments_controller.rb
+++ b/app/controllers/voucher_adjustments_controller.rb
@@ -47,9 +47,7 @@ class VoucherAdjustmentsController < BaseController
       return false
     end
 
-    # Clear payments and payment_fee, to not affect voucher adjustment calculation
-    @order.all_adjustments.payment_fee.destroy_all
-    @order.payments.clear
+    clear_payments
 
     VoucherAdjustmentsService.new(@order).update
     @order.update_totals_and_states
@@ -81,4 +79,10 @@ class VoucherAdjustmentsController < BaseController
   def voucher_params
     params.require(:order).permit(:voucher_code)
   end
+
+  # Clear payments and payment fees, to not affect voucher adjustment calculation
+  def clear_payments
+    @order.all_adjustments.payment_fee.destroy_all
+    @order.payments.clear
+  end
 end
diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb
index f3bcbf575b..c6eda33db4 100644
--- a/spec/requests/voucher_adjustments_spec.rb
+++ b/spec/requests/voucher_adjustments_spec.rb
@@ -67,9 +67,7 @@ describe VoucherAdjustmentsController, type: :request do
 
     context "when the order has a payment and payment feed" do
       let(:payment_method) { create(:payment_method, calculator: calculator) }
-      let(:calculator) do
-        ::Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10)
-      end
+      let(:calculator) { Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) }
 
       before do
         create(:payment, order: order, payment_method: payment_method, amount: order.total)

From 6ed35f4cc1034a6feada1253c384b1435f688d5d Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 4 Aug 2023 17:03:31 +1000
Subject: [PATCH 24/24] Per review, delete only incomplete payments

Use destroy_all so we don't have to manually delete the payment fees
adjustment
---
 app/controllers/voucher_adjustments_controller.rb | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb
index 152097f68c..094ef26cf7 100644
--- a/app/controllers/voucher_adjustments_controller.rb
+++ b/app/controllers/voucher_adjustments_controller.rb
@@ -82,7 +82,6 @@ class VoucherAdjustmentsController < BaseController
 
   # Clear payments and payment fees, to not affect voucher adjustment calculation
   def clear_payments
-    @order.all_adjustments.payment_fee.destroy_all
-    @order.payments.clear
+    @order.payments.incomplete.destroy_all
   end
 end