Provide a way to use third party tax services

[jordan-brough]

There is no good way to use third party tax services with Solidus. All solutions and extensions that the core team has seen so far are very hacky. This is because Solidus does not provide a good extension point for third party tax systems. We want to fix that.

One of the primary problems is that Solidus calculates taxes on a line-by-line basis, and third party tax systems generally calculate taxes on an entire order at once. This means that for a order with 5 line items, for example, Solidus will trigger tax calculation 5 times. If you submit the entire order to your third party tax service 5 times then 1) it will be fragile, 2) it will be slow, 3) some services (including Avatax) charge for every API call, so it may be costly.

One approach that some have tried is to attempt to cache api calls to the third party provider. However, this doesn't work in a scenario like this:

1. Start updating an order, e.g. applying a promo
2. Update line item 1, trigger tax calculation
3. Update line item 2, trigger tax calculation -- the totals for the order have now changed, so you cannot use the previously cached results
4. Update line item 3, ...
   and so on.

Here's an example spec that demonstrates this problem:
https://github.com/jordan-brough/solidus/blob/d92c5b5/core/spec/tax_spec.rb

[jordan-brough]

An idea from our last core team meeting:

Remove these two lines from ItemAdjustments:

@item.included_tax_total = tax.select(&:included?).map(&:update!).compact.sum
@item.additional_tax_total = tax.reject(&:included?).map(&:update!).compact.sum

And add another line right below this call to recalculate_adjustments in OrderUpdater that is something like recalculate_taxes. Also, make an extension point for swapping out the logic, e.g.:

Spree::Config.order_taxes_calculator_class.new(order).calculate_taxes

Possible problem: Anything using ItemAdjustments outside of OrderUpdater may be depending on ItemAdjustments to calculate taxes.

[jrochkind]

I think in the front-ends we've done where I am, we don't even show tax until the final 'confirm' step.

Does default solidus frontend actually show tax in the cart? it would be interesting to do a survey of major e-commerce sites (including European-audience-focused ones) and see how many do, at what point they show tax, and if they show it line-item-specific.

I think tax still needs to be (at least optionally) calculated, and ideally stored, on a line-by-line basis. This came up in the slack channel recently where someone was explaining how some U.S. states charge sales tax on only certain categories of products, and buy-something-get-something-else-free promotions complicate this further, especially when the two products are in different sales tax categories, and different states have different rules for how much sales tax is owed in that situation.

I think the solution is probably just refraining from automatically calculating/assigning tax when a line item is added to an order/cart, but instead having an explicit calculate_and_assign_sales_tax method/hook on an order, which is called at certain parts of the checkout process -- ideally easily customized as to which steps of the add-to-cart and checkout process call the method/hook.

[jordan-brough]

Does default solidus frontend actually show tax in the cart?

Yes:

it would be interesting to do a survey of major e-commerce sites

I actually did this once (six or so of the ones close to our field) and all of the ones I looked at only showed tax on the confirm step.

I think in the front-ends we've done where I am, we don't even show tax until the final 'confirm' step.

That's been my experience as well. But I know I've heard from others (@gmacdougall possibly?) that there are stores that want to show it at every step.

I think tax still needs to be (at least optionally) calculated, and ideally stored, on a line-by-line basis

stored yes, calculated no. Or rather, taxes do need to be calculated at the line level, but the whole order needs to be submitted to the third party tax service at the same time. (And then the results get stored at the line level)

having an explicit calculate_and_assign_sales_tax method/hook on an order

Agreed -- this is what the "idea from our last core team meeting" is suggesting. The customized calculator could hopefully decide at what states it should recalculate taxes?

[jrochkind]

Right on.

The customized calculator could hopefully decide at what states it should recalculate taxes?

I'm not sure if it makes sense for the calculator to decide this. I'd think ideally the calculator would simply calculate, and whatever was calling the calculator would decide when to call it and assign it's results to models.

But that may not fit into the current architecture as well, as far as supporting the customization needed. But i think it's a confusion of responsibility to have the calculator deciding when to calculate. The decision of when to calculate has to do with your UI, when you need to show tax to the user -- the calculator should not be concerned with or aware of such, or have to be modified when your UI choices change, the calculator should just be concerned with calculating.

[jordan-brough]

I'm not sure if it makes sense for the calculator to decide this

