Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark a shipment as canceled if all the inventory units have been canceled #5220

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Mark a shipment as canceled if all the inventory units have been canceled #5220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6f4cd13
mark a shipment as canceled if all the inventory units have been canc…
BenMorganIO Jul 5, 2023
3bde83e
backport logic from order cancelation to shipment determine_state for…
BenMorganIO Jul 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion core/app/models/spree/order_cancellations.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,9 @@ def update_shipped_shipments(inventory_units)
to_a

shipments.each do |shipment|
if shipment.inventory_units.all? { |iu| iu.shipped? || iu.canceled? }
if shipment.inventory_units_canceled?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kennyadsl I likely need to add a check here if the shipment is not in a shipped state.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having a look at the state machine of the shipment and it seems like it's not allowed to move a shipment from shipped to canceled already:

solidus/core/lib/spree/core/state_machines/shipment.rb

Line 36 in df68c0c

transition to: :canceled, from: [:pending, :ready]

Do you know if there's a specific reason not to rely on the state machine here? Same for when we set the state to shipped. I think we are also skipping the rest of the callbacks defined in the state machine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I should be able to use the state machine call. I was mimicking the original code and thought that there must have been a reason to jump the state machine. Although, we should likely use it.

For the code on line 124, should we update that code as well to utilize the state machine?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think the best path forward at this point is to make another PR that just changes OrderCancelations to try to use the state machine. If that works and we can't find blockers, changing this PR will be smooth. What do you think?

shipment.update!(state: 'canceled', shipped_at: nil)
elsif shipment.inventory_units_shipped_or_canceled?
shipment.update!(state: 'shipped', shipped_at: Time.current)
end
end
Expand Down
24 changes: 15 additions & 9 deletions core/app/models/spree/shipment.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,19 +200,25 @@ def selected_shipping_rate_id=(id)

# Determines the appropriate +state+ according to the following logic:
#
# canceled if order is canceled
# canceled if all of the inventory units are canceled
# shipped if already shipped (ie. does not change the state) or all of
# the inventory units are either shipped or canceled
# pending unless order is complete and +order.payment_state+ is +paid+
# shipped if already shipped (ie. does not change the state)
# ready all other cases
def determine_state(order)
return 'canceled' if order.canceled?
return 'shipped' if shipped?
return 'canceled' if order.canceled? || inventory_units_canceled?
return 'shipped' if shipped? || inventory_units_shipped_or_canceled?
return 'pending' unless order.can_ship?
if can_transition_from_pending_to_ready?
'ready'
else
'pending'
end

can_transition_from_pending_to_ready? ? 'ready' : 'pending'
end

def inventory_units_canceled?
inventory_units.all?(&:canceled?)
end

def inventory_units_shipped_or_canceled?
inventory_units.all? { |iu| iu.shipped? || iu.canceled? }
end

def set_up_inventory(state, variant, _order, line_item)
Expand Down
6 changes: 3 additions & 3 deletions core/spec/models/spree/order_cancellations_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,15 +83,15 @@
end

it "cancels the inventory unit" do
expect { subject }.to change { inventory_unit.state }.to "canceled"
expect { subject }.to change { inventory_unit.state }.to 'canceled'
end

it "updates the shipment.state" do
expect { subject }.to change { shipment.reload.state }.from('ready').to('shipped')
expect { subject }.to change { shipment.reload.state }.from('ready').to('canceled')
end

it "updates the order.shipment_state" do
expect { subject }.to change { order.shipment_state }.from('ready').to('shipped')
expect { subject }.to change { order.shipment_state }.from('ready').to('canceled')
end

it "adjusts the order" do
Expand Down
11 changes: 11 additions & 0 deletions core/spec/models/spree/shipment_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,17 @@
expect(shipment.determine_state(order)).to eq 'canceled'
end

it 'returns canceled if all of the inventory units are canceled' do
shipment.inventory_units.each(&:cancel!)
expect(shipment.determine_state(order)).to eql 'canceled'
end

it 'returns shipped if some of the inventory units are canceled and some shipped' do
shipment.inventory_units.each(&:cancel!)
create :inventory_unit, shipment: shipment, state: 'shipped'
expect(shipment.determine_state(order)).to eql 'shipped'
end

it 'returns pending unless order.can_ship?' do
allow(order).to receive_messages can_ship?: false
expect(shipment.determine_state(order)).to eq 'pending'
Expand Down