Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ codeunit 99001557 "Subc. Purchase Order Creator"
HasSubManagementSetup: Boolean;
HasManufacturingSetup: Boolean;
OperationNo: Code[10];
RoutingReferenceNo: Integer;
PurchOrderCreatedTxt: Label '%1 Purchase Order(s) created.\\Do you want to view them?', Comment = '%1 = No of Purchase Order(s) created.';
PurchOrderAlreadyCreatedQst: Label 'Purchase order(s) have already been created.\\Do you want to view them?';
CreationOfSubcontractingOrderIsNotAllowedErr: Label 'You cannot create Subcontracting Order, because the Production Order %1 is not released.', Comment = '%1=Production Order No.';
Expand Down Expand Up @@ -193,6 +194,8 @@ codeunit 99001557 "Subc. Purchase Order Creator"
PurchaseLine.SetLoadFields(SystemId);
if (NoOfCreatedPurchOrder = 1) and (OperationNo <> '') then
PurchaseLine.SetRange("Operation No.", OperationNo);
if (NoOfCreatedPurchOrder = 1) and (RoutingReferenceNo <> 0) then
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

$\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

PurchaseLine.SetRange("Routing Reference No.", RoutingReferenceNo);
PurchaseLine.FindFirst();
PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No.");
PageManagement.PageRun(PurchaseHeader);
Expand All @@ -210,6 +213,16 @@ codeunit 99001557 "Subc. Purchase Order Creator"
Clear(OperationNo);
end;

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

internal procedure ClearRoutingReferenceNoForCreatedPurchaseOrder()
begin
Clear(RoutingReferenceNo);
end;

local procedure CheckProdOrderRtngLine(ProdOrderRoutingLine: Record "Prod. Order Routing Line"; var ProdOrderLine: Record "Prod. Order Line")
var
WorkCenter: Record "Work Center";
Expand Down Expand Up @@ -249,6 +262,7 @@ codeunit 99001557 "Subc. Purchase Order Creator"
PurchaseLine.SetRange(Type, "Purchase Line Type"::Item);
PurchaseLine.SetRange("Prod. Order No.", ProdOrderRoutingLine."Prod. Order No.");
PurchaseLine.SetRange("Routing No.", ProdOrderRoutingLine."Routing No.");
PurchaseLine.SetRange("Routing Reference No.", ProdOrderRoutingLine."Routing Reference No.");
PurchaseLine.SetRange("Operation No.", ProdOrderRoutingLine."Operation No.");
if not PurchaseLine.IsEmpty() then
ExistingPOFound := true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -142,13 +142,16 @@ pageextension 99001503 "Subc. Prod. Order Rtng." extends "Prod. Order Routing"
PurchaseLine.SetRange(Type, PurchaseLine.Type::Item);
PurchaseLine.SetRange("Prod. Order No.", Rec."Prod. Order No.");
PurchaseLine.SetRange("Routing No.", Rec."Routing No.");
PurchaseLine.SetRange("Routing Reference No.", Rec."Routing Reference No.");
PurchaseLine.SetRange("Operation No.", Rec."Operation No.");
if PurchaseLine.IsEmpty() then
Message(NoPurchOrderCreatedMsg, ProdOrderRoutingLine."Prod. Order No.")
end else begin
if NoOfCreatedPurchOrder = 1 then begin
SubcPurchaseOrderCreator.ClearOperationNoForCreatedPurchaseOrder();
SubcPurchaseOrderCreator.SetOperationNoForCreatedPurchaseOrder(Rec."Operation No.");
SubcPurchaseOrderCreator.ClearRoutingReferenceNoForCreatedPurchaseOrder();
SubcPurchaseOrderCreator.SetRoutingReferenceNoForCreatedPurchaseOrder(Rec."Routing Reference No.");
end;
SubcPurchaseOrderCreator.ShowCreatedPurchaseOrder(Rec."Prod. Order No.", NoOfCreatedPurchOrder);
end;
Expand Down
6 changes: 6 additions & 0 deletions src/Apps/W1/Subcontracting/Test/app.json
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,12 @@
"name": "Tests-TestLibraries",
"publisher": "Microsoft",
"version": "29.0.0.0"
},
{
"id": "5095f467-0a01-4b99-99d1-9ff1237d286f",
"publisher": "Microsoft",
"name": "Library Variable Storage",
"version": "29.0.0.0"
}
],
"platform": "29.0.0.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ using Microsoft.Sales.Document;
using Microsoft.Utilities;
using Microsoft.Warehouse.Document;
using Microsoft.Warehouse.Structure;
using System.TestLibraries.Utilities;

