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
38a4fd967a
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
|
||||
Loading…
Add table
Add a link
Reference in a new issue