diff --git a/app/controllers/admin/vouchers_controller.rb b/app/controllers/admin/vouchers_controller.rb index 3f190a8001b..01ca689b911 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)) + # 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 diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 109ffd29656..4022a7559ff 100644 --- a/app/models/voucher.rb +++ b/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 diff --git a/app/models/vouchers/flat_rate.rb b/app/models/vouchers/flat_rate.rb new file mode 100644 index 00000000000..9ad0d3de898 --- /dev/null +++ b/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 diff --git a/app/models/vouchers/percentage_rate.rb b/app/models/vouchers/percentage_rate.rb new file mode 100644 index 00000000000..41b17e77f60 --- /dev/null +++ b/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 diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb index 355058b9d6d..98a4c63f5eb 100644 --- a/app/services/voucher_adjustments_service.rb +++ b/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 diff --git a/app/views/admin/vouchers/new.html.haml b/app/views/admin/vouchers/new.html.haml index fd83642389e..a27ea65d7a8 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.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 diff --git a/config/locales/en.yml b/config/locales/en.yml index eed3aa7db50..d3e147cf25c 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_rate: Flat + percentage_rate: Percentage (%) # Admin controllers controllers: 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..327cc96a1ee --- /dev/null +++ b/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 diff --git a/db/schema.rb b/db/schema.rb index c58419991e7..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,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" diff --git a/spec/factories/voucher_factory.rb b/spec/factories/voucher_factory.rb index c02de3fb52d..821728b09f3 100644 --- a/spec/factories/voucher_factory.rb +++ b/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 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..4e3c294af9b 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) } @@ -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 diff --git a/spec/models/vouchers/flat_rate_spec.rb b/spec/models/vouchers/flat_rate_spec.rb new file mode 100644 index 00000000000..419a103534c --- /dev/null +++ b/spec/models/vouchers/flat_rate_spec.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Vouchers::FlatRate do + describe 'validations' do + 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) } + 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(:voucher_flat_rate, 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, 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(: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 + 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 new file mode 100644 index 00000000000..e2e136bd14a --- /dev/null +++ b/spec/models/vouchers/percentage_rate_spec.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require 'spec_helper' + +describe Vouchers::PercentageRate do + describe 'validations' do + subject { build(:voucher_percentage_rate) } + + 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, 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(-1.5) + end + end + + describe "#rate" do + subject do + create(:voucher_percentage_rate, amount: 50) + end + + it "returns the voucher percentage rate" do + expect(subject.rate(nil)).to eq(-0.5) + end + end +end diff --git a/spec/requests/admin/vouchers_controller_spec.rb b/spec/requests/admin/vouchers_controller_spec.rb index e794a42af1e..60b0f34e176 100644 --- a/spec/requests/admin/vouchers_controller_spec.rb +++ b/spec/requests/admin/vouchers_controller_spec.rb @@ -26,21 +26,50 @@ let(:params) do { - voucher: { + vouchers_flat_rate: { code: code, - amount: amount + amount: amount, + voucher_type: type } } end let(:code) { "new_code" } let(:amount) { 15 } + 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) + 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/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/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb index b83a18ab809..4d710943c17 100644 --- a/spec/services/voucher_adjustments_service_spec.rb +++ b/spec/services/voucher_adjustments_service_spec.rb @@ -5,164 +5,277 @@ 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 - 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) + 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 "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 } - it "does not update the tax adjustment" do - expect do - VoucherAdjustmentsService.new(order).update - end.not_to change { subject.reload.included_tax } + 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 '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: true, + 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 diff --git a/spec/system/admin/vouchers_spec.rb b/spec/system/admin/vouchers_spec.rb index 7d9f586f6b0..68b7fe83f32 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 '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 + 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 '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 + 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 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) diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb index ffff4e6e9b5..f2aa3f281a6 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 @@ -146,9 +146,9 @@ 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, 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..09bc8e48f43 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,66 @@ 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 + + describe "moving between summary to summary via edit cart" do + let!(:voucher) do + create(:voucher_percentage_rate, 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") + + 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) - expect(voucher_adjustment.amount.to_f).to eq(-8.85) - expect(voucher_tax_adjustment.amount.to_f).to eq(-1.15) + 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.00% Voucher") + + # Go to summary + proceed_to_summary + + # Check voucher value + within ".summary-right" do + expect(page).to have_content "good_code" + expect(page).to have_content "-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 +287,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