From 1be8daaf8620750901d921f2f0fadc9c9de73850 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 01/17] 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       | 337 ++++++++++++------
 1 file changed, 224 insertions(+), 113 deletions(-)

diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb
index b83a18ab809..dc91347b7f7 100644
--- a/spec/services/voucher_adjustments_service_spec.rb
+++ b/spec/services/voucher_adjustments_service_spec.rb
@@ -5,164 +5,275 @@
 describe VoucherAdjustmentsService do
   describe '#update' do
     let(:enterprise) { build(:enterprise) }
-    let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) }
 
-    context 'when voucher covers the order total' do
-      subject { order.voucher_adjustments.first }
+    context 'with a flat rate voucher' do
+      let(:voucher) do
+        create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 10)
+      end
 
-      let(:order) { create(:order_with_totals) }
+      context 'when voucher covers the order total' do
+        subject { order.voucher_adjustments.first }
 
-      it 'updates the adjustment amount to -order.total' do
-        voucher.create_adjustment(voucher.code, order)
-        order.update_columns(item_total: 6)
+        let(:order) { create(:order_with_totals) }
 
-        VoucherAdjustmentsService.new(order).update
+        it 'updates the adjustment amount to -order.total' do
+          voucher.create_adjustment(voucher.code, order)
+          order.update_columns(item_total: 6)
 
-        expect(subject.amount.to_f).to eq(-6.0)
-      end
-    end
+          VoucherAdjustmentsService.new(order).update
 
-    context 'with tax included in order price' do
-      subject { order.voucher_adjustments.first }
-
-      let(:order) do
-        create(
-          :order_with_taxes,
-          distributor: enterprise,
-          ship_address: create(:address),
-          product_price: 110,
-          tax_rate_amount: 0.10,
-          included_in_price: true,
-          tax_rate_name: "Tax 1"
-        )
+          expect(subject.amount.to_f).to eq(-6.0)
+        end
       end
 
-      before do
-        # create adjustment before tax are set
-        voucher.create_adjustment(voucher.code, order)
+      context 'with tax included in order price' do
+        subject { order.voucher_adjustments.first }
+
+        let(:order) do
+          create(
+            :order_with_taxes,
+            distributor: enterprise,
+            ship_address: create(:address),
+            product_price: 110,
+            tax_rate_amount: 0.10,
+            included_in_price: true,
+            tax_rate_name: "Tax 1"
+          )
+        end
 
-        # Update taxes
-        order.create_tax_charge!
-        order.update_shipping_fees!
-        order.update_order!
+        before do
+          # create adjustment before tax are set
+          voucher.create_adjustment(voucher.code, order)
 
-        VoucherAdjustmentsService.new(order).update
-      end
+          # Update taxes
+          order.create_tax_charge!
+          order.update_shipping_fees!
+          order.update_order!
+
+          VoucherAdjustmentsService.new(order).update
+        end
+
+        it 'updates the adjustment included_tax' do
+          # voucher_rate = amount / order.total
+          # -10 / 160 = -0.0625
+          # included_tax = voucher_rate * order.included_tax_total
+          # -0.625 * 10 = -0.63
+          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.new(order).update
+            end.not_to change { subject.reload.amount }
+          end
 
-      it 'updates the adjustment included_tax' do
-        # voucher_rate = amount / order.total
-        # -10 / 160 = -0.0625
-        # included_tax = voucher_rate * order.included_tax_total
-        # -0.0625 * 10 = -0.625
-        expect(subject.included_tax.to_f).to eq(-0.63)
+          it "does not update the tax adjustment" do
+            expect do
+              VoucherAdjustmentsService.new(order).update
+            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.new(order).update
+              end.not_to change { subject.reload.amount }
+            end
+
+            it "updates the tax adjustment" do
+              expect do
+                VoucherAdjustmentsService.new(order).update
+              end.to change { subject.reload.included_tax }
+            end
+          end
+        end
       end
 
