From 66bf69b52c562ae228a81a714b77175398a713e4 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 27 Dec 2020 12:26:10 +0000 Subject: [PATCH 1/9] Add included column to spree_adjustments table --- ...01227122327_add_included_to_adjustments.rb | 20 +++++++++++++++++++ db/schema.rb | 7 ++++--- 2 files changed, 24 insertions(+), 3 deletions(-) create mode 100644 db/migrate/20201227122327_add_included_to_adjustments.rb diff --git a/db/migrate/20201227122327_add_included_to_adjustments.rb b/db/migrate/20201227122327_add_included_to_adjustments.rb new file mode 100644 index 0000000000..2aebc93706 --- /dev/null +++ b/db/migrate/20201227122327_add_included_to_adjustments.rb @@ -0,0 +1,20 @@ +class AddIncludedToAdjustments < ActiveRecord::Migration + class Spree::Adjustment < ActiveRecord::Base + belongs_to :originator, polymorphic: true + end + + def up + add_column :spree_adjustments, :included, :boolean, default: false + Spree::Adjustment.reset_column_information + + inclusive_tax_rates = Spree::TaxRate.where(included_in_price: true) + + # Set included boolean to true on all adjustments based on price-inclusive tax rates + Spree::Adjustment.where(originator_type: 'Spree::TaxRate', originator_id: inclusive_tax_rates). + update_all(included: true) + end + + def down + remove_column :spree_adjustments, :included + end +end diff --git a/db/schema.rb b/db/schema.rb index fc360b11e1..a0417bd885 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -385,16 +385,17 @@ ActiveRecord::Schema.define(version: 20210115143738) do t.string "label", limit: 255 t.string "source_type", limit: 255 t.integer "adjustable_id" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false t.boolean "mandatory" t.integer "originator_id" t.string "originator_type", limit: 255 t.boolean "eligible", default: true t.string "adjustable_type", limit: 255 - t.decimal "included_tax", precision: 10, scale: 2, default: 0.0, null: false + t.decimal "included_tax", precision: 10, scale: 2, default: 0.0, null: false t.string "state", limit: 255 t.integer "order_id" + t.boolean "included", default: false end add_index "spree_adjustments", ["adjustable_id"], name: "index_adjustments_on_order_id", using: :btree From 8be18cd05c473207fc9567faec4efd6819e5910c Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 27 Dec 2020 12:32:09 +0000 Subject: [PATCH 2/9] Add #included and #additional scopes to Spree::Adjustment We can now do things like: ``` included_tax = order.adjustments.tax.included.sum(:amount) additional_tax = order.adjustments.tax.additional.sum(:amount) ``` --- app/models/spree/adjustment.rb | 2 ++ spec/models/spree/adjustment_spec.rb | 29 ++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 942e7fc149..f5e5fe0707 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -70,6 +70,8 @@ module Spree scope :charge, -> { where('amount >= 0') } scope :credit, -> { where('amount < 0') } scope :return_authorization, -> { where(source_type: "Spree::ReturnAuthorization") } + scope :included, -> { where(included: true) } + scope :additional, -> { where(included: false) } scope :enterprise_fee, -> { where(originator_type: 'EnterpriseFee') } scope :admin, -> { where(source_type: nil, originator_type: nil) } diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index 1f77bd725b..cced77f9e8 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -460,5 +460,34 @@ module Spree context "extends LocalizedNumber" do it_behaves_like "a model using the LocalizedNumber module", [:amount] end + + describe "included and additional scopes" do + let!(:zone) { create(:zone_with_member) } + let(:order) { create(:order) } + let(:included_in_price) { true } + let(:tax_rate) { + create(:tax_rate, included_in_price: included_in_price, + calculator: ::Calculator::FlatRate.new(preferred_amount: 0.1)) + } + let(:included) { true } + let(:adjustment) { + create(:adjustment, adjustable: order, source: order, + originator: tax_rate, included: included) + } + + context "when tax is included in price" do + it "is returned by the #included scope" do + expect(Spree::Adjustment.included).to eq [adjustment] + end + end + + context "when tax is additional to the price" do + let(:included) { false } + + it "is returned by the #additional scope" do + expect(Spree::Adjustment.additional).to eq [adjustment] + end + end + end end end From a328a9bc1b1f902cc09f88201dbe1d89f84d44b4 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Sun, 27 Dec 2020 12:15:35 +0000 Subject: [PATCH 3/9] Use included boolean in tax adjustments --- app/models/spree/tax_rate.rb | 8 +++++++- config/locales/en.yml | 2 ++ lib/spree/core/calculated_adjustments.rb | 12 +++++++++++- 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/app/models/spree/tax_rate.rb b/app/models/spree/tax_rate.rb index 064baf8852..50abeeb021 100644 --- a/app/models/spree/tax_rate.rb +++ b/app/models/spree/tax_rate.rb @@ -61,7 +61,7 @@ module Spree def adjust(order) label = create_label if included_in_price - if Zone.default_tax.contains? order.tax_zone + if default_zone_or_zone_match? order order.line_items.each { |line_item| create_adjustment(label, line_item, line_item) } else amount = -1 * calculator.compute(order) @@ -89,6 +89,10 @@ module Spree end end + def default_zone_or_zone_match?(order) + Zone.default_tax.contains?(order.tax_zone) || order.tax_zone == zone + end + # Manually apply a TaxRate to a particular amount. TaxRates normally compute against # LineItems or Orders, so we mock out a line item here to fit the interface # that our calculator (usually DefaultTax) expects. @@ -114,6 +118,8 @@ module Spree label = "" label << (name.presence || tax_category.name) + " " label << (show_rate_in_label? ? "#{amount * 100}%" : "") + label << " (#{I18n.t('models.tax_rate.included_in_price')})" if included_in_price? + label end def with_tax_included_in_price diff --git a/config/locales/en.yml b/config/locales/en.yml index 0354b9c4d2..2dff764d14 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -140,6 +140,8 @@ en: models: order_cycle: cloned_order_cycle_name: "COPY OF %{order_cycle}" + tax_rate: + included_in_price: "Included in price" validators: date_time_string_validator: diff --git a/lib/spree/core/calculated_adjustments.rb b/lib/spree/core/calculated_adjustments.rb index 6388369209..301d821642 100644 --- a/lib/spree/core/calculated_adjustments.rb +++ b/lib/spree/core/calculated_adjustments.rb @@ -43,7 +43,8 @@ module Spree order: order_object_for(target), label: label, mandatory: mandatory, - state: state + state: state, + included: tax_included?(self, target) ) end @@ -78,6 +79,15 @@ module Spree private + # Used for setting the #included boolean on tax adjustments. This will be removed in a + # later step, as the responsibility for creating all adjustments related to tax will be + # moved into the Spree::TaxRate class. + def tax_included?(originator, target) + originator.is_a?(Spree::TaxRate) && + originator.included_in_price && + originator.default_zone_or_zone_match?(order_object_for(target)) + end + def order_object_for(target) # Temporary method for adjustments transition. if target.is_a? Spree::Order From 173e502c98b4288580db9f8f0edcfac2432387a8 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Wed, 13 Jan 2021 14:46:22 +0000 Subject: [PATCH 4/9] Rename #included scope to #inclusive This method name (#included) is reserved and used internally by ActiveRecord. After updating Ruby, this has changed from a silent warning to a fatal error. --- app/models/spree/adjustment.rb | 2 +- spec/models/spree/adjustment_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index f5e5fe0707..2d5d572110 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -70,7 +70,7 @@ module Spree scope :charge, -> { where('amount >= 0') } scope :credit, -> { where('amount < 0') } scope :return_authorization, -> { where(source_type: "Spree::ReturnAuthorization") } - scope :included, -> { where(included: true) } + scope :inclusive, -> { where(included: true) } scope :additional, -> { where(included: false) } scope :enterprise_fee, -> { where(originator_type: 'EnterpriseFee') } diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index cced77f9e8..bc3acb38ed 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -461,7 +461,7 @@ module Spree it_behaves_like "a model using the LocalizedNumber module", [:amount] end - describe "included and additional scopes" do + describe "inclusive and additional scopes" do let!(:zone) { create(:zone_with_member) } let(:order) { create(:order) } let(:included_in_price) { true } @@ -477,7 +477,7 @@ module Spree context "when tax is included in price" do it "is returned by the #included scope" do - expect(Spree::Adjustment.included).to eq [adjustment] + expect(Spree::Adjustment.inclusive).to eq [adjustment] end end From ddfcda0c0be7ff81a7437f59b78ddb4c11f25070 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 22 Jan 2021 18:50:03 +0000 Subject: [PATCH 5/9] Adapt imported Paypal code The #additional scope from Spree 2.2 is now present, so we can tidy up this conditional. --- app/controllers/spree/paypal_controller.rb | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/app/controllers/spree/paypal_controller.rb b/app/controllers/spree/paypal_controller.rb index 81f7433c22..9b14ce409e 100644 --- a/app/controllers/spree/paypal_controller.rb +++ b/app/controllers/spree/paypal_controller.rb @@ -15,9 +15,7 @@ module Spree order = current_order || raise(ActiveRecord::RecordNotFound) items = order.line_items.map(&method(:line_item)) - tax_adjustments = order.adjustments.tax - # TODO: Remove in Spree 2.2 - tax_adjustments = tax_adjustments.additional if tax_adjustments.respond_to?(:additional) + tax_adjustments = order.adjustments.tax.additional shipping_adjustments = order.adjustments.shipping order.adjustments.eligible.each do |adjustment| @@ -175,12 +173,8 @@ module Spree def payment_details(items) item_sum = items.sum { |i| i[:Quantity] * i[:Amount][:value] } - # Would use tax_total here, but it can include "included" taxes as well. - # For instance, tax_total would include the 10% GST in Australian stores. - # A quick sum will get us around that little problem. - # TODO: Remove additional check in 2.2 - tax_adjustments = current_order.adjustments.tax - tax_adjustments = tax_adjustments.additional if tax_adjustments.respond_to?(:additional) + + tax_adjustments = current_order.adjustments.tax.additional tax_adjustments_total = tax_adjustments.sum(:amount) if item_sum.zero? From 4d4c055577de58e7faaca804cf0c38f6163e8d31 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 22 Jan 2021 20:38:13 +0000 Subject: [PATCH 6/9] Loosen Spree::Adjustment#tax scope to include any tax adjustments --- app/models/spree/adjustment.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/spree/adjustment.rb b/app/models/spree/adjustment.rb index 2d5d572110..0c87668cfe 100644 --- a/app/models/spree/adjustment.rb +++ b/app/models/spree/adjustment.rb @@ -64,7 +64,7 @@ module Spree end end - scope :tax, -> { where(originator_type: 'Spree::TaxRate', adjustable_type: 'Spree::Order') } + scope :tax, -> { where(originator_type: 'Spree::TaxRate') } scope :price, -> { where(adjustable_type: 'Spree::LineItem') } scope :optional, -> { where(mandatory: false) } scope :charge, -> { where('amount >= 0') } From 7940a42e8cfae53145ace922e017b3b27959a4f2 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 22 Jan 2021 20:39:48 +0000 Subject: [PATCH 7/9] Add Spree::Order#all_adjustments scope, which can be used to get all adjustments on an order, and order's line_items, etc, not just adjustments directly on the order itself. --- app/models/spree/order.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/spree/order.rb b/app/models/spree/order.rb index ceb603e275..23bd5ad98f 100644 --- a/app/models/spree/order.rb +++ b/app/models/spree/order.rb @@ -47,6 +47,7 @@ module Spree dependent: :destroy has_many :line_item_adjustments, through: :line_items, source: :adjustments + has_many :all_adjustments, class_name: 'Spree::Adjustment', dependent: :destroy has_many :shipments, dependent: :destroy do def states From ff54426e30e49bde53c51e4f785d1eaf38d6faf4 Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 22 Jan 2021 20:41:13 +0000 Subject: [PATCH 8/9] Expand tests on additional and inclusive taxes. --- spec/models/spree/adjustment_spec.rb | 61 +++++++++++++++++++++------- 1 file changed, 46 insertions(+), 15 deletions(-) diff --git a/spec/models/spree/adjustment_spec.rb b/spec/models/spree/adjustment_spec.rb index bc3acb38ed..c95745e2f2 100644 --- a/spec/models/spree/adjustment_spec.rb +++ b/spec/models/spree/adjustment_spec.rb @@ -461,31 +461,62 @@ module Spree it_behaves_like "a model using the LocalizedNumber module", [:amount] end - describe "inclusive and additional scopes" do + describe "inclusive and additional taxes" do let!(:zone) { create(:zone_with_member) } - let(:order) { create(:order) } + let!(:tax_category) { create(:tax_category, name: "Tax Test") } + let(:distributor) { create(:distributor_enterprise, charges_sales_tax: true) } + let(:order) { create(:order, distributor: distributor) } let(:included_in_price) { true } let(:tax_rate) { - create(:tax_rate, included_in_price: included_in_price, + create(:tax_rate, included_in_price: included_in_price, zone: zone, calculator: ::Calculator::FlatRate.new(preferred_amount: 0.1)) } - let(:included) { true } - let(:adjustment) { - create(:adjustment, adjustable: order, source: order, - originator: tax_rate, included: included) - } + let(:product) { create(:product, tax_category: tax_category) } + let(:variant) { product.variants.first } - context "when tax is included in price" do - it "is returned by the #included scope" do - expect(Spree::Adjustment.inclusive).to eq [adjustment] + describe "tax adjustment creation" do + before do + tax_category.tax_rates << tax_rate + allow(order).to receive(:tax_zone) { zone } + order.line_items << create(:line_item, variant: variant, quantity: 5) + end + + context "with included taxes" do + it "records the tax as included" do + expect(order.all_adjustments.tax.count).to eq 1 + expect(order.all_adjustments.tax.first.included).to be true + end + end + + context "with additional taxes" do + let(:included_in_price) { false } + + it "records the tax as additional" do + expect(order.all_adjustments.tax.count).to eq 1 + expect(order.all_adjustments.tax.first.included).to be false + end end end - context "when tax is additional to the price" do - let(:included) { false } + describe "inclusive and additional scopes" do + let(:included) { true } + let(:adjustment) { + create(:adjustment, adjustable: order, source: order, + originator: tax_rate, included: included) + } - it "is returned by the #additional scope" do - expect(Spree::Adjustment.additional).to eq [adjustment] + context "when tax is included in price" do + it "is returned by the #included scope" do + expect(Spree::Adjustment.inclusive).to eq [adjustment] + end + end + + context "when tax is additional to the price" do + let(:included) { false } + + it "is returned by the #additional scope" do + expect(Spree::Adjustment.additional).to eq [adjustment] + end end end end From 4ec68dcd85eea1c2d103a7257baea332d8104fca Mon Sep 17 00:00:00 2001 From: Matt-Yorkley <9029026+Matt-Yorkley@users.noreply.github.com> Date: Fri, 29 Jan 2021 02:37:35 +0000 Subject: [PATCH 9/9] Update migration to include TaxRate model --- db/migrate/20201227122327_add_included_to_adjustments.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/db/migrate/20201227122327_add_included_to_adjustments.rb b/db/migrate/20201227122327_add_included_to_adjustments.rb index 2aebc93706..2ccbeab2a3 100644 --- a/db/migrate/20201227122327_add_included_to_adjustments.rb +++ b/db/migrate/20201227122327_add_included_to_adjustments.rb @@ -1,4 +1,6 @@ class AddIncludedToAdjustments < ActiveRecord::Migration + class Spree::TaxRate < ActiveRecord::Base; end + class Spree::Adjustment < ActiveRecord::Base belongs_to :originator, polymorphic: true end