From 8fccdbf92f1945ee9d5884f5a0261ff96929ba30 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 00:05:11 +0000 Subject: [PATCH 01/53] Introduce TaxCategories to ShippingMethods --- .../spree/admin/shipping_methods_controller.rb | 1 + app/models/calculator/default_tax.rb | 12 ++++++++---- app/models/spree/shipment.rb | 4 ++++ app/models/spree/shipping_method.rb | 2 ++ .../spree/admin/shipping_methods/_form.html.haml | 9 +++++++-- 5 files changed, 22 insertions(+), 6 deletions(-) diff --git a/app/controllers/spree/admin/shipping_methods_controller.rb b/app/controllers/spree/admin/shipping_methods_controller.rb index 1ede5fe867..9ff1f93831 100644 --- a/app/controllers/spree/admin/shipping_methods_controller.rb +++ b/app/controllers/spree/admin/shipping_methods_controller.rb @@ -79,6 +79,7 @@ module Spree def load_data @available_zones = Zone.order(:name) + @tax_categories = Spree::TaxCategory.order(:name) @calculators = ShippingMethod.calculators.sort_by(&:name) end diff --git a/app/models/calculator/default_tax.rb b/app/models/calculator/default_tax.rb index 5293dfd1cb..fa5a254252 100644 --- a/app/models/calculator/default_tax.rb +++ b/app/models/calculator/default_tax.rb @@ -13,6 +13,8 @@ module Calculator case computable when Spree::Order compute_order(computable) + when Spree::Shipment + compute_shipment(computable) when Spree::LineItem compute_line_item(computable) end @@ -70,17 +72,19 @@ module Calculator .sum { |applicator| applicator.enterprise_fee.compute_amount(order) } end - def compute_line_item(line_item) - if line_item.tax_category == rate.tax_category + def compute_shipment_or_line_item(item) + if item.tax_category == rate.tax_category if rate.included_in_price - deduced_total_by_rate(line_item.total, rate) + deduced_total_by_rate(item.amount, rate) else - round_to_two_places(line_item.total * rate.amount) + round_to_two_places(item.amount * rate.amount) end else 0 end end + alias_method :compute_shipment, :compute_shipment_or_line_item + alias_method :compute_line_item, :compute_shipment_or_line_item def round_to_two_places(amount) BigDecimal(amount.to_s).round(2, BigDecimal::ROUND_HALF_UP) diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index afad7e4428..9bd02bd1a0 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -104,6 +104,10 @@ module Spree save! end + def tax_category + selected_shipping_rate.try(:shipping_method).try(:tax_category) + end + def refresh_rates return shipping_rates if shipped? diff --git a/app/models/spree/shipping_method.rb b/app/models/spree/shipping_method.rb index 426d658b43..a910bf55ef 100644 --- a/app/models/spree/shipping_method.rb +++ b/app/models/spree/shipping_method.rb @@ -23,6 +23,8 @@ module Spree class_name: 'Spree::Zone', foreign_key: 'shipping_method_id' + belongs_to :tax_category, class_name: 'Spree::TaxCategory' + validates :name, presence: true validate :distributor_validation validate :at_least_one_shipping_category diff --git a/app/views/spree/admin/shipping_methods/_form.html.haml b/app/views/spree/admin/shipping_methods/_form.html.haml index ba11c45ecd..934e808a7a 100644 --- a/app/views/spree/admin/shipping_methods/_form.html.haml +++ b/app/views/spree/admin/shipping_methods/_form.html.haml @@ -43,9 +43,14 @@ = f.hidden_field :tag_list, "ng-value" => "shippingMethod.tag_list" %tags-with-translation#something{ object: "shippingMethod", 'find-tags' => 'findTags(query)' } - + = render partial: 'spree/admin/shared/calculator_fields', locals: { f: f } + %fieldset.tax_categories.no-border-bottom + = f.field_container :tax_categories do + = f.select :tax_category_id, @tax_categories.map { |tc| [tc.name, tc.id] }, {}, :class => "select2 fullwidth" + = error_message_on :shipping_method, :tax_category_id + %fieldset.categories.no-border-bottom %legend{align: "center"}= t('.categories') = f.field_container :categories do @@ -56,7 +61,7 @@ %br/ = error_message_on :shipping_method, :shipping_category_id - + %fieldset.no-border-bottom %legend{align: "center"}= t('.zones') = f.field_container :zones do From 431706b7043750581b0e53d8a7c0f06e449cb3ae Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 12:50:51 +0000 Subject: [PATCH 02/53] Add tax_category_id to spree_shipping_methods --- ...20210207120753_add_tax_category_id_to_shipping_methods.rb | 5 +++++ db/schema.rb | 1 + 2 files changed, 6 insertions(+) create mode 100644 db/migrate/20210207120753_add_tax_category_id_to_shipping_methods.rb diff --git a/db/migrate/20210207120753_add_tax_category_id_to_shipping_methods.rb b/db/migrate/20210207120753_add_tax_category_id_to_shipping_methods.rb new file mode 100644 index 0000000000..b330c066e8 --- /dev/null +++ b/db/migrate/20210207120753_add_tax_category_id_to_shipping_methods.rb @@ -0,0 +1,5 @@ +class AddTaxCategoryIdToShippingMethods < ActiveRecord::Migration + def change + add_column :spree_shipping_methods, :tax_category_id, :integer + end +end diff --git a/db/schema.rb b/db/schema.rb index 5321045034..565509c577 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -807,6 +807,7 @@ ActiveRecord::Schema.define(version: 20210320003951) do t.boolean "require_ship_address", default: true t.text "description" t.string "tracking_url", limit: 255 + t.integer "tax_category_id" end create_table "spree_shipping_methods_zones", id: false, force: :cascade do |t| From 45f008232111f1bc3a65d27e2ae646007916670f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Feb 2021 23:42:58 +0000 Subject: [PATCH 03/53] Add tax_category_id to shipping method permitted params --- app/controllers/spree/admin/shipping_methods_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/spree/admin/shipping_methods_controller.rb b/app/controllers/spree/admin/shipping_methods_controller.rb index 9ff1f93831..027603bff6 100644 --- a/app/controllers/spree/admin/shipping_methods_controller.rb +++ b/app/controllers/spree/admin/shipping_methods_controller.rb @@ -86,7 +86,7 @@ module Spree def permitted_resource_params params.require(:shipping_method).permit( :name, :description, :display_on, :require_ship_address, :tag_list, :calculator_type, - distributor_ids: [], + :tax_category_id, distributor_ids: [], calculator_attributes: PermittedAttributes::Calculator.attributes ) end From ac9ecdfcbc5e77f6ca8fee91313cf74f7f508fc3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 13:07:33 +0000 Subject: [PATCH 04/53] Introduce Spree::ItemAdjustments class --- app/models/spree/item_adjustments.rb | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 app/models/spree/item_adjustments.rb diff --git a/app/models/spree/item_adjustments.rb b/app/models/spree/item_adjustments.rb new file mode 100644 index 0000000000..c6cb1238c4 --- /dev/null +++ b/app/models/spree/item_adjustments.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +module Spree + # Manage (recalculate) item (LineItem or Shipment) adjustments + class ItemAdjustments + attr_reader :item + + delegate :adjustments, :order, to: :item + + def initialize(item) + @item = item + end + + def update + update_adjustments if item.persisted? + item + end + + def update_adjustments + adjustment_total = adjustments.map(&:update!).compact.sum + + item.update_columns( + adjustment_total: adjustment_total, + updated_at: Time.zone.now + ) + end + end +end From 605a94e3c9608972e935633726b39e98df09ea68 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 13:10:07 +0000 Subject: [PATCH 05/53] Use Spree::ItemAdjustments in Shipment callbacks for updating adjustments --- app/models/spree/shipment.rb | 12 +++++++++--- spec/models/spree/shipment_spec.rb | 9 +-------- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index 9bd02bd1a0..33e34f4ffd 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -15,7 +15,7 @@ module Spree has_many :adjustments, as: :adjustable, dependent: :destroy before_create :generate_shipment_number - after_save :ensure_correct_adjustment, :update_order + after_save :ensure_correct_adjustment, :update_adjustments attr_accessor :special_instructions alias_attribute :amount, :cost @@ -344,8 +344,14 @@ module Spree end end - def update_order - order.reload.update! + def update_adjustments + return unless cost_changed? && state != 'shipped' + + recalculate_adjustments + end + + def recalculate_adjustments + Spree::ItemAdjustments.new(self).update end end end diff --git a/spec/models/spree/shipment_spec.rb b/spec/models/spree/shipment_spec.rb index 7359921472..614711579f 100644 --- a/spec/models/spree/shipment_spec.rb +++ b/spec/models/spree/shipment_spec.rb @@ -423,13 +423,6 @@ describe Spree::Shipment do end end - context "update_order" do - it "should update order" do - expect(order).to receive_message_chain(:reload, :update!) - shipment.__send__(:update_order) - end - end - describe "#update_amounts" do it "persists the shipping cost from the shipping fee adjustment" do allow(shipment).to receive(:fee_adjustment) { double(:adjustment, amount: 10) } @@ -442,7 +435,7 @@ describe Spree::Shipment do context "after_save" do it "should run correct callbacks" do expect(shipment).to receive(:ensure_correct_adjustment) - expect(shipment).to receive(:update_order) + expect(shipment).to receive(:update_adjustments) shipment.run_callbacks(:save) end end From 89889bc280ad3a476ec807131399cf07fa7533aa Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 13:20:59 +0000 Subject: [PATCH 06/53] Record tax totals in ItemAdjustments --- app/models/spree/item_adjustments.rb | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/app/models/spree/item_adjustments.rb b/app/models/spree/item_adjustments.rb index c6cb1238c4..15aaafad0c 100644 --- a/app/models/spree/item_adjustments.rb +++ b/app/models/spree/item_adjustments.rb @@ -17,9 +17,16 @@ module Spree end def update_adjustments - adjustment_total = adjustments.map(&:update!).compact.sum + tax_adjustments = + (item.respond_to?(:all_adjustments) ? item.all_adjustments : item.adjustments).tax + + adjustment_total = adjustments.additional.map(&:update!).compact.sum + included_tax_total = tax_adjustments.inclusive.reload.map(&:update!).compact.sum + additional_tax_total = tax_adjustments.additional.reload.map(&:update!).compact.sum item.update_columns( + included_tax_total: included_tax_total, + additional_tax_total: additional_tax_total, adjustment_total: adjustment_total, updated_at: Time.zone.now ) From 9bbe57afdafcf25dc538086ab84b9985f9fe814c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 13:22:26 +0000 Subject: [PATCH 07/53] Extract method --- app/models/spree/item_adjustments.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/models/spree/item_adjustments.rb b/app/models/spree/item_adjustments.rb index 15aaafad0c..1dd334f47d 100644 --- a/app/models/spree/item_adjustments.rb +++ b/app/models/spree/item_adjustments.rb @@ -17,9 +17,6 @@ module Spree end def update_adjustments - tax_adjustments = - (item.respond_to?(:all_adjustments) ? item.all_adjustments : item.adjustments).tax - adjustment_total = adjustments.additional.map(&:update!).compact.sum included_tax_total = tax_adjustments.inclusive.reload.map(&:update!).compact.sum additional_tax_total = tax_adjustments.additional.reload.map(&:update!).compact.sum @@ -31,5 +28,11 @@ module Spree updated_at: Time.zone.now ) end + + private + + def tax_adjustments + (item.respond_to?(:all_adjustments) ? item.all_adjustments : item.adjustments).tax + end end end From 385b12d83b21e05e14908562073581c7dfc51cf8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 13:23:47 +0000 Subject: [PATCH 08/53] Add tax total fields to spree_shipments --- .../20210207131247_add_tax_totals_to_shipment.rb | 14 ++++++++++++++ db/schema.rb | 2 ++ 2 files changed, 16 insertions(+) create mode 100644 db/migrate/20210207131247_add_tax_totals_to_shipment.rb diff --git a/db/migrate/20210207131247_add_tax_totals_to_shipment.rb b/db/migrate/20210207131247_add_tax_totals_to_shipment.rb new file mode 100644 index 0000000000..8627226fda --- /dev/null +++ b/db/migrate/20210207131247_add_tax_totals_to_shipment.rb @@ -0,0 +1,14 @@ +class AddTaxTotalsToShipment < ActiveRecord::Migration + def up + add_column :spree_shipments, :included_tax_total, :decimal, + precision: 10, scale: 2, null: false, default: 0.0 + + add_column :spree_shipments, :additional_tax_total, :decimal, + precision: 10, scale: 2, null: false, default: 0.0 + end + + def down + remove_column :spree_shipments, :included_tax_total + remove_column :spree_shipments, :additional_tax_total + end +end diff --git a/db/schema.rb b/db/schema.rb index 565509c577..b50d299209 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -778,6 +778,8 @@ ActiveRecord::Schema.define(version: 20210320003951) do t.datetime "updated_at", null: false t.string "state", limit: 255 t.integer "stock_location_id" + t.decimal "included_tax_total", precision: 10, scale: 2, default: "0.0", null: false + t.decimal "additional_tax_total", precision: 10, scale: 2, default: "0.0", null: false t.index ["number"], name: "index_shipments_on_number", using: :btree t.index ["order_id"], name: "index_spree_shipments_on_order_id", unique: true, using: :btree end From 25a739ea75e4f6a1a2a4eb82f8684e89b05dc772 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 16 Feb 2021 17:36:31 +0000 Subject: [PATCH 09/53] Remove Shipment #update_adjustment_included_tax --- app/models/spree/shipment.rb | 9 --------- 1 file changed, 9 deletions(-) diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index 33e34f4ffd..5f3b6c6c55 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -281,7 +281,6 @@ module Spree end update_amounts if fee_adjustment&.amount != cost - update_adjustment_included_tax if fee_adjustment end def adjustment_label @@ -336,14 +335,6 @@ module Spree ShipmentMailer.shipped_email(id).deliver_later end - def update_adjustment_included_tax - if Config.shipment_inc_vat && (order.distributor.nil? || order.distributor.charges_sales_tax) - fee_adjustment.set_included_tax! Config.shipping_tax_rate - else - fee_adjustment.set_included_tax! 0 - end - end - def update_adjustments return unless cost_changed? && state != 'shipped' From 0fea92b63d81bc00b117a66823b5eba93baa913f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 8 Feb 2021 16:00:17 +0000 Subject: [PATCH 10/53] Add :default_tax_calculator factory --- spec/factories/calculator_factory.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/spec/factories/calculator_factory.rb b/spec/factories/calculator_factory.rb index 3142de884c..91c571a8fe 100644 --- a/spec/factories/calculator_factory.rb +++ b/spec/factories/calculator_factory.rb @@ -25,4 +25,6 @@ FactoryBot.define do c.save! } end + + factory :default_tax_calculator, class: Calculator::DefaultTax end From 0d5d4aca11a7dcabca8a8696534752a419aa19de Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 8 Feb 2021 18:13:57 +0000 Subject: [PATCH 11/53] Update tax_rate_factory The amount is a decimal, so `0.1` here is actually a 10% tax rate. `100` is ridiculous (10000% tax). Also, the factory fails without a calculator, and it should explicitly be a DefaultTax calculator or it will not be correct. --- spec/factories/tax_rate_factory.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/factories/tax_rate_factory.rb b/spec/factories/tax_rate_factory.rb index 2cb9f6e780..fa72254674 100644 --- a/spec/factories/tax_rate_factory.rb +++ b/spec/factories/tax_rate_factory.rb @@ -3,7 +3,8 @@ FactoryBot.define do factory :tax_rate, class: Spree::TaxRate do zone - amount { 100.00 } + amount { 0.1 } tax_category + association(:calculator, factory: :default_tax_calculator, strategy: :build) end end From 7425ad5c4aeea43e60fb5ea4f5f7a4ea99fb8ef8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 8 Feb 2021 17:58:29 +0000 Subject: [PATCH 12/53] Include ItemAdjustments handling in Adjustment class Currently only applies to shipments, but will later include line items, etc --- app/models/spree/adjustment.rb | 8 ++++++++ app/models/spree/item_adjustments.rb | 6 +++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index a0bd3e1541..164f292bbf 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -47,6 +47,8 @@ module Spree validates :label, presence: true validates :amount, numericality: true + after_create :update_adjustable_adjustment_total + state_machine :state, initial: :open do event :close do transition from: :open, to: :closed @@ -149,5 +151,11 @@ module Spree originator_type.constantize.unscoped { super } end + + private + + def update_adjustable_adjustment_total + Spree::ItemAdjustments.new(adjustable).update if adjustable + end end end diff --git a/app/models/spree/item_adjustments.rb b/app/models/spree/item_adjustments.rb index 1dd334f47d..aedd8f0fbf 100644 --- a/app/models/spree/item_adjustments.rb +++ b/app/models/spree/item_adjustments.rb @@ -12,7 +12,7 @@ module Spree end def update - update_adjustments if item.persisted? + update_adjustments if updatable_totals? item end @@ -31,6 +31,10 @@ module Spree private + def updatable_totals? + item.persisted? && item.is_a?(Spree::Shipment) + end + def tax_adjustments (item.respond_to?(:all_adjustments) ? item.all_adjustments : item.adjustments).tax end From 7e0aad7c7eb91ca10a24ec92d8480aaa60199221 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 23:17:11 +0000 Subject: [PATCH 13/53] Adapt current tax setup to work with shipment taxes This is a temporary step... --- app/models/calculator/default_tax.rb | 7 +++++++ app/models/spree/tax_rate.rb | 4 ++-- 2 files changed, 9 insertions(+), 2 deletions(-) diff --git a/app/models/calculator/default_tax.rb b/app/models/calculator/default_tax.rb index fa5a254252..3ab75caac3 100644 --- a/app/models/calculator/default_tax.rb +++ b/app/models/calculator/default_tax.rb @@ -33,6 +33,7 @@ module Calculator [ line_items_total(order), + shipments_total(order), per_item_fees_total(order, calculator), per_order_fees_total(order, calculator) ].sum do |total| @@ -48,6 +49,12 @@ module Calculator matched_line_items.sum(&:total) end + def shipments_total(order) + order.shipments.select do |shipment| + shipment.tax_category == rate.tax_category + end.sum(&:cost) + end + # Finds relevant fees for each line_item, # calculates the tax on them, and returns the total tax def per_item_fees_total(order, calculator) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index afb74a6f98..fb2f84a018 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -36,8 +36,7 @@ module Spree end def self.adjust(order) - order.adjustments.tax.destroy_all - order.line_item_adjustments.where(originator_type: 'Spree::TaxRate').destroy_all + order.all_adjustments.tax.destroy_all match(order).each do |rate| rate.adjust(order) @@ -63,6 +62,7 @@ module Spree if included_in_price if default_zone_or_zone_match? order order.line_items.each { |line_item| create_adjustment(label, line_item, line_item) } + order.shipments.each { |shipment| create_adjustment(label, shipment, shipment) } else amount = -1 * calculator.compute(order) label = Spree.t(:refund) + label From 232286b0be78baa8d6afd77a4d040de7d8b5143c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 7 Feb 2021 15:16:57 +0000 Subject: [PATCH 14/53] Add adjustment_total to spree_shipments --- ...151520_add_adjustment_total_to_shipment.rb | 31 +++++++++++++++++++ db/schema.rb | 1 + 2 files changed, 32 insertions(+) create mode 100644 db/migrate/20210207151520_add_adjustment_total_to_shipment.rb diff --git a/db/migrate/20210207151520_add_adjustment_total_to_shipment.rb b/db/migrate/20210207151520_add_adjustment_total_to_shipment.rb new file mode 100644 index 0000000000..aeee8eac43 --- /dev/null +++ b/db/migrate/20210207151520_add_adjustment_total_to_shipment.rb @@ -0,0 +1,31 @@ +class AddAdjustmentTotalToShipment < ActiveRecord::Migration + def up + add_column :spree_shipments, :adjustment_total, :decimal, + precision: 10, scale: 2, null: false, default: 0.0 + + populate_adjustment_totals + end + + def down + remove_column :spree_shipments, :adjustment_total + end + + def populate_adjustment_totals + # Populates the new `adjustment_total` field in the spree_shipments table. Sets the value + # to the shipment's (shipping fee) adjustment amount. + + adjustment_totals_sql = <<-SQL + UPDATE spree_shipments + SET adjustment_total = shipping_adjustment.fee_amount + FROM ( + SELECT spree_adjustments.source_id AS shipment_id, spree_adjustments.amount AS fee_amount + FROM spree_adjustments + WHERE spree_adjustments.source_type = 'Spree::Shipment' + AND spree_adjustments.amount <> 0 + ) shipping_adjustment + WHERE spree_shipments.id = shipping_adjustment.shipment_id + SQL + + ActiveRecord::Base.connection.execute(adjustment_totals_sql) + end +end diff --git a/db/schema.rb b/db/schema.rb index b50d299209..1ed302c3cf 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -780,6 +780,7 @@ ActiveRecord::Schema.define(version: 20210320003951) do t.integer "stock_location_id" t.decimal "included_tax_total", precision: 10, scale: 2, default: "0.0", null: false t.decimal "additional_tax_total", precision: 10, scale: 2, default: "0.0", null: false + t.decimal "adjustment_total", precision: 10, scale: 2, default: "0.0", null: false t.index ["number"], name: "index_shipments_on_number", using: :btree t.index ["order_id"], name: "index_spree_shipments_on_order_id", unique: true, using: :btree end From db07a73bd7cfec14c2b9873aaf671dad41bb194a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 16 Feb 2021 18:55:01 +0000 Subject: [PATCH 15/53] Ensure Adjustment#update! returns an amount --- app/models/spree/adjustment.rb | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 164f292bbf..03d147f0b7 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -101,15 +101,17 @@ module Spree # object on the association would be in a old state and therefore the # adjustment calculations would not performed on proper values def update!(calculable = nil, force: false) - return if immutable? && !force - return if originator.blank? + return amount if immutable? && !force - amount = originator.compute_amount(calculable || adjustable) + if originator.present? + amount = originator.compute_amount(calculable || adjustable) + update_columns( + amount: amount, + updated_at: Time.zone.now, + ) + end - update_columns( - amount: amount, - updated_at: Time.zone.now, - ) + amount end def currency From 7d3288ca53dbf599ffcc4e5662c91328ba8a9efa Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 16:28:30 +0000 Subject: [PATCH 16/53] Remove #to_package calls on shipment calculations --- lib/spree/core/calculated_adjustments.rb | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/lib/spree/core/calculated_adjustments.rb b/lib/spree/core/calculated_adjustments.rb index f773567647..ab220f365a 100644 --- a/lib/spree/core/calculated_adjustments.rb +++ b/lib/spree/core/calculated_adjustments.rb @@ -27,18 +27,12 @@ module Spree # calculator as applied to the given calculable (Order, LineItems[], Shipment, etc.) # By default the adjustment will not be considered mandatory def create_adjustment(label, target, calculable, mandatory = false, state = "closed") - # Adjustment calculations done on Spree::Shipment objects MUST - # be done on their to_package'd variants instead - # It's only the package that contains the correct information. - # See https://github.com/spree/spree_active_shipping/pull/96 et. al - old_calculable = calculable - calculable = calculable.to_package if calculable.is_a?(Spree::Shipment) amount = compute_amount(calculable) return if amount.zero? && !mandatory adjustment_attributes = { amount: amount, - source: old_calculable, + source: calculable, originator: self, order: order_object_for(target), label: label, From 8b0a9ddb2f7ab174ed43d80708a127de4a863aa3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 16:44:39 +0000 Subject: [PATCH 17/53] Update OrderTaxAdjustmentsFetcher --- app/services/order_tax_adjustments_fetcher.rb | 24 ++++--------------- .../order_tax_adjustments_fetcher_spec.rb | 9 ++----- 2 files changed, 7 insertions(+), 26 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 686efff9a4..103953947b 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -20,24 +20,11 @@ class OrderTaxAdjustmentsFetcher attr_reader :order def all - Spree::Adjustment - .where(order_adjustments.or(line_item_adjustments).or(shipment_adjustments)) - .order('created_at ASC') - end + tax_adjustments = order.all_adjustments.tax + enterprise_fees_with_tax = order.all_adjustments.enterprise_fee.with_tax + admin_adjustments_with_tax = order.adjustments.admin.with_tax - def order_adjustments - table[:adjustable_id].eq(order.id) - .and(table[:adjustable_type].eq('Spree::Order')) - end - - def line_item_adjustments - table[:adjustable_id].eq_any(order.line_item_ids) - .and(table[:adjustable_type].eq('Spree::LineItem')) - end - - def shipment_adjustments - table[:order_id].eq(order.id) - .and(table[:adjustable_type].eq('Spree::Shipment')) + tax_adjustments | enterprise_fees_with_tax | admin_adjustments_with_tax end def table @@ -66,10 +53,9 @@ class OrderTaxAdjustmentsFetcher end def no_tax_adjustments?(adjustment) - # Enterprise Fees, Admin Adjustments, and Shipping Fees currently do not have tax adjustments. + # Enterprise Fees and Admin Adjustments currently do not have tax adjustments. # The tax amount is stored in the included_tax attribute. adjustment.originator_type == "EnterpriseFee" || - adjustment.originator_type == "Spree::ShippingMethod" || (adjustment.source_type.nil? && adjustment.originator_type.nil?) end end diff --git a/spec/services/order_tax_adjustments_fetcher_spec.rb b/spec/services/order_tax_adjustments_fetcher_spec.rb index f5edd0d848..ba908dc83b 100644 --- a/spec/services/order_tax_adjustments_fetcher_spec.rb +++ b/spec/services/order_tax_adjustments_fetcher_spec.rb @@ -66,14 +66,9 @@ describe OrderTaxAdjustmentsFetcher do distributor: coordinator ) end - - before do - allow(Spree::Config).to receive(:shipment_inc_vat).and_return(true) - allow(Spree::Config).to receive(:shipping_tax_rate).and_return(tax_rate15.amount) - end - let(:shipping_method) do - create(:shipping_method, calculator: Calculator::FlatRate.new(preferred_amount: 46.0)) + create(:shipping_method, calculator: Calculator::FlatRate.new(preferred_amount: 46.0), + tax_category: tax_category15) end let!(:shipment) do create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) From a060b9fe5a9cdd18a94721a9c5049fbad261cf0f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 17:44:57 +0000 Subject: [PATCH 18/53] Update LineItemsController and order factory --- .../controllers/line_items_controller_spec.rb | 28 ++++++++++++++----- spec/factories/order_factory.rb | 4 ++- 2 files changed, 24 insertions(+), 8 deletions(-) diff --git a/spec/controllers/line_items_controller_spec.rb b/spec/controllers/line_items_controller_spec.rb index c7e169f9ff..0693f6f3d1 100644 --- a/spec/controllers/line_items_controller_spec.rb +++ b/spec/controllers/line_items_controller_spec.rb @@ -103,22 +103,36 @@ describe LineItemsController, type: :controller do end context "on a completed order with shipping and payment fees" do + let(:zone) { create(:zone_with_member) } + let(:shipping_tax_rate) do + create(:tax_rate, included_in_price: true, + calculator: Calculator::DefaultTax.new, + amount: 0.25, + zone: zone) + end + let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) } let(:shipping_fee) { 3 } let(:payment_fee) { 5 } let(:distributor_with_taxes) { create(:distributor_enterprise_with_tax) } - let(:order) { create(:completed_order_with_fees, distributor: distributor_with_taxes, shipping_fee: shipping_fee, payment_fee: payment_fee) } + let(:order) { + create(:completed_order_with_fees, distributor: distributor_with_taxes, + shipping_fee: shipping_fee, payment_fee: payment_fee, + shipping_tax_category: shipping_tax_category) + } before do - Spree::Config.shipment_inc_vat = true - Spree::Config.shipping_tax_rate = 0.25 + allow(order).to receive(:tax_zone) { zone } + order.reload + order.create_tax_charge! end it "updates the fees" do # Sanity check fees item_num = order.line_items.length initial_fees = item_num * (shipping_fee + payment_fee) - expect(order.adjustment_total).to eq initial_fees - expect(order.shipments.last.fee_adjustment.included_tax).to eq 1.2 + + expect(order.shipment.adjustments.tax.count).to eq 1 + expect(order.shipment.included_tax_total).to eq 1.2 # Delete the item item = order.line_items.first @@ -130,9 +144,9 @@ describe LineItemsController, type: :controller do order.reload order.shipment.reload expect(order.adjustment_total).to eq initial_fees - shipping_fee - payment_fee - expect(order.shipments.last.fee_adjustment.amount).to eq shipping_fee + expect(order.shipment.adjustment_total).to eq shipping_fee expect(order.payments.first.adjustment.amount).to eq payment_fee - expect(order.shipments.last.fee_adjustment.included_tax).to eq 0.6 + expect(order.shipment.included_tax_total).to eq 0.6 end end diff --git a/spec/factories/order_factory.rb b/spec/factories/order_factory.rb index 7e75d7a05f..eab716bd0b 100644 --- a/spec/factories/order_factory.rb +++ b/spec/factories/order_factory.rb @@ -196,6 +196,7 @@ FactoryBot.define do transient do payment_fee { 5 } shipping_fee { 3 } + shipping_tax_category { nil } end ship_address { create(:address) } @@ -214,7 +215,8 @@ FactoryBot.define do state: 'checkout') create(:shipping_method_with, :shipping_fee, shipping_fee: evaluator.shipping_fee, - distributors: [order.distributor]) + distributors: [order.distributor], + tax_category: evaluator.shipping_tax_category) order.reload while !order.completed? do break unless order.next! end From a8366a3a72994969666da101e983250f8fe6564a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 17:46:19 +0000 Subject: [PATCH 19/53] Update Order::Updater totals counts --- .../app/services/order_management/order/updater.rb | 2 +- .../spec/services/order_management/order/updater_spec.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) 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 b391592577..54cb620cb4 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -61,7 +61,7 @@ module OrderManagement order.additional_tax_total = all_adjustments.tax.additional.sum(:amount) order.included_tax_total = order.line_item_adjustments.tax.inclusive.sum(:amount) + all_adjustments.enterprise_fee.sum(:included_tax) + - all_adjustments.shipping.sum(:included_tax) + + order.shipment_adjustments.tax.inclusive.sum(:amount) + adjustments.admin.sum(:included_tax) end diff --git a/engines/order_management/spec/services/order_management/order/updater_spec.rb b/engines/order_management/spec/services/order_management/order/updater_spec.rb index 47a92a520c..f85ccfa176 100644 --- a/engines/order_management/spec/services/order_management/order/updater_spec.rb +++ b/engines/order_management/spec/services/order_management/order/updater_spec.rb @@ -32,6 +32,7 @@ module OrderManagement allow(order).to receive_message_chain(:all_adjustments, :tax, :additional, :sum).and_return(20) allow(order).to receive_message_chain(:all_adjustments, :enterprise_fee, :sum).and_return(10) allow(order).to receive_message_chain(:all_adjustments, :shipping, :sum).and_return(5) + allow(order).to receive_message_chain(:shipment_adjustments, :tax, :inclusive, :sum).and_return(5) allow(order).to receive_message_chain(:adjustments, :admin, :sum).and_return(2) updater.update_adjustment_total From ead85e71dea1f1a3105be2e12e4c45da3ed3d062 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 17:53:55 +0000 Subject: [PATCH 20/53] Update shipping tax in checkout spec --- spec/features/consumer/shopping/checkout_spec.rb | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb index a2a03a5979..14e6d151d0 100644 --- a/spec/features/consumer/shopping/checkout_spec.rb +++ b/spec/features/consumer/shopping/checkout_spec.rb @@ -17,9 +17,15 @@ feature "As a consumer I want to check out my cart", js: true do let(:product) { create(:taxed_product, supplier: supplier, price: 10, zone: zone, tax_rate_amount: 0.1) } let(:variant) { product.variants.first } let(:order) { create(:order, order_cycle: order_cycle, distributor: distributor, bill_address_id: nil, ship_address_id: nil) } + let(:shipping_tax_rate) { create(:tax_rate, amount: 0.25, zone: zone, included_in_price: true) } + let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) } let(:free_shipping) { create(:shipping_method, require_ship_address: true, name: "Frogs", description: "yellow", calculator: Calculator::FlatRate.new(preferred_amount: 0.00)) } - let(:shipping_with_fee) { create(:shipping_method, require_ship_address: false, name: "Donkeys", description: "blue", calculator: Calculator::FlatRate.new(preferred_amount: 4.56)) } + let(:shipping_with_fee) { + create(:shipping_method, require_ship_address: false, tax_category: shipping_tax_category, + name: "Donkeys", description: "blue", + calculator: Calculator::FlatRate.new(preferred_amount: 4.56)) + } let(:tagged_shipping) { create(:shipping_method, require_ship_address: false, name: "Local", tag_list: "local") } let!(:check_without_fee) { create(:payment_method, distributors: [distributor], name: "Roger rabbit", type: "Spree::PaymentMethod::Check") } let!(:check_with_fee) { create(:payment_method, distributors: [distributor], calculator: Calculator::FlatRate.new(preferred_amount: 5.67)) } @@ -32,9 +38,6 @@ feature "As a consumer I want to check out my cart", js: true do end before do - Spree::Config.shipment_inc_vat = true - Spree::Config.shipping_tax_rate = 0.25 - add_enterprise_fee enterprise_fee set_order order add_product_to_cart order, product From 2af0afafe9e027ea31ea46a31d3bc38830a0978f Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 18:30:01 +0000 Subject: [PATCH 21/53] Update Order#shipping_tax --- app/models/spree/order.rb | 2 +- spec/models/spree/order_spec.rb | 15 +++++++++------ 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index faac4a7089..df08a4dd47 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -646,7 +646,7 @@ module Spree end def shipping_tax - shipment_adjustments.reload.shipping.sum(:included_tax) + shipment_adjustments.reload.tax.sum(:amount) end def enterprise_fee_tax diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 58895613e9..51adbd5480 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -636,16 +636,19 @@ describe Spree::Order do describe "getting the shipping tax" do let(:order) { create(:order) } - let(:shipping_method) { create(:shipping_method_with, :flat_rate) } + let(:shipping_tax_rate) { create(:tax_rate, amount: 0.25, included_in_price: true, zone: create(:zone_with_member)) } + let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) } + let!(:shipping_method) { create(:shipping_method_with, :flat_rate, tax_category: shipping_tax_category) } context "with a taxed shipment" do - before do - allow(Spree::Config).to receive(:shipment_inc_vat).and_return(true) - allow(Spree::Config).to receive(:shipping_tax_rate).and_return(0.25) - end - let!(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) } + before do + allow(order).to receive(:tax_zone) { shipping_tax_rate.zone } + order.reload + order.create_tax_charge! + end + it "returns the shipping tax" do expect(order.shipping_tax).to eq(10) end From 889b357408ecf2cf0673911d5da579359a31caa1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 18:47:41 +0000 Subject: [PATCH 22/53] Update Order#total_tax --- spec/models/spree/order_spec.rb | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index 51adbd5480..b55fa2df9d 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -680,11 +680,7 @@ describe Spree::Order do end describe "getting the total tax" do - before do - allow(Spree::Config).to receive(:shipment_inc_vat).and_return(true) - allow(Spree::Config).to receive(:shipping_tax_rate).and_return(0.25) - end - + let(:shipping_tax_rate) { create(:tax_rate, amount: 0.25) } let(:order) { create(:order) } let(:shipping_method) { create(:shipping_method_with, :flat_rate) } let!(:shipment) do @@ -693,15 +689,10 @@ describe Spree::Order do let(:enterprise_fee) { create(:enterprise_fee) } before do - create( - :adjustment, - order: order, - adjustable: order, - originator: enterprise_fee, - label: "EF", - amount: 123, - included_tax: 2 - ) + create(:adjustment, adjustable: order, originator: enterprise_fee, label: "EF", amount: 123, + included_tax: 2, order: order) + create(:adjustment, adjustable: shipment, source: shipment, originator: shipping_tax_rate, + amount: 10, order: order, state: "closed") order.update! end From 1d1b842e0895320ceb50a809a6b3cb2009d413be Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 21:42:53 +0000 Subject: [PATCH 23/53] Delete dead code --- lib/open_food_network/sales_tax_report.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/lib/open_food_network/sales_tax_report.rb b/lib/open_food_network/sales_tax_report.rb index af50c3cc85..5d1099e824 100644 --- a/lib/open_food_network/sales_tax_report.rb +++ b/lib/open_food_network/sales_tax_report.rb @@ -102,13 +102,5 @@ module OpenFoodNetwork def tax_included_in(line_item) line_item.adjustments.tax.inclusive.sum(:amount) end - - def shipment_inc_vat - Spree::Config.shipment_inc_vat - end - - def shipping_tax_rate - Spree::Config.shipping_tax_rate - end end end From 1f897b1e441f43e33d2e1a03ef1346d80b920fd5 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 21:45:59 +0000 Subject: [PATCH 24/53] Update sales tax report spec --- spec/features/admin/reports_spec.rb | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb index 9b92596dd1..066592b4d3 100644 --- a/spec/features/admin/reports_spec.rb +++ b/spec/features/admin/reports_spec.rb @@ -166,7 +166,9 @@ feature ' let(:distributor2) { create(:distributor_enterprise, with_payment_and_shipping: true, charges_sales_tax: true) } let(:user1) { create(:user, enterprises: [distributor1]) } let(:user2) { create(:user, enterprises: [distributor2]) } - let!(:shipping_method) { create(:shipping_method_with, :expensive_name, distributors: [distributor1]) } + let(:shipping_tax_rate) { create(:tax_rate, amount: 0.20, included_in_price: true, zone: zone) } + let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) } + let!(:shipping_method) { create(:shipping_method_with, :expensive_name, distributors: [distributor1], tax_category: shipping_tax_category) } let(:enterprise_fee) { create(:enterprise_fee, enterprise: user1.enterprises.first, tax_category: product2.tax_category, calculator: Calculator::FlatRate.new(preferred_amount: 120.0)) } let(:order_cycle) { create(:simple_order_cycle, coordinator: distributor1, coordinator_fees: [enterprise_fee], distributors: [distributor1], variants: [product1.master]) } @@ -180,12 +182,12 @@ feature ' let!(:line_item2) { create(:line_item, variant: product2.master, price: 500.15, quantity: 3, order: order1) } before do - allow(Spree::Config).to receive(:shipment_inc_vat) { true } - allow(Spree::Config).to receive(:shipping_tax_rate) { 0.2 } - + order1.reload 2.times { order1.next } order1.select_shipping_method shipping_method.id order1.reload.recreate_all_fees! + order1.create_tax_charge! + order1.update! order1.finalize! login_as_admin_and_visit spree.admin_reports_path From 2f19428b4013f1dd7ea12b0bc7781372b83a4bc1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 21:53:14 +0000 Subject: [PATCH 25/53] Update orders controller --- app/controllers/spree/orders_controller.rb | 1 + .../spree/orders_controller_spec.rb | 20 +++++++++++++------ 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/app/controllers/spree/orders_controller.rb b/app/controllers/spree/orders_controller.rb index 7eb12a39c5..4a81b98a3a 100644 --- a/app/controllers/spree/orders_controller.rb +++ b/app/controllers/spree/orders_controller.rb @@ -86,6 +86,7 @@ module Spree if @order.complete? @order.update_shipping_fees! @order.update_payment_fees! + @order.create_tax_charge! end respond_with(@order) do |format| diff --git a/spec/controllers/spree/orders_controller_spec.rb b/spec/controllers/spree/orders_controller_spec.rb index b6ae15f616..36f5b4f386 100644 --- a/spec/controllers/spree/orders_controller_spec.rb +++ b/spec/controllers/spree/orders_controller_spec.rb @@ -265,7 +265,12 @@ describe Spree::OrdersController, type: :controller do describe "removing items from a completed order" do context "with shipping and transaction fees" do let(:distributor) { create(:distributor_enterprise, charges_sales_tax: true, allow_order_changes: true) } - let(:order) { create(:completed_order_with_fees, distributor: distributor, shipping_fee: shipping_fee, payment_fee: payment_fee) } + let(:shipping_tax_rate) { create(:tax_rate, amount: 0.25, included_in_price: true, zone: create(:zone_with_member)) } + let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) } + let(:order) { + create(:completed_order_with_fees, distributor: distributor, shipping_fee: shipping_fee, + payment_fee: payment_fee, shipping_tax_category: shipping_tax_category) + } let(:line_item1) { order.line_items.first } let(:line_item2) { order.line_items.second } let(:shipping_fee) { 3 } @@ -274,14 +279,16 @@ describe Spree::OrdersController, type: :controller do let(:expected_fees) { item_num * (shipping_fee + payment_fee) } before do - allow(Spree::Config).to receive(:shipment_inc_vat) { true } - allow(Spree::Config).to receive(:shipping_tax_rate) { 0.25 } + allow(order).to receive(:tax_zone) { shipping_tax_rate.zone } + order.reload + order.create_tax_charge! # Sanity check the fees - expect(order.all_adjustments.length).to eq 2 + expect(order.all_adjustments.length).to eq 3 expect(item_num).to eq 2 expect(order.adjustment_total).to eq expected_fees - expect(order.shipment.fee_adjustment.included_tax).to eq 1.2 + expect(order.shipment.adjustments.tax.first.amount).to eq 1.2 + expect(order.shipment.included_tax_total).to eq 1.2 allow(subject).to receive(:spree_current_user) { order.user } allow(subject).to receive(:order_to_update) { order } @@ -296,7 +303,8 @@ describe Spree::OrdersController, type: :controller do expect(order.reload.line_items.count).to eq 1 expect(order.adjustment_total).to eq(1 * (shipping_fee + payment_fee)) - expect(order.shipment.fee_adjustment.included_tax).to eq 0.6 + expect(order.shipment.adjustments.tax.first.amount).to eq 0.6 + expect(order.shipment.included_tax_total).to eq 0.6 end end From 93771aeb949f3c2d6c7f19f2f2f1540bfa5b29f6 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 22:18:40 +0000 Subject: [PATCH 26/53] Update orders spec --- spec/models/spree/order_spec.rb | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index b55fa2df9d..fc3512fa55 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1037,22 +1037,29 @@ describe Spree::Order do describe "a completed order with shipping and transaction fees" do let(:distributor) { create(:distributor_enterprise_with_tax) } - let(:order) { create(:completed_order_with_fees, distributor: distributor, shipping_fee: shipping_fee, payment_fee: payment_fee) } + let(:zone) { create(:zone_with_member) } + let(:shipping_tax_rate) { create(:tax_rate, amount: 0.25, included_in_price: true, zone: zone) } + let(:shipping_tax_category) { create(:tax_category, tax_rates: [shipping_tax_rate]) } + let(:order) { + create(:completed_order_with_fees, distributor: distributor, shipping_fee: shipping_fee, + payment_fee: payment_fee, + shipping_tax_category: shipping_tax_category) + } let(:shipping_fee) { 3 } let(:payment_fee) { 5 } let(:item_num) { order.line_items.length } let(:expected_fees) { item_num * (shipping_fee + payment_fee) } before do - Spree::Config.shipment_inc_vat = true - Spree::Config.shipping_tax_rate = 0.25 + order.reload + order.create_tax_charge! # Sanity check the fees - expect(order.all_adjustments.length).to eq 2 - expect(order.shipment_adjustments.length).to eq 1 + expect(order.all_adjustments.length).to eq 3 + expect(order.shipment_adjustments.length).to eq 2 expect(item_num).to eq 2 expect(order.adjustment_total).to eq expected_fees - expect(order.shipment.fee_adjustment.included_tax).to eq 1.2 + expect(order.shipment.included_tax_total).to eq 1.2 end context "removing line_items" do @@ -1061,7 +1068,7 @@ describe Spree::Order do order.save expect(order.adjustment_total).to eq expected_fees - shipping_fee - payment_fee - expect(order.shipment.fee_adjustment.included_tax).to eq 0.6 + expect(order.shipment.included_tax_total).to eq 0.6 end context "when finalized fee adjustments exist on the order" do @@ -1080,7 +1087,7 @@ describe Spree::Order do # Check if fees got updated order.reload expect(order.adjustment_total).to eq expected_fees - expect(order.shipment.fee_adjustment.included_tax).to eq 1.2 + expect(order.shipment.included_tax_total).to eq 1.2 end end end @@ -1093,7 +1100,7 @@ describe Spree::Order do order.save expect(order.adjustment_total).to eq expected_fees - (item_num * shipping_fee) - expect(order.shipment.fee_adjustment.included_tax).to eq 0 + expect(order.shipment.included_tax_total).to eq 0 end end From d635d70add97ac3ec52348e3b1b964ba96a8f4f2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Feb 2021 22:37:38 +0000 Subject: [PATCH 27/53] Update Order::Updater adjustment handling --- .../app/services/order_management/order/updater.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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 54cb620cb4..e1bd346619 100644 --- a/engines/order_management/app/services/order_management/order/updater.rb +++ b/engines/order_management/app/services/order_management/order/updater.rb @@ -122,7 +122,7 @@ module OrderManagement end def update_all_adjustments - order.adjustments.reload.each(&:update!) + order.all_adjustments.reload.each(&:update!) end def before_save_hook From 2ab9602ad707b781480e36bdd93346f852b97ee9 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 20 Feb 2021 11:49:16 +0000 Subject: [PATCH 28/53] Increase tested details in Order spec --- spec/models/spree/order_spec.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/spec/models/spree/order_spec.rb b/spec/models/spree/order_spec.rb index fc3512fa55..6f1e7f1317 100644 --- a/spec/models/spree/order_spec.rb +++ b/spec/models/spree/order_spec.rb @@ -1059,6 +1059,7 @@ describe Spree::Order do expect(order.shipment_adjustments.length).to eq 2 expect(item_num).to eq 2 expect(order.adjustment_total).to eq expected_fees + expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 1.2 expect(order.shipment.included_tax_total).to eq 1.2 end @@ -1068,6 +1069,7 @@ describe Spree::Order do order.save expect(order.adjustment_total).to eq expected_fees - shipping_fee - payment_fee + expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 0.6 expect(order.shipment.included_tax_total).to eq 0.6 end @@ -1087,6 +1089,7 @@ describe Spree::Order do # Check if fees got updated order.reload expect(order.adjustment_total).to eq expected_fees + expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 1.2 expect(order.shipment.included_tax_total).to eq 1.2 end end @@ -1100,6 +1103,7 @@ describe Spree::Order do order.save expect(order.adjustment_total).to eq expected_fees - (item_num * shipping_fee) + expect(order.shipment.adjustments.tax.inclusive.sum(:amount)).to eq 0 expect(order.shipment.included_tax_total).to eq 0 end end From ac67f7391e06e839554c0709001f9f67494642fb Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 20 Feb 2021 11:53:57 +0000 Subject: [PATCH 29/53] Update tax charging in checkout flow --- app/models/spree/order/checkout.rb | 2 +- spec/models/spree/order/state_machine_spec.rb | 10 ++++------ 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/models/spree/order/checkout.rb b/app/models/spree/order/checkout.rb index 9ffdebe724..123b929851 100644 --- a/app/models/spree/order/checkout.rb +++ b/app/models/spree/order/checkout.rb @@ -77,9 +77,9 @@ module Spree before_transition to: :delivery, do: :create_proposed_shipments before_transition to: :delivery, do: :ensure_available_shipping_rates + before_transition to: :payment, do: :create_tax_charge! after_transition to: :complete, do: :finalize! - after_transition to: :delivery, do: :create_tax_charge! after_transition to: :resumed, do: :after_resume after_transition to: :canceled, do: :after_cancel after_transition to: :payment, do: :charge_shipping_and_payment_fees! diff --git a/spec/models/spree/order/state_machine_spec.rb b/spec/models/spree/order/state_machine_spec.rb index 27d4e48c95..f455c36e72 100644 --- a/spec/models/spree/order/state_machine_spec.rb +++ b/spec/models/spree/order/state_machine_spec.rb @@ -48,16 +48,14 @@ describe Spree::Order do end end - context "when current state is address" do + context "when current state is delivery" do before do allow(order).to receive(:ensure_available_shipping_rates) - order.state = "address" + order.state = "delivery" end - it "adjusts tax rates when transitioning to delivery" do - # Once because the record is being saved - # Twice because it is transitioning to the delivery state - expect(Spree::TaxRate).to receive(:adjust).twice + it "adjusts tax rates when transitioning to payment" do + expect(Spree::TaxRate).to receive(:adjust) order.next! end end From 6d48471368f3ed3f567bee1d1a6c66fb92f21846 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 20 Feb 2021 12:08:05 +0000 Subject: [PATCH 30/53] Update shipment totals updating --- app/models/spree/shipment.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/spree/shipment.rb b/app/models/spree/shipment.rb index 5f3b6c6c55..baf32391b0 100644 --- a/app/models/spree/shipment.rb +++ b/app/models/spree/shipment.rb @@ -165,10 +165,13 @@ module Spree end def update_amounts + return unless fee_adjustment&.amount != cost + update_columns( cost: fee_adjustment&.amount || 0.0, updated_at: Time.zone.now ) + recalculate_adjustments end def manifest @@ -280,7 +283,7 @@ module Spree reload # ensure adjustment is present on later saves end - update_amounts if fee_adjustment&.amount != cost + update_amounts end def adjustment_label From bbd4a33a87359d36fca62eb222be8221db401353 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 20 Feb 2021 12:46:56 +0000 Subject: [PATCH 31/53] Tax adjustments should be open when created All adjustments get closed during `order.finalize!`, but before that point they should be open. --- app/models/spree/tax_rate.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index fb2f84a018..a2368ad08e 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -61,8 +61,8 @@ module Spree label = create_label if included_in_price if default_zone_or_zone_match? order - order.line_items.each { |line_item| create_adjustment(label, line_item, line_item) } - order.shipments.each { |shipment| create_adjustment(label, shipment, shipment) } + order.line_items.each { |line_item| create_adjustment(label, line_item, line_item, false, "open") } + order.shipments.each { |shipment| create_adjustment(label, shipment, shipment, false, "open") } else amount = -1 * calculator.compute(order) label = Spree.t(:refund) + label @@ -77,7 +77,7 @@ module Spree ) end else - create_adjustment(label, order, order) + create_adjustment(label, order, order, false, "open") end order.adjustments.reload From e81270d0d71c87b977dc54d485f9a60dc04d35d3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 20 Feb 2021 12:52:40 +0000 Subject: [PATCH 32/53] Fix adjustment test setups missing order associations All adjustments must be associated with an order --- spec/controllers/spree/admin/orders_controller_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/controllers/spree/admin/orders_controller_spec.rb b/spec/controllers/spree/admin/orders_controller_spec.rb index 58daf89755..409d49fc6b 100644 --- a/spec/controllers/spree/admin/orders_controller_spec.rb +++ b/spec/controllers/spree/admin/orders_controller_spec.rb @@ -25,6 +25,7 @@ describe Spree::Admin::OrdersController, type: :controller do adjustable: order, label: "invalid adjustment", eligible: false, + order: order, amount: 0 ) From 56a01194cdfa06c3d79aa094d95759684f35b1ae Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 20 Feb 2021 13:06:02 +0000 Subject: [PATCH 33/53] Update shipping tax adjustments specs --- spec/models/spree/adjustment_spec.rb | 132 ++++++++++++++++++++------- 1 file changed, 99 insertions(+), 33 deletions(-) diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index f1a580c234..3b37c155e7 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -197,11 +197,17 @@ module Spree end describe "Shipment adjustments" do + let(:zone) { create(:zone_with_member) } + let(:inclusive_tax) { true } + let(:tax_rate) { + create(:tax_rate, included_in_price: inclusive_tax, zone: zone, amount: 0.25) + } + let(:tax_category) { create(:tax_category, name: "Shipping", tax_rates: [tax_rate] ) } let(:hub) { create(:distributor_enterprise, charges_sales_tax: true) } let(:order) { create(:order, distributor: hub) } let(:line_item) { create(:line_item, order: order) } - let(:shipping_method) { create(:shipping_method_with, :flat_rate) } + let(:shipping_method) { create(:shipping_method_with, :flat_rate, tax_category: tax_category) } let(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method, order: order) } describe "the shipping charge" do @@ -212,54 +218,114 @@ module Spree end end - describe "when tax on shipping is disabled" do + context "with tax" do before do - allow(Config).to receive(:shipment_inc_vat).and_return(false) + allow(order).to receive(:tax_zone) { zone } end - it "records 0% tax on shipment adjustments" do - allow(Config).to receive(:shipping_tax_rate).and_return(0) - order.shipments = [shipment] + context "when the shipment has an inclusive tax rate" do + it "calculates the shipment tax from the tax rate" do + order.shipments = [shipment] + order.create_tax_charge! + order.update_totals - expect(order.shipment_adjustments.first.included_tax).to eq(0) + # Finding the tax included in an amount that's already inclusive of tax: + # total - ( total / (1 + rate) ) + # 50 - ( 50 / (1 + 0.25) ) + # = 10 + expect(order.shipment_adjustments.tax.first.amount).to eq(10) + expect(order.shipment_adjustments.tax.first.included).to eq true + + expect(shipment.reload.cost).to eq(50) + expect(shipment.included_tax_total).to eq(10) + expect(shipment.additional_tax_total).to eq(0) + + expect(order.included_tax_total).to eq(10) + expect(order.additional_tax_total).to eq(0) + end end - it "records 0% tax on shipments when a rate is set but shipment_inc_vat is false" do - allow(Config).to receive(:shipping_tax_rate).and_return(0.25) - order.shipments = [shipment] + context "when the shipment has an added tax rate" do + let(:inclusive_tax) { false } - expect(order.shipment_adjustments.first.included_tax).to eq(0) - end - end + # Current behaviour. Will be replaced by the pending test below + it "records the tax on the order's adjustments" do + order.shipments = [shipment] + order.create_tax_charge! + order.update_totals - describe "when tax on shipping is enabled" do - before do - allow(Config).to receive(:shipment_inc_vat).and_return(true) + expect(order.shipment_adjustments.tax.count).to be_zero + + # Finding the added tax for an amount: + # total * rate + # 50 * 0.25 + # = 12.5 + expect(order.adjustments.tax.first.amount).to eq(12.50) + expect(order.adjustments.tax.first.included).to eq false + + expect(shipment.reload.cost).to eq(50) + expect(shipment.included_tax_total).to eq(0) + expect(shipment.additional_tax_total).to eq(0) + + expect(order.included_tax_total).to eq(0) + expect(order.additional_tax_total).to eq(12.50) + end + + xit "records the tax on the shipment's adjustments" do + order.shipments = [shipment] + order.create_tax_charge! + order.update_totals + + # Finding the added tax for an amount: + # total * rate + # 50 * 0.25 + # = 12.5 + expect(order.shipment_adjustments.tax.first.amount).to eq(12.50) + expect(order.shipment_adjustments.tax.first.included).to eq false + + expect(shipment.reload.cost).to eq(50) + expect(shipment.included_tax_total).to eq(0) + expect(shipment.additional_tax_total).to eq(12.50) + + expect(order.included_tax_total).to eq(0) + expect(order.additional_tax_total).to eq(12.50) + end end - it "takes the shipment adjustment tax included from the system setting" do - allow(Config).to receive(:shipping_tax_rate).and_return(0.25) - order.shipments = [shipment] + context "when the distributor does not charge sales tax" do + it "records 0% tax on shipments" do + order.distributor.update! charges_sales_tax: false + order.shipments = [shipment] + order.create_tax_charge! + order.update_totals - # Finding the tax included in an amount that's already inclusive of tax: - # total - ( total / (1 + rate) ) - # 50 - ( 50 / (1 + 0.25) ) - # = 10 - expect(order.shipment_adjustments.first.included_tax).to eq(10.00) + expect(order.shipment_adjustments.tax.count).to be_zero + + expect(shipment.reload.cost).to eq(50) + expect(shipment.included_tax_total).to eq(0) + expect(shipment.additional_tax_total).to eq(0) + + expect(order.included_tax_total).to eq(0) + expect(order.additional_tax_total).to eq(0) + end end - it "records 0% tax on shipments when shipping_tax_rate is not set" do - allow(Config).to receive(:shipping_tax_rate).and_return(0) - order.shipments = [shipment] + context "when the shipment has no applicable tax rate" do + it "records 0% tax on shipments" do + allow(shipment).to receive(:tax_category) { nil } + order.shipments = [shipment] + order.create_tax_charge! + order.update_totals - expect(order.shipment_adjustments.first.included_tax).to eq(0) - end + expect(order.shipment_adjustments.tax.count).to be_zero - it "records 0% tax on shipments when the distributor does not charge sales tax" do - order.distributor.update! charges_sales_tax: false - order.shipments = [shipment] + expect(shipment.reload.cost).to eq(50) + expect(shipment.included_tax_total).to eq(0) + expect(shipment.additional_tax_total).to eq(0) - expect(order.shipment_adjustments.first.included_tax).to eq(0) + expect(order.included_tax_total).to eq(0) + expect(order.additional_tax_total).to eq(0) + end end end end From 63483818d72529ec0a2e3fa917b294097318313e Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 20 Feb 2021 14:32:27 +0000 Subject: [PATCH 34/53] Remove dead code This spec is doing nothing... --- spec/models/spree/order/state_machine_spec.rb | 7 ------- 1 file changed, 7 deletions(-) diff --git a/spec/models/spree/order/state_machine_spec.rb b/spec/models/spree/order/state_machine_spec.rb index f455c36e72..0b37ee41fa 100644 --- a/spec/models/spree/order/state_machine_spec.rb +++ b/spec/models/spree/order/state_machine_spec.rb @@ -59,13 +59,6 @@ describe Spree::Order do order.next! end end - - context "when current state is delivery" do - before do - order.state = "delivery" - allow(order).to receive_messages total: 10.0 - end - end end context "#can_cancel?" do From eb3c2da7404b0e8731fb5da5e38839e6cf5587d3 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 26 Feb 2021 11:34:37 +0000 Subject: [PATCH 35/53] Migrate shipping taxes to adjustments --- .../20210224190247_migrate_shipping_taxes.rb | 83 +++++++++++++++++++ 1 file changed, 83 insertions(+) create mode 100644 db/migrate/20210224190247_migrate_shipping_taxes.rb diff --git a/db/migrate/20210224190247_migrate_shipping_taxes.rb b/db/migrate/20210224190247_migrate_shipping_taxes.rb new file mode 100644 index 0000000000..963cfe6107 --- /dev/null +++ b/db/migrate/20210224190247_migrate_shipping_taxes.rb @@ -0,0 +1,83 @@ +class MigrateShippingTaxes < ActiveRecord::Migration + def up + return unless instance_uses_shipping_tax? + + create_shipping_tax_rates + assign_to_shipping_methods + migrate_tax_amounts_to_adjustments + end + + def instance_uses_shipping_tax? + Spree::Preference.find_by(key: '/spree/app_configuration/shipment_inc_vat').value + end + + def instance_shipping_tax_rate + Spree::Preference.find_by(key: '/spree/app_configuration/shipping_tax_rate').value + end + + def shipping_tax_category + @shipping_tax_category ||= Spree::TaxCategory.create(name: I18n.t(:shipping)) + end + + def create_shipping_tax_rates + # Create a shipping tax rate for each zone, set to current default rate + Spree::Zone.all.each do |tax_zone| + Spree::TaxRate.create!( + name: shipping_rate_label(tax_zone), + zone: tax_zone, + tax_category: shipping_tax_category, + amount: instance_shipping_tax_rate, + included_in_price: true, + calculator: Calculator::DefaultTax.new + ) + end + end + + def assign_to_shipping_methods + # Assign the new default shipping tax category to all existing shipping methods + Spree::ShippingMethod.update_all(tax_category_id: shipping_tax_category.id) + end + + def migrate_tax_amounts_to_adjustments + # Migrate all shipping tax amounts from shipment field to tax adjustments + Spree::Adjustment.shipping.where("included_tax <> 0").includes(:source, :order).find_each do |shipping_fee| + shipment = shipping_fee.source + order = shipping_fee.order + next if order.nil? + + tax_rate = Spree::TaxRate.find_by(tax_category: shipping_tax_category, zone: order.tax_zone) + + # Move all tax totals to adjustments + Spree::Adjustment.create!( + label: tax_adjustment_label(tax_rate), + amount: shipping_fee.included_tax, + included: true, + order_id: order.id, + state: "closed", + adjustable_type: "Spree::Shipment", + adjustable_id: shipment.id, + source_type: "Spree::Shipment", + source_id: shipment.id, + originator_type: "Spree::TaxRate", + originator_id: tax_rate.id + ) + + # Update shipment included tax total + shipment.update_columns( + included_tax_total: shipping_fee.included_tax + ) + end + end + + def shipping_rate_label(zone) + I18n.t(:shipping) + " - #{zone.name.chomp('_VAT')}" + end + + def tax_adjustment_label(tax_rate) + label = "" + label << tax_rate.name + label << " #{tax_rate.amount * 100}%" + label << " (#{I18n.t('models.tax_rate.included_in_price')})" + label + end +end From 3c602cad976aaaa19d8a167bccf8fbe06618b261 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 27 Feb 2021 17:25:47 +0000 Subject: [PATCH 36/53] Add unit tests for migration --- .../migrations/migrate_shipping_taxes_spec.rb | 89 +++++++++++++++++++ 1 file changed, 89 insertions(+) create mode 100644 spec/migrations/migrate_shipping_taxes_spec.rb diff --git a/spec/migrations/migrate_shipping_taxes_spec.rb b/spec/migrations/migrate_shipping_taxes_spec.rb new file mode 100644 index 0000000000..fb625a385f --- /dev/null +++ b/spec/migrations/migrate_shipping_taxes_spec.rb @@ -0,0 +1,89 @@ +# frozen_string_literal: true + +require 'spec_helper' +require_relative '../../db/migrate/20210224190247_migrate_shipping_taxes' + +describe MigrateShippingTaxes do + let(:migration) { MigrateShippingTaxes.new } + + describe '#shipping_tax_category' do + it "creates a new shipping tax category" do + migration.shipping_tax_category + + expect(Spree::TaxCategory.last.name).to eq I18n.t(:shipping) + end + end + + describe '#create_shipping_tax_rates' do + let!(:zone1) { create(:zone_with_member) } + let!(:zone2) { create(:zone_with_member) } + + before do + allow(migration).to receive(:instance_shipping_tax_rate) { 0.25 } + end + + it "creates a shipping tax rate for each zone" do + migration.create_shipping_tax_rates + + tax_rates = Spree::TaxRate.all + + expect(tax_rates.count).to eq 2 + expect(tax_rates.map(&:zone_id).uniq).to eq [zone1.id, zone2.id] + expect(tax_rates.map(&:amount).uniq).to eq [0.25] + end + end + + describe '#assign_to_shipping_methods' do + let!(:shipping_method1) { create(:shipping_method) } + let!(:shipping_method2) { create(:shipping_method) } + let(:shipping_tax_category) { create(:tax_category) } + + before do + allow(migration).to receive(:shipping_tax_category) { shipping_tax_category } + end + + it "assigns the new shipping tax category to all shipping methods" do + migration.assign_to_shipping_methods + + expect(shipping_method1.reload.tax_category).to eq shipping_tax_category + expect(shipping_method2.reload.tax_category).to eq shipping_tax_category + end + end + + describe '#migrate_tax_amounts_to_adjustments' do + let!(:zone) { create(:zone_with_member) } + let!(:shipping_tax_category) { create(:tax_category) } + let!(:shipping_tax_rate) { + create(:tax_rate, zone: zone, tax_category: shipping_tax_category, name: "Shipping Tax Rate") + } + let(:order) { create(:completed_order_with_fees) } + let(:shipment) { order.shipment } + + before do + shipment.adjustments.first.update_columns(included_tax: 0.23) + allow(order).to receive(:tax_zone) { zone } + allow(migration).to receive(:shipping_tax_category) { shipping_tax_category } + end + + it "migrates the shipment's tax to a tax adjustment" do + expect(shipment.adjustments.first.included_tax).to eq 0.23 + expect(shipment.included_tax_total).to be_zero + expect(shipment.adjustments.tax.count).to be_zero + + migration.migrate_tax_amounts_to_adjustments + + expect(shipment.reload.included_tax_total).to eq 0.23 + expect(shipment.adjustments.tax.count).to eq 1 + + shipping_tax_adjustment = shipment.adjustments.tax.first + + expect(shipping_tax_adjustment.amount).to eq 0.23 + expect(shipping_tax_adjustment.originator).to eq shipping_tax_rate + expect(shipping_tax_adjustment.source).to eq shipment + expect(shipping_tax_adjustment.adjustable).to eq shipment + expect(shipping_tax_adjustment.order_id).to eq order.id + expect(shipping_tax_adjustment.included).to eq true + expect(shipping_tax_adjustment.state).to eq "closed" + end + end +end From a26880a3a1f8136b46cb1c98529ca2ea257f1aa2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Mar 2021 17:52:26 +0000 Subject: [PATCH 37/53] Update shipment tax fetching in Xero Invoices report --- lib/open_food_network/xero_invoices_report.rb | 2 +- spec/features/admin/reports_spec.rb | 9 ++------- 2 files changed, 3 insertions(+), 8 deletions(-) diff --git a/lib/open_food_network/xero_invoices_report.rb b/lib/open_food_network/xero_invoices_report.rb index a7394defd6..fbb6f8fd4f 100644 --- a/lib/open_food_network/xero_invoices_report.rb +++ b/lib/open_food_network/xero_invoices_report.rb @@ -204,7 +204,7 @@ module OpenFoodNetwork end def tax_on_shipping_s(order) - tax_on_shipping = order.all_adjustments.shipping.sum(:included_tax) > 0 + tax_on_shipping = order.shipments.sum("additional_tax_total + included_tax_total").positive? tax_on_shipping ? I18n.t(:report_header_gst_on_income) : I18n.t(:report_header_gst_free_income) end diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb index 066592b4d3..2f0dde6e40 100644 --- a/spec/features/admin/reports_spec.rb +++ b/spec/features/admin/reports_spec.rb @@ -387,16 +387,11 @@ feature ' let(:product1) { create(:taxed_product, zone: zone, price: 12.54, tax_rate_amount: 0, sku: 'sku1') } let(:product2) { create(:taxed_product, zone: zone, price: 500.15, tax_rate_amount: 0.2, sku: 'sku2') } - before do - allow(Spree::Config).to receive(:shipment_inc_vat) { true } - allow(Spree::Config).to receive(:shipping_tax_rate) { 0.1 } - end - describe "with adjustments" do let!(:line_item1) { create(:line_item, variant: product1.master, price: 12.54, quantity: 1, order: order1) } let!(:line_item2) { create(:line_item, variant: product2.master, price: 500.15, quantity: 3, order: order1) } - let!(:adj_shipping) { create(:adjustment, order: order1, adjustable: order1, label: "Shipping", originator: shipping_method, amount: 100.55, included_tax: 10.06) } + let!(:adj_shipping) { create(:adjustment, order: order1, adjustable: order1, label: "Shipping", originator: shipping_method, amount: 100.55) } let!(:adj_fee1) { create(:adjustment, order: order1, adjustable: order1, originator: enterprise_fee1, label: "Enterprise fee untaxed", amount: 10, included_tax: 0) } let!(:adj_fee2) { create(:adjustment, order: order1, adjustable: order1, originator: enterprise_fee2, label: "Enterprise fee taxed", amount: 20, included_tax: 2) } let!(:adj_manual1) { create(:adjustment, order: order1, adjustable: order1, originator: nil, source: nil, label: "Manual adjustment", amount: 30, included_tax: 0) } @@ -404,8 +399,8 @@ feature ' before do order1.update_attribute :email, 'customer@email.com' + order1.shipment.update_columns(included_tax_total: 10.06) Timecop.travel(Time.zone.local(2015, 4, 25, 14, 0, 0)) { order1.finalize! } - login_as_admin_and_visit spree.admin_reports_path click_link 'Xero Invoices' From 903788903b1381d345fdee6771acab1e6ec724d2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sat, 6 Mar 2021 17:58:35 +0000 Subject: [PATCH 38/53] Remove Spree::Config[:shipping_tax_rate] --- app/controllers/spree/admin/tax_settings_controller.rb | 3 +-- app/models/spree/app_configuration.rb | 1 - app/views/spree/admin/tax_settings/edit.html.haml | 4 ---- spec/controllers/spree/admin/tax_settings_controller_spec.rb | 3 --- spec/features/admin/tax_settings_spec.rb | 4 ---- 5 files changed, 1 insertion(+), 14 deletions(-) diff --git a/app/controllers/spree/admin/tax_settings_controller.rb b/app/controllers/spree/admin/tax_settings_controller.rb index 564a090da4..ffc0a72b36 100644 --- a/app/controllers/spree/admin/tax_settings_controller.rb +++ b/app/controllers/spree/admin/tax_settings_controller.rb @@ -16,8 +16,7 @@ module Spree def preferences_params params.require(:preferences).permit( :products_require_tax_category, - :shipment_inc_vat, - :shipping_tax_rate, + :shipment_inc_vat ) end end diff --git a/app/models/spree/app_configuration.rb b/app/models/spree/app_configuration.rb index d30f0e6215..53ae4bd5ab 100644 --- a/app/models/spree/app_configuration.rb +++ b/app/models/spree/app_configuration.rb @@ -118,7 +118,6 @@ module Spree # Tax Preferences preference :products_require_tax_category, :boolean, default: false - preference :shipping_tax_rate, :decimal, default: 0 # Monitoring preference :last_job_queue_heartbeat_at, :string, default: nil diff --git a/app/views/spree/admin/tax_settings/edit.html.haml b/app/views/spree/admin/tax_settings/edit.html.haml index edd5a3b401..8d7afa0027 100644 --- a/app/views/spree/admin/tax_settings/edit.html.haml +++ b/app/views/spree/admin/tax_settings/edit.html.haml @@ -15,9 +15,5 @@ = check_box_tag 'preferences[shipment_inc_vat]', '1', Spree::Config[:shipment_inc_vat] = label_tag nil, t(:shipment_inc_vat) - .field.align-center{ "data-hook" => "shipping_tax_rate" } - = number_field_tag "preferences[shipping_tax_rate]", Spree::Config[:shipping_tax_rate].to_f, in: 0.0..1.0, step: 0.01 - = label_tag nil, t(:shipping_tax_rate) - .form-buttons{"data-hook" => "buttons"} = button t(:update), 'icon-refresh' diff --git a/spec/controllers/spree/admin/tax_settings_controller_spec.rb b/spec/controllers/spree/admin/tax_settings_controller_spec.rb index 7b64d4e2cd..134eb43b2a 100644 --- a/spec/controllers/spree/admin/tax_settings_controller_spec.rb +++ b/spec/controllers/spree/admin/tax_settings_controller_spec.rb @@ -9,7 +9,6 @@ describe Spree::Admin::TaxSettingsController, type: :controller do preferences: { products_require_tax_category: "1", shipment_inc_vat: "0", - shipping_tax_rate: "0.1", } } } @@ -25,13 +24,11 @@ describe Spree::Admin::TaxSettingsController, type: :controller do [ Spree::Config[:products_require_tax_category], Spree::Config[:shipment_inc_vat], - Spree::Config[:shipping_tax_rate], ] }.to( [ true, false, - 0.1, ] ) end diff --git a/spec/features/admin/tax_settings_spec.rb b/spec/features/admin/tax_settings_spec.rb index bbe8155bbf..43047bcdc2 100644 --- a/spec/features/admin/tax_settings_spec.rb +++ b/spec/features/admin/tax_settings_spec.rb @@ -11,7 +11,6 @@ feature 'Account and Billing Settings' do Spree::Config.set( products_require_tax_category: false, shipment_inc_vat: false, - shipping_tax_rate: 0 ) end @@ -22,7 +21,6 @@ feature 'Account and Billing Settings' do expect(page).to have_unchecked_field 'preferences_products_require_tax_category' expect(page).to have_unchecked_field 'preferences_shipment_inc_vat' - expect(page).to have_field 'preferences_shipping_tax_rate' end it "attributes can be changed" do @@ -30,13 +28,11 @@ feature 'Account and Billing Settings' do check 'preferences_products_require_tax_category' check 'preferences_shipment_inc_vat' - fill_in 'preferences_shipping_tax_rate', with: '0.12' click_button "Update" expect(Spree::Config.products_require_tax_category).to be true expect(Spree::Config.shipment_inc_vat).to be true - expect(Spree::Config.shipping_tax_rate).to eq 0.12 end end end From fec2b0b7c15e1891ffc697d86b19b6b4457fbd69 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 17 Mar 2021 16:48:38 +0000 Subject: [PATCH 39/53] Define models in migration --- .../20210224190247_migrate_shipping_taxes.rb | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/db/migrate/20210224190247_migrate_shipping_taxes.rb b/db/migrate/20210224190247_migrate_shipping_taxes.rb index 963cfe6107..f305de7862 100644 --- a/db/migrate/20210224190247_migrate_shipping_taxes.rb +++ b/db/migrate/20210224190247_migrate_shipping_taxes.rb @@ -1,4 +1,21 @@ class MigrateShippingTaxes < ActiveRecord::Migration + class Spree::Preference < ActiveRecord::Base; end + class Spree::TaxCategory < ActiveRecord::Base; end + class Spree::ShippingMethod < ActiveRecord::Base; end + class Spree::Zone < ActiveRecord::Base; end + class Spree::TaxRate < ActiveRecord::Base + belongs_to :zone, class_name: "Spree::Zone", inverse_of: :tax_rates + belongs_to :tax_category, class_name: "Spree::TaxCategory", inverse_of: :tax_rates + has_one :calculator, class_name: "Spree::Calculator", as: :calculable, dependent: :destroy + accepts_nested_attributes_for :calculator + end + class Spree::Adjustment < ActiveRecord::Base + belongs_to :adjustable, polymorphic: true + belongs_to :originator, polymorphic: true + belongs_to :source, polymorphic: true + belongs_to :order, class_name: "Spree::Order" + end + def up return unless instance_uses_shipping_tax? From fcdc627ce360879435207aa6d81cbaeeb6fe3c6b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 18 Mar 2021 17:01:04 +0000 Subject: [PATCH 40/53] Add fallbacks in migration for migrating locally with RAILS_ENV=test Instances have these preference values set, but when running this migration locally with RAILS_ENV=test, the preference does not exist in the database. --- db/migrate/20210224190247_migrate_shipping_taxes.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/db/migrate/20210224190247_migrate_shipping_taxes.rb b/db/migrate/20210224190247_migrate_shipping_taxes.rb index f305de7862..84721310e7 100644 --- a/db/migrate/20210224190247_migrate_shipping_taxes.rb +++ b/db/migrate/20210224190247_migrate_shipping_taxes.rb @@ -25,11 +25,11 @@ class MigrateShippingTaxes < ActiveRecord::Migration end def instance_uses_shipping_tax? - Spree::Preference.find_by(key: '/spree/app_configuration/shipment_inc_vat').value + Spree::Preference.find_by(key: '/spree/app_configuration/shipment_inc_vat')&.value || false end def instance_shipping_tax_rate - Spree::Preference.find_by(key: '/spree/app_configuration/shipping_tax_rate').value + Spree::Preference.find_by(key: '/spree/app_configuration/shipping_tax_rate')&.value || 0.0 end def shipping_tax_category From 96d8de35f27953a3f3342a00b3d150da1702d888 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 19 Mar 2021 15:07:25 +0000 Subject: [PATCH 41/53] Delete dead code Adjustment#set_included_tax! --- app/models/spree/adjustment.rb | 5 ----- spec/models/spree/adjustment_spec.rb | 9 --------- 2 files changed, 14 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 03d147f0b7..5023639369 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -126,11 +126,6 @@ module Spree state != "open" end - def set_included_tax!(rate) - tax = amount - (amount / (1 + rate)) - set_absolute_included_tax! tax - end - def set_absolute_included_tax!(tax) # This rubocop issue can now fixed by renaming Adjustment#update! to something else, # then AR's update! can be used instead of update_attributes! diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 3b37c155e7..0dcd24afa4 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -483,15 +483,6 @@ module Spree end end end - - describe "setting the included tax by tax rate" do - let(:adjustment) { Adjustment.new label: 'foo', amount: 50 } - - it "sets it, rounding to two decimal places" do - adjustment.set_included_tax! 0.25 - expect(adjustment.included_tax).to eq(10.00) - end - end end context "extends LocalizedNumber" do From 821be3eef50eaaee260ea2e8c6aeb0601b2b508d Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 22 Mar 2021 13:43:50 +0000 Subject: [PATCH 42/53] Update combined queries to use #or --- app/services/order_tax_adjustments_fetcher.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 103953947b..50eae0a89a 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -22,9 +22,9 @@ class OrderTaxAdjustmentsFetcher def all tax_adjustments = order.all_adjustments.tax enterprise_fees_with_tax = order.all_adjustments.enterprise_fee.with_tax - admin_adjustments_with_tax = order.adjustments.admin.with_tax + admin_adjustments_with_tax = order.all_adjustments.admin.with_tax - tax_adjustments | enterprise_fees_with_tax | admin_adjustments_with_tax + tax_adjustments.or(enterprise_fees_with_tax).or(admin_adjustments_with_tax) end def table From e4421876723971f791c036e2ac53b630c53135a7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 22 Mar 2021 13:44:30 +0000 Subject: [PATCH 43/53] Delete unused private method OrderTaxAdjustmentsFetcher#table --- app/services/order_tax_adjustments_fetcher.rb | 4 ---- 1 file changed, 4 deletions(-) diff --git a/app/services/order_tax_adjustments_fetcher.rb b/app/services/order_tax_adjustments_fetcher.rb index 50eae0a89a..06190e4c30 100644 --- a/app/services/order_tax_adjustments_fetcher.rb +++ b/app/services/order_tax_adjustments_fetcher.rb @@ -27,10 +27,6 @@ class OrderTaxAdjustmentsFetcher tax_adjustments.or(enterprise_fees_with_tax).or(admin_adjustments_with_tax) end - def table - @table ||= Spree::Adjustment.arel_table - end - def tax_rates_hash(adjustment) tax_rates = TaxRateFinder.tax_rates_of(adjustment) From 4fbbbbca76866faeb6534f41a63915b576d45ab0 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 29 Mar 2021 13:26:59 +0100 Subject: [PATCH 44/53] Load relevant tax rates in a single query outside of find_each block --- db/migrate/20210224190247_migrate_shipping_taxes.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/db/migrate/20210224190247_migrate_shipping_taxes.rb b/db/migrate/20210224190247_migrate_shipping_taxes.rb index 84721310e7..992f330b89 100644 --- a/db/migrate/20210224190247_migrate_shipping_taxes.rb +++ b/db/migrate/20210224190247_migrate_shipping_taxes.rb @@ -56,13 +56,15 @@ class MigrateShippingTaxes < ActiveRecord::Migration end def migrate_tax_amounts_to_adjustments + shipping_tax_rates = Spree::TaxRate.where(tax_category: shipping_tax_category).to_a + # Migrate all shipping tax amounts from shipment field to tax adjustments Spree::Adjustment.shipping.where("included_tax <> 0").includes(:source, :order).find_each do |shipping_fee| shipment = shipping_fee.source order = shipping_fee.order next if order.nil? - tax_rate = Spree::TaxRate.find_by(tax_category: shipping_tax_category, zone: order.tax_zone) + tax_rate = shipping_tax_rates.detect{ |rate| rate.zone == order.tax_zone } # Move all tax totals to adjustments Spree::Adjustment.create!( From d00bdbe1b8caa55a0259269b9ecc8fbefd89e43a Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 29 Mar 2021 13:42:24 +0100 Subject: [PATCH 45/53] Add index on spree_shipping_methods.tax_category_id --- ...10329123820_add_index_on_shipping_methods_tax_category.rb | 5 +++++ db/schema.rb | 3 ++- 2 files changed, 7 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20210329123820_add_index_on_shipping_methods_tax_category.rb diff --git a/db/migrate/20210329123820_add_index_on_shipping_methods_tax_category.rb b/db/migrate/20210329123820_add_index_on_shipping_methods_tax_category.rb new file mode 100644 index 0000000000..dcc1103fdc --- /dev/null +++ b/db/migrate/20210329123820_add_index_on_shipping_methods_tax_category.rb @@ -0,0 +1,5 @@ +class AddIndexOnShippingMethodsTaxCategory < ActiveRecord::Migration[5.0] + def change + add_index :spree_shipping_methods, :tax_category_id + end +end diff --git a/db/schema.rb b/db/schema.rb index 1ed302c3cf..454f7688d3 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.define(version: 20210320003951) do +ActiveRecord::Schema.define(version: 20210329123820) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -811,6 +811,7 @@ ActiveRecord::Schema.define(version: 20210320003951) do t.text "description" t.string "tracking_url", limit: 255 t.integer "tax_category_id" + t.index ["tax_category_id"], name: "index_spree_shipping_methods_on_tax_category_id", using: :btree end create_table "spree_shipping_methods_zones", id: false, force: :cascade do |t| From 595389cbeade3466b90725bf801317ad651aa04b Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 29 Mar 2021 14:23:29 +0100 Subject: [PATCH 46/53] Update stripe spec The hard-coded order total here was incorrect. It's now correctly including the $3 shipping fee used with that order factory, and the total is actually $13. --- spec/models/spree/gateway/stripe_sca_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/spree/gateway/stripe_sca_spec.rb b/spec/models/spree/gateway/stripe_sca_spec.rb index e62293ae12..43844a3763 100644 --- a/spec/models/spree/gateway/stripe_sca_spec.rb +++ b/spec/models/spree/gateway/stripe_sca_spec.rb @@ -32,7 +32,7 @@ describe Spree::Gateway::StripeSCA, type: :model do stub_request(:get, "https://api.stripe.com/v1/payment_intents/12345"). to_return(status: 200, body: payment_authorised) stub_request(:post, "https://api.stripe.com/v1/payment_intents/12345/capture"). - with(body: {"amount_to_capture" => "10.0"}). + with(body: {"amount_to_capture" => order.total}). to_return(status: 200, body: capture_successful) response = subject.purchase(order.total, credit_card, gateway_options) From ea3044f47acd4eee71e440e3b46def0869649ad1 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 29 Mar 2021 16:42:20 +0100 Subject: [PATCH 47/53] Bring in more explicit model code now that good_migrations disables all access to models --- .../20210224190247_migrate_shipping_taxes.rb | 66 ++++++++++++++++++- 1 file changed, 64 insertions(+), 2 deletions(-) diff --git a/db/migrate/20210224190247_migrate_shipping_taxes.rb b/db/migrate/20210224190247_migrate_shipping_taxes.rb index 992f330b89..cf536f9cf2 100644 --- a/db/migrate/20210224190247_migrate_shipping_taxes.rb +++ b/db/migrate/20210224190247_migrate_shipping_taxes.rb @@ -1,8 +1,68 @@ class MigrateShippingTaxes < ActiveRecord::Migration class Spree::Preference < ActiveRecord::Base; end - class Spree::TaxCategory < ActiveRecord::Base; end + class Spree::TaxCategory < ActiveRecord::Base + has_many :tax_rates, class_name: "Spree::TaxRate", inverse_of: :tax_category + end + class Spree::Order < ActiveRecord::Base + has_many :adjustments, as: :adjustable + belongs_to :bill_address, foreign_key: :bill_address_id, class_name: 'Spree::Address' + belongs_to :ship_address, foreign_key: :ship_address_id, class_name: 'Spree::Address' + + def tax_zone + Spree::Zone.match(tax_address) || Spree::Zone.default_tax + end + + def tax_address + Spree::Config[:tax_using_ship_address] ? ship_address : bill_address + end + end + class Spree::Address < ActiveRecord::Base; end + class Spree::Shipment < ActiveRecord::Base + has_many :adjustments, as: :adjustable + end class Spree::ShippingMethod < ActiveRecord::Base; end - class Spree::Zone < ActiveRecord::Base; end + class Spree::Zone < ActiveRecord::Base + has_many :zone_members, dependent: :destroy, class_name: "Spree::ZoneMember", inverse_of: :zone + has_many :tax_rates, class_name: "Spree::TaxRate", inverse_of: :zone + + def self.match(address) + return unless (matches = includes(:zone_members). + order('zone_members_count', 'created_at'). + select { |zone| zone.include? address }) + + ['state', 'country'].each do |zone_kind| + if (match = matches.detect { |zone| zone_kind == zone.kind }) + return match + end + end + matches.first + end + + def kind + return unless zone_members.any? && zone_members.none? { |member| member.try(:zoneable_type).nil? } + + zone_members.last.zoneable_type.demodulize.underscore + end + + def include?(address) + return false unless address + + zone_members.any? do |zone_member| + case zone_member.zoneable_type + when 'Spree::Country' + zone_member.zoneable_id == address.country_id + when 'Spree::State' + zone_member.zoneable_id == address.state_id + else + false + end + end + end + end + class Spree::ZoneMember < ActiveRecord::Base + belongs_to :zone, class_name: 'Spree::Zone', inverse_of: :zone_members + belongs_to :zoneable, polymorphic: true + end class Spree::TaxRate < ActiveRecord::Base belongs_to :zone, class_name: "Spree::Zone", inverse_of: :tax_rates belongs_to :tax_category, class_name: "Spree::TaxCategory", inverse_of: :tax_rates @@ -14,6 +74,8 @@ class MigrateShippingTaxes < ActiveRecord::Migration belongs_to :originator, polymorphic: true belongs_to :source, polymorphic: true belongs_to :order, class_name: "Spree::Order" + + scope :shipping, -> { where(originator_type: 'Spree::ShippingMethod') } end def up From 76cf239623bdd92fffd78f1649abbfd66bcc58bb Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Mon, 29 Mar 2021 16:49:47 +0100 Subject: [PATCH 48/53] Sort ids in test in case ordering is reversed --- spec/migrations/migrate_shipping_taxes_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/migrations/migrate_shipping_taxes_spec.rb b/spec/migrations/migrate_shipping_taxes_spec.rb index fb625a385f..218ff8ccc5 100644 --- a/spec/migrations/migrate_shipping_taxes_spec.rb +++ b/spec/migrations/migrate_shipping_taxes_spec.rb @@ -28,7 +28,7 @@ describe MigrateShippingTaxes do tax_rates = Spree::TaxRate.all expect(tax_rates.count).to eq 2 - expect(tax_rates.map(&:zone_id).uniq).to eq [zone1.id, zone2.id] + expect(tax_rates.map(&:zone_id).uniq.sort).to eq [zone1.id, zone2.id] expect(tax_rates.map(&:amount).uniq).to eq [0.25] end end From a2f6ff7b39f93f87cfd3139c2a319f9616d50aa7 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Thu, 1 Apr 2021 12:41:58 +0100 Subject: [PATCH 49/53] Update deprecated params in checkout spec --- spec/requests/checkout/failed_checkout_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/requests/checkout/failed_checkout_spec.rb b/spec/requests/checkout/failed_checkout_spec.rb index efd44cbb31..9dade977c1 100644 --- a/spec/requests/checkout/failed_checkout_spec.rb +++ b/spec/requests/checkout/failed_checkout_spec.rb @@ -16,7 +16,7 @@ describe "checking out an order that initially fails", type: :request do let!(:shipment) { create(:shipment_with, :shipping_method, shipping_method: shipping_method) } let!(:order) { create(:order, shipments: [shipment], distributor: shop, order_cycle: order_cycle) } let(:params) do - { format: :json, order: { + { order: { shipping_method_id: shipping_method.id, payments_attributes: [{ payment_method_id: payment_method.id }], bill_address_attributes: address.attributes.slice("firstname", "lastname", "address1", "address2", "phone", "city", "zipcode", "state_id", "country_id"), @@ -46,7 +46,7 @@ describe "checking out an order that initially fails", type: :request do end it "clears shipments and payments before rendering the checkout" do - put update_checkout_path, params + put update_checkout_path, params: params, as: :json # Checking out a BogusGateway without a source fails at :payment # Shipments and payments should then be cleared before rendering checkout @@ -62,7 +62,8 @@ describe "checking out an order that initially fails", type: :request do # Use a check payment method, which should work params[:order][:payments_attributes][0][:payment_method_id] = check_payment_method.id - put update_checkout_path, params + + put update_checkout_path, params: params, as: :json expect(response.status).to be 200 order.reload From 30923973462373eb14eb65f3e94c2152a906a575 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 4 Apr 2021 16:35:03 +0100 Subject: [PATCH 50/53] Isolate migration tests --- .github/workflows/build.yml | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index cd38d3c590..19f799186f 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -272,8 +272,11 @@ jobs: - name: Run JS tests run: RAILS_ENV=test bundle exec rake karma:run + - name: Run migration tests + run: bundle exec rspec --pattern "spec/{migrations}/**/*_spec.rb" + - name: Run all other tests - run: bundle exec rake ofn:specs:run:excluding_folders["models,controllers,serializers,features,lib"] + run: bundle exec rake ofn:specs:run:excluding_folders["models,controllers,serializers,features,lib,migrations"] test-the-rest: runs-on: ubuntu-18.04 From c702b398d61e05f28cc2b09303a70a00ef8521bf Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 4 Apr 2021 17:01:10 +0100 Subject: [PATCH 51/53] Add warning comment on migration test isolation --- .github/workflows/build.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 19f799186f..0059ce138d 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -272,6 +272,8 @@ jobs: - name: Run JS tests run: RAILS_ENV=test bundle exec rake karma:run + # Migration tests need to be run in a separate task. + # See: https://github.com/openfoodfoundation/openfoodnetwork/pull/6924#issuecomment-813056525 - name: Run migration tests run: bundle exec rspec --pattern "spec/{migrations}/**/*_spec.rb" From 01e6397e2703aadd3777f9cd2be080b0b8c0ac52 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 6 Apr 2021 20:08:43 +0100 Subject: [PATCH 52/53] Remove Spree::Config[:shipment_inc_vat] This is now done per ShippingMethod instead of globally --- .../spree/admin/tax_settings_controller.rb | 5 +---- app/models/spree/app_configuration.rb | 1 - app/models/spree/shipping_rate.rb | 8 +------ .../spree/admin/tax_settings/edit.html.haml | 5 ----- .../admin/tax_settings_controller_spec.rb | 21 ++----------------- spec/features/admin/tax_settings_spec.rb | 8 +------ spec/models/spree/shipping_rate_spec.rb | 15 ++----------- 7 files changed, 7 insertions(+), 56 deletions(-) diff --git a/app/controllers/spree/admin/tax_settings_controller.rb b/app/controllers/spree/admin/tax_settings_controller.rb index ffc0a72b36..7ebeec543b 100644 --- a/app/controllers/spree/admin/tax_settings_controller.rb +++ b/app/controllers/spree/admin/tax_settings_controller.rb @@ -14,10 +14,7 @@ module Spree private def preferences_params - params.require(:preferences).permit( - :products_require_tax_category, - :shipment_inc_vat - ) + params.require(:preferences).permit(:products_require_tax_category) end end end diff --git a/app/models/spree/app_configuration.rb b/app/models/spree/app_configuration.rb index 53ae4bd5ab..c93488105f 100644 --- a/app/models/spree/app_configuration.rb +++ b/app/models/spree/app_configuration.rb @@ -58,7 +58,6 @@ module Spree preference :products_per_page, :integer, default: 12 preference :redirect_https_to_http, :boolean, default: false preference :require_master_price, :boolean, default: true - preference :shipment_inc_vat, :boolean, default: false # Request instructions/info for shipping preference :shipping_instructions, :boolean, default: false # Displays variant full price or difference with product price. diff --git a/app/models/spree/shipping_rate.rb b/app/models/spree/shipping_rate.rb index 857847b398..31fcec4c05 100644 --- a/app/models/spree/shipping_rate.rb +++ b/app/models/spree/shipping_rate.rb @@ -24,13 +24,7 @@ module Spree delegate :name, to: :shipping_method def display_price - price = if Spree::Config[:shipment_inc_vat] - (1 + Spree::TaxRate.default) * cost - else - cost - end - - Spree::Money.new(price, { currency: currency }) + Spree::Money.new(cost, { currency: currency }) end alias_method :display_cost, :display_price diff --git a/app/views/spree/admin/tax_settings/edit.html.haml b/app/views/spree/admin/tax_settings/edit.html.haml index 8d7afa0027..7ad2a3cdc7 100644 --- a/app/views/spree/admin/tax_settings/edit.html.haml +++ b/app/views/spree/admin/tax_settings/edit.html.haml @@ -10,10 +10,5 @@ = check_box_tag 'preferences[products_require_tax_category]', '1', Spree::Config[:products_require_tax_category] = label_tag nil, t(:products_require_tax_category) - .field.align-center{"data-hook" => "shipment_vat"} - = hidden_field_tag 'preferences[shipment_inc_vat]', '0' - = check_box_tag 'preferences[shipment_inc_vat]', '1', Spree::Config[:shipment_inc_vat] - = label_tag nil, t(:shipment_inc_vat) - .form-buttons{"data-hook" => "buttons"} = button t(:update), 'icon-refresh' diff --git a/spec/controllers/spree/admin/tax_settings_controller_spec.rb b/spec/controllers/spree/admin/tax_settings_controller_spec.rb index 134eb43b2a..dbb320b1c6 100644 --- a/spec/controllers/spree/admin/tax_settings_controller_spec.rb +++ b/spec/controllers/spree/admin/tax_settings_controller_spec.rb @@ -4,14 +4,7 @@ require 'spec_helper' describe Spree::Admin::TaxSettingsController, type: :controller do describe "#update" do - let(:params) { - { - preferences: { - products_require_tax_category: "1", - shipment_inc_vat: "0", - } - } - } + let(:params) { { preferences: { products_require_tax_category: "1" } } } before do allow(controller).to receive(:spree_current_user) { create(:admin_user) } @@ -20,17 +13,7 @@ describe Spree::Admin::TaxSettingsController, type: :controller do it "changes Tax settings" do expect { spree_post :update, params - }.to change { - [ - Spree::Config[:products_require_tax_category], - Spree::Config[:shipment_inc_vat], - ] - }.to( - [ - true, - false, - ] - ) + }.to change { Spree::Config[:products_require_tax_category] }.to(true) end end end diff --git a/spec/features/admin/tax_settings_spec.rb b/spec/features/admin/tax_settings_spec.rb index 43047bcdc2..e15c49b0df 100644 --- a/spec/features/admin/tax_settings_spec.rb +++ b/spec/features/admin/tax_settings_spec.rb @@ -8,10 +8,7 @@ feature 'Account and Billing Settings' do describe "updating" do before do - Spree::Config.set( - products_require_tax_category: false, - shipment_inc_vat: false, - ) + Spree::Config.set(products_require_tax_category: false) end context "as an admin user" do @@ -20,19 +17,16 @@ feature 'Account and Billing Settings' do click_link "Tax Settings" expect(page).to have_unchecked_field 'preferences_products_require_tax_category' - expect(page).to have_unchecked_field 'preferences_shipment_inc_vat' end it "attributes can be changed" do login_as_admin_and_visit spree.edit_admin_tax_settings_path check 'preferences_products_require_tax_category' - check 'preferences_shipment_inc_vat' click_button "Update" expect(Spree::Config.products_require_tax_category).to be true - expect(Spree::Config.shipment_inc_vat).to be true end end end diff --git a/spec/models/spree/shipping_rate_spec.rb b/spec/models/spree/shipping_rate_spec.rb index 03871722fe..d071b15c3c 100644 --- a/spec/models/spree/shipping_rate_spec.rb +++ b/spec/models/spree/shipping_rate_spec.rb @@ -10,21 +10,10 @@ describe Spree::ShippingRate do shipping_method: shipping_method, cost: 10.55) } - before { allow(Spree::TaxRate).to receive_messages(default: 0.05) } context "#display_price" do - context "when shipment includes VAT" do - before { Spree::Config[:shipment_inc_vat] = true } - it "displays the correct price" do - expect(shipping_rate.display_price.to_s).to eq "$11.08" # $10.55 * 1.05 == $11.08 - end - end - - context "when shipment does not include VAT" do - before { Spree::Config[:shipment_inc_vat] = false } - it "displays the correct price" do - expect(shipping_rate.display_price.to_s).to eq "$10.55" - end + it "displays the shipping price" do + expect(shipping_rate.display_price.to_s).to eq "$10.55" end context "when the currency is JPY" do From 4a65e5817f49fcc2ecc169cded90f1623bdf0859 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Tue, 6 Apr 2021 20:17:19 +0100 Subject: [PATCH 53/53] Add fieldset title to shipping methods edit form --- app/views/spree/admin/shipping_methods/_form.html.haml | 1 + config/locales/en.yml | 1 + 2 files changed, 2 insertions(+) diff --git a/app/views/spree/admin/shipping_methods/_form.html.haml b/app/views/spree/admin/shipping_methods/_form.html.haml index 934e808a7a..6bba6e37ad 100644 --- a/app/views/spree/admin/shipping_methods/_form.html.haml +++ b/app/views/spree/admin/shipping_methods/_form.html.haml @@ -47,6 +47,7 @@ = render partial: 'spree/admin/shared/calculator_fields', locals: { f: f } %fieldset.tax_categories.no-border-bottom + %legend{align: "center"}= t('.tax_category') = f.field_container :tax_categories do = f.select :tax_category_id, @tax_categories.map { |tc| [tc.name, tc.id] }, {}, :class => "select2 fullwidth" = error_message_on :shipping_method, :tax_category_id diff --git a/config/locales/en.yml b/config/locales/en.yml index 0e4c11fd9a..de329cf871 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -3525,6 +3525,7 @@ See the %{link} to find out more about %{sitename}'s features and to start using back_to_shipping_methods_list: "Back To Shipping Methods List" form: categories: "Categories" + tax_category: "Tax Category" zones: "Zones" both: "Both Checkout and Back office" back_end: "Back office only"