-      context "when re calculating" do
-        it "does not update the adjustment amount" do
-          expect do
-            VoucherAdjustmentsService.new(order).update
-          end.not_to change { subject.reload.amount }
+      context 'with tax not included in order price' do
+        let(:order) do
+          create(
+            :order_with_taxes,
+            distributor: enterprise,
+            ship_address: create(:address),
+            product_price: 110,
+            tax_rate_amount: 0.10,
+            included_in_price: false,
+            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
+          voucher.create_adjustment(voucher.code, order)
 
-        it "does not update the tax adjustment" do
-          expect do
-            VoucherAdjustmentsService.new(order).update
-          end.not_to change { subject.reload.included_tax }
+          # Update taxes
+          order.create_tax_charge!
+          order.update_shipping_fees!
+          order.update_order!
         end
 
-        context "when the order changed" do
-          before do
-            order.update_columns(item_total: 200)
-          end
+        it 'includes amount without tax' do
+          # voucher_rate = amount / order.total
+          # -10 / 171 = -0.058479532
+          # amount = voucher_rate * (order.total - order.additional_tax_total)
+          # -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 / 171 = -0.058479532
+          # amount = voucher_rate * order.additional_tax_total
+          # -0.058479532 * 11 = -0.64
+          expect(tax_adjustment.amount.to_f).to eq(-0.64)
+          expect(tax_adjustment.label).to match("Tax")
+        end
 
-          it "does update the adjustment amount" do
+        context "when re calculating" do
+          it "does not update the adjustment amount" do
             expect do
               VoucherAdjustmentsService.new(order).update
-            end.not_to change { subject.reload.amount }
+            end.not_to change { adjustment.reload.amount }
           end
 
-          it "updates the tax adjustment" do
+          it "does not update the tax adjustment" do
             expect do
               VoucherAdjustmentsService.new(order).update
-            end.to change { subject.reload.included_tax }
+            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.new(order).update
+              end.to change { adjustment.reload.amount }
+            end
+
+            it "updates the tax adjustment" do
+              expect do
+                VoucherAdjustmentsService.new(order).update
+              end.to change { tax_adjustment.reload.amount }
+            end
           end
         end
       end
     end
 
-    context 'with tax not included in order price' do
-      let(:order) do
-        create(
-          :order_with_taxes,
-          distributor: enterprise,
-          ship_address: create(:address),
-          product_price: 110,
-          tax_rate_amount: 0.10,
-          included_in_price: false,
-          tax_rate_name: "Tax 1"
-        )
+    context 'with a percentage rate voucher' do
+      let(:voucher) do
+        create(:voucher_percentage_rate, code: 'new_code', enterprise: enterprise, amount: 10)
       end
       let(:adjustment) { order.voucher_adjustments.first }
       let(:tax_adjustment) { order.voucher_adjustments.second }
 
-      before do
-        # create adjustment before tax are set
-        voucher.create_adjustment(voucher.code, order)
+      context 'when voucher covers the order total' do
+        subject { order.voucher_adjustments.first }
 
-        # Update taxes
-        order.create_tax_charge!
-        order.update_shipping_fees!
-        order.update_order!
+        let(:voucher) do
+          create(:voucher_percentage_rate, code: 'new_code', enterprise: enterprise, amount: 100)
+        end
+        let(:order) { create(:order_with_totals) }
 
-        VoucherAdjustmentsService.new(order).update
-      end
+        it 'updates the adjustment amount to -order.total' do
+          voucher.create_adjustment(voucher.code, order)
 
-      it 'includes amount without tax' do
-        # voucher_rate = amount / order.total
-        # -10 / 171 = -0.058479532
-        # amount = voucher_rate * (order.total - order.additional_tax_total)
-        # -0.058479532 * (171 -11) = -9.36
-        expect(adjustment.amount.to_f).to eq(-9.36)
-      end
+          order.update_columns(item_total: 150)
+
+          VoucherAdjustmentsService.new(order).update
 
-      it 'creates a tax adjustment' do
-        # voucher_rate = amount / order.total
-        # -10 / 171 = -0.058479532
-        # amount = voucher_rate * order.additional_tax_total
-        # -0.058479532 * 11 = -0.64
-        expect(tax_adjustment.amount.to_f).to eq(-0.64)
-        expect(tax_adjustment.label).to match("Tax")
+          expect(subject.amount.to_f).to eq(-150.0)
+        end
       end
 
-      context "when re calculating" do
-        it "does not update the adjustment amount" do
-          expect do
-            VoucherAdjustmentsService.new(order).update
-          end.not_to change { adjustment.reload.amount }
+      context 'with tax included in order price' do
+        subject { order.voucher_adjustments.first }
+
+        let(:order) do
+          create(
+            :order_with_taxes,
+            distributor: enterprise,
+            ship_address: create(:address),
+            product_price: 110,
+            tax_rate_amount: 0.10,
+            included_in_price: false,
+            tax_rate_name: "Tax 1"
+          )
         end
 
-        it "does not update the tax adjustment" do
-          expect do
-            VoucherAdjustmentsService.new(order).update
-          end.not_to change { tax_adjustment.reload.amount }
+        before do
+          # create adjustment before tax are set
+          voucher.create_adjustment(voucher.code, order)
+
+          # Update taxes
+          order.create_tax_charge!
+          order.update_shipping_fees!
+          order.update_order!
+
+          VoucherAdjustmentsService.new(order).update
         end
 
-        context "when the order changed" do
-          before do
-            order.update_columns(item_total: 200)
-          end
+        it 'updates the adjustment included_tax' do
+          # voucher_rate = voucher.amount / 100
+          # 10 / 100 = 0.1
+          # included_tax = -voucher_rate * included_tax_total
+          # -0.1 * 10 = -1
+          expect(subject.included_tax.to_f).to eq(-1)
+        end
+      end
 
-          it "updates the adjustment amount" do
-            expect do
-              VoucherAdjustmentsService.new(order).update
-            end.to change { adjustment.reload.amount }
-          end
+      context 'with tax not included in order price' do
+        let(:order) do
+          create(
+            :order_with_taxes,
+            distributor: enterprise,
+            ship_address: create(:address),
+            product_price: 110,
+            tax_rate_amount: 0.10,
+            included_in_price: false,
+            tax_rate_name: "Tax 1"
+          )
+        end
 
-          it "updates the tax adjustment" do
-            expect do
-              VoucherAdjustmentsService.new(order).update
-            end.to change { tax_adjustment.reload.amount }
-          end
+        before do
+          # create adjustment before tax are set
+          voucher.create_adjustment(voucher.code, order)
+
+          # Update taxes
+          order.create_tax_charge!
+          order.update_shipping_fees!
+          order.update_order!
+
+          VoucherAdjustmentsService.new(order).update
+        end
+
+        it 'includes amount without tax' do
+          adjustment = order.voucher_adjustments.first
+          # voucher_rate = voucher.amount / 100
+          # 10 / 100 = 0.1
+          # amount = -voucher_rate * (order.total - order.additional_tax_total)
+          # -0.1 * (171 -11) = -16.0
+          expect(adjustment.amount.to_f).to eq(-16.0)
+        end
+
+        it 'creates a tax adjustment' do
+          # voucher_rate = voucher.amount / 100
+          # 10 / 100 = 0.1
+          # amount = -voucher_rate * order.additional_tax_total
+          # -0.1 * 11 = -1.1
+          tax_adjustment = order.voucher_adjustments.second
+          expect(tax_adjustment.amount.to_f).to eq(-1.1)
+          expect(tax_adjustment.label).to match("Tax")
         end
       end
     end

From 619285ad4acfa8475fef99a0f549a3dc18c9ae9c Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 8 May 2023 15:04:39 +1000
Subject: [PATCH 02/17] Add voucher_type to voucher

And update related specs
voucher_type doesn't do anything for now.
---
 app/models/voucher.rb                         | 15 +++++++++++-
 .../20230508035306_add_type_to_vouchers.rb    |  5 ++++
 db/schema.rb                                  |  1 +
 spec/factories/voucher_factory.rb             |  9 ++++++-
 spec/models/spree/order_spec.rb               |  2 +-
 spec/models/voucher_spec.rb                   | 24 ++++++++++++++++---
 spec/requests/voucher_adjustments_spec.rb     |  2 +-
 spec/system/consumer/split_checkout_spec.rb   |  6 +++--
 8 files changed, 55 insertions(+), 9 deletions(-)
 create mode 100644 db/migrate/20230508035306_add_type_to_vouchers.rb

diff --git a/app/models/voucher.rb b/app/models/voucher.rb
index 109ffd29656..6391b997f14 100644
--- a/app/models/voucher.rb
+++ b/app/models/voucher.rb
@@ -11,7 +11,20 @@ class Voucher < ApplicationRecord
            dependent: :nullify
 
   validates :code, presence: true, uniqueness: { scope: :enterprise_id }
-  validates :amount, presence: true, numericality: { greater_than: 0 }
+  validates :amount,
+            presence: true,
+            numericality: { greater_than: 0 },
+            if: ->(v) { v.voucher_type == FLAT_RATE }
+  validates :amount,
+            presence: true,
+            numericality: { greater_than: 0, less_than_or_equal_to: 100 },
+            if: ->(v) { v.voucher_type == PERCENTAGE_RATE }
+
+  FLAT_RATE = 'flat'.freeze
+  PERCENTAGE_RATE = 'percentage'.freeze
+  TYPES = [FLAT_RATE, PERCENTAGE_RATE].freeze
+
+  validates :voucher_type, inclusion: TYPES
 
   def code=(value)
     super(value.to_s.strip)
diff --git a/db/migrate/20230508035306_add_type_to_vouchers.rb b/db/migrate/20230508035306_add_type_to_vouchers.rb
new file mode 100644
index 00000000000..54a6cceec48
--- /dev/null
+++ b/db/migrate/20230508035306_add_type_to_vouchers.rb
@@ -0,0 +1,5 @@
+class AddTypeToVouchers < ActiveRecord::Migration[7.0]
+  def change
+    add_column :vouchers, :voucher_type, :string, limit: 255
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index c58419991e7..84fef13ddda 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -1081,6 +1081,7 @@
     t.bigint "enterprise_id"
     t.datetime "deleted_at", precision: nil
     t.decimal "amount", precision: 10, scale: 2, default: "0.0", null: false
+    t.string "voucher_type", limit: 255
     t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id", unique: true
     t.index ["deleted_at"], name: "index_vouchers_on_deleted_at"
     t.index ["enterprise_id"], name: "index_vouchers_on_enterprise_id"
diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb
index c02de3fb52d..d9076b91f7e 100644
--- a/spec/factories/voucher_factory.rb
+++ b/spec/factories/voucher_factory.rb
@@ -1,8 +1,15 @@
 # frozen_string_literal: true
 
 FactoryBot.define do
-  factory :voucher do
+  factory :voucher_flat_rate, class: Voucher do
     enterprise { build(:distributor_enterprise) }
+    voucher_type { Voucher::FLAT_RATE }
     amount { 15 }
   end
+
+  factory :voucher_percentage, class: Voucher do
+    enterprise { build(:distributor_enterprise) }
+    voucher_type { Voucher::PERCENTAGE_RATE }
+    amount { rand(1..100) }
+  end
 end
diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb
index 0899a3623ff..9c9d63593f4 100644
--- a/spec/models/spree/order_spec.rb
+++ b/spec/models/spree/order_spec.rb
@@ -1466,7 +1466,7 @@ def advance_to_delivery_state(order)
   describe "#voucher_adjustments" do
     let(:distributor) { create(:distributor_enterprise) }
     let(:order) { create(:order, user: user, distributor: distributor) }
-    let(:voucher) { create(:voucher, code: 'new_code', enterprise: order.distributor) }
+    let(:voucher) { create(:voucher_flat_rate, code: 'new_code', enterprise: order.distributor) }
 
     context "when no voucher adjustment" do
       it 'returns an empty array' do
diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb
index 05992db2d83..8af6daa029a 100644
--- a/spec/models/voucher_spec.rb
+++ b/spec/models/voucher_spec.rb
@@ -19,15 +19,31 @@
   end
 
   describe 'validations' do
-    subject { build(:voucher, code: 'new_code', enterprise: enterprise) }
+    subject { build(:voucher_flat_rate, code: 'new_code', enterprise: enterprise) }
 
     it { is_expected.to validate_presence_of(:code) }
     it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) }
     it { is_expected.to validate_presence_of(:amount) }
-    it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) }
+    it { is_expected.to validate_inclusion_of(:voucher_type).in_array(Voucher::TYPES) }
+
+    context "when voucher_type is flat rate" do
+      it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) }
+    end
+
+    context "when voucher_type is percentage rate" do
+      subject { build(:voucher_percentage, code: 'new_code', enterprise: enterprise) }
+
+      it do
+        is_expected.to validate_numericality_of(:amount)
+          .is_greater_than(0)
+          .is_less_than_or_equal_to(100)
+      end
+    end
   end
 
   describe '#compute_amount' do
+    subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 10) }
+
     let(:order) { create(:order_with_totals) }
 
     context 'when order total is more than the voucher' do
@@ -50,7 +66,9 @@
   describe '#create_adjustment' do
     subject(:adjustment) { voucher.create_adjustment(voucher.code, order) }
 
-    let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 25) }
+    let(:voucher) do
+      create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 25)
+    end
     let(:order) { create(:order_with_line_items, line_items_count: 3, distributor: enterprise) }
 
     it 'includes an amount of 0' do
diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb
index 0188236cbe3..42ce434f382 100644
--- a/spec/requests/voucher_adjustments_spec.rb
+++ b/spec/requests/voucher_adjustments_spec.rb
@@ -19,7 +19,7 @@
     )
   end
   let(:shipping_method) { distributor.shipping_methods.first }
-  let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor) }
+  let(:voucher) { create(:voucher_flat_rate, code: 'some_code', enterprise: distributor) }
 
   before do
     order.update!(created_by: user)
diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb
index 3f0761eaa07..e2f962a13fe 100644
--- a/spec/system/consumer/split_checkout_spec.rb
+++ b/spec/system/consumer/split_checkout_spec.rb
@@ -696,7 +696,7 @@
 
         context "with voucher available" do
           let!(:voucher) do
-            create(:voucher, code: 'some_code', enterprise: distributor, amount: 15)
+            create(:voucher_flat_rate, code: 'some_code', enterprise: distributor, amount: 15)
           end
 
           it "shows voucher input" do
@@ -1150,7 +1150,9 @@
       end
 
       describe "vouchers" do
-        let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor, amount: 6) }
+        let(:voucher) do
+          create(:voucher_flat_rate, code: 'some_code', enterprise: distributor, amount: 6)
+        end
 
         before do
           add_voucher_to_order(voucher, order)

From cc9069e9c6611f48cbed3e927526a1707b43b7bc Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 9 May 2023 10:09:08 +1000
Subject: [PATCH 03/17] Add voucher type to admin screen

Plus specs
---
 app/controllers/admin/vouchers_controller.rb  |  2 +-
 app/models/voucher.rb                         |  7 +-
 app/views/admin/vouchers/new.html.haml        |  6 +-
 config/locales/en.yml                         |  3 +
 .../admin/vouchers_controller_spec.rb         |  5 +-
 spec/system/admin/vouchers_spec.rb            | 89 ++++++++++++-------
 6 files changed, 77 insertions(+), 35 deletions(-)

diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb
index 3f190a8001b..4ce186d2c5c 100644
--- a/app/controllers/admin/vouchers_controller.rb
+++ b/app/controllers/admin/vouchers_controller.rb
@@ -30,7 +30,7 @@ def load_enterprise
     end
 
     def permitted_resource_params
-      params.require(:voucher).permit(:code, :amount)
+      params.require(:voucher).permit(:code, :amount, :voucher_type)
     end
   end
 end
diff --git a/app/models/voucher.rb b/app/models/voucher.rb
index 6391b997f14..2d74167b727 100644
--- a/app/models/voucher.rb
+++ b/app/models/voucher.rb
@@ -31,7 +31,12 @@ def code=(value)
   end
 
   def display_value
-    Spree::Money.new(amount)
+    case voucher_type
+    when FLAT_RATE
+      Spree::Money.new(amount)
+    when PERCENTAGE_RATE
+      I18n.t(:voucher_percentage, amount: amount)
+    end
   end
 
   # Ideally we would use `include CalculatedAdjustments` to be consistent with other adjustments,
diff --git a/app/views/admin/vouchers/new.html.haml b/app/views/admin/vouchers/new.html.haml
index fd83642389e..537d857f668 100644
--- a/app/views/admin/vouchers/new.html.haml
+++ b/app/views/admin/vouchers/new.html.haml
@@ -15,9 +15,13 @@
             = f.label :code, t('.voucher_code')
           .omega.eight.columns
             = f.text_field :code, class: 'fullwidth'
+        .row
+          .alpha.four.columns
+            = f.label :voucher_type, t('.voucher_type')
+          .omega.eight.columns
+            = f.select :voucher_type, options_for_select(Voucher::TYPES.map { |type| [t(".#{type}"), type] }, @voucher.voucher_type)
         .row
           .alpha.four.columns
             = f.label :amount, t('.voucher_amount')
           .omega.eight.columns
-            = Spree::Money.currency_symbol
             = f.text_field :amount, value: @voucher.amount
diff --git a/config/locales/en.yml b/config/locales/en.yml
index eed3aa7db50..d343a6d1513 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1712,6 +1712,9 @@ en:
         save: Save
         voucher_code: Voucher Code
         voucher_amount: Amount
+        voucher_type: Voucher Type 
+        flat: Flat 
+        percentage: Percentage (%) 
 
     # Admin controllers
     controllers:
diff --git a/spec/requests/admin/vouchers_controller_spec.rb b/spec/requests/admin/vouchers_controller_spec.rb
index e794a42af1e..6268d6250e1 100644
--- a/spec/requests/admin/vouchers_controller_spec.rb
+++ b/spec/requests/admin/vouchers_controller_spec.rb
@@ -28,12 +28,14 @@
       {
         voucher: {
           code: code,
-          amount: amount
+          amount: amount,
+          voucher_type: type
         }
       }
     end
     let(:code) { "new_code" }
     let(:amount) { 15 }
+    let(:type) { "percentage" }
 
     it "creates a new voucher" do
       expect { create_voucher }.to change(Voucher, :count).by(1)
@@ -41,6 +43,7 @@
       voucher = Voucher.last
       expect(voucher.code).to eq(code)
       expect(voucher.amount).to eq(amount)
+      expect(voucher.voucher_type).to eq(type)
     end
 
     it "redirects to admin enterprise setting page, voucher panel" do
diff --git a/spec/system/admin/vouchers_spec.rb b/spec/system/admin/vouchers_spec.rb
index 7d9f586f6b0..1ba4fa86ed5 100644
--- a/spec/system/admin/vouchers_spec.rb
+++ b/spec/system/admin/vouchers_spec.rb
@@ -23,7 +23,7 @@
 
   it 'lists enterprise vouchers' do
     # Given an enterprise with vouchers
-    create(:voucher, enterprise: enterprise, code: voucher_code, amount: amount)
+    create(:voucher_flat_rate, enterprise: enterprise, code: voucher_code, amount: amount)
 
     # When I go to the enterprise voucher tab
     visit edit_admin_enterprise_path(enterprise)
@@ -35,46 +35,73 @@
     expect(page).to have_content amount
   end
 
-  it 'creates a voucher' do
-    # Given an enterprise
-    # When I go to the enterprise voucher tab and click new
-    visit edit_admin_enterprise_path(enterprise)
+  describe "adding voucher" do
+    before do
+      # Given an enterprise
+      # When I go to the enterprise voucher tab and click new
+      visit edit_admin_enterprise_path(enterprise)
 
-    click_link 'Vouchers'
-    within "#vouchers_panel" do
-      click_link 'Add New'
+      click_link 'Vouchers'
+      within "#vouchers_panel" do
+        click_link 'Add New'
+      end
     end
 
-    # And I fill in the fields for a new voucher click save
-    fill_in 'voucher_code', with: voucher_code
-    fill_in 'voucher_amount', with: amount
-    click_button 'Save'
+    context "with a flat rate voucher" do
+      it 'creates a voucher' do
+        # And I fill in the fields for a new voucher click save
+        fill_in 'voucher_code', with: voucher_code
+        select "Flat", from: "voucher_voucher_type"
+        fill_in 'voucher_amount', with: amount
+        click_button 'Save'
+
+        # Then I should get redirect to the entreprise voucher tab and see the created voucher
+        expect_to_be_redirected_to_enterprise_voucher_tab(page, voucher_code, amount)
+        expect_voucher_to_be_created(enterprise, voucher_code)
+      end
+    end
 
-    # Then I should get redirect to the entreprise voucher tab and see the created voucher
-    expect(page).to have_selector '.success', text: 'Voucher has been successfully created!'
-    expect(page).to have_content voucher_code
-    expect(page).to have_content amount
+    context "with a percentage rate voucher" do
+      it 'creates a voucher' do
+        # And I fill in the fields for a new voucher click save
+        fill_in 'voucher_code', with: voucher_code
+        select "Percentage (%)", from:  "voucher_voucher_type"
+        fill_in 'voucher_amount', with: amount
+        click_button 'Save'
+
+        # Then I should get redirect to the entreprise voucher tab and see the created voucher
+        expect_to_be_redirected_to_enterprise_voucher_tab(page, voucher_code, amount)
+        expect_voucher_to_be_created(enterprise, voucher_code)
+      end
+    end
 
-    voucher = Voucher.where(enterprise: enterprise, code: voucher_code).first
+    context 'when entering invalid data' do
+      it 'shows an error flash message' do
+        # Given an enterprise
+        # When I go to the new voucher page
+        visit new_admin_enterprise_voucher_path(enterprise)
 
-    expect(voucher).not_to be(nil)
-  end
+        # And I fill in fields with invalid data and click save
+        click_button 'Save'
 
-  context 'when entering invalid data' do
-    it 'shows an error flash message' do
-      # Given an enterprise
-      # When I go to the new voucher page
-      visit new_admin_enterprise_voucher_path(enterprise)
+        # Then I should see an error flash message
+        expect(page).to have_selector '.error', text: "Code can't be blank"
 
-      # And I fill in fields with invalid data and click save
-      click_button 'Save'
+        vouchers = Voucher.where(enterprise: enterprise)
 
-      # Then I should see an error flash message
-      expect(page).to have_selector '.error', text: "Code can't be blank"
+        expect(vouchers).to be_empty
+      end
+    end
+  end
 
-      vouchers = Voucher.where(enterprise: enterprise)
+  def expect_to_be_redirected_to_enterprise_voucher_tab(page, voucher_code, amount)
+    expect(page).to have_selector '.success', text: 'Voucher has been successfully created!'
+    expect(page).to have_content voucher_code
+    expect(page).to have_content amount
+  end
 
-      expect(vouchers).to be_empty
-    end
+  def expect_voucher_to_be_created(enterprise, voucher_code)
+    voucher = Voucher.where(enterprise: enterprise, code: voucher_code).first
+    expect(voucher).not_to be(nil)
   end
 end

From cdb33aa0d0ab6ab02fd0a462a7bd7c2f85d972e7 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 9 May 2023 14:37:47 +1000
Subject: [PATCH 04/17] Add calculation for percentage voucher

It include calculation for order with taxes included in the price
---
 app/models/voucher.rb                          |  5 ++++-
 app/services/voucher_adjustments_service.rb    | 18 +++++++++++++-----
 spec/models/voucher_spec.rb                    | 16 ++++++++++++----
 .../voucher_adjustments_service_spec.rb        |  4 +++-
 4 files changed, 32 insertions(+), 11 deletions(-)

diff --git a/app/models/voucher.rb b/app/models/voucher.rb
index 2d74167b727..30065e8c10d 100644
--- a/app/models/voucher.rb
+++ b/app/models/voucher.rb
@@ -62,6 +62,9 @@ def create_adjustment(label, order)
   # We limit adjustment to the maximum amount needed to cover the order, ie if the voucher
   # covers more than the order.total we only need to create an adjustment covering the order.total
   def compute_amount(order)
-    -amount.clamp(0, order.pre_discount_total)
+    return -amount.clamp(0, order.pre_discount_total) if voucher_type == FLAT_RATE
+
+    percentage = amount / 100
+    -percentage * order.pre_discount_total
   end
 end
diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb
index 355058b9d6d..b699851af6c 100644
--- a/app/services/voucher_adjustments_service.rb
+++ b/app/services/voucher_adjustments_service.rb
@@ -15,7 +15,9 @@ def update
     adjustment = @order.voucher_adjustments.first
 
     # Calculate value
-    amount = adjustment.originator.compute_amount(@order)
+    voucher = adjustment.originator
+    # TODO: see if we can remove this and do it in  handle_tax_excluded_from_price
+    amount = voucher.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.
@@ -24,7 +26,7 @@ def update
     if @order.additional_tax_total.positive?
       handle_tax_excluded_from_price(amount)
     elsif @order.included_tax_total.positive?
