Repository navigation
UI knowledge: notification recall needs a known Id; open mixed-type documents through Page Management - #214
Conversation
… routing Two ui articles with compiled good/bad samples: - notification-recall-needs-known-id: a notification that the code also recalls needs a fixed Id (single instance; recall before re-send) or per-record tracking through codeunit "Notification Lifecycle Mgt." (SendNotification[WithAdditionalContext] / RecallNotificationsForRecord, HandleDelayedInsert semantics). Never-recalled notifications are exempt. - list-page-document-routing-uses-page-management: open documents from a mixed-type list with PageManagement.PageRun(Rec) instead of a hand-written case "Document Type" / Page.Run; register new tables through OnConditionalCardPageIDNotFound. Single-type opens and tables Page Management does not route are exempt. Wired into al-ui-review (entry gate now covers notification send/recall outside pages, tokens, high-signal mappings) and registered both pairs in the ui review-fixtures override. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n, 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>
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Reviewed 5c15b08eebc3695940e129ed17df5b3270665d64. The Page Management mapping and notification lifecycle facts are generally sound, and local contract/retrieval checks pass. One notification false-positive boundary blocks merge.
notification-recall-needs-known-id.md:23,29 and al-ui-review.md:55 treat direct Send/Recall with no explicit Id or with CreateGuid as inherently unrecoverable. That is not true if the same Notification instance or its generated Id is retained. For example, a page can keep a global Notification, assign CreateGuid once in OnOpenPage, send it, and recall that same instance on a later action/OnClosePage. Likewise, Send assigns an initially unassigned Id; retaining the instance or saving that Id permits a later recall. The defect in the bad fixture is losing/replacing the sent Id across fresh local variables, not whether the Id is literal or runtime-generated. Notification Lifecycle Mgt. itself calls Send and then reads NotificationToSend.Id to retain the generated identity, confirming the relevant mechanism.
Please require evidence that the recalled identity differs from or cannot recover the sent identity, explicitly allow retained instances/saved generated IDs (including correctly tracked per-record IDs), and add those as clean controls. Recommend the lifecycle helper without making it mandatory for every otherwise-correct direct tracking implementation. No change to the Page Management rule is required.
… 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>
|
Thanks, fixed in cff8917, plus a merge of current main in a37f45a.
The Page Management article is unchanged. All validators pass, including |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Reviewed cff8917ecb2849f2406cdd8380cbb38228901770 against the previously reviewed head. The notification blocker is resolved: article and routing now require evidence that Recall cannot recover the sent identity, explicitly allowing retained Notification instances, saved Send-assigned/generated IDs and correct direct per-record tracking. The new clean controls exercise a global instance with CreateGuid once and a local Send whose ID is saved for a later Recall; these mechanisms match the cited VAT posting-group, Certificate and Data Search examples. Lifecycle management is recommended, not mandatory. The Page Management rule and its fixtures are unchanged, and main registrations are preserved. Exact-head local frontmatter, changed-path fixture preparation (248 cases), review contract, skill/schema and index/retrieval checks pass. No merge-critical issue remains. GitHub's substantive workflows are awaiting approval and should run before merge.
86d809b
into
microsoft:main
Summary
Two ui articles. Both mistakes compile cleanly and look reasonable at a glance.
ui/notification-recall-needs-known-id.mdIdgets one atSend()(Learn, Notification.Id). A laterRecall()from a new variable can't target it, so the warning stays until the user dismisses it or the page closes (Learn, nonintrusive notifications).Recall()before re-sending.AnalysisView.ShowResetNeededNotification(lines 1039-1051) does this with a plainSend().SalesLine.SendBlockedItemNotification(10126-10135) andOver-Receipt Mgt.(212-228) pass the fixed Id to"Notification Lifecycle Mgt.".SendNotification.Send()does with an Id that is already on screen, so the article doesn't rely on replacement. It quotes Learn's "can be recalled successfully even if it hasn't been sent" and lists the documented cases whereRecall()returnsfalse.SendNotificationcallsCreateGuid()only when the Id is null, and tracks Id↔RecordId in the temporary "Notification Context" table (17-36).RecallNotificationsForRecordrecalls every tracked notification for the record (38-44)....WithAdditionalContextpair takes a fixed GUID per reason, asItem-Check Avail.does (89-90, 636-646).HandleDelayedInsertis documented fromGetUsableRecordId(177-191). TheNotification Lifecycle Handlerdelete subscribers passfalse(27-52). Tables outside Base App need their ownSetRecordID/UpdateRecordID/recall calls.ui/list-page-document-routing-uses-page-management.mdSales Listhas noCardPageID. ItsShowDocumentaction callsPageManagement.PageRun(Rec)(188-202), as doPurchase List,Sales LinesandNavigate.PageRunreturnsfalsewithoutGuiAllowed.case "Document Type"/Page.Runon a routed table, directly or afterGet, duplicates the mapping. It misses mappings Microsoft later adds to Page Management and routing that extensions add through its events.OnConditionalCardPageIDNotFound, which is raised only for unrouted tables (line 258), as the Sustainability app does. Quality Management uses theIsHandledvariant instead.Sales/Purchase Line Archive List,Copy Document Mgt.ShowSalesDoc/ShowPurchDoc,Office Document Handler), so severity is minor. Codeunit helpers are outside the UI page scope.CardPageID(Sales Order List), and tables Page Management doesn't route (Assembly List).Verification
837ef802485ee457e52310d2ecaa08b93d0122fd, and every cited range was re-opened.Wiring
al-ui-review.md: the entry gate also admits AL code that sends or recalls notifications, since Base App sends most of them from tables and codeunits. Such non-page files are checked only against notification knowledge.evaluation/review-fixtures.json.Test plan
validate_frontmatter.py: 0 errors (2 warnings, both in files this PR doesn't touch)Test-ReviewFixtures.ps1: 232 cases / 116 paired articlesTest-ReviewContract.ps1,Test-SkillIndex.ps1,Test-KnowledgeIndex.ps1,Test-KnowledgeRetrieval.ps1: pass🤖 Generated with Claude Code