[Subcontracting] Performance issues fix

[AleksandricMarko]

What & why

[Subcontracting] Performance issues flagged by PR Agent Reviewer

Linked work

Fixes AB#623643

[alexei-dobriansky]

Analysis

Scenario Validity:
The linked work item (AB#623643, [AI-REPRO]) describes a PR-Agent performance-lint sweep over the Subcontracting app (missing SetLoadFields, CalcSums/Count on potentially large tables without sufficient filtering, etc.), copied from #617394. There is no end-user defect, no concrete repro of a slow page/report, and no perf number being chased. Most of the diff is consistent with that scope (adding SetLoadFields, adding a SetCurrentKey, replacing TransferFields with explicit field copies, light early-exit refactors).

However, the diff also smuggles in a real behavior fix that is not a performance change:

• SubcSynchronizeManagement.Codeunit.al, DeleteEnhancedDocumentsByChangeOfVendorNo: the guard before TransferHeader.Delete(true) changes from if ItemLedgerEntry.IsEmpty() to if ItemLedgerEntry2.IsEmpty(). The two record variables have different filters at that point (ItemLedgerEntry2 is the one actually filtered by "Order No." := ProductionOrder."No." immediately above), so the original check was reading the wrong filter set. This is a correctness change in a destructive code path, not a perf hint.

That mismatch between the stated scenario and the actual scope of the diff is the central problem with this PR.

Correctness:
Going change by change:

1. SubcPlanningCompExt.Codeunit.al — adds PlanningRoutingLine.SetLoadFields("No.") before FindFirst(). Only "No." is read afterwards. Safe.
2. SubcProdOrderCompExt.Codeunit.al:
   • Moves SetLoadFields(SystemId) before IsEmpty(). The original placed it after IsEmpty() and before FindFirst(), which is the actually useful slot; moving it earlier is harmless but also doesn't help IsEmpty() (which doesn't fetch fields anyway). Fine.
   • Replaces TempPurchaseLine.TransferFields(PurchaseLine) with three explicit assignments (Document Type, Document No., Line No.). Downstream usage of TempPurchaseLine in the same procedure only reads Document Type and Document No. and iterates via the primary key, so the three fields copied are sufficient. Safe, and pairs correctly with the new SetLoadFields("Document Type", "Document No.", "Line No.").
   • Adds PurchaseLine.SetCurrentKey("Prod. Order No.", "Prod. Order Line No.", "Routing No.", "Operation No."). This key is published by the BaseApp MfgPurchaseLine.TableExt.al (key(Key8; "Prod. Order No.", "Prod. Order Line No.", "Routing No.", "Operation No.")), so the call resolves. Note however that the actual filters set are only on the first two fields, and the existing pattern elsewhere in this app (e.g., SubcCalculateSubcontracts.Report.al, SubcCreateSubCReturnOrder.Report.al, SubcPurchaseOrderCreator.Codeunit.al:600) consistently uses ("Document Type", Type, "Prod. Order No.", "Prod. Order Line No.", "Routing No.", "Operation No."). The new key is OK functionally, but it diverges from the established convention in this app — worth either aligning to the existing key or justifying why a narrower key is preferable here.
   • Adds ProdOrderRoutingLine.SetLoadFields("Starting Date", "Starting Time", Type, "No."). Subsequent reads are exactly those four fields; no Modify of this record in this path. Safe.
3. SubcItemJnlPostLineExt.Codeunit.al — converts the nested if to early-exits. Logic is equivalent; ProdOrderRoutingLine.Modify() is still unconditionally called after the Get() succeeds, matching prior behavior. Note (pre-existing, not introduced here): ProductionOrder.SetLoadFields("Created from Purch. Order") is followed by Get() but the field is never read — dead code that the PR doesn't remove.
4. SubcSynchronizeManagement.Codeunit.al:
   • SynchronizeExpectedReceiptDate and SynchronizeQuantity are flattened to early-exits. Tracing each path: behavior is equivalent to the original nested form.
   • The ItemLedgerEntry → ItemLedgerEntry2 change is the real fix described above and is correct: at that point ItemLedgerEntry has only the outer filter ("Order No." := ProductionOrder."No." from earlier in the loop) while ItemLedgerEntry2 is the one freshly filtered for the current TransferHeader/ProductionOrder. Calling IsEmpty() on ItemLedgerEntry2 is what was intended.

