Add Spree::ShippingRateTax model #904

mamhoff · 2016-02-24T21:41:23Z

Previously, shipping rate taxes were calculated on the fly every time
a shipping rate would be displayed. Now, shipping rate taxes are stored
on a dedicated table to look up.

There is a new model Spree::ShippingRateTax where the taxes are stored,
and a new Spree::Tax::ShippingRateTaxer that builds those taxes from within
Spree::Stock::Estimator.

The shipping rate taxer class can be exchanged for a custom estimator class
using the new Spree::Appconfiguration.shipping_rate_taxer_class preference.

houndci-bot · 2016-02-24T21:41:30Z

core/spec/models/spree/shipping_rate_tax_spec.rb

+    it { is_expected.to respond_to(:tax_rate) }
+    it { is_expected.to respond_to(:shipping_rate) }
+
+


Extra blank line detected.
Extra empty line detected at block body end.

mamhoff · 2016-02-25T18:15:00Z

I went somewhat beyond minimal, but I think this turned out great!

We have

persisted taxes for shipping rates
no direct association to the order or messing around with adjustments
MUCH cleaner code
shipping rates with more than one tax
and a data migration

@cbrunsdon @jordan-brough @jhawthorn This is for you now.

tvdeyen · 2016-02-25T20:11:43Z

core/app/models/spree/shipping_rate.rb

+
+      tax_explanations = taxes.map(&:label).join(tax_label_separator)
+
+      Spree.t :display_price_with_explanations,


I already love you for this one

cbrunsdon · 2016-02-26T18:01:31Z

This all seems really reasonable to me but I want to go over it again commit-by-commit with @jhawthorn.

As an aside, looking back I wish we made the TaxHelpers methods take params rather than expecting predefined methods on the included model.

mamhoff · 2016-02-26T18:03:52Z

All the methods in the TaxHelper module are declared private. It wouldn't be much of an issue to change those signatures IMO.

cbrunsdon · 2016-02-26T18:11:39Z

I would prefer we make them take an arg list, as it gives us more flexibility about how and why they're used (like the "ShippingRateTaxer" needing to take the order in as a class param because of it.

cbrunsdon · 2016-03-01T03:33:07Z

@mamhoff this needs to be rebased against the module-takes-param work

jordan-brough · 2016-03-01T05:49:16Z

I really like this direction of separating out shipping rates taxes into their own model!

jordan-brough · 2016-03-01T05:57:12Z

core/app/models/spree/shipping_rate.rb

-    def display_tax_amount(tax_amount)
-      Spree::Money.new(tax_amount, currency: currency)
+    def pre_tax_amount
+      default_vat_amount_sum = Spree::TaxRate.


I'm having a hard time wrapping my head around part of this. In current Solidus doesn't the pre_tax_amount depend on where the item is being shipped (i.e. depend on which tax rate should be used)? Yet here we are always using the tax rates for the default zone right? Doesn't that mean that the code below from default_tax.rb will compute the wrong amount if the order is actually being shipped somewhere other than the default zone?

def compute_item(item) if rate.included_in_price deduced_total_by_rate(item.pre_tax_amount, rate) else round_to_two_places(item.discounted_amount * rate.amount) end end

Where am I getting mixed up?

You're not getting mixed up, this is IMO a bug in the current taxation system. The pre_tax_amount is the basis on which to calculate VAT (another way to say this it's the actual value of the good). The current taxation system in Solidus allows that actual value to change under certain conditions ("the receiving country also has VAT"). If the receiving country does not have VAT, then we create this refund so it kinda sorta appears like we didn't charge it in the first place.

This is actually a glimpse into Solidus' future taxation system: All the prices that come out of the backend (calculated prices as well as prices entered by admins) will include the VAT of ONE place. If we ship to another place with different VAT, then we have to deduct that VAT (only), and apply another one.

I think I will extract this to its own PR as it's really quite a complex topic.

Here we go #941

mamhoff · 2016-03-01T19:42:43Z

Rebased on #941, so that doesn't pollute this. Otherwise ready for review!

jordan-brough · 2016-03-03T14:41:47Z

core/app/models/spree/shipping_rate.rb

@@ -1,46 +1,48 @@
 module Spree
  class ShippingRate < Spree::Base
+    attr_writer :order


Is this here primarily because the Coordinator builds shipping rates before it builds shipments? Are there other places where we need to build shipping rates without a shipment in hand? Does it make sense to have a shipping rate without a shipment? It just feels a bit awkward and I'm wondering if there's anything reasonable we could do to avoid this.

Yes, that's exactly the reason. The estimator doesn't really build shipments at all, one of the other stock classes does that. And no, a shipping rate without a shipment doesn't really make a lot of sense. But I (honestly) get a little lost between packages, inventory units and shipments at times, so I opted for not refactoring that here. If anyone has a good idea on how to avoid this, I'm all for it.

OK great. Looking at this was what prompted me to submit #950, which I think is a good idea in any case, but might allow us to clean things up for this also.

@mamhoff would #965 be sufficient to remove the attr_writer :order related code here?

