mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
Merge upstream/main to resolve conflict with #203/#207
Insert-only conflicts in al-data-modeling-review.md (scope line, token list, not-applicable list) and evaluation/review-fixtures.json: both sides kept. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer
commit
49b14834c4
26 changed files with 1410 additions and 44 deletions
|
|
@ -0,0 +1,33 @@
|
|||
// Demonstration only; independently authored, not copied from BaseApp.
|
||||
codeunit 50641 "Sales Doc. VAT Basis Bad"
|
||||
{
|
||||
procedure GetNetAndGrossTotals(SalesHeader: Record "Sales Header"; var NetTotal: Decimal; var GrossTotal: Decimal)
|
||||
var
|
||||
SalesLine: Record "Sales Line";
|
||||
begin
|
||||
SalesLine.SetRange("Document Type", SalesHeader."Document Type");
|
||||
SalesLine.SetRange("Document No.", SalesHeader."No.");
|
||||
if SalesLine.FindSet() then
|
||||
repeat
|
||||
// Wrong: "Line Amount" already includes VAT when the header has Prices Including VAT,
|
||||
// so NetTotal is gross and VAT is added a second time. Invoice discount is also ignored.
|
||||
NetTotal += SalesLine."Line Amount";
|
||||
GrossTotal += SalesLine."Line Amount" * (1 + SalesLine."VAT %" / 100);
|
||||
until SalesLine.Next() = 0;
|
||||
end;
|
||||
|
||||
procedure GetOutstandingNetAmount(SalesLine: Record "Sales Line"): Decimal
|
||||
begin
|
||||
// Wrong: despite its name, CalculateOutstandingAmountExclTax is based on "Line Amount"
|
||||
// and therefore includes VAT on a Prices Including VAT document.
|
||||
exit(SalesLine.CalculateOutstandingAmountExclTax());
|
||||
end;
|
||||
|
||||
procedure SetUnitPriceFromNetSourcePrice(var SalesLine: Record "Sales Line"; NetSourcePrice: Decimal)
|
||||
begin
|
||||
// Wrong: on a Prices Including VAT document this net price is read as a gross price,
|
||||
// so the net line amount drops to NetSourcePrice / (1 + "VAT %" / 100).
|
||||
SalesLine.Validate("Unit Price", NetSourcePrice);
|
||||
SalesLine.Modify(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,52 @@
|
|||
// Demonstration only; independently authored, not copied from BaseApp.
|
||||
codeunit 50640 "Sales Doc. VAT Basis Good"
|
||||
{
|
||||
procedure GetNetAndGrossTotals(SalesHeader: Record "Sales Header"; var NetTotal: Decimal; var GrossTotal: Decimal)
|
||||
var
|
||||
SalesLine: Record "Sales Line";
|
||||
begin
|
||||
SalesLine.SetRange("Document Type", SalesHeader."Document Type");
|
||||
SalesLine.SetRange("Document No.", SalesHeader."No.");
|
||||
// Amount is always net and "Amount Including VAT" always gross, after line and
|
||||
// invoice discounts, whatever the header's "Prices Including VAT" says.
|
||||
SalesLine.CalcSums(Amount, "Amount Including VAT");
|
||||
NetTotal := SalesLine.Amount;
|
||||
GrossTotal := SalesLine."Amount Including VAT";
|
||||
end;
|
||||
|
||||
procedure GetOutstandingNetAmount(SalesLine: Record "Sales Line"): Decimal
|
||||
var
|
||||
SalesHeader: Record "Sales Header";
|
||||
Currency: Record Currency;
|
||||
begin
|
||||
if SalesLine.Quantity = 0 then
|
||||
exit(0);
|
||||
SalesHeader.Get(SalesLine."Document Type", SalesLine."Document No.");
|
||||
Currency.Initialize(SalesHeader."Currency Code");
|
||||
// Amount is net after line and invoice discounts on every document, so the
|
||||
// uninvoiced share needs no VAT conversion.
|
||||
exit(Round(
|
||||
SalesLine.Amount * (SalesLine.Quantity - SalesLine."Quantity Invoiced") / SalesLine.Quantity,
|
||||
Currency."Amount Rounding Precision"));
|
||||
end;
|
||||
|
||||
procedure SetUnitPriceFromNetSourcePrice(var SalesLine: Record "Sales Line"; NetSourcePrice: Decimal)
|
||||
var
|
||||
SalesHeader: Record "Sales Header";
|
||||
Currency: Record Currency;
|
||||
UnitPrice: Decimal;
|
||||
begin
|
||||
SalesHeader.Get(SalesLine."Document Type", SalesLine."Document No.");
|
||||
// Scope of the conversion below: Normal VAT only; Full VAT and Sales Tax need their own handling.
|
||||
SalesLine.TestField("VAT Calculation Type", SalesLine."VAT Calculation Type"::"Normal VAT");
|
||||
Currency.Initialize(SalesHeader."Currency Code");
|
||||
|
||||
UnitPrice := NetSourcePrice;
|
||||
// "Unit Price" is gross on a Prices Including VAT document: convert the net source price into that basis.
|
||||
if SalesHeader."Prices Including VAT" then
|
||||
UnitPrice := Round(NetSourcePrice * (1 + SalesLine."VAT %" / 100), Currency."Unit-Amount Rounding Precision");
|
||||
|
||||
SalesLine.Validate("Unit Price", UnitPrice);
|
||||
SalesLine.Modify(true);
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,49 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: data-modeling
|
||||
keywords: [prices-including-vat, unit-price, line-amount, direct-unit-cost, prepmt-line-amount, amount-including-vat, sales-line, purchase-line, service-line, net-gross]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Sales, purchase, and service line prices follow the header's Prices Including VAT
|
||||
|
||||
> Contributions welcome — open a PR to refine or extend this article.
|
||||
|
||||
## Description
|
||||
|
||||
Line prices are gross or net depending on the header's `Prices Including VAT`. When the flag is set, `Unit Price` (sales, service), `Direct Unit Cost` (purchase), `Line Amount`, `Line Discount Amount`, `Inv. Discount Amount`, and the prepayment fields `Prepmt. Line Amount`, `Prepmt. Amt. Inv.`, `Prepmt Amt to Deduct`, and `Prepmt Amt Deducted` all include VAT. When it is cleared they exclude it. Only `Amount`, `VAT Base Amount`, and `Prepayment Amount` (always net) and `Amount Including VAT`, `Prepmt. Amt. Incl. VAT`, and `Prepmt. Amount Inv. Incl. VAT` (always gross) keep a fixed basis. Code that mixes the two groups without looking at the header flag is wrong for every document whose customer or vendor uses the other setting.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When code needs a known basis — exports, integrations, KPIs, custom totals, commission or margin calculations — read `Amount` for net and `Amount Including VAT` for gross. Both are already reduced by line and invoice discounts and are maintained by the line's VAT calculation, so no VAT arithmetic is needed.
|
||||
|
||||
When code combines fields, pair fields of the same group. BaseApp's `UpdatePrepmtAmounts` sets `Prepmt. Line Amount` from `Line Amount` minus `Inv. Discount Amount`. This is correct because all three follow the header flag. `Prepmt. Amt. Inv.` pairs with `Prepmt. Line Amount`; `Prepmt. Amount Inv. Incl. VAT` pairs only with other gross values.
|
||||
|
||||
When code writes a price from an external source whose basis is known (an EDI price list, an API payload, a web-shop order), get the document header and convert the source price into the header's basis before validating `Unit Price` or `Direct Unit Cost`, using the line's `VAT %` and the currency's `Unit-Amount Rounding Precision`. Alternatively, set `Prices Including VAT` on the header to match the source before the first line is created. Toggling it later on a sales header with priced lines asks the user to confirm a recalculation. Without a UI session, or when validation dialogs are hidden, BaseApp converts all line prices without asking.
|
||||
|
||||
The standard price calculation already converts a `Price List Line` whose `Price Includes VAT` differs from the document's setting, so a price it returns is in the document's basis and must not be converted a second time.
|
||||
|
||||
On purchase lines, `Unit Cost` and `Unit Cost (LCY)` are derived from `Direct Unit Cost` with VAT removed, so they stay net. Service lines follow the same rule for `Unit Price` and `Line Amount`: they share the caption switch and the `UpdateVATAmounts` split of the sales line.
|
||||
|
||||
See sample: [`document-line-prices-follow-prices-including-vat.good.al`](document-line-prices-follow-prices-including-vat.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Treating `Unit Price`, `Direct Unit Cost`, or `Line Amount` as net by default. Typical signals: summing `Line Amount` as a document's net total, computing VAT as `Line Amount * "VAT %" / 100`, putting a known net or gross external price straight into `Unit Price` or `Direct Unit Cost`, or comparing a line price with `Item."Unit Price"` or `Item."Last Direct Cost"` — all without reading the header's `Prices Including VAT`. For a gross-price customer at 19 % VAT, a net total built from `Line Amount` is 19 % too high, and a net price of 100 imported unconverted yields a net revenue of only 84.03.
|
||||
|
||||
Trusting a name instead of the source fields. The public procedure `CalculateOutstandingAmountExclTax` on `Sales Line` and `Purchase Line` returns `Line Amount` minus `Inv. Discount Amount` for the uninvoiced quantity, so its result includes VAT on a `Prices Including VAT` document despite its name. BaseApp only combines it with `Prepmt. Line Amount`, which has the same basis. Extension code that uses it as a net outstanding amount — compared with `Amount`, exported as net, or used to compute VAT — has this defect. The same applies to any variable or procedure named `ExclVAT`, `ExclTax`, or `Net` that is fed from a header-dependent field.
|
||||
|
||||
Do not report code that reads the header flag, uses only fixed-basis fields, or only combines fields of the same group (for example, prepayment amounts derived from `Line Amount`, or values copied between two lines of the same document).
|
||||
|
||||
See sample: [`document-line-prices-follow-prices-including-vat.bad.al`](document-line-prices-follow-prices-including-vat.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [BCApps: Sales Header `Prices Including VAT` OnValidate recalculates line prices](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Sales/Document/SalesHeader.Table.al).
|
||||
- [BCApps: Sales Line `UpdateVATAmounts`, `UpdatePrepmtAmounts`, and `CalculateOutstandingAmountExclTax`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Sales/Document/SalesLine.Table.al).
|
||||
- [BCApps: `Sales Line CaptionClass Mgmt` switches captions to Incl./Excl. VAT](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Sales/Document/SalesLineCaptionClassMgmt.Codeunit.al).
|
||||
- [BCApps: Purchase Line `UpdateUnitCost` removes VAT from `Direct Unit Cost`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Purchases/Document/PurchaseLine.Table.al).
|
||||
- [BCApps: Service Line `GetCaptionClass` and `UpdateVATAmounts`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Service/Document/ServiceLine.Table.al).
|
||||
- [BCApps: `Price Calculation Buffer Mgt.` `ConvertAmountByTax`](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Pricing/Calculation/PriceCalculationBufferMgt.Codeunit.al).
|
||||
|
|
@ -13,7 +13,7 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
`ChangeCompany` redirects the data access of one record variable to another company's table. Execution context does not move with it: Microsoft Learn states that triggers still run in the current company, not in the company passed to `ChangeCompany`. Code that knows this usually reaches for `Insert(false)` and copies the trigger's work by hand from the target company's setup. That closes only half of the gap. The runtime raises the database trigger events (`OnBeforeInsertEvent`, `OnAfterInsertEvent`, and their modify, delete, and rename counterparts) on every database operation and only passes the `RunTrigger` flag to the subscriber, so every subscriber that does not exit on `RunTrigger = false` still runs, in the calling company, against the calling company's setup, number series, and companion tables. The row lands in the target company, the side effects land in the caller, and nothing reports an error. The per-row cost of the call is a separate concern, see `changecompany-in-loop-drops-caches`.
|
||||
`ChangeCompany` redirects the data access of one record variable to another company's table. Execution context does not move with it: Microsoft Learn states that triggers still run in the current company, not in the company passed to `ChangeCompany`. Code that knows this usually reaches for `Insert(false)` and copies the trigger's work by hand from the target company's setup. That closes only half of the gap. The runtime raises the database trigger events (`OnBeforeInsertEvent`, `OnAfterInsertEvent`, and their modify, delete, and rename counterparts) on every database operation and only passes the `RunTrigger` flag to the subscriber, so every subscriber that does not exit on `RunTrigger = false` still runs, in the calling company, against the calling company's setup, number series, and companion tables. The row lands in the target company, the side effects land in the caller, and nothing reports an error. The per-row cost of the call is a separate concern, see `changecompany-in-loop-drops-caches`. Upgrade code must not use `ChangeCompany` at all, see `no-changecompany-in-upgrade`.
|
||||
|
||||
## Best Practice
|
||||
|
||||
|
|
|
|||
|
|
@ -8,4 +8,13 @@ codeunit 50211 "Perf Sample GetByPK Bad"
|
|||
if Customer.FindFirst() then
|
||||
Message(Customer.Name);
|
||||
end;
|
||||
|
||||
procedure ShowUnblockedName(CustomerNo: Code[20])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.SetRange(Blocked, Customer.Blocked::" ");
|
||||
if Customer.Get(CustomerNo) then
|
||||
Message(Customer.Name);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -7,4 +7,25 @@ codeunit 50210 "Perf Sample GetByPK Good"
|
|||
if Customer.Get(CustomerNo) then
|
||||
Message(Customer.Name);
|
||||
end;
|
||||
|
||||
procedure ShowUnblockedName(CustomerNo: Code[20])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.SetRange("No.", CustomerNo);
|
||||
Customer.SetRange(Blocked, Customer.Blocked::" ");
|
||||
if Customer.FindFirst() then
|
||||
Message(Customer.Name);
|
||||
end;
|
||||
|
||||
procedure ShowUnblockedNameByKey(CustomerNo: Code[20])
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
if not Customer.Get(CustomerNo) then
|
||||
exit;
|
||||
if Customer.Blocked <> Customer.Blocked::" " then
|
||||
exit;
|
||||
Message(Customer.Name);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,26 +1,34 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: performance
|
||||
keywords: [get, findfirst, primary-key, setrange, lookup]
|
||||
keywords: [get, findfirst, primary-key, setrange, lookup, filters, blocked]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Use Get when the full primary key is known; FindFirst is the wrong tool
|
||||
# Use Get for primary-key lookups without losing filter conditions
|
||||
|
||||
## Description
|
||||
|
||||
`Get(...)` is the direct primary-key lookup. `FindFirst()` walks an index — even when narrowed by `SetRange` on every primary-key field. The upstream review guidance treats `Customer.SetRange("No.", CustomerNo); if Customer.FindFirst() then ...` as a bad pattern and `if Customer.Get(CustomerNo) then ...` as the correction. The two reach the same record; only `Get` expresses the lookup as a primary-key seek.
|
||||
`Get(...)` retrieves a record by primary key, but ignores normal record filters. Replacing a `FindFirst()` filtered only by the full primary key with `Get` expresses the lookup directly. The same replacement is not equivalent when additional filters enforce business conditions, such as requiring an unblocked customer. Security filters are a separate mechanism: their effect on `Get` depends on Security Filter Mode.
|
||||
|
||||
## Best Practice
|
||||
|
||||
When all primary-key fields are available at the call site, call `Get` (or `GetBySystemId`) with them. Reserve `FindFirst` for cases where the filter is on something other than the full primary key — a unique secondary field, a partial composite key, a sort that the caller cares about.
|
||||
When all primary-key fields are available and no additional normal filter constrains the result, call `Get` with them. Reserve `FindFirst` for filtered searches, including partial keys, secondary fields, or additional business conditions.
|
||||
|
||||
Before recommending a replacement, inspect the effective filters at the call site, including filters set by callers or helpers. If additional conditions matter, retain the filtered `FindFirst` or explicitly enforce equivalent conditions after a successful `Get`, before using the record. Do not flag `FindFirst` merely because all primary-key fields are filtered when a non-key filter must also hold. A `SetRange` before `Get` does not enforce that condition.
|
||||
|
||||
See sample: [`use-get-instead-of-findfirst-on-full-primary-key.good.al`](use-get-instead-of-findfirst-on-full-primary-key.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Composing `SetRange` calls that exactly cover the primary key and then calling `FindFirst`. The result is correct but the call site reads as "search the table" rather than "look up by key", which obscures both the intent and the access pattern from later reviewers.
|
||||
Composing `SetRange` calls that cover only the full primary key and then calling `FindFirst` obscures a direct key lookup. Do not extend this finding to a lookup with additional business filters unless the proposed correction preserves them.
|
||||
|
||||
Replacing a filtered lookup with `Get` while assuming a normal filter still excludes records is a correctness defect: a blocked or otherwise ineligible record can pass the lookup. Recommending that replacement without preserving the condition is also an incorrect review finding.
|
||||
|
||||
See sample: [`use-get-instead-of-findfirst-on-full-primary-key.bad.al`](use-get-instead-of-findfirst-on-full-primary-key.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [Record.Get remarks: primary-key lookup and filter semantics](https://learn.microsoft.com/en-us/dynamics365/business-central/dev-itpro/developer/methods-auto/record/record-get-method).
|
||||
|
|
|
|||
|
|
@ -0,0 +1,59 @@
|
|||
enum 50700 "Sample Request Status"
|
||||
{
|
||||
Extensible = false;
|
||||
|
||||
value(0; New) { Caption = 'New'; }
|
||||
value(1; "Needs Review") { Caption = 'Needs Review'; }
|
||||
value(2; Approved) { Caption = 'Approved'; }
|
||||
}
|
||||
|
||||
table 50700 "Sample Request"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; Status; Enum "Sample Request Status") { }
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
||||
page 50700 "Sample Request Card"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = "Sample Request";
|
||||
ApplicationArea = All;
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
field("No."; Rec."No.") { }
|
||||
field(Status; Rec.Status) { }
|
||||
}
|
||||
}
|
||||
|
||||
actions
|
||||
{
|
||||
area(Processing)
|
||||
{
|
||||
action(Approve)
|
||||
{
|
||||
Caption = 'Approve';
|
||||
// AL0573: InListExpression is not valid for client expressions.
|
||||
Enabled = Rec.Status in [Rec.Status::New, Rec.Status::"Needs Review"];
|
||||
|
||||
trigger OnAction()
|
||||
begin
|
||||
Rec.Status := Rec.Status::Approved;
|
||||
Rec.Modify(true);
|
||||
end;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,83 @@
|
|||
enum 50700 "Sample Request Status"
|
||||
{
|
||||
Extensible = false;
|
||||
|
||||
value(0; New) { Caption = 'New'; }
|
||||
value(1; "Needs Review") { Caption = 'Needs Review'; }
|
||||
value(2; Approved) { Caption = 'Approved'; }
|
||||
}
|
||||
|
||||
table 50700 "Sample Request"
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; Status; Enum "Sample Request Status") { }
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
||||
page 50700 "Sample Request Card"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = "Sample Request";
|
||||
ApplicationArea = All;
|
||||
|
||||
layout
|
||||
{
|
||||
area(Content)
|
||||
{
|
||||
field("No."; Rec."No.")
|
||||
{
|
||||
// A plain field comparison is a valid client expression.
|
||||
Editable = Rec.Status = Rec.Status::New;
|
||||
}
|
||||
field(Status; Rec.Status)
|
||||
{
|
||||
trigger OnValidate()
|
||||
begin
|
||||
UpdateActionStates();
|
||||
end;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
actions
|
||||
{
|
||||
area(Processing)
|
||||
{
|
||||
action(Approve)
|
||||
{
|
||||
Caption = 'Approve';
|
||||
// The list membership is computed in AL and exposed as a global Boolean.
|
||||
Enabled = ApproveEnabled;
|
||||
|
||||
trigger OnAction()
|
||||
begin
|
||||
Rec.Status := Rec.Status::Approved;
|
||||
Rec.Modify(true);
|
||||
UpdateActionStates();
|
||||
end;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
var
|
||||
ApproveEnabled: Boolean;
|
||||
|
||||
trigger OnAfterGetCurrRecord()
|
||||
begin
|
||||
UpdateActionStates();
|
||||
end;
|
||||
|
||||
local procedure UpdateActionStates()
|
||||
begin
|
||||
ApproveEnabled := Rec.Status in [Rec.Status::New, Rec.Status::"Needs Review"];
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,38 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: ui
|
||||
keywords: [client-expression, in-list, inlistexpression, al0573, al0322, enabled, visible, editable, dynamic-enable]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Page client-expression properties must not use an `in [...]` list
|
||||
|
||||
## Description
|
||||
|
||||
`Enabled`, `Visible`, `Editable`, and `StyleExpr` on page controls (the list is not exhaustive; `HideValue` is rejected the same way) can be bound to a client expression instead of a literal. The documented dynamic forms are a global Boolean page variable, a Boolean field, or a Boolean expression over fields such as `"Credit Limit" > "Sales YTD"`; plain `=`/`<>`/`>` comparisons combined with `and`/`or`/`not` are valid. An `in [...]` set-membership test, such as `Rec.Status in [Rec.Status::New, Rec.Status::"Needs Review"]`, is not: the compiler reports it as "InListExpression is not valid for client expressions. Client expressions can only use simple data types and field references."
|
||||
|
||||
The severity depends on the control. On a page field the diagnostic is already an error (AL0322). On an action, group, or part it is AL0573, a warning that "will become an error in a future release", so the code still builds and is easy to ship, suppress in a ruleset, or carry forward. A procedure call in the same property position is rejected by the same diagnostics, so moving the list test into a method called from the property does not fix it.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Express the condition in a form a client expression accepts. For a short list, rewrite the membership as an `or` chain of field comparisons, which keeps the property a live client expression. For a longer or computed condition, evaluate it in AL (an `in [...]` list is fine there), store the result in a global page `Boolean` variable, and bind the property to that variable. Recompute the variable wherever its inputs change: `OnAfterGetCurrRecord` when the current record changes, and the `OnValidate` of each page field the condition reads for in-place edits. Do not use `OnAfterGetRecord` for action, group, or part state: it runs once per row loaded, so on a List or Worksheet page the variable ends up reflecting the last fetched row rather than the selected one (see [OnAfterGetCurrRecord is not per row](../performance/onaftergetcurrrecord-is-not-per-row.md)). `OnAfterGetRecord` is right only for per-row field state inside a repeater.
|
||||
|
||||
For `Visible` on field and action controls, the Visible property documentation requires the variable to be resolved in `OnInit` or `OnOpenPage`; do not rely on per-record recomputation to show and hide those controls. `Enabled` and `Editable` have no such restriction. See sample: [`page-client-expression-must-not-use-in-list.good.al`](page-client-expression-must-not-use-in-list.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A page or pageextension control property `Enabled`, `Visible`, `Editable`, or `StyleExpr` whose value contains `in [`, typically an enum or option field tested against several values. Reviewer signal: the `in [` token appears directly in the property value rather than inside a trigger or procedure body. Replacing it with a call to a procedure that performs the same test is the same defect in a different shape.
|
||||
|
||||
Do not flag plain comparisons joined with `and`/`or`, such as `Enabled = (Rec.Status = Rec.Status::New) or (Rec.Status = Rec.Status::"Needs Review");`; they compile cleanly and Microsoft uses them, for example in BCApps `AccountantExpenseReports.Page.al` and `AgentTaskLogEntry.Page.al`. Do not flag `in [...]` used inside procedures or triggers that assign a Boolean variable. See sample: [`page-client-expression-must-not-use-in-list.bad.al`](page-client-expression-must-not-use-in-list.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [Enabled property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-enabled-property): dynamic values are a Boolean variable, a Boolean field, or a Boolean expression such as "Credit Limit > Sales YTD"; variables must be global page variables.
|
||||
- [OnAfterGetCurrRecord (Page) trigger](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/triggers-auto/page/devenv-onaftergetcurrrecord-page-trigger): in a page with a repeater, called only when the current record is updated, after all `OnAfterGetRecord` calls for the rows.
|
||||
- [Visible property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-visible-property): variables for field and action controls must be resolved by `OnInit` or `OnOpenPage`.
|
||||
- [Compiler warning AL0573](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al573) and [compiler error AL0322](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al322). The Learn pages show only the `{0}` template; the compiler's message for this case is "InListExpression is not valid for client expressions. Client expressions can only use simple data types and field references." (AL compiler 30.0: AL0573 for action, group, and part properties; AL0322 for page field properties, including `HideValue`).
|
||||
- Comparison-based client expressions in BCApps, for example `Enabled = Rec.Status <> Rec.Status::Running;` in [BCPTSetupCard.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Tools/Performance%20Toolkit/App/src/BCPTSetupCard.Page.al). BCApps contains no page client expression that uses an `in [...]` list.
|
||||
- Global Boolean recomputed on current-record change in BCApps: [EDocumentLogs.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/EDocument/App/src/Logging/EDocumentLogs.Page.al), a List page, binds `Enabled = IsExportEnabled;` and sets `IsExportEnabled` in `OnAfterGetCurrRecord`.
|
||||
- Or-chain client expressions in BCApps: `Enabled = (Rec.Status = Rec.Status::"Pending Approval") or (Rec.Status = Rec.Status::"Interim Approved");` in [AccountantExpenseReports.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Apps/W1/ExpenseAgent/app/src/Expense/Pages/AccountantExpenseReports.Page.al) and `Visible = (Rec.Type = Rec.Type::"Output Message Draft") or (Rec.Type = Rec.Type::"Output Message");` in [AgentTaskLogEntry.Page.al](https://github.com/microsoft/BCApps/blob/main/src/System%20Application/App/Agent/Troubleshooting/AgentTaskLogEntry.Page.al).
|
||||
|
|
@ -0,0 +1,23 @@
|
|||
pageextension 50710 "Sample Bus. Mgr. RC Ext" extends "Business Manager Role Center"
|
||||
{
|
||||
layout
|
||||
{
|
||||
addafter(Control16)
|
||||
{
|
||||
part(SampleMyCustomers; "My Customers")
|
||||
{
|
||||
ApplicationArea = Basic, Suite;
|
||||
// AL0573: procedure calls are not valid for client expressions.
|
||||
Visible = CanSeeMyCustomers();
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// AL0569: a page of type Role Center cannot have procedures.
|
||||
local procedure CanSeeMyCustomers(): Boolean
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
exit(Customer.ReadPermission());
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,16 @@
|
|||
pageextension 50710 "Sample Bus. Mgr. RC Ext" extends "Business Manager Role Center"
|
||||
{
|
||||
layout
|
||||
{
|
||||
addafter(Control16)
|
||||
{
|
||||
part(SampleMyCustomers; "My Customers")
|
||||
{
|
||||
ApplicationArea = Basic, Suite;
|
||||
// Declarative permission gating: the part is removed for users
|
||||
// without Read permission on Customer.
|
||||
AccessByPermission = TableData Customer = R;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,32 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: ui
|
||||
keywords: [accessbypermission, rolecenter, role-center, pageextension, client-expression, permission, al0569, al0573, al0378]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Gate Role Center content by permission with AccessByPermission
|
||||
|
||||
## Description
|
||||
|
||||
A page of type `RoleCenter` cannot have triggers (AL0378, error) or procedures (AL0569, warning that will become an error), and the AL compiler applies both rules to a pageextension whose target is a Role Center. That removes the usual way to show a control conditionally: compute a global Boolean in `OnOpenPage` and bind `Visible` to it. An easy-looking remediation of AL0378 is to delete the trigger and bind `Visible` or `Enabled` directly to a local procedure such as `CanSeeMyCustomers()`. That still builds, but with two future errors: AL0573 for the procedure call in a client expression and AL0569 for the procedure itself. Neither message names the alternative.
|
||||
|
||||
When the condition is "the user has permission to this object", the declarative alternative is the `AccessByPermission` property on the part, action, or field. It takes `TableData <table> = R|I|M|D` (any combination; having any one of the listed permissions is enough) or `X` for `Table`, `Page`, `Report`, `Codeunit`, `XmlPort`, or `Query`. Its applies-to list covers page fields, parts, system parts, chart parts, actions, and whole pages and reports; it does not include groups or cue groups. The element is removed for users without the permission, not disabled. That per-user removal requires the UI Elements Removal setting `LicenseFileAndUserPermissions`; under `LicenseFile` removal follows the license, not the user's permission sets.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Set `AccessByPermission` on the Role Center part, action, or field that should appear only for users with the permission, naming the table or object that the part actually depends on. The base application does this on its own Role Centers, for example `AccessByPermission = TableData "Activities Cue" = I` on the activities part of the Business Manager Role Center and `TableData "Report Inbox" = IMD` on the Report Inbox part (`Control96`) of the same Role Center and of the Accountant Role Center.
|
||||
|
||||
Two limits apply. The property takes effect only when the server's UI Elements Removal setting is `LicenseFile` or `LicenseFileAndUserPermissions`. It is UI removal, not a security boundary: the part's source data must still be protected by real permissions. When the condition is not a permission check, such as a setup value or a feature flag, `AccessByPermission` is the wrong tool. Put the condition inside the part page, which is a normal `CardPart` or `ListPart` that can have triggers. The part cannot remove itself from the Role Center, but it can hide or empty its own controls. See sample: [`rolecenter-permission-gating-must-use-accessbypermission.good.al`](rolecenter-permission-gating-must-use-accessbypermission.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A `RoleCenter` page, or a pageextension whose target is a Role Center, binds `Visible` or `Enabled` on a part, action, or field to a procedure call whose body checks `ReadPermission`, `WritePermission`, or a similar permission test, and declares that procedure. The compiler reports AL0573 and AL0569 as warnings, and both will become errors. Reviewer signal: a pageextension declares a procedure and AL0569 appears in its build output. The extended page's name is not reliable evidence of its type, so confirm the target is a Role Center from its `PageType` or from that diagnostic. Do not flag setup- or feature-based gating implemented inside the part page itself; that is the correct location for non-permission conditions. See sample: [`rolecenter-permission-gating-must-use-accessbypermission.bad.al`](rolecenter-permission-gating-must-use-accessbypermission.bad.al).
|
||||
|
||||
## References
|
||||
|
||||
- [AccessByPermission property](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/properties/devenv-accessbypermission-property): applies-to list, permission values, any-one-of semantics, and the UI Elements Removal requirement ([Hide UI elements](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/administration/hide-ui-elements)).
|
||||
- [Compiler error AL0378](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al378), [compiler warning AL0569](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al569), and [compiler warning AL0573](https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/diagnostics/diagnostic-al573). AL compiler 30.0 reports all three on a pageextension of a Role Center, as it does on the Role Center page itself.
|
||||
- Base application usage: [BusinessManagerRoleCenter.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/RoleCenters/BusinessManagerRoleCenter.Page.al) (`Control96`, lines 124-127) and [AccountantRoleCenter.Page.al](https://github.com/microsoft/BCApps/blob/main/src/Layers/W1/BaseApp/Finance/RoleCenters/AccountantRoleCenter.Page.al). No Role Center page in BCApps declares a trigger or procedure.
|
||||
|
|
@ -0,0 +1,41 @@
|
|||
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;
|
||||
|
||||
trigger OnUpgradePerDatabase()
|
||||
var
|
||||
Company: Record Company;
|
||||
SalesOrderExt: Record "Sales Order Ext";
|
||||
begin
|
||||
// Reaches into every company from one session. Each company also has its own
|
||||
// upgrade session, triggers and subscribers run in the calling context, and a
|
||||
// data error in any company aborts the whole upgrade.
|
||||
if Company.FindSet() then
|
||||
repeat
|
||||
SalesOrderExt.ChangeCompany(Company.Name);
|
||||
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;
|
||||
until Company.Next() = 0;
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,96 @@
|
|||
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;
|
||||
|
||||
// The platform runs this trigger once per company, in that company's own session.
|
||||
trigger OnUpgradePerCompany()
|
||||
begin
|
||||
UpgradeShippingAgentCodes();
|
||||
end;
|
||||
|
||||
local procedure UpgradeShippingAgentCodes()
|
||||
var
|
||||
SalesOrderExt: Record "Sales Order Ext";
|
||||
UpgradeTag: Codeunit "Upgrade Tag";
|
||||
begin
|
||||
if UpgradeTag.HasUpgradeTag(ShippingAgentUpgradeTag()) then
|
||||
exit;
|
||||
|
||||
// Only the current company's rows; triggers, events, and the tag all apply here.
|
||||
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;
|
||||
|
||||
UpgradeTag.SetUpgradeTag(ShippingAgentUpgradeTag());
|
||||
end;
|
||||
|
||||
local procedure ShippingAgentUpgradeTag(): Code[250]
|
||||
begin
|
||||
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;
|
||||
}
|
||||
36
microsoft/knowledge/upgrade/no-changecompany-in-upgrade.md
Normal file
36
microsoft/knowledge/upgrade/no-changecompany-in-upgrade.md
Normal file
|
|
@ -0,0 +1,36 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: upgrade
|
||||
keywords: [changecompany, cross-company, onupgradepercompany, onupgradeperdatabase, feature-data-update, taskscheduler, multi-company, company-context]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Upgrade code must not use ChangeCompany
|
||||
|
||||
## 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. Cross-company work bypasses the platform's per-company execution boundary, making execution order and migration ownership difficult to reason about and potentially repeating work. A cross-company read cannot assume that the other company's migration has already run, so it can observe pre-upgrade or post-upgrade data. 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, which can cause B's own session to repeat the migration. 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` 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 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` 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).
|
||||
|
||||
## References
|
||||
|
||||
- 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
|
||||
|
|
@ -71,6 +71,22 @@ Sub-skills that fail either check are not invoked and are recorded in `skipped-s
|
|||
|
||||
The worklist is the list of sub-skills judged relevant by the previous step. Every sub-skill in the worklist will be invoked in the Action step.
|
||||
|
||||
Before dispatch, preserve that selection in a private host-owned expected
|
||||
composition artifact using DO's input contract. Start from the resolver's
|
||||
ordered selection; enrich configuration skips with the declared indexed
|
||||
skill's version, and record any input-incompatible exclusions with
|
||||
`reason: "not-applicable"`. Do not derive this artifact from leaf reports or
|
||||
change it merely because execution later runs out of budget. Pass its path as
|
||||
`-ExpectedCompositionPath` for final super-skill validation.
|
||||
Initialize `acceptedResults` to `[]`. After each leaf's acceptance gate, the
|
||||
host saves its exact accepted copy (or host-created failed validation result)
|
||||
in an immutable private file and appends its `id`, `version`, and `reportPath`
|
||||
to that array. The host alone owns these captures; neither workers nor the
|
||||
composing model may write them. Never derive them from composed `sub-results`.
|
||||
Keep the pre-dispatch selection and exclusions unchanged. Final validation
|
||||
requires every nested leaf to match its captured JSON content exactly and
|
||||
every captured result to be included. Property order is immaterial.
|
||||
|
||||
## Action
|
||||
|
||||
### Execution discipline (mandatory)
|
||||
|
|
@ -85,7 +101,7 @@ The Action step consists of **discrete leaf invocations**, not one combined gene
|
|||
- Do not collapse multiple sub-skills into one shared reasoning step. Each sub-skill has a distinct knowledge subset and a distinct evaluation procedure; sharing one rolled-up scan dilutes per-skill attention and causes leaves to silently underreport (this has been observed in production: leaf skills returned empty `findings[]` while their standalone runs against the same diff produced multiple matches).
|
||||
- The agent self-review pass is its own final iteration. Begin it only after every sub-skill in the worklist has completed and its sub-result is recorded.
|
||||
- Sub-skills are independent: re-walking the diff once per sub-skill is correct and expected. The output schema accommodates this — `sub-results` carries one entry per sub-skill, each a complete findings-report, in the frontmatter `sub-skills` order regardless of completion order.
|
||||
- When isolated calls are unavailable and the current model cannot finish every leaf within its budget, return `partial` with completed `sub-results` and name the first unevaluated sub-skill in `outcome-reason`. Never silently mark the remaining leaves clean.
|
||||
- When the execution budget prevents invoking every selected leaf, wait for started invocations to finish and preserve their accepted `sub-results`. Return `partial` if any returned report is non-`failed`, otherwise `failed`, and name the unfinished leaf IDs in `outcome-reason`. Do not run the self-review on incomplete composition, invent sub-results, or record budget exhaustion as a configured/input-incompatible skip. Never silently mark the remaining leaves clean.
|
||||
|
||||
### Roll up sub-skill findings
|
||||
|
||||
|
|
@ -138,7 +154,9 @@ Calculate `summary.counts` from the final top-level `findings[]`, after failed s
|
|||
Derive `outcome` using the DO rollup rules. `outcome-reason` is populated for `partial` and `failed` and SHOULD summarize per-sub-skill state, for example: *"al-security-review failed (tool timeout); al-performance-review completed."*
|
||||
|
||||
Before emitting the rollup, apply DO's consumer acceptance gate to every nested
|
||||
and top-level finding. A leaf's nested report is its accepted exact return or
|
||||
and top-level finding, and validate the final report with
|
||||
`-SkillKind super -ExpectedCompositionPath <host-owned-json>` against the
|
||||
selection preserved before dispatch. A leaf's nested report is its accepted exact return or
|
||||
its accepted normalized candidate copy; its exact Task return remains the
|
||||
separate immutable raw audit payload. Treat an invalid sub-result as failed and
|
||||
exclude all of its findings from the top-level rollup. Never reconstruct it
|
||||
|
|
|
|||
|
|
@ -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, `TableRelation` field-length design, 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, code that reads or writes sales/purchase/service line price and amount fields, `TransferFields`-based posting-cascade field mirroring, `TableRelation` field-length design, 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.
|
||||
|
||||
## 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`, `ValidateTableRelation`, `TestTableRelation`, `Text[`, `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`, `ValidateTableRelation`, `TestTableRelation`, `Text[`, `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`, `Prices Including VAT`, `Unit Price`, `Direct Unit Cost`, `Line Amount`, `Prepmt. Line Amount`, `Amount Including VAT`, `CalculateOutstandingAmountExclTax`, `Barcode Font Provider`, `Barcode Font Provider 2D`, `EncodeFont`, `ValidateInput`).
|
||||
|
||||
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.
|
||||
|
||||
|
|
@ -67,6 +67,7 @@ The following targeted checks cover every current `data-modeling` article. Treat
|
|||
- An `enumextension` extends `"Price Calculation Handler"` and implements the `Price Calculation` interface, without a matching `OnFindSupportedSetup` subscriber inserting a `Price Calculation Setup` record naming that implementation as the `Implementation` for a `Method`/`Type`/`Asset Type` — `activate-new-price-calculation-handler-via-onfindsupportedsetup`. `Default := true` is only required on that row when it is meant as the fallback for its `Method`/`Type`/`Asset Type` combination; a row meant to be selected only through an explicit, specific `"Dtld. Price Calculation Setup"` row does not need it, so do not flag a missing `Default := true` by itself — flag the missing setup row/subscriber entirely.
|
||||
- An `enumextension` extends `"Price Source Type"` with a new value intended for a sales, purchase, or job price list, without extending the matching document subset enum (`"Sales Price Source Type"`, `"Purchase Price Source Type"`, `"Job Price Source Type"`) with a value at the same numeric ID — `extend-price-source-type-must-sync-document-subset-enum`.
|
||||
- A codeunit subscribes to `"Sales Line - Price"`'s `OnAfterAddSources` to register a custom field as a price source via `PriceSourceList.Add`, but that field has no `OnValidate` (or matching `OnAfterValidate`) that triggers recalculation — either `SalesLine.UpdateUnitPrice(<field no.>)`, or the explicit `SalesLine.PlanPriceCalcByField(<field no.>)` followed by `SalesLine.UpdateUnitPriceByField(<same field no.>)`. A bare `UpdateUnitPriceByField` without a preceding `PlanPriceCalcByField` for the same field number does not count as recalculation (it exits without recalculating) — `new-price-source-must-add-candidate-and-trigger-recalculation`.
|
||||
- Code reads `Unit Price`, `Direct Unit Cost`, `Line Amount`, `Line Discount Amount`, `Inv. Discount Amount`, `Prepmt. Line Amount`, `Prepmt. Amt. Inv.`, `Prepmt Amt to Deduct`, `Prepmt Amt Deducted`, or the result of `CalculateOutstandingAmountExclTax` of a `Sales Line`/`Purchase Line`/`Service Line` as a known net or gross value (a net/gross total, a VAT computation, a comparison with `Amount` or `Item."Unit Price"`/`"Last Direct Cost"`, an export), or writes a source price of known basis into `Unit Price`/`Direct Unit Cost`, without reading the document header's `Prices Including VAT` — `document-line-prices-follow-prices-including-vat`. Reads of fixed-basis fields (`Amount`, `Amount Including VAT`, `Prepayment Amount`, `Prepmt. Amt. Incl. VAT`), combinations of header-dependent fields with each other, prices returned by the standard price calculation, and copies between lines of the same document are not this anti-pattern.
|
||||
- A report hand-constructs a barcode string only where a concrete, independently provable defect is visible: the source value can contain characters outside the symbology's character set and is never validated, a checksum the symbology/setup requires is never applied, or there is concrete evidence of an incompatible font binding. Do not flag manual start/stop delimiters by themselves — `*value*` is a documented, valid Code 39 form for IDAutomation fonts (IDAutomation also accepts parentheses), so delimiter choice alone is never a finding. Also flag module use that does not match the interface: a 1D `"Barcode Font Provider"` path must call both `ValidateInput` and `EncodeFont`; a 2D `"Barcode Font Provider 2D"` path calls `EncodeFont` only (the 2D interface has no `ValidateInput`, so its absence there is not a finding). Separately, flag an otherwise correctly encoded barcode whose report layout names an evaluation/demo font instead of the purchased production font name — `report-barcodes-must-use-barcode-module-and-production-font-name`.
|
||||
- A new field is typed `Code`/`Text` and its `OnValidate` calls `DimensionManagement`/`DimMgt`, or a table adds Shortcut Dimension fields, a `Dimension Set ID` field, or `AddDimSource`/`GetDefaultDimID` — `dimension-management-wiring`. A master table calling `SaveDefaultDim` and a document/journal table computing its own `Dimension Set ID` are two different valid shapes; do not flag a master table for lacking a `Dimension Set ID` field or a document for lacking `SaveDefaultDim`.
|
||||
- A journal-based posting codeunit is added or changed and validation, Journal-table access, ledger writes, and user-interaction (`Confirm`/dialogs) all occur in one procedure or one codeunit, rather than split across `Check Line`/`Post Line`/`Post Batch`-shaped companions — `check-post-line-batch-pattern`. A document posting routine calling `Post Line` directly without a `Post Batch` companion is not this anti-pattern.
|
||||
|
|
@ -101,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, `TableRelation` field length, 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, sales/purchase/service line price or amount read/write, posting-cascade `TransferFields` mirroring, `TableRelation` field length, barcode/report-font-provider usage, dimension wiring, posting-routine structure, or Item-Ledger-Entry-document-number surface.
|
||||
- `partial` — a budget was hit before the worklist was exhausted.
|
||||
- `failed` — an unrecoverable error occurred.
|
||||
|
||||
|
|
|
|||
|
|
@ -42,7 +42,7 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
- **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`, and to JavaScript/CSS/HTML that implements a control add-in's rendering or Business Central communication. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files.
|
||||
- A new or changed page's name/suffix, primary-key handling, `CardPageID`, `SubPageLink`, `AutoSplitKey`, or `UsageCategory` doesn't match the conventions of its own declared `PageType` — `page-design-must-match-bc-page-type-conventions.md`. A Card page over a composite-key table that supplements a master record, or a supporting/subpage/dialog page intended only to be reached through another workflow and correctly omitting `UsageCategory`, is not this anti-pattern on its own; check whether the page is actually mixing conventions or is meant as a searchable entry point before flagging.
|
||||
- For each relevant knowledge file, compute overlap against changed page declarations and control add-in files, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, `ShowCaption`, `InstructionalText`, `GridLayout`, `Style`, `StyleExpr`, promoted action definitions, field importance, page background tasks, DOM creation, ARIA attributes, keyboard/focus handlers, packaged-resource AJAX, and calls from JavaScript into AL.
|
||||
- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`).
|
||||
- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `fieldgroups`, `DropDown`, `UpdatePropagation`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`, `PageType = RoleCenter`, `Enabled`, `Visible`, `Editable`, `in [`, `AccessByPermission`, `ReadPermission`, `WritePermission`).
|
||||
|
||||
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 page element. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone.
|
||||
|
||||
|
|
@ -50,6 +50,8 @@ Apply these high-signal mappings before fuzzy topic ranking:
|
|||
|
||||
- A tableextension adds a field to `DropDown` while the corresponding lookup-page control remains `Visible = false` — `dropdown-fieldgroup-respects-lookup-page-visibility`.
|
||||
- An editable page part affects a total, FlowField, or FactBox on the parent but does not set `UpdatePropagation = Both` — `updatepropagation-both-refreshes-main-page`.
|
||||
- A page or pageextension `Enabled`, `Visible`, `Editable`, or `StyleExpr` value contains an `in [...]` list (compiler: "InListExpression is not valid for client expressions", AL0573 or AL0322), or replaces one with a procedure call — `page-client-expression-must-not-use-in-list`. Plain `=`/`<>` comparisons joined with `and`/`or`, and `in [...]` inside a trigger or procedure body, are valid; do not flag them.
|
||||
- A `RoleCenter` page, or a pageextension whose target is a Role Center, gates a part, action, or field by binding `Visible` or `Enabled` to a procedure that tests a permission, or declares a procedure (AL0569 "A page of type Role Center cannot have procedures", AL0573) — `rolecenter-permission-gating-must-use-accessbypermission`. The target page name is not reliable evidence of its type; confirm it is a Role Center from its `PageType` or the AL0569 diagnostic. Setup- or feature-flag gating inside the part page is not this pattern.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.
|
||||
|
||||
|
|
|
|||
|
|
@ -16,7 +16,7 @@ application-area: [all]
|
|||
|
||||
Reviews AL source changes against the `upgrade` 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`. Upgrade findings are narrow by design — they apply when the review scope contains upgrade codeunits, install codeunits, table schema, enums, or objects under migration namespaces. The skill returns `not-applicable` when none of those apply.
|
||||
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Upgrade findings are narrow by design — they apply when the review scope contains upgrade codeunits, install codeunits, `Feature Data Update` implementations, table schema, enums, or objects under migration namespaces. The skill returns `not-applicable` when none of those apply.
|
||||
|
||||
## Source
|
||||
|
||||
|
|
@ -37,13 +37,14 @@ Discard files that are not applicable. Retain conditionally applicable files (an
|
|||
|
||||
Narrow the relevant files to the subset that applies to the changes under review. For each relevant file, compute overlap against:
|
||||
|
||||
- The changed AL object names and types — especially codeunits with `Subtype = Upgrade` or `Subtype = Install`, tables and tableextensions adding or changing fields, enums and enumextensions, and objects under `Hybrid*`/`Migration`/`Upgrade` namespaces.
|
||||
- The changed triggers and procedures, weighted toward `OnCheckPreconditionsPerCompany`/`PerDatabase`, `OnUpgradePerCompany`/`PerDatabase`, `OnValidateUpgradePerCompany`/`PerDatabase`, `OnInstallAppPerCompany`/`PerDatabase`, the `OnGetPerCompanyUpgradeTags`/`OnGetPerDatabaseUpgradeTags` subscribers, and helper procedures transitively reachable from those entry points.
|
||||
- Tokens extracted from the diff that relate to upgrade concerns (`Subtype = Upgrade`, `Subtype = Install`, `Upgrade Tag`, `HasUpgradeTag`, `SetUpgradeTag`, `OnCheckPreconditions`, `OnUpgrade`, `OnValidateUpgrade`, `OnInstallApp`, `DataTransfer`, `CopyFields`, `Insert`, `Modify`, `Delete`, `Rename`, `InitValue`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `ModuleInfo`, `AppVersion`, `DataVersion`, `NavApp.GetCurrentModuleInfo`, `ExecutionContext`, `PrimaryKey`, `key(`, `field(`, `value(`, `enum`, `enumextension`, `HybridSL`, `HybridGP`, `HybridBC`, `HybridBaseDeployment`).
|
||||
- The changed AL object names and types — especially codeunits with `Subtype = Upgrade` or `Subtype = Install`, codeunits implementing `Feature Data Update`, tables and tableextensions adding or changing fields, enums and enumextensions, and objects under `Hybrid*`/`Migration`/`Upgrade` namespaces.
|
||||
- The changed triggers and procedures, weighted toward `OnCheckPreconditionsPerCompany`/`PerDatabase`, `OnUpgradePerCompany`/`PerDatabase`, `OnValidateUpgradePerCompany`/`PerDatabase`, `OnInstallAppPerCompany`/`PerDatabase`, the `OnGetPerCompanyUpgradeTags`/`OnGetPerDatabaseUpgradeTags` subscribers, the `UpdateData`/`AfterUpdate` methods of `Feature Data Update` implementations, and helper procedures transitively reachable from those entry points.
|
||||
- Tokens extracted from the diff that relate to upgrade concerns (`Subtype = Upgrade`, `Subtype = Install`, `Upgrade Tag`, `HasUpgradeTag`, `SetUpgradeTag`, `OnCheckPreconditions`, `OnUpgrade`, `OnValidateUpgrade`, `OnInstallApp`, `DataTransfer`, `CopyFields`, `Insert`, `Modify`, `Delete`, `Rename`, `InitValue`, `ObsoleteState`, `ObsoleteReason`, `ObsoleteTag`, `ModuleInfo`, `AppVersion`, `DataVersion`, `NavApp.GetCurrentModuleInfo`, `ExecutionContext`, `ChangeCompany`, `Feature Data Update`, `UpdateData`, `PrimaryKey`, `key(`, `field(`, `value(`, `enum`, `enumextension`, `HybridSL`, `HybridGP`, `HybridBC`, `HybridBaseDeployment`).
|
||||
- For each `OnCheckPreconditions...` and `OnValidateUpgrade...` trigger, build the best available call graph from surrounding unchanged source as well as changed hunks, tracing resolved calls through reachable local or internal helpers. Worklist the check-only rule when a database write occurs either directly in the trigger or in any helper procedure reachable from it. Writes include `Insert`, `Modify`, `ModifyAll`, `Delete`, `DeleteAll`, `Rename`, and `DataTransfer`. Also perform the reverse check when a PR changes a writing helper body: worklist the rule when that helper is invoked directly or transitively by an unchanged check or validation trigger.
|
||||
- Treat a direct write or a fully resolved call chain as high-confidence evidence. When cross-object dispatch, unavailable declarations, or an incomplete call graph prevents proving the complete chain, cap confidence at `medium`, name the unresolved edge in the finding, and do not claim a violation without a resolved path from a check or validation trigger to a write.
|
||||
- Worklist the install-versus-upgrade rule when migration helpers are reachable only from an install codeunit.
|
||||
- Worklist `install-and-upgrade-codeunits-have-no-order.md` when a change adds multiple install or upgrade codeunits whose same-phase triggers share state or depend on one another.
|
||||
- Worklist `no-changecompany-in-upgrade.md` when `ChangeCompany` with a company-name argument appears in an upgrade codeunit, in a helper reachable from its upgrade triggers, or in the `UpdateData` or `AfterUpdate` method of a `Feature Data Update` implementation or a helper reachable from them. Trace that reachability with the same call-graph and confidence rules as the check-only rule. Do not worklist it for `ChangeCompany` reachable only from `IsDataUpdateRequired` or `ReviewData`; that read-only preflight is permitted.
|
||||
- Worklist `appversion-meaning-depends-on-execution-context.md` when install or upgrade code branches on `ModuleInfo.AppVersion()` or confuses it with `DataVersion()`.
|
||||
- An upgrade tag's existence check (`HasUpgradeTag`) is nested inside another tag's guarded body, or one tagged procedure performs two or more functionally unrelated migrations (different tables, fields, or concerns) under a single tag, or one procedure mixes the gated logic for more than one distinct upgrade tag — `upgrade-tag-logic-must-not-nest-deeply.md`. Do not flag record loops or business-data safety guards (corruption checks, redundant-write checks, or other conditions) that serve the single migration the tag represents, however many `if` levels they take — that is the compliant shape the article explicitly permits.
|
||||
|
||||
|
|
@ -77,7 +78,7 @@ Outcome selection:
|
|||
|
||||
- `completed` — the skill evaluated every worklist item.
|
||||
- `no-knowledge` — no applicable upgrade knowledge survived filtering.
|
||||
- `not-applicable` — the diff touches no upgrade, install, schema, or enum surface.
|
||||
- `not-applicable` — the diff touches no upgrade, install, feature data update, schema, or enum surface.
|
||||
- `partial` — a budget was hit before the worklist was exhausted.
|
||||
- `failed` — an unrecoverable error occurred.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue