From a0d23a982c5d9c06bb8f2c8df720c95eb4e02a9c Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Fri, 2 Oct 2026 00:03:59 +0200 Subject: [PATCH 1/2] Add trigger-default and declined-Confirm knowledge with review cues Three Microsoft-layer articles with good/bad samples: - error-handling/declined-confirm-must-abort-not-partially-apply - data-modeling/delete-master-data-with-trigger - data-modeling/master-data-must-be-inserted-with-trigger Wire targeted worklist cues into al-error-handling-review and al-data-modeling-review and register the samples in review-fixtures.json. Co-Authored-By: Claude Opus 5.5 --- evaluation/review-fixtures.json | 3 ++ .../delete-master-data-with-trigger.bad.al | 12 +++++ .../delete-master-data-with-trigger.good.al | 12 +++++ .../delete-master-data-with-trigger.md | 38 +++++++++++++++ ...-data-must-be-inserted-with-trigger.bad.al | 15 ++++++ ...data-must-be-inserted-with-trigger.good.al | 16 +++++++ ...ster-data-must-be-inserted-with-trigger.md | 38 +++++++++++++++ ...firm-must-abort-not-partially-apply.bad.al | 46 ++++++++++++++++++ ...irm-must-abort-not-partially-apply.good.al | 47 +++++++++++++++++++ ...-confirm-must-abort-not-partially-apply.md | 41 ++++++++++++++++ .../skills/review/al-data-modeling-review.md | 8 ++-- .../skills/review/al-error-handling-review.md | 3 +- 12 files changed, 275 insertions(+), 4 deletions(-) create mode 100644 microsoft/knowledge/data-modeling/delete-master-data-with-trigger.bad.al create mode 100644 microsoft/knowledge/data-modeling/delete-master-data-with-trigger.good.al create mode 100644 microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md create mode 100644 microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.bad.al create mode 100644 microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al create mode 100644 microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md create mode 100644 microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al create mode 100644 microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al create mode 100644 microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md diff --git a/evaluation/review-fixtures.json b/evaluation/review-fixtures.json index 5f0e50a..2789314 100644 --- a/evaluation/review-fixtures.json +++ b/evaluation/review-fixtures.json @@ -17,10 +17,12 @@ "check-blocked-in-referencing-code-not-in-master", "code-must-not-change-workdate", "custom-document-dispatch-must-not-bypass-report-selections", + "delete-master-data-with-trigger", "document-print-and-email-actions-call-report-selections-directly", "extend-find-entries-navigate-for-new-document-types", "extend-price-source-type-must-sync-document-subset-enum", "extend-report-selection-usage-for-new-document-types", + "master-data-must-be-inserted-with-trigger", "new-price-source-must-add-candidate-and-trigger-recalculation", "pictures-must-use-media-not-blob", "report-barcodes-must-use-barcode-module-and-production-font-name", @@ -31,6 +33,7 @@ "error-handling": { "articles": [ "collect-validation-errors-with-errorbehavior", + "declined-confirm-must-abort-not-partially-apply", "defensive-vs-offensive-code-must-match-blast-radius", "log-writes-must-survive-rollback" ] diff --git a/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.bad.al b/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.bad.al new file mode 100644 index 0000000..f5dc85e --- /dev/null +++ b/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.bad.al @@ -0,0 +1,12 @@ +codeunit 50100 "Sample Currency Cleanup" +{ + procedure DeleteRetiredCurrencies(CurrencyFilter: Text) + var + Currency: Record Currency; + begin + Currency.SetFilter(Code, CurrencyFilter); + // RunTrigger defaults to false: Currency.OnDelete never runs, so the + // open-entry guard is skipped and exchange rates are left orphaned. + Currency.DeleteAll(); + end; +} diff --git a/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.good.al b/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.good.al new file mode 100644 index 0000000..65423ad --- /dev/null +++ b/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.good.al @@ -0,0 +1,12 @@ +codeunit 50100 "Sample Currency Cleanup" +{ + procedure DeleteRetiredCurrencies(CurrencyFilter: Text) + var + Currency: Record Currency; + begin + Currency.SetFilter(Code, CurrencyFilter); + // DeleteAll(true) runs Currency.OnDelete for each record: it errors while + // open ledger entries use the code and removes the exchange rates itself. + Currency.DeleteAll(true); + end; +} diff --git a/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md b/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md new file mode 100644 index 0000000..5cbeaba --- /dev/null +++ b/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md @@ -0,0 +1,38 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [delete, deleteall, runtrigger, ondelete, master-data, currency, cleanup, data-migration] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Delete master and reference data with `Delete(true)` so the owning table's `OnDelete` decides + +## Description + +`Record.Delete()` and `Record.DeleteAll()` do not run `OnDelete` unless `RunTrigger` is `true`; the default is `false`. On a master or reference table, `OnDelete` is where Business Central decides whether the delete is safe and removes what the record owns. `Currency.OnDelete` refuses the delete while any **open** customer, vendor, or employee ledger entry uses the code, then deletes the currency's `Currency Exchange Rate` rows itself. `Customer.OnDelete` refuses when a job bills the customer, calls codeunit 361 `MoveEntries` (which refuses while ledger entries fall in an unclosed fiscal year or are still open, and otherwise detaches the closed history), and removes default dimensions, related data, and the contact link. + +A cleanup, migration, or "remove obsolete codes" routine that calls `Delete()`/`DeleteAll()` without `true` on such a table skips all of it: it can remove a currency that open entries still use, and leaves exchange rates and other dependents orphaned. That a record *looks* obsolete — a superseded currency nobody posts in any more — is no evidence the guard would pass, and keeping a record that closed history still refers to is often the better choice. Where the master has a `Blocked` field, blocking rather than deleting is the usual way to retire it (see [`check-blocked-in-referencing-code-not-in-master`](check-blocked-in-referencing-code-not-in-master.md)); `Currency` has none. + +## Best Practice + +Outside the owning table's own triggers, delete master and reference records with `Delete(true)`/`DeleteAll(true)` and let `OnDelete` raise its error, as BCApps does when it removes items (`CatalogItemManagement`, `NewItem.Delete(true)`). This is the "trigger does work the caller depends on" case of [`pass-false-to-insert-when-trigger-not-needed`](../performance/pass-false-to-insert-when-trigger-not-needed.md); a hand-written reference check is no substitute for the guard. + +Legitimate `false` deletes, not in scope: the owning table's own `OnDelete` cascade removing its dependents (`Currency.OnDelete` itself calls `CurrExchRate.DeleteAll()`; see [`owning-table-must-delete-dependents-in-ondelete`](owning-table-must-delete-dependents-in-ondelete.md)); temporary records and buffers; and a deliberate full reset whose caller first clears every reference itself, such as codeunit 1812 `"Data Migration Del G/L Account"` before a migration reload. Posted ledger entries are not master data; see [`do-not-modify-or-delete-posted-ledger-entries`](../finance/do-not-modify-or-delete-posted-ledger-entries.md). + +See sample: [`delete-master-data-with-trigger.good.al`](delete-master-data-with-trigger.good.al). + +## Anti Pattern + +Code outside the owning table's `OnDelete` calls `Delete()`/`DeleteAll()` (or `false`) on a non-temporary master or reference table — `Currency`, `Customer`, `Vendor`, `Item`, `G/L Account`, or a custom equivalent with an `OnDelete` guard or cascade — typically from a filter on codes judged obsolete, without itself clearing every reference first. + +See sample: [`delete-master-data-with-trigger.bad.al`](delete-master-data-with-trigger.bad.al). + +## References + +- [Record.Delete method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-delete-method) and [Record.DeleteAll method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-deleteall-method): `RunTrigger` "The default value is false." +- BCApps `src/Layers/W1/BaseApp/Finance/Currency/Currency.Table.al`, `OnDelete`, lines 797-820. +- BCApps `src/Layers/W1/BaseApp/Sales/Customer/Customer.Table.al`, `OnDelete`, lines 2401-2430; `src/Layers/W1/BaseApp/Utilities/MoveEntries.Codeunit.al`, `MoveCustEntries`, lines 126-167. +- BCApps `src/Layers/W1/BaseApp/Inventory/Item/Catalog/CatalogItemManagement.Codeunit.al`, line 419. +- BCApps `src/Layers/W1/BaseApp/System/DataMigration/DataMigrationDelGLAccount.Codeunit.al`, lines 18-41. diff --git a/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.bad.al b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.bad.al new file mode 100644 index 0000000..cc9d5d4 --- /dev/null +++ b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.bad.al @@ -0,0 +1,15 @@ +codeunit 50100 "Sample Customer Import" +{ + procedure CreateCustomer(ExternalName: Text[100]; ExternalCountry: Code[10]): Code[20] + var + Customer: Record Customer; + begin + Customer.Init(); + Customer.Validate(Name, ExternalName); + Customer.Validate("Country/Region Code", ExternalCountry); + // RunTrigger defaults to false: OnInsert never runs, so "No." stays + // blank and no contact, salesperson, or timestamps are set. + Customer.Insert(); + exit(Customer."No."); + end; +} diff --git a/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al new file mode 100644 index 0000000..4597670 --- /dev/null +++ b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al @@ -0,0 +1,16 @@ +codeunit 50100 "Sample Customer Import" +{ + procedure CreateCustomer(ExternalName: Text[100]; ExternalCountry: Code[10]): Code[20] + var + Customer: Record Customer; + begin + Customer.Init(); + // Insert(true) runs Customer.OnInsert: "No." from the number series, + // contact, default salesperson, dimension sync, and timestamps. + Customer.Insert(true); + Customer.Validate(Name, ExternalName); + Customer.Validate("Country/Region Code", ExternalCountry); + Customer.Modify(true); + exit(Customer."No."); + end; +} diff --git a/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md new file mode 100644 index 0000000..6661fa4 --- /dev/null +++ b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md @@ -0,0 +1,38 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [insert, runtrigger, oninsert, master-data, no-series, customer, item, import] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Create master records with `Insert(true)` unless the caller does the trigger's work itself + +## Description + +`Record.Insert()` does not run `OnInsert`: `RunTrigger` defaults to `false`. On a standard master table that trigger initializes the record. `Customer.OnInsert` assigns `No.` and `No. Series` from Sales & Receivables Setup when `No.` is blank, then defaults `Invoice Disc. Code` and the salesperson, creates the contact, syncs `Global Dimension 1/2 Code` from existing Default Dimension rows (`DimMgt.UpdateDefaultDim` creates no default dimensions), calls `UpdateReferencedIds`, and sets the last-modified timestamp. `Item.OnInsert` assigns `No.`, `No. Series`, and `Costing Method` when `No.` is blank and always runs the dimension sync and `UpdateReferencedIds`. + +A bare `Insert()` produces a row that looks complete but lacks what downstream code assumes: with a blank `No.` the key stays blank; with a supplied `No.` the contact, defaults, and timestamps are silently missing. This is the caller-side counterpart of [`master-table-no-from-number-series-in-oninsert`](master-table-no-from-number-series-in-oninsert.md): that design only works when callers run the trigger. + +## Best Practice + +When code creates a record in `Customer`, `Vendor`, `Item`, `G/L Account`, `Contact`, or a custom master with initializing `OnInsert` logic, call `Insert(true)`, then validate fields and `Modify(true)`. Importing from an external source is not an exception: BCApps' data-migration facades (`CustomerDataMigrationFacade`, `ItemDataMigrationFacade`, `GLAccDataMigrationFacade`) use `Insert(true)`. This is the "trigger does work the caller depends on" case of [`pass-false-to-insert-when-trigger-not-needed`](../performance/pass-false-to-insert-when-trigger-not-needed.md); the decision stays per call. + +Legitimate `Insert()` calls, not in scope: temporary records and buffer or staging tables; a caller that visibly assigns what the trigger would and then applies a template (`CatalogItemManagement.CreateNewItem` sets `No.` and `Costing Method` before `Item.Insert()`); and an XMLport that round-trips complete rows exported from Business Central (`ExportItemData`). `Modify()` without the trigger is routine on masters for technical fields and is not covered. Upgrade code that bypasses triggers is covered by [`datatransfer-skips-triggers-and-subscribers`](../upgrade/datatransfer-skips-triggers-and-subscribers.md). + +See sample: [`master-data-must-be-inserted-with-trigger.good.al`](master-data-must-be-inserted-with-trigger.good.al). + +## Anti Pattern + +Code creates a non-temporary master record with `Init`, field assignments or `Validate` calls, and `Insert()`/`Insert(false)`, without itself assigning the number and the other fields `OnInsert` would set. + +See sample: [`master-data-must-be-inserted-with-trigger.bad.al`](master-data-must-be-inserted-with-trigger.bad.al). + +## References + +- [Record.Insert(Boolean) method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-insert-boolean-method): "If this parameter is false, the code in the OnInsert trigger is not executed. The default value is false." +- BCApps `src/Layers/W1/BaseApp/Sales/Customer/Customer.Table.al`, `OnInsert`, lines 2432-2472; `src/Layers/W1/BaseApp/Inventory/Item/Item.Table.al`, `OnInsert`, from line 2587. +- BCApps `src/Layers/W1/BaseApp/Finance/Dimension/DimensionManagement.Codeunit.al`, `UpdateDefaultDim`, lines 894-913. +- BCApps `src/Layers/W1/BaseApp/Inventory/Item/Catalog/CatalogItemManagement.Codeunit.al`, `CreateNewItem`, lines 545-565; `src/Layers/W1/BaseApp/Inventory/Item/ExportItemData.XmlPort.al`, line 405. +- BCApps `src/Layers/W1/BaseApp/System/DataMigration/`: `CustomerDataMigrationFacade.Codeunit.al` line 67, `ItemDataMigrationFacade.Codeunit.al` line 76, `GLAccDataMigrationFacade.Codeunit.al` line 77. diff --git a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al new file mode 100644 index 0000000..adc1516 --- /dev/null +++ b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al @@ -0,0 +1,46 @@ +table 50100 "Sample Mailbox Watch" +{ + fields + { + field(1; "Code"; Code[20]) + { + } + field(2; "Watched Email"; Text[250]) + { + trigger OnValidate() + var + StopWatchingQst: Label 'Email %1 is being watched. Stop watching it?', Comment = '%1 = previous email address'; + begin + if (xRec."Watched Email" <> '') and (xRec."Watched Email" <> "Watched Email") then + if Confirm(StopWatchingQst, false, xRec."Watched Email") then + Unsubscribe("Subscription ID"); + // On "no" the old subscription is never removed, and the only field + // that tracked it is overwritten here: it is orphaned. + "Subscription ID" := Subscribe("Watched Email"); + end; + } + field(3; "Subscription ID"; Guid) + { + Editable = false; + } + } + + keys + { + key(PK; "Code") + { + Clustered = true; + } + } + + local procedure Subscribe(EmailAddress: Text[250]): Guid + begin + // Registers EmailAddress with the external watch service and returns its subscription. + exit(CreateGuid()); + end; + + local procedure Unsubscribe(SubscriptionId: Guid) + begin + // Removes the subscription from the external watch service. + end; +} diff --git a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al new file mode 100644 index 0000000..2454a59 --- /dev/null +++ b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al @@ -0,0 +1,47 @@ +table 50100 "Sample Mailbox Watch" +{ + fields + { + field(1; "Code"; Code[20]) + { + } + field(2; "Watched Email"; Text[250]) + { + trigger OnValidate() + var + ReplaceWatchQst: Label 'Email %1 is being watched. Stop watching it and watch %2 instead?', Comment = '%1 = previous email address, %2 = new email address'; + begin + if (xRec."Watched Email" <> '') and (xRec."Watched Email" <> "Watched Email") then begin + // Declining cancels the whole change: the field keeps its old value. + if not Confirm(ReplaceWatchQst, false, xRec."Watched Email", "Watched Email") then + Error(''); + Unsubscribe("Subscription ID"); + end; + "Subscription ID" := Subscribe("Watched Email"); + end; + } + field(3; "Subscription ID"; Guid) + { + Editable = false; + } + } + + keys + { + key(PK; "Code") + { + Clustered = true; + } + } + + local procedure Subscribe(EmailAddress: Text[250]): Guid + begin + // Registers EmailAddress with the external watch service and returns its subscription. + exit(CreateGuid()); + end; + + local procedure Unsubscribe(SubscriptionId: Guid) + begin + // Removes the subscription from the external watch service. + end; +} diff --git a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md new file mode 100644 index 0000000..dcedb57 --- /dev/null +++ b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md @@ -0,0 +1,41 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [confirm, onvalidate, xrec, abort, revert, side-effect, consistency] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# A declined `Confirm` in `OnValidate` must abort or revert, not skip a required side effect + +## Description + +By the time a field's `OnValidate` body runs, the field already holds its new value. When the trigger asks the user to confirm a side effect that the new value makes **required for consistency** — releasing or replacing state that is tied to the old value and that nothing will reference after the change — the shape `if Confirm(...) then ;` with nothing on the `false` branch lets the new value commit while silently skipping that effect. No error is raised, the user sees no indication that anything was declined, and the record is left inconsistent with its own dependents. + +Not every confirmed follow-up is required. When the confirmed effect is a convenience the user may legitimately decline, skipping it is correct: `"Sales Header"`'s `UpdateSalesLinesByFieldNo` asks whether to update the lines after a header field changes and, on "no", simply `exit`s — the header keeps its new value and the lines stay as they were, by design. A `Confirm` at the top of an action procedure, before anything has been written, may also just `exit` on "no" (for example the delete action in `"Test Input Groups"`). Neither shape is this anti-pattern. + +## Best Practice + +Make the declined branch match what "no" means: + +- **Cancel the whole change** — raise an error before the side effect. The usual BCApps form inside `OnValidate` is the silent abort `if not Confirm(...) then Error('');` (for example `"Bank Account"`, field `"Disable Bank Rec. Optimization"`). The field trigger documentation states that in case of an error "the user entry is not written to the database." +- **Keep the old value but let the rest of the edit continue** — assign the field back in code. `"Upload And Deploy Extension"` resets its sync-mode value to `Add` when the user declines `Force Sync`. + +Ask before the side effect runs, and before taking locks the prompt would hold open (see [`avoid-user-prompts-inside-transactions`](../performance/avoid-user-prompts-inside-transactions.md)). + +See sample: [`declined-confirm-must-abort-not-partially-apply.good.al`](declined-confirm-must-abort-not-partially-apply.good.al). + +## Anti Pattern + +Inside a field `OnValidate` (or a procedure it calls), a `Confirm` gates a side effect that releases, cancels, or replaces state belonging to the old value (`xRec`), the `false` branch neither errors nor restores the field, and code after it proceeds as if the change were accepted — for example overwriting the only field that tracks the old state. Declining leaves that state orphaned. Do not flag optional follow-ups whose skipping leaves every record consistent, or `exit` on "no" in an action before any write. + +See sample: [`declined-confirm-must-abort-not-partially-apply.bad.al`](declined-confirm-must-abort-not-partially-apply.bad.al). + +## References + +- [OnValidate (Field) trigger](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/triggers-auto/field/devenv-onvalidate-field-trigger). +- BCApps `src/Layers/W1/BaseApp/Bank/BankAccount/BankAccount.Table.al`, lines 980-988 (silent abort in `OnValidate`). +- BCApps `src/System Application/App/Extension Management/src/UploadAndDeployExtension.Page.al`, lines 78-83 (explicit revert). +- BCApps `src/Layers/W1/BaseApp/Sales/Document/SalesHeader.Table.al`, `UpdateSalesLinesByFieldNo`, lines 4997-5014 (optional follow-up; `end else exit`). +- BCApps `src/Tools/Test Framework/Test Runner/src/DataDrivenTest/DataInputs/TestInputGroups.Page.al`, lines 85-86 (`exit` before any write). diff --git a/microsoft/skills/review/al-data-modeling-review.md b/microsoft/skills/review/al-data-modeling-review.md index 5f4cec7..3bb64ce 100644 --- a/microsoft/skills/review/al-data-modeling-review.md +++ b/microsoft/skills/review/al-data-modeling-review.md @@ -16,7 +16,7 @@ application-area: [all] Reviews AL source changes against the `data-modeling` 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 a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, audit fields, document print/email/Post-and-Send actions, `Navigate` page subscribers, Report Selection registration or dispatch, price-calculation/price-source extensibility, `TransferFields`-based posting-cascade field mirroring, barcode/report-layout font-provider usage, dimension wiring, journal-based posting-routine structure, or Item Ledger Entry document-number lookups after a combined sales post. The skill returns `not-applicable` when none of those apply. +An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, audit fields, document print/email/Post-and-Send actions, `Navigate` page subscribers, Report Selection registration or dispatch, price-calculation/price-source extensibility, `TransferFields`-based posting-cascade field mirroring, barcode/report-layout font-provider usage, dimension wiring, journal-based posting-routine structure, Item Ledger Entry document-number lookups after a combined sales post, or code that inserts or deletes master/reference records. The skill returns `not-applicable` when none of those apply. ## Source @@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially `* Setup` singleton tables and Card pages, custom master tables, tableextensions that add master-data fields, document or journal lines that reference a master, document pages/codeunits exposing print/email/Post-and-Send actions, codeunits subscribing to `Navigate`, enumextensions to `"Report Selection Usage"`/`"Price Calculation Handler"`/`"Price Source Type"`, and report objects that render barcodes. - The changed fields, keys, triggers, and procedures, weighted toward `Primary Key`, `No.`, `No. Series`, `Blocked`, `Last Date Modified`, `OnInsert`, `OnModify`, `OnRename`, reference-field `OnValidate`, posting validation, and posting-cascade `TransferFields` calls. -- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `InitRecord`, `Round`, `Precision`, `Direction`, `TableRelation`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`, `TransferFields`, `Navigate`, `OnAfterFindRecords`, `OnBeforeShowRecords`, `Report Selections`, `Report Selection Usage`, `InsertRecord`, `Document Sending Profile`, `PrintForCust`, `PrintWithDialogForCust`, `PrintWithDialogForVend`, `SendEmailToCust`, `SendEmailToVendor`, `Report.RunModal`, `Report.Run`, `Price Calculation Handler`, `Price Calculation`, `OnFindSupportedSetup`, `Price Calculation Setup`, `Price Source Type`, `PriceSourceList`, `OnAfterAddSources`, `UpdateUnitPrice`, `PlanPriceCalcByField`, `UpdateUnitPriceByField`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`). +- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `InitRecord`, `Round`, `Precision`, `Direction`, `TableRelation`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`, `TransferFields`, `Navigate`, `OnAfterFindRecords`, `OnBeforeShowRecords`, `Report Selections`, `Report Selection Usage`, `InsertRecord`, `Document Sending Profile`, `PrintForCust`, `PrintWithDialogForCust`, `PrintWithDialogForVend`, `SendEmailToCust`, `SendEmailToVendor`, `Report.RunModal`, `Report.Run`, `Price Calculation Handler`, `Price Calculation`, `OnFindSupportedSetup`, `Price Calculation Setup`, `Price Source Type`, `PriceSourceList`, `OnAfterAddSources`, `UpdateUnitPrice`, `PlanPriceCalcByField`, `UpdateUnitPriceByField`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`, `Insert`, `Delete`, `DeleteAll`, `Currency`, `Customer`). A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no data-modeling changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files. @@ -51,6 +51,8 @@ The following targeted checks cover every current `data-modeling` article. Treat - A new or extended table's name, fields, or usage positively establish it as one of Business Central's nine business-record types — a name ending `Ledger Entry`/`Register`/`Journal Line`/`Header`/`Line`/`Setup`, an auto-generated `Entry No.`/`No.` key posted from elsewhere, a `Template Name`+`Batch Name`+`Line No.` key, or a singleton `Primary Key` field — `table-design-must-match-bc-table-type-conventions`. Do not worklist it from a bare `keys` block or primary-key declaration alone: a temporary/buffer table, a work queue, a log, a cross-reference/mapping table, or a process-local staging table is not one of the nine types and is out of this rule's scope entirely, not an unresolved case. - Code reads `Item Ledger Entry."Document No."` (or `"Last Shipping No."`/`"Last Posting No."`) after a combined Ship+Invoice **sales** post — `item-ledger-entry-document-no-follows-last-shipping-no`. This is a sales-specific rule: purchase combined posting is Receive+Invoice and uses receiving fields such as `"Last Receiving No."`, not the shipment/document-number behavior this article describes. Do not worklist it from purchase posting code. - A custom master table changes its primary key, `No.`/`No. Series` fields, or `OnInsert` without assigning a blank `No.` from setup through a number series — `master-table-no-from-number-series-in-oninsert`. +- Code outside the table creates a non-temporary master record (`Customer`, `Vendor`, `Item`, `G/L Account`, `Contact`, or a custom master with initializing `OnInsert` logic) with `Insert()`/`Insert(false)` and does not itself assign the number and the fields `OnInsert` would set — `master-data-must-be-inserted-with-trigger`. Do not flag temporary records, buffer/staging tables, a caller that visibly assigns the trigger's fields before applying a template, or an XMLport round-tripping rows exported from Business Central; `Modify()` without the trigger is not this rule. +- Code outside the owning table's own `OnDelete` calls `Delete()`/`DeleteAll()` (or passes `false`) on a non-temporary master or reference table with an `OnDelete` guard or cascade (`Currency`, `Customer`, `Vendor`, `Item`, `G/L Account`, or a custom equivalent) — `delete-master-data-with-trigger`. Do not flag an owning table's `OnDelete` deleting its own dependents, temporary/buffer records, or a deliberate full reset whose caller first clears every reference itself. - BC v22 or later code introduces or retains `NoSeriesManagement`, `InitSeries`, `SelectSeries`, or `SetSeries`, or number assignment/manual-entry checks do not use codeunit `"No. Series"` methods such as `GetNextNo`, `IsManual`, or `TestManual` — `use-no-series-codeunit-not-noseriesmanagement`. - A master gains or changes `Blocked`, or a document line, journal line, reference-field `OnValidate`, or posting routine uses that master without `TestField(Blocked, false)` at the point of use; also cue when the check is placed only in the master's own triggers — `check-blocked-in-referencing-code-not-in-master`. - A master table adds or changes `Last Date Modified`, `OnModify`, or `OnRename`, but the non-editable field is not assigned `Today()` in both triggers — `set-last-date-modified-in-onmodify-and-onrename`. @@ -100,7 +102,7 @@ Outcome selection: - `completed` — the skill evaluated every worklist item. - `no-knowledge` — no applicable data-modeling knowledge survived filtering. -- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, or audit-field surface, and no document print/email/Post-and-Send action, `Navigate` subscriber, Report Selection registration/dispatch, price-calculation/price-source extensibility point, posting-cascade `TransferFields` mirroring, barcode/report-font-provider usage, dimension wiring, posting-routine structure, or Item-Ledger-Entry-document-number surface. +- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, or audit-field surface, and no document print/email/Post-and-Send action, `Navigate` subscriber, Report Selection registration/dispatch, price-calculation/price-source extensibility point, posting-cascade `TransferFields` mirroring, barcode/report-font-provider usage, dimension wiring, posting-routine structure, Item-Ledger-Entry-document-number surface, or master/reference-record insert/delete call. - `partial` — a budget was hit before the worklist was exhausted. - `failed` — an unrecoverable error occurred. diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index 3962363..fb08a8d 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -46,7 +46,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially codeunits that post or validate, tables and table extensions with `OnValidate` triggers, and any procedure that raises errors or orchestrates a batch over records. - The changed procedures and triggers, weighted toward `OnValidate`/`OnInsert`/`OnModify` triggers, posting and validation routines, and procedures attributed with `[ErrorBehavior(...)]` or `[TryFunction]`. -- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `FieldError`, `TestField`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`, `TryFunction`, `GetLastErrorText`, Boolean assignment). +- Tokens extracted from the diff that relate to error surfacing and diagnostics (`Error`, `ErrorInfo`, `FieldError`, `TestField`, `Title`, `Message`, `DetailedMessage`, `AddAction`, `AddNavigationAction`, `RecordId`, `PageNo`, `ErrorBehavior`, `Collect`, `HasCollectedErrors`, `GetCollectedErrors`, `ClearCollectedErrors`, `ErrorType`, `Internal`, `Client`, `TryFunction`, `GetLastErrorText`, `Confirm`, `xRec`, Boolean assignment). - For the outbound HTTP call paths identified in Source, include `HttpClient`, `Get`, `Post`, `HttpResponseMessage`, response use, and caller failure handling (including `[TryFunction]` call sites) in keyword and topic matching. - Resolve changed standalone call targets; when the target declaration has `[TryFunction]`, worklist the ignored-return rule even if the declaration itself is unchanged. Only assignment and conditional use activate try semantics. @@ -54,6 +54,7 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `error-handling` article: +- A field `OnValidate` (or a procedure it calls) uses `Confirm` to gate a side effect that releases, cancels, or replaces state tied to the old (`xRec`) value, and the declined branch neither raises an error nor restores the field while the change proceeds — `declined-confirm-must-abort-not-partially-apply`. Do not flag optional follow-ups whose skipping leaves every record consistent (such as declining to update document lines after a header change), or `exit` on a declined `Confirm` in an action before anything is written. - `[ErrorBehavior(ErrorBehavior::Collect)]`, `ErrorInfo.Collectible`, `HasCollectedErrors`, `GetCollectedErrors`, or `ClearCollectedErrors` is added or changed, especially when errors are collected without later surfacing/clearing them — `collect-validation-errors-with-errorbehavior`. - New or changed code inserts an error/duration log record around a failed `TryFunction`/`GetLastErrorText`/`GetLastErrorCode` path and then raises, propagates, or rethrows the error — `log-writes-must-survive-rollback`. Do not worklist it when the log insert already happens inside a `Session.StartSession`-targeted codeunit's `OnRun`; that is the compliant shape, not the signal to flag. - A guarded lookup (`if Record.Get(...) then ... else` or similar) sets a value used later, and the same guard shape (with the same blank/zero fallback style) is applied to a field that feeds a posted amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output — `defensive-vs-offensive-code-must-match-blast-radius`. The signal is a posting-critical or compliance-facing field guarded defensively with a silent fallback, not the mere presence of a guarded lookup. From 4e2a3ead4d246ef7f8bef74186c2c04219029379 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Fri, 2 Oct 2026 00:22:49 +0200 Subject: [PATCH 2/2] Tighten declined-confirm, delete and insert trigger articles after review Co-Authored-By: Claude Opus 5.5 --- .../delete-master-data-with-trigger.md | 11 ++++++----- ...r-data-must-be-inserted-with-trigger.good.al | 2 +- ...master-data-must-be-inserted-with-trigger.md | 7 ++++--- ...onfirm-must-abort-not-partially-apply.bad.al | 15 ++++++++++----- ...nfirm-must-abort-not-partially-apply.good.al | 12 ++++++++---- ...ed-confirm-must-abort-not-partially-apply.md | 17 ++++++++++------- .../skills/review/al-data-modeling-review.md | 4 ++-- .../skills/review/al-error-handling-review.md | 2 +- 8 files changed, 42 insertions(+), 28 deletions(-) diff --git a/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md b/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md index 5cbeaba..6e3467c 100644 --- a/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md +++ b/microsoft/knowledge/data-modeling/delete-master-data-with-trigger.md @@ -13,26 +13,27 @@ application-area: [all] `Record.Delete()` and `Record.DeleteAll()` do not run `OnDelete` unless `RunTrigger` is `true`; the default is `false`. On a master or reference table, `OnDelete` is where Business Central decides whether the delete is safe and removes what the record owns. `Currency.OnDelete` refuses the delete while any **open** customer, vendor, or employee ledger entry uses the code, then deletes the currency's `Currency Exchange Rate` rows itself. `Customer.OnDelete` refuses when a job bills the customer, calls codeunit 361 `MoveEntries` (which refuses while ledger entries fall in an unclosed fiscal year or are still open, and otherwise detaches the closed history), and removes default dimensions, related data, and the contact link. -A cleanup, migration, or "remove obsolete codes" routine that calls `Delete()`/`DeleteAll()` without `true` on such a table skips all of it: it can remove a currency that open entries still use, and leaves exchange rates and other dependents orphaned. That a record *looks* obsolete — a superseded currency nobody posts in any more — is no evidence the guard would pass, and keeping a record that closed history still refers to is often the better choice. Where the master has a `Blocked` field, blocking rather than deleting is the usual way to retire it (see [`check-blocked-in-referencing-code-not-in-master`](check-blocked-in-referencing-code-not-in-master.md)); `Currency` has none. +A cleanup, migration, or "remove obsolete codes" routine that calls `Delete()`/`DeleteAll()` without `true` on such a table skips all of it (only `OnBeforeDelete`/`OnAfterDelete` triggers in table extensions still run): it can remove a currency that open entries still use, and leaves exchange rates and other dependents orphaned. That a record *looks* obsolete — a superseded currency nobody posts in any more — is no evidence the guard would pass, and keeping a record that closed history still refers to is often the better choice. Where the master has a `Blocked` field, blocking it is an alternative to deleting it; `Currency` has none. ## Best Practice Outside the owning table's own triggers, delete master and reference records with `Delete(true)`/`DeleteAll(true)` and let `OnDelete` raise its error, as BCApps does when it removes items (`CatalogItemManagement`, `NewItem.Delete(true)`). This is the "trigger does work the caller depends on" case of [`pass-false-to-insert-when-trigger-not-needed`](../performance/pass-false-to-insert-when-trigger-not-needed.md); a hand-written reference check is no substitute for the guard. -Legitimate `false` deletes, not in scope: the owning table's own `OnDelete` cascade removing its dependents (`Currency.OnDelete` itself calls `CurrExchRate.DeleteAll()`; see [`owning-table-must-delete-dependents-in-ondelete`](owning-table-must-delete-dependents-in-ondelete.md)); temporary records and buffers; and a deliberate full reset whose caller first clears every reference itself, such as codeunit 1812 `"Data Migration Del G/L Account"` before a migration reload. Posted ledger entries are not master data; see [`do-not-modify-or-delete-posted-ledger-entries`](../finance/do-not-modify-or-delete-posted-ledger-entries.md). +Legitimate `false` deletes, not in scope: the owning table's own `OnDelete` cascade removing its dependents (`Currency.OnDelete` itself calls `CurrExchRate.DeleteAll()`; see [`owning-table-must-delete-dependents-in-ondelete`](owning-table-must-delete-dependents-in-ondelete.md)); temporary records and buffers; deleting and immediately re-inserting the same primary key to restore or recreate a record, where dependents stay valid (`JobArchiveManagement` restoring a project from its archive; codeunit 1812 recreating each `"Customer Posting Group"` with the same `Code`); and a data-migration or setup reset in a company with no posted entries that removes master rows and their setup references as one rebuild, such as codeunit 1812 `"Data Migration Del G/L Account"`, whose `DeleteAll()` bypasses `"G/L Account".OnDelete`'s `MoveGLEntries` guard — acceptable only because no postings exist yet. Posted ledger entries are not master data; see [`do-not-modify-or-delete-posted-ledger-entries`](../finance/do-not-modify-or-delete-posted-ledger-entries.md). See sample: [`delete-master-data-with-trigger.good.al`](delete-master-data-with-trigger.good.al). ## Anti Pattern -Code outside the owning table's `OnDelete` calls `Delete()`/`DeleteAll()` (or `false`) on a non-temporary master or reference table — `Currency`, `Customer`, `Vendor`, `Item`, `G/L Account`, or a custom equivalent with an `OnDelete` guard or cascade — typically from a filter on codes judged obsolete, without itself clearing every reference first. +Code outside the owning table's `OnDelete` calls `Delete()`/`DeleteAll()` (or `false`) on a non-temporary master or reference table — `Currency`, `Customer`, `Vendor`, `Item`, `G/L Account`, or a custom equivalent with an `OnDelete` guard or cascade — typically from a filter on codes judged obsolete, outside a same-key delete-and-reinsert or a no-postings migration/setup rebuild. See sample: [`delete-master-data-with-trigger.bad.al`](delete-master-data-with-trigger.bad.al). ## References -- [Record.Delete method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-delete-method) and [Record.DeleteAll method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-deleteall-method): `RunTrigger` "The default value is false." +- [Record.Delete method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-delete-method) and [Record.DeleteAll method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-deleteall-method): `RunTrigger` "The default value is false."; the DeleteAll note adds that setting it to false "only affects the OnDelete trigger" — table-extension `OnBeforeDelete`/`OnAfterDelete` still run. - BCApps `src/Layers/W1/BaseApp/Finance/Currency/Currency.Table.al`, `OnDelete`, lines 797-820. - BCApps `src/Layers/W1/BaseApp/Sales/Customer/Customer.Table.al`, `OnDelete`, lines 2401-2430; `src/Layers/W1/BaseApp/Utilities/MoveEntries.Codeunit.al`, `MoveCustEntries`, lines 126-167. - BCApps `src/Layers/W1/BaseApp/Inventory/Item/Catalog/CatalogItemManagement.Codeunit.al`, line 419. -- BCApps `src/Layers/W1/BaseApp/System/DataMigration/DataMigrationDelGLAccount.Codeunit.al`, lines 18-41. +- BCApps `src/Layers/W1/BaseApp/System/DataMigration/DataMigrationDelGLAccount.Codeunit.al`, `OnRun` lines 18-32 and `DeleteGLAccounts` lines 34-42 (rebuild); lines 53-56 (same-key delete and re-insert); `src/Layers/W1/BaseApp/Finance/GeneralLedger/Account/GLAccount.Table.al`, `OnDelete`, line 1144 (`MoveGLEntries`). +- BCApps `src/Layers/W1/BaseApp/Projects/Project/Archive/JobArchiveManagement.Codeunit.al`, lines 212-219 (restore: `Job.Delete()`, then re-insert the same `No.`). diff --git a/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al index 4597670..5917127 100644 --- a/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al +++ b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.good.al @@ -6,7 +6,7 @@ codeunit 50100 "Sample Customer Import" begin Customer.Init(); // Insert(true) runs Customer.OnInsert: "No." from the number series, - // contact, default salesperson, dimension sync, and timestamps. + // contact and salesperson defaults (when set up), global dimensions, timestamps. Customer.Insert(true); Customer.Validate(Name, ExternalName); Customer.Validate("Country/Region Code", ExternalCountry); diff --git a/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md index 6661fa4..2e2c3a1 100644 --- a/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md +++ b/microsoft/knowledge/data-modeling/master-data-must-be-inserted-with-trigger.md @@ -11,7 +11,7 @@ application-area: [all] ## Description -`Record.Insert()` does not run `OnInsert`: `RunTrigger` defaults to `false`. On a standard master table that trigger initializes the record. `Customer.OnInsert` assigns `No.` and `No. Series` from Sales & Receivables Setup when `No.` is blank, then defaults `Invoice Disc. Code` and the salesperson, creates the contact, syncs `Global Dimension 1/2 Code` from existing Default Dimension rows (`DimMgt.UpdateDefaultDim` creates no default dimensions), calls `UpdateReferencedIds`, and sets the last-modified timestamp. `Item.OnInsert` assigns `No.`, `No. Series`, and `Costing Method` when `No.` is blank and always runs the dimension sync and `UpdateReferencedIds`. +`Record.Insert()` does not run `OnInsert`: `RunTrigger` defaults to `false`. On a standard master table that trigger initializes the record. Unless an `OnBeforeInsert` subscriber sets `IsHandled`, `Customer.OnInsert` assigns `No.` and `No. Series` from Sales & Receivables Setup when `No.` is blank, then defaults `Invoice Disc. Code`, defaults a blank salesperson from the user's User Setup `"Salespers./Purch. Code"` when one is set, creates the contact when Marketing Setup has a `"Bus. Rel. Code for Customers"` (unless the insert comes from a contact or from a template with a contact), overwrites `Global Dimension 1/2 Code` from the customer's Default Dimension rows and clears them when there are none (`DimMgt.UpdateDefaultDim` creates no default dimensions), calls `UpdateReferencedIds`, and sets the last-modified timestamps. `Item.OnInsert` assigns `No.`, `No. Series`, and `Costing Method` when `No.` is blank and, blank or not, runs the same global-dimension update and `UpdateReferencedIds`. A bare `Insert()` produces a row that looks complete but lacks what downstream code assumes: with a blank `No.` the key stays blank; with a supplied `No.` the contact, defaults, and timestamps are silently missing. This is the caller-side counterpart of [`master-table-no-from-number-series-in-oninsert`](master-table-no-from-number-series-in-oninsert.md): that design only works when callers run the trigger. @@ -19,7 +19,7 @@ A bare `Insert()` produces a row that looks complete but lacks what downstream c When code creates a record in `Customer`, `Vendor`, `Item`, `G/L Account`, `Contact`, or a custom master with initializing `OnInsert` logic, call `Insert(true)`, then validate fields and `Modify(true)`. Importing from an external source is not an exception: BCApps' data-migration facades (`CustomerDataMigrationFacade`, `ItemDataMigrationFacade`, `GLAccDataMigrationFacade`) use `Insert(true)`. This is the "trigger does work the caller depends on" case of [`pass-false-to-insert-when-trigger-not-needed`](../performance/pass-false-to-insert-when-trigger-not-needed.md); the decision stays per call. -Legitimate `Insert()` calls, not in scope: temporary records and buffer or staging tables; a caller that visibly assigns what the trigger would and then applies a template (`CatalogItemManagement.CreateNewItem` sets `No.` and `Costing Method` before `Item.Insert()`); and an XMLport that round-trips complete rows exported from Business Central (`ExportItemData`). `Modify()` without the trigger is routine on masters for technical fields and is not covered. Upgrade code that bypasses triggers is covered by [`datatransfer-skips-triggers-and-subscribers`](../upgrade/datatransfer-skips-triggers-and-subscribers.md). +Legitimate `Insert()` calls, not in scope: temporary records and buffer or staging tables; a caller that visibly assigns what the trigger would and then applies a template (`CatalogItemManagement.CreateNewItem` sets `No.` and `Costing Method` before `Item.Insert()`) or copies from a source record (`CopyItem` transfers the source item's fields and assigns the target `No.` before `TargetItem.Insert()`); and an XMLport that round-trips complete rows exported from Business Central (`ExportItemData`). `Modify()` without the trigger is routine on masters for technical fields and is not covered. Upgrade code that bypasses triggers is covered by [`datatransfer-skips-triggers-and-subscribers`](../upgrade/datatransfer-skips-triggers-and-subscribers.md). See sample: [`master-data-must-be-inserted-with-trigger.good.al`](master-data-must-be-inserted-with-trigger.good.al). @@ -33,6 +33,7 @@ See sample: [`master-data-must-be-inserted-with-trigger.bad.al`](master-data-mus - [Record.Insert(Boolean) method](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-insert-boolean-method): "If this parameter is false, the code in the OnInsert trigger is not executed. The default value is false." - BCApps `src/Layers/W1/BaseApp/Sales/Customer/Customer.Table.al`, `OnInsert`, lines 2432-2472; `src/Layers/W1/BaseApp/Inventory/Item/Item.Table.al`, `OnInsert`, from line 2587. +- BCApps `src/Layers/W1/BaseApp/Sales/Customer/Customer.Table.al`, `SetDefaultSalesperson`, lines 3848-3863; `src/Layers/W1/BaseApp/CRM/BusinessRelation/CustContUpdate.Codeunit.al`, `OnInsert`, lines 26-40. - BCApps `src/Layers/W1/BaseApp/Finance/Dimension/DimensionManagement.Codeunit.al`, `UpdateDefaultDim`, lines 894-913. -- BCApps `src/Layers/W1/BaseApp/Inventory/Item/Catalog/CatalogItemManagement.Codeunit.al`, `CreateNewItem`, lines 545-565; `src/Layers/W1/BaseApp/Inventory/Item/ExportItemData.XmlPort.al`, line 405. +- BCApps `src/Layers/W1/BaseApp/Inventory/Item/Catalog/CatalogItemManagement.Codeunit.al`, `CreateNewItem`, lines 545-565; `src/Layers/W1/BaseApp/Inventory/Item/CopyItem.Codeunit.al`, `InitTargetItem` and `CopyItem`, lines 107-133; `src/Layers/W1/BaseApp/Inventory/Item/ExportItemData.XmlPort.al`, line 405. - BCApps `src/Layers/W1/BaseApp/System/DataMigration/`: `CustomerDataMigrationFacade.Codeunit.al` line 67, `ItemDataMigrationFacade.Codeunit.al` line 76, `GLAccDataMigrationFacade.Codeunit.al` line 77. diff --git a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al index adc1516..1b48efc 100644 --- a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al +++ b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.bad.al @@ -11,12 +11,17 @@ table 50100 "Sample Mailbox Watch" var StopWatchingQst: Label 'Email %1 is being watched. Stop watching it?', Comment = '%1 = previous email address'; begin - if (xRec."Watched Email" <> '') and (xRec."Watched Email" <> "Watched Email") then - if Confirm(StopWatchingQst, false, xRec."Watched Email") then + if "Watched Email" = xRec."Watched Email" then + exit; + if xRec."Watched Email" <> '' then + if Confirm(StopWatchingQst, false, xRec."Watched Email") then begin Unsubscribe("Subscription ID"); - // On "no" the old subscription is never removed, and the only field - // that tracked it is overwritten here: it is orphaned. - "Subscription ID" := Subscribe("Watched Email"); + Clear("Subscription ID"); + end; + // On "no" the old subscription is never removed, yet the new value is + // kept and the only field that tracked it is overwritten: it is orphaned. + if "Watched Email" <> '' then + "Subscription ID" := Subscribe("Watched Email"); end; } field(3; "Subscription ID"; Guid) diff --git a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al index 2454a59..6d3d3d4 100644 --- a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al +++ b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.good.al @@ -9,15 +9,19 @@ table 50100 "Sample Mailbox Watch" { trigger OnValidate() var - ReplaceWatchQst: Label 'Email %1 is being watched. Stop watching it and watch %2 instead?', Comment = '%1 = previous email address, %2 = new email address'; + StopWatchingQst: Label 'Email %1 is being watched. Stop watching it?', Comment = '%1 = previous email address'; begin - if (xRec."Watched Email" <> '') and (xRec."Watched Email" <> "Watched Email") then begin + if "Watched Email" = xRec."Watched Email" then + exit; + if xRec."Watched Email" <> '' then begin // Declining cancels the whole change: the field keeps its old value. - if not Confirm(ReplaceWatchQst, false, xRec."Watched Email", "Watched Email") then + if not Confirm(StopWatchingQst, false, xRec."Watched Email") then Error(''); Unsubscribe("Subscription ID"); + Clear("Subscription ID"); end; - "Subscription ID" := Subscribe("Watched Email"); + if "Watched Email" <> '' then + "Subscription ID" := Subscribe("Watched Email"); end; } field(3; "Subscription ID"; Guid) diff --git a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md index dcedb57..54efeb2 100644 --- a/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md +++ b/microsoft/knowledge/error-handling/declined-confirm-must-abort-not-partially-apply.md @@ -11,24 +11,24 @@ application-area: [all] ## Description -By the time a field's `OnValidate` body runs, the field already holds its new value. When the trigger asks the user to confirm a side effect that the new value makes **required for consistency** — releasing or replacing state that is tied to the old value and that nothing will reference after the change — the shape `if Confirm(...) then ;` with nothing on the `false` branch lets the new value commit while silently skipping that effect. No error is raised, the user sees no indication that anything was declined, and the record is left inconsistent with its own dependents. +By the time a field's `OnValidate` body runs, the field already holds its new value. When the trigger asks the user to confirm a side effect that the new value makes **required for consistency** — releasing or replacing state that is tied to the old value and that nothing will reference any more once the change commits — the shape `if Confirm(...) then ;` with nothing on the `false` branch lets the new value commit while silently skipping that effect. No error is raised, the user sees no indication that anything was declined, and the record is left inconsistent with its own dependents. -Not every confirmed follow-up is required. When the confirmed effect is a convenience the user may legitimately decline, skipping it is correct: `"Sales Header"`'s `UpdateSalesLinesByFieldNo` asks whether to update the lines after a header field changes and, on "no", simply `exit`s — the header keeps its new value and the lines stay as they were, by design. A `Confirm` at the top of an action procedure, before anything has been written, may also just `exit` on "no" (for example the delete action in `"Test Input Groups"`). Neither shape is this anti-pattern. +Not every confirmed follow-up is required. When the confirmed effect is a convenience the user may legitimately decline, skipping it is correct: `"Sales Header"`'s `UpdateSalesLinesByFieldNo` asks whether to update the lines after a header field changes and, on "no", simply `exit`s — the header keeps its new value and the lines stay as they were, by design. A `Confirm` at the top of an action procedure, before anything has been written, may also just `exit` on "no" (for example the delete action in `"Test Input Groups"`). Neither shape is this anti-pattern. Nor is a declined update whose old state stays valid: `Opportunity`'s `"Campaign No."` `OnValidate` asks before moving open tasks filtered on `xRec."Campaign No."` to the new campaign and does nothing on "no" — the tasks keep pointing at a campaign that still exists. ## Best Practice Make the declined branch match what "no" means: -- **Cancel the whole change** — raise an error before the side effect. The usual BCApps form inside `OnValidate` is the silent abort `if not Confirm(...) then Error('');` (for example `"Bank Account"`, field `"Disable Bank Rec. Optimization"`). The field trigger documentation states that in case of an error "the user entry is not written to the database." -- **Keep the old value but let the rest of the edit continue** — assign the field back in code. `"Upload And Deploy Extension"` resets its sync-mode value to `Add` when the user declines `Force Sync`. +- **Cancel the whole change** — raise an error before the side effect. The usual BCApps form inside `OnValidate` is the silent abort `if not Confirm(...) then Error('');` (for example `"Bank Account"`, field `"Disable Bank Rec. Optimization"`, and `"Interaction Template"`, field `"Language Code (Default)"`, which ends `if Confirm(...) then begin ... end else Error('');`). The field trigger documentation states that in case of an error "the user entry is not written to the database." +- **Keep the old value but let the rest of the edit continue** — assign the field back in code. In the `"To-do"` table, field `"Team Code"`, declining the reassignment runs `"Team Code" := xRec."Team Code"`; on a page, `"Upload And Deploy Extension"` resets its sync-mode value to `Add` when the user declines `Force Sync`. -Ask before the side effect runs, and before taking locks the prompt would hold open (see [`avoid-user-prompts-inside-transactions`](../performance/avoid-user-prompts-inside-transactions.md)). +Ask before the side effect runs, and before taking locks the prompt would hold open (see [`avoid-user-prompts-inside-transactions`](../performance/avoid-user-prompts-inside-transactions.md)). When the same validation can run without a UI, a required confirmation must not be silently skipped behind `GuiAllowed`; decide the non-interactive outcome explicitly (see [`job-queue-handlers-must-not-require-ui`](../performance/job-queue-handlers-must-not-require-ui.md)). See sample: [`declined-confirm-must-abort-not-partially-apply.good.al`](declined-confirm-must-abort-not-partially-apply.good.al). ## Anti Pattern -Inside a field `OnValidate` (or a procedure it calls), a `Confirm` gates a side effect that releases, cancels, or replaces state belonging to the old value (`xRec`), the `false` branch neither errors nor restores the field, and code after it proceeds as if the change were accepted — for example overwriting the only field that tracks the old state. Declining leaves that state orphaned. Do not flag optional follow-ups whose skipping leaves every record consistent, or `exit` on "no" in an action before any write. +Inside a field `OnValidate` (or a procedure it calls), a `Confirm` gates a side effect that releases, cancels, or replaces state belonging to the old value (`xRec`), the `false` branch neither errors nor restores the field, and code after it proceeds as if the change were accepted — for example overwriting the only field that tracks the old state. Flag it only when, after the change, nothing references the old state any more, so declining leaves it orphaned. Do not flag declined updates whose old state remains valid and referenced, optional follow-ups whose skipping leaves every record consistent, or `exit` on "no" in an action before any write. See sample: [`declined-confirm-must-abort-not-partially-apply.bad.al`](declined-confirm-must-abort-not-partially-apply.bad.al). @@ -36,6 +36,9 @@ See sample: [`declined-confirm-must-abort-not-partially-apply.bad.al`](declined- - [OnValidate (Field) trigger](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/triggers-auto/field/devenv-onvalidate-field-trigger). - BCApps `src/Layers/W1/BaseApp/Bank/BankAccount/BankAccount.Table.al`, lines 980-988 (silent abort in `OnValidate`). -- BCApps `src/System Application/App/Extension Management/src/UploadAndDeployExtension.Page.al`, lines 78-83 (explicit revert). +- BCApps `src/Layers/W1/BaseApp/CRM/Interaction/InteractionTemplate.Table.al`, lines 157-165 (`else Error('')` in `OnValidate`). +- BCApps `src/Layers/W1/BaseApp/CRM/Task/Todo.Table.al`, lines 80-90 (table-field revert to `xRec`). +- BCApps `src/System Application/App/Extension Management/src/UploadAndDeployExtension.Page.al`, lines 78-83 (explicit revert on a page). +- BCApps `src/Layers/W1/BaseApp/CRM/Opportunity/Opportunity.Table.al`, lines 142-157 (declined update; old campaign still valid). - BCApps `src/Layers/W1/BaseApp/Sales/Document/SalesHeader.Table.al`, `UpdateSalesLinesByFieldNo`, lines 4997-5014 (optional follow-up; `end else exit`). - BCApps `src/Tools/Test Framework/Test Runner/src/DataDrivenTest/DataInputs/TestInputGroups.Page.al`, lines 85-86 (`exit` before any write). diff --git a/microsoft/skills/review/al-data-modeling-review.md b/microsoft/skills/review/al-data-modeling-review.md index 3bb64ce..ec1696f 100644 --- a/microsoft/skills/review/al-data-modeling-review.md +++ b/microsoft/skills/review/al-data-modeling-review.md @@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially `* Setup` singleton tables and Card pages, custom master tables, tableextensions that add master-data fields, document or journal lines that reference a master, document pages/codeunits exposing print/email/Post-and-Send actions, codeunits subscribing to `Navigate`, enumextensions to `"Report Selection Usage"`/`"Price Calculation Handler"`/`"Price Source Type"`, and report objects that render barcodes. - The changed fields, keys, triggers, and procedures, weighted toward `Primary Key`, `No.`, `No. Series`, `Blocked`, `Last Date Modified`, `OnInsert`, `OnModify`, `OnRename`, reference-field `OnValidate`, posting validation, and posting-cascade `TransferFields` calls. -- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `InitRecord`, `Round`, `Precision`, `Direction`, `TableRelation`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`, `TransferFields`, `Navigate`, `OnAfterFindRecords`, `OnBeforeShowRecords`, `Report Selections`, `Report Selection Usage`, `InsertRecord`, `Document Sending Profile`, `PrintForCust`, `PrintWithDialogForCust`, `PrintWithDialogForVend`, `SendEmailToCust`, `SendEmailToVendor`, `Report.RunModal`, `Report.Run`, `Price Calculation Handler`, `Price Calculation`, `OnFindSupportedSetup`, `Price Calculation Setup`, `Price Source Type`, `PriceSourceList`, `OnAfterAddSources`, `UpdateUnitPrice`, `PlanPriceCalcByField`, `UpdateUnitPriceByField`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`, `Insert`, `Delete`, `DeleteAll`, `Currency`, `Customer`). +- Tokens extracted from the diff that relate to data modeling (`setup`, `master`, `Primary Key`, `Code[10]`, `Code[20]`, `AutoIncrement`, `SystemId`, `No.`, `No. Series`, `NoSeriesManagement`, `Codeunit "No. Series"`, `GetNextNo`, `IsManual`, `TestManual`, `Blocked`, `TestField`, `Last Date Modified`, `Today`, `WorkDate`, `InsertAllowed`, `DeleteAllowed`, `PageType = Card`, `OnOpenPage`, `GetRecordOnce`, `OnInsert`, `OnModify`, `OnRename`, `InitRecord`, `Round`, `Precision`, `Direction`, `TableRelation`, `tableextension`, `enumextension`, `Media`, `MediaSet`, `Item`, `Count`, `TransferFields`, `Navigate`, `OnAfterFindRecords`, `OnBeforeShowRecords`, `Report Selections`, `Report Selection Usage`, `InsertRecord`, `Document Sending Profile`, `PrintForCust`, `PrintWithDialogForCust`, `PrintWithDialogForVend`, `SendEmailToCust`, `SendEmailToVendor`, `Report.RunModal`, `Report.Run`, `Price Calculation Handler`, `Price Calculation`, `OnFindSupportedSetup`, `Price Calculation Setup`, `Price Source Type`, `PriceSourceList`, `OnAfterAddSources`, `UpdateUnitPrice`, `PlanPriceCalcByField`, `UpdateUnitPriceByField`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`, `Insert`, `Delete`, `DeleteAll`). A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no data-modeling changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files. @@ -52,7 +52,7 @@ The following targeted checks cover every current `data-modeling` article. Treat - Code reads `Item Ledger Entry."Document No."` (or `"Last Shipping No."`/`"Last Posting No."`) after a combined Ship+Invoice **sales** post — `item-ledger-entry-document-no-follows-last-shipping-no`. This is a sales-specific rule: purchase combined posting is Receive+Invoice and uses receiving fields such as `"Last Receiving No."`, not the shipment/document-number behavior this article describes. Do not worklist it from purchase posting code. - A custom master table changes its primary key, `No.`/`No. Series` fields, or `OnInsert` without assigning a blank `No.` from setup through a number series — `master-table-no-from-number-series-in-oninsert`. - Code outside the table creates a non-temporary master record (`Customer`, `Vendor`, `Item`, `G/L Account`, `Contact`, or a custom master with initializing `OnInsert` logic) with `Insert()`/`Insert(false)` and does not itself assign the number and the fields `OnInsert` would set — `master-data-must-be-inserted-with-trigger`. Do not flag temporary records, buffer/staging tables, a caller that visibly assigns the trigger's fields before applying a template, or an XMLport round-tripping rows exported from Business Central; `Modify()` without the trigger is not this rule. -- Code outside the owning table's own `OnDelete` calls `Delete()`/`DeleteAll()` (or passes `false`) on a non-temporary master or reference table with an `OnDelete` guard or cascade (`Currency`, `Customer`, `Vendor`, `Item`, `G/L Account`, or a custom equivalent) — `delete-master-data-with-trigger`. Do not flag an owning table's `OnDelete` deleting its own dependents, temporary/buffer records, or a deliberate full reset whose caller first clears every reference itself. +- Code outside the owning table's own `OnDelete` calls `Delete()`/`DeleteAll()` (or passes `false`) on a non-temporary master or reference table with an `OnDelete` guard or cascade (`Currency`, `Customer`, `Vendor`, `Item`, `G/L Account`, or a custom equivalent) — `delete-master-data-with-trigger`. Do not flag an owning table's `OnDelete` deleting its own dependents, temporary/buffer records, deleting and immediately re-inserting the same primary key (restore/recreate) where dependents stay valid, or a data-migration/setup reset in a company with no posted entries that removes master rows and their setup references as one rebuild. - BC v22 or later code introduces or retains `NoSeriesManagement`, `InitSeries`, `SelectSeries`, or `SetSeries`, or number assignment/manual-entry checks do not use codeunit `"No. Series"` methods such as `GetNextNo`, `IsManual`, or `TestManual` — `use-no-series-codeunit-not-noseriesmanagement`. - A master gains or changes `Blocked`, or a document line, journal line, reference-field `OnValidate`, or posting routine uses that master without `TestField(Blocked, false)` at the point of use; also cue when the check is placed only in the master's own triggers — `check-blocked-in-referencing-code-not-in-master`. - A master table adds or changes `Last Date Modified`, `OnModify`, or `OnRename`, but the non-editable field is not assigned `Today()` in both triggers — `set-last-date-modified-in-onmodify-and-onrename`. diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index fb08a8d..2af0cf4 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -54,7 +54,7 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `error-handling` article: -- A field `OnValidate` (or a procedure it calls) uses `Confirm` to gate a side effect that releases, cancels, or replaces state tied to the old (`xRec`) value, and the declined branch neither raises an error nor restores the field while the change proceeds — `declined-confirm-must-abort-not-partially-apply`. Do not flag optional follow-ups whose skipping leaves every record consistent (such as declining to update document lines after a header change), or `exit` on a declined `Confirm` in an action before anything is written. +- A field `OnValidate` (or a procedure it calls) uses `Confirm` to gate a side effect that releases, cancels, or replaces state tied to the old (`xRec`) value, after the change nothing references that old state any more (it is orphaned), and the declined branch neither raises an error nor restores the field while the change proceeds — `declined-confirm-must-abort-not-partially-apply`. Do not flag declined updates whose old state stays valid (such as leaving tasks on a still-existing old campaign), optional follow-ups whose skipping leaves every record consistent (such as declining to update document lines after a header change), or `exit` on a declined `Confirm` in an action before anything is written. - `[ErrorBehavior(ErrorBehavior::Collect)]`, `ErrorInfo.Collectible`, `HasCollectedErrors`, `GetCollectedErrors`, or `ClearCollectedErrors` is added or changed, especially when errors are collected without later surfacing/clearing them — `collect-validation-errors-with-errorbehavior`. - New or changed code inserts an error/duration log record around a failed `TryFunction`/`GetLastErrorText`/`GetLastErrorCode` path and then raises, propagates, or rethrows the error — `log-writes-must-survive-rollback`. Do not worklist it when the log insert already happens inside a `Session.StartSession`-targeted codeunit's `OnRun`; that is the compliant shape, not the signal to flag. - A guarded lookup (`if Record.Get(...) then ... else` or similar) sets a value used later, and the same guard shape (with the same blank/zero fallback style) is applied to a field that feeds a posted amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output — `defensive-vs-offensive-code-must-match-blast-radius`. The signal is a posting-critical or compliance-facing field guarded defensively with a silent fallback, not the mere presence of a guarded lookup.