Skip to content

Commit

Permalink
Migrate TaxRate, TaxCategory relation to a M-M
Browse files Browse the repository at this point in the history
Implements solidusio#1836

The current relation between TaxRate and TaxCategory can lead
to unnecessary tax rates if a store has multiple tax categories
and if it supports countries (such as Australia) that have a
standard tax rate. This means that for each TaxCategory, the store
will have an extra tax rate even if the percent is the same.

By transforming this relation to a M-M we can allow stores to
create tax rates that have multiple tax categories.
  • Loading branch information
vladstoick committed May 23, 2017
1 parent ed0fd82 commit c0f8394
Show file tree
Hide file tree
Showing 14 changed files with 109 additions and 22 deletions.
4 changes: 2 additions & 2 deletions backend/app/views/spree/admin/tax_rates/_form.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@
<%= f.collection_select(:zone_id, @available_zones, :id, :name, {}, {class: 'select2 fullwidth'}) %>
</div>
<div data-hook="category" class="field">
<%= f.label :tax_category_id, Spree::TaxCategory.model_name.human %>
<%= f.collection_select(:tax_category_id, @available_categories,:id, :name, {}, {class: 'select2 fullwidth'}) %>
<%= f.label :tax_category_ids, Spree::TaxCategory.model_name.human %>
<%= f.collection_select(:tax_category_ids, @available_categories, :id, :name, {}, {class: 'select2 fullwidth', multiple: "multiple"}) %>
</div>
<div data-hook="show_rate" class="field">
<%= f.check_box :show_rate_in_label %>
Expand Down
10 changes: 8 additions & 2 deletions backend/app/views/spree/admin/tax_rates/index.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@
<tr data-hook="rate_header">
<th><%= Spree::TaxRate.human_attribute_name(:zone) %></th>
<th><%= Spree::TaxRate.human_attribute_name(:name) %></th>
<th><%= Spree::TaxRate.human_attribute_name(:tax_category_id) %></th>
<th><%= Spree::TaxRate.human_attribute_name(:tax_categories) %></th>
<th><%= Spree::TaxRate.human_attribute_name(:amount) %></th>
<th><%= Spree::TaxRate.human_attribute_name(:included_in_price) %></th>
<th><%= Spree::TaxRate.human_attribute_name(:show_rate_in_label) %></th>
Expand All @@ -42,7 +42,13 @@
<tr id="<%= spree_dom_id tax_rate %>" data-hook="rate_row" class="<%= cycle('odd', 'even')%>">
<td class="align-center"><%=tax_rate.zone.try(:name) || Spree.t(:not_available) %></td>
<td class="align-center"><%=tax_rate.name %></td>
<td class="align-center"><%=tax_rate.tax_category.try(:name) || Spree.t(:not_available) %></td>
<td class="align-center">
<% if tax_rate.tax_categories.any? %>
<%= tax_rate.tax_categories.map(&:name).join(", ") %>
<% else %>
<%= Spree.t(:not_available) %>
<% end %>
</td>
<td class="align-center"><%=tax_rate.amount %></td>
<td class="align-center"><%=tax_rate.included_in_price? ? Spree.t(:say_yes) : Spree.t(:say_no) %></td>
<td class="align-center"><%=tax_rate.show_rate_in_label? ? Spree.t(:say_yes) : Spree.t(:say_no) %></td>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

# Regression test for https://github.com/spree/spree/issues/535
it "can see a tax rate in the list if the tax category has been deleted" do
tax_rate.tax_category.update_column(:deleted_at, Time.current)
tax_rate.tax_categories.first.update_column(:deleted_at, Time.current)
click_link "Tax Rates"

expect(find("table tbody td:nth-child(3)")).to have_content('N/A')
Expand Down
2 changes: 1 addition & 1 deletion backend/spec/features/admin/orders/adjustments_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@

