mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-07 23:56:56 +01:00
Address review: notification recall defect is a lost identity, not an unassigned or generated Id
- notification-recall-needs-known-id: the defect is a Recall() whose Id cannot be the sent one (fresh local Notification, new CreateGuid at recall time) while neither the sent instance nor its Id is kept. A retained global instance (CreateGuid once in OnOpenPage, or Id left for Send to assign), a generated Id saved after Send and reassigned before Recall, and correctly tracked per-record Ids are explicitly not findings. Findings require evidence that the recalled identity differs from or cannot recover the sent identity. Notification Lifecycle Mgt. is recommended for per-record tracking, not mandatory. Cites VAT Bus. Post. Grp. Part, Certificate, and Data Search Lines, and the lifecycle helper's Send-then-read-Id sequence. - good sample: adds a page with a retained global Notification (CreateGuid in OnOpenPage, Send in an action, Recall in a later action and OnClosePage) and a pageextension that saves the Send-assigned Id and recalls it from a later action. Bad sample comments name the lost identity. - al-ui-review: notification cue requires that evidence and lists the retained-instance, saved-Id, and direct per-record tracking controls as exclusions. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer
parent
a37f45ada4
commit
cff8917ecb
4 changed files with 144 additions and 12 deletions
|
|
@ -5,13 +5,14 @@ pageextension 50720 "Sample Customer Card Ext" extends "Customer Card"
|
|||
NoCreditLimitNotification: Notification;
|
||||
begin
|
||||
if Rec."Credit Limit (LCY)" = 0 then begin
|
||||
// No Id is assigned: Send assigns one that this code never keeps.
|
||||
// Send assigns an Id, but this local variable is discarded and
|
||||
// the Id is not saved anywhere.
|
||||
NoCreditLimitNotification.Message := NoCreditLimitMsg;
|
||||
NoCreditLimitNotification.Scope := NotificationScope::LocalScope;
|
||||
NoCreditLimitNotification.Send();
|
||||
end else
|
||||
// This new variable has no Id, so the warning sent for the
|
||||
// previous customer is not withdrawn.
|
||||
// A fresh local variable with no Id cannot be the notification
|
||||
// sent for the previous customer, so that warning is not withdrawn.
|
||||
NoCreditLimitNotification.Recall();
|
||||
end;
|
||||
|
||||
|
|
|
|||
|
|
@ -22,3 +22,124 @@ pageextension 50720 "Sample Customer Card Ext" extends "Customer Card"
|
|||
var
|
||||
NoCreditLimitMsg: Label 'This customer has no credit limit.';
|
||||
}
|
||||
|
||||
page 50721 "Sample Credit Review"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = Customer;
|
||||
Caption = 'Credit Review';
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
field("No."; Rec."No.")
|
||||
{
|
||||
ApplicationArea = All;
|
||||
ToolTip = 'Specifies the number of the customer.';
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
actions
|
||||
{
|
||||
area(Processing)
|
||||
{
|
||||
action(FlagForReview)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Flag for Review';
|
||||
ToolTip = 'Shows a reminder that this customer needs a credit review.';
|
||||
|
||||
trigger OnAction()
|
||||
begin
|
||||
if ReviewNotification.Recall() then;
|
||||
ReviewNotification.Message := ReviewNeededMsg;
|
||||
ReviewNotification.Scope := NotificationScope::LocalScope;
|
||||
ReviewNotification.Send();
|
||||
end;
|
||||
}
|
||||
action(ClearReviewFlag)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Clear Review Flag';
|
||||
ToolTip = 'Removes the credit review reminder.';
|
||||
|
||||
trigger OnAction()
|
||||
begin
|
||||
// The same global instance that was sent carries its Id here.
|
||||
if ReviewNotification.Recall() then;
|
||||
end;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
trigger OnOpenPage()
|
||||
begin
|
||||
// A generated Id is fine: it is assigned once and kept with the instance.
|
||||
ReviewNotification.Id := CreateGuid();
|
||||
end;
|
||||
|
||||
trigger OnClosePage()
|
||||
begin
|
||||
if ReviewNotification.Recall() then;
|
||||
end;
|
||||
|
||||
var
|
||||
ReviewNotification: Notification;
|
||||
ReviewNeededMsg: Label 'This customer needs a credit review.';
|
||||
}
|
||||
|
||||
pageextension 50722 "Sample Customer List Ext" extends "Customer List"
|
||||
{
|
||||
actions
|
||||
{
|
||||
addlast(Processing)
|
||||
{
|
||||
action(SampleShowStatementReminder)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Show Statement Reminder';
|
||||
ToolTip = 'Shows a reminder to send statements to the selected customers.';
|
||||
|
||||
trigger OnAction()
|
||||
var
|
||||
ReminderNotification: Notification;
|
||||
begin
|
||||
RecallStatementReminder();
|
||||
ReminderNotification.Message := StatementReminderMsg;
|
||||
ReminderNotification.Scope := NotificationScope::LocalScope;
|
||||
ReminderNotification.Send();
|
||||
// Send assigned the Id; saving it lets a later action recall it.
|
||||
LastReminderNotificationId := ReminderNotification.Id;
|
||||
end;
|
||||
}
|
||||
action(SampleDismissStatementReminder)
|
||||
{
|
||||
ApplicationArea = All;
|
||||
Caption = 'Dismiss Statement Reminder';
|
||||
ToolTip = 'Removes the statement reminder.';
|
||||
|
||||
trigger OnAction()
|
||||
begin
|
||||
RecallStatementReminder();
|
||||
end;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
local procedure RecallStatementReminder()
|
||||
var
|
||||
ReminderNotification: Notification;
|
||||
begin
|
||||
if IsNullGuid(LastReminderNotificationId) then
|
||||
exit;
|
||||
ReminderNotification.Id := LastReminderNotificationId;
|
||||
if ReminderNotification.Recall() then;
|
||||
Clear(LastReminderNotificationId);
|
||||
end;
|
||||
|
||||
var
|
||||
LastReminderNotificationId: Guid;
|
||||
StatementReminderMsg: Label 'Remember to send statements to the selected customers.';
|
||||
}
|
||||
|
|
|
|||
|
|
@ -11,26 +11,35 @@ 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", 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.
|
||||
`Notification.Recall()` withdraws the notification whose `Id` it carries. When `Id` is left unassigned, `Send()` assigns one. What matters is whether the `Notification` that `Recall()` runs on carries the same Id as the one that was sent, not whether that Id is a literal, a `CreateGuid()` value, or assigned by `Send()`. 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".
|
||||
|
||||
Which Id is right depends on how many instances must be visible at the same time:
|
||||
Code can keep the sent identity in any of these ways:
|
||||
|
||||
- **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.
|
||||
- **A fixed Id**, 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.
|
||||
- **A retained instance**: a global `Notification` variable on the page or codeunit that is sent and later recalled. `VAT Bus. Post. Grp. Part` assigns `Format(CreateGuid())` once in `OnOpenPage` and recalls the same global instance before re-sending and in `HideNotification`. The `Certificate` page never assigns an Id; it sends its global `PasswordNotification` and `ExpiredNotification` and later recalls the same instances.
|
||||
- **A saved generated Id**: `Data Search Lines` sends a local notification without an Id, saves `ChangedSetupNotification.Id` (assigned by `Send()`) in a global Guid, and later assigns that saved Id to a new local `Notification` to recall it.
|
||||
- **Tracked per record** through codeunit 1511 `"Notification Lifecycle Mgt."`. `SendNotification(Notification, RecId)` assigns `CreateGuid()` when `Id` is null, sends, and then reads `NotificationToSend.Id` to store it 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.
|
||||
|
||||
The defect is a `Recall()` whose Id cannot be the sent one: a fresh local `Notification` with no Id, or a new `CreateGuid()` assigned at recall time, while the sent instance and its Id are not kept anywhere. Such a warning cannot be withdrawn when its condition clears. It stays until the user dismisses it or the page instance closes.
|
||||
|
||||
## Best Practice
|
||||
|
||||
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).
|
||||
Keep the identity of every notification the code will recall, and recall with that identity: a fixed Id recalled before re-sending updated content, a retained global `Notification` instance, or a generated Id (from `CreateGuid()` or assigned by `Send()`) saved after `Send()` and assigned again before `Recall()`. See sample: [`notification-recall-needs-known-id.good.al`](notification-recall-needs-known-id.good.al).
|
||||
|
||||
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.
|
||||
When notifications for several records must be visible together, `"Notification Lifecycle Mgt."` is the recommended way to track them, though correct direct tracking of per-record Ids is equally valid. 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. 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).
|
||||
Code that both sends and recalls a notification where the `Notification` passed to `Recall()` cannot carry the sent Id: for example `Send()` on a local variable when a condition holds and `Recall()` on a fresh local variable when it clears, with no Id assigned, or with a new `CreateGuid()` assigned on each call, and neither the sent instance nor its Id kept in a global variable, a record, or `"Notification Lifecycle Mgt."`. 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).
|
||||
|
||||
Flag this only with evidence that the recalled identity differs from, or cannot recover, the sent identity: trace the variable passed to `Recall()` back to its Id assignment, and the sent variable forward to where its instance or Id is kept.
|
||||
|
||||
Not this pattern:
|
||||
|
||||
- A one-off informational notification that the code never recalls. Learn's own Sales Order example sends without an Id.
|
||||
- A global or page-level `Notification` instance that is sent and later recalled, whether its Id was assigned by `CreateGuid()` (once, for example in `OnOpenPage`) or left for `Send()` to assign.
|
||||
- A generated Id, from `CreateGuid()` or assigned by `Send()`, saved after `Send()` and assigned to the `Notification` used for `Recall()`.
|
||||
- Per-record Ids tracked correctly by the code itself, without `"Notification Lifecycle Mgt."`.
|
||||
- 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.`.
|
||||
|
||||
|
|
@ -39,5 +48,6 @@ Not this pattern:
|
|||
- [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): 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/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).
|
||||
- [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; `Send()` then `CreateNotificationContext(NotificationToSend.Id, RecId)` at 22-24), `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).
|
||||
- Retained instances and saved Ids: [VATBusPostGrpPart.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/Finance/VAT/Setup/VATBusPostGrpPart.Page.al) (`CreateGuid()` in `OnOpenPage` at line 86, global at 93, recall at 99 and 115), [Certificate.Page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Layers/W1/BaseApp/System/IsolatedStorage/Certificate.Page.al) (globals at 189-190 with no Id, sent at 226 and 245, recalled at 248-252), and [DataSearchLines.page.al](https://github.com/microsoft/BCApps/blob/837ef802485ee457e52310d2ecaa08b93d0122fd/src/Apps/W1/DataSearch/App/DataSearchLines.page.al) (`Send()` then `LastChangedSetupNotification := ChangedSetupNotification.Id` at 218-219, recall with the saved Id at 260-269).
|
||||
- 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).
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue