mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
Address review: OnAfterGetCurrRecord for action state, RC gating nits
- page-client-expression-must-not-use-in-list: recompute action/group/part state in OnAfterGetCurrRecord (OnAfterGetRecord runs per row), cite EDocumentLogs and concrete or-chain examples, note HideValue and that the property list is not exhaustive; good sample uses OnAfterGetCurrRecord. - rolecenter-permission-gating-must-use-accessbypermission: clarify LicenseFile vs LicenseFileAndUserPermissions removal, cite Business Manager RC Control96, samples gate a part the RC does not already have. - al-ui-review: add ReadPermission/WritePermission tokens. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer
parent
decbf7136c
commit
b6b532ecce
6 changed files with 21 additions and 18 deletions
|
|
@ -71,7 +71,7 @@ page 50700 "Sample Request Card"
|
|||
var
|
||||
ApproveEnabled: Boolean;
|
||||
|
||||
trigger OnAfterGetRecord()
|
||||
trigger OnAfterGetCurrRecord()
|
||||
begin
|
||||
UpdateActionStates();
|
||||
end;
|
||||
|
|
|
|||
|
|
@ -11,13 +11,13 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
`Enabled`, `Visible`, `Editable`, and `StyleExpr` on page controls can be bound to a client expression instead of a literal. The documented dynamic forms are a global Boolean page variable, a Boolean field, or a Boolean expression over fields such as `"Credit Limit" > "Sales YTD"`; plain `=`/`<>`/`>` comparisons combined with `and`/`or`/`not` are valid. An `in [...]` set-membership test, such as `Rec.Status in [Rec.Status::New, Rec.Status::"Needs Review"]`, is not: the compiler reports it as "InListExpression is not valid for client expressions. Client expressions can only use simple data types and field references."
|
||||
`Enabled`, `Visible`, `Editable`, and `StyleExpr` on page controls (the list is not exhaustive; `HideValue` is rejected the same way) can be bound to a client expression instead of a literal. The documented dynamic forms are a global Boolean page variable, a Boolean field, or a Boolean expression over fields such as `"Credit Limit" > "Sales YTD"`; plain `=`/`<>`/`>` comparisons combined with `and`/`or`/`not` are valid. An `in [...]` set-membership test, such as `Rec.Status in [Rec.Status::New, Rec.Status::"Needs Review"]`, is not: the compiler reports it as "InListExpression is not valid for client expressions. Client expressions can only use simple data types and field references."
|
||||
|
||||
The severity depends on the control. On a page field the diagnostic is already an error (AL0322). On an action, group, or part it is AL0573, a warning that "will become an error in a future release", so the code still builds and is easy to ship, suppress in a ruleset, or carry forward. A procedure call in the same property position is rejected by the same diagnostics, so moving the list test into a method called from the property does not fix it.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Express the condition in a form a client expression accepts. For a short list, rewrite the membership as an `or` chain of field comparisons, which keeps the property a live client expression. For a longer or computed condition, evaluate it in AL (an `in [...]` list is fine there), store the result in a global page `Boolean` variable, and bind the property to that variable. Recompute the variable wherever its inputs change: `OnAfterGetRecord` for record navigation, and the `OnValidate` of each page field the condition reads for in-place edits.
|
||||
Express the condition in a form a client expression accepts. For a short list, rewrite the membership as an `or` chain of field comparisons, which keeps the property a live client expression. For a longer or computed condition, evaluate it in AL (an `in [...]` list is fine there), store the result in a global page `Boolean` variable, and bind the property to that variable. Recompute the variable wherever its inputs change: `OnAfterGetCurrRecord` when the current record changes, and the `OnValidate` of each page field the condition reads for in-place edits. Do not use `OnAfterGetRecord` for action, group, or part state: it runs once per row loaded, so on a List or Worksheet page the variable ends up reflecting the last fetched row rather than the selected one (see [OnAfterGetCurrRecord is not per row](../performance/onaftergetcurrrecord-is-not-per-row.md)). `OnAfterGetRecord` is right only for per-row field state inside a repeater.
|
||||
|
||||
For `Visible` on field and action controls, the Visible property documentation requires the variable to be resolved in `OnInit` or `OnOpenPage`; do not rely on per-record recomputation to show and hide those controls. `Enabled` and `Editable` have no such restriction. See sample: [`page-client-expression-must-not-use-in-list.good.al`](page-client-expression-must-not-use-in-list.good.al).
|
||||
|
||||
|
|
@ -25,11 +25,14 @@ For `Visible` on field and action controls, the Visible property documentation r
|
|||
|
||||
A page or pageextension control property `Enabled`, `Visible`, `Editable`, or `StyleExpr` whose value contains `in [`, typically an enum or option field tested against several values. Reviewer signal: the `in [` token appears directly in the property value rather than inside a trigger or procedure body. Replacing it with a call to a procedure that performs the same test is the same defect in a different shape.
|
||||
|
||||
Do not flag plain comparisons joined with `and`/`or`, such as `Enabled = (Rec.Status = Rec.Status::New) or (Rec.Status = Rec.Status::"Needs Review");`; they compile cleanly and are common in the base application. Do not flag `in [...]` used inside procedures or triggers that assign a Boolean variable. See sample: [`page-client-expression-must-not-use-in-list.bad.al`](page-client-expression-must-not-use-in-list.bad.al).
|
||||
Do not flag plain comparisons joined with `and`/`or`, such as `Enabled = (Rec.Status = Rec.Status::New) or (Rec.Status = Rec.Status::"Needs Review");`; they compile cleanly and Microsoft uses them, for example in BCApps `AccountantExpenseReports.Page.al` and `AgentTaskLogEntry.Page.al`. Do not flag `in [...]` used inside procedures or triggers that assign a Boolean variable. See sample: [`page-client-expression-must-not-use-in-list.bad.al`](page-client-expression-must-not-use-in-list.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [Enabled property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-enabled-property): dynamic values are a Boolean variable, a Boolean field, or a Boolean expression such as "Credit Limit > Sales YTD"; variables must be global page variables.
|
||||
- [OnAfterGetCurrRecord (Page) trigger](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/triggers-auto/page/devenv-onaftergetcurrrecord-page-trigger): in a page with a repeater, called only when the current record is updated, after all `OnAfterGetRecord` calls for the rows.
|
||||
- [Visible property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-visible-property): variables for field and action controls must be resolved by `OnInit` or `OnOpenPage`.
|
||||
- [Compiler warning AL0573](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al573) and [compiler error AL0322](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al322). The Learn pages show only the `{0}` template; the compiler's message for this case is "InListExpression is not valid for client expressions. Client expressions can only use simple data types and field references." (AL compiler 30.0: AL0573 for action, group, and part properties; AL0322 for page field properties).
|
||||
- [Compiler warning AL0573](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al573) and [compiler error AL0322](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al322). The Learn pages show only the `{0}` template; the compiler's message for this case is "InListExpression is not valid for client expressions. Client expressions can only use simple data types and field references." (AL compiler 30.0: AL0573 for action, group, and part properties; AL0322 for page field properties, including `HideValue`).
|
||||
- Comparison-based client expressions in BCApps, for example `Enabled = Rec.Status <> Rec.Status::Running;` in [BCPTSetupCard.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Tools/Performance%20Toolkit/App/src/BCPTSetupCard.Page.al). BCApps contains no page client expression that uses an `in [...]` list.
|
||||
- Global Boolean recomputed on current-record change in BCApps: [EDocumentLogs.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/EDocument/App/src/Logging/EDocumentLogs.Page.al), a List page, binds `Enabled = IsExportEnabled;` and sets `IsExportEnabled` in `OnAfterGetCurrRecord`.
|
||||
- Or-chain client expressions in BCApps: `Enabled = (Rec.Status = Rec.Status::"Pending Approval") or (Rec.Status = Rec.Status::"Interim Approved");` in [AccountantExpenseReports.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/ExpenseAgent/app/src/Expense/Pages/AccountantExpenseReports.Page.al) and `Visible = (Rec.Type = Rec.Type::"Output Message Draft") or (Rec.Type = Rec.Type::"Output Message");` in [AgentTaskLogEntry.Page.al](https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/Agent/Troubleshooting/AgentTaskLogEntry.Page.al).
|
||||
|
|
|
|||
|
|
@ -4,20 +4,20 @@ pageextension 50710 "Sample Bus. Mgr. RC Ext" extends "Business Manager Role Cen
|
|||
{
|
||||
addafter(Control16)
|
||||
{
|
||||
part(SampleReportInbox; "Report Inbox Part")
|
||||
part(SampleMyCustomers; "My Customers")
|
||||
{
|
||||
ApplicationArea = Basic, Suite;
|
||||
// AL0573: procedure calls are not valid for client expressions.
|
||||
Visible = CanSeeReportInbox();
|
||||
Visible = CanSeeMyCustomers();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// AL0569: a page of type Role Center cannot have procedures.
|
||||
local procedure CanSeeReportInbox(): Boolean
|
||||
local procedure CanSeeMyCustomers(): Boolean
|
||||
var
|
||||
ReportInbox: Record "Report Inbox";
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
exit(ReportInbox.ReadPermission());
|
||||
exit(Customer.ReadPermission());
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -4,12 +4,12 @@ pageextension 50710 "Sample Bus. Mgr. RC Ext" extends "Business Manager Role Cen
|
|||
{
|
||||
addafter(Control16)
|
||||
{
|
||||
part(SampleReportInbox; "Report Inbox Part")
|
||||
part(SampleMyCustomers; "My Customers")
|
||||
{
|
||||
ApplicationArea = Basic, Suite;
|
||||
// Declarative permission gating: the part is removed for users
|
||||
// without Insert, Modify, or Delete permission on Report Inbox.
|
||||
AccessByPermission = TableData "Report Inbox" = IMD;
|
||||
// without Read permission on Customer.
|
||||
AccessByPermission = TableData Customer = R;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -11,13 +11,13 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
A page of type `RoleCenter` cannot have triggers (AL0378, error) or procedures (AL0569, warning that will become an error), and the AL compiler applies both rules to a pageextension whose target is a Role Center. That removes the usual way to show a control conditionally: compute a global Boolean in `OnOpenPage` and bind `Visible` to it. An easy-looking remediation of AL0378 is to delete the trigger and bind `Visible` or `Enabled` directly to a local procedure such as `CanSeeReportInbox()`. That still builds, but with two future errors: AL0573 for the procedure call in a client expression and AL0569 for the procedure itself. Neither message names the alternative.
|
||||
A page of type `RoleCenter` cannot have triggers (AL0378, error) or procedures (AL0569, warning that will become an error), and the AL compiler applies both rules to a pageextension whose target is a Role Center. That removes the usual way to show a control conditionally: compute a global Boolean in `OnOpenPage` and bind `Visible` to it. An easy-looking remediation of AL0378 is to delete the trigger and bind `Visible` or `Enabled` directly to a local procedure such as `CanSeeMyCustomers()`. That still builds, but with two future errors: AL0573 for the procedure call in a client expression and AL0569 for the procedure itself. Neither message names the alternative.
|
||||
|
||||
When the condition is "the user has permission to this object", the declarative alternative is the `AccessByPermission` property on the part, action, or field. It takes `TableData <table> = R|I|M|D` (any combination; having any one of the listed permissions is enough) or `X` for `Table`, `Page`, `Report`, `Codeunit`, `XmlPort`, or `Query`. Its applies-to list covers page fields, parts, system parts, chart parts, actions, and whole pages and reports; it does not include groups or cue groups. The element is removed for users without the permission, not disabled.
|
||||
When the condition is "the user has permission to this object", the declarative alternative is the `AccessByPermission` property on the part, action, or field. It takes `TableData <table> = R|I|M|D` (any combination; having any one of the listed permissions is enough) or `X` for `Table`, `Page`, `Report`, `Codeunit`, `XmlPort`, or `Query`. Its applies-to list covers page fields, parts, system parts, chart parts, actions, and whole pages and reports; it does not include groups or cue groups. The element is removed for users without the permission, not disabled. That per-user removal requires the UI Elements Removal setting `LicenseFileAndUserPermissions`; under `LicenseFile` removal follows the license, not the user's permission sets.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Set `AccessByPermission` on the Role Center part, action, or field that should appear only for users with the permission, naming the table or object that the part actually depends on. The base application does this on its own Role Centers, for example `AccessByPermission = TableData "Activities Cue" = I` on the activities part of the Business Manager Role Center and `TableData "Report Inbox" = IMD` on the Report Inbox part of the Accountant Role Center.
|
||||
Set `AccessByPermission` on the Role Center part, action, or field that should appear only for users with the permission, naming the table or object that the part actually depends on. The base application does this on its own Role Centers, for example `AccessByPermission = TableData "Activities Cue" = I` on the activities part of the Business Manager Role Center and `TableData "Report Inbox" = IMD` on the Report Inbox part (`Control96`) of the same Role Center and of the Accountant Role Center.
|
||||
|
||||
Two limits apply. The property takes effect only when the server's UI Elements Removal setting is `LicenseFile` or `LicenseFileAndUserPermissions`. It is UI removal, not a security boundary: the part's source data must still be protected by real permissions. When the condition is not a permission check, such as a setup value or a feature flag, `AccessByPermission` is the wrong tool. Put the condition inside the part page, which is a normal `CardPart` or `ListPart` that can have triggers. The part cannot remove itself from the Role Center, but it can hide or empty its own controls. See sample: [`rolecenter-permission-gating-must-use-accessbypermission.good.al`](rolecenter-permission-gating-must-use-accessbypermission.good.al).
|
||||
|
||||
|
|
@ -29,4 +29,4 @@ A `RoleCenter` page, or a pageextension whose target is a Role Center, binds `Vi
|
|||
|
||||
- [AccessByPermission property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-accessbypermission-property): applies-to list, permission values, any-one-of semantics, and the UI Elements Removal requirement ([Hide UI elements](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/administration/hide-ui-elements)).
|
||||
- [Compiler error AL0378](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al378), [compiler warning AL0569](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al569), and [compiler warning AL0573](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al573). AL compiler 30.0 reports all three on a pageextension of a Role Center, as it does on the Role Center page itself.
|
||||
- Base application usage: [BusinessManagerRoleCenter.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/RoleCenters/BusinessManagerRoleCenter.Page.al) and [AccountantRoleCenter.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/RoleCenters/AccountantRoleCenter.Page.al). No Role Center page in BCApps declares a trigger or procedure.
|
||||
- Base application usage: [BusinessManagerRoleCenter.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/RoleCenters/BusinessManagerRoleCenter.Page.al) (`Control96`, lines 124-127) and [AccountantRoleCenter.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/RoleCenters/AccountantRoleCenter.Page.al). No Role Center page in BCApps declares a trigger or procedure.
|
||||
|
|
|
|||
|
|
@ -42,7 +42,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
- **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`, and to JavaScript/CSS/HTML that implements a control add-in's rendering or Business Central communication. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files.
|
||||
- A new or changed page's name/suffix, primary-key handling, `CardPageID`, `SubPageLink`, `AutoSplitKey`, or `UsageCategory` doesn't match the conventions of its own declared `PageType` — `page-design-must-match-bc-page-type-conventions.md`. A Card page over a composite-key table that supplements a master record, or a supporting/subpage/dialog page intended only to be reached through another workflow and correctly omitting `UsageCategory`, is not this anti-pattern on its own; check whether the page is actually mixing conventions or is meant as a searchable entry point before flagging.
|
||||
- For each relevant knowledge file, compute overlap against changed page declarations and control add-in files, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, `ShowCaption`, `InstructionalText`, `GridLayout`, `Style`, `StyleExpr`, promoted action definitions, field importance, page background tasks, DOM creation, ARIA attributes, keyboard/focus handlers, packaged-resource AJAX, and calls from JavaScript into AL.
|
||||
- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`, `PageType = RoleCenter`, `Enabled`, `Visible`, `Editable`, `in [`, `AccessByPermission`).
|
||||
- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`, `PageType = RoleCenter`, `Enabled`, `Visible`, `Editable`, `in [`, `AccessByPermission`, `ReadPermission`, `WritePermission`).
|
||||
|
||||
A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed page element. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue