Bugs/concurrent posting for general ledger gl entry

[Alexander-Ya]

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

[github-actions]

$\textbf{🟡\ Medium\ Severity\ —\ Agent} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

CreateGeneralLedgerSetup.Codeunit.al unconditionally forces "Use Concurrent Posting" := true for every country's demo-data company, with a comment reading "to test concurrent posting for all countries".

This reads as leftover diagnostic/test code rather than an intentional demo-data default: it silently changes the posting behavior of the standard Contoso demo company for every locale, with no gating on a test/debug context, and the comment itself signals it was added for local verification. (Separately, the statement references the variable via inconsistent casing, "GeneralledgerSetup" instead of "GeneralLedgerSetup" — this compiles because AL identifiers are case-insensitive, but it is inconsistent with the casing used on every other line in the same procedure.) If enabling concurrent posting in the demo dataset is intentional, the hardcoded value should be reviewed and the debug-style comment removed; if it was left in from local testing, it should be reverted before merge.

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

[github-actions]

$\textbf{🟠\ High\ Severity\ —\ Privacy} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

New field 21 "SIFT Bucket No." on table 17 "G/L Entry" has no DataClassification property.

Its immediate neighbor, field 20 "Source Currency Code", explicitly sets DataClassification = SystemMetadata, but the new field omits the property entirely, so the AL compiler applies the ToBeClassified sentinel. This field is a purely system-generated technical value (the OnInsert trigger sets it to "G/L Register No." mod 5) and should be classified the same way as other technical fields on this table.

Suggested change

	field(21; "SIFT Bucket No."; Integer)
	{
	Caption = 'SIFT Bucket No.';
	ToolTip = 'Specifies an automatically generated number that is used by the system to enable better concurrency.';
	Editable = false;
	}
	field(21; "SIFT Bucket No."; Integer)
	{
	Caption = 'SIFT Bucket No.';
	ToolTip = 'Specifies an automatically generated number that is used by the system to enable better concurrency.';
	Editable = false;
	DataClassification = SystemMetadata;
	}

Knowledge:

• microsoft/knowledge/privacy/resolve-tobeclassified-before-release.md

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

[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	2	0	2	1	1
Security	1	1	0	1	0
Testing	1	0	1	0	1

Totals: 1 knowledge-backed · 3 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.}

[github-actions]

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

The new "SIFT Bucket No." field is computed as 'Rec."G/L Register No." mod 5' in OnInsert and is appended to Key2 (the SumIndexFields key used for G/L account/posting-date aggregation) specifically to reduce SIFT-index write contention.

Every G/L entry created within the same posting run shares the same G/L Register No., so every entry from a single large batch (mass recurring-journal posting, a big sales/purchase batch, etc.) lands in exactly the same bucket — the scenario most likely to generate heavy SIFT-index write contention in the first place gets no distribution benefit at all; only different, non-overlapping posting runs land in different buckets. The bucket key would more directly serve the stated 'better concurrency' goal if it were derived from something that varies within a single run (e.g. the G/L Entry's own Entry No.) rather than from the Register No. that is constant for the whole run. The underlying impact could be more than cosmetic for high-volume batch posting, but this is capped at minor since no BCQuality knowledge file specifically addresses SIFT bucket-key selection strategy today.

Suggested change

	Rec."SIFT Bucket No." := Rec."G/L Register No." mod 5;
	Rec."SIFT Bucket No." := Rec."Entry No." mod 5;

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

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

[github-actions]

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

This PR replaces the LockTable()+GetLastEntry()/FindLast() pattern for G/L Entry and VAT Entry numbering with sequence-based GetNextEntryNo() calls whenever GeneralLedgerSetup.UseConcurrentPosting() is true, specifically to remove the serialization point that LockTable causes under concurrent posting.

However, StartPosting still unconditionally does 'GLReg.LockTable(); if GLReg.FindLast() then GLReg."No." := GLReg."No." + 1 else GLReg."No." := 1;' to assign the next G/L Register number, in the very same procedure that was edited to branch on UseConcurrentPosting for VAT Entry numbering a few lines above. Because every posting run must go through this table lock to obtain its register number, all concurrent postings still serialize on the G/L Register table regardless of the new setting, which defeats the concurrency goal the rest of the change is trying to achieve for G/L Entry and VAT Entry. The real impact here would normally be major (it undermines the feature's stated purpose), but per agent-finding rules this is capped at minor since no BCQuality knowledge file backs it yet; the concern should be promoted to a knowledge-backed rule (or the register numbering path should be given the same sequence-based treatment) rather than accepted at this severity long-term.

_{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}

[github-actions]

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

The new VAT Entry.GetNextEntryNo() procedure is decorated with [InherentPermissions(PermissionObjectType::TableData, Database::"VAT Entry", 'r')], copied from the pre-existing GetLastEntryNo() immediately below it.

GetLastEntryNo() actually reads the VAT Entry table (via FindRecordManagement.GetLastEntryIntFieldValue) so the grant matches its body, but GetNextEntryNo() only calls SequenceNoMgt.GetNextSeqNo(DATABASE::"VAT Entry") — it never Gets, Finds, or otherwise reads a VAT Entry row. The inherent-permissions grant should match the procedure's actual work; here it grants table-data read access on VAT Entry that the procedure's body never exercises.

Knowledge:

• microsoft/knowledge/security/inherent-permissions-minimal-grant.md

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

[github-actions]

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

This PR adds several new conditional code paths gated on GeneralLedgerSetup.UseConcurrentPosting() — sequence-based G/L Entry/VAT Entry numbering in InitNextEntryNo/StartPosting/ContinuePosting, the new GetNextGLEntryNoForPosting/GetNextVATEntryNoForPosting helpers, the PreviousEntryNo rollback path in UpdateGLEntryNo, and the new ValidateSequenceNo call for VAT Entry — none of which are exercised by any test file in this diff (no *.Test.al / *.Codeunit.al test file appears in the changed-file list).

Given the change affects core posting entry-numbering logic across every localization layer, the absence of any accompanying automated test for the new UseConcurrentPosting = true branch is a real coverage gap for a change this central to posting correctness. Impact would ordinarily warrant more than a minor flag, but this is capped per agent-finding rules since no BCQuality knowledge file backs the specific gap.

_{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}