Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TaxRate M-M TaxCategory #1851

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,10 @@
- Ignore `adjustment.finalized` on tax adjustments. [\#1936](https://github.com/solidusio/solidus/pull/1936) ([jordan-brough](https://github.com/jordan-brough))
- Deprecate `#simple_current_order`
[\#1915](https://github.com/solidusio/solidus/pull/1915) ([ericsaupe](https://github.com/ericsaupe))
- Transform the relation between TaxRate and TaxCategory to a Many to Many [\#1851](https://github.com/solidusio/solidus/pull/1851) ([vladstoick](https://github.com/vladstoick))

This fixes issue [\#1836](https://github.com/solidusio/solidus/issues/1836). By allowing a TaxRate to tax multiple categories, stores don't have to create multiple TaxRates with the same value if a zone doesn't have different tax rates for some tax categories.


## Solidus 2.2.1 (2017-05-09)

Expand Down
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(", ") %>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, you might want to use an internationalized separator, as not all locales will work with a comma separated list. Most will though, so I won't insist. :)

<% 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: 4 additions & 4 deletions core/spec/lib/spree/core/price_migrator_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
name: "German reduced VAT",
included_in_price: true,
amount: 0.07,
tax_category: books_category,
tax_categories: [books_category],
zone: eu_zone
)
end
Expand All @@ -65,7 +65,7 @@
name: "German VAT",
included_in_price: true,
amount: 0.19,
tax_category: normal_category,
tax_categories: [normal_category],
zone: eu_zone
)
end
Expand All @@ -75,7 +75,7 @@
name: "German VAT",
included_in_price: true,
amount: 0.19,
tax_category: digital_category,
tax_categories: [digital_category],
zone: germany_zone
)
end
Expand All @@ -85,7 +85,7 @@
name: "Romanian VAT",
included_in_price: true,
amount: 0.24,
tax_category: digital_category,
tax_categories: [digital_category],
zone: romania_zone
)
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@
context 'when shipments should be taxed' do
let!(:ship_address) { create(:address) }
let!(:tax_zone) { create(:global_zone) } # will include the above address
let!(:tax_rate) { create(:tax_rate, amount: 0.10, zone: tax_zone, tax_category: tax_category) }
let!(:tax_rate) { create(:tax_rate, amount: 0.10, zone: tax_zone, tax_categories: [tax_category]) }

let(:tax_category) { create(:tax_category) }
let(:shipping_method) { create(:shipping_method, tax_category: tax_category, zones: [tax_zone]) }
Expand Down
2 changes: 1 addition & 1 deletion core/spec/models/spree/calculator/default_tax_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
let!(:zone) { create(:zone, name: "Country Zone", default_tax: default_tax, countries: [tax_rate_country]) }
let(:tax_rate_country) { address.country }
let(:tax_category) { create(:tax_category) }
let!(:rate) { create(:tax_rate, tax_category: tax_category, amount: 0.05, included_in_price: included_in_price, zone: zone) }
let!(:rate) { create(:tax_rate, tax_categories: [tax_category], amount: 0.05, included_in_price: included_in_price, zone: zone) }
let(:included_in_price) { false }
let(:default_tax) { false }
subject(:calculator) { Spree::Calculator::DefaultTax.new(calculable: rate ) }
Expand Down
2 changes: 1 addition & 1 deletion core/spec/models/spree/order/checkout_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -184,7 +184,7 @@ def assert_state_changed(order, from, to)

it "recalculates tax and updates totals" do
zone = create(:zone, countries: [order.tax_address.country])
create(:tax_rate, tax_category: line_item.tax_category, amount: 0.05, zone: zone)
create(:tax_rate, tax_categories: [line_item.tax_category], amount: 0.05, zone: zone)
order.next!
expect(order).to have_attributes(
adjustment_total: 0.5,
Expand Down
2 changes: 1 addition & 1 deletion core/spec/models/spree/order_capturing_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@

let!(:product) { create(:product, price: 10.00) }
let!(:variant) do
create(:variant, price: 10, product: product, track_inventory: false, tax_category: tax_rate.tax_category)
create(:variant, price: 10, product: product, track_inventory: false, tax_category: tax_rate.tax_categories.first)
end
let!(:shipping_method) { create(:free_shipping_method) }
let(:tax_rate) { create(:tax_rate, amount: 0.1, zone: create(:global_zone, name: "Some Tax Zone")) }
Expand Down
2 changes: 1 addition & 1 deletion core/spec/models/spree/order_contents_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -123,7 +123,7 @@
describe 'tax calculations' do
let!(:zone) { create(:global_zone) }
let!(:tax_rate) do
create(:tax_rate, zone: zone, tax_category: variant.tax_category)
create(:tax_rate, zone: zone, tax_categories: [variant.tax_category])
end

context 'when the order has a taxable address' do
Expand Down
2 changes: 1 addition & 1 deletion core/spec/models/spree/order_updater_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ def create_adjustment(label, amount)
describe 'tax recalculation' do
let!(:ship_address) { create(:address) }
let!(:tax_zone) { create(:global_zone) } # will include the above address
let!(:tax_rate) { create(:tax_rate, zone: tax_zone, tax_category: tax_category) }
let!(:tax_rate) { create(:tax_rate, zone: tax_zone, tax_categories: [tax_category]) }

let(:order) do
create(
Expand Down
2 changes: 1 addition & 1 deletion core/spec/models/spree/price_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,7 @@
describe 'net_amount' do
let(:country) { create(:country, iso: "DE") }
let(:zone) { create(:zone, countries: [country]) }
let!(:tax_rate) { create(:tax_rate, included_in_price: true, zone: zone, tax_category: variant.tax_category) }
let!(:tax_rate) { create(:tax_rate, included_in_price: true, zone: zone, tax_categories: [variant.tax_category]) }

let(:variant) { create(:product).master }

Expand Down
2 changes: 1 addition & 1 deletion core/spec/models/spree/promotion_handler/coupon_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ def expect_adjustment_creation(adjustable:, promotion:, promotion_code:nil)
let(:order) { create(:order, store: store) }
let(:tax_category) { create(:tax_category, name: "Taxable Foo") }
let(:zone) { create(:zone, :with_country) }
let!(:tax_rate) { create(:tax_rate, amount: 0.1, tax_category: tax_category, zone: zone )}
let!(:tax_rate) { create(:tax_rate, amount: 0.1, tax_categories: [tax_category], zone: zone ) }

before(:each) do
expect(order).to receive(:tax_address).at_least(:once).and_return(Spree::Tax::TaxLocation.new(country: zone.countries.first))
Expand Down
4 changes: 2 additions & 2 deletions core/spec/models/spree/shipment_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,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.1, zone: tax_zone, tax_category: tax_category) }
let!(:tax_rate) { create(:tax_rate, amount: 0.1, zone: tax_zone, tax_categories: [tax_category]) }
let(:tax_category) { create(:tax_category) }
let(:variant) { create(:variant, tax_category: tax_category) }

Expand Down Expand Up @@ -548,7 +548,7 @@
context "changes shipping rate via general update" do
let!(:ship_address) { create(:address) }
let!(:tax_zone) { create(:global_zone) } # will include the above address
let!(:tax_rate) { create(:tax_rate, amount: 0.10, zone: tax_zone, tax_category: tax_category) }
let!(:tax_rate) { create(:tax_rate, amount: 0.10, zone: tax_zone, tax_categories: [tax_category]) }
let(:tax_category) { create(:tax_category) }

let(:order) do
Expand Down
Loading