mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 09:26:52 +01:00
Add events knowledge domain and review leaf skill (#43)
* Add events knowledge domain and review leaf skill Add a new `events` knowledge domain covering AL events & subscribers, wired into the AL review pipeline. - 3 atomic articles (+ .good.al/.bad.al samples) under microsoft/knowledge/events/: the IsHandled override pattern, thin OnBefore/OnAfter integration-event publishers, and static vs manual subscribers. - New leaf skill microsoft/skills/review/al-events-review.md sourcing the events domain. - Wired into microsoft/skills/review/al-code-review.md (sub-skills + Source + description) and README.md (leaf-skill count + domain list). AL event syntax verified against Microsoft Learn. Samples are demonstration-only (not compiled by CI). Additive change; no contract change. Part of #34. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add 12 general AL event-design articles to events domain Add 12 atomic knowledge articles under microsoft/knowledge/events covering general AL event-design best practices: IsHandled initialization and OnAfter preservation, appending new event parameters, position-based event naming, reusing/extending events, avoiding per-iteration publishing, Temp-prefixing temporary record parameters, unabbreviated parameter names, preferring the this keyword over IncludeSender, avoiding loosely typed parameters, not mutating existing event contracts, and not bypassing critical operations with IsHandled. Each article ships a .good.al and .bad.al demonstration sample (object IDs 50240-50296; not compiled by CI). Extend the al-events-review leaf Worklist with one targeted check per new rule. Additive only; no contract or wiring change (events leaf already wired). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Refine events articles after review feedback Correct wording in five events articles to reflect that AL event subscribers bind by parameter name, not position: - add-new-event-parameters-at-the-end: drop the inaccurate claim that appending a parameter forces subscribers to be updated or causes wrong values; keep the append-at-end best practice. - do-not-add-ishandled-to-an-existing-event: reframe from "breaking change" to the semantic/purpose shift that leaves existing subscribers pointless; rename the breaking-change keyword to semantic-change. - name-events-by-publisher-position: extend the good sample with position-named publishers raised from table and report trigger contexts. - initialize-ishandled-to-false-before-publishing: scope the detection and best practice to events that actually carry a var IsHandled, so an OnBefore with no IsHandled is not flagged. - do-not-bypass-critical-operations-with-ishandled: add a litmus-test definition of a critical operation (code that cannot stand as an independent, self-contained unit). Knowledge-only; no contract or wiring change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Soften Anti Pattern wording in add-new-event-parameters article Remove the last name-vs-position misconception from the Anti Pattern so it is consistent with the corrected Description: mid-list insertion is framed as noisy and harder to review rather than as forcing subscriber re-mapping. Detection sentence unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Add events domain reviewers to CODEOWNERS Add @AleksandricMarko and @pchriste-microsoft-com as required reviewers for the events knowledge domain, matching the existing per-domain expert ownership convention. Inserted in alphabetical order ahead of the performance line. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Jesper Schulz-Wedde <jesper.schulzwedde@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
Jesper Schulz-Wedde
GitHub
parent
23d5478ac6
commit
f19f0618fb
49 changed files with 1406 additions and 2 deletions
|
|
@ -0,0 +1,21 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50251 "Param Append Bad Sample"
|
||||
{
|
||||
procedure PostDocument(var SalesHeader: Record "Sales Header"; CalledFromBatch: Boolean)
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
IsHandled := false;
|
||||
// Anti-pattern: 'CalledFromBatch' was inserted before the existing
|
||||
// IsHandled parameter, shifting it and breaking the argument positions
|
||||
// every existing subscriber relied on.
|
||||
OnBeforePostDocument(SalesHeader, CalledFromBatch, IsHandled);
|
||||
if IsHandled then
|
||||
exit;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforePostDocument(var SalesHeader: Record "Sales Header"; CalledFromBatch: Boolean; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,20 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50250 "Param Append Good Sample"
|
||||
{
|
||||
procedure PostDocument(var SalesHeader: Record "Sales Header"; CalledFromBatch: Boolean)
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
IsHandled := false;
|
||||
// The new 'CalledFromBatch' parameter was appended at the end of the
|
||||
// existing signature, so existing subscribers needed no re-mapping.
|
||||
OnBeforePostDocument(SalesHeader, IsHandled, CalledFromBatch);
|
||||
if IsHandled then
|
||||
exit;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforePostDocument(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean; CalledFromBatch: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [event-parameters, signature, backward-compatibility, append, onbefore, integration-event, versioning]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Add new event parameters at the end
|
||||
|
||||
## Description
|
||||
|
||||
Adding a parameter to an existing event publisher changes its signature. Appending the new parameter at the end of the parameter list keeps the change easy to review and track: existing subscribers still bind to the leading parameters, and the diff is a single clean addition. Inserting a parameter in the middle makes diffs noisy and harder to review, and obscures the history of how the signature evolved. New parameters belong after the existing ones.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When extending an existing publisher, append the new parameter after all existing ones, including after a trailing `var IsHandled: Boolean` when present. Subscribers that already match keep working against the leading parameters, and the change stays a one-line addition that is trivial to review.
|
||||
|
||||
See sample: `add-new-event-parameters-at-the-end.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Inserting a new parameter in the middle of an existing event's signature, shifting every subsequent parameter and making the change noisy and harder to review. Detection: a changed event signature where an added parameter appears before existing parameters rather than at the tail of the list.
|
||||
|
||||
See sample: `add-new-event-parameters-at-the-end.bad.al`.
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50286 "Typed Param Bad Sample"
|
||||
{
|
||||
procedure ValidateQuantity(var SalesLine: Record "Sales Line"; xSalesLine: Record "Sales Line")
|
||||
var
|
||||
RecRef: RecordRef;
|
||||
begin
|
||||
// Anti-pattern: a RecordRef drops the table type and xRec is ambiguous
|
||||
// out of context, so subscribers lose type safety and a clear contract.
|
||||
RecRef.GetTable(SalesLine);
|
||||
OnAfterValidateQuantity(RecRef, xSalesLine);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterValidateQuantity(var RecRef: RecordRef; xSalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50285 "Typed Param Good Sample"
|
||||
{
|
||||
procedure ValidateQuantity(var SalesLine: Record "Sales Line"; PreviousQuantity: Decimal)
|
||||
begin
|
||||
// A concrete record plus the specific value needed: type-safe contract.
|
||||
OnAfterValidateQuantity(SalesLine, PreviousQuantity);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterValidateQuantity(var SalesLine: Record "Sales Line"; PreviousQuantity: Decimal)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [recordref, xrec, type-safety, event-parameters, strong-typing, integration-event, clarity]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Avoid loosely typed event parameters
|
||||
|
||||
## Description
|
||||
|
||||
Passing `RecordRef` or `xRec` as event parameters weakens the contract. A `RecordRef` parameter erases the table type, so subscribers must inspect at run time which table they received and can be handed an unexpected one, losing compile-time checking and direct field access. `xRec` — the previous version of a record — is context-dependent: it is meaningful inside a specific table or page trigger, but ambiguous once passed around as a parameter, and is often stale or empty outside the context that produced it. Prefer a concrete, strongly-typed record plus the specific values a subscriber actually needs, so the contract is explicit and the compiler enforces it.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Give events concrete record types and explicit values, such as `(SalesLine: Record "Sales Line"; PreviousQuantity: Decimal)`, instead of a `RecordRef` or an `xRec` parameter. Subscribers then get type safety, field access, and an unambiguous contract.
|
||||
|
||||
See sample: `avoid-loosely-typed-event-parameters.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Event parameters typed as `RecordRef` (no table type) or an `xRec`-style "previous record" (ambiguous, possibly stale) without strong justification. Detection: an event signature containing a `RecordRef` parameter, or a passed-through `xRec` record, where a concrete typed record and explicit values would serve.
|
||||
|
||||
See sample: `avoid-loosely-typed-event-parameters.bad.al`.
|
||||
|
|
@ -0,0 +1,53 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50232 "Order Event Pub Bad Sample"
|
||||
{
|
||||
procedure ReleaseOrder(OrderNo: Code[20])
|
||||
begin
|
||||
OnAfterReleaseOrder(OrderNo);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterReleaseOrder(OrderNo: Code[20])
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
||||
// Anti-pattern 1: a static subscriber drives an always-on side effect that
|
||||
// should be scoped. Every release now emails the customer, in every session
|
||||
// and every automated test, with no way to switch it off.
|
||||
codeunit 50233 "Always Email Sub Bad Sample"
|
||||
{
|
||||
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Order Event Pub Bad Sample", 'OnAfterReleaseOrder', '', false, false)]
|
||||
local procedure SendEmailOnRelease(OrderNo: Code[20])
|
||||
begin
|
||||
// Send a confirmation email unconditionally on every release.
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50234 "Scoped Sub Bad Sample"
|
||||
{
|
||||
EventSubscriberInstance = Manual;
|
||||
|
||||
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Order Event Pub Bad Sample", 'OnAfterReleaseOrder', '', false, false)]
|
||||
local procedure OverrideRelease(OrderNo: Code[20])
|
||||
begin
|
||||
// Scoped behaviour intended only for a specific flow.
|
||||
end;
|
||||
}
|
||||
|
||||
// Anti-pattern 2: a manual subscriber is bound and never unbound. Because the
|
||||
// instance is held on a SingleInstance global, the binding lives for the whole
|
||||
// session, so later unrelated releases keep hitting the scoped subscriber.
|
||||
codeunit 50235 "Leaky Binder Bad Sample"
|
||||
{
|
||||
SingleInstance = true;
|
||||
|
||||
var
|
||||
Scoped: Codeunit "Scoped Sub Bad Sample";
|
||||
|
||||
procedure ActivateOverride()
|
||||
begin
|
||||
BindSubscription(Scoped);
|
||||
// Missing: a matching UnbindSubscription(Scoped) when the scope ends.
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,52 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50228 "Item Post Pub Good Sample"
|
||||
{
|
||||
procedure PostItemLine(ItemNo: Code[20]; Qty: Decimal)
|
||||
begin
|
||||
// ... post the line ...
|
||||
OnAfterPostItemLine(ItemNo, Qty);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterPostItemLine(ItemNo: Code[20]; Qty: Decimal)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50229 "Item Post Audit Good Sample"
|
||||
{
|
||||
// Always-on behaviour belongs in a static subscriber (the default).
|
||||
EventSubscriberInstance = StaticAutomatic;
|
||||
|
||||
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Item Post Pub Good Sample", 'OnAfterPostItemLine', '', false, false)]
|
||||
local procedure LogPostedLine(ItemNo: Code[20]; Qty: Decimal)
|
||||
begin
|
||||
// Audit every posted line, unconditionally.
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50230 "Item Post Stub Good Sample"
|
||||
{
|
||||
// Scoped/temporary behaviour belongs in a manual subscriber.
|
||||
EventSubscriberInstance = Manual;
|
||||
|
||||
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Item Post Pub Good Sample", 'OnAfterPostItemLine', '', false, false)]
|
||||
local procedure CaptureForTest(ItemNo: Code[20]; Qty: Decimal)
|
||||
begin
|
||||
// Record the call so a single test can assert on it.
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50231 "Item Post Test Good Sample"
|
||||
{
|
||||
procedure VerifyPostingRaisesEvent()
|
||||
var
|
||||
Publisher: Codeunit "Item Post Pub Good Sample";
|
||||
Stub: Codeunit "Item Post Stub Good Sample";
|
||||
begin
|
||||
// Activate the scoped subscriber only for the duration of the test.
|
||||
BindSubscription(Stub);
|
||||
Publisher.PostItemLine('1000', 5);
|
||||
UnbindSubscription(Stub);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [event-subscriber, static-subscriber, manual-subscriber, bindsubscription, unbindsubscription, eventsubscriberinstance, scoped-binding]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Choose static vs manual subscribers deliberately and bind manual ones with BindSubscription
|
||||
|
||||
## Description
|
||||
|
||||
An `[EventSubscriber]` codeunit is static by default (`EventSubscriberInstance = StaticAutomatic`): it is always bound, so it fires for every raise of the event in every session. That is correct for always-on behaviour such as auditing, but wrong for behaviour that must be scoped — test isolation, a one-off migration, or a conditional override — because a static subscriber cannot be switched off. For scoped behaviour, set `EventSubscriberInstance = Manual` and activate the codeunit only while needed with `BindSubscription`, releasing it with `UnbindSubscription`. LLMs are largely unaware the manual model exists and default everything to static, producing always-on side effects that leak across unrelated operations and tests.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use a static subscriber for behaviour that genuinely applies all the time. For anything scoped, mark the codeunit `EventSubscriberInstance = Manual`, call `BindSubscription(SubscriberInstance)` at the start of the scope and `UnbindSubscription(SubscriberInstance)` at the end. A manual subscriber held only in a local variable unbinds automatically when that variable leaves scope, which suits test setup/teardown; a binding you intend to outlive a single call must be unbound explicitly. Keep subscriber methods `local` per CodeCop AA0207.
|
||||
|
||||
See sample: `choose-static-vs-manual-subscribers-deliberately.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Two shapes. First, a static subscriber used for behaviour that should be scoped — an always-on side effect (sending mail, writing extra records) that now fires for every event in every session and test with no way to disable it. Second, a manual subscriber that is bound with `BindSubscription` and never unbound: when the instance is held beyond the intended scope (for example on a `SingleInstance` codeunit), the binding leaks for the whole session and later unrelated operations keep hitting it. Detection: scoped side effects on a static subscriber, or a `BindSubscription` call with no matching `UnbindSubscription` and no scope that releases the instance.
|
||||
|
||||
See sample: `choose-static-vs-manual-subscribers-deliberately.bad.al`.
|
||||
|
|
@ -0,0 +1,21 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50291 "New OnBefore Bad Sample"
|
||||
{
|
||||
procedure CalculateTotal(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
Total: Decimal;
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
Total := 100;
|
||||
|
||||
// Anti-pattern: IsHandled was bolted onto the existing
|
||||
// OnAfterCalculateTotal, changing its contract and breaking every
|
||||
// subscriber that matched the original signature.
|
||||
OnAfterCalculateTotal(SalesHeader, Total, IsHandled);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterCalculateTotal(var SalesHeader: Record "Sales Header"; Total: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50290 "New OnBefore Good Sample"
|
||||
{
|
||||
procedure CalculateTotal(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
Total: Decimal;
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
// New overridable seam added as a separate event; the existing
|
||||
// OnAfterCalculateTotal keeps its original signature and subscribers.
|
||||
IsHandled := false;
|
||||
OnBeforeCalculateTotal(SalesHeader, IsHandled);
|
||||
if not IsHandled then
|
||||
Total := 100;
|
||||
|
||||
OnAfterCalculateTotal(SalesHeader, Total);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeCalculateTotal(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterCalculateTotal(var SalesHeader: Record "Sales Header"; Total: Decimal)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [ishandled, semantic-change, event-contract, backward-compatibility, onbefore, integration-event, subscribers]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not add IsHandled to an existing event
|
||||
|
||||
## Description
|
||||
|
||||
Adding a `var IsHandled: Boolean` parameter to an event that already shipped without one silently changes the event's purpose — from a plain notification into an overridable seam. Existing subscribers were written against a "notify" contract they never agreed to make skippable, so their behaviour can quietly become wrong or pointless. The safe move is to leave the existing event untouched and introduce a new `OnBefore…` event carrying `IsHandled` at the point you want to make overridable. Existing subscribers keep working against the original event; new subscribers opt into the override seam through the new one.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Keep the existing event as-is and add a separate `OnBeforeX(…; var IsHandled: Boolean)` before the logic you want to make overridable. Two events with distinct, stable contracts are safer than one event whose meaning and signature were changed under its subscribers.
|
||||
|
||||
See sample: `do-not-add-ishandled-to-an-existing-event.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Mutating a shipped event — for example adding `var IsHandled` to `OnAfterCalculateTotal` — to retrofit override behaviour, which overloads the event's meaning and undermines existing subscribers. Detection: an `IsHandled` parameter added to a pre-existing event signature rather than introduced through a new dedicated `OnBefore` publisher.
|
||||
|
||||
See sample: `do-not-add-ishandled-to-an-existing-event.bad.al`.
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50296 "Critical Op Bad Sample"
|
||||
{
|
||||
procedure PostInvoice(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
// Anti-pattern: IsHandled wraps the entire posting. A subscriber can set
|
||||
// IsHandled := true and silently skip ledger-entry creation and the
|
||||
// status update, leaving imbalanced ledgers and orphaned documents.
|
||||
IsHandled := false;
|
||||
OnBeforePostInvoice(SalesHeader, IsHandled);
|
||||
if IsHandled then
|
||||
exit;
|
||||
|
||||
CreateCustomerLedgerEntry(SalesHeader);
|
||||
SalesHeader.Status := SalesHeader.Status::Released;
|
||||
SalesHeader.Modify(true);
|
||||
end;
|
||||
|
||||
local procedure CreateCustomerLedgerEntry(var SalesHeader: Record "Sales Header")
|
||||
begin
|
||||
// Posts the customer ledger entry (critical; must never be skipped).
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforePostInvoice(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,38 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50295 "Critical Op Good Sample"
|
||||
{
|
||||
procedure PostInvoice(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
DiscountAmount: Decimal;
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
// IsHandled guards only a safe, side-effect-free calculation.
|
||||
IsHandled := false;
|
||||
OnBeforeCalculateInvoiceDiscount(SalesHeader, DiscountAmount, IsHandled);
|
||||
if not IsHandled then
|
||||
DiscountAmount := 10;
|
||||
SalesHeader."Invoice Discount Amount" := DiscountAmount;
|
||||
|
||||
// Critical operations always run; no subscriber can bypass them.
|
||||
CreateCustomerLedgerEntry(SalesHeader);
|
||||
SalesHeader.Status := SalesHeader.Status::Released;
|
||||
SalesHeader.Modify(true);
|
||||
|
||||
OnAfterPostInvoice(SalesHeader);
|
||||
end;
|
||||
|
||||
local procedure CreateCustomerLedgerEntry(var SalesHeader: Record "Sales Header")
|
||||
begin
|
||||
// Posts the customer ledger entry (critical; must never be skipped).
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeCalculateInvoiceDiscount(var SalesHeader: Record "Sales Header"; var DiscountAmount: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterPostInvoice(var SalesHeader: Record "Sales Header")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [ishandled, critical-operations, posting, data-integrity, ledger, integration-event, safety]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not bypass critical operations with IsHandled
|
||||
|
||||
## Description
|
||||
|
||||
The IsHandled override pattern lets a subscriber skip the guarded code entirely. A critical operation is one that cannot stand as an independent, self-contained unit — code whose partial execution or omission leaves the system inconsistent (imbalanced ledgers, orphaned documents, gaps in a number series, or skipped permission checks). That is acceptable around a pure, side-effect-free calculation, but dangerous around critical operations — posting, ledger-entry creation, number-series consumption, and referential-integrity or permission validation. Wrapping those in `OnBeforeX(…; var IsHandled); if IsHandled then exit;` lets any subscriber silently suppress them, risking imbalanced ledgers, orphaned documents, skipped permission checks, or duplicated numbers — corruption that surfaces far from the subscriber that caused it. Make the calculation overridable, not the commit: expose the value computation through IsHandled, or offer a regular `OnAfter…` event to adjust results, while the critical work runs unconditionally.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Scope IsHandled to a safe value-calculation block and run the critical operations unconditionally afterwards; or expose a positive `OnAfter…` event for subscribers to adjust results, rather than a bypass around the commit.
|
||||
|
||||
See sample: `do-not-bypass-critical-operations-with-ishandled.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An `OnBefore…` IsHandled guard wrapping a posting or ledger routine — `if IsHandled then exit;` around the code that creates ledger entries and updates document status — letting subscribers skip the commit. Detection: an `if IsHandled then exit;` whose skipped body performs posting, ledger writes, number-series consumption, or integrity and permission validation.
|
||||
|
||||
See sample: `do-not-bypass-critical-operations-with-ishandled.bad.al`.
|
||||
|
|
@ -0,0 +1,22 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50266 "Loop Event Bad Sample"
|
||||
{
|
||||
procedure ProcessLines(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
if SalesLine.FindSet() then
|
||||
repeat
|
||||
// Anti-pattern: an event raised on every iteration. Each
|
||||
// subscriber runs once per line, so the cost scales with the
|
||||
// row count and large batches can time out.
|
||||
OnProcessLine(SalesLine);
|
||||
|
||||
SalesLine."Line Amount" := SalesLine.Quantity * SalesLine."Unit Price";
|
||||
SalesLine.Modify(true);
|
||||
until SalesLine.Next() = 0;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnProcessLine(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,28 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50265 "Loop Event Good Sample"
|
||||
{
|
||||
procedure ProcessLines(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
// Fire once before the loop; subscribers act on the whole set.
|
||||
OnBeforeProcessLines(SalesLine);
|
||||
|
||||
if SalesLine.FindSet() then
|
||||
repeat
|
||||
SalesLine."Line Amount" := SalesLine.Quantity * SalesLine."Unit Price";
|
||||
SalesLine.Modify(true);
|
||||
until SalesLine.Next() = 0;
|
||||
|
||||
// Fire once after the loop.
|
||||
OnAfterProcessLines(SalesLine);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeProcessLines(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterProcessLines(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [performance, loops, event-publishing, batch, onbefore, onafter, subscriber-cost]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Do not publish events inside loops
|
||||
|
||||
## Description
|
||||
|
||||
Raising an event on every iteration of a loop multiplies the cost of every subscriber by the number of records. A subscriber doing even a little work per call can turn a fast batch into a timeout when the loop runs over thousands of rows, and the publisher has no control over how expensive a subscriber is. Unless a genuine per-row hook is required, publish once before the loop and once after it, passing enough context — filters, a key, or a buffer — for subscribers to act on the whole set at once. Generated code tends to drop an event inside the `repeat … until` without weighing the per-iteration multiplier.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Raise `OnBeforeProcessLines` before the loop and `OnAfterProcessLines` after it, outside the `repeat … until`, so each subscriber runs once per batch rather than once per row. Give those events the record or filters they need to operate on the whole set.
|
||||
|
||||
See sample: `do-not-publish-events-inside-loops.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
An event raised inside the loop body, fired once per iteration, so subscriber cost scales with the row count and large batches slow down or time out. Detection: an `OnBefore…`/`OnAfter…`/`On…` raise located between `repeat` and `until` in a record loop.
|
||||
|
||||
See sample: `do-not-publish-events-inside-loops.bad.al`.
|
||||
|
|
@ -0,0 +1,31 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50241 "IsHandled Init Bad Sample"
|
||||
{
|
||||
procedure ApplyDiscounts(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
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.
|
||||
OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled);
|
||||
if not IsHandled then
|
||||
DiscountPct += 2;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,31 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50240 "IsHandled Init Good Sample"
|
||||
{
|
||||
procedure ApplyDiscounts(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
DiscountPct: Decimal;
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
IsHandled := false;
|
||||
OnBeforeApplyHeaderDiscount(SalesHeader, DiscountPct, IsHandled);
|
||||
if not IsHandled then
|
||||
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;
|
||||
OnBeforeApplyPaymentDiscount(SalesHeader, DiscountPct, IsHandled);
|
||||
if not IsHandled then
|
||||
DiscountPct += 2;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeApplyHeaderDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeApplyPaymentDiscount(var SalesHeader: Record "Sales Header"; var DiscountPct: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [ishandled, initialization, deterministic, onbefore, reset, integration-event, control-flow]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Initialize IsHandled to false before publishing
|
||||
|
||||
## 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.
|
||||
|
||||
## 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.
|
||||
|
||||
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;`.
|
||||
|
||||
See sample: `initialize-ishandled-to-false-before-publishing.bad.al`.
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50276 "Param Naming Bad Sample"
|
||||
{
|
||||
procedure RegisterPayment(var SalesHdr: Record "Sales Header"; DocNo: Code[20]; Amt: Decimal)
|
||||
begin
|
||||
// Anti-pattern: abbreviated parameter names force every subscriber to
|
||||
// guess what SalesHdr, DocNo and Amt mean.
|
||||
OnAfterRegisterPayment(SalesHdr, DocNo, Amt);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterRegisterPayment(var SalesHdr: Record "Sales Header"; DocNo: Code[20]; Amt: Decimal)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50275 "Param Naming Good Sample"
|
||||
{
|
||||
procedure RegisterPayment(var SalesHeader: Record "Sales Header"; DocumentNo: Code[20]; Amount: Decimal)
|
||||
begin
|
||||
// Full, spelled-out names make the event contract self-explanatory.
|
||||
OnAfterRegisterPayment(SalesHeader, DocumentNo, Amount);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterRegisterPayment(var SalesHeader: Record "Sales Header"; DocumentNo: Code[20]; Amount: Decimal)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [parameter-naming, readability, conventions, event-parameters, no-abbreviations, integration-event, clarity]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Name event parameters without abbreviations
|
||||
|
||||
## Description
|
||||
|
||||
Event parameter names are part of the public contract a subscriber codes against, so they must be self-explanatory. Record parameters take the full table name with the spaces removed — `SalesHeader` for `"Sales Header"`, not `SalesHdr` or `SH`. Simple parameters get a descriptive, spelled-out name — `DocumentNo`, not `DocNo`; `Amount`, not `Amt`. Abbreviated names force every subscriber author to guess intent and tend to be inconsistent across a codebase, where the same concept appears under several contractions. The cost of a clear name is paid once at the publisher; the cost of a cryptic one is paid by every subscriber that has to decode it.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Use full, unabbreviated names: `(SalesHeader: Record "Sales Header"; DocumentNo: Code[20]; Amount: Decimal)`. Record parameters mirror the table name without spaces, and value parameters read as whole words so the contract is unambiguous.
|
||||
|
||||
See sample: `name-event-parameters-without-abbreviations.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Abbreviated parameter names (`SalesHdr`, `DocNo`, `Amt`) that obscure meaning and vary across publishers, so subscribers must guess what each one holds. Detection: event parameters whose names are truncated forms of the table name or contracted words rather than the full term.
|
||||
|
||||
See sample: `name-event-parameters-without-abbreviations.bad.al`.
|
||||
|
|
@ -0,0 +1,35 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50256 "Event Naming Bad Sample"
|
||||
{
|
||||
procedure PostSalesLine(var SalesLine: Record "Sales Line")
|
||||
var
|
||||
LineAmount: Decimal;
|
||||
begin
|
||||
// Anti-pattern: names don't encode the host routine or the
|
||||
// before/after position, so subscribers can't tell when they fire.
|
||||
BeforePost(SalesLine);
|
||||
|
||||
LineAmount := SalesLine.Quantity * SalesLine."Unit Price";
|
||||
MyCustomSalesEvent(SalesLine, LineAmount);
|
||||
|
||||
SalesLine."Line Amount" := LineAmount;
|
||||
SalesLine.Modify(true);
|
||||
|
||||
SalesLineEvent(SalesLine);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure BeforePost(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure MyCustomSalesEvent(var SalesLine: Record "Sales Line"; var LineAmount: Decimal)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure SalesLineEvent(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,64 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50255 "Event Naming Good Sample"
|
||||
{
|
||||
procedure PostSalesLine(var SalesLine: Record "Sales Line")
|
||||
var
|
||||
LineAmount: Decimal;
|
||||
begin
|
||||
// Start of the routine: OnBefore<Name>.
|
||||
OnBeforePostSalesLine(SalesLine);
|
||||
|
||||
LineAmount := SalesLine.Quantity * SalesLine."Unit Price";
|
||||
// Middle of the routine: On<Name>OnAfter<Context>.
|
||||
OnPostSalesLineOnAfterCalcAmounts(SalesLine, LineAmount);
|
||||
|
||||
SalesLine."Line Amount" := LineAmount;
|
||||
SalesLine.Modify(true);
|
||||
|
||||
// End of the routine: OnAfter<Name>.
|
||||
OnAfterPostSalesLine(SalesLine);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforePostSalesLine(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnPostSalesLineOnAfterCalcAmounts(var SalesLine: Record "Sales Line"; var LineAmount: Decimal)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterPostSalesLine(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
|
||||
// Same position-naming convention applies to events raised from table and
|
||||
// report triggers, not just codeunit procedures.
|
||||
|
||||
// Raised at the end of a table field's OnValidate trigger (for example
|
||||
// Customer."No." OnValidate): the position is "after", so OnAfter<Field>.
|
||||
procedure HandleCustomerNoValidated(var Customer: Record Customer)
|
||||
begin
|
||||
OnAfterValidateCustomerNo(Customer);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterValidateCustomerNo(var Customer: Record Customer)
|
||||
begin
|
||||
end;
|
||||
|
||||
// Raised before a report prints a line from its processing trigger (for
|
||||
// example a dataitem OnAfterGetRecord): the position is "before", so
|
||||
// OnBefore<Action>.
|
||||
procedure HandleReportLineProcessing(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
OnBeforeReportPrintLine(SalesLine);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeReportPrintLine(var SalesLine: Record "Sales Line")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [event-naming, onbefore, onafter, conventions, discoverability, integration-event, publisher]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Name events by publisher position
|
||||
|
||||
## Description
|
||||
|
||||
An event name should tell a subscriber where in the publisher the event fires. The convention encodes the position: an event at the very start of a procedure or trigger is `OnBefore<Name>`; one at the very end is `OnAfter<Name>`; one in the middle names both the host routine and the local boundary, as `On<Name>OnBefore<Context>` or `On<Name>OnAfter<Context>`. Consistent, position-encoding names make events discoverable and predictable, and let developers and tooling reason about firing order without reading the publisher. Ad-hoc names such as `MyCustomEvent` or `BeforePost` hide where the event fires and break the conventions the ecosystem relies on.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Name by position: `OnBeforePostSalesLine` and `OnAfterPostSalesLine` at the routine boundaries, and `OnPostSalesLineOnAfterCalcAmounts` for an event raised partway through `PostSalesLine` after an amount calculation. The name alone then tells a subscriber both the host routine and the exact point it runs.
|
||||
|
||||
See sample: `name-events-by-publisher-position.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Ad-hoc event names that omit the host routine or the before/after position (`MyCustomSalesEvent`, `BeforePost`, `SalesLineEvent`), leaving subscribers unable to tell when the event fires relative to the publisher's logic. Detection: publisher names that do not follow the `OnBefore`/`OnAfter<Routine>` or `On<Routine>OnBefore`/`OnAfter<Context>` patterns.
|
||||
|
||||
See sample: `name-events-by-publisher-position.bad.al`.
|
||||
|
|
@ -0,0 +1,27 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50261 "Reuse Event Bad Sample"
|
||||
{
|
||||
procedure ProcessOrder(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20])
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
IsHandled := false;
|
||||
// Anti-pattern: a near-duplicate event raised right next to the original,
|
||||
// differing only by an extra parameter — two consecutive events where a
|
||||
// single extended event would do.
|
||||
OnBeforeProcessOrder(SalesHeader, IsHandled);
|
||||
OnBeforeProcessOrderWithCustomer(SalesHeader, CustomerNo, IsHandled);
|
||||
if IsHandled then
|
||||
exit;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeProcessOrder(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeProcessOrderWithCustomer(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20]; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,20 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50260 "Reuse Event Good Sample"
|
||||
{
|
||||
procedure ProcessOrder(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20])
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
IsHandled := false;
|
||||
// A single event, extended with CustomerNo appended at the end, covers
|
||||
// the need; no second event is raised beside it.
|
||||
OnBeforeProcessOrder(SalesHeader, CustomerNo, IsHandled);
|
||||
if IsHandled then
|
||||
exit;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeProcessOrder(var SalesHeader: Record "Sales Header"; CustomerNo: Code[20]; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [event-reuse, duplication, consecutive-events, extension-point, onbefore, integration-event, maintainability]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Prefer reusing or extending existing events
|
||||
|
||||
## Description
|
||||
|
||||
Before adding a publisher, check whether an event already fires at that point in the code. Two related smells signal that you should reuse or extend instead of adding one. The first is a brand-new event placed directly next to an existing one — two consecutive event raises with no logic between them, which gives subscribers two seams where one belongs. The second is a near-duplicate event that differs from an existing one only by an extra parameter. Both bloat the publisher surface and leave subscribers unsure which event to pick. Prefer subscribing to the existing event, or extending it by appending the parameter you need, over introducing a parallel one.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When the data you need is already exposed at an existing event, subscribe to it. When the event lacks a parameter, extend that event by appending the parameter at the end — one publisher, one raise — rather than adding a second event beside it.
|
||||
|
||||
See sample: `prefer-reusing-or-extending-existing-events.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Adding a second event raise immediately after an existing one, or creating `OnBeforeProcessOrderWithCustomer` next to `OnBeforeProcessOrder` just to add a single parameter. Detection: two consecutive `OnBefore…`/`OnAfter…` raises with no logic between them, or near-duplicate event names differing only by a parameter-describing suffix.
|
||||
|
||||
See sample: `prefer-reusing-or-extending-existing-events.bad.al`.
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50281 "Sender This Bad Sample"
|
||||
{
|
||||
procedure ProcessOrder(OrderNo: Code[20])
|
||||
begin
|
||||
OnBeforeProcessOrder(OrderNo);
|
||||
end;
|
||||
|
||||
// Anti-pattern: IncludeSender = true is used only to expose the publisher
|
||||
// instance to subscribers; a codeunit can pass 'this' explicitly instead.
|
||||
[IntegrationEvent(true, false)]
|
||||
local procedure OnBeforeProcessOrder(OrderNo: Code[20])
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50280 "Sender This Good Sample"
|
||||
{
|
||||
procedure ProcessOrder(OrderNo: Code[20])
|
||||
begin
|
||||
// Pass the current instance explicitly as a typed Sender parameter.
|
||||
OnBeforeProcessOrder(OrderNo, this);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeProcessOrder(OrderNo: Code[20]; Sender: Codeunit "Sender This Good Sample")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [25..]
|
||||
domain: events
|
||||
keywords: [this-keyword, includesender, sender, codeunit, self-reference, integration-event, type-safety]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Prefer this over IncludeSender in codeunit events
|
||||
|
||||
## Description
|
||||
|
||||
Some publishers set `IncludeSender` to `true` on `[IntegrationEvent]` or `[BusinessEvent]` so subscribers receive the publishing object as an implicit sender parameter. From Business Central 2024 release wave 2, a codeunit can instead pass itself explicitly with the `this` keyword as a normal, strongly-typed `Sender` parameter. Explicit passing is clearer at both the publisher and the subscriber: the sender appears in the signature, it is concretely typed to the publishing codeunit, and it avoids the implicit-parameter mechanics of `IncludeSender`. Reserve `IncludeSender = true` for cases where the sender genuinely cannot be passed explicitly. This guidance applies to code targeting Business Central 2024 release wave 2 or later, where the `this` keyword is available.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Declare the publisher `[IntegrationEvent(false, false)]` with an explicit `Sender: Codeunit "…"` parameter and raise it with `this`, for example `OnBeforeProcessOrder(OrderNo, this);`. Subscribers then receive a typed sender they can call directly.
|
||||
|
||||
See sample: `prefer-this-over-includesender-in-codeunit-events.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Relying on `[IntegrationEvent(true, …)]` solely to hand subscribers the publisher instance, where a codeunit could pass `this` explicitly as a typed parameter. Detection: `IncludeSender = true` on a codeunit event whose only purpose is to expose the sender, in code targeting Business Central 2024 release wave 2 or later.
|
||||
|
||||
See sample: `prefer-this-over-includesender-in-codeunit-events.bad.al`.
|
||||
|
|
@ -0,0 +1,16 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50271 "Temp Param Bad Sample"
|
||||
{
|
||||
procedure SummarizeLines(var SalesLineBuffer: Record "Sales Line" temporary)
|
||||
begin
|
||||
// Anti-pattern: the parameter is temporary but isn't named with a Temp
|
||||
// prefix, so subscribers can't tell the data isn't persisted and may
|
||||
// rely on writes that are discarded.
|
||||
OnAfterSummarizeLines(SalesLineBuffer);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterSummarizeLines(var SalesLineBuffer: Record "Sales Line" temporary)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,14 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50270 "Temp Param Good Sample"
|
||||
{
|
||||
procedure SummarizeLines(var TempSalesLineBuffer: Record "Sales Line" temporary)
|
||||
begin
|
||||
// The Temp prefix tells subscribers the buffer isn't persisted.
|
||||
OnAfterSummarizeLines(TempSalesLineBuffer);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterSummarizeLines(var TempSalesLineBuffer: Record "Sales Line" temporary)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [temporary-record, naming, event-parameters, buffer, temp-prefix, integration-event, conventions]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Prefix temporary record event parameters with Temp
|
||||
|
||||
## Description
|
||||
|
||||
When a record passed to an event is a temporary record — an in-memory buffer not persisted to the database — its parameter name must start with `Temp`. The prefix is the only reliable signal a subscriber has that writes to the record will not reach the database and that the data is scoped to the current call. Without it, subscribers may treat buffer data as persisted: calling `Modify` or `Insert` expecting durability, or reading it as the authoritative table, which leads to silent data loss and confusing behaviour. The `temporary` keyword sits on the variable declaration and is not visible at the subscriber, so the name has to carry the meaning.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Name temporary record parameters with a `Temp` prefix, for example `var TempSalesLineBuffer: Record "Sales Line" temporary`, so every subscriber sees immediately that the record is an in-memory buffer and treats writes accordingly.
|
||||
|
||||
See sample: `prefix-temporary-record-event-parameters-with-temp.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A temporary record parameter named without the `Temp` prefix (`var SalesLineBuffer: Record "Sales Line" temporary`), so subscribers cannot tell the record is non-persistent and may rely on writes that are silently discarded. Detection: an event parameter declared `temporary` whose name does not start with `Temp`.
|
||||
|
||||
See sample: `prefix-temporary-record-event-parameters-with-temp.bad.al`.
|
||||
|
|
@ -0,0 +1,32 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50246 "OnAfter Preserve Bad Sample"
|
||||
{
|
||||
procedure ReleaseDocument(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
IsHandled := false;
|
||||
OnBeforeReleaseDocument(SalesHeader, IsHandled);
|
||||
|
||||
// Bug: returning here also skips OnAfterReleaseDocument below, so
|
||||
// subscribers that rely on the after-event stop running whenever
|
||||
// another extension handles the OnBefore.
|
||||
if IsHandled then
|
||||
exit;
|
||||
|
||||
SalesHeader.Status := SalesHeader.Status::Released;
|
||||
SalesHeader.Modify(true);
|
||||
|
||||
OnAfterReleaseDocument(SalesHeader);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeReleaseDocument(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterReleaseDocument(var SalesHeader: Record "Sales Header")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,30 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50245 "OnAfter Preserve Good Sample"
|
||||
{
|
||||
procedure ReleaseDocument(var SalesHeader: Record "Sales Header")
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
IsHandled := false;
|
||||
OnBeforeReleaseDocument(SalesHeader, IsHandled);
|
||||
|
||||
// Skip only the default body, not the routine, so OnAfter still fires.
|
||||
if not IsHandled then begin
|
||||
SalesHeader.Status := SalesHeader.Status::Released;
|
||||
SalesHeader.Modify(true);
|
||||
end;
|
||||
|
||||
// Fires whether or not a subscriber handled the body above.
|
||||
OnAfterReleaseDocument(SalesHeader);
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeReleaseDocument(var SalesHeader: Record "Sales Header"; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterReleaseDocument(var SalesHeader: Record "Sales Header")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [ishandled, onafter, event-pairing, control-flow, guard, integration-event, side-effects]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Preserve OnAfter execution when IsHandled skips the body
|
||||
|
||||
## Description
|
||||
|
||||
A routine that exposes both an `OnBefore…` event (with `var IsHandled`) and a paired `OnAfter…` event has a subtle trap. The common `if IsHandled then exit;` guard returns from the whole routine, so when a subscriber handles the OnBefore the OnAfter event never fires. Subscribers that depend on OnAfter — logging, downstream integration, dependent updates — then silently stop running whenever some other extension overrides the body. The fix is to skip only the default body, not the routine, so the OnAfter still publishes. The two seams are independent: overriding the work should not cancel the notification that the work happened.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Wrap only the default work in `if not IsHandled then begin … end;` and keep the `OnAfterX(…)` raise after that block, outside the guard, so it always fires regardless of whether a subscriber handled the OnBefore. This keeps the override seam and the after-notification independent, which is what subscribers expect.
|
||||
|
||||
See sample: `preserve-onafter-execution-when-ishandled-skips-the-body.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Guarding with `if IsHandled then exit;` and placing the `OnAfterX` raise later in the same routine, so handling the OnBefore short-circuits the whole procedure and the OnAfter event is skipped along with the body. Detection: an `if IsHandled then exit;` in a routine that also raises a paired `OnAfter…` event after that point.
|
||||
|
||||
See sample: `preserve-onafter-execution-when-ishandled-skips-the-body.bad.al`.
|
||||
|
|
@ -0,0 +1,36 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
table 50226 "Reservation Entry Bad Sample"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Entry No."; Integer) { }
|
||||
field(2; "Item No."; Code[20]) { }
|
||||
field(3; Quantity; Decimal) { }
|
||||
field(4; Reserved; Boolean) { }
|
||||
}
|
||||
keys
|
||||
{
|
||||
key(PK; "Entry No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50227 "Reservation Post Bad Sample"
|
||||
{
|
||||
procedure Reserve(var ReservationEntry: Record "Reservation Entry Bad Sample")
|
||||
begin
|
||||
// Anti-pattern: the operation exposes no OnBefore/OnAfter seam, and the
|
||||
// logic that should be the routine's own work lives in the event body
|
||||
// below instead. Partners must overwrite this routine to change it.
|
||||
OnReserve(ReservationEntry);
|
||||
end;
|
||||
|
||||
// Anti-pattern: business logic inside an integration-event publisher. A
|
||||
// publisher must be a thin, empty hook; logic placed here runs on every
|
||||
// raise and cannot be overridden, which defeats the event entirely.
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnReserve(var ReservationEntry: Record "Reservation Entry Bad Sample")
|
||||
begin
|
||||
ReservationEntry.Reserved := true;
|
||||
ReservationEntry.Modify(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,43 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
table 50224 "Reservation Entry Sample"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Entry No."; Integer) { }
|
||||
field(2; "Item No."; Code[20]) { }
|
||||
field(3; Quantity; Decimal) { }
|
||||
field(4; Reserved; Boolean) { }
|
||||
}
|
||||
keys
|
||||
{
|
||||
key(PK; "Entry No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50225 "Reservation Post Good Sample"
|
||||
{
|
||||
procedure Reserve(var ReservationEntry: Record "Reservation Entry Sample")
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
OnBeforeReserve(ReservationEntry, IsHandled);
|
||||
if IsHandled then
|
||||
exit;
|
||||
|
||||
ReservationEntry.Reserved := true;
|
||||
ReservationEntry.Modify(true);
|
||||
|
||||
OnAfterReserve(ReservationEntry);
|
||||
end;
|
||||
|
||||
// Thin publishers: empty bodies, the calling routine owns the logic.
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeReserve(var ReservationEntry: Record "Reservation Entry Sample"; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnAfterReserve(var ReservationEntry: Record "Reservation Entry Sample")
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [integration-event, onbefore, onafter, extension-point, thin-publisher, publisher-body, extensibility]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Publish thin OnBefore/OnAfter integration events to expose extension points
|
||||
|
||||
## Description
|
||||
|
||||
A key operation — a posting, release, or validation routine — becomes a hard wall for partners when it ships no integration events: the only way to change it is to overwrite or duplicate the base code. The Business Central remedy is to raise thin `OnBeforeX`/`OnAfterX` integration events at the operation's boundaries, passing `var Rec` and the relevant parameters so subscribers have what they need. An equally common defect is the inverse: putting business logic *inside* the publisher method body. An event publisher is a hook, not a procedure — its body must be empty, and the platform even forbids variables, return values, and code other than comments in it. LLMs both omit the extension points and, when they do add an event, wrongly fill its body with logic.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Wrap the operation's core with events: raise `OnBeforeX(var Rec, var IsHandled)` before the default work and `OnAfterX(var Rec)` once it succeeds, at the natural boundaries of the routine. Declare each publisher `[IntegrationEvent(false, false)] local procedure` with an empty body and let the calling routine — never the publisher — own the logic. Pass records by `var` so subscribers can read and adjust them, and include the parameters a subscriber would need to act. This gives partners a stable seam without touching base code.
|
||||
|
||||
See sample: `publish-thin-onbefore-onafter-integration-events.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Business logic placed inside an `[IntegrationEvent]` publisher method, so the "event" actually mutates state every time it is raised — defeating the hook and surprising every reader — or a core operation that exposes no extension points at all, forcing partners to overwrite or duplicate it. Detection: an `[IntegrationEvent]`/`[BusinessEvent]` method whose body contains statements rather than being empty, or a posting/validation routine with no surrounding `OnBefore`/`OnAfter` publishers.
|
||||
|
||||
See sample: `publish-thin-onbefore-onafter-integration-events.bad.al`.
|
||||
|
|
@ -0,0 +1,38 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
|
||||
// Anti-pattern 1: no OnBefore/IsHandled hook. A partner cannot replace this
|
||||
// rule without overwriting base code, so the behaviour is not extensible.
|
||||
codeunit 50222 "Shipping Charge NoHook Bad"
|
||||
{
|
||||
procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal
|
||||
begin
|
||||
if OrderAmount >= 1000 then
|
||||
Charge := 0
|
||||
else
|
||||
Charge := 49;
|
||||
end;
|
||||
}
|
||||
|
||||
// Anti-pattern 2: the hook exists but the 'if IsHandled then exit;' guard is
|
||||
// missing, so the default logic still runs after a subscriber handled the call.
|
||||
codeunit 50223 "Shipping Charge Guard Bad"
|
||||
{
|
||||
procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
OnBeforeCalculateShippingCharge(OrderAmount, Charge, IsHandled);
|
||||
|
||||
// Bug: no 'if IsHandled then exit;' here. Even when a subscriber set
|
||||
// Charge and IsHandled := true, the default below overwrites the result.
|
||||
if OrderAmount >= 1000 then
|
||||
Charge := 0
|
||||
else
|
||||
Charge := 49;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeCalculateShippingCharge(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,37 @@
|
|||
// Demonstration-only AL. Not compiled by CI; illustrates the article.
|
||||
codeunit 50220 "Shipping Charge Good Sample"
|
||||
{
|
||||
procedure CalculateShippingCharge(OrderAmount: Decimal) Charge: Decimal
|
||||
var
|
||||
IsHandled: Boolean;
|
||||
begin
|
||||
// Give extensions a sanctioned seam to replace the calculation, then
|
||||
// skip the default logic when a subscriber has handled it.
|
||||
OnBeforeCalculateShippingCharge(OrderAmount, Charge, IsHandled);
|
||||
if IsHandled then
|
||||
exit(Charge);
|
||||
|
||||
if OrderAmount >= 1000 then
|
||||
Charge := 0
|
||||
else
|
||||
Charge := 49;
|
||||
end;
|
||||
|
||||
[IntegrationEvent(false, false)]
|
||||
local procedure OnBeforeCalculateShippingCharge(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50221 "Shipping Charge Sub Good Sample"
|
||||
{
|
||||
// A partner replaces the flat rate with a contract-specific rule.
|
||||
[EventSubscriber(ObjectType::Codeunit, Codeunit::"Shipping Charge Good Sample", 'OnBeforeCalculateShippingCharge', '', false, false)]
|
||||
local procedure ApplyContractRate(OrderAmount: Decimal; var Charge: Decimal; var IsHandled: Boolean)
|
||||
begin
|
||||
if IsHandled then
|
||||
exit;
|
||||
Charge := OrderAmount * 0.02;
|
||||
IsHandled := true;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: events
|
||||
keywords: [ishandled, overridable, onbefore, integration-event, extensibility, event-override, subscriber-hook]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use the IsHandled pattern to make base behaviour overridable
|
||||
|
||||
## Description
|
||||
|
||||
AL has no method overriding, so a `procedure` that runs its body unconditionally cannot be replaced by an extension without editing base code. The established Business Central seam for substituting default behaviour is the `IsHandled` pattern: the routine raises an `OnBefore…` integration event carrying a `var IsHandled: Boolean`, then exits early when a subscriber has set it. This hands a partner a sanctioned hook to replace the logic instead of overwriting the routine. LLMs trained on languages with inheritance emit routines whose logic always runs and expose no `OnBefore`/`IsHandled` seam, so the behaviour silently cannot be overridden.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Raise `OnBeforeX(…, IsHandled)` as the first step of the routine and guard with `if IsHandled then exit;` before any default logic runs. Declare the publisher `[IntegrationEvent(false, false)] local procedure OnBeforeX(…; var IsHandled: Boolean)` with an empty body, and keep `IsHandled` a `var` parameter so a subscriber can write to it. A subscriber that replaces the behaviour does its work and sets `IsHandled := true`; one that only augments leaves it untouched and guards with `if IsHandled then exit;` itself. Reserve the override hook for cases where a partner genuinely needs to replace logic — when the goal is only to react, a positive `OnAfter` event is the better seam.
|
||||
|
||||
See sample: `use-ishandled-to-make-base-behaviour-overridable.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Two shapes. First, a routine whose default logic always runs because there is no `OnBefore…`/`IsHandled` hook at all — extensions cannot change it without overwriting base code. Second, a routine that raises `OnBeforeX(IsHandled)` but omits the `if IsHandled then exit;` guard, so the default logic still executes after a subscriber set `IsHandled := true`, duplicating work and side effects. Detection: an `OnBefore` publisher with a `var IsHandled: Boolean` parameter whose caller never tests `IsHandled`, or a public routine doing non-trivial work with no overridable seam.
|
||||
|
||||
See sample: `use-ishandled-to-make-base-behaviour-overridable.bad.al`.
|
||||
|
|
@ -3,7 +3,8 @@ kind: action-skill
|
|||
id: al-code-review
|
||||
version: 1
|
||||
title: AL code review
|
||||
description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI, error handling, interfaces).
|
||||
description: Reviews AL source changes by composing the AL review leaf skills (performance, security, privacy, upgrade, style, UI, error handling, events, interfaces).
|
||||
|
||||
inputs: [pr-diff, file-path]
|
||||
outputs: [findings-report]
|
||||
bc-version: [all]
|
||||
|
|
@ -18,6 +19,7 @@ sub-skills:
|
|||
- microsoft/skills/review/al-style-review.md
|
||||
- microsoft/skills/review/al-ui-review.md
|
||||
- microsoft/skills/review/al-error-handling-review.md
|
||||
- microsoft/skills/review/al-events-review.md
|
||||
- microsoft/skills/review/al-interfaces-review.md
|
||||
---
|
||||
|
||||
|
|
@ -40,6 +42,7 @@ The sub-skills invoked by this skill are those listed in frontmatter `sub-skills
|
|||
- `microsoft/skills/review/al-style-review.md`
|
||||
- `microsoft/skills/review/al-ui-review.md`
|
||||
- `microsoft/skills/review/al-error-handling-review.md`
|
||||
- `microsoft/skills/review/al-events-review.md`
|
||||
- `microsoft/skills/review/al-interfaces-review.md`
|
||||
|
||||
Additional leaf skills (for example, telemetry, testing) are added by updating the `sub-skills` list. The skill does not discover sub-skills implicitly.
|
||||
|
|
|
|||
153
microsoft/skills/review/al-events-review.md
Normal file
153
microsoft/skills/review/al-events-review.md
Normal file
|
|
@ -0,0 +1,153 @@
|
|||
---
|
||||
kind: action-skill
|
||||
id: al-events-review
|
||||
version: 1
|
||||
title: AL events review
|
||||
description: Reviews AL source changes against events-and-subscribers guidance from BCQuality.
|
||||
inputs: [pr-diff, file-path]
|
||||
outputs: [findings-report]
|
||||
bc-version: [all]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# AL events review
|
||||
|
||||
Reviews AL source changes against the `events` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
|
||||
|
||||
An orchestrator invokes this skill with either a `pr-diff` (the standard PR-review entry point) or a `file-path` (single-file review). The skill produces a single JSON document conforming to the DO output contract.
|
||||
|
||||
## Source
|
||||
|
||||
Read the BCQuality knowledge index once — the `knowledge-index.json` BCQuality builds at the root of the knowledge checkout (Entry's preparation step regenerates it over the live, already-filtered clone — see `skills/entry.md`). It lists every article that survived layer and allow/deny filtering and carries, per article, its `path`, `layer`, `domain`, frontmatter dimensions, `keywords`, `title`, and a one-line `description` hint — exactly the fields Relevance and Worklist consume. Take the index entries whose `domain` is `events` as this skill's candidate set across every enabled layer; do not open the individual article files at this step. Open an article's full body only once it enters the Worklist below, so a review reads the index plus the handful of worklisted articles instead of every file under `*/knowledge/events/**`.
|
||||
|
||||
## Relevance
|
||||
|
||||
Apply the frontmatter matching rules defined in READ (*Frontmatter matching semantics*) against the task context:
|
||||
|
||||
- `bc-version` — the target BC version from the PR branch's `app.json` or the orchestrator-supplied version. If unavailable, the dimension is `unknown`.
|
||||
- `technologies` — `[al]`.
|
||||
- `countries` — the countries declared in the consuming app's `app.json`. Default to the orchestrator's configured context; if absent, `unknown`.
|
||||
- `application-area` — the union of application areas declared by the changed objects. Pass the actual set; do not substitute `[all]`. If the area cannot be determined from the changes, the dimension is `unknown`.
|
||||
|
||||
Discard files that are not applicable. Retain conditionally applicable files (any dimension `unknown`) only when the orchestrator's configuration permits them; findings derived from those files MUST have `confidence` no higher than `medium`, AND the finding's `message` MUST name the dimension or dimensions that were unknown.
|
||||
|
||||
## Worklist
|
||||
|
||||
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed AL object names and types — especially codeunits that publish events or host event subscribers, posting/release/validation routines that should expose extension points, and test codeunits that bind subscribers.
|
||||
- The changed procedures and triggers, weighted toward event publisher methods, methods carrying the `[EventSubscriber(...)]` attribute, routines that raise `OnBefore`/`OnAfter` events, and any procedure that calls `BindSubscription`/`UnbindSubscription`.
|
||||
- Tokens extracted from the diff that relate to events and the publish/subscribe model (`IntegrationEvent`, `BusinessEvent`, `EventSubscriber`, `IsHandled`, `BindSubscription`, `UnbindSubscription`, `EventSubscriberInstance`, `OnBefore`, `OnAfter`, `Manual`, `IncludeSender`, `Sender`, `this`, `RecordRef`, `xRec`, `temporary`, `Temp`, `repeat`).
|
||||
|
||||
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.
|
||||
|
||||
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`.
|
||||
|
||||
When the post-conflict worklist is empty because no applicable events knowledge exists, or because configuration suppressed every candidate, emit `outcome: "no-knowledge"`. When the worklist is empty because no applicable events knowledge matched the changes, emit `outcome: "completed"` with an empty `findings` array.
|
||||
|
||||
### Event-design checks
|
||||
|
||||
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`.
|
||||
- `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`.
|
||||
- A parameter added before existing parameters on a changed event signature instead of appended at the end — `add-new-event-parameters-at-the-end`.
|
||||
- Publisher names that do not encode firing position (`OnBefore`/`OnAfter<Routine>` at the boundaries, `On<Routine>OnBefore`/`OnAfter<Context>` mid-routine) — `name-events-by-publisher-position`.
|
||||
- Two consecutive `OnBefore`/`OnAfter` raises with no logic between them, or a near-duplicate event differing only by an extra parameter — `prefer-reusing-or-extending-existing-events`.
|
||||
- An event raised between `repeat` and `until` inside a record loop — `do-not-publish-events-inside-loops`.
|
||||
- A `temporary` record event parameter whose name does not start with `Temp` — `prefix-temporary-record-event-parameters-with-temp`.
|
||||
- Abbreviated event parameter names (`SalesHdr`, `DocNo`, `Amt`) instead of full table names and spelled-out values — `name-event-parameters-without-abbreviations`.
|
||||
- `[IntegrationEvent(true, …)]` (`IncludeSender`) on a codeunit event used only to expose the publisher, where `this` could be passed as a typed `Sender` parameter (Business Central 2024 release wave 2 and later) — `prefer-this-over-includesender-in-codeunit-events`.
|
||||
- A `RecordRef` event parameter, or a passed-through `xRec`, where a concrete typed record fits — `avoid-loosely-typed-event-parameters`.
|
||||
- A `var IsHandled` added to a pre-existing event rather than introduced through a new `OnBefore` publisher — `do-not-add-ishandled-to-an-existing-event`.
|
||||
- An `if IsHandled then exit;` whose skipped body performs posting, ledger-entry creation, number-series consumption, or integrity/permission validation — `do-not-bypass-critical-operations-with-ishandled`.
|
||||
|
||||
## Action
|
||||
|
||||
For each worklist entry, evaluate the diff against the file's `## Best Practice` and `## Anti Pattern` sections. Emit findings as follows:
|
||||
|
||||
- When the diff contains a clear match for an Anti Pattern, emit a finding with severity `major` or `blocker`, a message summarizing the anti-pattern, `location` pointing to the offending line or range, and a `references` entry pointing to the knowledge file. Use `blocker` only when the knowledge file states the anti-pattern violates a platform-level guarantee. When the file does not make such a claim, the ceiling is `major`.
|
||||
- When the diff contains code that contradicts a Best Practice without being a full anti-pattern, emit `minor` with the same reference shape.
|
||||
- When the skill cannot detect a violation but the file is clearly applicable to the change, emit `info` citing the file. Repository-wide observations MAY omit `location`.
|
||||
|
||||
Set `confidence` to:
|
||||
|
||||
- `high` when the detection is based on an unambiguous pattern match (identifier, syntax, object type).
|
||||
- `medium` when detection relies on heuristics or when any frontmatter dimension was `unknown`.
|
||||
- `low` when the finding is an advisory derived only from applicability.
|
||||
|
||||
After evaluating each worklist entry, also consider whether the diff exhibits an events 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 events 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 events and subscribers; 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: empty out a non-empty `[IntegrationEvent]` publisher body; add the missing `if IsHandled then exit;` guard after an `OnBefore` raise; add a matching `UnbindSubscription` for a leaked `BindSubscription`; set `EventSubscriberInstance = Manual;` on a codeunit that must be scoped). 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.
|
||||
|
||||
Outcome selection:
|
||||
|
||||
- `completed` — the skill evaluated every worklist item; default when the skill finishes normally, including when the resulting `findings` array is empty.
|
||||
- `no-knowledge` — no applicable events knowledge survived Source, Relevance, configuration filtering, and conflict resolution. `findings` is empty.
|
||||
- `not-applicable` — the task context lacks an AL dimension (no AL changes in the diff, or `technologies` filter rejected the task).
|
||||
- `partial` — a time or token budget was hit before the worklist was exhausted. `summary.coverage` reflects the evaluated subset; `outcome-reason` explains the cause.
|
||||
- `failed` — an unrecoverable error occurred. `outcome-reason` is required.
|
||||
|
||||
## Output
|
||||
|
||||
Output conforms to the DO output contract. A populated example:
|
||||
|
||||
```json
|
||||
{
|
||||
"skill": { "id": "al-events-review", "version": 1 },
|
||||
"outcome": "completed",
|
||||
"summary": {
|
||||
"counts": { "blocker": 0, "major": 1, "minor": 1, "info": 0 },
|
||||
"coverage": { "worklist-size": 2, "items-evaluated": 2 }
|
||||
},
|
||||
"findings": [
|
||||
{
|
||||
"id": "microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md",
|
||||
"severity": "major",
|
||||
"message": "Business logic is placed inside an [IntegrationEvent] publisher body, so the event mutates state on every raise instead of being a thin hook. Move the logic into the calling routine and leave the publisher body empty.",
|
||||
"location": {
|
||||
"file": "src/Sales/ReservationMgt.Codeunit.al",
|
||||
"line": 64,
|
||||
"range": { "start-line": 61, "end-line": 67 }
|
||||
},
|
||||
"references": [
|
||||
{ "path": "microsoft/knowledge/events/publish-thin-onbefore-onafter-integration-events.md" }
|
||||
],
|
||||
"confidence": "high"
|
||||
},
|
||||
{
|
||||
"id": "microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md",
|
||||
"severity": "minor",
|
||||
"message": "An OnBefore event is raised with a var IsHandled parameter, but the routine never guards with 'if IsHandled then exit;', so the default logic still runs after a subscriber handled the call.",
|
||||
"location": {
|
||||
"file": "src/Sales/ReservationMgt.Codeunit.al",
|
||||
"line": 41
|
||||
},
|
||||
"references": [
|
||||
{ "path": "microsoft/knowledge/events/use-ishandled-to-make-base-behaviour-overridable.md" }
|
||||
],
|
||||
"confidence": "high"
|
||||
}
|
||||
],
|
||||
"suppressed": []
|
||||
}
|
||||
```
|
||||
|
||||
The empty-corpus case — BCQuality's state until events knowledge files land — produces:
|
||||
|
||||
```json
|
||||
{
|
||||
"skill": { "id": "al-events-review", "version": 1 },
|
||||
"outcome": "no-knowledge",
|
||||
"summary": {
|
||||
"counts": { "blocker": 0, "major": 0, "minor": 0, "info": 0 },
|
||||
"coverage": { "worklist-size": 0, "items-evaluated": 0 }
|
||||
},
|
||||
"findings": [],
|
||||
"suppressed": []
|
||||
}
|
||||
```
|
||||
Loading…
Add table
Add a link
Reference in a new issue