Connect shipping rates to shipments earlier in Stock::Coordinator #965
Conversation
|
❤️ Can you look at the last failing test, and then I'll rebase #904. |
If this is the case we need a spec for that |
|
@mamhoff & @jhawthorn updated. Look good? I don't love the spec I added but lmk what you think. Any ideas are definitely welcome. |
|
I tried rebasing this onto #904. My work then breaks a gazillion other things, and I think the ugly spec and this failure are related somehow: https://circleci.com/gh/solidusio/solidus/2473. I'm not so sure how to remedy this, but essentially our spec setup/our factories are set up in such a way that with this change AND my changes in, 700 specs break because apparently lots of shipping rates have no shipment when they're created. 😿 |
|
@mamhoff it's this commit where things fall apart right? I'm happy to try to help take a look when I have a few minutes. I think it'd be very good to avoid the Re the spec I added in 86e4dd8: I think it's a not-great spec because really it's testing internal implementation rather than the public API of the class. There's probably a better way to approach a spec but I'm not sure if there's a pragmatic one. I'll think about that some more when I have a minute (open to ideas!). |
|
FYI I chatted briefly with @mamhoff on this and I'm going to take some time tomorrow morning to code up a fix and improve the specs. |
| @@ -55,15 +56,16 @@ def calculate_shipping_rates(package) | |||
| end | |||
|
|
|||
| def shipping_methods(package) | |||
| # TODO: Try changing this to shipment.address | |||
There was a problem hiding this comment.
Incorrect indentation detected (column 10 instead of 8).
jordan-brough
force-pushed
the
add-rates-to-shipments-earlier
branch
2 times, most recently
from
6217608 to
79b2914
Compare
|
@mamhoff @jhawthorn I've added a couple of commits which broaden the scope of this PR a bit but I think it's worth it. Thoughts? These changes fix the errors that @mamhoff's commit was seeing. There is some change to the public API so I've added a changelog for that. @mamhoff you'll want to omit this change to |
|
👍 |
This will give the estimator the information it needs to calculate taxes correctly in the pending tax work.
- The package passed to Spree::Stock::Estimator#shipping_rates must have its shipment assigned and that shipment must have its order assigned. This is needed for some upcoming tax work in to calculate taxes correctly. - Spree::Stock::Estimator.new no longer accepts an order argument. The order will be fetched from the shipment.
This was being done immediately after calling #to_shipment and it makes more sense to do it inside #to_shipment.
jordan-brough
force-pushed
the
add-rates-to-shipments-earlier
branch
from
79b2914 to
2842412
Compare
|
I said so before in real life, but this is a change that will make my life a lot easier. Thank you @jordan-brough! |
|
Great work @jordan-brough 👍 |
|
Thanks everyone! |
…lier Connect shipping rates to shipments earlier in Stock::Coordinator
This will give the estimator the information it needs to calculate
taxes correctly in the pending tax work.
See comments in #904 (diff)