Fix order state with customer returns when receiving return items #3483

elia · 2020-01-22T10:35:17Z

📝 Notes on reviewing: I suggest to review this PR commit by commit, knowing that the core of the fix is in the last commits:

Let Order#all_inventory_units_returned? always use fresh data (88b1220)
Which fixes the check on the Order state transition toward "returned".
Let the order transition to Returned after return items are all Received (6228fe9)
This one contains the feature specs demonstrating the problem along with the changes that make them pass.
Deprecate skip_customer_return_processing (0dd92fd)
Walk back the attempted fix introduced by Allow order with multiple line items to be marked as "Returned" #3199.

Description

Why?

Currently when a customer return transitions from "In transit" toward "Received" doesn't trigger the transition of the Order status to "Returned".

How

The problem is a composite of multiple bugs.

The first one was that order.inventory_units was memoized with completely different instances from ReturnItem#inventory_unit. So the check to see if they were all returned was, at times, ineffective.

The second part is that the state_machines library can halt callback chains without telling anything to anyone. That was happening in some cases when ReturnItem#attempt_accept was failing. In those cases, just to increase confusion, the acceptance state-machine was interfering with the reception state-machine.

The third part is that the introduction of the accessor for skip_customer_return_processing was fixing one code path (the one still visible in the original PR) but was disrupting another one, namely marking the order as returned after the return-item state-machine was getting the "receive" event.

Ref #3199

Checklist:

I have followed Pull Request guidelines
I have added a detailed description into each commit message
I have updated Guides and README accordingly to this change (if needed)
I have added tests to cover this change (if needed)
I have attached screenshots to this PR for visual changes (if needed)

backend/spec/controllers/spree/admin/return_items_controller_spec.rb

spaghetticode · 2020-01-22T13:47:29Z

@elia thank you for this fix 🎉

I see you added many tests at the controller and integration level, those are very good as they expose the functionality and the application flow in the admin area, but I'm thinking that, due to the changes to Spree::ReturnItem model, maye we should add some model specs in core as well in order to document those changes... WDYT?

elia · 2020-01-22T16:45:10Z

@spaghetticode I've added some model-level specs, let me know if you think there's enough coverage now 🔍

core/spec/models/spree/order/outstanding_balance_integration_spec.rb

core/app/models/spree/order.rb

core/app/models/spree/store_credit.rb

core/app/models/spree/promotion_rule_user.rb

core/app/models/spree/product.rb

core/app/models/spree/credit_card.rb

kennyadsl · 2020-01-29T17:20:05Z

@elia failures seem legit here, can you please take a look?

aldesantis · 2020-02-07T11:38:21Z

core/app/models/spree/order.rb

+      # Inventory units are transitioned to the return state by ReturnItem
+      # across a different association path goes through CustomerReturn and
+      # ReturnItem instead of using Order#inventory_units. That makes any
+      # the cached association on order stale and require a *reload* before
+      # checking if they're returned.


I think you could improve the English here a bit, looks like it has been rewritten a few times and the grammar doesn't make much sense anymore. Same goes for the commit.

aldesantis · 2020-02-07T11:39:55Z

backend/spec/controllers/spree/admin/return_items_controller_spec.rb

@@ -34,7 +34,13 @@
      let(:new_reception_status) { 'received' }
      let(:reception_status_event) { 'receive' }

-      before { return_item.update reception_status: 'in_transit'}
+      before do
+        allow(Spree::Deprecation).to receive(:warn) do |message|


FYI you can use a regexp here, but it's up to you:

allow(Spree::Deprecation).to receive(:warn).with(/#process_inventory_unit! will not call/)

aldesantis · 2020-02-07T11:40:28Z

backend/spec/controllers/spree/admin/return_items_controller_spec.rb

+          expect(message).to match('#process_inventory_unit! will not call')
+        end
+
+        return_item.update reception_status: 'in_transit'


Usually I use update! when setting up a test. That way, if the setup code fails for some reason, it does so loudly instead of making the expectations fail.

core/app/models/spree/order.rb

spaghetticode

@elia thank you 👍

spaghetticode · 2020-02-28T11:50:18Z

@aldesantis can you review the PR again after Elia's changes?

Checking for customer_return is much faster than the other checks and since it's already in #should_restock? we can get rid of the extra check and make the code more linear.

Avoid defining the shared examples globally.

This endpoint is hit from the button that marks return items as received in admin/customer_returns.

This is in preparation to adding more specs and cover more behavior of that part of the admin.

This method is mainly used after inventory items are put in "return" state, and that usually happens through order->customer_returns-> return_item->inventory_unit. This makes the instances of InventoryUnit cached in Order#inventory_units potentially stale. By using reload we avoid the bugs stemming from this discrepancy. See also the comment in the code.

Apparently the state_machines library can halt callback chains without telling anything to anyone. That was happening in some cases when ReturnItem#attempt_accept was failing. In those cases, just to increase confusion, the acceptance state-machine is interacting with the reception state-machine. Additionally, the introduction of the accessor for skip_customer_return_processing was fixing one code path (the one still visible in the original PR) but was disrupting another one, namely marking the order as "returned" after the return-item state-machine was delivered the "receive" event.

Walk back the use of skip_customer_return_processing and restore the call to process_return! within process_inventory_unit!.

aldesantis

Amazing work @elia, and much needed! Thanks! 🙏

kennyadsl · 2020-03-09T12:22:57Z

@elia merging. Can you help me writing the changelog entry for this? A comment here is enough, I will take care of moving it into a full changelog entry once we are approaching the release.

houndci-bot reviewed Jan 22, 2020

View reviewed changes

backend/spec/controllers/spree/admin/return_items_controller_spec.rb Outdated Show resolved Hide resolved

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch from 7a4e792 to fff7ab7 Compare January 22, 2020 11:08

elia marked this pull request as ready for review January 22, 2020 11:48

elia requested review from spaghetticode and kennyadsl January 22, 2020 11:48

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch from fff7ab7 to 423afdf Compare January 22, 2020 16:43

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch from 423afdf to 398a581 Compare January 24, 2020 18:14

houndci-bot reviewed Jan 24, 2020

View reviewed changes

elia changed the base branch from v2.9 to master January 24, 2020 18:53

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch 3 times, most recently from 6228fe9 to 0dd92fd Compare January 28, 2020 15:27

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch from 0dd92fd to 0e9e70a Compare January 31, 2020 17:40

kennyadsl added the Code Review Needed label Feb 3, 2020

aldesantis requested changes Feb 7, 2020

View reviewed changes

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch from 0e9e70a to ac8eff3 Compare February 14, 2020 14:36

houndci-bot reviewed Feb 14, 2020

View reviewed changes

core/app/models/spree/order.rb Outdated Show resolved Hide resolved

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch from ac8eff3 to 5f2a546 Compare February 14, 2020 14:38

elia requested a review from aldesantis February 14, 2020 14:42

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch from 5f2a546 to 545e764 Compare February 14, 2020 14:53

spaghetticode approved these changes Feb 28, 2020

View reviewed changes

elia added 5 commits March 6, 2020 11:11

ReturnItem#should_restock? already checks for customer_return

af07597

Checking for customer_return is much faster than the other checks and since it's already in #should_restock? we can get rid of the extra check and make the code more linear.

Move ReturnItem shared spec inside the main block

c8d09ce

Avoid defining the shared examples globally.

Spec ReturnItem reception state machine after-callbacks

99c9bcf

Spec reception_status for admin/return_items#update

3a67a9d

This endpoint is hit from the button that marks return items as received in admin/customer_returns.

Extract helpers for the Customer Returns feat spec

915d3e5

This is in preparation to adding more specs and cover more behavior of that part of the admin.

elia added 3 commits March 6, 2020 11:13

Deprecate skip_customer_return_processing

cad0a4c

Walk back the use of skip_customer_return_processing and restore the call to process_return! within process_inventory_unit!.

elia force-pushed the elia/customer-returns-order-state-after-in-transit branch from 545e764 to cad0a4c Compare March 6, 2020 10:13

aldesantis approved these changes Mar 6, 2020

View reviewed changes

kennyadsl merged commit d3be0c4 into solidusio:master Mar 9, 2020

kennyadsl deleted the elia/customer-returns-order-state-after-in-transit branch March 9, 2020 12:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix order state with customer returns when receiving return items #3483

Fix order state with customer returns when receiving return items #3483

elia commented Jan 22, 2020 •

edited

Loading

spaghetticode commented Jan 22, 2020

elia commented Jan 22, 2020

kennyadsl commented Jan 29, 2020

aldesantis Feb 7, 2020

aldesantis Feb 7, 2020

aldesantis Feb 7, 2020

spaghetticode left a comment

spaghetticode commented Feb 28, 2020

aldesantis left a comment

kennyadsl commented Mar 9, 2020

Fix order state with customer returns when receiving return items #3483

Fix order state with customer returns when receiving return items #3483

Conversation

elia commented Jan 22, 2020 • edited Loading

spaghetticode commented Jan 22, 2020

elia commented Jan 22, 2020

kennyadsl commented Jan 29, 2020

aldesantis Feb 7, 2020

Choose a reason for hiding this comment

aldesantis Feb 7, 2020

Choose a reason for hiding this comment

aldesantis Feb 7, 2020

Choose a reason for hiding this comment

spaghetticode left a comment

Choose a reason for hiding this comment

spaghetticode commented Feb 28, 2020

aldesantis left a comment

Choose a reason for hiding this comment

kennyadsl commented Mar 9, 2020

elia commented Jan 22, 2020 •

edited

Loading