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 |
| end; | ||
|
|
||
| [Test] | ||
| [HandlerFunctions('ConfirmHandler,MessageHandler')] |
There was a problem hiding this comment.
The newly added journal-posting tests opt into ConfirmHandler and MessageHandler, but those shared handlers always reply true / ignore the message and the tests do not enqueue expected interactions or prove the queue is empty. That means the tests never verify which confirm/message was raised or how many times, so a wrong dialog contract can still leave them green.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.32.4
| // [WHEN] Preview the second FA Journal Line through FA Jnl.-Post.Preview. | ||
| FAJournalLine.SetRange("Journal Template Name", FAJournalLine."Journal Template Name"); | ||
| FAJournalLine.SetRange("Journal Batch Name", FAJournalLine."Journal Batch Name"); | ||
| asserterror FAJnlPost.Preview(FAJournalLine); |
There was a problem hiding this comment.
The new FA preview test follows asserterror with Assert.ExpectedError(''), so it still accepts any error instead of proving that the expected preview-mode failure occurred. That leaves the test green on unrelated posting or fixture defects.
Knowledge:
👍 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