codeunit 139989 "Subc. Subcontracting Test"
{
Expand Down Expand Up @@ -2526,6 +2527,145 @@ codeunit 139989 "Subc. Subcontracting Test"
Assert.ExpectedError('Nothing to create. No components or WIP items to return for the specified subcontracting order');
end;

[Test]
[HandlerFunctions('DoNotConfirmShowCreatedPurchOrderForSubcontracting')]
procedure CreateSubcontractingPOForEachProdOrderLineWhenLinesShareRoutingAndOperation()
var
Item: Record Item;
ProductionLocation: array[2] of Record Location;
MachineCenter: array[2] of Record "Machine Center";
ProdOrderLine: Record "Prod. Order Line";
ProdOrderRoutingLine: Record "Prod. Order Routing Line";
ProductionOrder: Record "Production Order";
WorkCenter: array[2] of Record "Work Center";
ProdOrderRtng: TestPage "Prod. Order Routing";
I: Integer;
ProdOrderLineNo: array[2] of Integer;
begin
// [SCENARIO 634238] When a Released Production Order has multiple Prod. Order lines sharing the same
// Routing/Operation, creating a Subcontracting Order for the second line must not raise the false
// "Purchase order(s) have already been created" warning, and must create/show its own Purchase Order.

// [GIVEN] Subcontracting setup with direct transfer (no in-transit route)
Initialize();
SubcontractingMgmtLibrary.UpdateManufacturingSetupWithSubcontractingLocation();
Subcontracting := true;
UnitCostCalculation := UnitCostCalculation::Units;
CreateAndCalculateNeededWorkAndMachineCenter(WorkCenter, MachineCenter);
UpdateVendorWithSubcontractingLocationCode(WorkCenter[2]);
CreateItemForProductionIncludeRoutingAndProdBOM(Item, WorkCenter, MachineCenter);
UpdateProdBomAndRoutingWithRoutingLink(Item, WorkCenter[2]."No.");

// [GIVEN] One released production order created directly from item
LibraryManufacturing.CreateProductionOrder(ProductionOrder, "Production Order Status"::Released, ProductionOrder."Source Type"::Item, Item."No.", 0);

Comment thread
ChethanT marked this conversation as resolved.
// [GIVEN] No production order lines exist yet for this order
ProdOrderLine.SetRange(Status, ProdOrderLine.Status::Released);
ProdOrderLine.SetRange("Prod. Order No.", ProductionOrder."No.");
Assert.AreEqual(0, ProdOrderLine.Count(), 'Expected no production order lines to exist before manually creating them');

// [GIVEN] Two production order lines on the same production order, on different locations
LibraryWarehouse.CreateLocationWithInventoryPostingSetup(ProductionLocation[1]);
LibraryWarehouse.CreateLocationWithInventoryPostingSetup(ProductionLocation[2]);
LibraryManufacturing.CreateProdOrderLine(ProdOrderLine, ProductionOrder.Status, ProductionOrder."No.", Item."No.", '', ProductionLocation[1].Code, LibraryRandom.RandInt(10) + 2);
ProdOrderLineNo[1] := ProdOrderLine."Line No.";
Comment thread
ChethanT marked this conversation as resolved.
LibraryManufacturing.CreateProdOrderLine(ProdOrderLine, ProductionOrder.Status, ProductionOrder."No.", Item."No.", '', ProductionLocation[2].Code, LibraryRandom.RandInt(10) + 2);
ProdOrderLineNo[2] := ProdOrderLine."Line No.";
Assert.AreNotEqual(ProdOrderLineNo[1], ProdOrderLineNo[2], 'Expected two distinct production order lines');

// [GIVEN] Refresh the production order to update the routing and component lines
LibraryManufacturing.RefreshProdOrder(ProductionOrder, false, false, true, true, false);

// [GIVEN] The two production-order lines have transfer components on different locations
for I := 1 to 2 do begin
ProdOrderRoutingLine.Reset();
ProdOrderRoutingLine.SetRange(Status, ProdOrderRoutingLine.Status::Released);
ProdOrderRoutingLine.SetRange("Prod. Order No.", ProductionOrder."No.");
ProdOrderRoutingLine.SetRange("Routing Reference No.", ProdOrderLineNo[I]);
ProdOrderRoutingLine.SetRange("Work Center No.", WorkCenter[2]."No.");
Assert.RecordCount(ProdOrderRoutingLine, 1);

ProdOrderRoutingLine.FindFirst();

ProdOrderRtng.OpenView();
ProdOrderRtng.GoToRecord(ProdOrderRoutingLine);
ProdOrderRtng.CreateSubcontracting.Invoke();
Comment thread
ChethanT marked this conversation as resolved.
ProdOrderRtng.Close();

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');
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

$\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

LibraryVariableStorage.AssertEmpty();
end;
end;

[Test]
[HandlerFunctions('ConfirmYesShowSubcontractingPurchOrders,CapturePurchaseOrderPageNo')]
procedure CreateSubcontractingPONavigatesToOwnPOWhenLinesShareRoutingAndOperation()
var
Item: Record Item;
ProductionLocation: array[2] of Record Location;
MachineCenter: array[2] of Record "Machine Center";
ProdOrderLine: Record "Prod. Order Line";
ProdOrderRoutingLine: Record "Prod. Order Routing Line";
ProductionOrder: Record "Production Order";
PurchaseLine: Record "Purchase Line";
WorkCenter: array[2] of Record "Work Center";
ProdOrderRtng: TestPage "Prod. Order Routing";
I: Integer;
ProdOrderLineNo: array[2] of Integer;
OpenedPurchaseOrderNo: Code[20];
begin
// [SCENARIO 634238] When a Released Production Order has multiple Prod. Order lines sharing routing/operation,
// confirming "view them" on the just-created Subcontracting Order must open the PO tied to the invoked
// routing line, not the unrelated PO of a sibling line.

// [GIVEN] Subcontracting setup with two prod order lines sharing routing/operation but different Routing Reference No.
Initialize();
SubcontractingMgmtLibrary.UpdateManufacturingSetupWithSubcontractingLocation();
Subcontracting := true;
UnitCostCalculation := UnitCostCalculation::Units;
CreateAndCalculateNeededWorkAndMachineCenter(WorkCenter, MachineCenter);
UpdateVendorWithSubcontractingLocationCode(WorkCenter[2]);
CreateItemForProductionIncludeRoutingAndProdBOM(Item, WorkCenter, MachineCenter);
UpdateProdBomAndRoutingWithRoutingLink(Item, WorkCenter[2]."No.");

LibraryManufacturing.CreateProductionOrder(ProductionOrder, "Production Order Status"::Released, ProductionOrder."Source Type"::Item, Item."No.", 0);

LibraryWarehouse.CreateLocationWithInventoryPostingSetup(ProductionLocation[1]);
LibraryWarehouse.CreateLocationWithInventoryPostingSetup(ProductionLocation[2]);
LibraryManufacturing.CreateProdOrderLine(ProdOrderLine, ProductionOrder.Status, ProductionOrder."No.", Item."No.", '', ProductionLocation[1].Code, LibraryRandom.RandInt(10) + 2);
ProdOrderLineNo[1] := ProdOrderLine."Line No.";
LibraryManufacturing.CreateProdOrderLine(ProdOrderLine, ProductionOrder.Status, ProductionOrder."No.", Item."No.", '', ProductionLocation[2].Code, LibraryRandom.RandInt(10) + 2);
ProdOrderLineNo[2] := ProdOrderLine."Line No.";
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

$\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


LibraryManufacturing.RefreshProdOrder(ProductionOrder, false, false, true, true, false);

// [WHEN] Creating a Subcontracting Order from each routing line and confirming "view them"
for I := 1 to 2 do begin
ProdOrderRoutingLine.Reset();
ProdOrderRoutingLine.SetRange(Status, ProdOrderRoutingLine.Status::Released);
ProdOrderRoutingLine.SetRange("Prod. Order No.", ProductionOrder."No.");
ProdOrderRoutingLine.SetRange("Routing Reference No.", ProdOrderLineNo[I]);
ProdOrderRoutingLine.SetRange("Work Center No.", WorkCenter[2]."No.");
ProdOrderRoutingLine.FindFirst();

ProdOrderRtng.OpenView();
ProdOrderRtng.GoToRecord(ProdOrderRoutingLine);
ProdOrderRtng.CreateSubcontracting.Invoke();
ProdOrderRtng.Close();

// [THEN] The page handler opens the Purchase Order whose line carries this routing line's Routing Reference No.
OpenedPurchaseOrderNo := CopyStr(LibraryVariableStorage.DequeueText(), 1, MaxStrLen(OpenedPurchaseOrderNo));
PurchaseLine.Reset();
PurchaseLine.SetRange("Document Type", PurchaseLine."Document Type"::Order);
PurchaseLine.SetRange("Document No.", OpenedPurchaseOrderNo);
PurchaseLine.SetRange(Type, PurchaseLine.Type::Item);
PurchaseLine.SetRange("Prod. Order No.", ProductionOrder."No.");
PurchaseLine.SetRange("Routing Reference No.", ProdOrderLineNo[I]);
Assert.IsFalse(PurchaseLine.IsEmpty(), StrSubstNo(PurchOrderRoutingErr, OpenedPurchaseOrderNo, ProdOrderLineNo[I]));
LibraryVariableStorage.AssertEmpty();
end;
end;

[Test]
[HandlerFunctions('ConfirmYesShowSubcontractingPurchOrders,HandlePurchaseOrderPage,HandlePurchaseLinesPage')]
procedure ShowExistingPurchOrdersOpensListWhenAlreadyCreated()
Expand Down Expand Up @@ -2592,6 +2732,13 @@ codeunit 139989 "Subc. Subcontracting Test"
PurchaseOrderPage.Close();
end;

[PageHandler]
procedure CapturePurchaseOrderPageNo(var PurchaseOrderPage: TestPage "Purchase Order")
begin
LibraryVariableStorage.Enqueue(PurchaseOrderPage."No.".Value);
PurchaseOrderPage.Close();
end;

[PageHandler]
procedure HandlePurchaseLinesPage(var PurchaseLinesPage: TestPage "Purchase Lines")
begin
Expand Down Expand Up @@ -2623,6 +2770,7 @@ codeunit 139989 "Subc. Subcontracting Test"
[ConfirmHandler]
procedure DoNotConfirmShowCreatedPurchOrderForSubcontracting(Question: Text[1024]; var Reply: Boolean)
begin
LibraryVariableStorage.Enqueue(Question);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

$\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

Reply := false;
end;

Expand Down Expand Up @@ -3056,6 +3204,9 @@ codeunit 139989 "Subc. Subcontracting Test"

SubcontractingMgmtLibrary.Initialize();
SubcontractingMgmtLibrary.UpdateSubMgmtSetup_ComponentAtLocation("Components at Location"::Purchase);
UpdateSubMgmtSetupWithReqWkshTemplate();
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

$\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

LibraryVariableStorage.Clear();

LibraryMfgManagement.Initialize();

if IsInitialized then
Expand Down Expand Up @@ -3184,6 +3335,7 @@ codeunit 139989 "Subc. Subcontracting Test"
LibraryWarehouse: Codeunit "Library - Warehouse";
LibraryMfgManagement: Codeunit "Subc. Library Mfg. Management";
SubcontractingMgmtLibrary: Codeunit "Subc. Management Library";
LibraryVariableStorage: Codeunit "Library - Variable Storage";
SubSetupLibrary: Codeunit "Subc. Setup Library";
IsInitialized: Boolean;
Subcontracting: Boolean;
Expand All @@ -3193,5 +3345,6 @@ codeunit 139989 "Subc. Subcontracting Test"
UnitCostCalculation: Option Time,Units;
ConfirmDialogCalledCount: Integer;
AlreadySpecifiedErr: Label 'You cannot open Tracking Specification because this component is already specified in Transfer Order %1.', Comment = '|%1 = Transfer Order No.';
PurchOrderRoutingErr: Label 'Purchase Order %1 should contain a line tied to Routing Reference No. %2', Comment = '%1 = Purchase Order No., %2 = Routing Reference No.';

}
Loading