Skip to content

Test-Code-Reviewer-Comments #9607

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Alexander-Ya wants to merge 1 commit into
base: main
Choose a base branch
from
+23 −8
Open

Test-Code-Reviewer-Comments #9607

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions src/Layers/ES/BaseApp/Finance/GeneralLedger/Ledger/GLEntry.Table.al
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -789,10 +789,6 @@ table 17 "G/L Entry"
Caption = 'Period Trans. No.';
DataClassification = SystemMetadata;
}
field(7000000; "Bill No."; Code[20])
{
Caption = 'Bill No.';
}
}

keys
Expand Down Expand Up @@ -844,7 +840,7 @@ table 17 "G/L Entry"
key(Key12; "VAT Bus. Posting Group", "VAT Prod. Posting Group")
{
}
key(Key13; "G/L Account No.", "Document No.", "Bill No.")
key(Key13; "G/L Account No.", "Document No.")

@github-actions github-actions Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟠\ High\ Severity\ —\ Breaking\ Changes}$

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:

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.12.4

{
SumIndexFields = Amount, "Additional-Currency Amount";
}
Expand Down Expand Up @@ -1060,7 +1056,6 @@ table 17 "G/L Entry"
"No. Series" := GenJnlLine."Posting No. Series";
"IC Partner Code" := GenJnlLine."IC Partner Code";
"Prod. Order No." := GenJnlLine."Prod. Order No.";
"Bill No." := GenJnlLine."Bill No.";

OnAfterCopyGLEntryFromGenJnlLine(Rec, GenJnlLine);
Comment on lines 1057 to 1060
end;
Expand Down Expand Up @@ -1158,6 +1153,8 @@ table 17 "G/L Entry"
/// </summary>
/// <param name="DeferralPostBuffer">Deferral posting buffer to copy from</param>
procedure CopyFromDeferralPostBuffer(DeferralPostBuffer: Record "Deferral Posting Buffer")
var
Test: Boolean;
begin
"System-Created Entry" := DeferralPostBuffer."System-Created Entry";
"Gen. Posting Type" := DeferralPostBuffer."Gen. Posting Type";
Expand All @@ -1170,7 +1167,7 @@ table 17 "G/L Entry"
"Tax Group Code" := DeferralPostBuffer."Tax Group Code";
"Use Tax" := DeferralPostBuffer."Use Tax";

OnAfterCopyFromDeferralPostBuffer(Rec, DeferralPostBuffer);
OnAfterCopyFromDeferralPostBuffer(Rec, DeferralPostBuffer, Test);
end;
Comment on lines 1155 to 1171

/// <summary>
Expand Down Expand Up @@ -1218,8 +1215,9 @@ table 17 "G/L Entry"
/// </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)

@github-actions github-actions Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Events}$

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

begin
end;

Expand Down
17 changes: 17 additions & 0 deletions src/Layers/ES/BaseApp/Local/Finance/GeneralLedger/Ledger/GLEntryES.TableExt.al
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// ------------------------------------------------------------------------------------------------
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License. See License.txt in the project root for license information.
// ------------------------------------------------------------------------------------------------
namespace Microsoft.Finance.GeneralLedger.Ledger;

tableextension 7000017 "G/L Entry ES" extends "G/L Entry"
{

@github-actions github-actions Bot Jul 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$\textbf{🟡\ Medium\ Severity\ —\ Upgrade}$

"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

fields
{
field(7000000; "Bill No."; Code[20])
{
Caption = 'Bill No.';
DataClassification = CustomerContent;
}
}
}
Loading