Add a PORO for creating tax adjustments #685

mamhoff · 2016-01-12T17:15:23Z

Adds a Spree::Tax::ItemAdjuster, which adjusts an item, and a Spree::Tax::OrderAdjuster, which iterates over line items and shipments in an order and adjusts those.

This PR is a prerequisite for any further taxation work.

mamhoff · 2016-01-12T17:53:12Z

Force-pushed with no changes because of spurious failure in admin shipment spec.

jhawthorn · 2016-01-14T22:48:00Z

This does seem like a good target for extraction to a PORO, but the handling of the two cases (receiving either an item or an order) is a little awkward. The two cases share very little code.

Maybe we could adjust the signature of the adjuster? Brainstorming a little:

class Adjuster
  def initialize(order)
    @order = order
    @tax_zone = order.tax_zone
  end

  def adjust_order
    # adjust all items in the order
  end

  def adjust_item(item)
    # adjust just the item
  end
end

tvdeyen · 2016-01-14T23:27:28Z

One adjuster for two different things? It seams the only thing they share is the order tax zone.

Why not an OrderAdjuster and a LineItemAdjuster, both knowing the order tax zone but having separated concerns?

Sandi Metz is calling! Excuse me ;)

tvdeyen · 2016-01-14T23:31:14Z

There is too much knowledge of the two possible callers inside one class. Case statements on class types, then methods named after the caller, etc.

Two separate adjusters are much more flexible, more future safe and less error prone. If we use POROs than please let us use clean object oriented design.

Sorry, reading too much Sandi Metz lately.

Just my 0.02

mamhoff · 2016-01-15T14:41:57Z

Thank you for chipping in!

@jhawthorn Actually, they do share code, as the order adjuster doesn't actually adjust the order, but all the items (line items and shipments) contained within it.

The current stuff on TaxRate shaves some AR requests by only loading the potentially_applicable rates once per order - I would like that efficiency to stay, which is why I didn't separate the two into their own classes. @tvdeyen I hear you though. Possibly I can have the best of both worlds by making the LineItemAdjuster a Singleton and cache things like the tax rates in an instance variable.

At the moment, Spree::TaxRate.adjust is a method that is executed for side effects (create adjustments, update pre_tax_amount), which isn't super transparent. Ideally, I'd like something to this effect:

class Spree::LineItem
  has_many :adjustments, ... , autosave: true
  before_save :adjust

  private 

  # builds tax adjustments and sets pre tax amount, but doesn't actually save
  def adjust
    self = Spree::Tax::ItemAdjuster.new(self).adjust!
  end
end

mamhoff · 2016-01-16T18:13:35Z

Changed these to single responsibility objects - I do think we can deal with the efficiency after we've defined the interfaces.

tvdeyen · 2016-01-16T20:32:15Z

core/app/models/spree/order.rb

-      order_tax_zone = self.tax_zone
-      Spree::TaxRate.adjust(order_tax_zone, line_items)
-      Spree::TaxRate.adjust(order_tax_zone, shipments) if shipments.any?
+      Spree::Tax::Adjuster.new(self).adjust!


Don't you want to call OrderAdjuster here?

Absolutely. Thanks for the heads-up! 💯

mamhoff · 2016-01-19T20:55:07Z

Wheew! Green! Removing TaxRate.adjust, which is a monster, has not been that hard after all. I'm excited: I like the API, and because of some data sharing between the ItemAdjuster and the OrderAdjuster the expensive TaxRate.match call is only done once.

This is still lacking feedback. Once that's in, I'd start putting in unit tests and moving the Spree::TaxRate into some kind of taxation_integration_spec.rb file.

Please please, this isn't ready for merge, but definitely ready for feedback (although you do have a couple of days as I'll be out until the weekend from tomorrow).

mamhoff · 2016-01-23T16:45:40Z

Rebased on master, updated commit message on second commit. Feedback still welcome.

Btw: With #530 in, I'll take a go at removing TaxRate.match, too.

mamhoff · 2016-01-26T13:59:16Z

These last ones failed because of the Rails update (#736). Rebasing on master now, and adding a couple of performance fixes as well.

mamhoff · 2016-01-26T14:29:49Z

The build failed because of phantomjs crashing :(

mamhoff · 2016-01-26T14:33:44Z

Force-pushed to get a green build. Otherwise: This is ready for review.

I can think of more stuff to do, but the scope of this PR is already quite large, so I'll stop for now. :)

peterberkenbosch · 2016-01-26T14:38:58Z

👍 Nice @mamhoff

mamhoff · 2016-01-26T16:05:34Z

Damn. The shipping rates also have VAT, and they're not price-adjusted yet.

tvdeyen · 2016-01-27T08:24:42Z

I hopefully can spent some review time later this evening (CET)

mamhoff · 2016-01-27T14:36:22Z

This is really long, and hard to review. I'll split this PR in two: One that just adds the POROs and uses #530 to look up zones (that's all commits up to a68d9c8), and one that actually implements MOSS with that.

The one that actually implements MOSS will also have to deal with shipping rates, somehow.

tvdeyen · 2016-01-27T18:31:23Z

Alright

tvdeyen · 2016-01-27T19:44:57Z

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

+
+      def initialize(item, options = {})
+        @item = item
+        @order = @item.order


@item and @order should be attr_readers, so the TaxHelper module and potential sub classes don't rely on reading instance variables.

mamhoff · 2016-01-28T17:01:38Z

Did the naming changes, and also added some more tests to the adjusters.

For the time being, these two POROs (one for orders, another for items) call the already defined methods on `Spree::TaxRate`. The idea is, though, that the responsibilities between `Spree::Tax::{Item,Order}Adjuster` and `Spree::TaxRate` are divided as follows: `Spree::TaxRate` stores and selects rates, while `Spree::Tax::{Item,Order}Adjuster` create tax adjustments.

mamhoff · 2016-01-28T17:21:42Z

First commit was failing because of a naming change, rebased again.

The code in `Spree::TaxRate.adjust!` is hard to understand, and the adjusting should happen in dedicated adjuster classes. This commit ports the functionality of `TaxRate.match` over to two classes: * An `OrderAdjuster` which iterates over all items (line items and shipments) in an order and runs the * `ItemAdjuster`, which actually creates adjustments for every item. The code should read a lot easier now. In order to reduce database load, the tax rate array for an order can be passed into the `ItemAdjuster.new`. Considering the performance benefits, I think that's justifiable amount of shared knowledge between the two Adjuster classes.

TaxRate.for_zone will return all tax rates that are valid for a given zone. It does not care about the default VAT zone, as that should be handled by the Adjuster classes.

When using .with_shared_members, sometimes you pass nil - and that should just yield an empty ActiveRecord relation. Same goes for Spree::TaxRate.for_zone(nil).

This is really an intermediary step. I'm using for_zone already in Spree::TaxRate.match, which should result in a constant eight DB calls per .match call. Previously, the number of DB calls would depend on how many tax rates there are, and how these are configured (as Spree::Zone.contains? would be run for every tax rate in the system). A side effect of this refactoring is that the reimbursement specs had to be amended a little: The zones had to be created `:with_country`, because otherwise there'd be no shared members and the tax rates wouldn't apply. This actually makes the tests slightly more realistic (what is a zone without members, anyway?). Also, I had to change an expectation: The included tax total of the reimbursement was previously expected to be negative (which is something that IMO should never happen, as negative included taxes at the moment become positive additional taxes).

The order adjuster and the item adjuster share code, because I need to run some methods on the order level and cache them. This extracts these methods into a module. Inheritance would've also worked, but in this case I find a mixin more fitting as the behaviour of the two classes is still quite different.

TaxRate.match does some pretty weird things, and I want to change them. This commit moves that code to a temporary `applicable_rates` helper.

gmacdougall · 2016-02-02T17:57:22Z

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

+        @item = item
+        @order = @item.order
+        # set the instance variable so `TaxRate.match` is only called when necessary
+        @rates_for_order_zone = options[:rates_for_order_zone]


I'm not clear where this is being used.

These are used in private methods of the included module TaxHelpers, in order to run the database calls that only have to run once per order only once per order: https://github.com/magiclabs/solidus/blob/add-tax-adjuster/core/app/models/spree/tax/tax_helpers.rb#L28-L38.

jhawthorn · 2016-02-02T22:17:57Z

Looks good to me. 👍

One thought, should we keep TaxRate.adjust as a deprecated method that would use the PORO?

mamhoff · 2016-02-02T22:31:00Z

Well, the method signatures are a bit different. I could use just the item adjuster, but then that method will have abysmal performance. I'd rather have it entirely gone. However, if there's another person saying: Deprecate it and be nice about it - I'll do it, along with the requested YARD docs tomorrow.

Add Spree::TaxRate.adjust as a deprecated method, and test for the deprecation warnings. The deleted specs are covered in the specs for the ItemAdjuster.

Add a PORO for creating tax adjustments

tvdeyen · 2016-02-03T17:02:01Z

🍦

tonobo · 2016-02-04T19:09:44Z

Nice ;)

mamhoff mentioned this pull request Jan 12, 2016

WIP: Remove TaxRate.match #536

Closed

tvdeyen reviewed Jan 16, 2016
View reviewed changes

cbrunsdon mentioned this pull request Jan 25, 2016

Extendable adjustable updater #499

Closed

