-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Finalize shipment after inventory units are added to completed order #2331
Finalize shipment after inventory units are added to completed order #2331
Conversation
47e7621
to
0046c51
Compare
0046c51
to
ba65a2d
Compare
build-ci.rb
Outdated
# @return [Boolean] | ||
# the success of the installation | ||
def self.bundle_install | ||
system(*%W[ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass array contents as separate arguments.
build-ci.rb
Outdated
# | ||
# @return [Boolean] | ||
def self.bundle_check | ||
system(*%W[bundle check --path=#{VENDOR_BUNDLE}]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pass array contents as separate arguments.
ba65a2d
to
2f66b63
Compare
@@ -0,0 +1,32 @@ | |||
require 'rails_helper' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing magic comment # frozen_string_literal: true.
@@ -0,0 +1,45 @@ | |||
module Spree |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing magic comment # frozen_string_literal: true.
51cc0e9
to
eb3488e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just left 2 comments/questions. Great job!
@@ -142,6 +142,7 @@ | |||
} | |||
|
|||
it "should create a stock movement" do | |||
expect(Spree::Deprecation).to receive(:warn) | |||
Spree::InventoryUnit.finalize_units!(inventory_units) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not using the new call here and remove the Deprecation :warn
stub?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a test for Spree::InventoryUnit#finalize_units!
.
|
||
it "doesn't unstock items" do | ||
expect(inventory_unit_finalizer).to_not receive(:run!) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the feeling we can avoid this expectation: if run!
is called the next expectation will fail, right? Is there any other reason you wanted to set this expectation?
eb3488e
to
187a260
Compare
end | ||
|
||
it "updates the inventory units to not be pending" do | ||
expect { subject }.to change { inventory_unit.reload.updated_at } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This test doesn't match its description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed!
187a260
to
c9367a8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @DanielePalombo !
@gmacdougall can you please review again now? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Sorry for the delay on reviewing this one.
@DanielePalombo since it's so old, can you please rebase against latest master? Thanks! |
Spree::Stock::InventoryUnitsFinalizer is responsible to: - Unstock the desired quantity - Update the inventory units to not pending The finalize logic inside the `run!` method has been taken from https://github.com/solidusio/solidus/blob/64bb34aa5837bca0580002d608358b0826d94ae6/core/app/models/spree/shipment.rb#L167-L172
While inventory units are being add to the shipment, collect and finalize them if the order is completed.
Added `finalize_pending_inventory_units` method that is responsible to finalize just pending inventory units.
c9367a8
to
98cbf42
Compare
@kennyadsl I have rebased it! |
This PR is going to fix the issue #2141 .
Create a new class Spree::Stock::InventoryUnitsFinalizer
to finalize the inventory units provided.
It unstocks the desired quantity and marks as not pending the inventory units processed.
To fix the issue we run it, both when a shipment is finalized and when a variant is added to a shipment in a completed order.