[Bug 634238] 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{🟡\ Medium\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Hardcoded label text in test assertion

The assertion compares LibraryVariableStorage.DequeueText() against the literal string '1 Purchase Order(s) created.\\Do you want to view them?', duplicating the PurchOrderCreatedTxt label from the production codeunit. Any change to that label text — including punctuation — will silently break this test.

Recommendation:

• Expose PurchOrderCreatedTxt as a testable getter, or assert only the key distinguishing substring (e.g., that it does NOT contain 'already been created') rather than duplicating the full formatted label.

Suggested change

	Assert.AreEqual('1 Purchase Order(s) created.\\Do you want to view them?', LibraryVariableStorage.DequeueText(), 'Expected "created" confirmation for each prod order line, not the false "already created" warning');
	Assert.IsTrue(
	LibraryVariableStorage.DequeueText().Contains('Purchase Order(s) created'),
	'Expected "created" confirmation, not the false "already created" warning');

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

[github-actions]

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

Missing distinct-line guard in second scenario test

The test CreateSubcontractingPONavigatesToOwnPOWhenLinesShareRoutingAndOperation does not assert that ProdOrderLineNo[1] <> ProdOrderLineNo[2], unlike its sibling test. If both lines unexpectedly share the same line number — and thus the same Routing Reference No. — the test would still pass without verifying the multi-line navigation scenario.

Recommendation:

• Add the same guard assertion present in the sibling test immediately after both line numbers are captured.

Suggested change

	ProdOrderLineNo[2] := ProdOrderLine."Line No.";
	ProdOrderLineNo[2] := ProdOrderLine."Line No.";
	Assert.AreNotEqual(ProdOrderLineNo[1], ProdOrderLineNo[2], 'Expected two distinct production order lines with different Routing Reference Nos.');

_{👍 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{🟠\ High\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Shared confirm handler now enqueues—breaks existing tests

DoNotConfirmShowCreatedPurchOrderForSubcontracting is a shared [ConfirmHandler] that pre-existing tests already reference. Adding LibraryVariableStorage.Enqueue(Question) means every invocation from tests that do not subsequently call LibraryVariableStorage.DequeueText() will leave variable storage dirty, causing those tests to fail at any subsequent LibraryVariableStorage.AssertEmpty() call or polluting the next test's dequeue order.

Recommendation:

• Instead of modifying the shared handler, introduce a new dedicated confirm handler (e.g., CaptureAndRejectShowCreatedPurchOrderForSubcontracting) for the new tests that need to inspect the question text, and keep the original handler unchanged.

Suggested change

	LibraryVariableStorage.Enqueue(Question);
	[ConfirmHandler]
	procedure CaptureAndRejectShowCreatedPurchOrderForSubcontracting(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;

_{👍 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 init

UpdateSubMgmtSetupWithReqWkshTemplate() is added before the if IsInitialized then exit guard, so it runs unconditionally on every test's Initialize() call rather than only once per test session. If any other test reconfigures the Requisition Worksheet template to a different value after Initialize, that configuration will be overwritten the next time Initialize is called.

Recommendation:

• Move the call inside the if not IsInitialized then block (or equivalent one-time init guard) so that it only runs during first-time setup, consistent with how the rest of the one-time setup is structured.

Suggested change

	UpdateSubMgmtSetupWithReqWkshTemplate();
	if IsInitialized then
	exit;
	UpdateSubMgmtSetupWithReqWkshTemplate();
	// remaining one-time init ...

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

[github-actions]

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

RoutingReferenceNo = 0 silently skips the filter

The guard (RoutingReferenceNo <> 0) means that if a prod-order routing line genuinely has Routing Reference No. = 0 (e.g. a line created outside the normal production order refresh flow), the filter is not applied and FindFirst() can return a purchase line from the wrong routing context, reopening the original bug for those records.

Recommendation:

• Document that Routing Reference No. = 0 is intentionally excluded, or — if routing lines with value 0 are possible — add a dedicated boolean flag (similar to how OperationNo uses an empty-string check which is unambiguous for Code fields) to explicitly indicate whether the caller has set the filter.

Suggested change

	if (NoOfCreatedPurchOrder = 1) and (RoutingReferenceNo <> 0) then
	// Option: use a separate flag to distinguish "not set" from "value is 0"
	if (NoOfCreatedPurchOrder = 1) and RoutingReferenceNoIsSet then
	PurchaseLine.SetRange("Routing Reference No.", RoutingReferenceNo);

_{👍 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}