Side Effects:

• The TransferFields → explicit field copy in SubcProdOrderCompExt is a behavior tightening: any future code added downstream that reads another field from TempPurchaseLine will silently see blank/default instead of the populated value. That is acceptable here (downstream is local to the same procedure) but is the kind of change that benefits from a regression test pinning the contract.
• The ItemLedgerEntry2.IsEmpty() change alters the conditions under which TransferHeader.Delete(true) runs during vendor change on a subcontracting purchase header. Previously the guard could spuriously be true/false based on the outer loop's ItemLedgerEntry state, so the new behavior is closer to intent — but it is still a behavior change in a destructive path with no test added.
• Other changes are pure performance hints with no observable behavior change.

Risk Assessment: Medium.

• Most of the diff is low-risk perf hints (SetLoadFields, SetCurrentKey).
• The ItemLedgerEntry2 correctness fix is in a delete path on Production Order / Transfer Header — financially sensitive (data loss on the wrong branch). Narrow change but high-impact if mis-evaluated.
• The TransferFields removal is locally safe but reduces robustness to future edits.

"What if we don't make this change?" — The performance hints would remain as PR-Agent lint findings with no measurable user impact; nothing actually breaks. The hidden bug fix (ItemLedgerEntry2.IsEmpty()) would remain — TransferHeader.Delete(true) would continue to be guarded by the wrong record's IsEmpty(), occasionally deleting (or failing to delete) a transfer header it shouldn't. That latter item is the only thing here that materially matters.

Test Coverage:
The PR contains zero new or modified test files. Most of the diff is refactor/perf and could plausibly land without tests, but the ItemLedgerEntry → ItemLedgerEntry2 change is a non-trivial behavior change in a destructive path and falls squarely under the testing hard gate. There is no regression test pinning when TransferHeader.Delete(true) is or isn't invoked from DeleteEnhancedDocumentsByChangeOfVendorNo, and no test asserting that the explicit-field copy in SubcProdOrderCompExt preserves the downstream loop behavior.

Recommendation: Request Changes

1. Add a regression test for the ItemLedgerEntry2.IsEmpty() fix. Construct a scenario in DeleteEnhancedDocumentsByChangeOfVendorNo where the outer ItemLedgerEntry filter (Order No. from an earlier iteration / unrelated production order) is empty but a relevant Item Ledger Entry for the current ProductionOrder."No." does exist (or vice versa). Assert that TransferHeader.Delete(true) runs only when there are truly no item ledger entries for the current production order. The existing Subcontracting test app under src/Apps/W1/Subcontracting/Test/ already has helpers for setting up purchase orders, transfer orders and production orders for vendor-change flows; reuse those rather than build new scaffolding.
2. Separate the behavior fix from the perf cleanup. The ItemLedgerEntry → ItemLedgerEntry2 change is the only line in this PR that alters runtime behavior in a meaningful way and it deserves its own ADO bug and PR (with the test above). Mixing it into a "PR-Agent perf flags" PR makes it easy to miss in review — as demonstrated by the fact that the PR title and ADO repro never mention it.
3. Align the new SetCurrentKey with existing convention. Every other call site in this app uses ("Document Type", Type, "Prod. Order No.", "Prod. Order Line No.", "Routing No.", "Operation No."). Either use the same key in SubcProdOrderCompExt for consistency, or add a one-line comment explaining why a narrower key is intentionally chosen here.
4. Optional clean-up while you're here: the ProductionOrder.SetLoadFields("Created from Purch. Order") in UpdateProdOrderRoutingLine is followed by a Get() whose result field is never read. Drop the SetLoadFields (or the field, if it was meant to be checked). Pre-existing, but right in the function you're touching.

[alexei-dobriansky]

@AleksandricMarko, please review suggestions 2, 3, 4.