-      handle_tax_included_in_price(amount)
+      handle_tax_included_in_price(amount, voucher)
     else
       adjustment.amount = amount
       adjustment.save
@@ -69,9 +71,15 @@ def update_tax_adjustment_for(adjustment, tax_amount)
     tax_adjustment.save
   end
 
-  def handle_tax_included_in_price(amount)
-    voucher_rate = amount / @order.pre_discount_total
-    included_tax = voucher_rate * @order.included_tax_total
+  def handle_tax_included_in_price(amount, voucher)
+   if voucher.voucher_type == Voucher::FLAT_RATE
+      # Flat rate tax calulation
+      voucher_rate = amount / @order.pre_discount_total
+      included_tax = voucher_rate * @order.included_tax_total
+    else
+      # Percentage rate tax calculation
+      included_tax = -voucher.amount / 100 * @order.included_tax_total
+    end
 
     # Update Adjustment
     adjustment = @order.voucher_adjustments.first
diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb
index 8af6daa029a..67ba57dcff3 100644
--- a/spec/models/voucher_spec.rb
+++ b/spec/models/voucher_spec.rb
@@ -42,12 +42,10 @@
   end
 
   describe '#compute_amount' do
-    subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 10) }
-
     let(:order) { create(:order_with_totals) }
 
     context 'when order total is more than the voucher' do
-      subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 5) }
+      subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 5) }
 
       it 'uses the voucher total' do
         expect(subject.compute_amount(order).to_f).to eq(-5)
@@ -55,12 +53,22 @@
     end
 
     context 'when order total is less than the voucher' do
-      subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 20) }
+      subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 20) }
 
       it 'matches the order total' do
         expect(subject.compute_amount(order).to_f).to eq(-10)
       end
     end
+
+    context "with percentage rate voucher" do
+      subject { create(:voucher_percentage, code: 'new_code', enterprise: enterprise, amount: 10) }
+
+      it 'returns calculated anount based on the percentage' do
+        # -0.1 (10%)  * $10 = $1
+        expected_amount = -0.1 * order.total
+        expect(subject.compute_amount(order).to_f).to eq(expected_amount.to_f)
+      end
+    end
   end
 
   describe '#create_adjustment' do
diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb
index dc91347b7f7..4d710943c17 100644
--- a/spec/services/voucher_adjustments_service_spec.rb
+++ b/spec/services/voucher_adjustments_service_spec.rb
@@ -117,6 +117,8 @@
           order.create_tax_charge!
           order.update_shipping_fees!
           order.update_order!
+
+          VoucherAdjustmentsService.new(order).update
         end
 
         it 'includes amount without tax' do
@@ -206,7 +208,7 @@
             ship_address: create(:address),
             product_price: 110,
             tax_rate_amount: 0.10,
-            included_in_price: false,
+            included_in_price: true,
             tax_rate_name: "Tax 1"
           )
         end

From 70bd71436953ca21062af3a09a0937f3b24f96e1 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 9 May 2023 15:29:25 +1000
Subject: [PATCH 05/17] Finish calculation for percentage voucher

Add calculation when tax is not included in price
---
 app/services/voucher_adjustments_service.rb | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb
index b699851af6c..8f8ab9916a0 100644
--- a/app/services/voucher_adjustments_service.rb
+++ b/app/services/voucher_adjustments_service.rb
@@ -16,7 +16,6 @@ def update
 
     # Calculate value
     voucher = adjustment.originator
-    # TODO: see if we can remove this and do it in  handle_tax_excluded_from_price
     amount = voucher.compute_amount(@order)
 
     # It is quite possible to have an order with both tax included in and tax excluded from price.
@@ -35,8 +34,12 @@ def update
 
   private
 
-  def handle_tax_excluded_from_price(amount)
-    voucher_rate = amount / @order.pre_discount_total
+  def handle_tax_excluded_from_price(amount, voucher)
+    if voucher.voucher_type == Voucher::FLAT_RATE
+      voucher_rate = amount / @order.pre_discount_total
+    else
+      voucher_rate = -voucher.amount / 100
+    end
 
     adjustment = @order.voucher_adjustments.first
 
@@ -72,7 +75,7 @@ def update_tax_adjustment_for(adjustment, tax_amount)
   end
 
   def handle_tax_included_in_price(amount, voucher)
-   if voucher.voucher_type == Voucher::FLAT_RATE
+    if voucher.voucher_type == Voucher::FLAT_RATE
       # Flat rate tax calulation
       voucher_rate = amount / @order.pre_discount_total
       included_tax = voucher_rate * @order.included_tax_total

From 959e2308ddd61fa842a39962e91f2cceab5fe312 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 27 Jun 2023 14:50:48 +1000
Subject: [PATCH 06/17] Add system specs for percentage based voucher

Similar to tax included in price scenario, adds a test for percentage
based voucher to check the adjustments are recalculated when needed.
Plus fix tax incluced in price specs to use new factory
---
 .../consumer/split_checkout_tax_incl_spec.rb  |  4 +-
 .../split_checkout_tax_not_incl_spec.rb       | 80 +++++++++++++++++--
 2 files changed, 76 insertions(+), 8 deletions(-)

diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb
index ffff4e6e9b5..bc0e9aa7e05 100644
--- a/spec/system/consumer/split_checkout_tax_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb
@@ -112,7 +112,7 @@
         before { Flipper.enable :vouchers }
 
         let!(:voucher) do
-          create(:voucher, code: 'some_code', enterprise: distributor, amount: 10)
+          create(:voucher_flat_rate, code: 'some_code', enterprise: distributor, amount: 10)
         end
 
         it "will include a tax included amount on the voucher adjustment" do
@@ -148,7 +148,7 @@
 
         describe "moving between summary to summary via edit cart" do
           let!(:voucher) do
-            create(:voucher, code: 'good_code', enterprise: distributor, amount: 15)
+            create(:voucher_flat_rate, code: 'good_code', enterprise: distributor, amount: 15)
           end
 
           it "recalculate the tax component properly" do
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 3e9982d26b4..fc467ed7fab 100644
--- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
@@ -118,7 +118,7 @@
 
       context "when using a voucher" do
         let!(:voucher) do
-          create(:voucher, code: 'some_code', enterprise: distributor, amount: 10)
+          create(:voucher_flat_rate, code: 'some_code', enterprise: distributor, amount: 10)
         end
 
         it "will include a tax included amount on the voucher adjustment" do
@@ -151,12 +151,71 @@
           end
 
           # DB check
-          order_within_zone.reload
-          voucher_adjustment = order_within_zone.voucher_adjustments.first
-          voucher_tax_adjustment = order_within_zone.voucher_adjustments.second
+          #order_within_zone.reload
+          #voucher_adjustment = order_within_zone.voucher_adjustments.first
+          #voucher_tax_adjustment = order_within_zone.voucher_adjustments.second
 
-          expect(voucher_adjustment.amount.to_f).to eq(-8.85)
-          expect(voucher_tax_adjustment.amount.to_f).to eq(-1.15)
+          assert_db_voucher_adjustment(-8.85, -1.15)
+        end
+
+        describe "moving between summary to summary via edit cart" do
+          let!(:voucher) do
+            create(:voucher_percentage, code: 'good_code', enterprise: distributor, amount: 20)
+          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")
+
+            #pause
+            proceed_to_summary
+            assert_db_voucher_adjustment(-2.00, -0.26)
+
+            # 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(-4.00, -0.52)
+
+            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("20.0 % 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 "-4"
+              expect(page).to have_content "Tax good_code"
+              expect(page).to have_content "-0.52"
+            end
+
+            # Check adjustment has been recalculated, we are not expecting any changes here
+            assert_db_voucher_adjustment(-4.00, -0.52)
+          end
         end
       end
     end
@@ -233,4 +292,13 @@
       end
     end
   end
+
+  private
+
+  def assert_db_voucher_adjustment(amount, tax_amount)
+    adjustment = order_within_zone.voucher_adjustments.first
+    tax_adjustment = order_within_zone.voucher_adjustments.second
+    expect(adjustment.amount.to_f).to eq(amount)
+    expect(tax_adjustment.amount.to_f).to eq(tax_amount)
+  end
 end

From 204f3933d01953bce8511774a0a02d9e8d17aac7 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 3 Jul 2023 13:57:05 +1000
Subject: [PATCH 07/17] Refactor voucher: 1 base voucher class

Refactor voucher to use a single table inheritance. It will simplify the
code and remove a bunch of conditional
---
 app/models/voucher.rb                         | 35 +++---------
 .../20230508035306_add_type_to_vouchers.rb    |  9 +++-
 db/schema.rb                                  |  6 +--
 spec/factories/voucher_factory.rb             |  5 +-
 spec/models/voucher_spec.rb                   | 53 ++++++-------------
 5 files changed, 34 insertions(+), 74 deletions(-)

diff --git a/app/models/voucher.rb b/app/models/voucher.rb
index 30065e8c10d..2c68a9ff5b4 100644
--- a/app/models/voucher.rb
+++ b/app/models/voucher.rb
@@ -11,34 +11,13 @@ class Voucher < ApplicationRecord
            dependent: :nullify
 
   validates :code, presence: true, uniqueness: { scope: :enterprise_id }
-  validates :amount,
-            presence: true,
-            numericality: { greater_than: 0 },
-            if: ->(v) { v.voucher_type == FLAT_RATE }
-  validates :amount,
-            presence: true,
-            numericality: { greater_than: 0, less_than_or_equal_to: 100 },
-            if: ->(v) { v.voucher_type == PERCENTAGE_RATE }
 
-  FLAT_RATE = 'flat'.freeze
-  PERCENTAGE_RATE = 'percentage'.freeze
-  TYPES = [FLAT_RATE, PERCENTAGE_RATE].freeze
-
-  validates :voucher_type, inclusion: TYPES
+  TYPES = ["Vouchers::FlatRate", "Vouchers::PercentageRate"].freeze
 
   def code=(value)
     super(value.to_s.strip)
   end
 
-  def display_value
-    case voucher_type
-    when FLAT_RATE
-      Spree::Money.new(amount)
-    when PERCENTAGE_RATE
-      I18n.t(:voucher_percentage, amount: amount)
-    end
-  end
-
   # Ideally we would use `include CalculatedAdjustments` to be consistent with other adjustments,
   # but vouchers have complicated calculation so we can't easily use Spree::Calculator. We keep
   # the same method to stay as consistent as possible.
@@ -59,12 +38,12 @@ def create_adjustment(label, order)
     order.adjustments.create(adjustment_attributes)
   end
 
-  # We limit adjustment to the maximum amount needed to cover the order, ie if the voucher
-  # covers more than the order.total we only need to create an adjustment covering the order.total
-  def compute_amount(order)
-    return -amount.clamp(0, order.pre_discount_total) if voucher_type == FLAT_RATE
+  # The following method must be overriden in a concrete voucher.
+  def display_value
+    raise NotImplementedError, 'please use concrete voucher'
+  end
 
-    percentage = amount / 100
-    -percentage * order.pre_discount_total
+  def compute_amount(_order)
+    raise NotImplementedError, 'please use concrete voucher'
   end
 end
diff --git a/db/migrate/20230508035306_add_type_to_vouchers.rb b/db/migrate/20230508035306_add_type_to_vouchers.rb
index 54a6cceec48..327cc96a1ee 100644
--- a/db/migrate/20230508035306_add_type_to_vouchers.rb
+++ b/db/migrate/20230508035306_add_type_to_vouchers.rb
@@ -1,5 +1,10 @@
 class AddTypeToVouchers < ActiveRecord::Migration[7.0]
-  def change
-    add_column :vouchers, :voucher_type, :string, limit: 255
+  def up
+    # It will set all the vouchers till now to Vouchers::FlatRate
+    add_column :vouchers, :type, :string, limit: 255, null: false, default: "Vouchers::FlatRate"
+  end
+
+  def down
+    remove_column :vouchers, :type
   end
 end
diff --git a/db/schema.rb b/db/schema.rb
index 84fef13ddda..10ecedec2c7 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -93,7 +93,7 @@
     t.datetime "terms_and_conditions_accepted_at", precision: nil
     t.string "first_name", default: "", null: false
     t.string "last_name", default: "", null: false
-    t.boolean "created_manually", default: false, null: false
+    t.boolean "created_manually", default: false
     t.index ["bill_address_id"], name: "index_customers_on_bill_address_id"
     t.index ["created_manually"], name: "index_customers_on_created_manually"
     t.index ["email"], name: "index_customers_on_email"
@@ -1075,13 +1075,13 @@
 
   create_table "vouchers", force: :cascade do |t|
     t.string "code", limit: 255, null: false
-    t.datetime "expiry_date", precision: nil
+    t.datetime "expiry_date"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.bigint "enterprise_id"
     t.datetime "deleted_at", precision: nil
     t.decimal "amount", precision: 10, scale: 2, default: "0.0", null: false
-    t.string "voucher_type", limit: 255
+    t.string "type", limit: 255, default: "Vouchers::FlatRate", null: false
     t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id", unique: true
     t.index ["deleted_at"], name: "index_vouchers_on_deleted_at"
     t.index ["enterprise_id"], name: "index_vouchers_on_enterprise_id"
diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb
index d9076b91f7e..5acea9959f1 100644
--- a/spec/factories/voucher_factory.rb
+++ b/spec/factories/voucher_factory.rb
@@ -1,10 +1,9 @@
 # frozen_string_literal: true
 
 FactoryBot.define do
-  factory :voucher_flat_rate, class: Voucher do
+  factory :voucher, class: Voucher do
     enterprise { build(:distributor_enterprise) }
-    voucher_type { Voucher::FLAT_RATE }
-    amount { 15 }
+    amount { 10 }
   end
 
   factory :voucher_percentage, class: Voucher do
diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb
index 67ba57dcff3..56ff73fcf99 100644
--- a/spec/models/voucher_spec.rb
+++ b/spec/models/voucher_spec.rb
@@ -2,6 +2,11 @@
 
 require 'spec_helper'
 
+# This is used to test non implemented methods
+module Vouchers
+  class TestVoucher < Voucher; end
+end
+
 describe Voucher do
   let(:enterprise) { build(:enterprise) }
 
@@ -23,51 +28,23 @@
 
     it { is_expected.to validate_presence_of(:code) }
     it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) }
-    it { is_expected.to validate_presence_of(:amount) }
-    it { is_expected.to validate_inclusion_of(:voucher_type).in_array(Voucher::TYPES) }
-
-    context "when voucher_type is flat rate" do
-      it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) }
-    end
+  end
 
-    context "when voucher_type is percentage rate" do
-      subject { build(:voucher_percentage, code: 'new_code', enterprise: enterprise) }
+  describe '#display_value' do
+    subject(:voucher) { Vouchers::TestVoucher.new(code: 'new_code', enterprise: enterprise) }
 
-      it do
-        is_expected.to validate_numericality_of(:amount)
-          .is_greater_than(0)
-          .is_less_than_or_equal_to(100)
-      end
+    it "raises not implemented error" do
+      expect{ voucher.display_value }
+        .to raise_error(NotImplementedError, 'please use concrete voucher')
     end
   end
 
   describe '#compute_amount' do
-    let(:order) { create(:order_with_totals) }
-
-    context 'when order total is more than the voucher' do
-      subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 5) }
-
-      it 'uses the voucher total' do
-        expect(subject.compute_amount(order).to_f).to eq(-5)
-      end
-    end
-
-    context 'when order total is less than the voucher' do
-      subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 20) }
-
-      it 'matches the order total' do
-        expect(subject.compute_amount(order).to_f).to eq(-10)
-      end
-    end
-
-    context "with percentage rate voucher" do
-      subject { create(:voucher_percentage, code: 'new_code', enterprise: enterprise, amount: 10) }
+    subject(:voucher) { Vouchers::TestVoucher.new(code: 'new_code', enterprise: enterprise) }
 
-      it 'returns calculated anount based on the percentage' do
-        # -0.1 (10%)  * $10 = $1
-        expected_amount = -0.1 * order.total
-        expect(subject.compute_amount(order).to_f).to eq(expected_amount.to_f)
-      end
+    it "raises not implemented error" do
+      expect{ voucher.compute_amount(nil) }
+        .to raise_error(NotImplementedError, 'please use concrete voucher')
     end
   end
 

From def594ab81ffcfcada9e2d43abaadb590d843a69 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 3 Jul 2023 14:03:48 +1000
Subject: [PATCH 08/17] Refactor voucher: 2 FlatRate voucher

---
 app/models/vouchers/flat_rate.rb       | 19 ++++++++++++++
 spec/factories/voucher_factory.rb      |  6 ++---
 spec/models/vouchers/flat_rate_spec.rb | 34 ++++++++++++++++++++++++++
 3 files changed, 55 insertions(+), 4 deletions(-)
 create mode 100644 app/models/vouchers/flat_rate.rb
 create mode 100644 spec/models/vouchers/flat_rate_spec.rb

diff --git a/app/models/vouchers/flat_rate.rb b/app/models/vouchers/flat_rate.rb
new file mode 100644
index 00000000000..05c70bbc34e
--- /dev/null
+++ b/app/models/vouchers/flat_rate.rb
@@ -0,0 +1,19 @@
+# frozen_string_literal: false
+
+module Vouchers
+  class FlatRate < Voucher
+    validates :amount,
+              presence: true,
+              numericality: { greater_than: 0 }
+
+    def display_value
+      Spree::Money.new(amount)
+    end
+
+    # We limit adjustment to the maximum amount needed to cover the order, ie if the voucher
+    # covers more than the order.total we only need to create an adjustment covering the order.total
+    def compute_amount(order)
+      -amount.clamp(0, order.pre_discount_total)
+    end
+  end
+end
diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb
index 5acea9959f1..7c1a7ce7063 100644
--- a/spec/factories/voucher_factory.rb
+++ b/spec/factories/voucher_factory.rb
@@ -6,9 +6,7 @@
     amount { 10 }
   end
 
-  factory :voucher_percentage, class: Voucher do
-    enterprise { build(:distributor_enterprise) }
-    voucher_type { Voucher::PERCENTAGE_RATE }
-    amount { rand(1..100) }
+  factory :voucher_flat_rate, parent: :voucher, class: Vouchers::FlatRate do
+    amount { 15 }
   end
 end
diff --git a/spec/models/vouchers/flat_rate_spec.rb b/spec/models/vouchers/flat_rate_spec.rb
new file mode 100644
index 00000000000..543a564d1c1
--- /dev/null
+++ b/spec/models/vouchers/flat_rate_spec.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Vouchers::FlatRate do
+  let(:enterprise) { build(:enterprise) }
+
+  describe 'validations' do
+    subject { build(:voucher_flat_rate, code: 'new_code', enterprise: enterprise) }
+
+    it { is_expected.to validate_presence_of(:amount) }
+    it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) }
+  end
+
+  describe '#compute_amount' do
+    let(:order) { create(:order_with_totals) }
+
+    context 'when order total is more than the voucher' do
+      subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 5) }
+
+      it 'uses the voucher total' do
+        expect(subject.compute_amount(order).to_f).to eq(-5)
+      end
+    end
+
+    context 'when order total is less than the voucher' do
+      subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 20) }
+
+      it 'matches the order total' do
+        expect(subject.compute_amount(order).to_f).to eq(-10)
+      end
+    end
+  end
+end

