Skip to content

Concurrent Posting for General Ledger VAT Entry#9442

Open
Alexander-Ya wants to merge 4 commits into
mainfrom
bugs/Concurrent-Posting-for-General-Ledger-VAT-Entry
Open

Concurrent Posting for General Ledger VAT Entry#9442
Alexander-Ya wants to merge 4 commits into
mainfrom
bugs/Concurrent-Posting-for-General-Ledger-VAT-Entry

Conversation

@Alexander-Ya

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

Copy link
Copy Markdown
Contributor

What & why

Linked work

Fixes AB#624164

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 14, 2026 14:31
@github-actions github-actions Bot added this to the Version 29.0 milestone Jul 14, 2026
@github-actions

github-actions Bot commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Copilot PR Review

Iteration 4 · 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
Breaking Changes 1 1 0 1 0
Events 1 1 0 1 0

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

@Alexander-Ya Alexander-Ya added the Finance GitHub request for Finance area label Jul 15, 2026
@Alexander-Ya
Alexander-Ya requested a review from a team July 15, 2026 12:48
@github-actions github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label Jul 15, 2026
@alexei-dobriansky

Copy link
Copy Markdown
Contributor

Agentic PR Review - Round 1

Recommendation: Request Changes

What this PR does

This PR adds VAT Entry.GetNextEntryNo() backed by Sequence No. Mgt. and uses it in Gen. Jnl.-Post Line when General Ledger Setup.UseConcurrentPosting() is enabled. The old VATEntry.LockTable() plus FindLast() path is still used when the setup flag is off.

The direction matches the concurrent-posting pattern, but the implementation is not safe yet. The PR changes how NextVATEntryNo is advanced without updating callers that treat GetNextVATEntryNo() - 1 as the last inserted VAT entry. It also enables the setup flag in demo data and does not add tests for this sensitive posting path.

Suggestions

S1 - Fix last VAT entry number callers
Do not rely on GetNextVATEntryNo() - 1 after VAT entry numbers come from a sequence. Other sessions or skipped sequence numbers can make this point to the wrong VAT entry. Update these callers, for example VAT settlement and CZ advance payment posting, to use the actual inserted VAT entry number.

S2 - Allocate VAT numbers only when needed
The concurrent path reserves a VAT entry number in StartPosting() and again after each insert. This can consume unused numbers for postings without VAT and one extra number after the last VAT entry. Allocate the number at the insert point, or keep a separate last-inserted VAT entry number.

S3 - Remove the test setup change
Create General Ledger Setup now sets Use Concurrent Posting to true with a comment saying it is for testing all countries. This changes demo setup data and makes the feature on by default there. Remove this from product/demo data and use proper tests instead.

S4 - Add focused posting tests
This is financial posting and changes VAT entry numbering, links, and setup-driven behavior. Add tests for the legacy path, the concurrent path, VAT entry links, settlement/advance-payment callers, and a no-VAT posting. The existing VAT and general journal test apps make this feasible.

S5 - Clarify or complete the concurrency scope
The work item says general ledger concurrent posting, but InitNextEntryNo() still locks G/L Entry, and unrealized VAT still calls VATEntry.LockTable(). If those are out of scope, document the dependency and remaining serialization. Otherwise, this PR does not fully deliver the stated concurrency feature.

Risk assessment and necessity

Risk: High. This touches VAT entry numbers in Codeunit 12 across many country layers. Wrong entry numbers can break VAT entry links, VAT settlement, reversal, and localization code such as CZ advance payments. The setup flag preserves legacy behavior for normal customers, but the public GetNextVATEntryNo() / IncrNextVATEntryNo() behavior changes when the flag is enabled.

Necessity: The Slice is valid and important because VAT entry allocation can become a posting bottleneck. The scope is not ready to merge yet because correctness gaps, default setup change, and missing tests leave too much risk for financial posting.


[AI-PR-REVIEW] version=1 system=github pr=9442 round=1 by=alexei-dobriansky at=2026-07-15T17:03:19Z lastSha=0810eda2251c32566da999d4ee404e8818ba6d0a suggestions=S1,S2,S3,S4,S5

Removed the line enabling concurrent posting for testing.
@github-actions github-actions Bot removed the AL: Apps (W1) Add-on apps for W1 label Jul 17, 2026
/// Retrieves the next entry number from the VAT Entry table.
/// </summary>
/// <returns>Next VAT entry number to be used in the system</returns>
procedure GetNextEntryNo(): Integer

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\ —\ Breaking\ Changes} \quad \color{gray}{\texttt{\small Iteration\ 4}}$

VATEntry.GetNextEntryNo() is added as a public (non-local) table procedure across all changed VAT Entry table copies, even though the diff only shows it consumed as an internal numbering helper from the posting codeunit (VATEntry.GetNextEntryNo() inside GetNextVATEntryNoForPosting / StartPosting).

Exposing it publicly makes it a supported external API surface that other extensions can start depending on, even though it is only intended as an internal implementation detail today. Consider marking it internal unless cross-extension callers are intentionally meant to depend on it.

Knowledge:

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

if IsHandled then
exit;
NextVATEntryNo := NextVATEntryNo + 1;

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{🟠\ High\ Severity\ —\ Events} \quad \color{gray}{\texttt{\small Iteration\ 4}}$

OnBeforeIncrNextVATEntryNo(NextVATEntryNo, IsHandled); if IsHandled then exit; still lets a subscriber skip VAT entry number allocation entirely, and this PR moves the concurrent-posting sequence allocation (GetNextVATEntryNoForPosting, which calls `Codeunit "Sequence No.

Mgt."underGLSetup.UseConcurrentPosting()`) inside that bypassable block. Number/sequence allocation is a critical operation per the referenced guidance (number-series consumption must not be skippable via IsHandled); a subscriber that sets IsHandled now silently skips sequence-based VAT entry numbering, risking duplicate or inconsistent VAT entry numbers under concurrent posting. Keep IsHandled scoped to overriding the computed value only, and run the allocation unconditionally, or expose a positive OnAfter event instead.

Knowledge:

👍 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

Finance GitHub request for Finance area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants