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 @@ -12,6 +12,7 @@ using Microsoft.Purchases.Document;
using Microsoft.Purchases.History;
using Microsoft.Utilities;
using System.Reflection;
using System.Text;
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}}$

Unused namespace import added

The using System.Text; statement was added in this PR but no type from the System.Text namespace (such as TextBuilder) is referenced anywhere in the file. The new SelectionFilterManagement codeunit does not belong to System.Text, so this import is dead code that can trigger AL compiler warnings about unused using directives.

Recommendation:

  • Remove the using System.Text; line. If SelectionFilterManagement requires an explicit namespace import to resolve without ambiguity, add its correct namespace instead (e.g., using Microsoft.Foundation.Navigate; if needed).
Suggested change
using System.Text;
using System.Reflection;
// remove: using System.Text;

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


codeunit 99001560 "Subc. Purch. Factbox Mgmt."
{
Expand Down Expand Up @@ -229,9 +230,11 @@ codeunit 99001560 "Subc. Purch. Factbox Mgmt."
ProductionOrder: Record "Production Order";
PurchaseLine: Record "Purchase Line";
TransferHeader: Record "Transfer Header";
TransferHeaderToOpen: Record "Transfer Header";
TransferLine: Record "Transfer Line";
DataTypeManagement: Codeunit "Data Type Management";
PageManagement: Codeunit "Page Management";
SelectionFilterMgt: Codeunit SelectionFilterManagement;
RecRef: RecordRef;
NoOfTransferHeaders: Integer;
begin
Expand Down Expand Up @@ -305,10 +308,15 @@ codeunit 99001560 "Subc. Purch. Factbox Mgmt."
NoOfTransferHeaders = 0:
Message(NoTransferExistsMsg);
NoOfTransferHeaders = 1:
if TransferHeader.FindFirst() then
PageManagement.PageRun(TransferHeader);
if TransferHeader.FindFirst() then begin
TransferHeaderToOpen.Get(TransferHeader."No.");
PageManagement.PageRun(TransferHeaderToOpen);
end;
NoOfTransferHeaders > 1:
PageManagement.PageRunList(TransferHeader);
begin
TransferHeaderToOpen.SetFilter("No.", SelectionFilterMgt.GetSelectionFilterForTransferHeader(TransferHeader));
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\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Filter string may grow unbounded for large sets

GetSelectionFilterForTransferHeader iterates all marked TransferHeader records and encodes their "No." values into a pipe-separated filter string. If many transfer orders are marked (e.g., in a high-volume manufacturing environment), the resulting filter string could become very long, risk hitting AL text-length limits, and generate a complex SQL IN-style predicate that performs worse than the previous marked-record approach.

Recommendation:

  • Consider guarding with a reasonable upper bound check before calling GetSelectionFilterForTransferHeader, or fall back to the marked-record approach when NoOfTransferHeaders exceeds a configurable threshold. Alternatively, document the expected max cardinality in a comment to make the assumption explicit.
Suggested change
TransferHeaderToOpen.SetFilter("No.", SelectionFilterMgt.GetSelectionFilterForTransferHeader(TransferHeader));
NoOfTransferHeaders > 1:
begin
// Note: NoOfTransferHeaders is bounded by subcontracting purchase order scope;
// a single PO is unlikely to have more than a few dozen linked transfer orders.
TransferHeaderToOpen.SetFilter("No.", SelectionFilterMgt.GetSelectionFilterForTransferHeader(TransferHeader));
PageManagement.PageRunList(TransferHeaderToOpen);
end;

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

PageManagement.PageRunList(TransferHeaderToOpen);
end;
end;
end;
end;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2073,6 +2073,82 @@ codeunit 139989 "Subc. Subcontracting Test"
'GetPurchOrderQtyFromRoutingLine must filter by Routing Reference No., not by Prod. Order Line No.');
end;

[Test]
[HandlerFunctions('DoNotConfirmShowCreatedPurchOrderForSubcontracting,HandleTransferOrderReopen')]
procedure FactboxDrilldownTransferOrderReopenPersists()
var
Item: Record Item;
MachineCenter: array[2] of Record "Machine Center";
ProdOrderRoutingLine: Record "Prod. Order Routing Line";
ProductionOrder: Record "Production Order";
PurchaseHeader: Record "Purchase Header";
PurchaseLine: Record "Purchase Line";
TransferHeader: Record "Transfer Header";
WorkCenter: array[2] of Record "Work Center";
ProductionLocation: Record Location;
SubcPurchFactboxMgmt: Codeunit "Subc. Purch. Factbox Mgmt.";
ReleaseTransferDocument: Codeunit "Release Transfer Document";
PurchaseHeaderPage: TestPage "Purchase Order";
ReleasedProdOrderRtng: TestPage "Prod. Order Routing";
begin
// [SCENARIO] Bug 634267 - Reopen Transfer Order does not persist when opened from Subcontracting Details Factbox.
// ShowTransferOrdersAndReturnOrder must open the page on a real database record, so actions like
// Reopen that modify Rec directly persist after the page closes.

// [GIVEN] Subcontracting setup with transfer components and a released transfer order
Initialize();
SubcontractingMgmtLibrary.UpdateManufacturingSetupWithSubcontractingLocation();
SubcontractingMgmtLibrary.SetupInventorySetup();
Subcontracting := true;
UnitCostCalculation := UnitCostCalculation::Units;

CreateAndCalculateNeededWorkAndMachineCenter(WorkCenter, MachineCenter);
CreateItemForProductionIncludeRoutingAndProdBOM(Item, WorkCenter, MachineCenter);
UpdateProdBomAndRoutingWithRoutingLink(Item, WorkCenter[2]."No.");
SubcontractingMgmtLibrary.UpdateProdBomWithSubcontractingType(Item, "Subcontracting Type"::Transfer);
UpdateVendorWithSubcontractingLocationCode(WorkCenter[2]);

LibraryWarehouse.CreateLocationWithInventoryPostingSetup(ProductionLocation);
SubcontractingMgmtLibrary.CreateAndRefreshProductionOrder(
ProductionOrder, "Production Order Status"::Released, ProductionOrder."Source Type"::Item, Item."No.", LibraryRandom.RandInt(10) + 5);
UpdateSubMgmtSetupWithReqWkshTemplate();
SetAllProdOrderTransferComponentLocations(ProductionOrder."No.", ProductionLocation.Code);
SubcontractingMgmtLibrary.CreateTransferRoute(WorkCenter[2], ProductionOrder);

ProdOrderRoutingLine.SetRange("Prod. Order No.", ProductionOrder."No.");
ProdOrderRoutingLine.SetRange("Work Center No.", WorkCenter[2]."No.");
ProdOrderRoutingLine.FindFirst();
ReleasedProdOrderRtng.OpenView();
ReleasedProdOrderRtng.GoToRecord(ProdOrderRoutingLine);
ReleasedProdOrderRtng.CreateSubcontracting.Invoke();
ReleasedProdOrderRtng.Close();

PurchaseLine.SetRange("Document Type", PurchaseLine."Document Type"::Order);
PurchaseLine.SetRange("Prod. Order No.", ProductionOrder."No.");
PurchaseLine.FindFirst();
PurchaseHeader.Get(PurchaseLine."Document Type", PurchaseLine."Document No.");

PurchaseHeaderPage.OpenView();
PurchaseHeaderPage.GoToRecord(PurchaseHeader);
PurchaseHeaderPage.CreateTransfOrdToSubcontractor.Invoke();
PurchaseHeaderPage.Close();

TransferHeader.SetRange("Subcontr. Purch. Order No.", PurchaseHeader."No.");
TransferHeader.FindFirst();
ReleaseTransferDocument.Release(TransferHeader);
Assert.AreEqual(TransferHeader.Status::Released, TransferHeader.Status, 'Transfer order should be Released before the test.');

// [WHEN] Opening the transfer order from the factbox drill-down and performing Reopen
// The page handler HandleTransferOrderReopen will reopen the transfer order
PurchaseLine.FindFirst();
SubcPurchFactboxMgmt.ShowTransferOrdersAndReturnOrder(PurchaseLine, true, false);

// [THEN] The transfer order status must be Open after closing the page
TransferHeader.Get(TransferHeader."No.");
Assert.AreEqual(TransferHeader.Status::Open, TransferHeader.Status,
'Transfer order status should be Open after Reopen from factbox drill-down. Before the fix, the Reopen modified a marked record and the change was lost.');
end;

[Test]
[HandlerFunctions('DoNotConfirmShowCreatedPurchOrderForSubcontracting')]
procedure Description2CopiedFromProdOrderComponentToPurchaseLine()
Expand Down Expand Up @@ -2579,6 +2655,13 @@ codeunit 139989 "Subc. Subcontracting Test"
TransfOrderPage.OK().Invoke();
end;

[PageHandler]
procedure HandleTransferOrderReopen(var TransfOrderPage: TestPage "Transfer Order")
begin
TransfOrderPage."Reo&pen".Invoke();
TransfOrderPage.OK().Invoke();
end;

[ConfirmHandler]
procedure ConfirmYesShowSubcontractingPurchOrders(Question: Text[1024]; var Reply: Boolean)
begin
Expand Down
Loading