RfC Make shipping rates taxable like line items or shipments #759
Conversation
|
On first pass this looks like a fantastic step forward. Nice call. Will review in depth when I get a minute. |
|
I think this works for a new shop. For existing shipping rates, this is a breaking change. However, things are mitigated by the fact that all order-relevant data is actually stored on the shipments. Do we need to migrate? If so: How would we do that? |
|
I really like this and I think it's a good direction, but I have some concerns we should think about when reviewing. As Martin points out, we'll need some sort of migration strategy to move over older shipping rates. This is the first time we've calculated and stored tax rates on an item which was not included in the order's total proper. This could have issues if a site did any reporting by counting adjustments through their To be clear, I think this is a good step forward, I just want extra caution as always with anything tax-related. |
|
As for the old shipping rates: We have to consider that old shipping rates might have been taxed using different tax rates, as they might have been deleted or updated in the meantime. This opens up another issue I've been hoping to be able to defer: How can we transparently handle changing tax rates? I.e.: A country raises their Sales tax by the beginning of, say, 2016... Ideally I'd have tax rates be immutable, and some kind of For deleted tax rates, which are It'd be a lot simpler to move that migration to the end of this branch, and not try stuffing it into the current situation, as there's lots of places where that might be necessary. |
|
As for the Given that the |
| it { is_expected.to respond_to(:pre_tax_amount) } | ||
| it { is_expected.to respond_to(:pre_tax_amount=) } | ||
|
|
||
| # TODO taxable items should not need a pre tax amount column |
There was a problem hiding this comment.
Annotation keywords like TODO should be all upper case, followed by a colon, and a space, then a note describing the problem.
The shared example group "a taxable item" spells out what API has to have to be tax adjusted using the item adjuster class.
This mostly consists in adding an attribute alias for the misnamed `cost` attribute to `amount`.
With the common API for shipments in place, this method can be simplified a lot. We also get rid of an unnecessary `.any?` check here.
Adds `included_tax_total`, `additional_tax_total` and `pre_tax_amount` to Spree::ShippingRates. `promo_total`, which would have also been possible, is implemented as a dummy setter and a getter that always returns zero. Shipping rates do not have promotions applied to them, that is what happens when they are used for a shipment.
Rather that calculating from only one tax rate on the fly, use Solidus' taxation system using adjustments to calculate tax amounts.
|
Rebased on current master, including the new shipping specs. |
| let(:original_shipment) { shipped_order.shipments.first } | ||
| let(:original_stock_location) { original_shipment.stock_location } | ||
| let(:original_inventory_unit) { shipped_order.inventory_units.first } | ||
| let(:original_variant) { original_inventory_unit.variant } | ||
| let(:shipping_method) { create(:shipping_method, tax_category: original_variant.tax_category)} |
Shipments are now correctly adjusted again in a large majority of cases. For the time being, the adjusting should happen in `Shipment`.
|
|
||
| it_behaves_like 'a taxable item' | ||
|
|
||
|
|
There is a concern over shipping rate adjustments messing up shop reporting - as those do not actually count towards the order total, but are actually more estimations of what's going happen if people use the `Spree::Order#all_adjustments` association, which the shipping rate adjustments would be part of. We had earlier discussed setting the `order_id` for those adjustments to `nil`, but that makes the shipping rate adjustments invalid. Setting the `order_id` on adjustments to `nil` would require the presence validation for the order to be conditional on something - in effect, touching the `Spree::Adjustment` class, which is not a beast I want to touch here. Now, there is a documented Boolean on adjustments that literally says: > 1. *eligible?* > > This boolean attributes stores whether this adjustment is currently > eligible for its order. Only eligible adjustments count towards the > order's adjustment total. This allows an adjustment to be preserved if > it becomes ineligible so it might be reinstated. While this was initially conceived for promotion adjustments, it can just as well be used for tax adjustments. This commit adds new instance methods on taxable items (Spree::LineItem, Spree::Shipment and newly Spree::ShippingRate) indicating whether they contribute to the order total. If they do, their adjustments surely must as well, and if they don't (as in the case of shipping rates), they answer `false` when asked: Are you `eligible?`. While this is not a cure-for-all (If your reporting does not honour the `eligible` bit, you'll still have wrong numbers in your tax reports), I think it provides the necessary distinguiability for reporting. Also, the complexity increase for the taxation system is small, and the adjustment system in general is not even touched. The order updater explicitly only calls adjustments on line items and shipments, or adjustments that have the order instance has adjustable when calculating the `adjustment_total`. `Spree::Order#all_adjustments` is only executed for re-calculating adjustment amounts.
|
I do not think we need to migrate, even for existing shops: Shipping rate adjustments actually do not show up anywhere except for the select box where customers choose a shipping method in the shipment step. I'm not sure we need a migration for this: If the order is Setting This would require patching |
|
In private conversation with @jordan-brough he voiced concerns about further cementing Likewise, other members of the community would like to see something like taxation system that is pluggable, and not even necessarily in core. #862 addresses some of the issues we currently have with shipping rates, and the refactoring of the These were supposed to be complementary, but can also be thought of different ways of achieving similar goals. @jordan-brough I'd still like you to look over this though; many thanks for yesterday's conversation! |
|
@mamhoff thanks for all the details! I've been looking at this today and am still wrapping my brain around everything. Will look some more and leave more thoughts tomorrow. |
|
I'm starting to be less terrified by this change :). Making the shipping rate adjustments eligible=false helps my confidence in safe upgrades a lot. A couple concerns still:
|
|
I'm actually less and less a fan of this. It introduces useless columns ( #862 showed me that this is possible entirely without adjustments, at the expense of quite a few database calls. I think I can find a solid middle ground with a new AR model Having |
|
Closing this in favor of #904. Much more fun. |
The taxation code for shipping rates is distributed over
Spree::ShippingRate,Spree::Stock::Estimatorand the worst parts ofSpree::TaxRate. The aim of the taxation refactoring I'm doing is to isolate taxation code to as few moving parts as possible. As discussed on Slack, I'm introducing a spec for taxable items so that I can easily test whether a shipment can possibly be tax adjusted using the tax adjustment mechanism we already have:Spree::Taxrate.adjust.