mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
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 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer
parent
fd59919778
commit
a0d23a982c
12 changed files with 275 additions and 4 deletions
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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.
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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;
|
||||
}
|
||||
|
|
@ -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 <side effect>;` 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).
|
||||
Loading…
Add table
Add a link
Reference in a new issue