diff --git a/app/controllers/split_checkout_controller.rb b/app/controllers/split_checkout_controller.rb index db900e7f0d1..31f2368b695 100644 --- a/app/controllers/split_checkout_controller.rb +++ b/app/controllers/split_checkout_controller.rb @@ -24,7 +24,6 @@ class SplitCheckoutController < ::BaseController def edit redirect_to_step_based_on_order unless params[:step] check_step if params[:step] - recalculate_tax if params[:step] == "summary" flash_error_when_no_shipping_method_available if available_shipping_methods.none? end @@ -204,6 +203,7 @@ def shipping_method_ship_address_not_required? def process_voucher if add_voucher + VoucherAdjustmentsService.calculate(@order) render_voucher_section_or_redirect elsif @order.errors.present? render_error @@ -226,7 +226,7 @@ def add_voucher adjustment = voucher.create_adjustment(voucher.code, @order) - if !adjustment.valid? + unless adjustment.valid? @order.errors.add(:voucher, I18n.t('split_checkout.errors.add_voucher_error')) adjustment.errors.each { |error| @order.errors.import(error) } return false @@ -308,18 +308,4 @@ def check_step redirect_to checkout_step_path(:payment) if params[:step] == "summary" end end - - def recalculate_tax - @order.create_tax_charge! - @order.update_order! - - apply_voucher if @order.voucher_adjustments.present? - end - - def apply_voucher - VoucherAdjustmentsService.calculate(@order) - - # update order to take into account the voucher we applied - @order.update_order! - end end diff --git a/app/controllers/spree/admin/orders/customer_details_controller.rb b/app/controllers/spree/admin/orders/customer_details_controller.rb index ed13ae4e523..cc6d6f679fd 100644 --- a/app/controllers/spree/admin/orders/customer_details_controller.rb +++ b/app/controllers/spree/admin/orders/customer_details_controller.rb @@ -24,7 +24,6 @@ def update end refresh_shipment_rates - recalculate_taxes OrderWorkflow.new(@order).advance_to_payment flash[:success] = Spree.t('customer_details_updated') @@ -52,15 +51,6 @@ def refresh_shipment_rates @order.shipments.map(&:refresh_rates) end - def recalculate_taxes - # If the order's address has been changed, the tax zone could be different, - # which means a different set of tax rates might be applicable. - @order.create_tax_charge! - Spree::TaxRate.adjust(@order, @order.adjustments.admin) - - @order.update_totals_and_states - end - def order_params params.require(:order).permit( :email, diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 57ef3d549c7..7ebacacafdf 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -67,6 +67,8 @@ class Adjustment < ApplicationRecord scope :charge, -> { where('amount >= 0') } scope :credit, -> { where('amount < 0') } scope :return_authorization, -> { where(originator_type: "Spree::ReturnAuthorization") } + scope :voucher, -> { where(originator_type: "Voucher") } + scope :non_voucher, -> { where.not(originator_type: "Voucher") } scope :inclusive, -> { where(included: true) } scope :additional, -> { where(included: false) } scope :legacy_tax, -> { additional.tax.where(adjustable_type: "Spree::Order") } diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index d180a4b8333..027bff09821 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -86,15 +86,6 @@ def states delegate :create_line_item_fees!, :create_order_fees!, :update_order_fees!, :update_line_item_fees!, :recreate_all_fees!, to: :fee_handler - # Needs to happen before save_permalink is called - before_validation :set_currency - before_validation :generate_order_number, if: :new_record? - before_validation :clone_billing_address, if: :use_billing? - before_validation :ensure_customer - - before_create :link_by_email - after_create :create_tax_charge! - validates :customer, presence: true, if: :require_customer? validate :products_available_from_new_distribution, if: lambda { distributor_id_changed? || order_cycle_id_changed? @@ -107,16 +98,25 @@ def states validates :order_cycle, presence: true, on: :require_distribution validates :distributor, presence: true, on: :require_distribution - make_permalink field: :number + before_validation :set_currency + before_validation :generate_order_number, if: :new_record? + before_validation :clone_billing_address, if: :use_billing? + before_validation :ensure_customer + before_create :link_by_email before_save :update_shipping_fees!, if: :complete? before_save :update_payment_fees!, if: :complete? + after_create :create_tax_charge! + after_save :reapply_tax_on_changed_address + after_save_commit DefaultAddressUpdater + make_permalink field: :number + attribute :send_cancellation_email, type: :boolean, default: true attribute :restock_items, type: :boolean, default: true - # -- Scopes + scope :not_empty, -> { left_outer_joins(:line_items).where.not(spree_line_items: { id: nil }) } @@ -171,6 +171,11 @@ def amount line_items.inject(0.0) { |sum, li| sum + li.amount } end + # Order total without any applied discounts from vouchers + def pre_discount_total + item_total + all_adjustments.additional.eligible.non_voucher.sum(:amount) + end + def currency self[:currency] || Spree::Config[:currency] end @@ -317,12 +322,13 @@ def ship_total # Creates new tax charges if there are any applicable rates. If prices already # include taxes then price adjustments are created instead. def create_tax_charge! - return if state.in?(["cart", "address", "delivery"]) + return if before_payment_state? clear_legacy_taxes! Spree::TaxRate.adjust(self, line_items) Spree::TaxRate.adjust(self, shipments) if shipments.any? + Spree::TaxRate.adjust(self, adjustments.admin) if adjustments.admin.any? fee_handler.tax_enterprise_fees! end @@ -574,8 +580,20 @@ def sorted_line_items end end + def before_payment_state? + state.in?(["cart", "address", "delivery"]) + end + private + def reapply_tax_on_changed_address + return if before_payment_state? + return unless tax_address&.saved_changes? + + create_tax_charge! + update_totals_and_states + end + def deliver_order_confirmation_email return if subscription.present? diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb index 9d0bc4c54cf..3575dc1ae0b 100644 --- a/app/models/spree/order/checkout.rb +++ b/app/models/spree/order/checkout.rb @@ -77,7 +77,10 @@ def self.define_state_machine! before_transition to: :delivery, do: :ensure_available_shipping_rates before_transition to: :confirmation, do: :validate_payment_method! - after_transition to: :payment, do: :create_tax_charge! + after_transition to: :payment do |order| + order.create_tax_charge! + 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 @@ -144,7 +147,7 @@ def state_changed(name) private def after_cancel - shipments.each(&:cancel!) + shipments.reject(&:canceled?).each(&:cancel!) payments.checkout.each(&:void!) OrderMailer.cancel_email(id).deliver_later if send_cancellation_email diff --git a/app/models/voucher.rb b/app/models/voucher.rb index 94743fc7316..1199ad0820f 100644 --- a/app/models/voucher.rb +++ b/app/models/voucher.rb @@ -41,6 +41,6 @@ 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.total) + -amount.clamp(0, order.pre_discount_total) end end diff --git a/app/services/order_fees_handler.rb b/app/services/order_fees_handler.rb index f05351aec7e..47be2c99035 100644 --- a/app/services/order_fees_handler.rb +++ b/app/services/order_fees_handler.rb @@ -21,7 +21,7 @@ def recreate_all_fees! create_order_fees! end - tax_enterprise_fees! + tax_enterprise_fees! unless order.before_payment_state? order.update_order! end diff --git a/app/services/order_workflow.rb b/app/services/order_workflow.rb index 53d7813e57e..815f3e1ee4c 100644 --- a/app/services/order_workflow.rb +++ b/app/services/order_workflow.rb @@ -24,13 +24,13 @@ def next(options = {}) end def advance_to_payment - return unless order.state.in? ["cart", "address", "delivery"] + return unless order.before_payment_state? advance_to_state("payment", advance_order_options) end def advance_checkout(options = {}) - advance_to = order.state.in?(["cart", "address", "delivery"]) ? "payment" : "confirmation" + advance_to = order.before_payment_state? ? "payment" : "confirmation" advance_to_state(advance_to, advance_order_options.merge(options)) end diff --git a/app/views/layouts/darkswarm.html.haml b/app/views/layouts/darkswarm.html.haml index 472f37b74f6..e28fb78bffe 100644 --- a/app/views/layouts/darkswarm.html.haml +++ b/app/views/layouts/darkswarm.html.haml @@ -46,7 +46,6 @@ = yield #footer - %loading = yield :scripts = inject_current_hub diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb index 5664d9e3a56..cca61f44d38 100644 --- a/spec/controllers/spree/admin/orders_controller_spec.rb +++ b/spec/controllers/spree/admin/orders_controller_spec.rb @@ -57,7 +57,7 @@ it "updates fees and taxes and redirects to order details page" do expect(order).to receive(:recreate_all_fees!) - expect(order).to receive(:create_tax_charge!) + expect(order).to receive(:create_tax_charge!).at_least :once spree_put :update, params diff --git a/spec/models/proxy_order_spec.rb b/spec/models/proxy_order_spec.rb index 8f6c9973aaa..17078eb5d2d 100644 --- a/spec/models/proxy_order_spec.rb +++ b/spec/models/proxy_order_spec.rb @@ -118,6 +118,7 @@ } break unless order.next! while !order.completed? order.cancel + order.reload end it "returns true, clears canceled_at and resumes the order" do diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 35a6ba5779a..f1b30805b55 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -250,6 +250,7 @@ module Spree context "when the shipment has an inclusive tax rate" do it "calculates the shipment tax from the tax rate" do order.shipments = [shipment] + order.state = "payment" order.create_tax_charge! order.update_totals @@ -274,6 +275,7 @@ module Spree it "records the tax on the shipment's adjustments" do order.shipments = [shipment] + order.state = "payment" order.create_tax_charge! order.update_totals @@ -355,6 +357,7 @@ module Spree context "when enterprise fees have a fixed tax_category" do before do + order.update(state: "payment") order.recreate_all_fees! end @@ -440,6 +443,11 @@ module Spree calculator: ::Calculator::FlatRate.new(preferred_amount: 50.0)) } + before do + order.update(state: "payment") + order.create_tax_charge! + end + describe "when the tax rate includes the tax in the price" do it "records no tax on the enterprise fee adjustments" do # EnterpriseFee tax category is nil and inheritance only applies to per item fees @@ -471,6 +479,11 @@ module Spree calculator: ::Calculator::PerItem.new(preferred_amount: 50.0)) } + before do + order.update(state: "payment") + order.create_tax_charge! + end + describe "when the tax rate includes the tax in the price" do it "records the correct amount in a tax adjustment" do # Applying product tax rate of 0.2 to enterprise fee of $50 @@ -522,6 +535,8 @@ module Spree tax_category.tax_rates << tax_rate allow(order).to receive(:tax_zone) { zone } order.line_items << create(:line_item, variant: variant, quantity: 5) + order.update(state: "payment") + order.create_tax_charge! end context "with included taxes" do diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index adaaf97f4ec..526ebb71b06 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -336,6 +336,7 @@ before do order.cancel! + order.reload order.resume! end diff --git a/spec/models/voucher_spec.rb b/spec/models/voucher_spec.rb index c5bbcdabe54..a41ec65812f 100644 --- a/spec/models/voucher_spec.rb +++ b/spec/models/voucher_spec.rb @@ -20,20 +20,21 @@ end describe '#compute_amount' do - subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 10) } - let(:order) { create(:order_with_totals) } - it 'returns -10' do - expect(subject.compute_amount(order).to_f).to eq(-10) + context 'when order total is more than the voucher' do + subject { create(:voucher, 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 smaller than 10' do - it 'returns minus the order total' do - order.total = 6 - order.save! + context 'when order total is less than the voucher' do + subject { create(:voucher, code: 'new_code', enterprise: enterprise, amount: 20) } - expect(subject.compute_amount(order).to_f).to eq(-6) + it 'matches the order total' do + expect(subject.compute_amount(order).to_f).to eq(-10) end end end diff --git a/spec/services/voucher_adjustments_service_spec.rb b/spec/services/voucher_adjustments_service_spec.rb index 0fc08d7260d..fa2c1bb9516 100644 --- a/spec/services/voucher_adjustments_service_spec.rb +++ b/spec/services/voucher_adjustments_service_spec.rb @@ -14,9 +14,7 @@ it 'updates the adjustment amount to -order.total' do voucher.create_adjustment(voucher.code, order) - - order.total = 6 - order.save! + order.update_columns(item_total: 6) VoucherAdjustmentsService.calculate(order) diff --git a/spec/system/admin/bulk_order_management_spec.rb b/spec/system/admin/bulk_order_management_spec.rb index 269e06ccb0a..ce2921bf9ad 100644 --- a/spec/system/admin/bulk_order_management_spec.rb +++ b/spec/system/admin/bulk_order_management_spec.rb @@ -945,6 +945,9 @@ let!(:li2) { create(:line_item_with_shipment, order: o2 ) } before :each do + o1.update_totals_and_states + o2.update_totals_and_states + visit_bulk_order_management end diff --git a/spec/system/consumer/split_checkout_spec.rb b/spec/system/consumer/split_checkout_spec.rb index f2939bb53e7..2bcc9f1cd0f 100644 --- a/spec/system/consumer/split_checkout_spec.rb +++ b/spec/system/consumer/split_checkout_spec.rb @@ -1114,11 +1114,8 @@ let(:voucher) { create(:voucher, code: 'some_code', enterprise: distributor, amount: 6) } before do - # Add voucher to the order voucher.create_adjustment(voucher.code, order) - - # Update order so voucher adjustment is properly taken into account - order.update_order! + order.update_totals VoucherAdjustmentsService.calculate(order) visit checkout_step_path(:summary) 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 b899f5d01de..d4a7ddda411 100644 --- a/spec/system/consumer/split_checkout_tax_not_incl_spec.rb +++ b/spec/system/consumer/split_checkout_tax_not_incl_spec.rb @@ -10,8 +10,12 @@ include AuthenticationHelper include WebHelper - let!(:address_within_zone) { create(:address, state: Spree::State.first) } - let!(:address_outside_zone) { create(:address, state: Spree::State.second) } + let!(:state) { create(:state, name: "Victoria") } + let!(:zone) { create(:zone_with_state_member, default_tax: false, member: state) } + let!(:address_within_zone) { create(:address, state: state) } + let!(:address_outside_zone) { + create(:address, state: create(:state, name: "Timbuktu", country: state.country)) + } let(:user_within_zone) { create(:user, bill_address: address_within_zone, ship_address: address_within_zone) @@ -20,8 +24,7 @@ create(:user, bill_address: address_outside_zone, ship_address: address_outside_zone) } - let!(:zone) { create(:zone_with_state_member, name: 'Victoria', default_tax: false) } - let!(:tax_category) { create(:tax_category, name: "Veggies", is_default: "f") } + let!(:tax_category) { create(:tax_category, name: "Veggies", is_default: false) } let!(:tax_rate) { create(:tax_rate, name: "Tax rate - included or not", amount: 0.13, zone: zone, tax_category: tax_category, included_in_price: false) @@ -62,7 +65,7 @@ Spree::Config.set(tax_using_ship_address: true) end - pending "a not-included tax" do + describe "a not-included tax" do before do zone.update!(default_tax: false) tax_rate.update!(included_in_price: false) @@ -99,12 +102,12 @@ choose "Delivery" click_button "Next - Payment method" - click_on "Next - Order summary" - click_on "Complete order" # DB checks - order_within_zone.reload - expect(order_within_zone.additional_tax_total).to eq(1.3) + expect(order_within_zone.reload.additional_tax_total).to eq(1.3) + + click_on "Next - Order summary" + click_on "Complete order" # UI checks expect(page).to have_content("Confirmed") @@ -112,7 +115,7 @@ expect(page).to have_selector('#tax-row', text: with_currency(1.30)) end - context "when using a voucher" do + pending "when using a voucher" do let!(:voucher) do create(:voucher, code: 'some_code', enterprise: distributor, amount: 10) end