Allow splitting shipments when not tracking inventory #3338

nspinazz89 · 2019-09-19T16:20:44Z

Description

Closes #2817.

When Spree::Config.track_inventory_levels is false we are not supposed to change inventory levels when shipping orders.

At the moment though, if we try to split a shipment with the following scenario:

track_inventory_levels is false
there is no inventory in the second stock location for the related stock item

we end up creating a second shipment with items backorderable, making the second shipment impossible to ship.

This is because the FulfilmentChanger class at the moment does not take into account this scenario. But it was currently doing a lot of things that are not required when inventory levels are not tracked.

This fix proposes two different paths, driven by a new argument required in the initialization of the class, that performs a
complex stock movement when we are tracking inventory and a more simple otherwise.

Not passing this new argument to the class' new method is deprecated, assuming the old behavior for backward compatibility.

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)

kennyadsl

Thanks, @nspinazz89! This is a great change, anyway I'd like to consider another solution with you. I think that a lot of code above in the class is useless at this point and maybe we could wrap the code into an if statement so that we avoid performing some checks in case track_inventory is false. I'm thinking at all the on_hand/backorderable number checks, we could avoid those, right?

Something like:

if Spree::Config.track_inventory_levels
  change_fulfillment_with_inventory
else
  change_fulfillment_without_inventory
end

moving the different logic into different private (?) methods, but I'm even fine keeping all the code in a single method if you think it's better for some reason.

Also, it would be great to review unit tests for this class to see if we can add some specs to cover this new code there as well.

Thanks again!

octave · 2020-03-24T22:10:48Z

👍

kennyadsl · 2023-02-17T14:09:08Z

Checking if this is still a bug. Locally the provided specs are not failing to remove the change to the fulfillment changer class. Just pushed the spec only to see if we are still effected. If everything is green, we can close this without merging.

codecov · 2023-02-17T14:26:46Z

Codecov Report

Merging #3338 (de85741) into master (5525671) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3338      +/-   ##
==========================================
+ Coverage   86.70%   86.71%   +0.01%     
==========================================
  Files         578      578              
  Lines       14688    14700      +12     
==========================================
+ Hits        12735    12747      +12     
  Misses       1953     1953

Impacted Files	Coverage Δ
.../app/controllers/spree/api/shipments_controller.rb	`93.13% <ø> (ø)`
core/app/models/spree/fulfilment_changer.rb	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

kennyadsl · 2023-02-17T15:18:34Z

I can reproduce the failing spec. Will push an update soon.

kennyadsl · 2023-02-17T17:08:46Z

I pushed an updated version with more unit specs and the implementation of this suggestion.

rainerdema

Fantastic work, @kennyadsl @nspinazz89! Everything seems to be working perfectly. Thank you!

spaghetticode · 2023-03-08T09:23:29Z

api/app/controllers/spree/api/shipments_controller.rb

@@ -127,7 +127,8 @@ def transfer_to_shipment
          current_shipment: @original_shipment,
          desired_shipment: @desired_shipment,
          variant: @variant,
-          quantity: @quantity
+          quantity: @quantity,
+          track_inventory: Spree::Config.track_inventory_levels


I'm thinking this new param is not necessary since Spree::Config.track_inventory_levels could be accessed directly within FulfilmentChanger, but given we need to deprecate the previous behavior I can't think of any other way than this 🤔 . I'm wondering if we should try to remove it later, when we finally remove the default behavior.

My idea was that at the end it's not that bad because this track_inventory value determines heavily the logic used in the class, and it's better to be as explicit as possible about it when it's initialized. After the deprecation period, we could evaluate removing it (with another deprecation), though.

spaghetticode · 2023-03-08T09:28:09Z

core/app/models/spree/fulfilment_changer.rb


-    def initialize(current_shipment:, desired_shipment:, variant:, quantity:)
+    def initialize(current_shipment:, desired_shipment:, variant:, quantity:, track_inventory: nil)


I'm thinking nil might be somehow a valid value for Spree::Config.track_inventory_levels, as it still evaluates to false. I'm thinking the default value for track_inventory may be a constant with an arbitrary value and use that instead of nil, so we're 100% sure we didn't receive that param from the caller. Something like what's being done in this PR with PAYMENT_NOT_PROVIDED. WDYT?

I thought people always need to pass something that is true or false explicitly here. Do you mean that someone might pass nil initializing this class, but they actually mean to pass false? I'm not sure when that could happen, but open to changing this if you have a concrete example.

Spree::Config.track_inventory_levels is a boolean preference, but can also be nil. If nil is considered false in all the places, this is actually a very good suggestion. I updated the PR, let me know!

@kennyadsl looks great, thanks!

spaghetticode

Thanks @kennyadsl , LGTM!
I see there are some CI runs failing but I think they're not related to these changes.

When Spree::Config.track_inventory_levels is false we are not supposed to change inventory levels when shipping orders. At the moment though, if we try to split a shipment with the following scenario: - track_inventory_levels is false - there is no inventory in the second stock location for the related stock item we end up creating a second shipment with items backorderable, making the second shipment impossible to ship. This is because the FulfilmentChanger class at the moment does not take into account this scenario. But it was currently doing a lot of things that are not required when inventory leves are not tracked. This fix proposes two different paths, driven by a new argument required in the initialization of the class, that perform a complex stock movement when we are tracking inventory and a more simple otherwise. Not passing this new argument to the class' `new` method is deprecated, assuming the old behavior for backward compatibility.

Ref: https://app.circleci.com/pipelines/github/solidusio/solidus/4122/workflows/e273818e-ef20-4238-ab3b-04b7055436de/jobs/38843

Ref solidusio#3338

