diff --git a/app/models/concerns/vouchers/flat_ratable.rb b/app/models/concerns/vouchers/flat_ratable.rb new file mode 100644 index 0000000000..21f4882cf7 --- /dev/null +++ b/app/models/concerns/vouchers/flat_ratable.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +require "active_support/concern" + +module Vouchers + module FlatRatable + extend ActiveSupport::Concern + + included do + validates :amount, + presence: true, + numericality: { greater_than: 0 } + end + + 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 diff --git a/app/models/voucher.rb b/app/models/voucher.rb index b8ff92a81d..5be186d4d9 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -1,8 +1,6 @@ # frozen_string_literal: false class Voucher < ApplicationRecord - VINE_TYPE = "VINE".freeze - self.belongs_to_required_by_default = false acts_as_paranoid @@ -16,13 +14,10 @@ class Voucher < ApplicationRecord class_name: 'Spree::Adjustment', dependent: nil - validates :code, presence: true, uniqueness: { scope: :enterprise_id } + validates :code, presence: true TYPES = ["Vouchers::FlatRate", "Vouchers::PercentageRate"].freeze - scope :vine, -> { where(voucher_type: VINE_TYPE) } - scope :local, -> { where("voucher_type IS DISTINCT FROM ?", VINE_TYPE) } - def code=(value) super(value.to_s.strip) end @@ -47,10 +42,6 @@ class Voucher < ApplicationRecord order.adjustments.create(adjustment_attributes) end - def vine? - voucher_type == VINE_TYPE - end - # The following method must be overriden in a concrete voucher. def display_value raise NotImplementedError, 'please use concrete voucher' diff --git a/app/models/vouchers/flat_rate.rb b/app/models/vouchers/flat_rate.rb index 9ad0d3de89..45b85e26ab 100644 --- a/app/models/vouchers/flat_rate.rb +++ b/app/models/vouchers/flat_rate.rb @@ -2,24 +2,8 @@ module Vouchers class FlatRate < Voucher - validates :amount, - presence: true, - numericality: { greater_than: 0 } + include FlatRatable - 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 + validates :code, uniqueness: { scope: :enterprise_id } end end diff --git a/app/models/vouchers/percentage_rate.rb b/app/models/vouchers/percentage_rate.rb index 41b17e77f6..06d0263280 100644 --- a/app/models/vouchers/percentage_rate.rb +++ b/app/models/vouchers/percentage_rate.rb @@ -5,6 +5,7 @@ module Vouchers validates :amount, presence: true, numericality: { greater_than: 0, less_than_or_equal_to: 100 } + validates :code, uniqueness: { scope: :enterprise_id } def display_value ActionController::Base.helpers.number_to_percentage(amount, precision: 2) diff --git a/app/models/vouchers/vine.rb b/app/models/vouchers/vine.rb new file mode 100644 index 0000000000..3e87047a54 --- /dev/null +++ b/app/models/vouchers/vine.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: false + +module Vouchers + class Vine < Voucher + include FlatRatable + + # a VINE voucher : + # - can potentially be associated with mutiple enterprise + # - code ( "short code" in VINE ) can be recycled, but they shouldn't be linked to the same + # voucher_id + validates :code, uniqueness: { scope: [:enterprise_id, :external_voucher_id] } + end +end diff --git a/db/migrate/20241112230401_update_index_and_remove_voucher_type_from_voucher.rb b/db/migrate/20241112230401_update_index_and_remove_voucher_type_from_voucher.rb new file mode 100644 index 0000000000..df967c3e19 --- /dev/null +++ b/db/migrate/20241112230401_update_index_and_remove_voucher_type_from_voucher.rb @@ -0,0 +1,9 @@ +class UpdateIndexAndRemoveVoucherTypeFromVoucher < ActiveRecord::Migration[7.0] + def change + remove_column :vouchers, :voucher_type + remove_index :vouchers, [:code, :enterprise_id], unique: true + + add_index :vouchers, [:code, :enterprise_id] + add_index :vouchers, [:code, :enterprise_id, :external_voucher_id], name: "index_vouchers_on_code_and_enterprise_id_and_ext_voucher_id" + end +end diff --git a/db/schema.rb b/db/schema.rb index c9128ba17a..be371a9499 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2024_10_30_033956) do +ActiveRecord::Schema[7.0].define(version: 2024_11_12_230401) do # These are extensions that must be enabled in order to support this database enable_extension "pg_stat_statements" enable_extension "plpgsql" @@ -1112,8 +1112,8 @@ ActiveRecord::Schema[7.0].define(version: 2024_10_30_033956) do t.string "type", limit: 255, default: "Vouchers::FlatRate", null: false t.uuid "external_voucher_id" t.uuid "external_voucher_set_id" - t.string "voucher_type" - t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id", unique: true + t.index ["code", "enterprise_id", "external_voucher_id"], name: "index_vouchers_on_code_and_enterprise_id_and_ext_voucher_id" + t.index ["code", "enterprise_id"], name: "index_vouchers_on_code_and_enterprise_id" t.index ["deleted_at"], name: "index_vouchers_on_deleted_at" t.index ["enterprise_id"], name: "index_vouchers_on_enterprise_id" end diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb index 034fae6b77..df6835e61e 100644 --- a/spec/factories/voucher_factory.rb +++ b/spec/factories/voucher_factory.rb @@ -15,8 +15,8 @@ FactoryBot.define do amount { rand(1..100) } end - factory :vine_voucher, parent: :voucher_flat_rate do - voucher_type { Voucher::VINE_TYPE } + factory :vine_voucher, parent: :voucher, class: Vouchers::Vine do + amount { 20 } external_voucher_id { SecureRandom.uuid } external_voucher_set_id { SecureRandom.uuid } end diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index c0043f42eb..aff8f19eef 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -15,31 +15,6 @@ RSpec.describe Voucher do it { is_expected.to have_many(:adjustments).dependent(nil) } end - describe "scope" do - let!(:vine_voucher) { create(:vine_voucher) } - - describe ".vine" do - it "returns only vine vouchers" do - create(:voucher) - expect(Voucher.vine.count).to eq(1) - end - end - - describe ".local" do - it "returns only local vouchers" do - create(:voucher) - expect(Voucher.local.count).to eq(1) - end - - context "with voucher type other than VINE" do - it "returns only local vouchers" do - create(:voucher, voucher_type: "other") - expect(Voucher.local.count).to eq(1) - end - end - end - end - describe '#code=' do it "removes leading and trailing whitespace" do voucher = build(:voucher, code: "\r\n\t new_code \r\n\t") @@ -52,7 +27,6 @@ RSpec.describe Voucher do subject { build(:voucher_flat_rate, code: 'new_code', enterprise:) } it { is_expected.to validate_presence_of(:code) } - it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } end describe '#display_value' do diff --git a/spec/models/vouchers/flat_rate_spec.rb b/spec/models/vouchers/flat_rate_spec.rb index 9294784746..10c0f56a46 100644 --- a/spec/models/vouchers/flat_rate_spec.rb +++ b/spec/models/vouchers/flat_rate_spec.rb @@ -8,6 +8,7 @@ RSpec.describe Vouchers::FlatRate do 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_uniqueness_of(:code).scoped_to(:enterprise_id) } end describe '#compute_amount' do diff --git a/spec/models/vouchers/percentage_rate_spec.rb b/spec/models/vouchers/percentage_rate_spec.rb index b172baf016..116751d126 100644 --- a/spec/models/vouchers/percentage_rate_spec.rb +++ b/spec/models/vouchers/percentage_rate_spec.rb @@ -12,6 +12,7 @@ RSpec.describe Vouchers::PercentageRate do .is_greater_than(0) .is_less_than_or_equal_to(100) end + it { is_expected.to validate_uniqueness_of(:code).scoped_to(:enterprise_id) } end describe '#compute_amount' do diff --git a/spec/models/vouchers/vine_spec.rb b/spec/models/vouchers/vine_spec.rb new file mode 100644 index 0000000000..157aac3776 --- /dev/null +++ b/spec/models/vouchers/vine_spec.rb @@ -0,0 +1,83 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe Vouchers::Vine do + describe 'validations' do + subject { build(:vine_voucher) } + + it { is_expected.to validate_presence_of(:amount) } + it { is_expected.to validate_numericality_of(:amount).is_greater_than(0) } + + describe "#code" do + subject { build(:vine_voucher, code: 'vine_code', enterprise:, external_voucher_id: ) } + + let(:external_voucher_id) { SecureRandom.uuid } + let(:enterprise) { create(:enterprise) } + + it { + is_expected.to validate_uniqueness_of(:code).scoped_to( + [:enterprise_id, :external_voucher_id] + ) + } + + it "can be reused within the same enterprise" do + subject.save! + # Voucher with the same code but different external_voucher_id, it is mapped to a + # different voucher in VINE + voucher = build(:vine_voucher, code: 'vine_code', enterprise: ) + expect(voucher.valid?).to be(true) + end + + it "can be used by mutiple enterprises" do + subject.save! + # Voucher with the same code and external_voucher_id, ie exiting VINE voucher used by + # another enterprise + voucher = build(:vine_voucher, code: 'vine_code', enterprise: build(:enterprise), + external_voucher_id: ) + expect(voucher.valid?).to be(true) + end + end + end + + 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(:vine_voucher, 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(:vine_voucher, amount: 20) } + + it 'matches the order total' do + expect(subject.compute_amount(order).to_f).to eq(-15) + end + end + end + + describe "#rate" do + subject do + create(:vine_voucher, 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 + expect(subject.rate(order).to_f).to eq(-0.5) + end + end +end