[main] Bug 648630 Movement Worksheet Creates Incorrect Warehouse Movement for FEFO Lot-Tracked Items - #10924
Conversation
There was a problem hiding this comment.
🟢 Approval recommended
The focused fix matches the root cause and is covered by a representative regression test.
Pull request overview
Corrects FEFO movement worksheet availability by excluding stock already moved to outbound bins.
Changes:
- Subtracts outbound-bin quantities from movement availability, clamped at zero.
- Adds regression coverage for registered and pending picks.
File summaries
| File | Description |
|---|---|
CreatePick.Codeunit.al |
Corrects movement availability calculation. |
SCMMovement.Codeunit.al |
Adds end-to-end regression test and setup helpers. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| LotNo: array[2] of Code[50]; | ||
| begin | ||
| // [SCENARIO 648630] Movement Worksheet with FEFO excludes picked-not-shipped and pending pick quantities when calculating lot availability | ||
| Initialize(); |
There was a problem hiding this comment.
The new test MovementWorksheetFEFOExcludesPickedNotShippedAndPendingPick (and the shared local Initialize procedure it relies on) never calls LibraryVariableStorage.Clear() before enqueueing values for its ModalPageHandlers (WhseItemTrackingLinesAssignLotAndExpirationPageHandler, ItemTrackingLinesModalPageHandlerMultipleEntries). The test does end with LibraryVariableStorage.AssertEmpty(), but per BCQuality guidance the Initialize routine itself should call LibraryVariableStorage.Clear() so a value leaked by an earlier failing test cannot cascade into this one and cause a misleading pass/fail. This is a pre-existing gap shared by the other tests in this codeunit, but it also applies to the new test being added here.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.36.6
Good Sense Reviewer - Round 1Recommendation: Request ChangesWhat this PR doesThe change subtracts outbound-bin quantity from directed put-away and pick availability when creating a movement worksheet, and it adds a regression test for FEFO lots with picked-not-shipped and open pick quantities. For replenishing a normal destination bin, this matches the intended rule that a movement must not source stock from outbound bins, and the test covers the reported lot choice. The implementation is still too broad. It subtracts outbound-bin quantity for every movement worksheet call, but the existing movement-worksheet logic keeps ship-zone destinations exempt from that subtraction. Problem-solution fitFit: Partial The reported replenishment scenario is clear, and the change handles the picked-not-shipped lot for a normal destination bin. The fix also changes movements whose destination is the ship zone, which is outside the reported problem and does not match the existing availability rule. SuggestionsS1 (🔴 High): Keep ship-zone movements exempt Risk assessment and necessityRisk: The changed code is in CreatePick.Codeunit.al availability calculation for directed put-away and pick movement worksheets. A wrong availability result can create too little movement or no movement for a valid warehouse movement, especially when the destination is the ship zone. The new test covers the reported FEFO replenishment path, and the required checks are passing. Necessity: The change is needed because movement replenishment should not choose stock that is already picked-not-shipped or committed to another pick. The scope should be narrowed to non-ship destinations so the fix keeps the established ship-zone movement behavior.
|
| WarehouseActivityLine.SetRange("Action Type", WarehouseActivityLine."Action Type"::Take); | ||
| Assert.RecordCount(WarehouseActivityLine, 1); | ||
| WarehouseActivityLine.FindFirst(); | ||
| Assert.AreEqual(LotNo[2], WarehouseActivityLine."Lot No.", 'Movement must source LOT-B, since LOT-A is picked-not-shipped and on a pending pick'); |
There was a problem hiding this comment.
The THEN comment in the new test states the movement take line must keep "Bin Code" blank when "Always Create Pick Line" is enabled, but the test only asserts the lot and quantity. If bin assignment regresses while lot selection still correctly picks LOT-B, this scenario will stay green even though the documented behavior changed. Add an explicit assertion for the blank bin code.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
Assert.AreEqual('', WarehouseActivityLine."Bin Code", 'Movement must leave the source bin blank when Always Create Pick Line is enabled');
Assert.AreEqual(LotNo[2], WarehouseActivityLine."Lot No.", 'Movement must source LOT-B, since LOT-A is picked-not-shipped and on a pending pick');
Assert.AreEqual(5, WarehouseActivityLine."Qty. (Base)", 'Movement quantity should be 5');Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6
| GeneralPostingSetup: Record "General Posting Setup"; | ||
| LibraryERM: Codeunit "Library - ERM"; | ||
| begin | ||
| if not GeneralPostingSetup.Get('', Item."Gen. Prod. Posting Group") then begin |
There was a problem hiding this comment.
EnsureGeneralPostingSetupForItem hand-rolls a General Posting Setup record with Init/Validate/Insert even though the test libraries already expose LibraryERM.CreateGeneralPostingSetup for this prerequisite. Using the library keeps the fixture aligned with future setup requirements instead of duplicating record construction inline.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
if not GeneralPostingSetup.Get('', Item."Gen. Prod. Posting Group") then
LibraryERM.CreateGeneralPostingSetup(GeneralPostingSetup, '', Item."Gen. Prod. Posting Group");Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.38.6
Good Sense Reviewer - Round 2Recommendation: AcceptWhat this PR doesThis update narrows the outbound-bin subtraction for movement worksheet availability. It now keeps the subtraction for normal replenishment destinations, but leaves ship-zone movements on the existing path. The new guard addresses the previous concern. The reported FEFO replenishment case still excludes picked-not-shipped and pending-pick quantity, while destination movements into the ship zone are no longer shorted by outbound-bin stock. Status of previous suggestions
New observations (commits since round 1)None - the latest change only addresses the previous suggestion. Risk assessment and necessityRisk: The touched code is in directed put-away and pick availability for movement worksheets. A wrong result can pick the wrong lot or create the wrong movement quantity, but the subtraction is now scoped away from ship-zone destinations and the regression test covers the reported FEFO lot case. Necessity: The change is needed because movement replenishment should not choose stock that is already picked-not-shipped or committed to another pick. The scope now matches that need without changing the ship-zone movement rule.
|
Bug 648630: [master][ALL-E] Movement Worksheet Creates Incorrect Warehouse Movement for FEFO Lot-Tracked Items When Picked-Not-Shipped and Pending Picks Exist. - regression.
Fixes AB#648630
Issue
Movement Worksheet Creates Incorrect Warehouse Movement for FEFO Lot-Tracked Items When Picked-Not-Shipped and Pending Picks Exist.
Root Cause
The availability calculation in CalcTotalAvailQtyToPick (codeunit 7312 "Create Pick") didn't exclude quantity that was already picked but not yet shipped (sitting in an outbound/ship-type bin) or committed to a pending pick. Since a movement can never source stock from the ship zone, this caused the worksheet to select the wrong (FEFO-earliest) lot for the movement — the lot that was already fully committed to outstanding sales orders — instead of the next available lot.
Solution
In CreatePick.Codeunit.al, when called from the Movement Worksheet (CalledFromMoveWksh), subtract the quantity already sitting on outbound bins (WarehouseAvailabilityMgt.CalcQtyOnOutboundBins) from MaxPickableQtyInWhse before it's added to the total available quantity, clamping at zero.