mamhoff changed the title ~~WIP / RfC: Add a PORO for creating tax adjustments~~ RfC: Add a PORO for creating tax adjustments Jan 26, 2016

mamhoff mentioned this pull request Jan 27, 2016

WIP MOSS refactoring #747

Closed

tvdeyen reviewed Jan 27, 2016
View reviewed changes

mamhoff added 6 commits January 28, 2016 20:41

Add TaxRate.for_zone scope

b24c9cb

TaxRate.for_zone will return all tax rates that are valid for a given zone. It does not care about the default VAT zone, as that should be handled by the Adjuster classes.

Allow passing nil into Spree::Zone.with_shared_members

49b8aa2

When using .with_shared_members, sometimes you pass nil - and that should just yield an empty ActiveRecord relation. Same goes for Spree::TaxRate.for_zone(nil).

Move TaxRate.match logic to TaxHelpers

d5e15d0

TaxRate.match does some pretty weird things, and I want to change them. This commit moves that code to a temporary `applicable_rates` helper.

mamhoff changed the title ~~RfC: Add a PORO for creating tax adjustments~~ Add a PORO for creating tax adjustments Feb 2, 2016

gmacdougall reviewed Feb 2, 2016
View reviewed changes

mamhoff added 2 commits February 3, 2016 11:43

Add YARD documentation to Tax Adjuster classes

11a9b3d

Reintroduce TaxRate.adjust as a deprecated method

52ca997

Add Spree::TaxRate.adjust as a deprecated method, and test for the deprecation warnings. The deleted specs are covered in the specs for the ItemAdjuster.

tvdeyen pushed a commit that referenced this pull request Feb 3, 2016

Merge pull request #685 from magiclabs/add-tax-adjuster

dac756c

Add a PORO for creating tax adjustments

tvdeyen merged commit dac756c into solidusio:master Feb 3, 2016

mamhoff mentioned this pull request Mar 16, 2016

Solidus VAT Roadmap #989

Closed

23 tasks

mamhoff deleted the add-tax-adjuster branch May 24, 2016 19:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a PORO for creating tax adjustments #685

Add a PORO for creating tax adjustments #685

mamhoff commented Jan 12, 2016

mamhoff commented Jan 12, 2016

jhawthorn commented Jan 14, 2016

tvdeyen commented Jan 14, 2016

tvdeyen commented Jan 14, 2016

mamhoff commented Jan 15, 2016

mamhoff commented Jan 16, 2016

tvdeyen Jan 16, 2016

mamhoff Jan 16, 2016

mamhoff commented Jan 19, 2016

mamhoff commented Jan 23, 2016

mamhoff commented Jan 26, 2016

mamhoff commented Jan 26, 2016

mamhoff commented Jan 26, 2016

peterberkenbosch commented Jan 26, 2016

mamhoff commented Jan 26, 2016

tvdeyen commented Jan 27, 2016

mamhoff commented Jan 27, 2016

tvdeyen commented Jan 27, 2016

tvdeyen Jan 27, 2016

mamhoff commented Jan 28, 2016

mamhoff commented Jan 28, 2016

gmacdougall Feb 2, 2016

mamhoff Feb 2, 2016

jhawthorn commented Feb 2, 2016

mamhoff commented Feb 2, 2016

tvdeyen commented Feb 3, 2016

tonobo commented Feb 4, 2016

Add a PORO for creating tax adjustments #685

Add a PORO for creating tax adjustments #685

Conversation

mamhoff commented Jan 12, 2016

mamhoff commented Jan 12, 2016

jhawthorn commented Jan 14, 2016

tvdeyen commented Jan 14, 2016

tvdeyen commented Jan 14, 2016

mamhoff commented Jan 15, 2016

mamhoff commented Jan 16, 2016

tvdeyen Jan 16, 2016

Choose a reason for hiding this comment

mamhoff Jan 16, 2016

Choose a reason for hiding this comment

mamhoff commented Jan 19, 2016

mamhoff commented Jan 23, 2016

mamhoff commented Jan 26, 2016

mamhoff commented Jan 26, 2016

mamhoff commented Jan 26, 2016

peterberkenbosch commented Jan 26, 2016

mamhoff commented Jan 26, 2016

tvdeyen commented Jan 27, 2016

mamhoff commented Jan 27, 2016

tvdeyen commented Jan 27, 2016

tvdeyen Jan 27, 2016

Choose a reason for hiding this comment

mamhoff commented Jan 28, 2016

mamhoff commented Jan 28, 2016

gmacdougall Feb 2, 2016

Choose a reason for hiding this comment

mamhoff Feb 2, 2016

Choose a reason for hiding this comment

jhawthorn commented Feb 2, 2016

mamhoff commented Feb 2, 2016

tvdeyen commented Feb 3, 2016

tonobo commented Feb 4, 2016