From ead337f9cbf237dd07532393afcb690c4ae160b3 Mon Sep 17 00:00:00 2001 From: wenjiefan Date: Tue, 18 Aug 2026 14:09:11 +0200 Subject: [PATCH] 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`.