Bug 634238: [Subcontracting] Subcontracting false 'already created' warning when prod order has multiple lines sharing routing/operation

[ChethanT]

Bug

AB#634238 — When a Released Production Order has multiple Prod. Order lines that share the same Routing/Operation but differ by Routing Reference No., creating a Subcontracting Order from the routing of a second line:

• Symptom 1: Falsely raises "Purchase order(s) have already been created. Do you want to view them?" — even though the second line never had a PO.
• Symptom 2: Confirming "view them" opens the unrelated PO of the first line, not the newly-created PO of the second line.

Root cause

Three filter sites identified Prod. Order lines by Prod. Order No. + Routing No. + Operation No. only, missing Routing Reference No. — the field that disambiguates Prod. Order lines sharing the same routing/operation:

• Subc. Purchase Order Creator (codeunit 99001557)
  • ShowExistingPurchaseOrdersForRoutingLines — drives the "already created" prompt.
  • ShowCreatedPurchaseOrder (single-PO navigation branch) — drives which PO opens.
• SubcProdOrderRtng.PageExt.al — the Create Subcontracting Order action's if NoOfCreatedPurchOrder = 0 fallback that decides whether to show the "No subcontracting order was created" message.

The static RunPageLink on the Subcontracting Purchase Order Lines action in SubcProdOrderRtng.PageExt.al already includes Routing Reference No. as a filter — confirming this is the canonical filter pattern the programmatic checks should follow.

Fix

1. SubcPurchaseOrderCreator.Codeunit.al

   • ShowExistingPurchaseOrdersForRoutingLines: add PurchaseLine.SetRange("Routing Reference No.", ProdOrderRoutingLine."Routing Reference No.") to the existence check.
   • ShowCreatedPurchaseOrder: add a RoutingReferenceNo global with Set/Clear accessors (mirroring the existing OperationNo pattern) and apply it in the single-PO navigation branch.

2. SubcProdOrderRtng.PageExt.al

   • The Create Subcontracting Order action now calls ClearRoutingReferenceNoForCreatedPurchaseOrder() + SetRoutingReferenceNoForCreatedPurchaseOrder(Rec."Routing Reference No.") before invoking ShowCreatedPurchaseOrder.
   • The if NoOfCreatedPurchOrder = 0 fallback also filters by "Routing Reference No." = Rec."Routing Reference No.", so the "No subcontracting order was created" message is no longer wrongly suppressed when a sibling Prod. Order line already has an unrelated PO.

The change is minimal and targeted — no broader refactoring.

Tests

Two tests added to codeunit 139989 "Subc. Subcontracting Test". Both build a single Released Production Order with two Prod. Order lines for the same item on different locations (so they share routing/operation but have different Routing Reference No.) and invoke Create Subcontracting Order on each line's routing entry.

1. CreateSubcontractingPOForEachProdOrderLineWhenLinesShareRoutingAndOperation — covers Symptom 1. Captures the confirm-handler question text and asserts each invocation ends with the "1 Purchase Order(s) created" confirmation — i.e. neither raises the false "already created" warning.
2. CreateSubcontractingPONavigatesToOwnPOWhenLinesShareRoutingAndOperation — covers Symptom 2. Confirms "view them" with Yes and captures the opened Purchase Order's No. via a page handler, then asserts the opened PO contains a Purchase Line carrying the invoked routing line's Routing Reference No. — proving the single-PO navigation branch routes to the correct PO.

Both tests fail on the unfixed code and pass after the fix.

The test app gains a dependency on Library - Variable Storage so the confirm/page handlers can enqueue captured values for assertion.

Work item

AB#634238

[ChethanT]

@SPinkow

[github-actions]

AL Documentation Audit

Documentation gaps were detected in the following apps:

• Subcontracting-Test: 9% documentation coverage
• Subcontracting: 0% documentation coverage

To generate documentation, run /al-docs init or /al-docs update using GitHub Copilot CLI or Claude Code.
This review is for awareness to help keep documentation in sync with code changes. It is okay to dismiss this request.

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Shared handler now enqueues data for 20+ unaware tests

DoNotConfirmShowCreatedPurchOrderForSubcontracting is used by 20+ existing tests that were written before it enqueued anything. Those tests will now silently accumulate an unclaimed item in LibraryVariableStorage on each invocation. While LibraryVariableStorage.Clear() in Initialize() prevents cross-test leakage, any test that triggers the handler multiple times in a single run will leave surplus items, which could cause obscure failures if AssertEmpty() is ever added to teardown or a future test.

Recommendation:

• Either add a dedicated handler for the new test (e.g., CaptureAndDenyShowCreatedPurchOrderForSubcontracting) that enqueues, keeping the original handler as a simple Reply := false, or document clearly that all users of the shared handler must dequeue exactly once per invocation.

    [ConfirmHandler]
    procedure CaptureAndDenyShowCreatedPurchOrderForSubcontracting(Question: Text[1024]; var Reply: Boolean)
    begin
        LibraryVariableStorage.Enqueue(Question);
        Reply := false;
    end;

    [ConfirmHandler]
    procedure DoNotConfirmShowCreatedPurchOrderForSubcontracting(Question: Text[1024]; var Reply: Boolean)
    begin
        Reply := false;
    end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Shared Initialize modified; may break existing tests

