table 12116 "Withholding Tax": scope Reported/Paid modify guard to st…

[Explorer986]

What & why

The original request asked for an IsHandled event in OnModify to bypass the Reported/Paid immutability check - which triage rejected because it lets any subscriber silently disable a compliance guard that official withholding-tax reporting/payment relies on. Instead, this narrows the guard: a modify on a Reported/Paid entry is still blocked when any standard field changed, but modifications limited to extension fields (added by other apps) are allowed. Partners can maintain their own data without altering officially reported values, and no bypass hook is exposed.

Linked work

Fixes AB#640860

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

• Diff review: OnModify now calls StandardFieldModified(xRec), which compares Rec vs xRec over base fields (field no. < 50000, FieldClass::Normal) via RecordRef. Table has only simple field types (no BLOB/Media), so the value comparison is safe. AL language server reports no errors.
• modifying a standard field on a reported entry still errors, modifying only an extension field succeeds.

Risk & compatibility

Medium-low. Standard reported values stay fully immutable (no regression there); the only relaxation is allowing extension-field-only edits, which were previously over-blocked. Small per-modify RecordRef loop on a low-volume, user-driven table — negligible perf. No schema/data impact. Worth a reviewer/product nod since it touches compliance behavior.

[github-actions]

Copilot PR Review

Iteration 2 · 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
Performance	1	0	1	0	1

Totals: 0 knowledge-backed · 1 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 1

Recommendation: Request Changes

What this PR does

This PR narrows the OnModify guard in table 12116 "Withholding Tax". A Reported or Paid record still errors when a standard field changes, but a modify that only changes extension fields is allowed.

The approach is safer than an IsHandled event because it does not add a general bypass. The RecordRef comparison checks normal base fields below 50000 against xRec, so changes to official withholding tax values are still caught. I did not find a direct data-integrity hole in the code, but the new allowed path is not covered by a test.

Suggestions

S1 - Add tests for scoped modify guard
Add a test for table 12116 that changes only a test extension field on a Reported or Paid record and verifies Modify(true) succeeds. Also keep or add a check that changing a standard field on the same record still fails, because this guard protects reported withholding tax data.

Risk assessment and necessity

Risk: This touches an Italian compliance guard for reported or paid withholding tax records. The code change is narrow and has only a small RecordRef loop on modify, but a wrong comparison would either keep blocking partner data or allow changes to official reported values. There is no public event or schema change, and no BaseApp publisher dependency was introduced.

Necessity: The linked request is valid, but the requested IsHandled bypass would be too broad. This narrower fix is the right direction because partners can store their own data without changing official values. A regression test is needed and feasible here; the IT test app already has table 12116 OnModify tests, and this is sensitive data-integrity logic.

---

[AI-PR-REVIEW] version=1 system=github pr=9422 round=1 by=alexei-dobriansky at=2026-07-14T17:06:09Z lastSha=c1bb712215486b7377dbc9ff9caac71d1a1944b9 suggestions=S1

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

OnModify() now declares a local 'PreviousWithholdingTax: Record "Withholding Tax"' and re-fetches it with Get(Rec."Entry No.") purely to diff it against Rec.

In a table trigger, xRec already holds the exact pre-modification state of the record, populated by the platform with no extra database round-trip. The added Get() (plus the 'if not ... then exit' guard, which can never actually fail here since OnModify only fires for a record that already exists) performs a redundant read on every single Modify() of this table. The underlying impact would normally be rated at least 'major' for a hot ledger-style table, but is capped here per agent-finding severity rules.

Recommendation:

• drop the local variable and the Get(), and call StandardFieldModified(xRec) directly.

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

    trigger OnModify()
    begin
        if (Reported or Paid) and StandardFieldModified(xRec) then
            Error(Text1033);
    end;

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

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

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