Extract NL customization to object extensions (TrMode)

[Alexander-Ya]

What & why

Linked work

Fixes AB#640601

How I validated this

• I read the full diff and it contains only changes I intended.
• I built the affected app(s) locally with no new analyzer warnings.
• I ran the change in Business Central and confirmed it behaves as expected.
• I added or updated tests for the new behavior, or explained below why none are needed.

What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)

Risk & compatibility

[Alexander-Ya]

Fixes AB#640601

[github-actions]

Copilot PR Review

Iteration 16 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@186d8a131465475c79244d994acb872cd5c0d4bf

Findings by domain

Findings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).

Domain	Findings	Knowledge-backed	Agent	Inline	Fallback
Style	3	2	1	3	0
Testing	1	0	1	0	0

Totals: 2 knowledge-backed · 2 agent findings.

Orchestrator pre-filter (2 file(s) excluded)

• layer-disabled (knowledge) : 2 file(s)

_{Findings produced by the AL review agent v1.7.3. Reply 👎 on any inline comment to flag false positives.}

[alexei-dobriansky]

Agentic PR Review - Round 5

Recommendation: Request Changes

What this PR does

This PR extracts NL BaseApp Transaction Mode customization into extension objects for AB#640601. Since round 4, the author-owned AL changes are test-only: one commit fixes setup in an NL purchase document amount test, and one commit disables a W1 service-order warehouse shipment test.

The NL test setup change looks like a real test fix: it enables Check Doc. Total Amounts and fills the document amount fields before posting. It does not weaken Transaction Mode coverage, but it also does not address the open Transaction Mode scoping issue. The disabled W1 test is not justified and removes active coverage.

Status of previous suggestions

ID	Title	Status	Author response / evidence
S1	Keep the G/L Account field id stable	Addressed	Still addressed. G/L Account.Omit Default Descr. in Jnl. remains field 11400 in src/Layers/NL/BaseApp/Finance/GeneralLedger/Account/GLAccount.Table.al.
S2	Update the matching role center tooltip	Addressed	Still addressed for the copies where the caption was expanded to General Business Posting Groups.
S3	Add coverage for Transaction Mode wiring	Not addressed	The only NL test change is in ERMPurchaseDocAmounts.Codeunit.al, which covers purchase document amount setup/correction, not Transaction Mode copying or NL-company scoping.
S4	Keep abbreviated tooltip text where the caption is abbreviated	Not addressed	APAC, AU, IT, NA, NL, NO, and RU still show caption Gen. Business Posting Groups with tooltip Open the General Business Posting Groups page.
S5	Scope all Transaction Mode effects	Not addressed	No author reply was found after round 4, and the new commits do not touch the unguarded Transaction Mode subscribers in SalesHeaderNL.Codeunit.al, PurchaseHeaderNL.Codeunit.al, CustomerNL.Codeunit.al, or GenJournalLineNL.Codeunit.al.

New observations (commits since round 4)

S6 - Do not disable unrelated W1 tests
Skip one test comments out [Test] on PullServiceLineNoShipmentUsingUseFilter in src/Layers/W1/Tests/SCM-Service/ServiceOrderRelease.Codeunit.al. This removes a warehouse shipment regression test instead of fixing it. Re-enable it, or replace it with an active equivalent test.

Risk assessment and necessity

Risk: The main product risk is still that non-NL companies can receive NL Transaction Mode behavior through unguarded subscribers that copy or enforce Transaction Mode outside the guarded field OnValidate triggers. The new skipped W1 service-order test adds a separate test-quality risk because it reduces coverage for warehouse shipment filtering in a shared W1 area.

Necessity: The extraction remains necessary for AB#640601 because it removes NL full-object customization and lowers future merge cost. The round-5 test changes are not enough to make the PR safe: the Transaction Mode scope must cover all paths, and unrelated tests should not be disabled to get the branch green.

---

[AI-PR-REVIEW] version=1 system=github pr=8933 round=5 by=alexei-dobriansky at=2026-07-15T17:03:19Z lastSha=7d59f77109426874a158f3c53db67f16c2f3a498 suggestions=S1:addressed,S2:addressed,S3:notaddressed,S4:notaddressed,S5:notaddressed,S6:new parentRound=4

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Testing} \quad \color{gray}{\texttt{\small Iteration\ 15}}$

The [Test] attribute on procedure PullServiceLineNoShipmentUsingUseFilter was commented out (changed to '// [Test]'), which silently removes this test case from the test run instead of deleting it or marking it Obsolete with a documented reason.

The test body, assertions, and helper calls remain fully intact, indicating this was very likely a leftover debugging change rather than an intentional removal. If genuinely intentional, impact would be major (a real regression test for warehouse-shipment source filtering with no matching orders would stop running in CI without any record of why); capped to minor per agent-finding severity rules. Recommend restoring the [Test] attribute, or if the test is no longer valid, deleting it or using [Test, Disabled] equivalent tracked in an issue with a comment explaining the reason.

Suggested fix (apply manually — could not be anchored as a one-click suggestion):

    [Test]

Line mapping was unavailable, so this was posted as an issue comment.

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

[github-actions]

$\textbf{🔴\ Critical\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 16}}$

The new EventSubscriber OnAfterNoOnAfterValidate on Page::"Purch.

Invoice Subform" declares its second parameter as xPurchaseLine: Record "Purchase Line" (no var), but the publisher (PurchInvoiceSubform.Page.al line 1593) declares it as var xPurchaseLine: Record "Purchase Line". AL requires a subscriber to reproduce the publisher's parameter list, including var/byref modifiers, verbatim. This var mismatch will fail to compile/bind. Compare with the sibling PurchCrMemoSubformNL.Codeunit.al, which correctly copies both var modifiers from its own publisher.

Suggested change

	local procedure OnAfterNoOnAfterValidate(var PurchaseLine: Record "Purchase Line"; xPurchaseLine: Record "Purchase Line")
	local procedure OnAfterNoOnAfterValidate(var PurchaseLine: Record "Purchase Line"; var xPurchaseLine: Record "Purchase Line")

Knowledge:

• microsoft/knowledge/style/event-subscriber-param-names-match-publisher.md

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

[github-actions]

$\textbf{🔴\ Critical\ Severity\ —\ Style} \quad \color{gray}{\texttt{\small Iteration\ 16}}$

The new EventSubscriber OnBeforePostCustomerEntry on Codeunit::"Sales-Post Prepayments" is declared with only two parameters, `(var GenJnlLine: Record "Gen.

Journal Line"; SalesHeader: Record "Sales Header"), but the publisher (SalesPostPrepayments.Codeunit.al line 2431) declares six parameters: (var GenJnlLine: Record "Gen. Journal Line"; TotalPrepmtInvLineBuffer: Record "Prepayment Inv. Line Buffer"; TotalPrepmtInvLineBufferLCY: Record "Prepayment Inv. Line Buffer"; CommitIsSuppressed: Boolean; SalesHeader: Record "Sales Header"; DocumentType: Option Invoice,"Credit Memo")`. A subscriber cannot cherry-pick a subset of the publisher's parameters; the full parameter list, in order, must be reproduced. As written this subscriber will not bind to the publisher and the extracted NL Transaction Mode Code/Bank Account Code logic that used to run inline in the old NL override of Sales-Post Prepayments will silently stop firing.

Knowledge:

• microsoft/knowledge/style/event-subscriber-param-names-match-publisher.md

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

[github-actions]

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

SalesHeader.Table.al's BatchConfirmUpdatePostingDate procedure was changed to if ReplacePostingDate and ReplaceDocDate and (...), but the procedure already exits immediately at its top with if not ReplacePostingDate then exit;.

By the time execution reaches this line, ReplacePostingDate is always true, so the added conjunct is dead/redundant code. The same change is duplicated identically across every per-country-layer copy of SalesHeader.Table.al touched by this PR (W1, NO, and others), suggesting an unrelated, mechanically-applied edit rather than an intentional fix. Recommend removing the redundant ReplacePostingDate and or clarifying the actual intent if this guard is meant to protect against a future refactor of the early exit.

Suggested change

	if ReplacePostingDate and ReplaceDocDate and ("Document Date" <> PostingDateReq) then begin
	if ReplaceDocDate and ("Document Date" <> PostingDateReq) then begin

_{Agent judgement — not directly backed by a BCQuality knowledge article.}

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