Deprecate order tax zone #1543
Deprecate order tax zone #1543
Conversation
mamhoff
force-pushed
the
deprecate-order-tax-zone
branch
from
a336321 to
7b3f26e
Compare
This commit removes call to `Spree::Order#tax_zone` from the codebase. These calls are problematic, because whenever we have to check whether a zone includes another zone entirely, we have to iterate over all of both zones members. Really, it's enough to check whether the order's tax address is within the desired zone (for a taxation or shipping zone). This also comes with a number of fixes in specs where calls to `order.tax_zone` were abused for test setup.
mamhoff
force-pushed
the
deprecate-order-tax-zone
branch
from
7b3f26e to
d5320e3
Compare
There was a problem hiding this comment.
Great stuff. Like how it simplifies things.
I only have some nit picks about lots of law of Demeter violations (only talk to your immediate neighbor). But as you mostly doing a search and replace here, we can address this in a later PR. But I saw to many "undefined method yada for NilClass" errors in my life, because of expectations made on the objects being correctly build. They're often enough not.
Can be merged in my opinion
|
|
||
| @order = Spree::Order.create!(store: store) | ||
| allow(@order).to receive_messages coupon_code: "10off" | ||
| expect(order).to receive(:tax_address).at_least(:once).and_return(Spree::Tax::TaxLocation.new(country: zone.countries.first)) |
There was a problem hiding this comment.
Haha. Great. Much better to read and understand 👏🏻
| @@ -141,7 +141,9 @@ | |||
| end | |||
|
|
|||
| it "shows correct tax amount" do | |||
There was a problem hiding this comment.
I like to use aggregate_failures when using multiple expectations
|
Without |
|
@jrochkind An order's shipping address might actually match several zones with tax rates attached. It's far easier to check which zones have the state or country of the shipping address, and then get the tax rates for them. The way this worked before was: We run |
|
Thanks Martin. This is much more sensible. Looking forward to the follow-up removal of |
This PR is two commits which are extracted from #1537, so that the Giant Removal is hopefully easier to understand.
First, its a fix to
promotion_handler/coupon_spec, which had weird spec setup in places that led to hard-to-debug errors. Second, it's deprecatingSpree::Order#tax_zone, which is conceptually a terrible idea and rather expensive to use.With the deprecation, I can also safely drop extra behaviour to
Spree::Order#reload. Yay!