cbrunsdon · 2016-03-04T00:34:48Z

Okay, I've read through this a bunch of times and don't have any serious feedback left.

Going to 👍.

Also standard "need changelog at some time for this".

gmacdougall · 2016-03-04T00:41:40Z

@mamhoff Could you please add some YARD to the new classes describing their purpose and public interfaces? I'd like to have that in place so people who are wanting to provide their own implementations have a good reference.

mamhoff · 2016-03-10T07:13:21Z

Added a changelog entry, rebased on @jordan-brough s excellent work in #965 , added YARD documentation. Ready for final review.

adammathys · 2016-03-11T00:53:13Z

core/db/migrate/20160225152313_remove_tax_rate_from_shipping_rate.rb

@@ -0,0 +1,29 @@
+class RemoveTaxRateFromShippingRate < ActiveRecord::Migration
+  def up
+    Spree::ShippingRate.find_each do |shipping_rate|


While I know we don't exactly have a very good track record of it on this project, I would like to be more careful about using models in migrations.

Would you mind adding simple class declarations above?

class Spree::ShippingRate < ActiveRecord::Base; end class Spree::TaxRate < ActiveRecord::Base; end # etc.

That way if for whatever reason we decide to change the class names or remove those classes we don't accidentally break this migration. Alternatively, we could rewrite the migration to use SQL but in this particular case I think that's probably more effort than it's worth.

Hm, I do have to call compute_amount on the tax_rates, which is a method with very weird behaviour that's actually about to change. I just had a long chat with @tvdeyen about ways to remedy this particular rabbit hole (it's deep) and essentially it will mean moving the weird behaviour of compute_amount into the Spree::Calculator::DefaultTax, then marking that class as deprecated, and creating a new, simpler Spree::Calculator::Tax::Default which hopefully makes things better. This is not ideal, but we have to make sure changing the implementation of how taxes are calculated does not break old taxes.

@adammathys you are totally right.

But in this case we need a trusted interface and S.P.O.T. of TaxRate#compute_amount.
That interface will not change for a long time (we can ensure that, because we own it).
The internal implementation will change, though. But that's ok.

If we ensure that the public interface will not change, then violating the rules here is safe IMO.

adammathys · 2016-03-11T00:59:32Z

Overall 👍 from me. Very much appreciate the YARD documentation.

mamhoff · 2016-03-14T15:52:19Z

Rebase on current master. I also added a few methods to the mini-implementation of Spree::TaxRate in the migration so that ti the tax rate knows everything it needs to calculate amounts for shipping rates.

The implementation is the one post #980 , so we might want to merge that PR first.

This is the minimal thing required to correctly display shipping rates without having to recalculate all the taxes every time we display a tax rate's price. For the time being I resisted already creating a `Tax` model with a polymorphic `taxable`.

Which will just modify new shipping rates to have taxes attached. Coming soon.

The idea is just for this to work essentially without needing to save, or having saved records.

So that we have a display_absolute_amount method.

This creates a beautiful little label for every tax attached to a shipping rate.

This method used to be a monster. With the new ShippingRateTax Model, we can now add support for multiple taxes on shipping rates. The calculation of shipping rate tax amounts now goes through Spree::TaxRate#compute_amount. This way, we don't have to duplicate the multiplying by (-1) in the tax calculator and further align how shipping rate taxes are calculated. This required a serious spec-rewrite.

Since now we have the persisted Spree::ShippingRate#taxes, we do not need to store one of the tax rates anymore on the shipping rate. The data migration contains three lines of code from tax_rate.rb so that the migration still runs in the future.

All the other methods involved in creating stock-related things are configurable via the AppConfiguration. This one should, too!

PR #904, which this commit belongs to, introduces two new classes and a new preference. This adds a changelog entry as well as YARD documentation for both the `Spree::ShippingRateTax` model and the `Spree::Tax::ShippingRateTaxer` class.

mamhoff · 2016-04-06T12:39:46Z

Rebased on master, still good to go.

gmacdougall · 2016-04-08T12:52:23Z

👍

When reviewing solidusio#904, we realized that we can not change the logic of Spree::TaxRate#compute_amount if we want the migration included there to always to the right thing. Because that method modifies the tax amount of things, it should actually be in a calculator. I now moved that logic to the default tax calculator, with the intention of deprecating use of that calculator entirely. There is code for pre-Spree-2.2 order-based taxation in there, as well as code for generating tax refunds, which in the future we won't need either.

PR solidusio#904, which this commit belongs to, introduces two new classes and a new preference. This adds a changelog entry as well as YARD documentation for both the `Spree::ShippingRateTax` model and the `Spree::Tax::ShippingRateTaxer` class.

houndci-bot reviewed Feb 24, 2016
View reviewed changes

mamhoff changed the title ~~WIP Add Spree::ShippingRateTax model~~ Add Spree::ShippingRateTax model Feb 25, 2016

mamhoff mentioned this pull request Feb 25, 2016

RfC Make shipping rates taxable like line items or shipments #759

Closed