From 46e04ca7eeed98a57e02371021dd49aae0fa3b78 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 3 Jul 2023 14:07:04 +1000
Subject: [PATCH 09/17] Refactor voucher: 3 percentage rate voucher

---
 app/models/vouchers/percentage_rate.rb        | 18 ++++++++++++
 spec/factories/voucher_factory.rb             |  4 +++
 spec/models/vouchers/percentage_rate_spec.rb  | 29 +++++++++++++++++++
 .../split_checkout_tax_not_incl_spec.rb       |  5 ++--
 4 files changed, 53 insertions(+), 3 deletions(-)
 create mode 100644 app/models/vouchers/percentage_rate.rb
 create mode 100644 spec/models/vouchers/percentage_rate_spec.rb

diff --git a/app/models/vouchers/percentage_rate.rb b/app/models/vouchers/percentage_rate.rb
new file mode 100644
index 00000000000..5a750dcdac5
--- /dev/null
+++ b/app/models/vouchers/percentage_rate.rb
@@ -0,0 +1,18 @@
+# frozen_string_literal: false
+
+module Vouchers
+  class PercentageRate < Voucher
+    validates :amount,
+              presence: true,
+              numericality: { greater_than: 0, less_than_or_equal_to: 100 }
+
+    def display_value
+      ActionController::Base.helpers.number_to_percentage(amount, precision: 2)
+    end
+
+    def compute_amount(order)
+      percentage = amount / 100
+      -percentage * order.pre_discount_total
+    end
+  end
+end
diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb
index 7c1a7ce7063..8f7b26fffc5 100644
--- a/spec/factories/voucher_factory.rb
+++ b/spec/factories/voucher_factory.rb
@@ -9,4 +9,8 @@
   factory :voucher_flat_rate, parent: :voucher, class: Vouchers::FlatRate do
     amount { 15 }
   end
+
+  factory :voucher_percentage_rate, parent: :voucher, class: Vouchers::PercentageRate do
+    amount { rand(1..100) }
+  end
 end
diff --git a/spec/models/vouchers/percentage_rate_spec.rb b/spec/models/vouchers/percentage_rate_spec.rb
new file mode 100644
index 00000000000..be1d1adfaad
--- /dev/null
+++ b/spec/models/vouchers/percentage_rate_spec.rb
@@ -0,0 +1,29 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Vouchers::PercentageRate do
+  let(:enterprise) { build(:enterprise) }
+
+  describe 'validations' do
+    subject { build(:voucher_percentage_rate, code: 'new_code', enterprise: enterprise) }
+
+    it { is_expected.to validate_presence_of(:amount) }
+    it do
+      is_expected.to validate_numericality_of(:amount)
+        .is_greater_than(0)
+        .is_less_than_or_equal_to(100)
+    end
+  end
+
+  describe '#compute_amount' do
+    subject do
+      create(:voucher_percentage_rate, code: 'new_code', enterprise: enterprise, amount: 10)
+    end
+    let(:order) { create(:order_with_totals) }
+
+    it 'returns percentage of the order total' do
+      expect(subject.compute_amount(order)).to eq(-order.pre_discount_total * 0.1)
+    end
+  end
+end
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 fc467ed7fab..196b130ad94 100644
--- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
@@ -160,7 +160,7 @@
 
         describe "moving between summary to summary via edit cart" do
           let!(:voucher) do
-            create(:voucher_percentage, code: 'good_code', enterprise: distributor, amount: 20)
+            create(:voucher_percentage_rate, code: 'good_code', enterprise: distributor, amount: 20)
           end
 
           it "recalculate the tax component properly" do
@@ -171,7 +171,6 @@
             fill_in "Enter voucher code", with: voucher.code
             click_button("Apply")
 
-            #pause
             proceed_to_summary
             assert_db_voucher_adjustment(-2.00, -0.26)
 
@@ -200,7 +199,7 @@
             proceed_to_payment
 
             # Check voucher is still there
-            expect(page).to have_content("20.0 % Voucher")
+            expect(page).to have_content("20.00% Voucher")
 
             # Go to summary
             proceed_to_summary

From 29a38467d256cbf93b596d935b4b9eade37a151a Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 3 Jul 2023 15:32:44 +1000
Subject: [PATCH 10/17] Fix admin pages to work with refactored vouchers

---
 app/controllers/admin/vouchers_controller.rb  | 24 ++++++++---
 app/views/admin/vouchers/new.html.haml        |  2 +-
 config/locales/en.yml                         |  4 +-
 .../admin/vouchers_controller_spec.rb         | 42 +++++++++++++++----
 spec/system/admin/vouchers_spec.rb            | 12 +++---
 5 files changed, 62 insertions(+), 22 deletions(-)

diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb
index 4ce186d2c5c..4ec300f3cc3 100644
--- a/app/controllers/admin/vouchers_controller.rb
+++ b/app/controllers/admin/vouchers_controller.rb
@@ -9,19 +9,33 @@ def new
     end
 
     def create
-      @voucher = Voucher.create(permitted_resource_params.merge(enterprise: @enterprise))
+      case params[:vouchers_flat_rate][:voucher_type]
+      when "Vouchers::FlatRate"
+        @voucher =
+          Vouchers::FlatRate.create(permitted_resource_params.merge(enterprise: @enterprise))
+      when "Vouchers::PercentageRate"
+        @voucher =
+          Vouchers::PercentageRate.create(permitted_resource_params.merge(enterprise: @enterprise))
+      else
+        @voucher.errors.add(:type)
+        return render_error
+      end
 
       if @voucher.save
-        flash[:success] = flash_message_for(@voucher, :successfully_created)
+        flash[:success] = I18n.t(:successfully_created, resource: "Voucher")
         redirect_to edit_admin_enterprise_path(@enterprise, anchor: :vouchers_panel)
       else
-        flash[:error] = @voucher.errors.full_messages.to_sentence
-        render :new
+        render_error
       end
     end
 
     private
 
+    def render_error
+      flash[:error] = @voucher.errors.full_messages.to_sentence
+      render :new
+    end
+
     def load_enterprise
       @enterprise = OpenFoodNetwork::Permissions
         .new(spree_current_user)
@@ -30,7 +44,7 @@ def load_enterprise
     end
 
     def permitted_resource_params
-      params.require(:voucher).permit(:code, :amount, :voucher_type)
+      params.require(:vouchers_flat_rate).permit(:code, :amount)
     end
   end
 end
diff --git a/app/views/admin/vouchers/new.html.haml b/app/views/admin/vouchers/new.html.haml
index 537d857f668..a27ea65d7a8 100644
--- a/app/views/admin/vouchers/new.html.haml
+++ b/app/views/admin/vouchers/new.html.haml
@@ -19,7 +19,7 @@
           .alpha.four.columns
             = f.label :voucher_type, t('.voucher_type')
           .omega.eight.columns
-            = f.select :voucher_type, options_for_select(Voucher::TYPES.map { |type| [t(".#{type}"), type] }, @voucher.voucher_type)
+            = f.select :voucher_type, options_for_select(Voucher::TYPES.map { |type| [t(".#{type.demodulize.underscore}"), type] }, @voucher.class.to_s)
         .row
           .alpha.four.columns
             = f.label :amount, t('.voucher_amount')
diff --git a/config/locales/en.yml b/config/locales/en.yml
index d343a6d1513..d3e147cf25c 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1713,8 +1713,8 @@ en:
         voucher_code: Voucher Code
         voucher_amount: Amount
         voucher_type: Voucher Type 
-        flat: Flat 
-        percentage: Percentage (%) 
+        flat_rate: Flat 
+        percentage_rate: Percentage (%) 
 
     # Admin controllers
     controllers:
diff --git a/spec/requests/admin/vouchers_controller_spec.rb b/spec/requests/admin/vouchers_controller_spec.rb
index 6268d6250e1..60b0f34e176 100644
--- a/spec/requests/admin/vouchers_controller_spec.rb
+++ b/spec/requests/admin/vouchers_controller_spec.rb
@@ -26,7 +26,7 @@
 
     let(:params) do
       {
-        voucher: {
+        vouchers_flat_rate: {
           code: code,
           amount: amount,
           voucher_type: type
@@ -35,15 +35,41 @@
     end
     let(:code) { "new_code" }
     let(:amount) { 15 }
-    let(:type) { "percentage" }
+    let(:type) { "Vouchers::PercentageRate" }
 
-    it "creates a new voucher" do
-      expect { create_voucher }.to change(Voucher, :count).by(1)
+    context "with a flat rate voucher" do
+      let(:type) { "Vouchers::FlatRate" }
 
-      voucher = Voucher.last
-      expect(voucher.code).to eq(code)
-      expect(voucher.amount).to eq(amount)
-      expect(voucher.voucher_type).to eq(type)
+      it "creates a new voucher" do
+        expect { create_voucher }.to change(Vouchers::FlatRate, :count).by(1)
+
+        voucher = Vouchers::FlatRate.last
+        expect(voucher.code).to eq(code)
+        expect(voucher.amount).to eq(amount)
+      end
+    end
+
+    context "with a percentage rate voucher" do
+      let(:type) { "Vouchers::PercentageRate" }
+
+      it "creates a new voucher" do
+        expect { create_voucher }.to change(Vouchers::PercentageRate, :count).by(1)
+
+        voucher = Vouchers::PercentageRate.last
+        expect(voucher.code).to eq(code)
+        expect(voucher.amount).to eq(amount)
+      end
+    end
+
+    context "with a wrong type" do
+      let(:type) { "Random" }
+
+      it "render the new page with an error" do
+        create_voucher
+
+        expect(response).to render_template("admin/vouchers/new")
+        expect(flash[:error]).to eq("Type is invalid")
+      end
     end
 
     it "redirects to admin enterprise setting page, voucher panel" do
diff --git a/spec/system/admin/vouchers_spec.rb b/spec/system/admin/vouchers_spec.rb
index 1ba4fa86ed5..68b7fe83f32 100644
--- a/spec/system/admin/vouchers_spec.rb
+++ b/spec/system/admin/vouchers_spec.rb
@@ -50,9 +50,9 @@
     context "with a flat rate voucher" do
       it 'creates a voucher' do
         # And I fill in the fields for a new voucher click save
-        fill_in 'voucher_code', with: voucher_code
-        select "Flat", from: "voucher_voucher_type"
-        fill_in 'voucher_amount', with: amount
+        fill_in 'vouchers_flat_rate_code', with: voucher_code
+        select "Flat", from: "vouchers_flat_rate_voucher_type"
+        fill_in 'vouchers_flat_rate_amount', with: amount
         click_button 'Save'
 
         # Then I should get redirect to the entreprise voucher tab and see the created voucher
@@ -64,9 +64,9 @@
     context "with a percentage rate voucher" do
       it 'creates a voucher' do
         # And I fill in the fields for a new voucher click save
-        fill_in 'voucher_code', with: voucher_code
-        select "Percentage (%)", from:  "voucher_voucher_type"
-        fill_in 'voucher_amount', with: amount
+        fill_in 'vouchers_flat_rate_code', with: voucher_code
+        select "Percentage (%)", from: "vouchers_flat_rate_voucher_type"
+        fill_in 'vouchers_flat_rate_amount', with: amount
         click_button 'Save'
 
         # Then I should get redirect to the entreprise voucher tab and see the created voucher

From 2828bd098d4362f24eaba4ba08d4c23090a7baaf Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 3 Jul 2023 16:03:55 +1000
Subject: [PATCH 11/17] Refactor VoucherAdjustment service

We are taking advantage of having a FlatRate and a PercentageRate
model to simplify the code a little
---
 app/models/voucher.rb                        |  4 ++++
 app/models/vouchers/flat_rate.rb             |  6 ++++++
 app/models/vouchers/percentage_rate.rb       |  7 +++++--
 app/services/voucher_adjustments_service.rb  | 22 ++++----------------
 spec/models/voucher_spec.rb                  |  9 ++++++++
 spec/models/vouchers/flat_rate_spec.rb       | 13 ++++++++++++
 spec/models/vouchers/percentage_rate_spec.rb | 10 +++++++++
 7 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/app/models/voucher.rb b/app/models/voucher.rb
index 2c68a9ff5b4..4022a7559ff 100644
--- a/app/models/voucher.rb
+++ b/app/models/voucher.rb
@@ -46,4 +46,8 @@ def display_value
   def compute_amount(_order)
     raise NotImplementedError, 'please use concrete voucher'
   end
+
+  def rate(_order)
+    raise NotImplementedError, 'please use concrete voucher'
+  end
 end
diff --git a/app/models/vouchers/flat_rate.rb b/app/models/vouchers/flat_rate.rb
index 05c70bbc34e..9ad0d3de898 100644
--- a/app/models/vouchers/flat_rate.rb
+++ b/app/models/vouchers/flat_rate.rb
@@ -15,5 +15,11 @@ def display_value
     def compute_amount(order)
       -amount.clamp(0, order.pre_discount_total)
     end
+
+    def rate(order)
+      amount = compute_amount(order)
+
+      amount / order.pre_discount_total
+    end
   end
 end
diff --git a/app/models/vouchers/percentage_rate.rb b/app/models/vouchers/percentage_rate.rb
index 5a750dcdac5..41b17e77f60 100644
--- a/app/models/vouchers/percentage_rate.rb
+++ b/app/models/vouchers/percentage_rate.rb
@@ -11,8 +11,11 @@ def display_value
     end
 
     def compute_amount(order)
-      percentage = amount / 100
-      -percentage * order.pre_discount_total
+      rate(order) * order.pre_discount_total
+    end
+
+    def rate(_order)
+      -amount / 100
     end
   end
 end
diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb
index 8f8ab9916a0..98a4c63f5eb 100644
--- a/app/services/voucher_adjustments_service.rb
+++ b/app/services/voucher_adjustments_service.rb
@@ -23,7 +23,7 @@ def update
     #
     # 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(amount)
+      handle_tax_excluded_from_price(voucher)
     elsif @order.included_tax_total.positive?
       handle_tax_included_in_price(amount, voucher)
     else
@@ -32,15 +32,8 @@ def update
     end
   end
 
-  private
-
-  def handle_tax_excluded_from_price(amount, voucher)
-    if voucher.voucher_type == Voucher::FLAT_RATE
-      voucher_rate = amount / @order.pre_discount_total
-    else
-      voucher_rate = -voucher.amount / 100
-    end
-
+  def handle_tax_excluded_from_price(voucher)
+    voucher_rate = voucher.rate(@order)
     adjustment = @order.voucher_adjustments.first
 
     # Adding the voucher tax part
@@ -75,14 +68,7 @@ def update_tax_adjustment_for(adjustment, tax_amount)
   end
 
   def handle_tax_included_in_price(amount, voucher)
-    if voucher.voucher_type == Voucher::FLAT_RATE
-      # Flat rate tax calulation
-      voucher_rate = amount / @order.pre_discount_total
-      included_tax = voucher_rate * @order.included_tax_total
-    else
-      # Percentage rate tax calculation
-      included_tax = -voucher.amount / 100 * @order.included_tax_total
-    end
+    included_tax = voucher.rate(@order) * @order.included_tax_total
 
     # Update Adjustment
     adjustment = @order.voucher_adjustments.first
diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb
index 56ff73fcf99..4e3c294af9b 100644
--- a/spec/models/voucher_spec.rb
+++ b/spec/models/voucher_spec.rb
@@ -68,4 +68,13 @@ class TestVoucher < Voucher; end
       expect(adjustment.state).to eq("open")
     end
   end
+
+  describe '#rate' do
+    subject(:voucher) { Vouchers::TestVoucher.new(code: 'new_code', enterprise: enterprise) }
+
+    it "raises not implemented error" do
+      expect{ voucher.rate(nil) }
+        .to raise_error(NotImplementedError, 'please use concrete voucher')
+    end
+  end
 end
diff --git a/spec/models/vouchers/flat_rate_spec.rb b/spec/models/vouchers/flat_rate_spec.rb
index 543a564d1c1..f6d05eec51e 100644
--- a/spec/models/vouchers/flat_rate_spec.rb
+++ b/spec/models/vouchers/flat_rate_spec.rb
@@ -31,4 +31,17 @@
       end
     end
   end
+
+  describe "#rate" do
+    subject do
+      create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 5)
+    end
+    let(:order) { create(:order_with_totals) }
+
+    it "returns the voucher rate" do
+      # rate = -voucher_amount / order.pre_discount_total
+      # -5 / 10 = -0.5
+      expect(subject.rate(order).to_f).to eq(-0.5)
+    end
+  end
 end
diff --git a/spec/models/vouchers/percentage_rate_spec.rb b/spec/models/vouchers/percentage_rate_spec.rb
index be1d1adfaad..01bd7c1e767 100644
--- a/spec/models/vouchers/percentage_rate_spec.rb
+++ b/spec/models/vouchers/percentage_rate_spec.rb
@@ -26,4 +26,14 @@
       expect(subject.compute_amount(order)).to eq(-order.pre_discount_total * 0.1)
     end
   end
+
+  describe "#rate" do
+    subject do
+      create(:voucher_percentage_rate, code: 'new_code', enterprise: enterprise, amount: 50)
+    end
+
+    it "returns the voucher percentage rate" do
+      expect(subject.rate(nil)).to eq(-0.5)
+    end
+  end
 end

From e8b374d0f29e0ccb705a9daaf7a4d6b7501d9a9e Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 3 Jul 2023 16:05:38 +1000
Subject: [PATCH 12/17] Remove left over comment

---
 spec/system/consumer/split_checkout_tax_not_incl_spec.rb | 4 ----
 1 file changed, 4 deletions(-)

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 196b130ad94..09bc8e48f43 100644
--- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb
@@ -151,10 +151,6 @@
           end
 
           # DB check
-          #order_within_zone.reload
-          #voucher_adjustment = order_within_zone.voucher_adjustments.first
-          #voucher_tax_adjustment = order_within_zone.voucher_adjustments.second
-
           assert_db_voucher_adjustment(-8.85, -1.15)
         end
 

From 3f63cfbc278ebbf44b7a687ab72eff920097b158 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Fri, 14 Jul 2023 15:32:44 +1000
Subject: [PATCH 13/17] Fix Rubocop warning

---
 spec/models/vouchers/flat_rate_spec.rb       | 8 ++++----
 spec/models/vouchers/percentage_rate_spec.rb | 6 +++---
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/spec/models/vouchers/flat_rate_spec.rb b/spec/models/vouchers/flat_rate_spec.rb
index f6d05eec51e..3860410cfb9 100644
--- a/spec/models/vouchers/flat_rate_spec.rb
+++ b/spec/models/vouchers/flat_rate_spec.rb
@@ -6,7 +6,7 @@
   let(:enterprise) { build(:enterprise) }
 
   describe 'validations' do
-    subject { build(:voucher_flat_rate, code: 'new_code', enterprise: enterprise) }
+    subject { build(:voucher_flat_rate, code: 'new_code', enterprise:) }
 
     it { is_expected.to validate_presence_of(:amount) }
     it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) }
@@ -16,7 +16,7 @@
     let(:order) { create(:order_with_totals) }
 
     context 'when order total is more than the voucher' do
-      subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 5) }
+      subject { create(:voucher_flat_rate, code: 'new_code', enterprise:, amount: 5) }
 
       it 'uses the voucher total' do
         expect(subject.compute_amount(order).to_f).to eq(-5)
