Merge pull request #10821 from rioug/10727-vouchers-percentage-rate

[Vouchers] Percentage rate

app/controllers/admin/vouchers_controller.rb

@@ -9,19 +9,33 @@ def new
     end
 
     def create
-      @voucher = Voucher.create(permitted_resource_params.merge(enterprise: @enterprise))
+      # 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(
+          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)
+      params.require(:vouchers_flat_rate).permit(:code, :amount)
     end
   end
 end

app/models/voucher.rb

@@ -11,16 +11,13 @@ class Voucher < ApplicationRecord
            dependent: :nullify
 
   validates :code, presence: true, uniqueness: { scope: :enterprise_id }
-  validates :amount, presence: true, numericality: { greater_than: 0 }
+
+  TYPES = ["Vouchers::FlatRate", "Vouchers::PercentageRate"].freeze
 
   def code=(value)
     super(value.to_s.strip)
   end
 
-  def display_value
-    Spree::Money.new(amount)
-  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.
@@ -41,9 +38,16 @@ 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)
-    -amount.clamp(0, order.pre_discount_total)
+  # The following method must be overriden in a concrete voucher.
+  def display_value
+    raise NotImplementedError, 'please use concrete voucher'
+  end
+
+  def compute_amount(_order)
+    raise NotImplementedError, 'please use concrete voucher'
+  end
+
+  def rate(_order)
+    raise NotImplementedError, 'please use concrete voucher'
   end
 end

app/models/vouchers/flat_rate.rb

@@ -0,0 +1,25 @@
+# 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
+
+    def rate(order)
+      amount = compute_amount(order)
+
+      amount / order.pre_discount_total
+    end
+  end
+end

app/models/vouchers/percentage_rate.rb

@@ -0,0 +1,21 @@
+# 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)
+      rate(order) * order.pre_discount_total
+    end
+
+    def rate(_order)
+      -amount / 100
+    end
+  end
+end

app/services/voucher_adjustments_service.rb

@@ -15,27 +15,25 @@ def update
     adjustment = @order.voucher_adjustments.first
 
     # Calculate value
-    amount = adjustment.originator.compute_amount(@order)
+    voucher = adjustment.originator
+    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.
     #
     # 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)
+      handle_tax_included_in_price(amount, voucher)
     else
       adjustment.amount = amount
       adjustment.save
     end
   end
 
-  private
-
-  def handle_tax_excluded_from_price(amount)
-    voucher_rate = amount / @order.pre_discount_total
-
+  def handle_tax_excluded_from_price(voucher)
+    voucher_rate = voucher.rate(@order)
     adjustment = @order.voucher_adjustments.first
 
     # Adding the voucher tax part
@@ -69,9 +67,8 @@ 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)
+    included_tax = voucher.rate(@order) * @order.included_tax_total
 
     # Update Adjustment
     adjustment = @order.voucher_adjustments.first

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.demodulize.underscore}"), type] }, @voucher.class.to_s)
         .row
           .alpha.four.columns
             = f.label :amount, t('.voucher_amount')
           .omega.eight.columns
-            = Spree::Money.currency_symbol
             = f.text_field :amount, value: @voucher.amount

config/locales/en.yml

@@ -1712,6 +1712,9 @@ en:
         save: Save
         voucher_code: Voucher Code
         voucher_amount: Amount
+        voucher_type: Voucher Type 
+        flat_rate: Flat 
+        percentage_rate: Percentage (%) 
 
     # Admin controllers
     controllers:

db/migrate/20230508035306_add_type_to_vouchers.rb

@@ -0,0 +1,10 @@
+class AddTypeToVouchers < ActiveRecord::Migration[7.0]
+  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

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,12 +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 "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"

spec/factories/voucher_factory.rb

@@ -1,8 +1,17 @@
 # frozen_string_literal: true
 
 FactoryBot.define do
-  factory :voucher do
+  factory :voucher, class: Voucher do
+    code { "new_code" }
     enterprise { build(:distributor_enterprise) }
+    amount { 10 }
+  end
+
+  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

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

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) }
 
@@ -19,38 +24,36 @@
   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) }
   end
 
-  describe '#compute_amount' do
-    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) }
+  describe '#display_value' do
+    subject(:voucher) { Vouchers::TestVoucher.new(code: 'new_code', enterprise: enterprise) }
 
-      it 'uses the voucher total' do
-        expect(subject.compute_amount(order).to_f).to eq(-5)
-      end
+    it "raises not implemented error" do
+      expect{ voucher.display_value }
+        .to raise_error(NotImplementedError, 'please use concrete voucher')
     end
+  end
 
-    context 'when order total is less than the voucher' do
-      subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 20) }
+  describe '#compute_amount' do
+    subject(:voucher) { Vouchers::TestVoucher.new(code: 'new_code', enterprise: enterprise) }
 
-      it 'matches the order total' do
-        expect(subject.compute_amount(order).to_f).to eq(-10)
-      end
+    it "raises not implemented error" do
+      expect{ voucher.compute_amount(nil) }
+        .to raise_error(NotImplementedError, 'please use concrete voucher')
     end
   end
 
   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
@@ -65,4 +68,13 @@
       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