Good point

But that may not fit into the current architecture as well

Yeah, it's hard because there aren't any robust "stages" in Solidus right now. E.g., we could support something like before_transition to: :confirm, do: :caculate_taxes, but anything can really happen in any state of the state machine, especially if you are using the api and not frontend, so you might be in confirm already and then apply a promo (or even add an item) and there would be no state transition to depend on for calculating taxes. And there's also the issue of modifications to an order after it's been completed.

[jrochkind]

Hmm, could it be in the OrderUpdateAttributes object? If it's updating order attributes (or only if certain attributes have changed) and the order is in a certain state, then do a tax (re)calculation?

Inside the OrderUpdateAttributes would be easier to customize based on config (or subclass, delegate, etc), than the monstrous state machine.

You're already talking about having OrderUpdateAttributes trigger the recalculation, right, it's just a question of what/where decides if a recalc needs to happen (which may depend on how 'expensive' the tax calc is, as well as what UI you are using, which you may or may not have chosen based on expense of the calc, heh). I think it'd be better to plan for such customization being done in the OrderUpdateAttributes object, instead of assuming a TaxCalculator will decide it. I should be able to change when/how often it gets called, if needed, without having to rewrite or modify or customize the tax calculator, the only reason a tax calculator should need to be changed is if the tax calculation computations or third-party integration have changed.

And there's also the issue of modifications to an order after it's been completed.

I have to admit I've never understood the use case for changing an order's contents or totals/amounts after it's completed, I've thought it would be better if Spree didn't even allow it! But if you're talking about tax calc being triggered by the OrderUpdateAttributes obj in either case, this shouldn't make a difference I think. (Typing that a couple times, makes me think it really ought to have been called OrderAttributeUpdater, heh)

[jordan-brough]

could it be in the OrderUpdateAttributes object?

Theoretically that could be great, but OrderUpdateAttributes isn't used in a lot of important places, so I don't think we can rely on it. We could add it there and also in a lot of other places, but there's a good chance that'll be a breaking change for a lot of people since in their own custom code they're currently relying on OrderUpdater & etc to trigger tax recalculation. Going in that direction might be great long term but it seems hard in the near term.

I should be able to change when/how often it gets called, if needed, without having to rewrite or modify or customize the tax calculator

Agreed, but I'm not sure there's a reasonable alternative in the near term. We could offer something specific like Spree::Config.decide_whether_to_calculate_taxes_class, if that made it slightly less painful in the short term, though I expect inheriting from a built-in tax calculator might be just as easy.

[jrochkind]

Ah, yeah, I understand existing architecture constrains.

I understood that in what was being proposed by jordan-brough, you'd miss out on tax calcs unless you used the OrderUpdateAttributes anyway already. If that's not an acceptable situation (I am not sure), then jordan's proposal doesn't work, right? If it is, then it becomes okay to plan on tax calculation points being customized via extension points in the OrderUpdateAttributes, not in a tax calculator. I think?

[jordan-brough]

@jrochkind ah, perhaps I misunderstood what you said. I'm talking about modifying OrderUpdater, not OrderUpdateAttributes. Is that what you mean? Yes, that is a great point that the decision logic (and customization of the decision) could be in OrderUpdater, separate from the calculating logic (and customization).

[jordan-brough]

Possible problem: Anything using ItemAdjustments outside of OrderUpdater may be depending on ItemAdjustments to calculate taxes.

Places in Solidus that invoke ItemAdjustments outside of OrderUpdater

Adjustment creation or deletion:

Update: This has been removed by: #1389

class Adjustment
  after_create :update_adjustable_adjustment_total
  after_destroy :update_adjustable_adjustment_total

☝️ This shouldn't be a problem for tax adjustments, but it could be a problem for other adjustments -- e.g. if a promo modifies the amount on an item then we need to recalculate taxes. We'd have to rely on order.update! being called after the adjustment was created. That should happen, but it seems like a possible problem spot for existing stores & code.

During apply_free_shipping_promotions:

Update: This has been removed by: #1400

class Order

  before_transition from: :delivery, do: :apply_free_shipping_promotions

  def apply_free_shipping_promotions
    Spree::PromotionHandler::FreeShipping.new(self).activate
    shipments.each { |shipment| ItemAdjustments.new(shipment).update }
    updater.update_shipment_total
    persist_totals
  end