UpdateSubMgmtSetupWithReqWkshTemplate() is added unconditionally to the shared Initialize() method that runs for all tests in the codeunit. Any existing test that relies on the submanagement setup NOT having a Req. Worksheet Template configured may now silently change behavior or fail.

Recommendation:

• Either call UpdateSubMgmtSetupWithReqWkshTemplate() only within the new test that requires it, or verify by running the full test suite that no existing test behavior is changed by this addition.

// Move call into the new test procedure instead of Initialize():
        Initialize();
        UpdateManufacturingSetupWithSubcontractingLocation();
        UpdateSubMgmtSetupWithReqWkshTemplate(); // only needed for this scenario

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

RoutingReferenceNo not cleared in internal cleanup

The existing cleanup code at line 213 clears OperationNo but does not clear the newly added RoutingReferenceNo. If this procedure is called independently (e.g. in a path that doesn't go through the page extension's explicit Clear+Set calls), RoutingReferenceNo will retain its previous value and incorrectly filter the purchase line lookup.

Recommendation:

• Add Clear(RoutingReferenceNo) alongside Clear(OperationNo) in the same cleanup procedure to maintain symmetry and prevent state leaks.

        Clear(OperationNo);
        Clear(RoutingReferenceNo);

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

RoutingReferenceNo Not Cleared in ShowCreatedPurchaseOrder

ShowCreatedPurchaseOrder clears OperationNo at the end of the procedure (line 213) to avoid state leakage, but the newly introduced RoutingReferenceNo global variable is not cleared there. Any call path that invokes ShowCreatedPurchaseOrder without first calling ClearRoutingReferenceNoForCreatedPurchaseOrder—including batch scenarios or requisition worksheet flows—will use a stale RoutingReferenceNo filter and navigate to the wrong purchase order.

Recommendation:

• Add Clear(RoutingReferenceNo) at the end of ShowCreatedPurchaseOrder, consistent with how OperationNo is already handled.

Clear(OperationNo);
Clear(RoutingReferenceNo);

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

ClearOperationNo doesn't reset RoutingReferenceNo

The existing ClearOperationNoForCreatedPurchaseOrder procedure only clears OperationNo, so any caller that resets filter state via that procedure alone will leave a stale RoutingReferenceNo that incorrectly narrows the FindFirst() query in ShowCreatedPurchaseOrder.

Recommendation:

• Add Clear(RoutingReferenceNo) inside ClearOperationNoForCreatedPurchaseOrder, or introduce a single combined ClearNavigationFiltersForCreatedPurchaseOrder procedure that resets both variables, and update the pageext to call only that.

internal procedure ClearOperationNoForCreatedPurchaseOrder()
begin
    Clear(OperationNo);
    Clear(RoutingReferenceNo);
end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

RoutingReferenceNo not cleared after ShowCreatedPurchaseOrder

The existing OperationNo global variable is cleared at the end of ShowCreatedPurchaseOrder (Clear(OperationNo) at line 213), but the newly introduced RoutingReferenceNo is never cleared inside that procedure. Any caller that does not manually call ClearRoutingReferenceNoForCreatedPurchaseOrder first will see the previous invocation's stale value silently applied to subsequent purchase-line filter lookups.

Recommendation:

• Add Clear(RoutingReferenceNo); immediately after the existing Clear(OperationNo); at the end of ShowCreatedPurchaseOrder to keep the two fields symmetric and self-cleaning.

        Clear(OperationNo);
        Clear(RoutingReferenceNo);
    end;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Paired Clear/Set API is fragile; consider a single setter

Exposing ClearRoutingReferenceNoForCreatedPurchaseOrder and SetRoutingReferenceNoForCreatedPurchaseOrder as two separate internal procedures means every caller must remember to call both in the right order. If a caller only calls Set without Clear first, it still works correctly (assignment overwrites), but if a caller only calls Clear without Set, the default 0 silently disables the filter with no indication that an error occurred.

Recommendation:

• Consolidate into a single setter that always overwrites the previous value, matching the mental model callers actually need. Expose a separate ClearAll or no-argument reset only if there is a genuine use-case for explicitly resetting to "no filter".

internal procedure SetRoutingReferenceNoForCreatedPurchaseOrder(RoutingReferenceNoToSet: Integer)
begin
    RoutingReferenceNo := RoutingReferenceNoToSet;
end;

// Remove ClearRoutingReferenceNoForCreatedPurchaseOrder and update caller:
SubcPurchaseOrderCreator.SetRoutingReferenceNoForCreatedPurchaseOrder(Rec."Routing Reference No.");

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

UpdateSubMgmtSetupWithReqWkshTemplate runs on every test

UpdateSubMgmtSetupWithReqWkshTemplate() is placed before the if IsInitialized then exit guard, so it executes for every single test in the codeunit, not just once. Depending on what this function does (database writes, setup changes), this causes unnecessary overhead and could interfere with tests that do not expect the requisition worksheet template to be updated.

