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 @@ -15,12 +15,13 @@ using System.Reflection;
codeunit 99001559 "Subc. ProdO. Factbox Mgmt."
{
/// <summary>
/// Opens the Released Production Order page for the production order linked to the given variant record.
/// Opens the appropriate Production Order page (Released or Finished) for the production order linked to the given variant record.
/// </summary>
/// <param name="RecRelatedVariant">A record variant of a purchase or transfer document line.</param>
procedure ShowProductionOrder(RecRelatedVariant: Variant)
var
ProductionOrder: Record "Production Order";
FinishedProductionOrder: Page "Finished Production Order";
ReleasedProductionOrder: Page "Released Production Order";
OperationNo: Code[10];
ProdOrderNo: Code[20];
Expand All @@ -29,11 +30,26 @@ codeunit 99001559 "Subc. ProdO. Factbox Mgmt."
begin
if not SetProdOrderInformationByVariant(RecRelatedVariant, ProdOrderNo, ProdOrderLineNo, RoutingNo, OperationNo) then
exit;
ProductionOrder.SetRange(Status, ProductionOrder.Status::Released);
ProductionOrder.SetFilter(Status, '>=%1', ProductionOrder.Status::Released);
ProductionOrder.SetRange("No.", ProdOrderNo);
ReleasedProductionOrder.SetTableView(ProductionOrder);
ReleasedProductionOrder.Editable := false;
ReleasedProductionOrder.Run();
if not ProductionOrder.FindFirst() then
exit;
case ProductionOrder.Status of
Copy link
Copy Markdown
Contributor

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

Case statement silently ignores unexpected status

The case ProductionOrder.Status of block handles only Released and Finished. Although the preceding SetFilter('>=%1', Released) makes other statuses theoretically unreachable today, any future enum addition or unexpected data would cause ShowProductionOrder to silently exit without opening any page, giving the user no feedback.

Recommendation:

  • Add an else branch that either opens a generic fallback page or raises an informative error.
Suggested change
case ProductionOrder.Status of
else
Error('Unexpected production order status: %1', ProductionOrder.Status);

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

ProductionOrder.Status::Released:
begin
ProductionOrder.SetRange(Status, ProductionOrder.Status::Released);
ReleasedProductionOrder.SetTableView(ProductionOrder);
ReleasedProductionOrder.Editable := false;
ReleasedProductionOrder.Run();
end;
ProductionOrder.Status::Finished:
begin
ProductionOrder.SetRange(Status, ProductionOrder.Status::Finished);
FinishedProductionOrder.SetTableView(ProductionOrder);
FinishedProductionOrder.Editable := false;
FinishedProductionOrder.Run();
end;
end;
end;

/// <summary>
Expand Down Expand Up @@ -77,9 +93,9 @@ codeunit 99001559 "Subc. ProdO. Factbox Mgmt."
exit(ProdOrderRoutingLine.Count());
end;

local procedure SetFilterProductionOrderRouting(var ProdOrderRoutingLine: Record "Prod. Order Routing Line"; ProdOrderNo: Code[20]; ProdOrderLineNo: Integer; RoutingNo: Code[20]; OperationNo: Code[10])
local procedure SetFilterProductionOrderRouting(var ProdOrderRoutingLine: Record "Prod. Order Routing Line"; ProdOrderNo: Code[20]; ProdOrderLineNo: Integer; RoutingNo: Code[20]; OperationNo: Code[10])
begin
ProdOrderRoutingLine.SetRange(Status, ProdOrderRoutingLine.Status::Released);
ProdOrderRoutingLine.SetFilter(Status, '>=%1', ProdOrderRoutingLine.Status::Released);
ProdOrderRoutingLine.SetRange("Prod. Order No.", ProdOrderNo);
ProdOrderRoutingLine.SetRange("Routing Reference No.", ProdOrderLineNo);
ProdOrderRoutingLine.SetRange("Routing No.", RoutingNo);
Expand Down Expand Up @@ -130,15 +146,15 @@ local procedure SetFilterProductionOrderRouting(var ProdOrderRoutingLine: Record
ProdOrderRoutingLine: Record "Prod. Order Routing Line";
begin
ProdOrderRoutingLine.SetLoadFields("Routing Link Code");
ProdOrderRoutingLine.SetRange(Status, ProdOrderRoutingLine.Status::Released);
ProdOrderRoutingLine.SetFilter(Status, '>=%1', ProdOrderRoutingLine.Status::Released);
ProdOrderRoutingLine.SetRange("Prod. Order No.", ProdOrderNo);
ProdOrderRoutingLine.SetRange("Routing Reference No.", ProdOrderLineNo);
ProdOrderRoutingLine.SetRange("Routing No.", RoutingNo);
ProdOrderRoutingLine.SetRange("Operation No.", OperationNo);
if ProdOrderRoutingLine.FindFirst() then
ProdOrderComponent.SetRange("Routing Link Code", ProdOrderRoutingLine."Routing Link Code");

ProdOrderComponent.SetRange(Status, ProdOrderComponent.Status::Released);
ProdOrderComponent.SetRange(Status, ProdOrderRoutingLine.Status);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

Stale routing-line status used after failed FindFirst

When ProdOrderRoutingLine.FindFirst() returns false the record variable retains its default Status value (Simulated = 0). The immediately following ProdOrderComponent.SetRange(Status, ProdOrderRoutingLine.Status) then filters components by Simulated status, finding nothing and silently returning 0 components instead of the expected count.

Recommendation:

  • Only set the component status filter when a routing line was actually found, or fall back to the production order's status otherwise. Alternatively, store the production order status and use it unconditionally for the component filter.
Suggested change
ProdOrderComponent.SetRange(Status, ProdOrderRoutingLine.Status);
if ProdOrderRoutingLine.FindFirst() then begin
ProdOrderComponent.SetRange("Routing Link Code", ProdOrderRoutingLine."Routing Link Code");
ProdOrderComponent.SetRange(Status, ProdOrderRoutingLine.Status);
end else
ProdOrderComponent.SetRange(Status, ProdOrderComponent.Status::Released);

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

ProdOrderComponent.SetRange("Prod. Order No.", ProdOrderNo);
ProdOrderComponent.SetRange("Prod. Order Line No.", ProdOrderLineNo);
end;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1800,6 +1800,57 @@ codeunit 139989 "Subc. Subcontracting Test"
'CalcNoOfProductionOrderComponents should return a positive count for an Item Ledger Entry from a subcontracting receipt.');
end;

[Test]
[HandlerFunctions('ConfirmArchiveOrderHandler,HandlePurchaseOrderPage')]
procedure ProdOFactboxMgmtShowsDataAfterProdOrderFinished()
var
Item: Record Item;
ProductionOrder: Record "Production Order";
PurchaseHeader: Record "Purchase Header";
PurchaseLine: Record "Purchase Line";
SubcWorkCenter: Record "Work Center";
SubcProdOFactboxMgmt: Codeunit "Subc. ProdO. Factbox Mgmt.";
begin
// [SCENARIO 634953] Subcontracting factbox drilldowns should work after production order is finished.
Initialize();

// [GIVEN] A released production order with a subcontracting routing operation and a subcontracting purchase order
Subcontracting := true;
UnitCostCalculation := UnitCostCalculation::Units;

CreateItemWithSingleSubcontractingOperation(Item, SubcWorkCenter);
SubcontractingMgmtLibrary.UpdateVendorWithSubcontractingLocationCode(SubcWorkCenter);
SubcontractingMgmtLibrary.CreateAndRefreshProductionOrder(
ProductionOrder, "Production Order Status"::Released, ProductionOrder."Source Type"::Item, Item."No.", LibraryRandom.RandInt(10) + 5);
UpdateSubMgmtSetupWithReqWkshTemplate();

SubcontractingMgmtLibrary.CreateSubcontractingOrderFromProdOrderRtngPage(Item."Routing No.", SubcWorkCenter."No.");
PurchaseLine.SetRange("Document Type", PurchaseLine."Document Type"::Order);
PurchaseLine.SetRange("Prod. Order No.", ProductionOrder."No.");
#pragma warning disable AA0210
PurchaseLine.SetRange("Work Center No.", SubcWorkCenter."No.");
#pragma warning restore AA0210
PurchaseLine.FindFirst();
PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No.");
EnsureGeneralPostingSetupIsValid(PurchaseLine."Gen. Bus. Posting Group", PurchaseLine."Gen. Prod. Posting Group");
LibraryPurchase.PostPurchaseDocument(PurchaseHeader, true, false);

// [GIVEN] The production order is changed to Finished status
LibraryManufacturing.ChangeProdOrderStatus(ProductionOrder, "Production Order Status"::Finished, WorkDate(), true);

// Re-read purchase line (the order still exists because only receipt was posted)
PurchaseLine.FindFirst();

// [WHEN] CalcNoOfProductionOrderRoutings / CalcNoOfProductionOrderComponents are called with the Purchase Line
// [THEN] Both return a positive count even though the production order is now Finished
Assert.IsTrue(
SubcProdOFactboxMgmt.CalcNoOfProductionOrderRoutings(PurchaseLine) > 0,
'CalcNoOfProductionOrderRoutings should return a positive count after the production order is finished.');
Assert.IsTrue(
SubcProdOFactboxMgmt.CalcNoOfProductionOrderComponents(PurchaseLine) > 0,
'CalcNoOfProductionOrderComponents should return a positive count after the production order is finished.');
end;

[Test]
[HandlerFunctions('ConfirmHandler')]
procedure RoutingFactboxMgmtFiltersPurchOrderQtyByRoutingReferenceNo()
Expand Down
Loading