Test-Code-Reviewer-Comments#9607
Conversation
|
Could not find a linked ADO work item. Please link one by using the pattern 'AB#' followed by the relevant work item number. You may use the 'Fixes' keyword to automatically resolve the work item when the pull request is merged. E.g. 'Fixes AB#1234' |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Spanish (ES) localization of G/L Entry by moving the ES-specific "Bill No." field out of the localized table definition and into a dedicated tableextension, while also adjusting related table logic.
Changes:
- Added ES-local
tableextensionfor G/L Entry introducing field "Bill No." withDataClassification = CustomerContent. - Removed "Bill No." from the ES localized
table 17 "G/L Entry"definition and adjusted a related key definition. - Modified
CopyFromDeferralPostBufferand itsOnAfterCopyFromDeferralPostBufferIntegrationEvent signature.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Layers/ES/BaseApp/Local/Finance/GeneralLedger/Ledger/GLEntryES.TableExt.al | Introduces ES-specific tableextension for G/L Entry, adding "Bill No." as an extension field. |
| src/Layers/ES/BaseApp/Finance/GeneralLedger/Ledger/GLEntry.Table.al | Removes "Bill No." from the localized table definition and adjusts related logic/event surface. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "IC Partner Code" := GenJnlLine."IC Partner Code"; | ||
| "Prod. Order No." := GenJnlLine."Prod. Order No."; | ||
| "Bill No." := GenJnlLine."Bill No."; | ||
|
|
||
| OnAfterCopyGLEntryFromGenJnlLine(Rec, GenJnlLine); |
| @@ -1170,7 +1167,7 @@ | |||
| "Tax Group Code" := DeferralPostBuffer."Tax Group Code"; | |||
| "Use Tax" := DeferralPostBuffer."Use Tax"; | |||
|
|
|||
| OnAfterCopyFromDeferralPostBuffer(Rec, DeferralPostBuffer); | |||
| OnAfterCopyFromDeferralPostBuffer(Rec, DeferralPostBuffer, Test); | |||
| end; | |||
| /// <summary> | ||
| /// Integration event raised after copying G/L entry values from deferral posting buffer. | ||
| /// Enables extensions to modify G/L entry fields during deferral posting. | ||
| /// </summary> | ||
| /// <param name="GLEntry">G/L entry being updated</param> | ||
| /// <param name="DeferralPostingBuffer">Source deferral posting buffer</param> | ||
| /// <param name="Test">Test flag</param> | ||
| [IntegrationEvent(false, false)] | ||
| local procedure OnAfterCopyFromDeferralPostBuffer(var GLEntry: Record "G/L Entry"; DeferralPostingBuffer: Record "Deferral Posting Buffer") | ||
| local procedure OnAfterCopyFromDeferralPostBuffer(var GLEntry: Record "G/L Entry"; DeferralPostingBuffer: Record "Deferral Posting Buffer"; Test: Boolean) | ||
| begin | ||
| end; |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| { | ||
| } | ||
| key(Key13; "G/L Account No.", "Document No.", "Bill No.") | ||
| key(Key13; "G/L Account No.", "Document No.") |
There was a problem hiding this comment.
Field 7000000 "Bill No." is deleted outright from the shipped base-app table "G/L Entry" (table 17) instead of being staged through ObsoleteState. This field has existed since early NAV/ES sync commits, so it is a released, shipped field with persisted customer data. Deleting it (and its use in key(Key13; "G/L Account No.", "Document No.", "Bill No.")) removes the on-disk column and its data with no ObsoleteState=Pending/Removed staging, no ObsoleteReason/ObsoleteTag, and no upgrade code to migrate the stored values into the new tableextension field of the same ID. Re-adding a field under the same ID 7000000 via a table extension is not an established/supported migration path and is not guaranteed to preserve existing data across the upgrade.
Knowledge:
- microsoft/knowledge/breaking-changes/obsolete-table-fields-instead-of-deleting-them.md
- microsoft/knowledge/upgrade/obsolete-pending-to-removed-staging.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
| /// <param name="Test">Test flag</param> | ||
| [IntegrationEvent(false, false)] | ||
| local procedure OnAfterCopyFromDeferralPostBuffer(var GLEntry: Record "G/L Entry"; DeferralPostingBuffer: Record "Deferral Posting Buffer") | ||
| local procedure OnAfterCopyFromDeferralPostBuffer(var GLEntry: Record "G/L Entry"; DeferralPostingBuffer: Record "Deferral Posting Buffer"; Test: Boolean) |
There was a problem hiding this comment.
CopyFromDeferralPostBuffer declares a local variable Test: Boolean that is never assigned any value (always defaults to false) and is passed only to raise the new OnAfterCopyFromDeferralPostBuffer event parameter, also named Test. The name and the fact that it carries no computed value strongly suggest leftover debug/placeholder code rather than an intentional API addition. Publishing an event parameter with a non-descriptive name and no real semantics permanently pollutes the (local) event's public signature for subscribers with no actual capability. If a genuine capability is intended, name the parameter for what it represents and populate it meaningfully before publishing; otherwise remove it and the unused variable. Note: because the publisher is local, adding this parameter itself does not break existing subscribers (they bind by name and can ignore it), so the real defect is the meaningless placeholder rather than the addition of a parameter.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
| namespace Microsoft.Finance.GeneralLedger.Ledger; | ||
|
|
||
| tableextension 7000017 "G/L Entry ES" extends "G/L Entry" | ||
| { |
There was a problem hiding this comment.
"Bill No." is removed from table 17 "G/L Entry" and the line that populated it during posting ("Bill No." := GenJnlLine."Bill No." in CopyGLEntryFromGenJnlLine) is deleted, but no upgrade code migrates existing persisted values into the new tableextension field "G/L Entry ES"."Bill No." (same ID 7000000), and no event subscriber is added anywhere in the repository to repopulate it from GenJnlLine during posting going forward (confirmed: no subscriber to OnAfterCopyGLEntryFromGenJnlLine exists for G/L Entry in the ES layer). "Bill No." is still read elsewhere (e.g. ReversalEntry.Table.al, GeneralLedgerEntries.Page.al), so after this change it will silently be blank for all newly posted G/L entries, and any pre-existing data in the column is at risk during the underlying schema change. This is real impact would normally be major/blocker; it is capped to minor here per agent-finding rules pending a knowledge-backed rule or a fix that (a) migrates existing data and (b) restores population of the field via a subscriber in the ES tableextension/codeunit.
Agent judgement — not directly backed by a BCQuality knowledge article.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4
What & why
Linked work
Fixes #
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Risk & compatibility