New configurable tax calculator interface #1892

adammathys · 2017-05-08T18:44:10Z

One of the major barriers to implmenting a custom tax calculator is having to reimplement a large amount of logic around creating/updating tax adjustments.

In order to simplify this, the integration point for configurable tax calculation should change a little bit. The new interface this PR proposes takes in a Spree::Order in the initializer, then returns tax information in a set of POROs which can then be used by Solidus to create or update tax adjustments accordingly.

Here are the new POROs, in all of their simple glory:

module Spree
  module Tax
    class TaxedOrder
      include ActiveModel::Model
      attr_accessor :id, :line_items, :shipments
    end

    class TaxedItem
      include ActiveModel::Model
      attr_accessor :id, :label, :tax_rate, :amount, :included_in_price
    end
  end
end

Eventually, it would be nice to also have the input for the tax calculator be a PORO as well. However, I felt that would be too large of a change for this initial PR. I have put some initial work into what those objects could look like and pushed them up to a Gist.

TODO

More comprehensive unit tests of the new models.
Ensure existing adjuster overrides work
Shipping rate taxation
Good comments at the top of TaxCalculator::Default, to help future implementers

jhawthorn

Looking at tax_calculator/default.rb, I can easily see how that would be implemented in different tax extensions 👍 .

I think we need to bounce around ideas for more descriptive names (Application and Output are too overloaded), and there are a few outstanding questions (like how we should deal with shipping rate taxation), but I think this is a great start.

jhawthorn · 2017-05-08T19:01:07Z

core/app/models/spree/tax/application.rb

@@ -0,0 +1,51 @@
+module Spree
+  module Tax
+    class Application


I think we need a different class name here. Application has too strong another meaning

Adjuster, Performer, Applier, Applicator, OrderTaxer. Just throwing in ideas here, I second Johns thought.

I agree. Just went with the the simplest placeholder name for the time being.

jhawthorn · 2017-05-08T19:05:39Z

core/app/models/spree/tax/application.rb

+      def perform!
+        %i(line_items shipments).each do |items|
+          order.send(items).each do |item|
+            rates = output.send(items).select { |i| i.id == item.id }


I think we could avoid the sends here (but the rest of the structure looks good).

jhawthorn · 2017-05-08T19:10:12Z

core/app/models/spree/tax/output/item.rb

@@ -0,0 +1,10 @@
+module Spree
+  module Tax
+    module Output


Maybe we can come up with a more obvious name to what these classes represent than Output. Maybe Spree::Tax::TaxedItem? Naming is hard.

TaxationData?

jhawthorn · 2017-05-08T19:18:36Z

core/app/models/spree/order_updater.rb

@@ -226,7 +226,8 @@ def update_order_promotions
    end

    def update_taxes
-      Spree::Config.tax_adjuster_class.new(order).adjust!
+      output = Spree::Config.tax_calculator_class.new(order).calculate
+      Spree::Tax::Application.new(order, output).perform!


If we move this change into the Spree::Tax::OrderAdjuster, we can keep overrides of that working (and delete the existing code in there)

mamhoff · 2017-05-08T19:52:40Z

core/app/models/spree/app_configuration.rb

    attr_writer :tax_adjuster_class
    def tax_adjuster_class
+      Spree::Deprecation.warn("Spree::AppConfiguration#tax_adjuster_class is DEPRECATED, please use #tax_calculator_class instead.", caller)


If we keep this interface and move the new stuff into the default order adjuster? As per @jhawthorns comment on a later commit?

mamhoff · 2017-05-08T19:58:18Z

core/app/models/spree/tax/output/item.rb

+    module Output
+      class Item
+        include ActiveModel::Model
+        attr_accessor :id, :tax_rate, :amount, :included_in_price


I would like the label to be part of this. Avalara can add labels to whatever they return, and so should we.

I would like the matching not to be done by ID. When recording taxation API interactions with e.g. Avalara, it always hits us that matching is done by what is essentially a database artifact. Can we just try relying on sorting by ID and adding taxes - or no taxes - based on that?

I would like our tax rate to be a hidden implementation detail. If using an external service, e.g. Avalara, we don't have tax rates to play with.

attr_accessor :i_of_sorted_items, :label, :amount, :included_in_price

Love the idea of moving labels to the output object. I'll put together that change in a few.

We need some way of tying the rate to the line item or shipment it should be applied to. The ID is our only real way of doing so. Sorting both lists won't always work. (For example, some items can be tax exempt and shouldn't have any rates associated to them.)

As you've commented later, this is due to adjustments requiring a source.

