Allow order with multiple line items to be marked as "Returned" #3199
Conversation
spaghetticode
force-pushed
the
spaghetticode/fix-3198-order-state-after-return
branch
from
6b5082a to
3be97d7
Compare
There was a problem hiding this comment.
Hey @spaghetticode, I don't get why this change is necessary, even if the issue and the solution are pretty clear. I just need some more info about it, thanks!
core/app/models/spree/order.rb
Outdated
| @@ -286,7 +286,7 @@ def allow_cancel? | |||
| end | |||
|
|
|||
| def all_inventory_units_returned? | |||
| inventory_units.all?(&:returned?) | |||
| inventory_units.returned.count == inventory_units.count | |||
There was a problem hiding this comment.
I'm not sure to understand why the former code is not working. 🤔 Did you get that? Can you please explain it a little bit?
There was a problem hiding this comment.
This code is executed for each return item, for a better understanding of the process you can start from ReturnItem#process_inventory_unit!. The first inventory unit is marked as returned, and eventually order.inventory.units is called.
The query is executed only for the first return item, and as we know there's no issue as long as the customer return is for one single item. But when there's more than one, from the second time on, order.inventory.units refers to stale data, and must be updated. Using count ensures a new (lightweight) query is performed.
Basically, it happens something similar to this:
o = create :shipped_order, line_items_count: 2
o.inventory_units.all? &:shipped?
=> true
u = Spree::InventoryUnit.find(o.inventory_units.first.id)
u.return!
u.reload.returned?
=> true
o.inventory_units.all? &:shipped?
=> true
spaghetticode
force-pushed
the
spaghetticode/fix-3198-order-state-after-return
branch
5 times, most recently
from
d644b96 to
7a94c47
Compare
| if customer_return | ||
| customer_return.stock_location.restock(inventory_unit.variant, 1, customer_return) if should_restock? | ||
| customer_return.process_return! | ||
| unless skip_customer_return_processing |
There was a problem hiding this comment.
I think we should also test that a deprecation warning is raised in some circumstances.
There was a problem hiding this comment.
Makes sense, I added this spec: https://github.com/solidusio/solidus/pull/3199/files#diff-0f55d3185362ecf4697cad5429703bd0
| @@ -38,7 +38,10 @@ | |||
| let(:order) { customer_return.order } | |||
| let(:return_item) { customer_return.return_items.first } | |||
| let(:payment) { order.payments.first } | |||
| before { return_item.receive! } | |||
| before do | |||
| return_item.skip_customer_return_processing = true | |||
There was a problem hiding this comment.
Do you think it could be a good idea to add this as default in the return item factory?
There was a problem hiding this comment.
I totally agree 👍
This also reduced the changes I had to make to spec files
spaghetticode
force-pushed
the
spaghetticode/fix-3198-order-state-after-return
branch
from
7a94c47 to
0b43aed
Compare
…! callback Processing order returns in `ReturnItem#process_inventory_unit!` does not work reliably when there are multiple return items processed at the same time (see issue solidusio#3198). Also, decoupling sensible interactions with other models in ActiveRecord callbacks simplifies the understandability of code and its flow. Order processing in `#process_inventory_unit!` is now only deprecated, but will be completely removed in the following release of Solidus.
…m specs Deprecations make test fails on circleCI. The easiest way to remove them is by updating the `return_item` factory.
…eturn! The condition that verifies if an order can change state to `returned` is now completely delegated to the order state machine in order to avoid leaking of context and knowledge.
spaghetticode
force-pushed
the
spaghetticode/fix-3198-order-state-after-return
branch
from
0b43aed to
4870e08
Compare
Description
This PR fixes issue #3198
When a customer return is created for an order with multiple line items and the
return state of all the items is
Receivedthen the order state should changeto
Returnedjust like it happens with orders with only one line item.Checklist: