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

Jump to bottom

Keep backorders when splitting part of variant to new shipment with same SL #5670

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

Keep backorders when splitting part of variant to new shipment with same SL #5670

wants to merge 10 commits into from

Conversation

spaghetticode
Copy link
Member

@spaghetticode spaghetticode commented Feb 26, 2024 •
edited
Loading

Summary

Fixes #5669

When splitting a shipment to the same stock location, when not the whole quantity for the variant is moved it may happen, if there are backordered items, that these disappear (they become on hand).

This PR first reworks and adds further coverage to specs for the Spree::FulfilmentChanger service to expose the issue, then addresses it in the last commit.

fix.mov

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

  • I have written a thorough PR description.
  • I have kept my commits small and atomic.
  • [I have used clear, explanatory commit messages]
  • I have added automated tests to cover my changes.

@github-actions github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Feb 26, 2024
@spaghetticode spaghetticode force-pushed the spaghetticode/fix-issue-5669 branch 2 times, most recently from 8781d81 to 356feb9 Compare February 26, 2024 15:25
@spaghetticode spaghetticode changed the title Maintain backordered items when splitting part of a variant to a new shipment with same SL Keep backordered items when splitting part of a variant to a new shipment with same SL Feb 26, 2024
@spaghetticode spaghetticode changed the title Keep backordered items when splitting part of a variant to a new shipment with same SL Keep backorders when splitting part of a variant to a new shipment with same SL Feb 26, 2024
@spaghetticode spaghetticode changed the title Keep backorders when splitting part of a variant to a new shipment with same SL Keep backorders when splitting part of variant to new shipment with same SL Feb 26, 2024
@kennyadsl
Copy link
Member

Hey @spaghetticode, is this still a draft intentionally or can we start reviewing?

@spaghetticode spaghetticode marked this pull request as ready for review February 27, 2024 10:33
@spaghetticode spaghetticode requested a review from a team as a code owner February 27, 2024 10:33
@spaghetticode
Copy link
Member Author

@kennyadsl it should be good for review, I'm sorry for the confusion.

kennyadsl
kennyadsl reviewed Feb 27, 2024
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Left a couple of questions, thanks Andrea!

core/app/models/spree/fulfilment_changer.rb Outdated Show resolved Hide resolved
core/app/models/spree/fulfilment_changer.rb Outdated Show resolved Hide resolved
@spaghetticode
Use let! to remove explicit calls from before block
319cb6e
@spaghetticode
Move let statements inside context block
9062828 
These are already overridden in all other first-level context blocks
so it makes sense to move them in the single block that relies on
these values.
@spaghetticode
Extract shared examples
57c62ab 
Also, a couple of useless `let!` are removed (these records already
exist).
@spaghetticode
Reuse existing shared example
2ff1bf4 
The removed spec can be replaced by the shared example, which
includes one further test so it slightly improves coverage.
@spaghetticode
Reuse existing let value
0d1e747 
This helps keeping things in sync just in the case that numbers
change.
@spaghetticode
Extract shared example for later reuse
0079208 
These specs don't really require a specific first level scenario,
so we're first extracting them to a shared example, and with the
following commit we're going to use it in other existing scenarios.
@spaghetticode
Rework and reuse inventory units shared example
67e609b 
The existing scenario is reworked in order to expose an issue
when moving a few items of a variant (not all of them) to a
shipment with same stock location as the original one.
@spaghetticode spaghetticode force-pushed the spaghetticode/fix-issue-5669 branch from 356feb9 to 2a82493 Compare March 7, 2024 14:00
kennyadsl
kennyadsl approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Left a comment on the method name but it's not blocking, 👍

core/app/models/spree/stock/quantifier.rb Outdated Show resolved Hide resolved
@spaghetticode
Refactor Quantifier initialization
0524dcd 
This is propedeutic for the following commit.
@spaghetticode spaghetticode force-pushed the spaghetticode/fix-issue-5669 branch 2 times, most recently from 64f159c to 9539dd4 Compare March 7, 2024 17:33
@spaghetticode
Add Quantifier#positive_stock method
2e17aeb 
When the quantifier is initialized with a stock location or its ID,
the new method returns the positive amount of stock on hand or zero,
if the stock for the variant is negative. Otherwise, it returns nil.
@spaghetticode
Properly calculate inventory item quantities to be moved
a1bb98e 
The backordered quantity count should differ depending on whether
moving to the same or a different stock location. For this reason,
the way we calculate `available_quantity` changes as follows:

* when the stock location differs:
  the stock on hand at the new shipment stock location;
* when the stock location is the same:
  the sum of the stock on hand at the shipment stock location
  plus the number of on_hand inventory items from the shipment

The explicit `backordered_quantity` variable is introduced to track
the number of backordered items for the target shipment. The value
is calculated as follows:

* when the stock location differs:
  the quantity to be moved minus the positive available quantity at
  the stock location;
* when the stock location is the same:
  the shipment total quantity for the variant minus the positive
  available quantity at the stock location.

Also, we start the process by moving backordered items first to
to make sure no pending backordered item remains. If the backordered
count decreased, we're going to leave a few to be later moved and
transformed to on hand, while if the backordered count increased, we
are going to move also some previously on hand items.
@spaghetticode spaghetticode force-pushed the spaghetticode/fix-issue-5669 branch from 9539dd4 to a1bb98e Compare March 7, 2024 17:46
@tvdeyen tvdeyen added this to the 4.4 milestone Oct 18, 2024
@tvdeyen
Copy link
Member

tvdeyen commented Nov 15, 2024

@spaghetticode any chance to rebase this with latest main?

@tvdeyen tvdeyen removed this from the 4.4 milestone Nov 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MadelineCollier MadelineCollier MadelineCollier approved these changes

@adammathys adammathys adammathys approved these changes

@jarednorman jarednorman jarednorman approved these changes

@kennyadsl kennyadsl kennyadsl approved these changes

Assignees
No one assigned
Labels
changelog:solidus_core Changes to the solidus_core gem type:bug Error, flaw or fault
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Backend] Backordered items may become on hand when splitting shipment
6 participants
@spaghetticode @kennyadsl @tvdeyen @jarednorman @adammathys @MadelineCollier