tvdeyen reviewed Feb 25, 2016
View reviewed changes

mamhoff mentioned this pull request Feb 26, 2016

Use better i18n for generating tax adjustment labels #914

Merged

mamhoff mentioned this pull request Feb 26, 2016

Use delegate for finding the tax category of a shipment #926

Merged

jordan-brough reviewed Mar 1, 2016
View reviewed changes

mamhoff mentioned this pull request Mar 1, 2016

Calculate pre tax amounts on the fly #941

Merged

jordan-brough reviewed Mar 3, 2016
View reviewed changes

jordan-brough mentioned this pull request Mar 4, 2016

Connect shipping rates to shipments earlier in Stock::Coordinator #965

Merged

adammathys reviewed Mar 11, 2016
View reviewed changes

mamhoff mentioned this pull request Mar 11, 2016

Move the strange refunding logic from Spree::TaxRate into the default tax calculator #980

Merged

mamhoff closed this Mar 14, 2016

mamhoff reopened this Mar 14, 2016

mamhoff mentioned this pull request Mar 31, 2016

Solidus VAT Roadmap #989

Closed

23 tasks

mamhoff and others added 12 commits April 6, 2016 11:40

Add Spree::ShippingRateTax model

d6a511a

This is the minimal thing required to correctly display shipping rates without having to recalculate all the taxes every time we display a tax rate's price. For the time being I resisted already creating a `Tax` model with a polymorphic `taxable`.

Add ShippingRateTaxer object

b1dd116

Which will just modify new shipping rates to have taxes attached. Coming soon.

Make the shipping rate taxer add taxes to the shipping rate

ba7955c

The idea is just for this to work essentially without needing to save, or having saved records.

Add absolute_amount method on shipping_rate

5cea3dd

Extend ShippingRateTax with Spree::DisplayMoney

f4cee35

So that we have a display_absolute_amount method.

Delegate Spree::ShippingRateTax#currency to shipment

2cc4b1b

Add i18n labels for Spree::ShippingRateTax

54dce3c

Add ShippingRateTax#label

8b78bda

This creates a beautiful little label for every tax attached to a shipping rate.

Make the ShippingRateTaxer configurable

6850fa5

All the other methods involved in creating stock-related things are configurable via the AppConfiguration. This one should, too!

Add changelog and YARD for shipping rate taxes

2871d7e

PR #904, which this commit belongs to, introduces two new classes and a new preference. This adds a changelog entry as well as YARD documentation for both the `Spree::ShippingRateTax` model and the `Spree::Tax::ShippingRateTaxer` class.

gmacdougall merged commit 13d67a4 into solidusio:master Apr 8, 2016

mamhoff deleted the tax-estimations-on-shipping-rates branch May 24, 2016 19:46

jordan-brough mentioned this pull request Jun 3, 2016

Failing spec for change in Coordinator behavior #1219

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Spree::ShippingRateTax model #904

Add Spree::ShippingRateTax model #904

mamhoff commented Feb 24, 2016

houndci-bot Feb 24, 2016

mamhoff commented Feb 25, 2016

tvdeyen Feb 25, 2016

cbrunsdon commented Feb 26, 2016

mamhoff commented Feb 26, 2016

cbrunsdon commented Feb 26, 2016

cbrunsdon commented Mar 1, 2016

jordan-brough commented Mar 1, 2016

jordan-brough Mar 1, 2016

mamhoff Mar 1, 2016

mamhoff Mar 1, 2016

mamhoff commented Mar 1, 2016

jordan-brough Mar 3, 2016

mamhoff Mar 3, 2016

jordan-brough Mar 3, 2016

jordan-brough Mar 4, 2016

cbrunsdon commented Mar 4, 2016

gmacdougall commented Mar 4, 2016

mamhoff commented Mar 10, 2016

adammathys Mar 11, 2016

mamhoff Mar 11, 2016

tvdeyen Mar 11, 2016

adammathys commented Mar 11, 2016

mamhoff commented Mar 14, 2016

mamhoff commented Apr 6, 2016

gmacdougall commented Apr 8, 2016

		it { is_expected.to respond_to(:tax_rate) }
		it { is_expected.to respond_to(:shipping_rate) }


		tax_explanations = taxes.map(&:label).join(tax_label_separator)

		Spree.t :display_price_with_explanations,

Add Spree::ShippingRateTax model #904

Add Spree::ShippingRateTax model #904

Conversation

mamhoff commented Feb 24, 2016

Choose a reason for hiding this comment

mamhoff commented Feb 25, 2016

Choose a reason for hiding this comment

cbrunsdon commented Feb 26, 2016

mamhoff commented Feb 26, 2016

cbrunsdon commented Feb 26, 2016

cbrunsdon commented Mar 1, 2016

jordan-brough commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamhoff commented Mar 1, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbrunsdon commented Mar 4, 2016

gmacdougall commented Mar 4, 2016

mamhoff commented Mar 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adammathys commented Mar 11, 2016

mamhoff commented Mar 14, 2016

mamhoff commented Apr 6, 2016

gmacdougall commented Apr 8, 2016