[Master] Different source reference on reservation entries created directly from Job Planning Line and via a warehouse pick.#8905
Conversation
github-actions
Bot
added
From Fork
|
@microsoft-github-policy-service agree company="Microsoft" |
Copilot PR ReviewIteration 11 · Outcome: completed Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630 Findings by domainFindings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).
Totals: 2 knowledge-backed · 0 agent findings. Orchestrator pre-filter (13 file(s) excluded)
Findings produced by the Copilot CLI agent against BCQuality at |
Agentic PR Review - Round 1Recommendation: Request ChangesWhat this PR doesThis PR changes job warehouse source references from Database::Job / subtype � to Database::"Job Planning Line" / subtype Order, so warehouse picks use the same source key as direct Job Planning Line reservations. That matches the bug evidence: direct reservations use source type 1003 and subtype 2, while picks currently use Job / �, so reservation availability can be double-counted after picking. The main creation and item-tracking paths are updated, and the new inventory-pick and warehouse-pick tests cover the core repro. However, the upgrade path for existing warehouse records is not safe, so tenants with open job warehouse requests or picks can be left with corrupted source references. SuggestionsS1 - Fix the warehouse upgrade migration S2 - Keep Show Source Document working Risk assessment and necessityRisk: High regression surface. The change touches reservation, item tracking, warehouse pick/register/post, location validation, upgrade code, and localized upgrade code. Wrong source references can affect availability calculations and item tracking, and the current upgrade routine can also rewrite source subtype on unrelated warehouse records. Necessity: The change is required. The linked Bug 630792 / AI Repro shows a real mismatch: direct reservations use Job Planning Line / Order while job warehouse picks use Job / 0, causing later reservations to see wrong availability. The behavioral scope is appropriate, but the migration must be corrected before merge.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if not WhseValidateSourceLine.WhseLinesExist( | ||
| Database::"Job Planning Line", "Job Planning Line Status"::Order.AsInteger(), NewJobPlanningLine."Job No.", NewJobPlanningLine."Job Contract Entry No.", NewJobPlanningLine."Line No.", NewJobPlanningLine.Quantity) | ||
| then begin | ||
| // Check for WhseWorksheetLines using flexible filter that handles both old and new formats |
There was a problem hiding this comment.
In codeunit 5998 "Job Warehouse Mgt.", both JobPlanningLineVerifyChange and JobPlanningLineDelete used to detect an existing Whse.
Worksheet Line via WhseValidateSourceLine.WhseWorkSheetLinesExist(...), which raises the [IntegrationEvent] OnBeforeWhseWorkSheetLinesExist before querying the table. The PR replaced both call sites with an inline WhseWorksheetLine.SetFilter("Source Type", '%1|%2', Database::Job, Database::"Job Planning Line") query that never goes through WhseWorkSheetLinesExist, so OnBeforeWhseWorkSheetLinesExist is no longer raised for Job/Job Planning Line worksheet-existence checks. Any partner extension subscribing to that event to customize or suppress the 'cannot change/cannot delete while a worksheet line exists' behaviour for Job sources will silently stop being invoked. The equivalent WhseLinesExist calls in the same procedures were correctly kept as two calls (one per source type) to preserve their event; the worksheet-line check could have used the same two-call pattern against WhseWorkSheetLinesExist instead of hand-rolling the query.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
In codeunit 7322 "Create Inventory Pick/Movement", local procedure GetSourceLineNo (line 2294-2299) still checks only 'if WarehouseActivityLine."Source Type" <> Database::Job then exit(WarehouseActivityLine."Source Line No."); exit(-1);'.Every other Source-Type switch in this same PR and this same file (e.g. line 1343: 'if WarehouseActivityLine."Source Type" in [Database::Job, Database::"Job Planning Line"] then') was updated to treat the new Database::"Job Planning Line" source type the same way as the legacy Database::Job, but this one was missed. Since job-sourced pick/movement lines are now created with Source Type = Database::"Job Planning Line" (see WarehouseActivityLine.Table.al SetSource and CreateInventoryPickMovement.Codeunit.al), the condition now evaluates the opposite of the pre-upgrade behaviour: it returns the real 'Source Line No.' instead of the sentinel -1. This value flows straight into WarehouseAvailabilityMgt.CalcLineReservedQtyOnInvt as the SourceLineNo/'Source Ref. No.' filter argument, which narrows the reserved-quantity-on-inventory calculation to a single Job Planning Line's reservation entries instead of aggregating across the whole Job the way it did before the source-type migration -- silently changing the computed available-to-pick quantity for every job-sourced inventory pick going forward. If the intended production impact were confirmed, this would be a functional (major) regression in inventory availability; it is capped to minor here because it has no backing knowledge-file citation. Recommendation:
Suggested fix (apply manually — could not be anchored as a one-click suggestion): local procedure GetSourceLineNo(WarehouseActivityLine: Record "Warehouse Activity Line"): Integer
begin
if not (WarehouseActivityLine."Source Type" in [Database::Job, Database::"Job Planning Line"]) then
exit(WarehouseActivityLine."Source Line No.");
exit(-1);Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why |
|
|
||
| var | ||
| CannotChangeFieldWithWhseWorksheetErr: Label 'must not be changed when a %1 for this %2 exists: ', Comment = '%1 = Whse. Worksheet Line table caption, %2 = Job Planning Line table caption'; | ||
| CannotDeleteWithWhseWorksheetErr: Label 'The %1 cannot be deleted when a related %2 exists.', Comment = '%1 = Table 1 caption, %2 = Table 2 caption'; |
There was a problem hiding this comment.
The new Label CannotDeleteWithWhseWorksheetErr in codeunit 5998 "Job Warehouse Mgt." has two placeholders (%1, %2) but its Comment reads 'Comment = '%1 = Table 1 caption, %2 = Table 2 caption'' -- it only restates the placeholder names instead of naming what they actually are.
The call site is Error(CannotDeleteWithWhseWorksheetErr, JobPlanningLine.TableCaption(), WhseWorksheetLine.TableCaption()), so %1 is the Job Planning Line table caption and %2 is the Whse. Worksheet Line table caption. Translators cannot disambiguate a vague comment like this, and the sibling label two lines above (CannotChangeFieldWithWhseWorksheetErr) already documents its placeholders correctly, showing the intended convention was simply not followed here.
| CannotDeleteWithWhseWorksheetErr: Label 'The %1 cannot be deleted when a related %2 exists.', Comment = '%1 = Table 1 caption, %2 = Table 2 caption'; | |
| CannotDeleteWithWhseWorksheetErr: Label 'The %1 cannot be deleted when a related %2 exists.', Comment = '%1 = Job Planning Line table caption, %2 = Whse. Worksheet Line table caption'; |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
…erenceOnReservEntryFrmJPL
| // Upgrade Warehouse Request | ||
| // Source Type and Source Subtype are part of the primary key (Type, Location Code, Source Type, Source Subtype, Source No.), | ||
| // so DataTransfer cannot be used to update them. Rename each record to the new key values instead. | ||
| WarehouseRequest.SetRange("Source Type", Database::Job); |
There was a problem hiding this comment.
In UpgradeWarehouseActivitySourceTypeForJobPlanningLine, the loop filters WarehouseRequest on (Source Type = Job, Source Subtype = 0) via FindSet()/Next(), then calls WarehouseRequest.Rename(...) on that SAME record instance to change its Source Type/Subtype to the new Job Planning Line/Order key.
Renaming the record fields that define the active filter/current key while iterating with FindSet/Next on that same variable risks corrupting the cursor's position: after the first row is renamed to Source Type = Database::"Job Planning Line" (a higher table ID than Database::Job), the platform's Next() call resumes from the new (renamed) key value, which now falls outside the filtered range of remaining (Source Type = Job) rows. In practice this is very likely to make the loop terminate after processing only the first matching row per iteration path, silently leaving any additional legacy Warehouse Request rows un-upgraded on tenants with more than one. This codebase already has an established, deliberate fix for exactly this hazard: UpgradePermissionSets.Codeunit.al and UpgradeAppIDPermissions.Codeunit.al both iterate with an Old*/WithoutAppId record and explicitly re-Get() the row into a separate Current/*WithAppId record before Delete/Rename, specifically to avoid disturbing the iterator. The new procedure does not follow that pattern.
Recommendation:
- introduce a second record variable (e.g. WarehouseRequestToRename), re-Get() the current row into it by its original key inside the loop, and call Rename on that separate variable instead of on the iterator (WarehouseRequest) itself. Note: because this is an agent-identified concern with no existing BCQuality knowledge-file citation, its severity is capped at 'minor' here per policy, but the underlying data-migration-correctness risk is materially higher (potentially major) and this pattern should be promoted to a knowledge-backed BCQuality upgrade rule. This exact new local procedure is duplicated verbatim across all 8 touched country layers (AT, BE, CH, IT, NL, NO, RU, W1), so the same fix is needed in each.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Agentic PR Review - Round 2Recommendation: Request ChangesWhat this PR doesThe new commits address the prior migration and navigation feedback. The upgrade now uses source filters and constants for Warehouse Activity Line and Whse. Worksheet Line, renames Warehouse Request rows because source fields are part of its key, and adds upgrade tests for those three tables. The Show Source Document subscriber now handles Status of previous suggestions
New observations (commits since round 1)S3 - Keep job availability aggregation Risk assessment and necessityRisk: High regression surface. The PR changes reservation source references across warehouse picks, inventory picks, item tracking, availability calculation, posting/register flows, and upgrade code. The migration fixes reduce upgrade risk, but the remaining Necessity: The change is required. Bug 630792 is a valid reservation-engine defect: direct Job Planning Line reservations and warehouse-pick reservations need the same source reference to avoid wrong availability. The scope is still appropriate, but the availability helper must be aligned before merge.
|
alexei-dobriansky
left a comment
alexei-dobriansky
left a comment
There was a problem hiding this comment.
Please review S3
Agentic PR Review - Round 3Recommendation: AcceptWhat this PR doesThe new commit addresses the open round-2 availability issue in inventory pick creation for job planning lines. The added test creates three reserved job planning lines for the same item and location, then creates one inventory pick. This covers the regression path because pick creation must keep reserved availability across all planning lines while it creates each pick line. Status of previous suggestions
New observations (commits since round 2)None - the new commit addresses the remaining open suggestion. Risk assessment and necessityRisk: High regression surface because this is reservation-engine and warehouse availability logic. The new commit is narrow and directly fixes the remaining aggregation risk. Necessity: The change is required for Bug 630792. Direct Job Planning Line reservations and inventory/warehouse picks must use compatible source references so availability does not show false shortages.
|
…erenceOnReservEntryFrmJPL
| if UpgradeTag.HasUpgradeTag(UpgradeTagDefinitions.GetWarehouseActivitySourceTypeForJobPlanningLineUpgradeTag()) then | ||
| exit; | ||
|
|
||
| // Upgrade Warehouse Activity Line (Source Type / Source Subtype are not part of the primary key -> safe to use DataTransfer) |
There was a problem hiding this comment.
UpgradeWarehouseActivitySourceTypeForJobPlanningLine uses DataTransfer.CopyFields() to overwrite the pre-existing 'Source Type'/'Source Subtype' fields on Warehouse Activity Line and Whse.
Worksheet Line. The accompanying comment only justifies why DataTransfer is safe with respect to the primary key ('not part of the primary key -> safe to use DataTransfer'); per the referenced guidance, DataTransfer on a pre-existing field additionally requires an explicit comment stating that validation triggers and event subscribers are intentionally not raised, plus confirmation that skipping them is safe.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| repeat | ||
| // Guard against a target record already existing with the new key values | ||
| if not WarehouseRequest2.Get(WarehouseRequest.Type, WarehouseRequest."Location Code", Database::"Job Planning Line", "Job Planning Line Status"::Order.AsInteger(), WarehouseRequest."Source No.") then | ||
| WarehouseRequest.Rename(WarehouseRequest.Type, WarehouseRequest."Location Code", Database::"Job Planning Line", "Job Planning Line Status"::Order.AsInteger(), WarehouseRequest."Source No."); |
There was a problem hiding this comment.
UpgradeWarehouseActivitySourceTypeForJobPlanningLine renames legacy Warehouse Request rows to the new (Database::"Job Planning Line", Order) primary key with an unguarded call: WarehouseRequest.Rename(...) — no `if not ...
thencheck and no telemetry fallback. Per the referenced guidance, an unguarded erroring API inside upgrade code aborts the whole per-company upgrade the first time it hits a row it did not anticipate (e.g. a Rename collision, a locked row, or any other Rename failure not caught by the preceding WarehouseRequest2.Get duplicate-key check). Compounding this, Rename is called onWarehouseRequest— the exact same record variable driving the enclosingFindSet()/repeat/until WarehouseRequest.Next() = 0 loop. Renaming the loop's own cursor record changes the primary-key fields ("Source Type", "Source Subtype") that the loop's SetRange filter and default key ordering depend on, which risks the iteration skipping or reprocessing rows. This codebase already has an established, safer pattern for exactly this situation: src/Layers/W1/BaseApp/OtherCapabilities/Upgrade/UpgradeAppIDPermissions.Codeunit.al (SetAppIdOnUserGroup* procedures) explicitly re-Gets the row into a *separate* record variable before renaming ("Get the current record to avoid side effects from FindSet()") and guards the Rename with if not ... then Session.LogMessage(...)` instead of letting the error propagate. The same fix should be applied here. The identical code is duplicated verbatim in 8 layers and needs the same fix in each: src/Layers/AT/BaseApp/OtherCapabilities/Upgrade/UpgradeBaseApp.Codeunit.al:4051, src/Layers/BE/.../UpgradeBaseApp.Codeunit.al:4163, src/Layers/CH/.../UpgradeBaseApp.Codeunit.al:4117, src/Layers/IT/.../UpgradeBaseApp.Codeunit.al:4182, src/Layers/NL/.../UpgradeBaseApp.Codeunit.al:4099, src/Layers/NO/.../UpgradeBaseApp.Codeunit.al:4052, src/Layers/RU/.../UpgradeBaseApp.Codeunit.al:4202, src/Layers/W1/.../UpgradeBaseApp.Codeunit.al:4096.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| "Source Type" := Database::Job; | ||
| begin | ||
| "Source Type" := Database::"Job Planning Line"; | ||
| "Source Subtype" := 2; // Job Planning Line Status::Order |
There was a problem hiding this comment.
In the new "Job Usage" branch of the Source Document case, "Source Subtype" is hardcoded to the literal 2 with only a comment explaining it means Job Planning Line Status::Order.
Every other one of the roughly fifteen call sites this same PR touches (WarehouseActivityLine.Table.al, WhseWorksheetCreate.Codeunit.al, CreatePick.Codeunit.al, JobPlanningLine.Table.al, etc.) uses the symbolic "Job Planning Line Status"::Order.AsInteger() instead, including the case immediately above it (Assembly Consumption) which uses AssemblyLine."Document Type"::Order.AsInteger(). The literal is fragile against enum-value reordering and inconsistent with the rest of the change; replace it with the symbolic form used everywhere else in this PR.
| "Source Subtype" := 2; // Job Planning Line Status::Order | |
| "Source Subtype" := "Job Planning Line Status"::Order.AsInteger(); |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| JobPlanningLine.SetRange(Type, JobPlanningLine.Type::Item); | ||
| JobPlanningLine.SetFilter("Quantity", '>0'); | ||
| JobPlanningLine.SetLoadFields(JobPlanningLine."Job No.", JobPlanningLine."Job Contract Entry No.", JobPlanningLine."Line No."); | ||
| JobPlanningLine.SetLoadFields(JobPlanningLine."Job No.", JobPlanningLine."Job Contract Entry No.", JobPlanningLine."Line No.", JobPlanningLine.Status); |
There was a problem hiding this comment.
SetLoadFields on JobPlanningLine now includes Status, but Status is never read in this loop (the call to InitItemTrackingForTempWhseWorksheetLine passes a hardcoded "Job Planning Line Status"::Order.AsInteger(), not JobPlanningLine.Status).
Per the referenced guidance, SetLoadFields should list exactly the fields the procedure reads; loading an unused field adds unnecessary data transfer for every row in the loop.
| JobPlanningLine.SetLoadFields(JobPlanningLine."Job No.", JobPlanningLine."Job Contract Entry No.", JobPlanningLine."Line No.", JobPlanningLine.Status); | |
| JobPlanningLine.SetLoadFields(JobPlanningLine."Job No.", JobPlanningLine."Job Contract Entry No.", JobPlanningLine."Line No."); |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
3 participants
Bug 630792: [master] [ALL-E] Different source reference on reservation entries created directly from Job Planning Line and via a warehouse pick.
AB#630792