Bug 642053: [master] [Sustainability] Preview Posting Creates Gaps in Sustainability Ledger Entry Numbers - #10051
Conversation
… numbers - intercept the ledger insert with an internal handled event before SQL allocation - store preview rows under session-local negative keys; keep AutoIncrement for real posts - add identity-continuity regressions for journal, purchase, and fixed-asset previews 🌱 - Generated by Copilot
GitHub Actions
Inactive
github-actions
Bot
added
AL: Apps (W1)
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if SustLedgEntry.IsTemporary() then | ||
| exit; | ||
|
|
||
| if NextSustLedgerPreviewEntryNo = 0 then |
There was a problem hiding this comment.
NextSustLedgerPreviewEntryNo uses 0 as its "not initialized" sentinel, but a counter that starts at -2000000000 can legitimately reach 0. On the next preview insert, this code resets the counter back to -2000000000 and reuses an existing temporary primary key, so a sufficiently large preview fails with a generic duplicate-key/runtime error instead of a controlled exhaustion check. Use a non-reachable sentinel or add an explicit guard when the negative preview range is exhausted.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
There was a problem hiding this comment.
The counter only reaches 0 after about 2 billion previews in one session, so it is not reachable in practice.
Aleksandr Gladkov (AleksanderGladkov)
changed the title
…AA0021 🎨 - Generated by Copilot
| var | ||
| SustainabilityLedgerEntry: Record "Sustainability Ledger Entry"; | ||
| FeatureTelemetry: Codeunit "Feature Telemetry"; | ||
| IsHandled: Boolean; |
There was a problem hiding this comment.
FeatureTelemetry.LogUsage('0000PH5', ...) and LogUptake(..., "Used") are still called unconditionally near the top of InsertLedgerEntry, before the new OnInsertLedgerEntryOnBeforeInsert/IsHandled branch decides whether a real ledger entry is ever persisted. With the new preview-diversion path, a call that ends up fully handled (no physical Insert) still reports feature usage as if a real Sustainability Ledger Entry was created, inflating usage telemetry for preview-only invocations of this procedure and making usage counts unreliable for measuring real adoption.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| PostedCrMemoNo: Code[20]; | ||
| BaselineEntryNo: Integer; | ||
| BaselineEmissionCO2: Decimal; | ||
| begin |
There was a problem hiding this comment.
The new preview-mode tests in SustGeneralJournalTest.Codeunit.al, SustValueChainFixedAsset.Codeunit.al, and SustainabilityPostingTest.Codeunit.al all follow asserterror with Assert.ExpectedError(''). An empty expected-error string matches any error text, so each of these tests will pass even if the failure is an unrelated setup or posting error rather than the intended preview-mode 'stop the transaction' error, silently defeating the purpose of the assertion.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
| [PageHandler] | ||
| procedure GLPostingPreviewPageHandler(var GLPostingPreview: TestPage "G/L Posting Preview") | ||
| begin | ||
| end; |
There was a problem hiding this comment.
GLPostingPreviewPageHandler in SustValueChainFixedAsset.Codeunit.al is an empty handler body wired to the new FA-journal preview test. It proves only that some preview page opened, without asserting the Sustainability Ledger Entry row count or count shown on the preview page, so a regression that drops or duplicates preview rows would not be caught.
| [PageHandler] | |
| procedure GLPostingPreviewPageHandler(var GLPostingPreview: TestPage "G/L Posting Preview") | |
| begin | |
| end; | |
| [PageHandler] | |
| procedure GLPostingPreviewPageHandler(var GLPostingPreview: TestPage "G/L Posting Preview") | |
| begin | |
| GLPostingPreview.Filter.SetFilter("Table ID", Format(Database::"Sustainability Ledger Entry")); | |
| GLPostingPreview."No. of Records".AssertEquals(1); | |
| GLPostingPreview.OK().Invoke(); | |
| end; |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.31.4
Agentic PR Review - Round 1Recommendation: Accept with SuggestionsWhat this PR doesThis PR fixes a gap in The design is correct. The SuggestionsS1 -
Risk assessment and necessityRisk: Low. The change is additive — it introduces a new internal event and one new subscriber; it does not modify any existing event or posting logic. Normal posting (not in preview mode) is fully unchanged because the new subscriber is only bound during preview. The negative temporary keys (-2,000,000,000 and up) are well outside the range of real AutoIncrement values and cannot collide with committed entries. The Necessity: Clear. Entry-number gaps in a financial ledger are a data-quality issue that can confuse auditors and break sequence-dependent queries. The fix is scoped precisely to the root cause and does not touch any other posting path.
|
…G/L account - CreateBankAccount overload creates a posting group that has a G/L account, so the journal line can post 🧪 - Generated by Copilot
|
InsertLedgerEntry still calls FeatureTelemetry.LogUsage('0000PH5', ...) before the new OnInsertLedgerEntryOnBeforeInsert/IsHandled branch decides whether a real ledger-entry insert will happen. Sust. Preview Posting Handler handles that event by copying the record into a temporary preview buffer and setting IsHandled := true, so preview runs now emit a successful "Sustainability Ledger Entry Added" usage event even though no committed ledger entry was created. Keep LogUptake(...Used) if attempt telemetry is desired, but move LogUsage to the path that runs only after a real insert succeeds. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4 |
Agentic PR Review - Round 2Recommendation: AcceptWhat this PR doesThis PR fixes Sustainability Ledger Entry number gaps caused by Preview Posting. The product code still redirects preview-only Sustainability Ledger Entries into a temporary buffer before the real insert, using negative temporary entry numbers so SQL AutoIncrement values are not consumed. This round covers the commit after round 1. The new commit only changes the General Journal preview tests to create a bank account whose posting group has a G/L account, so the test journal lines can post through the intended path. That setup change is correct and does not change product behavior. Status of previous suggestions
New observations (commits since round 1)None - the new commit only corrects the General Journal test setup. Risk assessment and necessityRisk: The post-round-1 change is test-only and narrow. The product risk remains limited to preview posting for Sustainability entries; normal posting is still outside the manual preview subscriber path. Necessity: Clear. The bug affects ledger entry-number continuity after preview posting, and the added test setup change is needed so the new General Journal preview regression tests exercise the intended posting path.
|
| SustPreviewPostInstance.InsertSustLedgEntry(SustainabilityLedgerEntry, true); | ||
| IsHandled := true; | ||
| end; | ||
|
|
There was a problem hiding this comment.
This PR adds a new [InternalEvent] OnInsertLedgerEntryOnBeforeInsert in codeunit 6212 "Sustainability Post Mgt", raised immediately before SustainabilityLedgerEntry.Insert(true) and guarded by IsHandled. Codeunit 6228 "Sust. Preview Posting Handler" subscribes to it and, when bound during preview, sets IsHandled := true, which now prevents the real Insert(true) call from ever running during preview. However, that same codeunit 6228 still contains the pre-existing subscriber OnInsertSustLedgEntry on Database::"Sustainability Ledger Entry" 'OnAfterInsertEvent' (the previous, buggy mechanism for capturing preview entries after a real insert). Since the real Insert(true) can no longer occur while codeunit 6228 is bound (the only scenario the OnAfterInsertEvent subscriber is active in), that OnAfterInsertEvent subscriber is now unreachable dead code for the ledger-entry preview path it was written for. Recommend removing (or repurposing) OnInsertSustLedgEntry to avoid confusing future maintainers into thinking it still participates in the preview flow.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4
|
In codeunit 6212 "Sustainability Post Mgt", procedure InsertLedgerEntry still calls FeatureTelemetry.LogUsage('0000PH5', SustainabilityLbl, SustainabilityLedgerEntryAddedLbl) near the top of the procedure, before the new OnInsertLedgerEntryOnBeforeInsert event is raised and before it is known whether the real Sustainability Ledger Entry insert will actually happen. With this PR's fix, a bound preview subscriber (codeunit 6228) now sets IsHandled := true and skips SustainabilityLedgerEntry.Insert(true) entirely during preview, redirecting the row to a temporary buffer instead. Previously, previewing still inserted into the real table (which was the very identity-consumption bug this PR fixes), so the LogUsage call at least corresponded to a real insert; after this fix, every preview run will now log 'Sustainability Ledger Entry Added' usage telemetry even though no real ledger entry is created. This inflates usage/adoption telemetry with preview-only attempts. Move the LogUsage call to after Insert(true) succeeds in the not-IsHandled branch (or condition it on not IsHandled) so preview runs are not counted as real usage. Knowledge: Line mapping was unavailable, so this was posted as an issue comment. 👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4 |
2 participants
ISSUE:
Running Preview Posting on a journal line that includes Sustainability emissions leaves gaps in the Sustainability Ledger Entry "Entry No." sequence (for example, 128 then 132). General Ledger Entries are not affected and stay sequential.
CAUSE:
Preview built the Sustainability Ledger Entries by physically inserting them, which consumed the AutoIncrement identity. The rollback that ends the preview did not return those numbers, so every preview permanently burned entry numbers.
SOLUTION:
Preview no longer inserts real Sustainability Ledger Entries. The shared insert is intercepted just before it reaches the database, and preview rows are collected in memory under temporary numbers, so no identity is used. Normal posting is untouched and keeps its sequential AutoIncrement numbering.
TESTS:
Added identity-continuity regressions for repeated general journal preview, purchase credit memo, native and recurring Sustainability journals, a custom generic preview path, and direct fixed asset journal preview. Each verifies that repeated preview consumes no number and the next real post continues the sequence.
Fixes AB#642053