let!(:ship_address) { create(:address) }
let!(:tax_zone) { create(:global_zone) } # will include the above address
let!(:tax_rate) { create(:tax_rate, amount: 0.20, zone: tax_zone, tax_category: tax_category) }
let!(:tax_rate) { create(:tax_rate, amount: 0.20, zone: tax_zone, tax_categories: [tax_category]) }

let!(:order) do
create(
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/calculator/default_tax.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ class Calculator::DefaultTax < Calculator
# Orders created with Spree 2.2 and after, have them applied to the line items individually.
def compute_order(order)
matched_line_items = order.line_items.select do |line_item|
line_item.tax_category == rate.tax_category
rate.tax_categories.include?(line_item.tax_category)
end

line_items_total = matched_line_items.sum(&:discounted_amount)
Expand Down
2 changes: 1 addition & 1 deletion core/app/models/spree/tax/shipping_rate_taxer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ def tax(shipping_rate)

def tax_rates_for_shipping_rate(shipping_rate)
applicable_rates(shipping_rate.order).select do |tax_rate|
tax_rate.tax_category == shipping_rate.tax_category
tax_rate.tax_categories.include?(shipping_rate.tax_category)
end
end
end
Expand Down
8 changes: 5 additions & 3 deletions core/app/models/spree/tax/tax_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,9 @@ module TaxHelpers
#
# For further discussion, see https://github.com/spree/spree/issues/4397 and https://github.com/spree/spree/issues/4327.
def applicable_rates(order)
order_zone_tax_category_ids = rates_for_order(order).map(&:tax_category_id)
order_zone_tax_category_ids = rates_for_order(order).flat_map(&:tax_categories).map(&:id)
default_rates_with_unmatched_tax_category = rates_for_default_zone.to_a.delete_if do |default_rate|
order_zone_tax_category_ids.include?(default_rate.tax_category_id)
(order_zone_tax_category_ids & default_rate.tax_categories.map(&:id)).any?
end

(rates_for_order(order) + default_rates_with_unmatched_tax_category).uniq
Expand All @@ -38,7 +38,9 @@ def sum_of_included_tax_rates(item)
end

def rates_for_item(item)
applicable_rates(item.order).select { |rate| rate.tax_category_id == item.tax_category_id }
applicable_rates(item.order).select do |rate|
rate.tax_categories.map(&:id).include?(item.tax_category_id)
end
end
end
end
Expand Down
10 changes: 9 additions & 1 deletion core/app/models/spree/tax_category.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,15 @@ class TaxCategory < Spree::Base
validates :name, presence: true
validates_uniqueness_of :name, unless: :deleted_at

has_many :tax_rates, dependent: :destroy, inverse_of: :tax_category
has_many :tax_rate_tax_categories,
class_name: Spree::TaxRateTaxCategory,
dependent: :destroy,
inverse_of: :tax_category
has_many :tax_rates,
through: :tax_rate_tax_categories,
class_name: Spree::TaxRate,
inverse_of: :tax_categories

after_save :ensure_one_default

def self.default
Expand Down
21 changes: 15 additions & 6 deletions core/app/models/spree/tax_rate.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,20 @@ class TaxRate < Spree::Base
include Spree::AdjustmentSource

belongs_to :zone, class_name: "Spree::Zone", inverse_of: :tax_rates
belongs_to :tax_category, class_name: "Spree::TaxCategory", inverse_of: :tax_rates

has_many :tax_rate_tax_categories,
class_name: Spree::TaxRateTaxCategory,
dependent: :destroy,
inverse_of: :tax_rate
has_many :tax_categories,
through: :tax_rate_tax_categories,
class_name: Spree::TaxCategory,
inverse_of: :tax_rates

has_many :adjustments, as: :source
has_many :shipping_rate_taxes, class_name: "Spree::ShippingRateTax"

validates :amount, presence: true, numericality: true
validates :tax_category_id, presence: true

# Finds all tax rates whose zones match a given address
scope :for_address, ->(address) { joins(:zone).merge(Spree::Zone.for_address(address)) }
Expand Down Expand Up @@ -94,10 +101,12 @@ def compute_amount(item)
private

def adjustment_label(amount)
Spree.t translation_key(amount),
scope: "adjustment_labels.tax_rates",
name: name.presence || tax_category.name,
amount: amount_for_adjustment_label
Spree.t(
translation_key(amount),
scope: "adjustment_labels.tax_rates",
name: name.presence || tax_categories.map(&:name).join(", "),
amount: amount_for_adjustment_label
)
end

def amount_for_adjustment_label
Expand Down
6 changes: 6 additions & 0 deletions core/app/models/spree/tax_rate_tax_category.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
module Spree
class TaxRateTaxCategory < Spree::Base
belongs_to :tax_rate, class_name: Spree::TaxRate, inverse_of: :tax_rate_tax_categories
belongs_to :tax_category, class_name: Spree::TaxCategory, inverse_of: :tax_rate_tax_categories
end
end
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
class TransformTaxRateCategoryRelation < ActiveRecord::Migration[5.0]
class TaxRate < ActiveRecord::Base
self.table_name = "spree_tax_rates"
end

class TaxRateTaxCategory < ActiveRecord::Base
self.table_name = "spree_tax_rate_tax_categories"
end

def up
create_table :spree_tax_rate_tax_categories do |t|
t.integer :tax_category_id, index: true, null: false
t.integer :tax_rate_id, index: true, null: false
end

add_foreign_key :spree_tax_rate_tax_categories, :spree_tax_categories, column: :tax_category_id
add_foreign_key :spree_tax_rate_tax_categories, :spree_tax_rates, column: :tax_rate_id

TaxRate.where.not(tax_category_id: nil).find_each do |tax_rate|
TaxRateTaxCategory.create!(
tax_rate_id: tax_rate.id,
tax_category_id: tax_rate.tax_category_id
)
end

remove_column :spree_tax_rates, :tax_category_id
end

def down
add_column :spree_tax_rates, :tax_category_id, :integer, index: true
add_foreign_key :spree_tax_rates, :spree_tax_categories, column: :tax_category_id

TaxRate.find_each do |tax_rate|
tax_category_ids = TaxRateTaxCategory.where(tax_rate_id: tax_rate.id).pluck(:tax_category_id)

tax_category_ids.each_with_index do |category_id, i|
if i.zero?
tax_rate.update!(tax_category_id: category_id)
else
new_tax_rate = tax_rate.dup
new_tax_rate.update!(tax_category_id: category_id)
end
end
end

drop_table :spree_tax_rate_tax_categories
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,11 @@
after(:create) do |adjustment|
# Set correct tax category, so that adjustment amount is not 0
if adjustment.adjustable.is_a?(Spree::LineItem)
adjustment.source.tax_category = adjustment.adjustable.tax_category
if adjustment.adjustable.tax_category.present?
adjustment.source.tax_categories = [adjustment.adjustable.tax_category]
else
adjustment.source.tax_categories = []
end
adjustment.source.save
adjustment.update!
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
factory :tax_rate, class: Spree::TaxRate do
zone
amount 0.1
tax_category
association(:calculator, factory: :default_tax_calculator)
tax_categories { [build(:tax_category)] }
end
end
8 changes: 6 additions & 2 deletions sample/db/samples/tax_rates.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,11 @@
tax_rate = Spree::TaxRate.create(
name: "North America",
zone: north_america,
amount: 0.05,
tax_category: clothing)
amount: 0.05
)
tax_rate.calculator = Spree::Calculator::DefaultTax.create!
tax_rate.save!
Spree::TaxRateTaxCategory.create!(
tax_rate: tax_rate,
tax_category: clothing
)

0 comments on commit c0f8394

Please sign in to comment.