☝️ We could call updater.update instead of updater.update_shipment_total+persist_totals to ensure that shipment taxes got recalculated.

After adding or removing items:

Update: This has been removed by: #1400

class OrderContents
  def after_add_or_remove(...)
    ...
    ItemAdjustments.new(line_item).update
    reload_totals

☝️ reload_totals there invokes order_updater.update so that should already be OK.

After changing the quantity on line items:

Update: This has been removed by #1356

class LineItem

  after_save :update_adjustments

  def update_adjustments
    if quantity_changed?
      update_tax_charge # <- I think we can get rid of this...
      recalculate_adjustments
    end
  end

  def recalculate_adjustments
    Spree::ItemAdjustments.new(self).update
  end

☝️ This should be ok if all the triggering code calls order.update! afterward (like OrderContents#add does, for example) which is likely to be true but probably dangerous to assume.

After shipping cost changes:

Update: This has been removed by #1356

class Shipment < Spree::Base

  after_save :update_adjustments

  def update_adjustments
    if cost_changed? && state != 'shipped'
      recalculate_adjustments
    end
  end

☝️ Same thing as for LineItem.

Update: Another one found by @jhawthorn:

In Shipment#update_amounts:

PR: #1435

class Shipment < Spree::Base
    def update_amounts
      if selected_shipping_rate
        self.cost = selected_shipping_rate.cost
        self.adjustment_total = adjustments.additional.map(&:update!).compact.sum

[jrochkind]

Ah, it was me that was confused, I didn't realize/forgot the two things are different. I don't really entirely understand what's going on -- is OrderUpdater triggered from the front-end CheckoutsController? (That's where I saw the OrderUpdateAttributes being used; why is it using OrderUpdateAttributes instead of OrderUpdater? Is OrderUpdater still triggered at some point by frontend checkouts controller?)

Anyway, yeah, if that works, great. You understand the arch better than me, I'm happy with any plan that plans for extension/customization points as to when tax calculation happens somewhere that's part of general order updating, not a tax calculator.

[jordan-brough]

is OrderUpdater triggered from the front-end CheckoutsController?

I took a look:

• OrdersController#update (frontend) and Api::OrdersController#update use OrderContents#update_cart, which triggers OrderUpdater#update
• I can't see anywhere where CheckoutController#update (frontend) or Api::CheckoutsController#update trigger OrderUpdater#update. There may be a possible bug in there somewhere (more likely for Api usage than Frontend usage). Those controllers don't permit adding/changing line items at least, but they do allow changing at least ship_address, which should trigger a tax recalculation.

[jasonfb]

(speaking big-picture here) It seems that the transition hook stuff we're never really going to get around as long as the order has state transitions before being complete, and that a sensible approach is to simply recalculate totals inside of the Updater as suggested. I know that our code relies heavily on the Updater doing its job, and we know that whenever any total might change we must call the Updater.

As far as the lack of OrderUpdater#update being called in the CheckoutController#update -- moving in this direction would make most sense (and figuring out how to make the notorious @order.next easier to debug). It seems to me that until the state transition problem is tackled, calling the Updater explicitly is a fine expectation. As long as it is in the release notes clearly I do not thing we need to be overly concerned with supporting existing implementations that fail to call the updater correctly after a total has changed, which probably already have bugs due to their existing implantation.

Finally, the tax gem we use is boomerdigital/spree_avatax_certified. As you discussed above, it submits the whole order (in XML I think), including individualized itemization of each lineitem, in a single transaction (that, yes, is resubmitted when thing change).

Yes, because items can cross tax categories, the taxes need to be calculated on a line-by-line basis

[jordan-brough]

An update: Thanks to help from @jhawthorn the problem spots listed here have all been addressed. I'm now working on a PR to add extension points for tax calculation that will be usable by 3rd party tax services.

[jhawthorn]

@jordan-brough I think we've made huge progress here. What in your opinion is needed to close this.

It would be great if we had a example of what a very basic tax plugin would look like.

[jordan-brough]

PR #1892 from @adammathys gets us nearly there I think. After that I'd just like us to finalize what the configurable interface should be exactly and call this done.

[jhawthorn]

Considering this fixed by #1892 and similar work 🎉