From 5c15b08eebc3695940e129ed17df5b3270665d64 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Sat, 3 Oct 2026 12:59:46 +0200 Subject: [PATCH] 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 --- ...e-document-routing-uses-page-management.md | 17 ++++++------ .../ui/notification-recall-needs-known-id.md | 26 +++++++++++-------- microsoft/skills/review/al-ui-review.md | 10 +++---- 3 files changed, 29 insertions(+), 24 deletions(-) diff --git a/microsoft/knowledge/ui/list-page-document-routing-uses-page-management.md b/microsoft/knowledge/ui/list-page-document-routing-uses-page-management.md index 3f67a32..3e16a2c 100644 --- a/microsoft/knowledge/ui/list-page-document-routing-uses-page-management.md +++ b/microsoft/knowledge/ui/list-page-document-routing-uses-page-management.md @@ -11,19 +11,19 @@ application-area: [all] ## Description -Some tables back several document pages, chosen by a type field. `Sales Header` rows open as Sales Quote, Sales Order, Sales Invoice, Sales Credit Memo, Blanket Sales Order, or Sales Return Order, depending on `Document Type`. A list over such a table cannot use one `CardPageID`. Codeunit 700 `"Page Management"` already holds that mapping. `PageRun(Rec)` resolves the page through `GetPageID`. That calls `GetConditionalCardPageID`, which handles `Sales Header`, `Purchase Header`, their archives, general and item journal batches and lines, requisition worksheets, and several other tables. When no conditional page applies, it falls back to the default card or lookup page. Base App's own lists call it: the `Show Document` actions on `Sales List` and `Purchase List`, `Sales Lines` (after getting the header), and `Navigate` for posted documents. +Some tables back several document pages, chosen by a type field. `Sales Header` rows open as Sales Quote, Sales Order, Sales Invoice, Sales Credit Memo, Blanket Sales Order, or Sales Return Order, depending on `Document Type`. A list over such a table cannot use one `CardPageID`. Codeunit 700 `"Page Management"` already holds that mapping. `PageRun(Rec)` resolves the page through `GetPageID`, in this order: `GetConditionalCardPageID`, then the default card page (only for an existing record), then `GetConditionalListPageID`, then the table's lookup page. `GetConditionalCardPageID` handles `Sales Header`, `Purchase Header`, their archives, general and item journal batches and lines, requisition worksheets, and several other tables. Base App's own lists call it: the `Show Document` actions on `Sales List` and `Purchase List`, `Sales Lines` (after getting the header), and `Navigate` for posted documents. -A hand-written `case Rec."Document Type" of ... Page.Run(Page::"Sales Order", Rec)` copies that mapping into a single action. The copy misses document types added later. It also bypasses routing that other extensions add through Page Management's events (`OnBeforeGetConditionalCardPageID`, `OnAfterGetPageID`, `OnPageRunAtFieldOnBeforeRunPage`). +A hand-written `case Rec."Document Type" of ... Page.Run(Page::"Sales Order", Rec)` copies that mapping into a single action. It misses mappings that Microsoft later adds to Page Management, and routing that extensions add through its events (`OnBeforeGetConditionalCardPageID`, `OnAfterGetPageID`, `OnPageRunAtFieldOnBeforeRunPage`). Page Management does not route new values of an extended `Sales Document Type` enum by itself either, so those still need a subscriber. ## Best Practice In the list's open-document action, call `PageManagement.PageRun(Rec)`, or `PageRunModal` or `PageRunList` as needed. `PageRun` returns `false` without opening anything when `GuiAllowed` is false or no page resolves. See sample: [`list-page-document-routing-uses-page-management.good.al`](list-page-document-routing-uses-page-management.good.al). -For a new table whose rows map to different pages, register the mapping once and then use `PageRun` everywhere. Subscribe to `OnConditionalCardPageIDNotFound`, which is raised only for tables the codeunit does not route itself. Microsoft's IRS Forms and Sustainability apps register their tables this way. `OnBeforeGetConditionalCardPageID` is the `IsHandled` alternative, used by the Quality Management app. +For a new table whose rows map to different pages, register the mapping once and then use `PageRun` everywhere. Subscribe to `OnConditionalCardPageIDNotFound`, which is raised only for tables the codeunit does not route itself. Microsoft's Sustainability app routes its journal batch and line tables this way. `OnBeforeGetConditionalCardPageID` is the `IsHandled` alternative, used by the Quality Management app. ## Anti Pattern -An action trigger that switches on `Document Type`, or a similar type field, of a table that Page Management already routes, and calls `Page.Run(Page::..., Rec)` in each branch. Base App still has a few of these, for example `Sales Line Archive List`. The result is a duplicated mapping, not a runtime error, so report it as minor. See sample: [`list-page-document-routing-uses-page-management.bad.al`](list-page-document-routing-uses-page-management.bad.al). +A page action that opens a record of a table Page Management routes, directly or after a `Get`, by switching on its `Document Type` (or a similar type field) and calling `Page.Run(Page::...)` in each branch. Base App still does this in several places, for example the `Show Document` actions of `Sales Line Archive List` and `Purchase Line Archive List`, and `Copy Document Mgt.` `ShowSalesDoc`/`ShowPurchDoc`. The result is a duplicated mapping, not a runtime error, so report it as minor. The same switch in a codeunit helper is outside this review's page scope. See sample: [`list-page-document-routing-uses-page-management.bad.al`](list-page-document-routing-uses-page-management.bad.al). Not this pattern: @@ -32,7 +32,8 @@ Not this pattern: ## References -- [PageManagement.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Utilities/PageManagement.Codeunit.al): `PageRun` (lines 44-47), `PageRunAtField` with the `GuiAllowed` exit (75-100), `GetPageID` fallback order (112-138), `GetConditionalCardPageID` (194-263; unrouted tables raise `OnConditionalCardPageIDNotFound` at 258), `GetSalesHeaderPageID` (289-315), integration events (671-719). -- Callers: [SalesList.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Sales/Document/SalesList.Page.al) (`ShowDocument`, lines 188-202; no `CardPageID`), [PurchaseList.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Purchases/Document/PurchaseList.Page.al) (line 200), [SalesLines.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Sales/Document/SalesLines.Page.al) (lines 224-230), [Navigate.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Foundation/Navigate/Navigate.Page.al) (from line 1564). -- Subscribers: [IRS1099BaseAppSubscribers.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Apps/US/IRSForms/app/src/Extensions/IRS1099BaseAppSubscribers.Codeunit.al) (lines 149-156), [SustWorkflowEventHandling.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/Sustainability/app/src/Workflow/SustWorkflowEventHandling.Codeunit.al) (lines 184-193), [QltyUtilitiesIntegration.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/Quality%20Management/app/src/Integration/Utilities/QltyUtilitiesIntegration.Codeunit.al) (lines 20-28). -- Counterexamples: [SalesLineArchiveList.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Sales/Archive/SalesLineArchiveList.Page.al) (lines 106-121), [AssemblyList.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Assembly/Document/AssemblyList.Page.al) (lines 111-121), [Opportunity.Table.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/CRM/Opportunity/Opportunity.Table.al) (lines 1221-1223), [SalesOrderList.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Sales/Document/SalesOrderList.Page.al) (line 44). +- [PageManagement.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Utilities/PageManagement.Codeunit.al): `PageRun` (lines 44-47), `PageRunAtField` with the `GuiAllowed` exit (75-100), `GetPageID` resolution order (112-138, order at 121-133), `GetConditionalCardPageID` (194-263; unrouted tables raise `OnConditionalCardPageIDNotFound` at 258), `GetSalesHeaderPageID` with no `else` branch (289-315), integration events (671-719). [SalesDocumentType.Enum.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Document/SalesDocumentType.Enum.al) is `Extensible` (line 12). +- Callers: [SalesList.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Document/SalesList.Page.al) (`ShowDocument`, lines 188-202; no `CardPageID`), [PurchaseList.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Purchases/Document/PurchaseList.Page.al) (line 200), [SalesLines.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Document/SalesLines.Page.al) (lines 224-230), [Navigate.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Foundation/Navigate/Navigate.Page.al) (from line 1564). +- Subscribers: [SustWorkflowEventHandling.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Apps/W1/Sustainability/app/src/Workflow/SustWorkflowEventHandling.Codeunit.al) (lines 184-193), [QltyUtilitiesIntegration.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Apps/W1/Quality%20Management/app/src/Integration/Utilities/QltyUtilitiesIntegration.Codeunit.al) (lines 20-28). +- Remaining hand-rolled routing of routed tables: [SalesLineArchiveList.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Archive/SalesLineArchiveList.Page.al) (lines 106-121), [PurchaseLineArchiveList.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Purchases/Archive/PurchaseLineArchiveList.Page.al) (from line 109), [CopyDocumentMgt.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Utilities/CopyDocumentMgt.Codeunit.al) (lines 1369-1409), [OfficeDocumentHandler.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/CRM/Outlook/OfficeDocumentHandler.Codeunit.al) (lines 342-348). +- Not this pattern: [AssemblyList.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Assembly/Document/AssemblyList.Page.al) (lines 111-121), [Opportunity.Table.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/CRM/Opportunity/Opportunity.Table.al) (lines 1221-1223), [SalesOrderList.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Document/SalesOrderList.Page.al) (line 44). diff --git a/microsoft/knowledge/ui/notification-recall-needs-known-id.md b/microsoft/knowledge/ui/notification-recall-needs-known-id.md index 2994e55..eaf13d7 100644 --- a/microsoft/knowledge/ui/notification-recall-needs-known-id.md +++ b/microsoft/knowledge/ui/notification-recall-needs-known-id.md @@ -11,29 +11,33 @@ application-area: [all] ## Description -`Notification.Recall()` withdraws the notification whose `Id` it carries. When `Id` is left unassigned, `Send()` assigns one. A later `Recall()` on a new `Notification` variable has no way to name that Id, so a warning sent that way cannot be withdrawn when its condition clears. It stays until the user dismisses it or the page instance closes. Microsoft Learn's own `Id`/`Recall` example uses a predefined Id "so that the notification can be recalled". `Recall()` does not fail on a notification that was never sent or was already recalled, so code with a known Id can recall unconditionally. +`Notification.Recall()` withdraws the notification whose `Id` it carries. When `Id` is left unassigned, `Send()` assigns one. A later `Recall()` on a new `Notification` variable has no way to name that Id, so a warning sent that way cannot be withdrawn when its condition clears. It stays until the user dismisses it or the page instance closes. Microsoft Learn's own `Id`/`Recall` example uses a predefined Id "so that the notification can be recalled", and the Recall page states that a notification "can be recalled successfully even if it hasn't been sent". Base App relies on that: it assigns a fixed Id and calls `Recall()` before every send. -Which Id is right depends on how many instances can be shown at once: +Which Id is right depends on how many instances must be visible at the same time: -- **At most one at a time** (one condition per page or task): a fixed GUID, returned from a procedure or assigned as a literal. Base App's `Analysis View.ShowResetNeededNotification` assigns a literal Id, calls `Recall()`, then sets the message and calls `Send()`. Learn does not document what `Send()` does when a notification with the same Id is already displayed, so recall first rather than relying on `Send()` to replace it. -- **One per record** (for example one warning per document line): a single fixed Id cannot tell the records apart. Use codeunit 1511 `"Notification Lifecycle Mgt."`. `SendNotification(Notification, RecId)` assigns `CreateGuid()` when `Id` is null, sends, and stores the Id against the `RecordId` in the temporary table `"Notification Context"`. `RecallNotificationsForRecord(RecId, HandleDelayedInsert)` recalls every tracked notification for that record. When one record can carry several independent warnings, pass a fixed GUID per reason to `SendNotificationWithAdditionalContext` and `RecallNotificationsForRecordWithAdditionalContext`. `Item-Check Avail.` does this: a `CreateGuid()` Id per notification, its fixed availability GUID as the additional context. +- **One at a time**, even when the warning is about different records: a fixed GUID, returned from a procedure or assigned as a literal, recalled before each send. `Analysis View.ShowResetNeededNotification` does this with plain `Send()`. `Sales Line.SendBlockedItemNotification` does it for line records, passing the fixed Id to `"Notification Lifecycle Mgt.".SendNotification`, which keeps an Id that is already set. Showing only the latest line's warning is a deliberate design choice there, not a defect. Learn does not document what `Send()` does when a notification with the same Id is already displayed, so recall first rather than relying on `Send()` to replace it. +- **Several records' notifications visible at the same time** (for example one availability warning per document line): one fixed Id cannot tell them apart. Use codeunit 1511 `"Notification Lifecycle Mgt."`. `SendNotification(Notification, RecId)` assigns `CreateGuid()` when `Id` is null, sends, and stores the Id against the `RecordId` in the temporary table `"Notification Context"`. `RecallNotificationsForRecord(RecId, HandleDelayedInsert)` recalls every tracked notification for that record. When one record can carry several independent warnings, pass a fixed GUID per reason to `SendNotificationWithAdditionalContext` and `RecallNotificationsForRecordWithAdditionalContext`. `Item-Check Avail.` does this: a `CreateGuid()` Id per notification, its fixed availability GUID as the additional context. ## Best Practice -Assign a fixed `Id` to any single-instance notification that the same code path can also withdraw, and recall it with that Id before re-sending updated content. See sample: [`notification-recall-needs-known-id.good.al`](notification-recall-needs-known-id.good.al). +Assign a fixed `Id` to any notification that the same code can also withdraw while only one instance needs to be visible, and recall it with that Id before re-sending updated content. See sample: [`notification-recall-needs-known-id.good.al`](notification-recall-needs-known-id.good.al). -For per-record notifications, send and recall through `"Notification Lifecycle Mgt."` instead of calling `Send()`/`Recall()` directly. The codeunit is `SingleInstance`, so tracking lasts for the session. While a record does not exist yet, its notification is stored under the table's empty `RecordId`. Pass `HandleDelayedInsert = true` when recalling for a record that may not be inserted yet, and `false` when recalling after the record is deleted, as Base App's own delete subscribers do. Base App's `"Notification Lifecycle Handler"` (codeunit 1508) moves tracked notifications on insert and rename, and recalls them on delete, only for the Base App tables it subscribes to, such as `Sales Line`. For another table, call `SetRecordID`, `UpdateRecordID`, and `RecallNotificationsForRecord` from that table's own insert, rename, and delete paths. +When notifications for several records must be visible together, send and recall through `"Notification Lifecycle Mgt."` instead of calling `Send()`/`Recall()` directly. The codeunit is `SingleInstance`, so tracking lasts for the session. While a record does not exist yet, its notification is stored under the table's empty `RecordId`. Pass `HandleDelayedInsert = true` when recalling for a record that may not be inserted yet, and `false` when recalling after the record is deleted, as Base App's own delete subscribers do. Base App's `"Notification Lifecycle Handler"` (codeunit 1508) moves tracked notifications on insert and rename, and recalls them on delete, only for the Base App tables it subscribes to, such as `Sales Line`. For another table, call `SetRecordID`, `UpdateRecordID`, and `RecallNotificationsForRecord` from that table's own insert, rename, and delete paths. ## Anti Pattern -Code that both sends and recalls a notification, for example `Send()` when a condition holds and `Recall()` in the `else` branch or when the condition clears, but never assigns `Id`, or assigns a fresh `CreateGuid()` and calls `Send()`/`Recall()` directly. The `Recall()` cannot reach the notification that was sent. A single fixed Id shared by notifications for several records, sent and recalled directly, is the per-record form of the same mistake. See sample: [`notification-recall-needs-known-id.bad.al`](notification-recall-needs-known-id.bad.al). +Code that both sends and recalls a notification, for example `Send()` when a condition holds and `Recall()` in the `else` branch or when the condition clears, but never assigns `Id`, or assigns a fresh `CreateGuid()` and calls `Send()`/`Recall()` directly. The `Recall()` cannot reach the notification that was sent. The per-record form: notifications for several records must be visible at the same time, but they share one fixed Id sent and recalled directly, so recalling one record's warning cannot leave the others in place. See sample: [`notification-recall-needs-known-id.bad.al`](notification-recall-needs-known-id.bad.al). -Not this pattern: a one-off informational notification that the code never recalls, which Learn's own Sales Order example sends without an Id; and a notification sent through `"Notification Lifecycle Mgt."` without an Id, because the codeunit assigns and tracks one. +Not this pattern: + +- A one-off informational notification that the code never recalls. Learn's own Sales Order example sends without an Id. +- A notification sent through `"Notification Lifecycle Mgt."` without an Id, because the codeunit assigns and tracks one. +- A fixed Id shared across records when one warning at a time is intended, recalled before re-send, with or without `SendNotification`, as in `Sales Line`'s blocked-item notification or `Over-Receipt Mgt.`. ## References - [Notification.Id method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/notification/notification-id-method): an unassigned Id is assigned at `Send()`; the example sets a predefined Id so the notification can be recalled. -- [Notification.Recall method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/notification/notification-recall-method): recalling more than once, or before sending, does not fail. +- [Notification.Recall method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/notification/notification-recall-method): a notification can be recalled more than once, and before it is sent. The same page lists client communication failure, or recalling a notification with no instance, as reasons `Recall()` can return `false`, and an uncaptured failure is a runtime error. Base App's unconditional recall-before-send (Analysis View, Sales Line) shows that recalling a fixed Id with nothing on screen is safe in practice. - [Using nonintrusive notifications](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-notifications-developing): notifications remain for the page instance or until dismissed. -- [NotificationLifecycleMgt.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Modules/System/Notifications/NotificationLifecycleMgt.Codeunit.al): `SendNotification` and `SendNotificationWithAdditionalContext` (lines 17-36), `RecallNotificationsForRecord` (38-44), `GetUsableRecordId` (177-191). [NotificationLifecycleHandler.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/System/Notifications/NotificationLifecycleHandler.Codeunit.al): `Sales Line` insert, rename, and delete subscribers (lines 27-52). -- Base App usage: [AnalysisView.Table.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/Analysis/AnalysisView.Table.al) (`ShowResetNeededNotification`, lines 1039-1051) and [ItemCheckAvail.Codeunit.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Inventory/Availability/ItemCheckAvail.Codeunit.al) (recall at lines 89-90, `CreateGuid()` Id and send at 636-646). +- [NotificationLifecycleMgt.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Modules/System/Notifications/NotificationLifecycleMgt.Codeunit.al): `SendNotification` and `SendNotificationWithAdditionalContext` (lines 17-36), `RecallNotificationsForRecord` (38-44), `GetUsableRecordId` (177-191). [NotificationLifecycleHandler.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/System/Notifications/NotificationLifecycleHandler.Codeunit.al): `Sales Line` insert, rename, and delete subscribers (lines 27-52). +- Base App usage: [AnalysisView.Table.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Finance/Analysis/AnalysisView.Table.al) (`ShowResetNeededNotification`, lines 1039-1051), [SalesLine.Table.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Sales/Document/SalesLine.Table.al) (`SendBlockedItemNotification`, lines 10126-10135), [OverReceiptMgt.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Purchases/Document/OverReceiptMgt.Codeunit.al) (lines 212-228), and [ItemCheckAvail.Codeunit.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Inventory/Availability/ItemCheckAvail.Codeunit.al) (recall at lines 89-90, `CreateGuid()` Id and send at 636-646). diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index d6ab93c..0d6bf3b 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -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.