nspinazz89 force-pushed the bug/backorder-status-when-track-inventory-false branch from 324c83f to 904ba9d Compare September 19, 2019 17:33

kennyadsl reviewed Sep 20, 2019

View reviewed changes

hrdchz mentioned this pull request Feb 11, 2020

Out of stock by default solidusio-contrib/solidus_virtual_gift_card#64

Closed

kennyadsl added type:bug Error, flaw or fault Needs Work labels Aug 24, 2022

waiting-for-dev added changelog:solidus_core Changes to the solidus_core gem changelog:solidus_backend Changes to the solidus_backend gem labels Aug 30, 2022

waiting-for-dev removed Needs Tests labels Dec 1, 2022

kennyadsl force-pushed the bug/backorder-status-when-track-inventory-false branch from 904ba9d to c034d65 Compare February 17, 2023 14:06

kennyadsl requested a review from a team as a code owner February 17, 2023 14:06

github-actions bot removed the changelog:solidus_core Changes to the solidus_core gem label Feb 17, 2023

kennyadsl force-pushed the bug/backorder-status-when-track-inventory-false branch from c034d65 to db5bf50 Compare February 17, 2023 16:25

github-actions bot added changelog:solidus_api Changes to the solidus_api gem changelog:solidus_core Changes to the solidus_core gem labels Feb 17, 2023

kennyadsl changed the title ~~Bug/backorder status when track inventory false~~ Allow splitting shipments when not tracking inventory Feb 17, 2023

kennyadsl self-assigned this Feb 17, 2023

rainerdema approved these changes Mar 3, 2023

View reviewed changes

spaghetticode reviewed Mar 8, 2023

View reviewed changes

kennyadsl mentioned this pull request Mar 10, 2023

Allow providing an existing payment during the checkout #4909

Closed

3 tasks

kennyadsl force-pushed the bug/backorder-status-when-track-inventory-false branch from dbaaefa to 1a8860a Compare March 10, 2023 14:32

spaghetticode approved these changes Mar 10, 2023

View reviewed changes

nspinazz89 and others added 2 commits March 15, 2023 14:18

Mark another spec as flaky

de85741

Ref: https://app.circleci.com/pipelines/github/solidusio/solidus/4122/workflows/e273818e-ef20-4238-ab3b-04b7055436de/jobs/38843

kennyadsl force-pushed the bug/backorder-status-when-track-inventory-false branch from 1a8860a to de85741 Compare March 15, 2023 13:18

kennyadsl merged commit 2cf1269 into solidusio:master Mar 15, 2023

kennyadsl added a commit to nebulab/solidus that referenced this pull request Mar 21, 2023

Remove support for not passing track_inventory to FulfilmentChanger

37f9408

Ref solidusio#3338

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 12, 2023

Remove support for not passing track_inventory to FulfilmentChanger

820d2e5

Ref solidusio#3338

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 14, 2023

Remove support for not passing track_inventory to FulfilmentChanger

9457094

Ref solidusio#3338

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 18, 2023

Remove support for not passing track_inventory to FulfilmentChanger

1267810

Ref solidusio#3338

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 19, 2023

Remove support for not passing track_inventory to FulfilmentChanger

49c701b

Ref solidusio#3338

kennyadsl added a commit to nebulab/solidus that referenced this pull request Apr 24, 2023

Remove support for not passing track_inventory to FulfilmentChanger

a4f3a20

Ref solidusio#3338

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow splitting shipments when not tracking inventory #3338

Allow splitting shipments when not tracking inventory #3338

nspinazz89 commented Sep 19, 2019 •

edited by kennyadsl

Loading

kennyadsl left a comment •

edited

Loading

octave commented Mar 24, 2020

kennyadsl commented Feb 17, 2023

codecov bot commented Feb 17, 2023 •

edited

Loading

kennyadsl commented Feb 17, 2023

kennyadsl commented Feb 17, 2023

rainerdema left a comment

spaghetticode Mar 8, 2023

kennyadsl Mar 10, 2023

spaghetticode Mar 8, 2023

kennyadsl Mar 10, 2023

kennyadsl Mar 10, 2023

spaghetticode Mar 10, 2023

spaghetticode left a comment


		def initialize(current_shipment:, desired_shipment:, variant:, quantity:)
		def initialize(current_shipment:, desired_shipment:, variant:, quantity:, track_inventory: nil)

Allow splitting shipments when not tracking inventory #3338

Allow splitting shipments when not tracking inventory #3338

Conversation

nspinazz89 commented Sep 19, 2019 • edited by kennyadsl Loading

kennyadsl left a comment • edited Loading

Choose a reason for hiding this comment

octave commented Mar 24, 2020

kennyadsl commented Feb 17, 2023

codecov bot commented Feb 17, 2023 • edited Loading

Codecov Report

kennyadsl commented Feb 17, 2023

kennyadsl commented Feb 17, 2023

rainerdema left a comment

Choose a reason for hiding this comment

spaghetticode Mar 8, 2023

Choose a reason for hiding this comment

kennyadsl Mar 10, 2023

Choose a reason for hiding this comment

spaghetticode Mar 8, 2023

Choose a reason for hiding this comment

kennyadsl Mar 10, 2023

Choose a reason for hiding this comment

kennyadsl Mar 10, 2023

Choose a reason for hiding this comment

spaghetticode Mar 10, 2023

Choose a reason for hiding this comment

spaghetticode left a comment

Choose a reason for hiding this comment

nspinazz89 commented Sep 19, 2019 •

edited by kennyadsl

Loading

kennyadsl left a comment •

edited

Loading

codecov bot commented Feb 17, 2023 •

edited

Loading