mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 22:56:55 +01:00
Address review: scope shared notification Ids, routed-table definition, pinned links
- notification-recall-needs-known-id: a fixed Id shared across records is valid for one-at-a-time warnings (Sales Line blocked-item, Over-Receipt Mgt. pass a fixed Id to SendNotification after Recall); the per-record anti-pattern now requires several records' notifications to be visible at once. Recall wording follows Learn (including its false-return reasons) and cites Base App's recall-before-send practice. - list-page-document-routing-uses-page-management: definition covers opening a routed table directly or after Get; cite archive line lists, Copy Document Mgt. ShowSalesDoc/ShowPurchDoc and Office handler; no "document types added later" overclaim (GetSalesHeaderPageID has no else for the extensible enum); full GetPageID resolution order; drop the IRS single-page subscriber. - al-ui-review: non-page files admitted by the notification clause are checked only against notification knowledge; description updated; drop the "Document Type" token; case-insensitive token matching; cues aligned with both articles. - Pin BCApps links to 837ef802485ee457e52310d2ecaa08b93d0122fd. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
750466c7f3
commit
5c15b08eeb
3 changed files with 29 additions and 24 deletions
|
|
@ -3,7 +3,7 @@ kind: action-skill
|
|||
id: al-ui-review
|
||||
version: 1
|
||||
title: AL UI and accessibility review
|
||||
description: Reviews AL page and control add-in UI files against UI text, caption, tooltip, and accessibility guidance from BCQuality.
|
||||
description: Reviews AL page and control add-in UI files, and AL code that sends or recalls notifications, against UI text, caption, tooltip, notification, and accessibility guidance from BCQuality.
|
||||
inputs: [pr-diff, file-path, folder-path]
|
||||
outputs: [findings-report]
|
||||
bc-version: [all]
|
||||
|
|
@ -39,10 +39,10 @@ Discard files that are not applicable. Retain conditionally applicable files onl
|
|||
|
||||
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`; to any AL object whose changed code calls `Send()` or `Recall()` on a `Notification` variable or calls codeunit `"Notification Lifecycle Mgt."`; 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.
|
||||
- **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`; to any AL object whose changed code calls `Send()` or `Recall()` on a `Notification` variable or calls codeunit `"Notification Lifecycle Mgt."`; and to JavaScript/CSS/HTML that implements a control add-in's rendering or Business Central communication. A non-page AL file admitted only by the notification clause is evaluated only against notification knowledge (`notification-recall-needs-known-id`), not against caption, tooltip, message-text, or client-expression knowledge. 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, notification send/recall, open-document actions, 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`, `ReadPermission`, `WritePermission`, `Notification`, `NotificationScope`, `.Send()`, `.Recall()`, `CreateGuid`, `Notification Lifecycle Mgt.`, `SendNotification`, `SendNotificationWithAdditionalContext`, `RecallNotificationsForRecord`, `Page Management`, `PageRun`, `Page.Run(`, `"Document Type"`, `ShowDocument`).
|
||||
- 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`, `Notification`, `NotificationScope`, `.Send()`, `.Recall()`, `CreateGuid`, `Notification Lifecycle Mgt.`, `SendNotification`, `SendNotificationWithAdditionalContext`, `RecallNotificationsForRecord`, `Page Management`, `PageRun`, `Page.Run(`, `ShowDocument`). Match tokens case-insensitively; Base App writes both `Page.Run(` and `PAGE.Run(`.
|
||||
|
||||
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.
|
||||
|
||||
|
|
@ -52,8 +52,8 @@ Apply these high-signal mappings before fuzzy topic ranking:
|
|||
- 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.
|
||||
- The same code path calls `Send()` and `Recall()` on a `Notification` (for example `Send()` when a condition holds, `Recall()` in the `else` branch or when it clears) but never assigns its `Id`, or assigns `CreateGuid()` and calls `Send()`/`Recall()` directly; or one fixed `Id` is sent and recalled directly for notifications that belong to different records — `notification-recall-needs-known-id`. A notification that is never recalled, and one sent through `"Notification Lifecycle Mgt."` (`SendNotification`, `SendNotificationWithAdditionalContext`) without an `Id`, are valid; do not flag them.
|
||||
- An action trigger switches on `"Document Type"` (or a similar type field) of `Sales Header`, `Purchase Header`, or another table that codeunit `"Page Management"` routes, and calls `Page.Run(Page::...)` per branch — `list-page-document-routing-uses-page-management`. Severity `minor`. Opening one known document type directly, a list with a fixed `CardPageID`, and a table that `"Page Management"` does not route (for example `Assembly Header`) are not this pattern.
|
||||
- The same code path calls `Send()` and `Recall()` on a `Notification` (for example `Send()` when a condition holds, `Recall()` in the `else` branch or when it clears) but never assigns its `Id`, or assigns `CreateGuid()` and calls `Send()`/`Recall()` directly; or, when notifications for several records must be visible at the same time, they share one fixed `Id` sent and recalled directly — `notification-recall-needs-known-id`. A notification that is never recalled; one sent through `"Notification Lifecycle Mgt."` (`SendNotification`, `SendNotificationWithAdditionalContext`) without an `Id`; and a fixed `Id` shared across records for a one-at-a-time warning that is recalled before re-send (directly or through `SendNotification`, as in `Sales Line`'s blocked-item notification) are valid; do not flag them.
|
||||
- A page action opens a record of `Sales Header`, `Purchase Header`, their archives, or another table that codeunit `"Page Management"` routes (directly or after a `Get`) by switching on its `Document Type` or a similar type field and calling `Page.Run(Page::...)` per branch — `list-page-document-routing-uses-page-management`. Severity `minor`. Opening one known document type directly, a list with a fixed `CardPageID`, and a table that `"Page Management"` does not route (for example `Assembly Header`) are not this pattern.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue