diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb index 01779e11cbc..501e6605dd4 100644 --- a/app/controllers/spree/orders_controller.rb +++ b/app/controllers/spree/orders_controller.rb @@ -69,6 +69,10 @@ def update if @order.contents.update_cart(order_params) @order.recreate_all_fees! # Enterprise fees on line items and on the order itself + # Re apply the voucher + VoucherAdjustmentsService.new(@order).update + @order.update_totals_and_states + if @order.complete? @order.update_payment_fees! @order.create_tax_charge! diff --git a/app/controllers/voucher_adjustments_controller.rb b/app/controllers/voucher_adjustments_controller.rb index c34570269c3..094ef26cf74 100644 --- a/app/controllers/voucher_adjustments_controller.rb +++ b/app/controllers/voucher_adjustments_controller.rb @@ -5,9 +5,6 @@ class VoucherAdjustmentsController < BaseController def create if add_voucher - VoucherAdjustmentsService.calculate(@order) - @order.update_totals_and_states - update_payment_section elsif @order.errors.present? render_error @@ -17,6 +14,9 @@ def create def destroy @order.voucher_adjustments.find_by(id: params[:id])&.destroy + # Update order to make sure we display the appropriate payment method + @order.update_totals_and_states + update_payment_section end @@ -41,12 +41,17 @@ def add_voucher adjustment = voucher.create_adjustment(voucher.code, @order) - unless adjustment.valid? + unless adjustment.persisted? @order.errors.add(:voucher_code, I18n.t('split_checkout.errors.add_voucher_error')) adjustment.errors.each { |error| @order.errors.import(error) } return false end + clear_payments + + VoucherAdjustmentsService.new(@order).update + @order.update_totals_and_states + true end @@ -74,4 +79,9 @@ def render_error def voucher_params params.require(:order).permit(:voucher_code) end + + # Clear payments and payment fees, to not affect voucher adjustment calculation + def clear_payments + @order.payments.incomplete.destroy_all + end end diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb index 3575dc1ae0b..8d5ec88b5c1 100644 --- a/app/models/spree/order/checkout.rb +++ b/app/models/spree/order/checkout.rb @@ -81,6 +81,12 @@ def self.define_state_machine! order.create_tax_charge! order.update_totals_and_states end + + after_transition to: :confirmation do |order| + VoucherAdjustmentsService.new(order).update + order.update_totals_and_states + end + after_transition to: :complete, do: :finalize! after_transition to: :resumed, do: :after_resume after_transition to: :canceled, do: :after_cancel diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 1199ad0820f..1551c6807b7 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -21,12 +21,11 @@ def display_value # but vouchers have complicated calculation so we can't easily use Spree::Calculator. We keep # the same method to stay as consistent as possible. # - # Creates a new voucher adjustment for the given order + # Creates a new voucher adjustment for the given order with an amount of 0 + # The amount will be calculated via VoucherAdjustmentsService#update def create_adjustment(label, order) - amount = compute_amount(order) - adjustment_attributes = { - amount: amount, + amount: 0, originator: self, order: order, label: label, diff --git a/app/services/voucher_adjustments_service.rb b/app/services/voucher_adjustments_service.rb index ccadb8eb752..355058b9d6d 100644 --- a/app/services/voucher_adjustments_service.rb +++ b/app/services/voucher_adjustments_service.rb @@ -1,68 +1,80 @@ # frozen_string_literal: true class VoucherAdjustmentsService - def self.calculate(order) - return if order.nil? + def initialize(order) + @order = order + end + + def update + return if @order.nil? # Find open Voucher Adjustment - return if order.voucher_adjustments.empty? + return if @order.voucher_adjustments.empty? # We only support one voucher per order right now, we could just loop on voucher_adjustments - adjustment = order.voucher_adjustments.first + adjustment = @order.voucher_adjustments.first - # Recalculate value - amount = adjustment.originator.compute_amount(order) + # Calculate value + amount = adjustment.originator.compute_amount(@order) # It is quite possible to have an order with both tax included in and tax excluded from price. # We should be able to caculate the relevant amount apply the current calculation. # # For now we just assume it is either all tax included in price or all tax excluded from price. - if order.additional_tax_total.positive? - handle_tax_excluded_from_price(order, amount) - elsif order.included_tax_total.positive? - handle_tax_included_in_price(order, amount) + if @order.additional_tax_total.positive? + handle_tax_excluded_from_price(amount) + elsif @order.included_tax_total.positive? + handle_tax_included_in_price(amount) else adjustment.amount = amount + adjustment.save end - - # Move to closed state - adjustment.close end - def self.handle_tax_excluded_from_price(order, amount) - voucher_rate = amount / order.total + private + + def handle_tax_excluded_from_price(amount) + voucher_rate = amount / @order.pre_discount_total + + adjustment = @order.voucher_adjustments.first # Adding the voucher tax part - tax_amount = voucher_rate * order.additional_tax_total + tax_amount = voucher_rate * @order.additional_tax_total + + update_tax_adjustment_for(adjustment, tax_amount) + + # Update the adjustment amount + adjustment_amount = voucher_rate * (@order.pre_discount_total - @order.additional_tax_total) + + adjustment.update_columns( + amount: adjustment_amount, + updated_at: Time.zone.now + ) + end - adjustment = order.voucher_adjustments.first + def update_tax_adjustment_for(adjustment, tax_amount) adjustment_attributes = { - amount: tax_amount, originator: adjustment.originator, - order: order, + order: @order, label: "Tax #{adjustment.label}", mandatory: false, - state: 'closed', + state: 'open', tax_category: nil, included_tax: 0 } - order.adjustments.create(adjustment_attributes) - - # Update the adjustment amount - amount = voucher_rate * (order.total - order.additional_tax_total) - adjustment.update_columns( - amount: amount, - updated_at: Time.zone.now - ) + # Update the amount if tax adjustment already exist, create if not + tax_adjustment = @order.adjustments.find_or_initialize_by(adjustment_attributes) + tax_adjustment.amount = tax_amount + tax_adjustment.save end - def self.handle_tax_included_in_price(order, amount) - voucher_rate = amount / order.total - included_tax = voucher_rate * order.included_tax_total + def handle_tax_included_in_price(amount) + voucher_rate = amount / @order.pre_discount_total + included_tax = voucher_rate * @order.included_tax_total # Update Adjustment - adjustment = order.voucher_adjustments.first + adjustment = @order.voucher_adjustments.first return unless amount != adjustment.amount || included_tax != 0 @@ -72,6 +84,4 @@ def self.handle_tax_included_in_price(order, amount) updated_at: Time.zone.now ) end - - private_class_method :handle_tax_included_in_price, :handle_tax_excluded_from_price end diff --git a/engines/order_management/app/services/order_management/order/updater.rb b/engines/order_management/app/services/order_management/order/updater.rb index fc727d7e35c..47d17f9877b 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -137,7 +137,10 @@ def update_payment_state end def update_all_adjustments - order.all_adjustments.reload.each(&:update_adjustment!) + # Voucher are modelled as a Spree::Adjustment but they don't behave like all the other + # adjustments, so we don't want voucher adjustment to be updated here. + # Calculation are handled by VoucherAdjustmentsService.calculate + order.all_adjustments.non_voucher.reload.each(&:update_adjustment!) end def before_save_hook diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index a41ec65812f..0c62a814968 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -45,8 +45,8 @@ let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 25) } let(:order) { create(:order_with_line_items, line_items_count: 3, distributor: enterprise) } - it 'includes the full voucher amount' do - expect(adjustment.amount.to_f).to eq(-25.0) + it 'includes an amount of 0' do + expect(adjustment.amount.to_f).to eq(0.0) end it 'has no included_tax' do diff --git a/spec/requests/voucher_adjustments_spec.rb b/spec/requests/voucher_adjustments_spec.rb index 8c93fb3f698..c6eda33db4b 100644 --- a/spec/requests/voucher_adjustments_spec.rb +++ b/spec/requests/voucher_adjustments_spec.rb @@ -8,7 +8,7 @@ let(:distributor) { create(:distributor_enterprise, with_payment_and_shipping: true) } let(:order_cycle) { create(:order_cycle, distributors: [distributor]) } let(:exchange) { order_cycle.exchanges.outgoing.first } - let(:order) do + let!(:order) do create( :order_with_line_items, line_items_count: 1, @@ -61,9 +61,28 @@ post "/voucher_adjustments", params: params expect(response).to be_unprocessable - expect(flash[:error]).to match( - "There was an error while adding the voucher and Label can't be blank" - ) + expect(flash[:error]).to match("Voucher code There was an error while adding the voucher") + end + end + + context "when the order has a payment and payment feed" do + let(:payment_method) { create(:payment_method, calculator: calculator) } + let(:calculator) { Calculator::FlatPercentItemTotal.new(preferred_flat_percent: 10) } + + before do + create(:payment, order: order, payment_method: payment_method, amount: order.total) + end + + it "removes existing payments" do + expect do + post "/voucher_adjustments", params: params + end.to change { order.reload.payments.count }.from(1).to(0) + end + + it "removes existing payment fees" do + expect do + post "/voucher_adjustments", params: params + end.to change { order.reload.all_adjustments.payment_fee.count }.from(1).to(0) end end end @@ -83,7 +102,7 @@ expect(response).to be_successful end - context "when adjustment doesn't exits" do + context "when adjustment doesn't exist" do it "does nothing" do delete "/voucher_adjustments/-1" diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb index fa2c1bb9516..b83a18ab809 100644 --- a/spec/services/voucher_adjustments_service_spec.rb +++ b/spec/services/voucher_adjustments_service_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe VoucherAdjustmentsService do - describe '.calculate' do + describe '#update' do let(:enterprise) { build(:enterprise) } let(:voucher) { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) } @@ -16,13 +16,13 @@ voucher.create_adjustment(voucher.code, order) order.update_columns(item_total: 6) - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).update expect(subject.amount.to_f).to eq(-6.0) end end - context 'with price included in order price' do + context 'with tax included in order price' do subject { order.voucher_adjustments.first } let(:order) do @@ -46,23 +46,51 @@ order.update_shipping_fees! order.update_order! - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).update end it 'updates the adjustment included_tax' do # voucher_rate = amount / order.total - # -10 / 150 = -0.066666667 + # -10 / 160 = -0.0625 # included_tax = voucher_rate * order.included_tax_total - # -0.66666666 * 10 = -0.67 - expect(subject.included_tax.to_f).to eq(-0.67) + # -0.0625 * 10 = -0.625 + expect(subject.included_tax.to_f).to eq(-0.63) end - it 'moves the adjustment state to closed' do - expect(subject.state).to eq('closed') + 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 'with price not included in order price' do + context 'with tax not included in order price' do let(:order) do create( :order_with_taxes, @@ -74,6 +102,8 @@ tax_rate_name: "Tax 1" ) end + let(:adjustment) { order.voucher_adjustments.first } + let(:tax_adjustment) { order.voucher_adjustments.second } before do # create adjustment before tax are set @@ -84,37 +114,62 @@ order.update_shipping_fees! order.update_order! - VoucherAdjustmentsService.calculate(order) + VoucherAdjustmentsService.new(order).update end - it 'includes amount withou tax' do - adjustment = order.voucher_adjustments.first + it 'includes amount without tax' do # voucher_rate = amount / order.total - # -10 / 161 = -0.062111801 + # -10 / 171 = -0.058479532 # amount = voucher_rate * (order.total - order.additional_tax_total) - # -0.062111801 * (161 -11) = -9.32 - expect(adjustment.amount.to_f).to eq(-9.32) + # -0.058479532 * (171 -11) = -9.36 + expect(adjustment.amount.to_f).to eq(-9.36) end it 'creates a tax adjustment' do # voucher_rate = amount / order.total - # -10 / 161 = -0.062111801 + # -10 / 171 = -0.058479532 # amount = voucher_rate * order.additional_tax_total - # -0.0585 * 11 = -0.68 - tax_adjustment = order.voucher_adjustments.second - expect(tax_adjustment.amount.to_f).to eq(-0.68) + # -0.058479532 * 11 = -0.64 + expect(tax_adjustment.amount.to_f).to eq(-0.64) expect(tax_adjustment.label).to match("Tax") end - it 'moves the adjustment state to closed' do - adjustment = order.voucher_adjustments.first - expect(adjustment.state).to eq('closed') + context "when re calculating" do + it "does not update the adjustment amount" do + expect do + VoucherAdjustmentsService.new(order).update + end.not_to change { adjustment.reload.amount } + end + + it "does not update the tax adjustment" do + expect do + VoucherAdjustmentsService.new(order).update + 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 context 'when no order given' do it "doesn't blow up" do - expect { VoucherAdjustmentsService.calculate(nil) }.to_not raise_error + expect { VoucherAdjustmentsService.new(nil).update }.to_not raise_error end end @@ -122,7 +177,7 @@ let(:order) { create(:order_with_line_items, line_items_count: 1, distributor: enterprise) } it "doesn't blow up" do - expect { VoucherAdjustmentsService.calculate(order) }.to_not raise_error + expect { VoucherAdjustmentsService.new(order).update }.to_not raise_error end end end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index 148f2a212c0..ffaaaff8b95 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -724,18 +724,20 @@ create(:voucher, code: 'some_code', enterprise: distributor, amount: 15) end - before do - visit checkout_step_path(:payment) - end - it "shows voucher input" do + visit checkout_step_path(:payment) + expect(page).to have_field "Enter voucher code" expect(page).to have_content "Apply voucher" end describe "adding voucher to the order" do + before do + visit checkout_step_path(:payment) + end + shared_examples "adding voucher to the order" do before do - fill_in "Enter voucher code", with: voucher.code + fill_in "Enter voucher code", with: "some_code" click_button("Apply") end @@ -756,7 +758,7 @@ it_behaves_like "adding voucher to the order" it "shows a warning message" do - fill_in "Enter voucher code", with: voucher.code + fill_in "Enter voucher code", with: "some_code" click_button("Apply") expect(page).to have_content( @@ -764,6 +766,16 @@ "you may not be able to spend the remaining value." ) end + + it "proceeds without requiring payment" do + fill_in "Enter voucher code", with: "some_code" + click_button("Apply") + + expect(page).to have_content "No payment required" + click_button "Next - Order summary" + # Expect to be on the Order Summary page + expect(page).to have_content "Delivery details" + end end context "voucher doesn't exist" do @@ -778,22 +790,45 @@ describe "removing voucher from order" do before do - voucher.create_adjustment(voucher.code, order) - # Reload the page so we pickup the voucher + add_voucher_to_order(voucher, order) + visit checkout_step_path(:payment) - end - it "removes voucher" do accept_confirm "Are you sure you want to remove the voucher?" do click_on "Remove code" end + end + it "removes voucher" do within '#voucher-section' do expect(page).to have_button("Apply", disabled: true) + expect(page).to have_field "Enter voucher code" # Currently no confirmation msg end + expect(page).not_to have_content "No payment required" expect(order.voucher_adjustments.length).to eq(0) end + + it "can re-enter a voucher" do + # Re-enter a voucher code + fill_in "Enter voucher code", with: "some_code" + click_button("Apply") + + expect(page).to have_content("$15.00 Voucher") + expect(order.reload.voucher_adjustments.length).to eq(1) + + expect(page).to have_content "No payment required" + + click_button "Next - Order summary" + # Expect to be on the Order Summary page + expect(page).to have_content "Delivery details" + end + + it "can proceed with payment" do + click_button "Next - Order summary" + # Expect to be on the Order Summary page + expect(page).to have_content "Delivery details" + end end end end @@ -1117,9 +1152,7 @@ let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor, amount: 6) } before do - voucher.create_adjustment(voucher.code, order) - order.update_totals - VoucherAdjustmentsService.calculate(order) + add_voucher_to_order(voucher, order) visit checkout_step_path(:summary) end @@ -1173,4 +1206,10 @@ end end end + + def add_voucher_to_order(voucher, order) + voucher.create_adjustment(voucher.code, order) + VoucherAdjustmentsService.new(order).update + order.update_totals_and_states + end end diff --git a/spec/system/consumer/split_checkout_tax_incl_spec.rb b/spec/system/consumer/split_checkout_tax_incl_spec.rb index 6663c6a970a..44e85ad1133 100644 --- a/spec/system/consumer/split_checkout_tax_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_incl_spec.rb @@ -47,9 +47,12 @@ calculator: Calculator::FlatRate.new(preferred_amount: 0.00)) } let!(:order_within_zone) { - create(:order, order_cycle: order_cycle, distributor: distributor, user: user_within_zone, - bill_address: address_within_zone, ship_address: address_within_zone, - state: "cart", line_items: [create(:line_item, variant: variant_with_tax)]) + create( + :order, + order_cycle: order_cycle, distributor: distributor, user: user_within_zone, + bill_address: address_within_zone, ship_address: address_within_zone, + state: "cart", line_items: [create(:line_item, variant: variant_with_tax, quantity: 1)] + ) } let!(:order_outside_zone) { create(:order, order_cycle: order_cycle, distributor: distributor, user: user_outside_zone, @@ -120,7 +123,7 @@ click_button "Next - Payment method" # add Voucher - fill_in "Enter voucher code", with: voucher.code + fill_in "Enter voucher code", with: "some_code" click_button("Apply") # Choose payment @@ -139,11 +142,64 @@ end # DB check - order_within_zone.reload - voucher_adjustment = order_within_zone.voucher_adjustments.first + assert_db_voucher_adjustment(-10.00, -1.15) + end + + describe "moving between summary to summary via edit cart" do + let!(:voucher) do + create(:voucher, code: 'good_code', enterprise: distributor, amount: 15) + end + + it "recalculate the tax component properly" do + visit checkout_step_path(:details) + proceed_to_payment + + # add Voucher + fill_in "Enter voucher code", with: "good_code" + click_button("Apply") + + proceed_to_summary + + assert_db_voucher_adjustment(-10.00, -1.15) + + # Click on edit link + within "div", text: /Order details/ do + # It's a bit brittle, but the scoping doesn't seem to work + all(".summary-edit").last.click + end + + # Update quantity + within ".cart-item-quantity" do + fill_in "order_line_items_attributes_0_quantity", with: "2" + end - expect(voucher_adjustment.amount.to_f).to eq(-10) - expect(voucher_adjustment.included_tax.to_f).to eq(-1.15) + click_button("Update") + + # Check adjustment has been recalculated + assert_db_voucher_adjustment(-15.00, -1.73) + + within "#cart-container" do + click_link("Checkout") + end + + # Go back to payment step + proceed_to_payment + + # Check voucher is still there + expect(page).to have_content("$15.00 Voucher") + + # Go to summary + proceed_to_summary + + # Check voucher value + within ".summary-right" do + expect(page).to have_content "good_code" + expect(page).to have_content "-15" + end + + # Check adjustment has been recalculated, we are not expecting any changes here + assert_db_voucher_adjustment(-15.00, -1.73) + end end end end @@ -190,4 +246,10 @@ def assert_db_no_tax_incl expect(order_outside_zone.included_tax_total).to eq(0.0) expect(order_outside_zone.additional_tax_total).to eq(0.0) end + + def assert_db_voucher_adjustment(amount, tax_amount) + adjustment = order_within_zone.voucher_adjustments.first + expect(adjustment.amount.to_f).to eq(amount) + expect(adjustment.included_tax.to_f).to eq(tax_amount) + 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 046a6ac4833..3f44c4328f6 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -129,7 +129,7 @@ click_button "Next - Payment method" # add Voucher - fill_in "Enter voucher code", with: voucher.code + fill_in "Enter voucher code", with: "some_code" click_button("Apply") expect(page).to have_selector ".voucher-added"