@@ -24,7 +24,7 @@
     end
 
     context 'when order total is less than the voucher' do
-      subject { create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 20) }
+      subject { create(:voucher_flat_rate, code: 'new_code', enterprise:, amount: 20) }
 
       it 'matches the order total' do
         expect(subject.compute_amount(order).to_f).to eq(-10)
@@ -34,7 +34,7 @@
 
   describe "#rate" do
     subject do
-      create(:voucher_flat_rate, code: 'new_code', enterprise: enterprise, amount: 5)
+      create(:voucher_flat_rate, code: 'new_code', enterprise:, amount: 5)
     end
     let(:order) { create(:order_with_totals) }
 
diff --git a/spec/models/vouchers/percentage_rate_spec.rb b/spec/models/vouchers/percentage_rate_spec.rb
index 01bd7c1e767..e59be0fed1a 100644
--- a/spec/models/vouchers/percentage_rate_spec.rb
+++ b/spec/models/vouchers/percentage_rate_spec.rb
@@ -6,7 +6,7 @@
   let(:enterprise) { build(:enterprise) }
 
   describe 'validations' do
-    subject { build(:voucher_percentage_rate, code: 'new_code', enterprise: enterprise) }
+    subject { build(:voucher_percentage_rate, code: 'new_code', enterprise:) }
 
     it { is_expected.to validate_presence_of(:amount) }
     it do
@@ -18,7 +18,7 @@
 
   describe '#compute_amount' do
     subject do
-      create(:voucher_percentage_rate, code: 'new_code', enterprise: enterprise, amount: 10)
+      create(:voucher_percentage_rate, code: 'new_code', enterprise:, amount: 10)
     end
     let(:order) { create(:order_with_totals) }
 
@@ -29,7 +29,7 @@
 
   describe "#rate" do
     subject do
-      create(:voucher_percentage_rate, code: 'new_code', enterprise: enterprise, amount: 50)
+      create(:voucher_percentage_rate, code: 'new_code', enterprise:, amount: 50)
     end
 
     it "returns the voucher percentage rate" do

From e6b53c0a42eadd651fa0442d159189df3ed18fde Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Tue, 18 Jul 2023 13:24:41 +1000
Subject: [PATCH 14/17] Per review , Simplify creation of Voucher

We now check against known type to instanciate the correct voucher
instead of using a case
---
 app/controllers/admin/vouchers_controller.rb | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb
index 4ec300f3cc3..38a4203fd56 100644
--- a/app/controllers/admin/vouchers_controller.rb
+++ b/app/controllers/admin/vouchers_controller.rb
@@ -9,13 +9,11 @@ def new
     end
 
     def create
-      case params[:vouchers_flat_rate][:voucher_type]
-      when "Vouchers::FlatRate"
-        @voucher =
-          Vouchers::FlatRate.create(permitted_resource_params.merge(enterprise: @enterprise))
-      when "Vouchers::PercentageRate"
-        @voucher =
-          Vouchers::PercentageRate.create(permitted_resource_params.merge(enterprise: @enterprise))
+      voucher_type = params[:vouchers_flat_rate][:voucher_type]
+      if Voucher::TYPES.include?(voucher_type)
+        @voucher = voucher_type.safe_constantize.create(
+          permitted_resource_params.merge(enterprise: @enterprise)
+        )
       else
         @voucher.errors.add(:type)
         return render_error

From 054eac082261e9f141253b877d4d97803974694b Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <40413322+rioug@users.noreply.github.com>
Date: Mon, 24 Jul 2023 11:26:17 +1000
Subject: [PATCH 15/17] Use a better describe

Co-authored-by: David Cook <david@redcliffs.net>
---
 spec/system/consumer/split_checkout_tax_incl_spec.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb
index bc0e9aa7e05..f2aa3f281a6 100644
--- a/spec/system/consumer/split_checkout_tax_incl_spec.rb
+++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb
@@ -146,7 +146,7 @@
           assert_db_voucher_adjustment(-10.00, -1.15)
         end
 
-        describe "moving between summary to summary via edit cart" do
+        describe "updating voucher adjustment after editing order" do
           let!(:voucher) do
             create(:voucher_flat_rate, code: 'good_code', enterprise: distributor, amount: 15)
           end

From 9c9a6234e1f4c9e43cd60c464a0f60de54aed9d0 Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 24 Jul 2023 11:28:32 +1000
Subject: [PATCH 16/17] Per review, clean up voucher specs

Add explicit 'order.item_total' to make specs more readable
---
 spec/factories/voucher_factory.rb            |  1 +
 spec/models/vouchers/flat_rate_spec.rb       | 20 +++++++++++++-------
 spec/models/vouchers/percentage_rate_spec.rb | 14 ++++++++------
 3 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb
index 8f7b26fffc5..821728b09f3 100644
--- a/spec/factories/voucher_factory.rb
+++ b/spec/factories/voucher_factory.rb
@@ -2,6 +2,7 @@
 
 FactoryBot.define do
   factory :voucher, class: Voucher do
+    code { "new_code" }
     enterprise { build(:distributor_enterprise) }
     amount { 10 }
   end
diff --git a/spec/models/vouchers/flat_rate_spec.rb b/spec/models/vouchers/flat_rate_spec.rb
index 3860410cfb9..419a103534c 100644
--- a/spec/models/vouchers/flat_rate_spec.rb
+++ b/spec/models/vouchers/flat_rate_spec.rb
@@ -3,10 +3,8 @@
 require 'spec_helper'
 
 describe Vouchers::FlatRate do
-  let(:enterprise) { build(:enterprise) }
-
   describe 'validations' do
-    subject { build(:voucher_flat_rate, code: 'new_code', enterprise:) }
+    subject { build(:voucher_flat_rate) }
 
     it { is_expected.to validate_presence_of(:amount) }
     it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) }
@@ -15,8 +13,12 @@
   describe '#compute_amount' do
     let(:order) { create(:order_with_totals) }
 
+    before do
+      order.update_columns(item_total: 15)
+    end
+
     context 'when order total is more than the voucher' do
-      subject { create(:voucher_flat_rate, code: 'new_code', enterprise:, amount: 5) }
+      subject { create(:voucher_flat_rate, amount: 5) }
 
       it 'uses the voucher total' do
         expect(subject.compute_amount(order).to_f).to eq(-5)
@@ -24,20 +26,24 @@
     end
 
     context 'when order total is less than the voucher' do
-      subject { create(:voucher_flat_rate, code: 'new_code', enterprise:, amount: 20) }
+      subject { create(:voucher_flat_rate, amount: 20) }
 
       it 'matches the order total' do
-        expect(subject.compute_amount(order).to_f).to eq(-10)
+        expect(subject.compute_amount(order).to_f).to eq(-15)
       end
     end
   end
 
   describe "#rate" do
     subject do
-      create(:voucher_flat_rate, code: 'new_code', enterprise:, amount: 5)
+      create(:voucher_flat_rate, code: 'new_code', amount: 5)
     end
     let(:order) { create(:order_with_totals) }
 
+    before do
+      order.update_columns(item_total: 10)
+    end
+
     it "returns the voucher rate" do
       # rate = -voucher_amount / order.pre_discount_total
       # -5 / 10 = -0.5
diff --git a/spec/models/vouchers/percentage_rate_spec.rb b/spec/models/vouchers/percentage_rate_spec.rb
index e59be0fed1a..e2e136bd14a 100644
--- a/spec/models/vouchers/percentage_rate_spec.rb
+++ b/spec/models/vouchers/percentage_rate_spec.rb
@@ -3,10 +3,8 @@
 require 'spec_helper'
 
 describe Vouchers::PercentageRate do
-  let(:enterprise) { build(:enterprise) }
-
   describe 'validations' do
-    subject { build(:voucher_percentage_rate, code: 'new_code', enterprise:) }
+    subject { build(:voucher_percentage_rate) }
 
     it { is_expected.to validate_presence_of(:amount) }
     it do
@@ -18,18 +16,22 @@
 
   describe '#compute_amount' do
     subject do
-      create(:voucher_percentage_rate, code: 'new_code', enterprise:, amount: 10)
+      create(:voucher_percentage_rate, amount: 10)
     end
     let(:order) { create(:order_with_totals) }
 
+    before do
+      order.update_columns(item_total: 15)
+    end
+
     it 'returns percentage of the order total' do
-      expect(subject.compute_amount(order)).to eq(-order.pre_discount_total * 0.1)
+      expect(subject.compute_amount(order)).to eq(-1.5)
     end
   end
 
   describe "#rate" do
     subject do
-      create(:voucher_percentage_rate, code: 'new_code', enterprise:, amount: 50)
+      create(:voucher_percentage_rate, amount: 50)
     end
 
     it "returns the voucher percentage rate" do

From a2def2424ca67b3bd1631ede220c9969e2e83bec Mon Sep 17 00:00:00 2001
From: Gaetan Craig-Riou <gaetan.riou@gmail.com>
Date: Mon, 24 Jul 2023 11:39:26 +1000
Subject: [PATCH 17/17] Add a comment around the use of safe_constantize

It triggers a Brakeman error that can be safely ignored
---
 app/controllers/admin/vouchers_controller.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb
index 38a4203fd56..01ca689b911 100644
--- a/app/controllers/admin/vouchers_controller.rb
+++ b/app/controllers/admin/vouchers_controller.rb
@@ -9,6 +9,8 @@ def new
     end
 
     def create
+      # The use of "safe_constantize" here will trigger a Brakeman error, it can safely be ignored
+      # as it's a false positive : https://github.com/openfoodfoundation/openfoodnetwork/pull/10821
       voucher_type = params[:vouchers_flat_rate][:voucher_type]
       if Voucher::TYPES.include?(voucher_type)
         @voucher = voucher_type.safe_constantize.create(