Fix issue in Spree 2.1 and bring Spree::Stock classes to OFN 🎉

[luisramos0]

What? Why?

Closes #5694 and #5690

I moved all the remaining Spree::Stock classes and respective tests to OFN under app/models/spree/stock and then moved them all to order_management engine so now they are neatly named OrderManagement::Stock::Package etc. There are quite a few good things about this:

• moving spree code to ofn
• moving services out of app/models into app/services
• moving code to specific domain engine
• keeping all the tests from spree
• remove spree complexity that is not needed in OFN - some work was done now, but there's more to remove, because we only have on stock_location we only have one package per order, so we can make more of these classes like Prioritizer work with a single package.

What should we test?

This was done in bulk (sorry dear reviewers!) so that we only have to test this once 👍
We need to validate order creation both on checkout and in the backoffice. Some of the tests I think we need to do here is to checkout with both variants on_demand and variants with on_hand values and then on the backoffice change/decrease these quantities and make sure that: 1. the fees on the orders are updated correctly and 2. the stock levels are correctly restocked. We should also validate the removal of a line item from the order and see that the stock levels of the respective variant is increased correctly.
We need to validate that issue 5694 is resolved.
We need to validate when we change the order and click "update and refresh..." in the backoffice, we need to check the order rates are updated correctly.
On the checkout process we should try a few variants, for example, checkout products with different shipping categories. We should verify we can checkout with a shipping method in a different zone from the order shipping address.

Another case I think it's worth verifying is the variant that goes out of stock while the user is checking out:

• make variant on_hand equal 3
• go up to the checkout page with quantity 3
• on a parallel session on the backoffice move the stock level to 2
• try to checkout, see redirect to cart
• change quantity to 2
• checkout
• go to admin, edit the order and decrease quantity to 1
• verify variant on_hand is now 1

Release notes

Changelog Category: Fixed
Fixed problem when changing order shipping method in the backoficce.
Moved quite a bit of stock management code from spree so we can gradually become 100% independent of spree.

[sauloperez]

This is awesome! it's already lowering complexity and this can only do us good!

Regarding the codeclimate issues, I'm in favor of approving the PR there and fix those in the upcoming PRs that remove the spree logic OFN doesn't need. Then we'll have a clear reason to invest in fixing those issues and it'll be easier to review with better-scoped PRs.

[luisramos0]

@sauloperez I made the change you asked and I added a change to "fix the fix" to #5694: the order adjustments were not being updated when needed. Now they are.

Re rubocop, I think we should have an exception for the "bye bye spree" effort, when bringing spree code to OFN it should not be mandatory to fix non trivial rubocop issues, specially because it will introduce risk to the PRs. I'd say long lines and similar are ok to fix but everything else like refactoring methods to reduce ABCSize, that should not be done when bringing code in as it will introduce too much risk imo.

[sauloperez]

Re rubocop, I think we should have an exception for the "bye bye spree" effort, when bringing spree code to OFN it should not be mandatory to fix non trivial rubocop issues, specially because it will introduce risk to the PRs. I'd say long lines and similar are ok to fix but everything else like refactoring methods to reduce ABCSize, that should not be done when bringing code in as it will introduce too much risk imo.

yes, I share your opinion.

[luisramos0]

Pau, I had a meaningful broken spec in the build that I had to fix.
The important commit is this one:
07a44cf

We need to make sure that if refresh_rates changes the ship method (even if it's a new ship method and before there was none) the shipment gets saved so that the callbacks (shipment.ensure_correct_adjustment and order.update!) are called. This was not being done before, it looks good now 👍

ADDED: more refactoring could be done here like extracting refresh_rates to a service but I am leaving as is for now.

[luisramos0]

And finally, I found why this problem was not detected in the build. There is a spec to verify exactly it but the final assertion in the test was not correct. I added a commit to fix bug in the test.

[Matt-Yorkley]

💪

[Matt-Yorkley]

Exciting stuff! 😄

[filipefurtad0]

Hey @luisramos0 ,

I see a few issues on codeclimate, with different warnings - should I go ahead and test this one? Or best to solve these before proceeding? Thank you!

[luisramos0]

Hey Filipe, it's discussed above. Rubocop and codeclimate are equivalent in this context.
You can always ignore code climate, it's used for code review.
Please go ahead! #exciting

[filipefurtad0]

Hi @luisramos0 ,

Thank you so much for the detailed test cases!

To test this, I've set different enterprise fees (included in the price), shipping fees, VAT, and payment method fees.

Orders placed on the backoffice render the same final value, when compared to orders placed in the shopfront. Also, removing items works in the same way - both shopfront vs. BO - render the same values, after adjustments (adding/removing items, adding/removing line items): fees are correctly re-calculated and both have the same and correct effect on the remaining stock.

Changes on the stock during the customers shopping journey have the expected effect as well.

Going to your notes, step by step, below:

• on_demand and variants with on_hand values and then on the backoffice change/decrease these quantities and make sure that:

1. the fees on the orders are updated correctly OK
2. the stock levels are correctly restocked. OK

• removal of a line item from the order correctly increases the stock levels of the respective variant OK
• Issue 5694 is resolved OK
• changing the order and clicking "update and refresh..." in the backoffice, we need to check the order rates are updated correctly. OK
• checking out products with different shipping categories OK
• checkout with a shipping method in a different zone from the order shipping address OK

Verified the procedure below as well - all good:

• made variant on_hand equal 3
• checked out with quantity 3
• on a parallel session on the backoffice decrease the stock level to 2
• attempted checkout -> redirected to cart
• change quantity to 2 and checking out
• on admin, edited the order and decreased quantity to 1
• verified that the on_hand is now 1

As admin, also made changes on stock levels, while the customer is on the shopfront, triggering:

This is ready to go! 🎉

[luisramos0]

nice testing Filipe, thanks a lot.

I am merging this 🎉 🎉 🎉