Tax exempt items can just have an adjustment_amount of 0. We can calculate shipping rates without passing database IDs to external services, so why can we not do the same with taxation? :)

Shipping rates are much easier to avoid passing database IDs. Since there's no need to tie the rate to any existing database record afterwards. If we were looking to make an interface for returning a list of tax rates for an item I would entirely agree with you.

However, we're looking to return a list of calculated taxes for items on an order. Meaning we need to tie those values together in some concrete way. Sorting the items and using their indices, giving them a new alphanumeric identifier, or using the database IDs are all functionally equivalent. Using the database IDs is just the simplest and most robust approach.

I appreciate both sides here. We do want to associate tax amounts with specific items, and an ID is the most obvious and robust way to do that. That said, the more abstracted from the DB this is the better.

It's worth considering that passing using an ID here doesn't necessarily require that the ID is sent over the wire to an external service, the sort-by-id could be done in the extension.

mamhoff · 2017-05-08T19:59:52Z

core/app/models/spree/tax/output/order.rb

+    module Output
+      class Order
+        include ActiveModel::Model
+        attr_accessor :id, :line_items, :shipments


The current implementation allows for order-level tax adjustments. For all sane configurations I know we shouldn't allow that, just noting that this is conspicuously absent here.

We allow that only as legacy pre-spree-2.2 data. The current Tax::OrderAdjuster and OrderUpdater don't support it so neither should this.

mamhoff

I /really, really like/ the direction this is going in. However, I have a bunch of concerns with some of the things, namely:

using adjustments for taxation (IMO they shouldnt as they tie us to the DB-based Spree::TaxRate.
using database IDs for identifying items in an order when communicating with external services (it prevents VCR from working)

mamhoff · 2017-05-08T20:01:43Z

core/app/models/spree/tax_calculator/default.rb

+      def calculate
+        Spree::Tax::Output::Order.new(
+          id: order.id,
+          line_items: line_item_rates,


This line reveals that Spree::Tax::Output::Order#line_items should be named line_item_taxes.

mamhoff · 2017-05-08T20:04:24Z

core/app/models/spree/tax_calculator/default.rb

+        Spree::Tax::Output::Order.new(
+          id: order.id,
+          line_items: line_item_rates,
+          shipments: shipment_rates


Similarly here, with shipment_taxes.

By the way: We had a discussion earlier about the fact that for orders that are beyond the delivery step, it's actually preferable to have the shipment's manifest be taxed for contents rather than the line items. The shipments know where they will be shipped from, making correct taxation for multiple stock locations possible.

mamhoff · 2017-05-08T20:05:34Z

core/app/models/spree/tax_calculator/default.rb

+            id: item.id,
+            tax_rate: rate,
+            amount: rate.compute_amount(item),
+            included_in_price: rate.included_in_price


This should have the label thing as well. We want to abstract from the Spree::TaxRate, so rate should not be in here.

mamhoff · 2017-05-08T20:07:27Z

core/app/models/spree/tax/application.rb

@@ -0,0 +1,51 @@
+module Spree
+  module Tax
+    class Application


Adjuster, Performer, Applier, Applicator, OrderTaxer. Just throwing in ideas here, I second Johns thought.

mamhoff · 2017-05-08T20:09:36Z

core/app/models/spree/tax/application.rb

+        end
+
+        tax_adjustment ||= item.adjustments.new(
+          source: output_item.tax_rate,


Ah! This is why Spree::TaxRate still plays a role.

Adjustments need a source so they can be individually re-calculated. However, with any order-level, API-based taxation solution, this will never happen.

We might need to think about something like a Spree::Tax model, similar to adjustments but NOT adjustments.

While I agree that using adjustments for taxes is not ideal, I believe that is much too large of a change to make in this PR.

I might make it a thing for the Hack days to replace tax adjustments with a new thing. Do you mind adding a comment that the source is just there to satisfy the requirement of a source on adjustments, and not actually (in terms of business logic) required?

houndci-bot · 2017-05-09T23:47:16Z

core/app/models/spree/order_taxation.rb

+  # This class will create or update adjustments on the taxed items and remove
+  # any now inapplicable tax adjustments from the order.
+  class OrderTaxation
+


Extra empty line detected at class body beginning.

houndci-bot · 2017-05-09T23:47:17Z

core/app/models/spree/order_taxation.rb

+  # This class will create or update adjustments on the taxed items and remove
+  # any now inapplicable tax adjustments from the order.
+  class OrderTaxation
+


Extra empty line detected at class body beginning.

mamhoff

I like this. I still dislike the Spree::TaxRate is leaking into the abstraction, but understand that we can't do anything about it as long as we use adjustments for taxes. Incremental improvements for the win!

mamhoff · 2017-05-12T16:46:40Z

core/app/models/spree/order_taxation.rb

+    # @param [#adjustments] item a {Spree::LineItem} or {Spree::Shipment}
+    # @param [Array<Spree::Tax::TaxedItem>] rates a list of calculated taxes for an item
+    # @return [void]
+    def update_adjustments(item, rates)


Can we leave the rates out of the signature of this method, and rather attach them to the TaxedItem object? Because external services do not have actual rates.

It currently is an array of TaxedItem objects. This looks to just be a on old name that got left behind in one of my rebases. I'll update it.

jordan-brough · 2017-05-12T18:08:05Z

core/app/models/spree/app_configuration.rb

+    # @!attribute [rw] shipping_rate_tax_calculator_class
+    # @return [Class] a class with the same public interfaces as
+    #   Spree::TaxCalculator::ShippingRate
+    attr_writer :shipping_rate_tax_calculator_class


Could we remove this until we've had a chance to look at options on the interface a bit?

Do we want to remove both configuration points? Or just the shipping rate one?

Oops, yes, I meant both.

The PR looks really great! I'd love for us to merge it but it would be great to have a chance to collaborate (maybe even at SolidusConf?) on the interface before committing to it.

If that sounds OK could we perhaps also mark these two new classes as @api private for the moment, as we had done for the Tax::ItemAdjuster class?

I am becoming less convinced by the need for an "input"-type class, but we should absolutely discuss it at SolidusConf 😄 . I think marking it @api private for now is a good compromise.

WDYT @adammathys ?

Definitely. I've marked both classes and their configuration points with @api experimental. (Which is what we used for the tax_adjuster_class configuration point.) I also added in a note to each class explaining that they're still in development and that the input specifically is likely to change.

kennyadsl

This is awesome! 👏

kennyadsl · 2017-05-15T19:56:05Z

core/app/models/spree/tax_calculator/shipping_rate.rb

@@ -0,0 +1,43 @@
+module Spree


What about moving tax calculators folder into models/spree/calculator?

I didn't put the new tax calculators inside of models/spree/calculator because they aren't like the rest of the calculators in there. All of those calculators inherit from Spree::Calculator and as such have an already established interface.

The two new tax calculators share only the description of being a "calculator" with everything else in that namespace. Perhaps this is an indication that we should use a different name altogether for these new classes, because they are not meant to replace the existing calculators. (And the default implementations still use the Spree::Calculator::DefaultTax at the lowest level to compute the tax amount.)

jordan-brough · 2017-05-16T15:10:06Z

Commenting here to connect this to issue #1252, which is advanced nicely by this PR

jordan-brough · 2017-05-19T07:46:37Z

core/app/models/spree/order_taxation.rb

+      )
+
+      unless tax_adjustment.finalized?
+        tax_adjustment.update(amount: tax_item.amount)


If this fails where do we find out about it?

Ideally, this shouldn't ever fail. I'll update this to update_attributes! (update! is unfortunately overridden on adjustments) so that if it ever does it makes a noise.

jordan-brough · 2017-05-19T07:54:38Z

core/app/models/spree/order_taxation.rb

+        included: tax_item.included_in_price
+      )
+
+      unless tax_adjustment.finalized?


This is likely outside the scope of this PR (since the same thing happens in the old code as of v2.1) but is this really what we want? E.g. if an order is complete and I change the quantity (and thus price) of a line item, don't I need to update the VAT amount I have recorded for that item?

To me at least it doesn't seems clear that I need to go unlock tax adjustments before modifying completed orders. (Either via the Admin or some other way) Again, possibly outside the scope of this PR, but between v1.4 and v2.1 at least we did update taxes on completed orders, if I understand the code correctly.

I believe this is out of scope of this PR. The goal isn't to change any of the current behavior, just to add in a new extension point.

That being said, I agree that it doesn't make sense that taxes aren't recalculated on completed orders. I'll open a second PR to propose a reversion of that change.

👍 absolutely agreed. FYI I opened a PR just today actually: #1917

jordan-brough · 2017-05-19T07:59:38Z

core/app/models/spree/tax/taxed_item.rb

+    # @attr_reader [BigDecimal] amount the amount of tax applied to the item
+    # @attr_reader [Boolean] included_in_price whether the amount is included
+    #   in the items price, or additional tax.
+    class TaxedItem


Would ItemTax be a clearer name for this? (Same question for TaxedOrder)

jordan-brough · 2017-05-19T08:00:11Z

core/app/models/spree/tax/taxed_item.rb

+    # This generic object will hold the amount of tax that should be applied to
+    # an item. (Either a {Spree::LineItem} or a {Spree::Shipment}.)
+    #
+    # @attr_reader [Integer] id the {Spree::LineItem} or {Spree::Shipment} ID


How about item_id instead of id? (Same question for the id in TaxedOrder)

jordan-brough · 2017-05-22T13:12:12Z

core/app/models/spree/tax/item_tax.rb

+    # This generic object will hold the amount of tax that should be applied to
+    # an item. (Either a {Spree::LineItem} or a {Spree::Shipment}.)
+    #
+    # @attr_reader [Integer] id the {Spree::LineItem} or {Spree::Shipment} ID


s/id/item_id/ right?

Will be used as a configuration point for hooking in different methods of calculating tax. For example, using an external service versus tax rates stored in the database.

These very simple POROs will be usable by tax calculators to ensure they return a consistent result.

Uses the built-in tax rates to calculate the taxes for an order.

Will help consolidate the logic for applying taxes to line items and shipments. Making it easier to create third-party extensions.

Means we can remove the item adjuster since it's no longer used. This provides us an easy upgrade path for users while also ensuring anyone who has extended taxation using an adjuster won't have anything stop working.

Adds another simple integration point to make configuring tax calculations to use third-party services easier.

We haven't finalized the API yet and want to ensure any early adopters know this interface is still under development and likely to change.

jhawthorn · 2017-05-23T10:28:00Z

jhawthorn requested review from jordan-brough and mamhoff May 8, 2017 18:59

jhawthorn suggested changes May 8, 2017

View reviewed changes

mamhoff reviewed May 8, 2017

View reviewed changes

houndci-bot reviewed May 9, 2017

View reviewed changes

mamhoff approved these changes May 12, 2017

View reviewed changes

jordan-brough suggested changes May 12, 2017

View reviewed changes

kennyadsl reviewed May 15, 2017

View reviewed changes

jordan-brough mentioned this pull request May 16, 2017

Provide a way to use third party tax services #1252

Closed

jordan-brough reviewed May 19, 2017

View reviewed changes

jordan-brough mentioned this pull request May 22, 2017

Tax fix for finalized adjustments v2 #1936

Merged

jordan-brough approved these changes May 23, 2017

View reviewed changes

adammathys and others added 9 commits May 23, 2017 02:58

Add Config#tax_calculator_class

d5f98b8

Will be used as a configuration point for hooking in different methods of calculating tax. For example, using an external service versus tax rates stored in the database.

Add simple output structure for tax calculation

d592df2

These very simple POROs will be usable by tax calculators to ensure they return a consistent result.

Add default tax calculator

ed908ae

Uses the built-in tax rates to calculate the taxes for an order.

Add tax applicator model

f662f49

Will help consolidate the logic for applying taxes to line items and shipments. Making it easier to create third-party extensions.

Use new tax calculator in order adjuster

c4f640d

Means we can remove the item adjuster since it's no longer used. This provides us an easy upgrade path for users while also ensuring anyone who has extended taxation using an adjuster won't have anything stop working.

Add shipping rate tax calculator

4972cd9

Adds another simple integration point to make configuring tax calculations to use third-party services easier.

Mark the new tax calculators as experimental

9654cba

We haven't finalized the API yet and want to ensure any early adopters know this interface is still under development and likely to change.

Rename TaxedItem as ItemTax

c3b8590

Rename TaxedOrder as OrderTax

38004cb

jhawthorn approved these changes May 23, 2017

View reviewed changes

jhawthorn merged commit 6e8c953 into solidusio:master May 23, 2017

jordan-brough added the SolidusConf Hack Day label May 23, 2017

adammathys deleted the tax-calculator-configuration branch May 23, 2017 19:48

jhawthorn mentioned this pull request Feb 21, 2018

RFC: Higher level interface for applying promotions #2593

Closed

kennyadsl mentioned this pull request Sep 26, 2019

Officialize new taxation system #3354

Merged

3 tasks

New configurable tax calculator interface #1892

New configurable tax calculator interface #1892

Conversation

adammathys commented May 8, 2017 • edited Loading

TODO

jhawthorn left a comment

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

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

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

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

mamhoff left a comment

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mamhoff left a comment

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhawthorn May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jordan-brough commented May 16, 2017 • edited Loading

Choose a reason for hiding this comment

adammathys May 22, 2017 • edited Loading

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

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

jhawthorn commented May 23, 2017

adammathys commented May 8, 2017 •

edited

Loading

jhawthorn May 16, 2017 •

edited

Loading

jordan-brough commented May 16, 2017 •

edited

Loading

adammathys May 22, 2017 •

edited

Loading