Allow splitting shipments when not tracking inventory

[nspinazz89]

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]

👍

[kennyadsl]

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]

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]

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

[kennyadsl]

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]

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.

[kennyadsl]

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]

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?

[kennyadsl]

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.

[kennyadsl]

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!

[spaghetticode]

@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.