Extract NL customization to object extensions (TrMode)#8933
Extract NL customization to object extensions (TrMode)#8933Alexander-Ya wants to merge 10 commits into
Conversation
|
Fixes AB#640601 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| var | ||
| Text1000000: Label '%1 can only be filled in when %2 %3 is equal to Customer or Vendor.'; | ||
| } |
There was a problem hiding this comment.
Label 'Text1000000' has no approved suffix and is missing a Comment for its three placeholders.
The label is passed to Error() so it must end with the 'Err' suffix (AA0074). Its three placeholders (%1 = FieldCaption('Transaction Mode Code'), %2 = FieldCaption('Account Type'), %3 = FieldCaption('Bal. Account Type')) also require a Comment parameter (AA0470). Rename the label to 'Text1000000Err' and add the Comment, then update the Error() call site to match.
| } | |
| Text1000000Err: Label '%1 can only be filled in when %2 %3 is equal to Customer or Vendor.', Comment = '%1 = Field Caption (Transaction Mode Code), %2 = Field Caption (Account Type), %3 = Field Caption (Bal. Account Type)'; |
Knowledge:
- microsoft/knowledge/style/label-suffix-approved-list.md
- microsoft/knowledge/style/label-comment-explains-placeholders.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| } | ||
|
|
||
| var | ||
| Text1000001: Label 'It is not allowed to specify %1 when %2 is %3.'; |
There was a problem hiding this comment.
Label 'Text1000001' has no approved suffix and is missing a Comment for its three placeholders.
The label is passed to Error() so it must end with the 'Err' suffix (AA0074). Its three placeholders (%1 = Field Caption 'Currency Euro', %2 = Field Caption 'Local Currency', %3 = the option value) also require a Comment parameter (AA0470) so translators can identify each slot. Rename the label to 'Text1000001Err' and add the Comment, then update the Error() call site to match.
| Text1000001: Label 'It is not allowed to specify %1 when %2 is %3.'; | |
| Text1000001Err: Label 'It is not allowed to specify %1 when %2 is %3.', Comment = '%1 = Field Caption (Currency Euro), %2 = Field Caption (Local Currency), %3 = Local Currency value'; |
Knowledge:
- microsoft/knowledge/style/label-suffix-approved-list.md
- microsoft/knowledge/style/label-comment-explains-placeholders.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| end; | ||
|
|
||
| var | ||
| Text001: Label 'must be 0 if %1 is %2'; |
There was a problem hiding this comment.
Labels 'MustBe0Lbl', 'MustBeOddLbl', and 'MustBeEvenLbl' carry the 'Lbl' suffix, which is reserved for captions and tooltips, but all three are used as the error-description argument to FieldError() — an error-producing call.
Per the label-suffix guidance, labels consumed in error contexts must use the 'Err' suffix. Rename them to 'MustBe0Err', 'MustBeOddErr', and 'MustBeEvenErr', and update the six FieldError() call sites accordingly.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| TransactionMode: Record "Transaction Mode"; | ||
| AccountType: Option Customer,Vendor,Employee; | ||
| begin | ||
| if not TransactionMode.CheckTransactionModePartnerType(AccountType::Vendor, "Transaction Mode Code", "Partner Type") then |
There was a problem hiding this comment.
Label 'PartnerTypeMismatchMsg' uses the 'Msg' suffix, which is reserved for Message() calls, but the label is passed to Confirm().
Per the label-suffix guidance, labels consumed by Confirm() or StrMenu must use the 'Qst' suffix. Note that the parallel label in CustomerNL.TableExt.al is already correctly named 'PartnerTypeMismatchQst'. Rename this label to 'PartnerTypeMismatchQst' and update its Confirm() call site.
| if not TransactionMode.CheckTransactionModePartnerType(AccountType::Vendor, "Transaction Mode Code", "Partner Type") then | |
| PartnerTypeMismatchQst: Label 'The Partner Type does not match the Partner Type defined in Transaction Mode. Do you still want to change the Partner Type?'; |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| VendBankAcc: Record "Vendor Bank Account"; | ||
| IsHandled: Boolean; | ||
| begin | ||
| IsHandled := false; |
There was a problem hiding this comment.
Confirm() is called with a StrSubstNo-wrapped format string: 'Confirm(StrSubstNo(UpdateBankAccountsQst, UseFieldCaption))'.
The AL platform's Confirm() accepts a format string and variable arguments directly — wrapping with StrSubstNo hides the placeholder from analyzers and breaks the translation link. Replace with 'Confirm(UpdateBankAccountsQst, false, UseFieldCaption)', passing false as the default-button argument and the caption as the format argument.
| IsHandled := false; | |
| if not Confirm(UpdateBankAccountsQst, false, UseFieldCaption) then |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
There was a problem hiding this comment.
Code suggestion is off. Addressing issue in #8930.
| CustBankAcc: Record "Customer Bank Account"; | ||
| IsHandled: Boolean; | ||
| begin | ||
| IsHandled := false; |
There was a problem hiding this comment.
Confirm() is called with a StrSubstNo-wrapped format string: 'Confirm(StrSubstNo(UpdateBankAccountsQst, UseFieldCaption))'.
The AL platform's Confirm() accepts a format string and variable arguments directly — wrapping with StrSubstNo hides the placeholder from analyzers and breaks the translation link. Replace with 'Confirm(UpdateBankAccountsQst, false, UseFieldCaption)', passing false as the default-button argument and the caption as the format argument.
| IsHandled := false; | |
| if not Confirm(UpdateBankAccountsQst, false, UseFieldCaption) then |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
There was a problem hiding this comment.
Code suggestion is off. Addressing issue in #8930.
Copilot PR ReviewIteration 6 · Outcome: completed
Knowledge source: https://github.com/microsoft/BCQuality@822cae1b2771ac25f665f73369f69093bd4fd630 Findings by domainFindings split into Knowledge-backed (cite a BCQuality article) and Agent (the agent's own judgement, no matching BCQuality rule).
Totals: 2 knowledge-backed · 1 agent findings. Orchestrator pre-filter (13 file(s) excluded)
Findings produced by the Copilot CLI agent against BCQuality at |
The model-reported anchor for a single-line suggestion can be off by more than 8 lines (e.g. PR #8933 VendorNL Confirm() was off by 10, label rename off by 9). Widen the search window to 40 lines either side so the correct target within the same procedure is considered, while the 0.5 similarity floor and 0.1 ambiguity margin keep an unrelated look-alike from winning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Widening the search window exposed a precision problem: a label-rename suggestion whose added Comment text echoes the field captions at the Error()/Confirm() call site would re-anchor onto the call site (PR #8933 GenJournalLineNL/GeneralLedgerSetupNL 'Text1000000/1' findings), which is worse than the original mis-anchor. Raise the similarity floor to 0.6 and use a bounded window (20). Genuine edit targets - an edited statement or a renamed declaration - score ~0.75-0.99 and re-anchor confidently (the PR #8933 Confirm() findings and the PartnerTypeMismatchMsg->Qst rename now land on the right line). Lower- confidence look-alikes stay below the floor and are suppressed, so the caller posts a manual snippet instead of a wrong auto-applicable anchor. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| VATMod11NotAllowedCharErr: Label 'The VAT registration number must have the format NLdddddddddBdd where d is a digit.'; | ||
| VATMod97NotAllowedCharErr: Label 'The VAT registration number for a natural person must have the format NLXXXXXXXXXXdd where d is a digit, and x can be a digit, an uppercase letter, ''+'', or ''*''.'; | ||
| VATMod11Err: Label 'The VAT registration number is not valid according to the Modulus-11 checksum algorithm.'; | ||
| VATMod97Err: Label 'The VAT registration number is not valid according to the Modulus-97 checksum algorithm.'; |
There was a problem hiding this comment.
'SummaryTwoErr' has Comment = '%1, %2 - error text': both placeholders are described only as 'error text' without naming what they represent.
%1 is always the fixed format-error preamble (VATRegNoFormatErr) and %2 is the specific validation error string (e.g. Mod11 or Mod97 error). Translators working from the label alone cannot determine whether word order can be changed, as required in many languages.
| VATMod97Err: Label 'The VAT registration number is not valid according to the Modulus-97 checksum algorithm.'; | |
| SummaryTwoErr: Label '%1%2', Comment = '%1 = VAT registration number format error prefix, %2 = specific validation error detail'; |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| @@ -0,0 +1,22 @@ | |||
| // ------------------------------------------------------------------------------------------------ | |||
There was a problem hiding this comment.
The file is named 'COmpanyInformationNL.Codeunit.al' — the leading 'CO' breaks PascalCase.
The correct name is 'CompanyInformationNL.Codeunit.al', matching the AL object name 'Company Information NL'. Wrong casing breaks grep, symbol search, and the implicit file-to-object map that tooling relies on.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| Caption = 'Open'; | ||
| DataClassification = CustomerContent; | ||
| InitValue = true; | ||
| } |
There was a problem hiding this comment.
The new 'Open' Boolean field added to G/L Entry via GLEntryNL.TableExt.al declares InitValue = true.
G/L Entry is a production-scale ledger table: every row that existed before this extension is installed will carry Open = false (the datatype default), not true. GLEntryNL.Codeunit.al branches on GLEntry.Open at runtime, so existing entries silently fall into the wrong code path. The analogous InitValue = true field on GeneralLedgerSetup ('Local SEPA Instr. Priority') is correctly paired with upgrade code in UPGSEPANL.Codeunit.al, but no equivalent upgrade procedure exists for G/L Entry in this PR.
Recommendation:
- add an upgrade procedure that sets Open = true for existing G/L Entry rows via DataTransfer or ModifyAll, guarded by an upgrade tag registered in UpgradeTagDefCountry.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
…Extract-NL-customization-to-object-extensions-v2
…Extract-NL-customization-to-object-extensions-v2
The action "General Business" had its
|
| PartnerTypeMismatchQst: Label 'The Partner Type does not match the Partner Type defined in Transaction Mode. Do you still want to change the Partner Type?'; | ||
|
|
||
| [Scope('OnPrem')] | ||
| procedure UpdateCustomerBankAccounts(UseFieldCaption: Text[250]) |
There was a problem hiding this comment.
In the new CustomerNL.TableExt.al, UpdateCustomerBankAccounts was changed from the original local procedure (no scope restriction) to a global procedure decorated with [Scope('OnPrem')].
It is only ever called from OnAfterValidate triggers within the same table extension (Name/Address/City/Country/Post Code), so making it global is unnecessary API-surface exposure, and Scope('OnPrem') was never present on the original code — it appears to have been copy-pasted from the analogous VendorNL.TableExt.al, where [Scope('OnPrem')] legitimately pre-existed on UpdateVendorBankAccounts in the pre-PR Vendor.Table.al. Restricting a procedure invoked from a base-table trigger chain to on-premises-only risks breaking that validation flow for Business Central Online (SaaS) tenants running the NL localization, which is a functional regression, not merely a style nit — the true impact would warrant major, but per BCQuality's agent-finding rules this is capped at minor until a knowledge-backed rule covers this pattern; the concern crosses the security (API-surface) and reliability/availability (SaaS-vs-OnPrem) domains, which is why no single leaf owns it.
Recommendation:
- revert to
local procedureand drop[Scope('OnPrem')]to match the pre-existing (correct) behavior.
| procedure UpdateCustomerBankAccounts(UseFieldCaption: Text[250]) | |
| local procedure UpdateCustomerBankAccounts(UseFieldCaption: Text[250]) |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| field(11000000; "Transaction Mode Code"; Code[20]) | ||
| { | ||
| Caption = 'Transaction Mode Code'; | ||
| DataClassification = CustomerContent; |
There was a problem hiding this comment.
Customer, Vendor, Sales Header, and Employee already declare DataClassification = CustomerContent at the table level (base app).
The new NL telebanking fields added by this PR (e.g. "Transaction Mode Code", "Bank Account Code", "Bank Name", "Bank City") repeat the identical DataClassification = CustomerContent at field level in the new table extensions. Per the referenced guidance this is the 'mirror anti-pattern': redundant field-level classification that duplicates an already-declared table-level default and adds nothing. Occurs in CustomerNL.TableExt.al:23, VendorNL.TableExt.al:16, SalesHeaderNL.TableExt.al:23/45, EmployeeNL.TableExt.al:16/30/35, and PurchaseHeaderNL.TableExt.al:17/36.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| @@ -77,6 +77,7 @@ page 8900 "Administrator Main Role Center" | |||
| ApplicationArea = All; | |||
| Caption = 'System Information'; | |||
| RunObject = page "Latest Error"; | |||
| Tooltip = 'Open the System Information page.'; | |||
There was a problem hiding this comment.
Same 'Tooltip' vs.
'ToolTip' casing inconsistency as the al-style-review finding above, rolled up here because it also affects the UI/accessibility surface: pages carry a large number of new tooltip properties whose casing diverges from the codebase's dominant convention and from this article's own examples. No functional/accessibility impact (the tooltip still renders), but it is a widespread, easily-fixed naming inconsistency introduced by this PR across Role Center pages.
| Tooltip = 'Open the System Information page.'; | |
| ToolTip = 'Open the System Information page.'; |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| @@ -7581,7 +7582,7 @@ table 36 "Sales Header" | |||
| if ReplaceVATDate then | |||
| "VAT Reporting Date" := VATDateReq; | |||
|
|
|||
| if ReplaceDocDate and ("Document Date" <> PostingDateReq) then begin | |||
| if ReplacePostingDate and ReplaceDocDate and ("Document Date" <> PostingDateReq) then begin | |||
There was a problem hiding this comment.
In BatchConfirmUpdatePostingDate, the condition was changed to if ReplacePostingDate and ReplaceDocDate and ("Document Date" <> PostingDateReq) then begin, but the procedure already exits immediately at its top with if not ReplacePostingDate then exit;.
By the time execution reaches this line, ReplacePostingDate is always true, so the added conjunct is dead weight that changes nothing at runtime. It's harmless today but misleads future readers/maintainers into thinking the flag is still meaningfully guarding this branch, and could mask a future refactor mistake if the early exit is ever removed. Recommend reverting to the original if ReplaceDocDate and ("Document Date" <> PostingDateReq) then begin or, if defensive redundancy is deliberate, adding a short comment noting ReplacePostingDate is guaranteed true here.
| if ReplacePostingDate and ReplaceDocDate and ("Document Date" <> PostingDateReq) then begin | |
| if ReplaceDocDate and ("Document Date" <> PostingDateReq) then begin |
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| end; | ||
|
|
||
| [EventSubscriber(ObjectType::Table, Database::"Purchase Header", 'OnValidatePurchaseHeaderPayToVendorNoOnBeforeCheckDocType', '', false, false)] | ||
| local procedure OnValidatePurchaseHeaderPayToVendorNoOnBeforeCheckDocType(Vendor: Record Vendor; var PurchaseHeader: Record "Purchase Header"; xPurchaseHeader: Record "Purchase Header"; SkipPayToContact: Boolean) |
There was a problem hiding this comment.
The new subscriber OnValidatePurchaseHeaderPayToVendorNoOnBeforeCheckDocType in PurchaseHeaderNL.Codeunit.al declares xPurchaseHeader: Record "Purchase Header" without var, while the publisher (PurchaseHeader.Table.al) declares it as var xPurchaseHeader.
Every other subscriber of this same event in the repo that includes this parameter (CZ PurchaseHeaderHandlerCZL, DK FIKSubscribers, NA DIOTSubscribers) declares it as var xPurchaseHeader, matching the publisher. The referenced guidance requires reproducing the publisher signature verbatim; since the parameter is unused in this subscriber body, the safer and more consistent fix is to omit it entirely (as GSTPurchaseSubscribers does) rather than redeclare it with a mismatched modifier.
| local procedure OnValidatePurchaseHeaderPayToVendorNoOnBeforeCheckDocType(Vendor: Record Vendor; var PurchaseHeader: Record "Purchase Header"; xPurchaseHeader: Record "Purchase Header"; SkipPayToContact: Boolean) | |
| local procedure OnValidatePurchaseHeaderPayToVendorNoOnBeforeCheckDocType(Vendor: Record Vendor; var PurchaseHeader: Record "Purchase Header"; SkipPayToContact: Boolean) |
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
| GeneralLedgerSetup.Validate("Local SEPA Instr. Priority", true); | ||
| GeneralLedgerSetup.Modify(true); | ||
| GeneralLedgerSetup."Local SEPA Instr. Priority" := true; | ||
| GeneralLedgerSetup.Modify(false); |
There was a problem hiding this comment.
Two NL upgrade codeunits were changed from Validate(...) + Modify(true) to direct field assignment + Modify(false) when writing to pre-existing setup fields (`ElecTaxDeclarationSetup."Tax Decl.
Schema Version"et al., andGeneralLedgerSetup."Local SEPA Instr. Priority"). Neither field currently has an OnValidate/OnModify trigger, but Modify(true)also raises the table's genericOnBeforeModifyEvent/OnAfterModifyEventintegration events;Modify(false)` silently suppresses them. Any current or future subscriber relying on these generic modify events for these setup tables (audit, sync, telemetry) will stop firing for this update path. Per the referenced guidance's principle (applies equally to direct writes as to DataTransfer): this is fine only if intentional and safe — confirm with the field owners or add a comment stating the triggers are intentionally skipped.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why
Alexander-Ya
added
Finance
2 participants
What & why
Linked work
Fixes AB#640601
How I validated this
What I tested and the outcome (required — be specific: scenarios, commands, screenshots for UI changes)
Risk & compatibility