search

solidusio / solidus

🛒 Solidus, the open-source eCommerce framework for industry trailblazers.
https://solidus.io
Other
4.97k stars 1.29k forks source link

Incorrect behaviour for Cancel Item #2947

Open gerritwessels opened 5 years ago

gerritwessels commented 5 years ago

Steps to reproduce

Get the Solidus sandbox up and running. Ensure that 'track_inventory' is set to true for variants. ( I believe this is the default setting anyway ) Create an order with a single line item and capture the payment. At this point in time, the order statuses should be:

In the database: New Spree::StockMovement record will be present for this line_item, with quantity set to -1. New Spree::Shipment record with state of 'Ready' New Spree::InventoryUnit record with state 'on_hand' that belongs to the Spree::Shipment

Trigger the Issue Go to the admin interface and cancel the item.

Expected behavior

I would expect to see the item canceled, the payment balance to be updated and the spree_stock to be replenished with the canceled item.

If there was only one item in the Spree::Shipment, I would expect the shipment state to be changed to 'Canceled' or Spree::Shipment removed.

If there was multiple items in the same Spree::Shipment, I would expect the shipment state to remain as 'Ready'

Actual behavior

Database No Spree::StockMovement record gets created to replenish the stock, which seems incorrect. The Spree::Shipment state transitions to 'shipped' and populates the shipped_at column, which seems incorrect. The Spree::InventoryUnit state transitions to 'canceled', which is as expected.

Question This behaviour seems wrong to me, am I missing something?

System configuration

Solidus Version: v.2.7.0

Extensions in use: N/A

gerritwessels commented 5 years ago

Regarding Spree::Shipment records that transition to 'shipped' when all the inventory_units are 'canceled'

I quickly poked around in the code to see if I could see anything obvious. This line: https://github.com/solidusio/solidus/blob/35010c23483086e1be832708bd06d30bb05b1a88/core/app/models/spree/order_cancellations.rb#L141

Quick fix for this could be:

# Before we update a shipment to 'shipped':
# Ensure that ALL the inventory_units for this shipment are in either a 'shipped' or 'canceled' state AND ensure that they are NOT ALL in a 'canceled' state
if (shipment.inventory_units.all? { |iu| iu.shipped? || iu.canceled? }) && (!shipment.inventory_units.all? { |iu| iu.canceled? })
jacobherrington commented 5 years ago

Hey @gerritwessels if you have an idea to fix this, we'd love to get a PR for it. 👍

spaghetticode commented 5 years ago

I tried to reproduce the issue and I think that as of today it's gone.

With a single item purchase I followed the steps and I got the shipment destroyed, the stock movement that replenishes the stock created and the payment state changed to Credit owed

Same happens with 2 different items, the only difference is this time the shipment does not get destroyed, which is right.

Did not check all the code details, but I noticed some action happens in Spree::OrderInventory#remove_from_shipment where the shipment is destroyed and the stock location replenished.

aitbw commented 5 years ago

As today (02/25/2019), this issue is still present, although partially, in master @ 7665973

By partially, I mean:

ellypham commented 5 years ago

This issue is happening with me as well.

jacobherrington commented 4 years ago

@ellypham what version of Solidus are you seeing this on?