Skip to content

Extract AU WHT GenJnlPostBatch#9319

Open
Alexander-Ya wants to merge 2 commits into
mainfrom
bugs/Extract-AU-WHT-GenJnlPostBatch
Open

Extract AU WHT GenJnlPostBatch#9319
Alexander-Ya wants to merge 2 commits into
mainfrom
bugs/Extract-AU-WHT-GenJnlPostBatch

Conversation

@Alexander-Ya

@Alexander-Ya Alexander-Ya commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What & why

Linked work

Fixes AB#636621

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 Alexander-Ya requested a review from a team July 10, 2026 14:20
@github-actions github-actions Bot modified the milestone: Version 29.0 Jul 10, 2026
CustLedgEntry: Record "Cust. Ledger Entry";
VendLedgEntry: Record "Vendor Ledger Entry";
begin
GLSetup.Get();

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\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

The new event subscriber OnPostGenJournalLineOnAfterPrepareGenJnlLineAddCurr calls GLSetup.Get() unconditionally before checking the cheap, already-in-memory GenJnlLine5."Skip WHT" flag.

Per the referenced guidance, cheap in-memory guards should run before any database call. This subscriber fires once per posted journal line, so reordering the check ahead of GLSetup.Get() avoids a setup-table read on every line where Skip WHT is already true.

Knowledge:

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

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 2 · Outcome: completed

Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630

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
Agent 1 0 1 1 0

Totals: 0 knowledge-backed · 1 agent findings.

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

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

Findings produced by the Copilot CLI agent against BCQuality at 822cae1b2771ac25f665f73369f69093bd4fd630. Reply 👎 on any inline comment to flag false positives.

end;

[IntegrationEvent(false, false)]
local procedure OnPostGenJournalLineOnAfterPrepareGenJnlLineAddCurr(GenJnlLine5: Record "Gen. Journal Line"; CurrGenJnlLine: Record "Gen. Journal Line"; GenJournalLine: Record "Gen. Journal Line")

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\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 2}}$

The new IntegrationEvent 'OnPostGenJournalLineOnAfterPrepareGenJnlLineAddCurr' (Gen.

Jnl.-Post Batch codeunit 13) declares its 'GenJnlLine5' parameter by value, not 'var'. The new subscriber in codeunit 'WHT Gen. Jnl.-Post Batch' relies entirely on this event to set 'GenJnlLine5."Skip WHT"' (via CheckWHTCalculationRule/VendorMinWHT/CustomerMinWHT, all of which take 'var GenJnlLine5'). Because AL passes Record parameters by value unless declared 'var', the subscriber mutates only its own local copy; the change is discarded when the event returns. Immediately after the event call, PostGenJournalLine continues to use its own 'GenJnlLine5' unchanged and passes it to GenJnlPostLine.RunWithoutCheck/RunWithCheck, whose logic (see GenJnlPostLine.Codeunit.al, many 'if not GenJnlLine."Skip WHT"' checks) depends on that flag. This silently disables the withholding-tax minimum-invoice-amount 'Skip WHT' computation for the paths that used to run inline before this refactor moved the logic behind the event. Impact would be major/blocker in practice (WHT is calculated/skipped incorrectly for affected postings); this should be promoted to a fix (declare the event parameter 'var GenJnlLine5') before merge, and ideally backed by a BCQuality knowledge rule on IntegrationEvent parameters needing 'var' when the subscriber's contract is to return a computed value to the publisher.

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

    [IntegrationEvent(false, false)]
    local procedure OnPostGenJournalLineOnAfterPrepareGenJnlLineAddCurr(var GenJnlLine5: Record "Gen. Journal Line"; CurrGenJnlLine: Record "Gen. Journal Line"; GenJournalLine: Record "Gen. Journal Line")
    begin
    end;

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant