diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index 5f0e50a..abfa8e7 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -158,7 +158,9 @@ "ui": { "articles": [ "default-descending-sort-on-historical-pages", - "page-design-must-match-bc-page-type-conventions" + "page-design-must-match-bc-page-type-conventions", + "page-client-expression-must-not-use-in-list", + "rolecenter-permission-gating-must-use-accessbypermission" ] }, "upgrade": { diff --git a/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.bad.al b/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.bad.al new file mode 100644 index 0000000..cb537ca --- /dev/null +++ b/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.bad.al @@ -0,0 +1,59 @@ +enum 50700 "Sample Request Status" +{ + Extensible = false; + + value(0; New) { Caption = 'New'; } + value(1; "Needs Review") { Caption = 'Needs Review'; } + value(2; Approved) { Caption = 'Approved'; } +} + +table 50700 "Sample Request" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "No."; Code[20]) { } + field(2; Status; Enum "Sample Request Status") { } + } + + keys + { + key(PK; "No.") { Clustered = true; } + } +} + +page 50700 "Sample Request Card" +{ + PageType = Card; + SourceTable = "Sample Request"; + ApplicationArea = All; + + layout + { + area(Content) + { + field("No."; Rec."No.") { } + field(Status; Rec.Status) { } + } + } + + actions + { + area(Processing) + { + action(Approve) + { + Caption = 'Approve'; + // AL0573: InListExpression is not valid for client expressions. + Enabled = Rec.Status in [Rec.Status::New, Rec.Status::"Needs Review"]; + + trigger OnAction() + begin + Rec.Status := Rec.Status::Approved; + Rec.Modify(true); + end; + } + } + } +} diff --git a/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.good.al b/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.good.al new file mode 100644 index 0000000..0aaa24e --- /dev/null +++ b/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.good.al @@ -0,0 +1,83 @@ +enum 50700 "Sample Request Status" +{ + Extensible = false; + + value(0; New) { Caption = 'New'; } + value(1; "Needs Review") { Caption = 'Needs Review'; } + value(2; Approved) { Caption = 'Approved'; } +} + +table 50700 "Sample Request" +{ + DataClassification = CustomerContent; + + fields + { + field(1; "No."; Code[20]) { } + field(2; Status; Enum "Sample Request Status") { } + } + + keys + { + key(PK; "No.") { Clustered = true; } + } +} + +page 50700 "Sample Request Card" +{ + PageType = Card; + SourceTable = "Sample Request"; + ApplicationArea = All; + + layout + { + area(Content) + { + field("No."; Rec."No.") + { + // A plain field comparison is a valid client expression. + Editable = Rec.Status = Rec.Status::New; + } + field(Status; Rec.Status) + { + trigger OnValidate() + begin + UpdateActionStates(); + end; + } + } + } + + actions + { + area(Processing) + { + action(Approve) + { + Caption = 'Approve'; + // The list membership is computed in AL and exposed as a global Boolean. + Enabled = ApproveEnabled; + + trigger OnAction() + begin + Rec.Status := Rec.Status::Approved; + Rec.Modify(true); + UpdateActionStates(); + end; + } + } + } + + var + ApproveEnabled: Boolean; + + trigger OnAfterGetRecord() + begin + UpdateActionStates(); + end; + + local procedure UpdateActionStates() + begin + ApproveEnabled := Rec.Status in [Rec.Status::New, Rec.Status::"Needs Review"]; + end; +} diff --git a/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.md b/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.md new file mode 100644 index 0000000..b7b3c9f --- /dev/null +++ b/microsoft/knowledge/ui/page-client-expression-must-not-use-in-list.md @@ -0,0 +1,35 @@ +--- +bc-version: [all] +domain: ui +keywords: [client-expression, in-list, inlistexpression, al0573, al0322, enabled, visible, editable, dynamic-enable] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Page client-expression properties must not use an `in [...]` list + +## 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." + +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. + +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). + +## Anti Pattern + +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). + +## 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. +- [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). +- 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. diff --git a/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.bad.al b/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.bad.al new file mode 100644 index 0000000..204c810 --- /dev/null +++ b/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.bad.al @@ -0,0 +1,23 @@ +pageextension 50710 "Sample Bus. Mgr. RC Ext" extends "Business Manager Role Center" +{ + layout + { + addafter(Control16) + { + part(SampleReportInbox; "Report Inbox Part") + { + ApplicationArea = Basic, Suite; + // AL0573: procedure calls are not valid for client expressions. + Visible = CanSeeReportInbox(); + } + } + } + + // AL0569: a page of type Role Center cannot have procedures. + local procedure CanSeeReportInbox(): Boolean + var + ReportInbox: Record "Report Inbox"; + begin + exit(ReportInbox.ReadPermission()); + end; +} diff --git a/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.good.al b/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.good.al new file mode 100644 index 0000000..df241d1 --- /dev/null +++ b/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.good.al @@ -0,0 +1,16 @@ +pageextension 50710 "Sample Bus. Mgr. RC Ext" extends "Business Manager Role Center" +{ + layout + { + addafter(Control16) + { + part(SampleReportInbox; "Report Inbox Part") + { + 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; + } + } + } +} diff --git a/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.md b/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.md new file mode 100644 index 0000000..02715ae --- /dev/null +++ b/microsoft/knowledge/ui/rolecenter-permission-gating-must-use-accessbypermission.md @@ -0,0 +1,32 @@ +--- +bc-version: [all] +domain: ui +keywords: [accessbypermission, rolecenter, role-center, pageextension, client-expression, permission, al0569, al0573, al0378] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Gate Role Center content by permission with AccessByPermission + +## 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. + +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 = 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. + +## 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. + +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). + +## Anti Pattern + +A `RoleCenter` page, or a pageextension whose target is a Role Center, binds `Visible` or `Enabled` on a part, action, or field to a procedure call whose body checks `ReadPermission`, `WritePermission`, or a similar permission test, and declares that procedure. The compiler reports AL0573 and AL0569 as warnings, and both will become errors. Reviewer signal: a pageextension declares a procedure and AL0569 appears in its build output. The extended page's name is not reliable evidence of its type, so confirm the target is a Role Center from its `PageType` or from that diagnostic. Do not flag setup- or feature-based gating implemented inside the part page itself; that is the correct location for non-permission conditions. See sample: [`rolecenter-permission-gating-must-use-accessbypermission.bad.al`](rolecenter-permission-gating-must-use-accessbypermission.bad.al). + +## References + +- [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. diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index f54c0c9..b68dd0a 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -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`). +- 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`). 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. @@ -50,6 +50,8 @@ Apply these high-signal mappings before fuzzy topic ranking: - A tableextension adds a field to `DropDown` while the corresponding lookup-page control remains `Visible = false` — `dropdown-fieldgroup-respects-lookup-page-visibility`. - An editable page part affects a total, FlowField, or FactBox on the parent but does not set `UpdatePropagation = Both` — `updatepropagation-both-refreshes-main-page`. +- A page or pageextension `Enabled`, `Visible`, `Editable`, or `StyleExpr` value contains an `in [...]` list (compiler: "InListExpression is not valid for client expressions", AL0573 or AL0322), or replaces one with a procedure call — `page-client-expression-must-not-use-in-list`. Plain `=`/`<>` comparisons joined with `and`/`or`, and `in [...]` inside a trigger or procedure body, are valid; do not flag them. +- A `RoleCenter` page, or a pageextension whose target is a Role Center, gates a part, action, or field by binding `Visible` or `Enabled` to a procedure that tests a permission, or declares a procedure (AL0569 "A page of type Role Center cannot have procedures", AL0573) — `rolecenter-permission-gating-must-use-accessbypermission`. The target page name is not reliable evidence of its type; confirm it is a Role Center from its `PageType` or the AL0569 diagnostic. Setup- or feature-flag gating inside the part page is not this pattern. Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.