mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
Address review: self-contained samples, fixture registration, scoped feature routing
- Declare the sample table in both companions so each compiles on its own. - Register no-changecompany-in-upgrade and changecompany-runs-triggers-in-the-calling-company in review-fixtures.json. - Limit the Feature Data Update rule to UpdateData/AfterUpdate and their reachable helpers; read-only IsDataUpdateRequired/ReviewData preflight is permitted and shown as a clean control. - Add feature data update execution to al-upgrade-review applicability and not-applicable scope. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
parent
4e33ca87e2
commit
48bd2e2e1b
5 changed files with 95 additions and 11 deletions
|
|
@ -1,3 +1,20 @@
|
|||
table 50263 "Sales Order Ext"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; "Shipping Agent Code"; Code[10]) { TableRelation = "Shipping Agent"; }
|
||||
field(3; "Legacy Carrier Code"; Code[10]) { }
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50261 "Upgrade All Companies"
|
||||
{
|
||||
Subtype = Upgrade;
|
||||
|
|
|
|||
|
|
@ -1,3 +1,20 @@
|
|||
table 50262 "Sales Order Ext"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; "Shipping Agent Code"; Code[10]) { TableRelation = "Shipping Agent"; }
|
||||
field(3; "Legacy Carrier Code"; Code[10]) { }
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50260 "Upgrade Current Company"
|
||||
{
|
||||
Subtype = Upgrade;
|
||||
|
|
@ -32,3 +49,48 @@ codeunit 50260 "Upgrade Current Company"
|
|||
exit('CONTOSO-1001-ShippingAgentCode-20260101');
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50264 "Shipping Agent Feat. Data Upd." implements "Feature Data Update"
|
||||
{
|
||||
// Read-only preflight: counting rows across companies to report scope is allowed.
|
||||
procedure IsDataUpdateRequired(): Boolean
|
||||
var
|
||||
Company: Record Company;
|
||||
SalesOrderExt: Record "Sales Order Ext";
|
||||
begin
|
||||
if Company.FindSet() then
|
||||
repeat
|
||||
SalesOrderExt.ChangeCompany(Company.Name);
|
||||
SalesOrderExt.SetRange("Shipping Agent Code", '');
|
||||
if not SalesOrderExt.IsEmpty() then
|
||||
exit(true);
|
||||
until Company.Next() = 0;
|
||||
exit(false);
|
||||
end;
|
||||
|
||||
procedure ReviewData()
|
||||
begin
|
||||
end;
|
||||
|
||||
// Feature Management runs this once per company, in that company.
|
||||
procedure UpdateData(FeatureDataUpdateStatus: Record "Feature Data Update Status")
|
||||
var
|
||||
SalesOrderExt: Record "Sales Order Ext";
|
||||
begin
|
||||
SalesOrderExt.SetRange("Shipping Agent Code", '');
|
||||
if SalesOrderExt.FindSet(true) then
|
||||
repeat
|
||||
SalesOrderExt.Validate("Shipping Agent Code", SalesOrderExt."Legacy Carrier Code");
|
||||
SalesOrderExt.Modify(true);
|
||||
until SalesOrderExt.Next() = 0;
|
||||
end;
|
||||
|
||||
procedure AfterUpdate(FeatureDataUpdateStatus: Record "Feature Data Update Status")
|
||||
begin
|
||||
end;
|
||||
|
||||
procedure GetTaskDescription(): Text
|
||||
begin
|
||||
exit('Copies legacy carrier codes to the shipping agent code.');
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -11,19 +11,19 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
The platform upgrades each company in its own context: `OnUpgradePerCompany` (like the other `PerCompany` upgrade and install triggers) runs once per company, in a separate system session opened for that company, while `PerDatabase` triggers run in a session that opens no company. Calling `ChangeCompany(<name>)` in upgrade code reaches from the company being upgraded into another company. That company has its own upgrade session, so the work is either duplicated or racing it. The platform does not document an order between companies, so a read can see data in the pre-upgrade or post-upgrade shape. Per-company upgrade tags are set for the current company only, so a tag set after writing to company B is recorded for company A, and B's own session runs the migration again. Execution context does not move with `ChangeCompany`: table triggers and trigger-event subscribers still run in the calling company (see `changecompany-runs-triggers-in-the-calling-company`). Data and side effects then land in different companies, and an error in another company's data aborts the current company's upgrade. The same applies to feature-switch data updates, the `UpdateData` implementation of the `Feature Data Update` interface. Feature Management schedules that update as one task per company.
|
||||
The platform upgrades each company in its own context: `OnUpgradePerCompany` (like the other `PerCompany` upgrade and install triggers) runs once per company, in a separate system session opened for that company, while `PerDatabase` triggers run in a session that opens no company. Calling `ChangeCompany(<name>)` in upgrade code reaches from the company being upgraded into another company. That company has its own upgrade session, so the work is either duplicated or racing it. The platform does not document an order between companies, so a read can see data in the pre-upgrade or post-upgrade shape. Per-company upgrade tags are set for the current company only, so a tag set after writing to company B is recorded for company A, and B's own session runs the migration again. Execution context does not move with `ChangeCompany`: table triggers and trigger-event subscribers still run in the calling company (see `changecompany-runs-triggers-in-the-calling-company`). Data and side effects then land in different companies, and an error in another company's data aborts the current company's upgrade. The same applies to the migration path of a feature-switch data update: the `UpdateData` and `AfterUpdate` methods of a `Feature Data Update` implementation. Feature Management runs them for one company's status row, either as a task scheduled in that company or in the current session. The interface's preflight methods, `IsDataUpdateRequired` and `ReviewData`, are a separate phase that reports scope before any update is scheduled.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Upgrade code operates only on the company it is running in. Put per-company data migration in `OnUpgradePerCompany` (or a helper reachable only from it), guard it with a per-company upgrade tag, and let the platform invoke it for every company. Reserve `OnUpgradePerDatabase` for tables with `DataPerCompany = false` and other database-wide state; it needs no `ChangeCompany`. A feature data update implements `UpdateData` against the current company only. When code outside the upgrade pipeline must start work in other companies, schedule it in each company with `TaskScheduler.CreateTask` and the company name, as Feature Management does, rather than writing there through `ChangeCompany`. The scheduled task runs in the target company, so triggers, events, permissions, and upgrade tags all use that company.
|
||||
Upgrade code operates only on the company it is running in. Put per-company data migration in `OnUpgradePerCompany` (or a helper reachable only from it), guard it with a per-company upgrade tag, and let the platform invoke it for every company. Reserve `OnUpgradePerDatabase` for tables with `DataPerCompany = false` and other database-wide state; it needs no `ChangeCompany`. A feature data update implements `UpdateData` and `AfterUpdate` against the current company only. Its read-only preflight may use `ChangeCompany` to count or inspect rows in other companies, because it migrates nothing. When code outside the upgrade pipeline must start work in other companies, schedule it in each company with `TaskScheduler.CreateTask` and the company name, as Feature Management does, rather than writing there through `ChangeCompany`. The scheduled task runs in the target company, so triggers, events, permissions, and upgrade tags all use that company.
|
||||
|
||||
See sample: [`no-changecompany-in-upgrade.good.al`](no-changecompany-in-upgrade.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A loop over the `Company` table in `OnUpgradePerDatabase` or `OnUpgradePerCompany` that calls `ChangeCompany(Company.Name)` on a record or `RecordRef` and then reads, inserts, modifies, or deletes data. Another form is a `Feature Data Update` implementation that does the same to update every company from one task. Reads are not exempt: they observe another company whose upgrade state is unknown.
|
||||
A loop over the `Company` table in `OnUpgradePerDatabase` or `OnUpgradePerCompany` that calls `ChangeCompany(Company.Name)` on a record or `RecordRef` and then reads, inserts, modifies, or deletes data. Another form is a `Feature Data Update` implementation whose `UpdateData` or `AfterUpdate` does the same to update every company from one task. On these paths reads are not exempt: they observe another company whose upgrade state is unknown.
|
||||
|
||||
Detection signal: `Record.ChangeCompany(<name>)` or `RecordRef.ChangeCompany(<name>)` with a company-name argument in a codeunit with `Subtype = Upgrade`, in any procedure transitively reachable from its upgrade triggers, or in a codeunit implementing the `Feature Data Update` interface. Do not flag the parameterless `ChangeCompany()`, which only points the variable back at the current company, or `ChangeCompany` in ordinary runtime code; `changecompany-runs-triggers-in-the-calling-company` governs that.
|
||||
Detection signal: `Record.ChangeCompany(<name>)` or `RecordRef.ChangeCompany(<name>)` with a company-name argument in a codeunit with `Subtype = Upgrade` or in any procedure transitively reachable from its upgrade triggers, or in a `Feature Data Update` implementation's `UpdateData` or `AfterUpdate` or any procedure transitively reachable from them. Do not flag `ChangeCompany` in `IsDataUpdateRequired`, `ReviewData`, or helpers reachable only from them; a helper shared with `UpdateData` or `AfterUpdate` is in scope. Do not flag the parameterless `ChangeCompany()`, which only points the variable back at the current company, or `ChangeCompany` in ordinary runtime code; `changecompany-runs-triggers-in-the-calling-company` governs that.
|
||||
|
||||
See sample: [`no-changecompany-in-upgrade.bad.al`](no-changecompany-in-upgrade.bad.al).
|
||||
|
||||
|
|
@ -32,4 +32,5 @@ See sample: [`no-changecompany-in-upgrade.bad.al`](no-changecompany-in-upgrade.b
|
|||
- Upgrading extensions, Upgrade triggers: `PerCompany` triggers run once per company, each in its own system session for that company — https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/devenv-upgrading-extensions
|
||||
- Record.ChangeCompany method, Remarks: triggers still run in the current company — https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-changecompany-method
|
||||
- Interface "Feature Data Update" — https://learn.microsoft.com/en-us/dynamics365/business-central/application/system-application/interface/system.environment.configuration.feature-data-update
|
||||
- `FeatureManagementImpl.UpdateData` calls the interface's `UpdateData` then `AfterUpdate`; `ReviewData` calls `IsDataUpdateRequired` and `ReviewData` — https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/Feature%20Key/src/FeatureManagementImpl.Codeunit.al
|
||||
- `FeatureManagementImpl.CreateTask` schedules `Update Feature Data` with `TaskScheduler.CreateTask` for the status row's company — https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/Feature%20Key/src/FeatureManagementImpl.Codeunit.al
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue