Concurrent Posting for General Ledger VAT Entry#9442
Conversation
Copilot PR ReviewIteration 4 · Outcome: completed Knowledge source: https://github.com/microsoft/BCQuality@186d8a131465475c79244d994acb872cd5c0d4bf Findings by domainFindings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).
Totals: 2 knowledge-backed · 0 agent findings. Orchestrator pre-filter (2 file(s) excluded)
Findings produced by the AL review agent v1.7.3. Reply 👎 on any inline comment to flag false positives. |
Agentic PR Review - Round 1Recommendation: Request ChangesWhat this PR doesThis PR adds The direction matches the concurrent-posting pattern, but the implementation is not safe yet. The PR changes how SuggestionsS1 - Fix last VAT entry number callers S2 - Allocate VAT numbers only when needed S3 - Remove the test setup change S4 - Add focused posting tests S5 - Clarify or complete the concurrency scope Risk assessment and necessityRisk: 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 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.
|
Removed the line enabling concurrent posting for testing.
| /// 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 |
There was a problem hiding this comment.
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; | ||
|
|
There was a problem hiding this comment.
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
What & why
Linked work
Fixes AB#624164
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Risk & compatibility