Recommendation:

• Move UpdateSubMgmtSetupWithReqWkshTemplate() inside the one-time initialization block (after the if IsInitialized then exit guard), if only the new tests rely on it. If it must run per-test, add a comment explaining why.

if IsInitialized then
    exit;

UpdateSubMgmtSetupWithReqWkshTemplate();
// ... other one-time setup ...
IsInitialized := true;

Line mapping was unavailable, so this was posted as an issue comment.

_{👍 useful · ❤️ especially valuable · 👎 wrong - reply with why}

[alexei-dobriansky]

Analysis

Scenario Validity:

• Valid and clearly described. The linked ADO bug and its [AI-REPRO] comment spell out a deterministic repro: create a released production order with two lines sharing routing/operation but differing by Routing Reference No., create a subcontracting PO for line 1, then create one for line 2. The AI repro explicitly states that the second action incorrectly shows "Purchase order(s) have already been created. Do you want to view them?" even though line 2 never had a PO, and that navigation then opens the PO for line 1.
• The scenario is important enough to justify a fix. Official documentation says subcontract purchase orders are created from released production order routings, and only collapse to one PO when the subcontracted operations go to the same vendor location. Here the repro deliberately uses different locations, so separate POs are expected and the current behavior is wrong.

Correctness:

• The root cause in the diff matches the bug report. Before this PR, ShowExistingPurchaseOrdersForRoutingLines filtered purchase lines by Prod. Order No., Routing No., and Operation No. only; this PR adds Routing Reference No. at SubcPurchaseOrderCreator.Codeunit.al:263-266. That directly prevents line 2 from seeing line 1's PO as an existing match.
• The fallback in SubcProdOrderRtng.PageExt.al:139-147 is corrected the same way by adding PurchaseLine.SetRange("Routing Reference No.", Rec."Routing Reference No."). That keeps the "No subcontracting order was created" decision aligned with the same key.
• The navigation fix is also targeted. ShowCreatedPurchaseOrder now applies the stored RoutingReferenceNo in the single-PO branch (SubcPurchaseOrderCreator.Codeunit.al:194-199), and the action wires that value in immediately before the call (SubcProdOrderRtng.PageExt.al:150-156). For the exact repro, the second invocation now looks up the just-created PO line for the selected routing line instead of the sibling line.
• This aligns with the existing in-area analog already present in the product: the static RunPageLink on Subcontracting Purchase Order Lines already filters by Prod. Order No., Routing No., Routing Reference No., and Operation No. (SubcProdOrderRtng.PageExt.al:74-80). The PR makes the programmatic checks follow the same discriminator set.
• I do not see dead code introduced by the fix. The new filter is conditional on the single-PO path only, which is the path that needed narrowing.
• BaseApp verification is not applicable here: the PR adds no new subscribers and does not depend on event timing in <baseapp>.

Side Effects:

• The change narrows lookups; it does not broaden behavior. Previously working scenarios where the same routing line already owns the PO should still match because the routing reference for that line is unchanged.
• The main behavioral change is that sibling production-order lines no longer collide when they share routing/operation. That is exactly the intended correction.
• No public API or event surface changes are introduced. The added procedures are internal, and the change stays inside the Subcontracting app flow.

Risk Assessment:

• Medium-Low. The business area is financially sensitive because it creates purchase orders, but the implementation is a small filter correction in three places plus two focused regression tests.
• If the fix were wrong, the blast radius is limited to subcontracting PO detection/navigation for production routing lines.
• What if we don't make this change? Users can still create the second PO, but the workflow remains misleading and opens the wrong document, making it easy to inspect or act on the wrong purchase order. The workaround is manual lookup of the correct PO, which is error-prone and not acceptable for a standard manufacturing flow.

Test Coverage:

• CreateSubcontractingPOForEachProdOrderLineWhenLinesShareRoutingAndOperation (SubcSubcontractingTest.Codeunit.al:2530-2597) covers the false-warning symptom. It invokes the action for both routing lines, captures the confirm text through the handler, asserts the result is the normal created message, and calls LibraryVariableStorage.AssertEmpty() so the test would fail if the old extra warning still appeared.
• CreateSubcontractingPONavigatesToOwnPOWhenLinesShareRoutingAndOperation (SubcSubcontractingTest.Codeunit.al:2600-2666) covers the wrong-navigation symptom. The page handler captures the opened PO number (2735-2739), then the test asserts that PO contains a purchase line with the selected line's Routing Reference No.. That is the right assertion point: it proves the changed lookup opened the correct document, not merely that some downstream state looked plausible.
• The tests also include the happy path implicitly by looping over both production-order lines; line 1 verifies the pre-existing scenario still behaves normally, while line 2 verifies the regression fix.
• GitHub CI is green, including the W1 unit/unclassified test legs, which is consistent with the change being narrow and well covered.

Recommendation: Accept

1. No further changes requested — the bug is clearly documented, the diff fixes the exact missing discriminator in the affected lookups, the implementation aligns with the existing RunPageLink pattern, and the two added regression tests exercise both the false-warning and wrong-navigation symptoms directly.