From 5f1cff2fb6427c14cf9b2ef1f790ee404a144b9e Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Tue, 18 Aug 2026 11:17:14 +0200 Subject: [PATCH 1/4] Refine self-improvement review guidance Narrow IsHandled, label-scope, UI-handler, checkpoint, and bulk-operation guidance to evidence-backed false-positive boundaries. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- ...shandled-to-false-before-publishing.bad.al | 6 +- ...handled-to-false-before-publishing.good.al | 16 +++--- ...ze-ishandled-to-false-before-publishing.md | 8 +-- .../avoid-commit-inside-loops.good.al | 25 ++++++++- .../performance/avoid-commit-inside-loops.md | 8 ++- .../prefer-modifyall-over-per-row-modify.md | 4 +- ...ggers-and-media-field-regress-modifyall.md | 8 +-- .../labels-declared-at-object-scope.bad.al | 11 ---- .../labels-declared-at-object-scope.good.al | 13 ----- .../style/labels-declared-at-object-scope.md | 20 ++----- .../testing/ui-handlers-in-tests.bad.al | 45 +++++---------- .../testing/ui-handlers-in-tests.good.al | 55 +++++-------------- .../knowledge/testing/ui-handlers-in-tests.md | 12 ++-- microsoft/skills/review/al-events-review.md | 2 +- .../skills/review/al-performance-review.md | 5 +- microsoft/skills/review/al-privacy-review.md | 2 +- microsoft/skills/review/al-security-review.md | 2 +- microsoft/skills/review/al-style-review.md | 4 +- microsoft/skills/review/al-testing-review.md | 4 +- microsoft/skills/review/al-ui-review.md | 2 +- microsoft/skills/review/al-upgrade-review.md | 2 +- 21 files changed, 99 insertions(+), 155 deletions(-) delete mode 100644 microsoft/knowledge/style/labels-declared-at-object-scope.bad.al delete mode 100644 microsoft/knowledge/style/labels-declared-at-object-scope.good.al diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al index 94b50f3..2cb465d 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al @@ -6,14 +6,12 @@ codeunit 50241 "IsHandled Init Bad Sample" DiscountPct: Decimal; IsHandled: Boolean; begin - // IsHandled is never initialized before the first raise, so flow depends - // on the variable's default rather than an explicit, documented intent. OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); if not IsHandled then DiscountPct := 5; - // Bug: IsHandled is not reset. If the first subscriber set it true, the - // payment-discount default below is silently skipped too. + // Bug: execution continues when the first event set IsHandled to true, + // and that stale value is passed to a different publisher. OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); if not IsHandled then DiscountPct += 2; diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al index 190e321..a595e8a 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al @@ -6,17 +6,17 @@ codeunit 50240 "IsHandled Init Good Sample" DiscountPct: Decimal; IsHandled: Boolean; begin - IsHandled := false; + // A freshly declared local Boolean is false. OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); - if not IsHandled then - DiscountPct := 5; + if IsHandled then + exit; + DiscountPct := 5; - // Reset before reusing the same variable for the next event so a - // subscriber that handled the first raise can't suppress this one. - IsHandled := false; + // Reaching this point proves that IsHandled is still false. OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); - if not IsHandled then - DiscountPct += 2; + if IsHandled then + exit; + DiscountPct += 2; end; [IntegrationEvent(false, false)] diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md index acfb54a..9a2aef2 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md @@ -7,20 +7,20 @@ countries: [w1] application-area: [all] --- -# Initialize IsHandled to false before publishing +# Reset IsHandled before publishing only when its value can carry over ## Description -A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable in by reference, so its incoming value decides whether the default logic is skipped. A freshly declared Boolean starts as `false`, but the same variable is frequently reused to raise several events in one routine, and after the first raise it may already be `true`. Assigning `IsHandled := false;` on the line immediately before every raise makes the control flow deterministic and self-documenting, and prevents a stale `true` from silently suppressing logic the author never meant to make skippable. Generated code often reuses one `IsHandled` across several raises without resetting it. +A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event is already deterministic. The same is true when control flow proves the variable is `false`; for example, reaching a second raise after `if IsHandled then exit;` proves that the first raise did not leave it `true`. ## Best Practice -Set `IsHandled := false;` immediately before each `OnBeforeX(…, IsHandled)` raise, then guard the default logic with `if IsHandled then exit;` or `if not IsHandled then …`. Do this even when the variable was just declared: the explicit reset documents intent and stays correct if a second event raise is added to the routine later. This applies only to events that carry a `var IsHandled: Boolean`; an `OnBefore` event with no `IsHandled` parameter needs no reset. +Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, the value comes from an input parameter, field, or global, or earlier code seeds it. A reset on a guaranteed-false fresh local can be retained for readability, but its absence is not a correctness finding. See sample: `initialize-ishandled-to-false-before-publishing.good.al`. ## Anti Pattern -Raising `OnBeforeX(…, IsHandled)` with a variable whose value carries over from an earlier raise, so a subscriber that handled the first event unintentionally suppresses the second routine's default logic. Detection: an `IsHandled` variable passed to more than one event in a routine without an intervening `IsHandled := false;`, or any `OnBefore…` raise that passes an `IsHandled` variable without an intervening `IsHandled := false;`. +Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise or another source, so the new publisher call starts with stale state. Do not match a single raise using a fresh local Boolean, or a later raise reached only after `if IsHandled then exit;`. See sample: `initialize-ishandled-to-false-before-publishing.bad.al`. diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al index 2ffa386..e82f98b 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al @@ -16,11 +16,18 @@ codeunit 50128 "Perf Sample CommitInLoop Good" { procedure NormalizeCustomerNames() var + NormalizeState: Record "Perf Normalize State"; LastCustomerNo: Code[20]; begin - // The outer loop owns checkpoints; the per-row loop contains no Commit. - while NormalizeNextChunk(LastCustomerNo) do + NormalizeState.Get('CUSTOMER'); + LastCustomerNo := NormalizeState."Last Customer No."; + + while NormalizeNextChunk(LastCustomerNo) do begin + // Persist progress in the same transaction as the completed chunk. + NormalizeState."Last Customer No." := LastCustomerNo; + NormalizeState.Modify(); Commit(); + end; end; local procedure NormalizeNextChunk(var LastCustomerNo: Code[20]): Boolean @@ -58,3 +65,17 @@ codeunit 50128 "Perf Sample CommitInLoop Good" exit(true); end; } + +table 50128 "Perf Normalize State" +{ + fields + { + field(1; Code; Code[10]) { } + field(2; "Last Customer No."; Code[20]) { } + } + + keys + { + key(PK; Code) { Clustered = true; } + } +} diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.md b/microsoft/knowledge/performance/avoid-commit-inside-loops.md index 13f483a..25ad958 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.md +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.md @@ -13,16 +13,18 @@ application-area: [all] ## Description -Commit ends the current write transaction. Calling it inside a per-row loop produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with the platform's ability to batch write operations. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). When the batch is too large for one transaction, the fix is not a per-row Commit but bounded checkpoints that select an exact list of at most N keys and process only those rows. +Commit ends the current write transaction. Calling it inside a per-row loop usually produces one transaction per iteration and loses the ability to roll back the whole operation atomically; it also interferes with batching. Most loops need no explicit Commit at all — AL auto-commits the enclosing code module on successful completion (see `understand-implicit-transaction-boundary.md`). + +A durability checkpoint inside an outer batch loop can be valid only when the same transaction persists a progress marker or state that makes retries strictly exclude completed work, the checkpoint follows a complete business unit, and errors propagate instead of being swallowed. Restart safety and bounded retrieval are separate requirements: a persisted watermark can make retries safe, but an outer `FindSet` over the full remaining tail with periodic commits still retrieves the complete set because [`FindSet` is not implemented as `TOP X`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#get-find-findset-and-next). ## Best Practice -If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. `FindSet` is optimized for reading the complete filtered set and isn't implemented as `TOP X`, so calling it over the remaining tail and breaking after N rows does not bound retrieval. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Commit after the bounded inner loop returns and persist its last selected key as the next watermark. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`. +If the batch is large enough that a single transaction is untenable, use an ordered primary-key watermark and retrieve a bounded next-N key list. The sample uses a query capped by [`TopNumberOfRows`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/query/queryinstance-topnumberofrows-method) to fill a temporary key buffer, then takes update locks and modifies only those exact keys. It does not reconstruct an inclusive first-to-last range that concurrent inserts could expand. Persist the last selected key in the same transaction as the completed chunk, then commit after the bounded helper returns. Use a stable key and define how a later run handles records inserted at or below an already committed watermark. Let errors escape so failed work is not recorded as complete. A `Codeunit.Run` boundary can also own a chunk when its implicit commit and error behavior fit the caller — see `codeunit-run-as-atomic-sub-operation.md`. See sample: `avoid-commit-inside-loops.good.al`. ## Anti Pattern -Placing Commit inside `repeat ... until Next() = 0` is almost always a mistake: it is unusual for the correctness of the operation to depend on per-row commits, and the cost of starting a new transaction on every row dominates the work. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint. +Placing Commit inside `repeat ... until Next() = 0` without persisted progress is almost always a mistake: retries re-enter already committed work, while the cost of starting a transaction on every row dominates the operation. A progress variable held only in memory is not restart-safe. A full-tail `FindSet` with a commit every N rows is not bounded retrieval, even if a persisted watermark makes it restart-safe. A capped query that discovers only an upper key and then re-reads an inclusive key range is not exact batching either; concurrent inserts inside that range can enlarge the checkpoint. See sample: `avoid-commit-inside-loops.bad.al`. diff --git a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md index ae83968..024aae1 100644 --- a/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md +++ b/microsoft/knowledge/performance/prefer-modifyall-over-per-row-modify.md @@ -15,12 +15,12 @@ application-area: [all] ## Best Practice -Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table-extension triggers, event subscribers, global triggers, or media fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). +Use `ModifyAll` when the loop directly assigns the same value, does not call `Validate`, needs no per-row calculation, and does not depend on `OnModify` unless the equivalent `RunTrigger` value is supplied. Check whether table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields force row-by-row fallback (see `triggers-and-media-field-regress-modifyall.md`). A visible loop for progress UX is acceptable only when evidence shows the equivalent bulk call already executes as individual operations and the loop preserves trigger and business semantics. See sample: `prefer-modifyall-over-per-row-modify.good.al`. ## Anti Pattern -A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior. +A loop that only assigns a constant and calls `Modify(false)` on a field with no validation side effects or bulk fallback condition. A progress dialog alone does not exempt this loop. Conversely, replacing `Validate(Field, Value); Modify(true)` with `ModifyAll(Field, Value)` is also an anti-pattern because it silently drops field validation and may drop table-trigger behavior. See sample: `prefer-modifyall-over-per-row-modify.bad.al`. diff --git a/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md b/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md index c4890a0..dabeb31 100644 --- a/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md +++ b/microsoft/knowledge/performance/triggers-and-media-field-regress-modifyall.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: performance -keywords: [modifyall, deleteall, regression, triggers, media, getglobaltabletriggermask, subscriber] +keywords: [modifyall, deleteall, regression, triggers, media, security-filtering, companion-fields, subscriber, progress] technologies: [al] countries: [w1] application-area: [all] @@ -11,12 +11,12 @@ application-area: [all] ## Description -`ModifyAll` and `DeleteAll` usually execute as single SQL statements, but the platform falls back to a fetch-then-row-by-row loop under specific conditions. Per the upstream guidance, the regression is triggered by any of: global database triggers defined via `GetGlobalTableTriggerMask` or `GetDatabaseTableTriggerSetup` (so that `OnDatabaseDelete`/`OnGlobalDelete` must run); event subscribers on the table's `OnBeforeDelete`/`OnAfterDelete` (for `DeleteAll`) or `OnBeforeModify`/`OnAfterModify` (for `ModifyAll`); or "adding a Media or MediaSet table field to either the table or table extension." Each of these forces the platform to materialize each affected row in AL. +`ModifyAll` and `DeleteAll` can limit SQL calls, but Microsoft documents that they [revert to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related modify/delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. These conditions must be assessed from the target table and runtime context, not only from the visible bulk call. ## Best Practice -Before introducing any of the above on a table — a global trigger registration, a `Modify`/`Delete` subscriber, a media or media-set field — note every `ModifyAll`/`DeleteAll` that targets the table and assess whether the regression cost is acceptable. The upstream guidance is explicit: "There should be a very good reason for doing any of the above since they will significantly regress performance of `ModifyAll` and/or `DeleteAll`." Once a table has regressed, multiple `ModifyAll` calls each iterate the rows themselves, so consolidating to one explicit `FindSet`+`Modify` loop becomes faster than chaining several `ModifyAll` calls. +Before introducing a fallback condition, audit the `ModifyAll`/`DeleteAll` call sites that target the table and assess the regression cost. Once a bulk path already executes row by row, one explicit loop can be reasonable when it preserves the same trigger semantics and adds required per-row progress UX; consolidating several regressed bulk calls into one pass can also avoid repeated iteration. This is a narrow equivalence check, not a generic progress-dialog exemption: when no fallback condition applies, retain the bulk API. ## Anti Pattern -Adding a media field to a hot table — or subscribing to its modify/delete events from a generic logging codeunit — without auditing the bulk-write call sites. The schema change is mechanical; the performance change is invisible at the call site and only surfaces when a previously fast `ModifyAll` starts paying the per-row trigger cost in production. The mirror anti-pattern is chaining several `ModifyAll` calls on a table that has already regressed; each one re-iterates the same rows. +Adding a fallback condition to a hot table without auditing bulk-write call sites, or replacing a working bulk API with a per-row loop solely to show progress. The mirror anti-pattern is chaining several bulk calls on a table that already falls back, causing repeated row-by-row passes. diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al b/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al deleted file mode 100644 index 33c63fb..0000000 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.bad.al +++ /dev/null @@ -1,11 +0,0 @@ -codeunit 50262 "Sample Label Scope Bad" -{ - procedure LookupCustomer(CustomerNo: Code[20]) - var - Customer: Record Customer; - GreetingMsg: Label 'Hello %1', Comment = '%1 = Customer Name'; - begin - if Customer.Get(CustomerNo) then - Message(GreetingMsg, Customer.Name); - end; -} diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.good.al b/microsoft/knowledge/style/labels-declared-at-object-scope.good.al deleted file mode 100644 index 415bda7..0000000 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.good.al +++ /dev/null @@ -1,13 +0,0 @@ -codeunit 50263 "Sample Label Scope Good" -{ - var - GreetingMsg: Label 'Hello %1', Comment = '%1 = Customer Name'; - - procedure LookupCustomer(CustomerNo: Code[20]) - var - Customer: Record Customer; - begin - if Customer.Get(CustomerNo) then - Message(GreetingMsg, Customer.Name); - end; -} diff --git a/microsoft/knowledge/style/labels-declared-at-object-scope.md b/microsoft/knowledge/style/labels-declared-at-object-scope.md index 24a868e..91d75d7 100644 --- a/microsoft/knowledge/style/labels-declared-at-object-scope.md +++ b/microsoft/knowledge/style/labels-declared-at-object-scope.md @@ -1,30 +1,18 @@ --- bc-version: [all] domain: style -keywords: [label, scope, procedure, translation, localization, xliff] +keywords: [label, scope, procedure, translation, localization, xliff, false-positive] technologies: [al] countries: [w1] application-area: [all] --- -# Declare Labels at object scope, not inside procedure `var` blocks +# Procedure-local Labels are valid ## Description -`Label` is the AL declaration that participates in the translation pipeline: the build extracts every Label declared in an object into the `.xlf` file shipped to translators, and the runtime substitutes the localized value when the object is loaded. Translation tooling discovers Labels by walking the object's top-level declarations. - -Labels declared inside a procedure-local `var` block are still **compiled** as Label values, but their participation in localization is fragile: depending on the BC version, the build pipeline, and the translation toolchain in use, procedure-local Labels may be missed during XLIFF extraction, may be re-emitted with auto-generated keys that change between builds, or may not be addressable by reviewers triaging translations. The reliable, supported pattern is to declare every Label in the object's top-level `var` block. - -The same rule applies to all object types that own behavior: codeunits, pages, tables, reports, queries, and their extensions. For shared messages used by multiple objects, declare the Label in the most appropriate owning object and reference it — do not duplicate the literal across procedure-scoped declarations in several places. +The AL language supports `Label` variables at both object and procedure scope. Microsoft documents the [Label data type](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-using-labels#label-data-type) without imposing an object-scope requirement, and the translation pipeline generates an XLF file containing [all labels used by the extension](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-work-with-translation-files#generating-the-xliff-file). There is no documented correctness or localization defect caused solely by declaring a Label in a procedure-local `var` block. ## Best Practice -Move every `Label` to the object's top-level `var` block. Use the appropriate suffix (`Msg`, `Err`, `Qst`, `Lbl`, `Tok`, `Txt`) on the variable name so reviewers and the translation team can see at a glance what role the string plays. Pair non-translatable strings (URLs, JSON/XML fragments, integration tokens) with `Locked = true`, as covered by `label-locked-for-non-translatable.md`. - -See sample: `labels-declared-at-object-scope.good.al`. - -## Anti Pattern - -Declaring `Label` inside a procedure-local `var` block — `procedure Lookup() var GreetingMsg: Label 'Hello %1';` — couples the translatable string to one procedure, hides it from object-level review, and depends on a translation pipeline behavior that is not part of the AL language contract. - -See sample: `labels-declared-at-object-scope.bad.al`. +Choose object scope when a Label is reused or when an established repository convention prefers central declarations; choose procedure scope when the Label belongs to one procedure. Do not report a correctness or localization finding solely because a Label is local. An explicit object-scope convention is at most a low-severity maintainability preference. This guidance applies equally to production and test apps: test code still needs localization where its strings are user-facing or translator-facing. diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al index 1ecf47d..e1b8fc7 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al @@ -1,43 +1,28 @@ -codeunit 50401 "Test UI Handlers Bad" +codeunit 50401 "Test UI Handler Proof Bad" { Subtype = Test; - // Several wiring mistakes, each of which fails at runtime rather than as a - // clean assertion the reviewer can read: - // * A UI call with no listed handler -> "unhandled UI" abort (the Message - // below has no handler). - // * The mirror mistake, listing a handler the path never hits, instead - // fails with "handler function was not executed". - // * A handler that hardcodes its answer and asserts inline, with no - // enqueue/dequeue -> nothing proves the RIGHT dialog fired the RIGHT - // number of times, and a failed inline assert can be swallowed by the - // calling UI operation. [Test] - [HandlerFunctions('ConfirmHandler')] - procedure PostDocumentConfirmsAndMessages() + [HandlerFunctions('CustomerCardHandler')] + procedure CustomerCardActionSucceeds() + var + Customer: Record Customer; begin - // No Initialize(): a value leaked by an earlier test corrupts this one. - RunPostingThatConfirmsAndMessages(); - // No AssertEmpty(): a missing or extra dialog goes unnoticed. + Customer.Get('10000'); + ActionSucceeded := true; + + Page.RunModal(Page::"Customer Card", Customer); + + // This only proves a value assigned before the action stayed true. + Assert.IsTrue(ActionSucceeded, 'The customer card action failed.'); end; - local procedure RunPostingThatConfirmsAndMessages() + [ModalPageHandler] + procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin - // Raises a Confirm AND a Message, but only ConfirmHandler is listed: - // the Message has nothing to intercept it -> unhandled-UI runtime abort. - if Confirm('Post this document?', false) then - Message('Posting completed.'); - end; - - [ConfirmHandler] - procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean) - begin - // Hardcoded expectation and hardcoded reply. If the wrong dialog fires, - // this inline assert may never surface as the test's verdict. - Assert.AreEqual('Post this document?', Question, 'Wrong confirm.'); - Reply := true; end; var Assert: Codeunit "Library Assert"; + ActionSucceeded: Boolean; } diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al index f955477..42b7471 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al @@ -1,57 +1,28 @@ -codeunit 50400 "Test UI Handlers Good" +codeunit 50400 "Test UI Handler Capture Good" { Subtype = Test; [Test] - [HandlerFunctions('ConfirmHandler,PostMessageHandler')] - procedure PostDocumentConfirmsAndMessages() + [HandlerFunctions('CustomerCardHandler')] + procedure CustomerCardShowsSelectedCustomer() + var + Customer: Record Customer; begin - Initialize(); + Customer.Get('10000'); + CapturedCustomerNo := ''; - // [GIVEN] the test enqueues, in interaction order, what each handler - // will see and how it should answer: the Confirm's expected - // question plus the reply to return, then the expected Message. - LibraryVariableStorage.Enqueue('Post this document?'); // expected question (substring) - LibraryVariableStorage.Enqueue(true); // reply ConfirmHandler returns - LibraryVariableStorage.Enqueue('Posting completed.'); // expected message (substring) + Page.RunModal(Page::"Customer Card", Customer); - // [WHEN] the code under test raises the Confirm and then the Message - RunPostingThatConfirmsAndMessages(); - - // [THEN] every enqueued expectation was consumed exactly once - LibraryVariableStorage.AssertEmpty(); + Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.'); end; - local procedure Initialize() + [ModalPageHandler] + procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin - // Clear leftover values so a value leaked by an earlier test cannot - // cascade into this one. - LibraryVariableStorage.Clear(); - end; - - local procedure RunPostingThatConfirmsAndMessages() - begin - // Stands in for the production routine that confirms, then messages. - if Confirm('Post this document?', false) then - Message('Posting completed.'); - end; - - [ConfirmHandler] - procedure ConfirmHandler(Question: Text[1024]; var Reply: Boolean) - begin - // Verify the RIGHT dialog fired (substring match), then return the - // reply the test enqueued for it. - Assert.ExpectedConfirm(LibraryVariableStorage.DequeueText(), Question); - Reply := LibraryVariableStorage.DequeueBoolean(); - end; - - [MessageHandler] - procedure PostMessageHandler(Message: Text[1024]) - begin - Assert.ExpectedMessage(LibraryVariableStorage.DequeueText(), Message); + CapturedCustomerNo := CustomerCard."No.".Value(); end; var Assert: Codeunit "Library Assert"; - LibraryVariableStorage: Codeunit "Library - Variable Storage"; + CapturedCustomerNo: Code[20]; } diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.md b/microsoft/knowledge/testing/ui-handlers-in-tests.md index 338e9ec..42a8d83 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.md +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.md @@ -1,28 +1,28 @@ --- bc-version: [all] domain: testing -keywords: [handler, handlerfunctions, confirm, message, strmenu, variable-storage, enqueue, unhandled-ui] +keywords: [handler, handlerfunctions, confirm, message, strmenu, variable-storage, enqueue, capture, runmodal, unhandled-ui] technologies: [al] countries: [w1] application-area: [all] --- -# Wire and verify UI handlers with enqueue-driven expectations +# Wire UI handlers and verify meaningful outcomes ## Description -A test runs headless: there is no interactive user to answer a dialog. Every UI call the executed path raises — `Confirm`, `Message`, error dialogs, `Page.Run`/`RunModal`, `Report.Run`/`RunModal`, request pages, `StrMenu`, `Notification.Send` — must be intercepted by a handler carrying the matching attribute (`[ConfirmHandler]`, `[MessageHandler]`, `[StrMenuHandler]`, `[ModalPageHandler]`, …) and named in the method's `[HandlerFunctions(...)]`. The list is a two-sided contract: raise a UI call with no listed handler and the platform aborts with an *unhandled UI* error; list a handler the path never hits and it fails with *"handler function was not executed"*. Both are runtime failures — the test never reaches its verdict, so a reviewer sees an infrastructure error instead of a result on the behavior under test. +A test runs headless, so every UI call on the executed path must be intercepted by a matching handler named in `[HandlerFunctions(...)]`. The list is a two-sided contract: an unhandled UI call aborts the test, while Microsoft documents that [every listed handler must execute at least once](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/attributes/devenv-handlerfunctions-attribute#remarks) or the test fails. -Getting the handler *present* is only half the job; the handler must also verify the *right* dialog fired the *right* number of times. Do that by driving handlers from the test, not by hardcoding answers inside them. +Beyond that wiring guarantee, the test must verify the behavior it cares about. The appropriate pattern depends on the contract: a handler can capture concrete page state or a result and the test can assert that semantic postcondition after `RunModal`; assertions inside a handler are also supported. Queue/enqueue/dequeue and `LibraryVariableStorage.AssertEmpty` are useful when interaction order, count, text, replies, or a scripted sequence is itself part of the contract, but they are not mandatory for every handler. ## Best Practice -Make the test own the expectations and the handlers consume them. Before acting, the test `Enqueue`s — in interaction order — the expected text (a stable substring) and any reply each handler must return. The handler `Dequeue`s the expected text, verifies it with the purpose-built asserts (`Assert.ExpectedMessage`, `Assert.ExpectedConfirm`, `Assert.ExpectedStrMenu` — which match on a fragment, not the full localized caption), then `Dequeue`s and returns its reply. Finish the test body with `LibraryVariableStorage.AssertEmpty` to prove every enqueued interaction fired exactly once, and start each test with an `Initialize` that calls `LibraryVariableStorage.Clear` so a value leaked by an earlier test cannot cascade. List in `[HandlerFunctions]` precisely the handlers the scenario triggers — no superset "just in case", no subset that happens to work today. +List precisely the handlers the scenario triggers and make each handler contribute meaningful evidence. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. For ordered or repeated interactions, let the test enqueue expectations, let handlers dequeue and verify them, clear storage during initialization, and finish with `AssertEmpty`. See sample: `ui-handlers-in-tests.good.al`. ## Anti Pattern -Omitting a handler for a UI call the path raises (unhandled-UI abort), padding the list with a handler the path never reaches ("handler function was not executed"), or writing handlers that hardcode their answer and assert inline with no enqueue/dequeue. The last is the subtle one: nothing proves the correct dialog fired the expected number of times, and an inline assertion that fails inside a handler can be swallowed by the calling UI operation, leaving the suite green while the behavior is broken. Skipping `Initialize`/`AssertEmpty` hides both a leaked queue and a missing or extra dialog. +Omitting a handler for a UI call, listing a handler the path never reaches, or claiming action success from a Boolean set before the action runs. A handler that only closes a page can also leave the test without a semantic assertion. Do not flag the absence of queue storage by itself; require it only when the test needs to prove interaction order, count, text, replies, or a scripted sequence. See sample: `ui-handlers-in-tests.bad.al`. diff --git a/microsoft/skills/review/al-events-review.md b/microsoft/skills/review/al-events-review.md index de2700c..f624de3 100644 --- a/microsoft/skills/review/al-events-review.md +++ b/microsoft/skills/review/al-events-review.md @@ -51,7 +51,7 @@ When the post-conflict worklist is empty because no applicable events knowledge The following targeted checks map diff signals to specific `events` articles. Treat each as a candidate-selection cue: when the signal appears in the changed code, add the named article to the worklist and evaluate it in Action. -- `IsHandled` raised without an immediately preceding `IsHandled := false;`, or one `IsHandled` variable reused across several raises with no reset between them — `initialize-ishandled-to-false-before-publishing`. +- An `IsHandled` value that can carry over as `true` (reused after an earlier raise, input/global/field, or otherwise seeded) is passed to another publisher without a reset — `initialize-ishandled-to-false-before-publishing`. Do not match a single raise using a fresh local Boolean, or a later raise reached only after `if IsHandled then exit;`. - `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event later, so the after-event is skipped whenever the call is handled — `preserve-onafter-execution-when-ishandled-skips-the-body`. - Any parameter added to a public Business/Integration event procedure, regardless of position; do not flag additions or reordering on `local`/`internal` publishers merely because a new parameter was not appended — `add-new-event-parameters-at-the-end`. - A shipped Business/Integration event renamed or removed, or an existing parameter renamed, removed, retyped, or changed to/from `var`, based on the mistaken assumption that `local` or `internal` prevents dependent subscription; parameter order alone is not a subscriber-contract violation — `treat-local-and-internal-events-as-subscriber-contracts`. diff --git a/microsoft/skills/review/al-performance-review.md b/microsoft/skills/review/al-performance-review.md index 3262179..bf2f4e8 100644 --- a/microsoft/skills/review/al-performance-review.md +++ b/microsoft/skills/review/al-performance-review.md @@ -47,7 +47,8 @@ Apply these targeted cues even when simple token overlap would rank the article - Worklist `use-setautocalcfields-for-per-row-flowfields.md` when a record loop calls `CalcFields`, or when every row reads the same FlowField for a comparison, branch, or per-record action. Worklist `calcsums-instead-of-calcfields-in-loop.md` instead when the loop only accumulates one set total. - Worklist `hidden-flowfields-still-calculate-before-bc26-opt-in.md` when a page control directly sources a FlowField and sets `Visible = false` or a visibility expression. Suppress it when the target is known to have BC26's **Calculate only visible FlowFields** feature enabled, or when the FlowField is cheap and intentionally preloaded. -- Worklist `avoid-commit-inside-loops.md` only when `Commit()` is inside a record-iteration body or a helper invoked once per row. Do not match one `Commit()` after a bounded checkpoint helper returns, a `Commit()` outside iteration, or comments and documentation that merely mention commits. +- Worklist `avoid-commit-inside-loops.md` when `Commit()` is inside a record-iteration body or a checkpoint loop lacks persisted progress that excludes completed work on retry. Do not match a commit after a complete business unit when the same transaction persists a restart-safe watermark/state and errors propagate. Still match a full-tail `FindSet` with periodic commits as unbounded retrieval; restart safety does not make it `TOP X`. +- Worklist `prefer-modifyall-over-per-row-modify.md` for a constant-assignment `Modify(false)` loop with no validation or per-row semantics. Worklist `triggers-and-media-field-regress-modifyall.md` when table trigger code, related subscribers, security filtering, `Media`/`MediaSet`, or companion fields affect a bulk path. A progress dialog does not generically exempt a loop; accept it only when the equivalent bulk call already falls back to individual operations and semantics are preserved. - Worklist `avoid-cloning-records-before-modify-delete-in-loops.md` when an iteration calls `Copy` or `RecordRef.GetTable` before `Modify`/`Delete`, or passes the iterated record without `var` to a helper that writes that record. Do not worklist it from `Modify`, `Delete`, or `RecordRef` alone; exclude a direct write on the iterator, a read-only copy, a temporary record, a different target table, and a `RecordRef` opened and iterated directly. - Worklist `use-tryfunction-for-error-catching-not-rollback.md` only when writes occur inside a try method and the code or surrounding flow expects an error to roll them back. A bare try-method call whose Boolean result is ignored belongs exclusively to `error-handling/ignored-tryfunction-return-disables-try-semantics.md`; do not worklist the performance article from that call shape alone. - For `LockTable` in a pure read helper, select exactly one owner. Use `do-not-locktable-in-read-only-procedure.md` when the helper needs no stronger isolation and should remove the lock. Use `prefer-readisolation-over-locktable-for-reads.md` instead when the code explicitly requires committed-read semantics and `ReadIsolation` is the replacement. Never emit both findings for the same call. @@ -74,7 +75,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a performance defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material performance defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly performance; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. -For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-privacy-review.md b/microsoft/skills/review/al-privacy-review.md index ea95269..0bbd8ae 100644 --- a/microsoft/skills/review/al-privacy-review.md +++ b/microsoft/skills/review/al-privacy-review.md @@ -70,7 +70,7 @@ Set `confidence` to: This leaf emits only knowledge-backed privacy findings. Do NOT emit reference-less `agent:` findings in this domain: online evaluation shows the privacy agent-finding channel yields almost no accepted findings and a high volume of dismissed noise, so a privacy concern that no worklist knowledge file covers is omitted here rather than emitted with `references: []`. When you spot a material privacy defect no article covers, the durable fix is to add a knowledge article in BCQuality (per the online-eval self-improvement loop) so this leaf can cite it — not a one-off reference-less finding. Before treating a candidate as uncovered, check the worklist for a knowledge file that matches it; if one exists, emit it as a knowledge-backed finding. See `skills/do.md` for the full contract. -For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-security-review.md b/microsoft/skills/review/al-security-review.md index 12956bd..8472afe 100644 --- a/microsoft/skills/review/al-security-review.md +++ b/microsoft/skills/review/al-security-review.md @@ -70,7 +70,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a security defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material security defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly security; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. -For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-style-review.md b/microsoft/skills/review/al-style-review.md index ee4daf5..bffef4d 100644 --- a/microsoft/skills/review/al-style-review.md +++ b/microsoft/skills/review/al-style-review.md @@ -60,7 +60,7 @@ When the post-conflict worklist is empty because no applicable style knowledge e For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Style findings rarely reach `blocker` — reserve it for cases where the knowledge file documents a platform-level requirement (for example, API page property constraints the OData runtime rejects). Most style findings are `minor` or `info`; egregious misuse (`Error` with pre-built Text losing translation and telemetry classification) may reach `major`. -Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, variable-declaration order by type AA0021, a missing `ToolTip`, required parentheses). On those, BCQuality's value is the *explanation* of why the rule exists, not a second gate; emit them at `info` so a consumer that gates on severity does not re-flag what CodeCop/AppSourceCop already reports. Reserve `minor` for style issues with concrete downstream impact the analyzer does not catch — a `Label` declared at procedure-local instead of object scope (no analyzer enforces label scope, and mis-scoped Labels are fragile in the translation pipeline), lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, a misleading named invocation. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. +Severity calibration — a formal analyzer already flags the mechanical presence/naming conventions (the `this` keyword AA0248, approved label suffixes AA0074, variable-declaration order by type AA0021, a missing `ToolTip`, required parentheses). On those, BCQuality's value is the *explanation* of why the rule exists, not a second gate; emit them at `info` so a consumer that gates on severity does not re-flag what CodeCop/AppSourceCop already reports. Reserve `minor` for style issues with concrete downstream impact the analyzer does not catch — lost translation or telemetry classification from a string-built `Error`, an `OptionCaption` that does not match its `OptionMembers`, or a misleading named invocation. A procedure-local `Label` is valid and is not a correctness or localization finding; an explicit repository preference for object scope is at most low-severity maintainability guidance. This keeps the domain's default output advisory and prevents analyzer-redundant noise from competing with substantive review. Set `confidence` to: @@ -70,7 +70,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a style defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a clear, widely-accepted AL style violation with a concrete basis a knowledgeable BC reviewer would agree on — steelman it first and drop personal preference, speculation, and any single defensible formatting choice among several; when in doubt, omit. The scope is strictly style — naming, labelling, formatting, and analyzer-adjacent conventions. A correctness, logic, data-integrity, or contract defect is NOT a style finding even when it can be reworded as a convention: a method that mutates a shared `Record`'s filters, an unfiltered `DeleteAll`, a violated interface contract, or a wrong boolean guard are behavioural defects, not conventions — do not emit them here under a style framing. If a specific domain leaf covers the concern (performance, security, error-handling, …) it belongs there; if no knowledge file in any domain covers it, it belongs to the `al-code-review` super-skill's cross-cutting self-review agent channel (`from-sub-skill: "agent"`, `severity` capped at `minor`), not to this leaf. A reliable test: if you cannot cite a style `## Best Practice`/`## Anti Pattern` for the concern, it is very likely not a style finding. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. -For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 2483f12..168b6c8 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -50,7 +50,7 @@ The following targeted checks cover every current `testing` article. Treat each - A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`. - Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`. - `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`. -- A test path raises UI, `[HandlerFunctions(...)]` does not exactly match the invoked handlers, a handler hardcodes replies instead of using enqueue/dequeue expectations, or `LibraryVariableStorage.Clear`/`AssertEmpty` is missing — `ui-handlers-in-tests`. +- A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`. @@ -64,6 +64,8 @@ For each worklist entry, evaluate the diff against the file's `## Best Practice` - When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape. - Applicability alone is not a finding. Emit `info` only for a concrete, non-actionable observation the article explicitly defines; otherwise emit nothing when no violation is present. +For `ui-handlers-in-tests`, use `major` when missing or incorrectly listed handlers make the test fail at runtime. Use `minor` when the test executes but lacks a meaningful semantic postcondition, including a pre-set Boolean used as proof. Do not escalate solely because a handler does not use queue storage or asserts inside the handler. + Set `confidence` to: - `high` when the detection is based on an unambiguous pattern match (attribute, handler declaration, assertion sequence, or fixture call). diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index 81ab12e..c0af5af 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -63,7 +63,7 @@ Set `confidence` to: This leaf emits only knowledge-backed UI and accessibility findings. Do NOT emit reference-less `agent:` findings in this domain: online evaluation shows the UI/accessibility agent-finding channel yields almost no accepted findings and a high volume of dismissed noise, so a UI or accessibility concern that no worklist knowledge file covers is omitted here rather than emitted with `references: []`. When you spot a material UI or accessibility defect no article covers, the durable fix is to add a knowledge article in BCQuality (per the online-eval self-improvement loop) so this leaf can cite it — not a one-off reference-less finding. Before treating a candidate as uncovered, check the worklist for a knowledge file that matches it; if one exists, emit it as a knowledge-backed finding. See `skills/do.md` for the full contract. -For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index 879e788..667ea32 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -66,7 +66,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a upgrade defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material upgrade or breaking-change defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly upgrade; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. -For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; move a local `Label` to object scope; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: delete unreachable lines; replace `Count() > 0` with `not IsEmpty()`; add a missing `ToolTip`, `OptionCaption`, or `DataClassification`; replace a string-concatenated `Error` with a Label-backed call; change an over-broad permission token; or add an obvious `else`/guard branch). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. From ead337f9cbf237dd07532393afcb690c4ae160b3 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Tue, 18 Aug 2026 14:09:11 +0200 Subject: [PATCH 2/4] Address review guidance feedback Preserve independent event seams, cover loop-carried handled state, strengthen checkpoint and UI-handler fixtures, and align DeleteAll fallback guidance. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 10646e50-2d8b-4cca-b02b-dfa78629e6a1 --- ...shandled-to-false-before-publishing.bad.al | 19 +++++++++++ ...handled-to-false-before-publishing.good.al | 21 ++++++------ ...ze-ishandled-to-false-before-publishing.md | 6 ++-- .../avoid-commit-inside-loops.bad.al | 14 +++++++- .../avoid-commit-inside-loops.good.al | 6 +++- ...se-deleteall-for-filtered-bulk-deletion.md | 8 ++--- .../testing/ui-handlers-in-tests.bad.al | 34 +++++++++++++++++-- .../testing/ui-handlers-in-tests.good.al | 3 +- microsoft/skills/review/al-events-review.md | 2 +- microsoft/skills/review/al-testing-review.md | 2 +- skills/do.md | 2 +- 11 files changed, 91 insertions(+), 26 deletions(-) diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al index 2cb465d..7192bc1 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al @@ -17,6 +17,20 @@ codeunit 50241 "IsHandled Init Bad Sample" DiscountPct += 2; end; + procedure ApplyLineDiscounts(var SalesLine: Record "Sales Line") + var + LineIsHandled: Boolean; + begin + if SalesLine.FindSet() then + repeat + // Bug: the local initializes only once. A subscriber that handles + // one line leaves true for every later iteration. + OnBeforeApplyLineDiscount(SalesLine, LineIsHandled); + if not LineIsHandled then + SalesLine.Validate("Line Discount %", 5); + until SalesLine.Next() = 0; + end; + [IntegrationEvent(false, false)] local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) begin @@ -26,4 +40,9 @@ codeunit 50241 "IsHandled Init Bad Sample" local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) begin end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyLineDiscount(var SalesLine: Record "Sales Line"; var IsHandled: Boolean) + begin + end; } diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al index a595e8a..f3e57a3 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al @@ -4,19 +4,18 @@ codeunit 50240 "IsHandled Init Good Sample" procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") var DiscountPct: Decimal; - IsHandled: Boolean; + HeaderIsHandled: Boolean; + PaymentIsHandled: Boolean; begin - // A freshly declared local Boolean is false. - OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled); - if IsHandled then - exit; - DiscountPct := 5; + // Each fresh local is false and belongs to one non-looping raise. + OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, HeaderIsHandled); + if not HeaderIsHandled then + DiscountPct := 5; - // Reaching this point proves that IsHandled is still false. - OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled); - if IsHandled then - exit; - DiscountPct += 2; + // Handling the header event does not suppress this independent seam. + OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, PaymentIsHandled); + if not PaymentIsHandled then + DiscountPct += 2; end; [IntegrationEvent(false, false)] diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md index 9a2aef2..12eb39c 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md +++ b/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md @@ -11,16 +11,16 @@ application-area: [all] ## Description -A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event is already deterministic. The same is true when control flow proves the variable is `false`; for example, reaching a second raise after `if IsHandled then exit;` proves that the first raise did not leave it `true`. +A routine that raises an `OnBefore…` integration event with a `var IsHandled: Boolean` parameter passes that variable by reference, so a pre-existing `true` can affect the following control flow. AL [automatically initializes Boolean variables to `false`](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-al-variables#initialization), so a freshly declared local Boolean passed to one event exactly once per procedure invocation is already deterministic. Initialization does not repeat for each loop iteration: a local declared outside a loop can carry `true` from one iteration to the next even when the source contains only one textual event raise. Outside a loop, reaching a later raise after `if IsHandled then exit;` also proves the value is `false`, provided that early exit is semantically correct and does not skip required downstream events. ## Best Practice -Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, the value comes from an input parameter, field, or global, or earlier code seeds it. A reset on a guaranteed-false fresh local can be retained for readability, but its absence is not a correctness finding. +Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, a raise is re-entered by a loop, the value comes from an input parameter, field, or global, or earlier code seeds it. Prefer separate fresh locals when independent event seams need independent handled state. A reset on a guaranteed-false fresh local used by one non-looping raise, or before a later raise reached only after a semantically valid `if IsHandled then exit;`, can be retained for readability, but its absence is not a correctness finding. See sample: `initialize-ishandled-to-false-before-publishing.good.al`. ## Anti Pattern -Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise or another source, so the new publisher call starts with stale state. Do not match a single raise using a fresh local Boolean, or a later raise reached only after `if IsHandled then exit;`. +Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise, an earlier loop iteration, or another source, so the publisher call starts with stale state. Do not match a single non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false. See sample: `initialize-ishandled-to-false-before-publishing.bad.al`. diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al b/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al index feacfcc..696b671 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.bad.al @@ -3,12 +3,24 @@ codeunit 50129 "Perf Sample CommitInLoop Bad" procedure NormalizeCustomerNames() var Customer: Record Customer; + LastCustomerNo: Code[20]; + ProcessedCount: Integer; begin + Customer.SetFilter("No.", '>%1', LastCustomerNo); if Customer.FindSet(true) then repeat Customer.Name := UpperCase(Customer.Name); Customer.Modify(); - Commit(); + + // LastCustomerNo exists only in memory, so a retry cannot exclude + // work that was already committed. + LastCustomerNo := Customer."No."; + ProcessedCount += 1; + + // This still opened a FindSet over the complete remaining tail; + // periodic commits do not turn retrieval into bounded TOP X. + if ProcessedCount mod 500 = 0 then + Commit(); until Customer.Next() = 0; end; } diff --git a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al index e82f98b..2eb5bd0 100644 --- a/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al +++ b/microsoft/knowledge/performance/avoid-commit-inside-loops.good.al @@ -19,7 +19,11 @@ codeunit 50128 "Perf Sample CommitInLoop Good" NormalizeState: Record "Perf Normalize State"; LastCustomerNo: Code[20]; begin - NormalizeState.Get('CUSTOMER'); + if not NormalizeState.Get('CUSTOMER') then begin + NormalizeState.Init(); + NormalizeState.Code := 'CUSTOMER'; + NormalizeState.Insert(); + end; LastCustomerNo := NormalizeState."Last Customer No."; while NormalizeNextChunk(LastCustomerNo) do begin diff --git a/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md b/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md index 1c80835..41ad41f 100644 --- a/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md +++ b/microsoft/knowledge/performance/use-deleteall-for-filtered-bulk-deletion.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: performance -keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass] +keywords: [deleteall, bulk-delete, sql, ondelete, trigger-bypass, security-filtering, media, companion-fields] technologies: [al] countries: [w1] application-area: [all] @@ -13,16 +13,16 @@ application-area: [all] ## Description -`DeleteAll(false)` is eligible for a set-based SQL delete with the record variable's filters applied. It is not guaranteed to stay one statement. The base table `OnDelete` trigger is skipped, but table-extension `OnBeforeDelete` and `OnAfterDelete` triggers still run. Extension event subscribers, global delete triggers, and media fields can also require row processing. `DeleteAll(true)` runs the base table `OnDelete` trigger as well and has no performance advantage over `Delete(true)` in a loop. +`DeleteAll(false)` is eligible for a set-based SQL delete with the record variable's filters applied, but it is not guaranteed to stay one statement. Microsoft documents that `DeleteAll` [reverts to individual calls](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/administration/optimize-sql-al-database-methods-and-performance-on-server#modifyall-and-deleteall) when the table has trigger code, related delete/global/database event subscribers, active security filtering, `Media` or `MediaSet` fields, or fields added through companion tables. Setting `RunTrigger` to false skips the base table `OnDelete` trigger, but [table-extension `OnBeforeDelete` and `OnAfterDelete` triggers still run](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-deleteall-method#remarks). ## Best Practice -Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and installed extensions, subscribers, global triggers, and media fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately. +Use filtered `DeleteAll(false)` for purpose-built staging or cleanup tables only after verifying that base-table `OnDelete` logic is unnecessary and that trigger code, related subscribers, security filtering, media fields, and companion fields do not add required per-row behavior or regress the bulk path. If deletion requires per-row business logic, keep an explicit triggered operation instead of simulating trigger execution separately. See sample: `use-deleteall-for-filtered-bulk-deletion.good.al`. ## Anti Pattern -Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking table extensions and subscribers. +Iterating with `FindSet` + `Delete(false)` to clear a filtered staging batch that has no delete logic or fallback condition. The reverse mistake is assuming `DeleteAll` is always one SQL statement without checking the documented fallback conditions. See sample: `use-deleteall-for-filtered-bulk-deletion.bad.al`. diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al index e1b8fc7..d0832fe 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al @@ -4,11 +4,11 @@ codeunit 50401 "Test UI Handler Proof Bad" [Test] [HandlerFunctions('CustomerCardHandler')] - procedure CustomerCardActionSucceeds() + procedure PreSetBooleanDoesNotProveCustomerCardResult() var Customer: Record Customer; begin - Customer.Get('10000'); + LibrarySales.CreateCustomer(Customer); ActionSucceeded := true; Page.RunModal(Page::"Customer Card", Customer); @@ -17,12 +17,42 @@ codeunit 50401 "Test UI Handler Proof Bad" Assert.IsTrue(ActionSucceeded, 'The customer card action failed.'); end; + [Test] + [HandlerFunctions('CustomerCardHandler')] + procedure MissingMessageHandlerFailsAtRuntime() + var + Customer: Record Customer; + begin + LibrarySales.CreateCustomer(Customer); + + Page.RunModal(Page::"Customer Card", Customer); + Message('Customer card closed.'); + end; + + [Test] + [HandlerFunctions('CustomerCardHandler,UnusedConfirmHandler')] + procedure UnreachedListedHandlerFailsAtRuntime() + var + Customer: Record Customer; + begin + LibrarySales.CreateCustomer(Customer); + + Page.RunModal(Page::"Customer Card", Customer); + end; + [ModalPageHandler] procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin end; + [ConfirmHandler] + procedure UnusedConfirmHandler(Question: Text[1024]; var Reply: Boolean) + begin + Reply := true; + end; + var Assert: Codeunit "Library Assert"; + LibrarySales: Codeunit "Library - Sales"; ActionSucceeded: Boolean; } diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al index 42b7471..fafc515 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al @@ -8,7 +8,7 @@ codeunit 50400 "Test UI Handler Capture Good" var Customer: Record Customer; begin - Customer.Get('10000'); + LibrarySales.CreateCustomer(Customer); CapturedCustomerNo := ''; Page.RunModal(Page::"Customer Card", Customer); @@ -24,5 +24,6 @@ codeunit 50400 "Test UI Handler Capture Good" var Assert: Codeunit "Library Assert"; + LibrarySales: Codeunit "Library - Sales"; CapturedCustomerNo: Code[20]; } diff --git a/microsoft/skills/review/al-events-review.md b/microsoft/skills/review/al-events-review.md index f624de3..ebd2976 100644 --- a/microsoft/skills/review/al-events-review.md +++ b/microsoft/skills/review/al-events-review.md @@ -51,7 +51,7 @@ When the post-conflict worklist is empty because no applicable events knowledge The following targeted checks map diff signals to specific `events` articles. Treat each as a candidate-selection cue: when the signal appears in the changed code, add the named article to the worklist and evaluate it in Action. -- An `IsHandled` value that can carry over as `true` (reused after an earlier raise, input/global/field, or otherwise seeded) is passed to another publisher without a reset — `initialize-ishandled-to-false-before-publishing`. Do not match a single raise using a fresh local Boolean, or a later raise reached only after `if IsHandled then exit;`. +- An `IsHandled` value that can carry over as `true` (reused after an earlier raise, re-entered on a later loop iteration, input/global/field, or otherwise seeded) is passed to a publisher without a reset — `initialize-ishandled-to-false-before-publishing`. Do not match one non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false. - `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event later, so the after-event is skipped whenever the call is handled — `preserve-onafter-execution-when-ishandled-skips-the-body`. - Any parameter added to a public Business/Integration event procedure, regardless of position; do not flag additions or reordering on `local`/`internal` publishers merely because a new parameter was not appended — `add-new-event-parameters-at-the-end`. - A shipped Business/Integration event renamed or removed, or an existing parameter renamed, removed, retyped, or changed to/from `var`, based on the mistaken assumption that `local` or `internal` prevents dependent subscription; parameter order alone is not a subscriber-contract violation — `treat-local-and-internal-events-as-subscriber-contracts`. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 168b6c8..fb5d50f 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -74,7 +74,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a testing defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material testing defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly AL testing; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. -For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: add the matching `ExpectedError` assertion after `asserterror`; add or remove a handler name in `HandlerFunctions`; add `LibraryVariableStorage.Clear` or `AssertEmpty`; or replace hand-rolled fixture creation with an evident library call). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: add the matching `ExpectedError` assertion after `asserterror`; add or remove a handler name in `HandlerFunctions`; add `LibraryVariableStorage.Clear` or `AssertEmpty` when queue/LVS intentionally verifies interaction order, count, text, replies, or a scripted sequence; or replace hand-rolled fixture creation with an evident library call). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract. diff --git a/skills/do.md b/skills/do.md index 23bfb4a..a79c5fc 100644 --- a/skills/do.md +++ b/skills/do.md @@ -220,7 +220,7 @@ A review super-skill MUST preserve `domain` verbatim when rolling a leaf finding **`findings[].suggested-code`** — optional in the schema but **expected for mechanical findings**. It is a concrete code-replacement payload for the lines indicated by `location`. When present, the string MUST be a literal replacement for the source lines covered by `location.line` (or `location.range` if set) — i.e., what the file would contain after the fix, with no surrounding diff markers, fences, or commentary. Consumers MAY render it as a one-click suggestion in the delivery surface (for example, a GitHub ```` ```suggestion ```` block). -Emit `suggested-code` whenever the fix is small, local, and mechanical: deleting unreachable code; replacing one expression (`Count() > 0` → `not IsEmpty()`); moving a local `Label` to object scope; adding a missing property such as `ToolTip`, `OptionCaption`, or `DataClassification`; replacing a string-concatenated `Error` with a Label-backed call; changing a permission token; or adding a missing `else`/guard branch whose replacement is unambiguous from the surrounding diff. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, prefer adapting the `.good.al` replacement into `suggested-code`. +Emit `suggested-code` whenever the fix is small, local, and mechanical: deleting unreachable code; replacing one expression (`Count() > 0` → `not IsEmpty()`); adding a missing property such as `ToolTip`, `OptionCaption`, or `DataClassification`; replacing a string-concatenated `Error` with a Label-backed call; changing a permission token; or adding a missing `else`/guard branch whose replacement is unambiguous from the surrounding diff. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, prefer adapting the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but `suggested-code` is omitted, set `findings[].suggested-code-omission-reason` to a short explanation (for example, `requires choosing a real event id` or `fix spans multiple non-contiguous locations`). The `suggested-code` payload supplements `message`; it does not replace the explanation in `message`. From 5016962b40cb7d9a7e4bad30a42e8e4d18f4b5b0 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Wed, 2 Sep 2026 11:08:16 +0200 Subject: [PATCH 3/4] Align the IsHandled article slug, keywords and good sample with its narrowed scope The article was rewritten to say a reset is required only when the value can carry over, and its H1 was updated to match, but three artefacts still carried the old "always initialize to false" premise: - The slug still read `initialize-ishandled-to-false-before-publishing`, which contradicts the body. The slug is not cosmetic: Build-KnowledgeIndex.ps1 ranks candidates on keywords, frontmatter dimensions, domain, path and title, so a stale path pushes selection back toward the behaviour this change narrows. Renamed to `reset-ishandled-only-when-the-value-can-carry-over`, following the existing precedent for conditional slugs such as `unreleased-symbol-change-is-not-a-breaking-change`. - Keywords still listed `initialization` and `deterministic` and omitted `false-positive`, the tag this repository uses for suppression articles. Replaced with `carry-over` and `loop-iteration` and added `false-positive`. - The good sample demonstrated only the "prefer separate fresh locals" clause and contained no reset at all, so the article's headline case had no positive example. It was also asymmetric with the bad sample, which gained a loop procedure showing a local that carries `true` into the next iteration. Added the matching loop procedure to the good sample: a local declared outside the loop is reset at the top of each iteration. That case cannot be solved by introducing another local, because AL has no block scope, so it is the only shape that demonstrates the reset the article still requires. It also gives the engine the correct `suggested-code` shape for the loop finding; without it the one-click fix adapted from the good sample would propose splitting the variable rather than adding one line. Also renamed the sample codeunits from "IsHandled Init ..." to "IsHandled Carry Over ...", and updated the two references to the old slug: the events leaf skill cue and the events pin in evaluation/review-fixtures.json. validate_frontmatter.py reports 0 errors; Test-ReviewFixtures.ps1 passes with 32 cases across 16 leaf domains and resolves the events fixture to the renamed article. --- evaluation/review-fixtures.json | 2 +- ...only-when-the-value-can-carry-over.bad.al} | 2 +- ...nly-when-the-value-can-carry-over.good.al} | 22 ++++++++++++++++++- ...led-only-when-the-value-can-carry-over.md} | 6 ++--- microsoft/skills/review/al-events-review.md | 2 +- 5 files changed, 27 insertions(+), 7 deletions(-) rename microsoft/knowledge/events/{initialize-ishandled-to-false-before-publishing.bad.al => reset-ishandled-only-when-the-value-can-carry-over.bad.al} (97%) rename microsoft/knowledge/events/{initialize-ishandled-to-false-before-publishing.good.al => reset-ishandled-only-when-the-value-can-carry-over.good.al} (58%) rename microsoft/knowledge/events/{initialize-ishandled-to-false-before-publishing.md => reset-ishandled-only-when-the-value-can-carry-over.md} (89%) diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index 2f85d5c..68a646d 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -11,7 +11,7 @@ "article": "do-not-expose-sensitive-data-through-public-api" }, "events": { - "article": "initialize-ishandled-to-false-before-publishing" + "article": "reset-ishandled-only-when-the-value-can-carry-over" }, "interfaces": { "article": "set-defaultimplementation-on-enum" diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.bad.al similarity index 97% rename from microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al rename to microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.bad.al index 7192bc1..a7d7709 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.bad.al +++ b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.bad.al @@ -1,5 +1,5 @@ // Demonstration-only AL. Not compiled by CI; illustrates the article. -codeunit 50241 "IsHandled Init Bad Sample" +codeunit 50241 "IsHandled Carry Over Bad Sample" { procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") var diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.good.al similarity index 58% rename from microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al rename to microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.good.al index f3e57a3..84d547e 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.good.al +++ b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.good.al @@ -1,5 +1,5 @@ // Demonstration-only AL. Not compiled by CI; illustrates the article. -codeunit 50240 "IsHandled Init Good Sample" +codeunit 50240 "IsHandled Carry Over Good Sample" { procedure ApplyDiscounts(var SalesHeader: Record "Sales Header") var @@ -18,6 +18,21 @@ codeunit 50240 "IsHandled Init Good Sample" DiscountPct += 2; end; + procedure ApplyLineDiscounts(var SalesLine: Record "Sales Line") + var + LineIsHandled: Boolean; + begin + if SalesLine.FindSet() then + repeat + // The local initializes once, so reset it per iteration; a + // subscriber that handles one line must not skip the rest. + LineIsHandled := false; + OnBeforeApplyLineDiscount(SalesLine, LineIsHandled); + if not LineIsHandled then + SalesLine.Validate("Line Discount %", 5); + until SalesLine.Next() = 0; + end; + [IntegrationEvent(false, false)] local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) begin @@ -27,4 +42,9 @@ codeunit 50240 "IsHandled Init Good Sample" local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean) begin end; + + [IntegrationEvent(false, false)] + local procedure OnBeforeApplyLineDiscount(var SalesLine: Record "Sales Line"; var IsHandled: Boolean) + begin + end; } diff --git a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.md similarity index 89% rename from microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md rename to microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.md index 12eb39c..fba378b 100644 --- a/microsoft/knowledge/events/initialize-ishandled-to-false-before-publishing.md +++ b/microsoft/knowledge/events/reset-ishandled-only-when-the-value-can-carry-over.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: events -keywords: [ishandled, initialization, deterministic, onbefore, reset, integration-event, control-flow] +keywords: [ishandled, carry-over, loop-iteration, onbefore, reset, integration-event, control-flow, false-positive] technologies: [al] countries: [w1] application-area: [all] @@ -17,10 +17,10 @@ A routine that raises an `OnBefore…` integration event with a `var IsHandled: Reset `IsHandled := false;` before a raise only when the value might otherwise carry over as `true`: the same variable is reused after an earlier raise without a control-flow proof that it is false, a raise is re-entered by a loop, the value comes from an input parameter, field, or global, or earlier code seeds it. Prefer separate fresh locals when independent event seams need independent handled state. A reset on a guaranteed-false fresh local used by one non-looping raise, or before a later raise reached only after a semantically valid `if IsHandled then exit;`, can be retained for readability, but its absence is not a correctness finding. -See sample: `initialize-ishandled-to-false-before-publishing.good.al`. +See sample: `reset-ishandled-only-when-the-value-can-carry-over.good.al`. ## Anti Pattern Raising `OnBeforeX(…, IsHandled)` when the variable can still be `true` from an earlier raise, an earlier loop iteration, or another source, so the publisher call starts with stale state. Do not match a single non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false. -See sample: `initialize-ishandled-to-false-before-publishing.bad.al`. +See sample: `reset-ishandled-only-when-the-value-can-carry-over.bad.al`. diff --git a/microsoft/skills/review/al-events-review.md b/microsoft/skills/review/al-events-review.md index ebd2976..c854f1c 100644 --- a/microsoft/skills/review/al-events-review.md +++ b/microsoft/skills/review/al-events-review.md @@ -51,7 +51,7 @@ When the post-conflict worklist is empty because no applicable events knowledge The following targeted checks map diff signals to specific `events` articles. Treat each as a candidate-selection cue: when the signal appears in the changed code, add the named article to the worklist and evaluate it in Action. -- An `IsHandled` value that can carry over as `true` (reused after an earlier raise, re-entered on a later loop iteration, input/global/field, or otherwise seeded) is passed to a publisher without a reset — `initialize-ishandled-to-false-before-publishing`. Do not match one non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false. +- An `IsHandled` value that can carry over as `true` (reused after an earlier raise, re-entered on a later loop iteration, input/global/field, or otherwise seeded) is passed to a publisher without a reset — `reset-ishandled-only-when-the-value-can-carry-over`. Do not match one non-looping raise using a fresh local Boolean, or a later raise reached only after a semantically valid `if IsHandled then exit;` proves the value is false. - `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event later, so the after-event is skipped whenever the call is handled — `preserve-onafter-execution-when-ishandled-skips-the-body`. - Any parameter added to a public Business/Integration event procedure, regardless of position; do not flag additions or reordering on `local`/`internal` publishers merely because a new parameter was not appended — `add-new-event-parameters-at-the-end`. - A shipped Business/Integration event renamed or removed, or an existing parameter renamed, removed, retyped, or changed to/from `var`, based on the mistaken assumption that `local` or `internal` prevents dependent subscription; parameter order alone is not a subscriber-contract violation — `treat-local-and-internal-events-as-subscriber-contracts`. From c213f1495ec10079f957fbb11032170b5a1ed174 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Wed, 2 Sep 2026 11:46:13 +0200 Subject: [PATCH 4/4] Exempt optional notification handlers from the HandlerFunctions execution rule The narrowed UI-handler guidance still stated the execution rule without the qualifier the linked Microsoft reference uses. The article said every listed handler must execute at least once, and the testing leaf skill asked for `[HandlerFunctions(...)]` to match the invoked handlers exactly. The reference says every *nonoptional* listed handler must execute, and that send-notification and recall-notification handlers can be optional. As written, an agent could flag a deliberately unused optional notification handler. The discriminator is narrower than the handler type. Both `[SendNotificationHandler([HandlerIsOptional: Boolean])]` and `[RecallNotificationHandler([HandlerIsOptional: Boolean])]` take an explicit optionality argument, so `[SendNotificationHandler(true)]` is exempt while the same attribute written without the argument stays nonoptional like every other handler type. Keying the exemption on the argument rather than the type keeps it checkable from the diff and avoids the opposite false positive, where an agent stops flagging genuinely nonoptional notification handlers. Changes: - The article now states the nonoptional qualifier, explains that optionality is declared rather than inferred, and adds an explicit do-not-flag clause. That clause also forbids proposing removal, because the listed entry is what keeps the test passing on the runs where the notification does fire. - The testing leaf skill carries the same boundary in its `ui-handlers-in-tests` cue, and its mechanical-fix list no longer allows removing a listed optional notification handler as a one-click suggestion. - `SendNotificationHandler` and `RecallNotificationHandler` were missing from the skill's testing token list, so notification handlers were not reliably surfaced to the relevance step at all. Both are now listed. - The good sample gains a test that lists an unreached `[SendNotificationHandler(true)]`; the bad sample gains the mirror image, an unreached `[SendNotificationHandler]` with no optionality argument. The pair differs only by that argument, which is the point. - `evaluation/review-fixtures.json` pins the testing domain to `ui-handlers-in-tests` so the boundary is exercised: the good sample is the clean control at `minimumCleanRate` 1.0 and the bad sample is the expected finding. Keywords were retagged with `notification` and `optional-handler`. validate_frontmatter.py reports 0 errors; Test-ReviewFixtures.ps1 passes with 32 cases across 16 leaf domains and resolves the testing fixture to this article. --- evaluation/review-fixtures.json | 3 +++ .../testing/ui-handlers-in-tests.bad.al | 17 ++++++++++++++++ .../testing/ui-handlers-in-tests.good.al | 20 +++++++++++++++++++ .../knowledge/testing/ui-handlers-in-tests.md | 10 ++++++---- microsoft/skills/review/al-testing-review.md | 6 +++--- 5 files changed, 49 insertions(+), 7 deletions(-) diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index 68a646d..62fcf19 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -28,6 +28,9 @@ "telemetry": { "article": "telemetry-event-id-stable-unique" }, + "testing": { + "article": "ui-handlers-in-tests" + }, "upgrade": { "article": "initvalue-does-not-update-existing-rows", "context": "The extended table existed in the previous app version and already contains rows." diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al index d0832fe..47743e0 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.bad.al @@ -40,6 +40,17 @@ codeunit 50401 "Test UI Handler Proof Bad" Page.RunModal(Page::"Customer Card", Customer); end; + [Test] + [HandlerFunctions('CustomerCardHandler,MandatoryNotificationHandler')] + procedure UnreachedNonoptionalNotificationHandlerFailsAtRuntime() + var + Customer: Record Customer; + begin + LibrarySales.CreateCustomer(Customer); + + Page.RunModal(Page::"Customer Card", Customer); + end; + [ModalPageHandler] procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin @@ -51,6 +62,12 @@ codeunit 50401 "Test UI Handler Proof Bad" Reply := true; end; + [SendNotificationHandler] + procedure MandatoryNotificationHandler(var TheNotification: Notification): Boolean + begin + exit(true); + end; + var Assert: Codeunit "Library Assert"; LibrarySales: Codeunit "Library - Sales"; diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al index fafc515..753873f 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.good.al +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.good.al @@ -16,12 +16,32 @@ codeunit 50400 "Test UI Handler Capture Good" Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.'); end; + [Test] + [HandlerFunctions('CustomerCardHandler,CreditLimitNotificationHandler')] + procedure CustomerCardOpensForCustomerWithinCreditLimit() + var + Customer: Record Customer; + begin + LibrarySales.CreateCustomer(Customer); + CapturedCustomerNo := ''; + + Page.RunModal(Page::"Customer Card", Customer); + + Assert.AreEqual(Customer."No.", CapturedCustomerNo, 'The customer card opened for the wrong customer.'); + end; + [ModalPageHandler] procedure CustomerCardHandler(var CustomerCard: TestPage "Customer Card") begin CapturedCustomerNo := CustomerCard."No.".Value(); end; + [SendNotificationHandler(true)] + procedure CreditLimitNotificationHandler(var CreditLimitNotification: Notification): Boolean + begin + exit(true); + end; + var Assert: Codeunit "Library Assert"; LibrarySales: Codeunit "Library - Sales"; diff --git a/microsoft/knowledge/testing/ui-handlers-in-tests.md b/microsoft/knowledge/testing/ui-handlers-in-tests.md index 42a8d83..d451301 100644 --- a/microsoft/knowledge/testing/ui-handlers-in-tests.md +++ b/microsoft/knowledge/testing/ui-handlers-in-tests.md @@ -1,7 +1,7 @@ --- bc-version: [all] domain: testing -keywords: [handler, handlerfunctions, confirm, message, strmenu, variable-storage, enqueue, capture, runmodal, unhandled-ui] +keywords: [handler, handlerfunctions, confirm, message, notification, optional-handler, enqueue, capture, runmodal, unhandled-ui] technologies: [al] countries: [w1] application-area: [all] @@ -11,18 +11,20 @@ application-area: [all] ## Description -A test runs headless, so every UI call on the executed path must be intercepted by a matching handler named in `[HandlerFunctions(...)]`. The list is a two-sided contract: an unhandled UI call aborts the test, while Microsoft documents that [every listed handler must execute at least once](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/attributes/devenv-handlerfunctions-attribute#remarks) or the test fails. +A test runs headless, so every UI call on the executed path must be intercepted by a matching handler named in `[HandlerFunctions(...)]`. The list is a two-sided contract: an unhandled UI call aborts the test, while Microsoft documents that [every nonoptional listed handler must execute at least once](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/attributes/devenv-handlerfunctions-attribute#remarks) or the test fails. + +Optionality is declared, not inferred. `SendNotificationHandler` and `RecallNotificationHandler` accept a `HandlerIsOptional` argument, so `[SendNotificationHandler(true)]` may stay listed on a run that never raises the notification, while the same attribute written without that argument is nonoptional like every other handler type. Notifications are conditional by nature, so an optional notification handler is listed precisely because the scenario may or may not reach it. Beyond that wiring guarantee, the test must verify the behavior it cares about. The appropriate pattern depends on the contract: a handler can capture concrete page state or a result and the test can assert that semantic postcondition after `RunModal`; assertions inside a handler are also supported. Queue/enqueue/dequeue and `LibraryVariableStorage.AssertEmpty` are useful when interaction order, count, text, replies, or a scripted sequence is itself part of the contract, but they are not mandatory for every handler. ## Best Practice -List precisely the handlers the scenario triggers and make each handler contribute meaningful evidence. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. For ordered or repeated interactions, let the test enqueue expectations, let handlers dequeue and verify them, clear storage during initialization, and finish with `AssertEmpty`. +List the handlers the scenario triggers, keep an optional notification handler listed for a notification the scenario may conditionally raise, and make each executed handler contribute meaningful evidence. For a single modal page, reset a capture variable before the action, capture a concrete value from the page in the handler, and assert the expected value after `RunModal`. For ordered or repeated interactions, let the test enqueue expectations, let handlers dequeue and verify them, clear storage during initialization, and finish with `AssertEmpty`. See sample: `ui-handlers-in-tests.good.al`. ## Anti Pattern -Omitting a handler for a UI call, listing a handler the path never reaches, or claiming action success from a Boolean set before the action runs. A handler that only closes a page can also leave the test without a semantic assertion. Do not flag the absence of queue storage by itself; require it only when the test needs to prove interaction order, count, text, replies, or a scripted sequence. +Omitting a handler for a UI call, listing a nonoptional handler the path never reaches, or claiming action success from a Boolean set before the action runs. A handler that only closes a page can also leave the test without a semantic assertion. Do not flag the absence of queue storage by itself; require it only when the test needs to prove interaction order, count, text, replies, or a scripted sequence. Do not flag a listed `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` that the run does not reach, and never propose removing one: the entry is what keeps the test passing on the runs where the notification does fire. See sample: `ui-handlers-in-tests.bad.al`. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index fb5d50f..8bad734 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially codeunits with `Subtype = Test`, test runner codeunits with `TestIsolation`, test libraries, and codeunits that define UI handlers. - The changed methods and attributes, weighted toward `[Test]`, `[TransactionModel(...)]`, `[TestPermissions(...)]`, `[HandlerFunctions(...)]`, handler attributes, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, fixture initialization, and test-library calls. -- Tokens extracted from the diff that relate to testing (`Subtype = Test`, `Subtype = TestRunner`, `TestIsolation`, `TestPermissions`, `Restrictive`, `NonRestrictive`, `Disabled`, `Permissions Mock`, `Library - Lower Permissions`, `TransactionModel`, `AutoRollback`, `AutoCommit`, `Commit`, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, `HandlerFunctions`, `ConfirmHandler`, `MessageHandler`, `StrMenuHandler`, `ModalPageHandler`, `Enqueue`, `Dequeue`, `AssertEmpty`, `Library Assert`, `LibraryVariableStorage`, `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, `Init`, `Insert`). +- Tokens extracted from the diff that relate to testing (`Subtype = Test`, `Subtype = TestRunner`, `TestIsolation`, `TestPermissions`, `Restrictive`, `NonRestrictive`, `Disabled`, `Permissions Mock`, `Library - Lower Permissions`, `TransactionModel`, `AutoRollback`, `AutoCommit`, `Commit`, `asserterror`, `ExpectedError`, `ExpectedErrorCode`, `HandlerFunctions`, `ConfirmHandler`, `MessageHandler`, `StrMenuHandler`, `ModalPageHandler`, `SendNotificationHandler`, `RecallNotificationHandler`, `Enqueue`, `Dequeue`, `AssertEmpty`, `Library Assert`, `LibraryVariableStorage`, `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, `Init`, `Insert`). 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 object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no testing-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files. @@ -50,7 +50,7 @@ The following targeted checks cover every current `testing` article. Treat each - A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`. - Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`. - `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`. -- A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. +- A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Only nonoptional handlers have to execute: a listed handler declared `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` is optional by design, so do not treat it as unmatched when the run never raises the notification. Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`. @@ -74,7 +74,7 @@ Set `confidence` to: After evaluating each worklist entry, also consider whether the diff exhibits a testing defect the agent recognises from its general AL knowledge that no knowledge file in the worklist covers. Such candidates are agent findings within this skill's domain — emit them with `references: []`, an `id` slug prefixed with `agent:`, `confidence` capped at `medium`, `severity` capped at `minor` (agent findings are advisory and non-gating), and a `message` that is self-contained (describing both the issue and a concrete recommendation, since there is no knowledge-file footer for the consumer to fall back on). Hold every candidate to the precision bar in `skills/do.md` (*Agent findings*): emit only a concrete, material testing defect a knowledgeable BC reviewer would agree is wrong — steelman it first and drop anything stylistic, speculative, dependent on code outside the diff, or merely a valid alternative; when in doubt, omit. The scope is strictly AL testing; defects outside this domain belong to other leaves and MUST NOT be emitted here. Before emitting, check the worklist for a knowledge file that matches the candidate — if one exists, upgrade the candidate to a knowledge-backed finding instead. See `skills/do.md` for the full contract. -For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: add the matching `ExpectedError` assertion after `asserterror`; add or remove a handler name in `HandlerFunctions`; add `LibraryVariableStorage.Clear` or `AssertEmpty` when queue/LVS intentionally verifies interaction order, count, text, replies, or a scripted sequence; or replace hand-rolled fixture creation with an evident library call). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. +For every emitted finding, decide whether the fix is mechanical. A fix is mechanical when it is small, local, and unambiguous from the diff context (for example: add the matching `ExpectedError` assertion after `asserterror`; add or remove a handler name in `HandlerFunctions`, except that a listed optional notification handler must never be proposed for removal; add `LibraryVariableStorage.Clear` or `AssertEmpty` when queue/LVS intentionally verifies interaction order, count, text, replies, or a scripted sequence; or replace hand-rolled fixture creation with an evident library call). For mechanical findings, emit `findings[].suggested-code` with the literal replacement for the source lines indicated by `location`. The payload must be a verbatim replacement — no diff markers, no fences, no commentary — that the consumer can render as a one-click suggestion. When a `.good.al` companion exists and the diff context matches the `.bad.al` shape, adapt the `.good.al` replacement into `suggested-code`. Omit `suggested-code` only when the appropriate fix depends on context the skill cannot determine, when multiple defensible replacements exist, or when the fix spans non-contiguous code. If a finding is mechanical-looking but you omit `suggested-code`, set `findings[].suggested-code-omission-reason` to a short explanation. See `skills/do.md` for the full contract.