mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
Tighten declined-confirm, delete and insert trigger articles after review
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
parent
a0d23a982c
commit
4e2a3ead4d
8 changed files with 42 additions and 28 deletions
|
|
@ -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.`).
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue