mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 06:36:55 +01:00
Fix four merge-critical issues from Jesper's 2026-09-22 review
- pages-must-not-contain-business-logic.good.al/.bad.al: the "good" codeunit still directly assigned real Sales Line."Line Amount" and called Modify(), bypassing the field's normal Validate cascade (discount, VAT, related-amount maintenance) - persisting inconsistent document lines regardless of which object the code lived in. Replaced the real Sales Line example with a self-contained "Sample Order Line" table and switched the codeunit to Validate()/Modify(true), so the fixture demonstrates the page-vs-codeunit separation without teaching unsafe direct field writes to a real BC document table. - bcpt-scenarios-must-be-app-specific.good.al: Customer.FindFirst() assumed a pre-existing customer (fails against an empty environment), and a session-local NextNo counter for the header key collides across concurrent BCPT sessions and repeated runs. Creates its own customer when none exists, and generates keys from CreateGuid() instead of an in-memory counter. - upgrade-tag-logic-must-not-nest-deeply: the rule conflated two different things - nesting one tag's existence check inside another (the real anti-pattern Microsoft's guidance warns against) with having business-data safety conditions inside a single tagged migration's own loop body (which Microsoft's own worked example does, and its own design guidance explicitly requires: "Implement extra safety checks to avoid data corruption, even though you're using upgrade tags"). Rewrote the Description/Best Practice/Anti Pattern to scope the rule to actual tag nesting and migrations blended under one tag, and rewrote both fixtures: good.al now shows two safety conditions correctly nested inside one migration's own loop plus a second, genuinely separate migration as its own flat tagged procedure; bad.al now shows the real anti-pattern, one tag's check nested inside another's guarded body. - table-design-must-match-bc-table-type-conventions: the rule and its worklist cue fired on any new table with a keys block, forcing buffers, queues, logs, mapping tables, and staging tables into the nearest-looking one of nine business-record archetypes. Added an explicit scope note that these nine types aren't an exhaustive table catalogue, and narrowed the al-data-modeling-review.md cue to require positive evidence (a type-specific naming suffix, key shape, or usage) before worklisting, instead of a bare keys/primary-key declaration.
This commit is contained in:
Michael Dieringer
parent
67962727f6
commit
4a95985c8c
8 changed files with 154 additions and 37 deletions
|
|
@ -72,6 +72,19 @@ table with a real business-field key and no page), say so explicitly
|
|||
rather than forcing a classification; settling it requires checking actual
|
||||
row cardinality or call sites, not just the object definition.
|
||||
|
||||
These nine types cover Business Central's *business-record* tables — they
|
||||
are not an exhaustive catalogue of every legitimate table shape. A
|
||||
temporary/buffer table, a work queue, a log or telemetry table, a
|
||||
cross-reference/mapping table with no business meaning of its own, or a
|
||||
staging/working table used only inside one process is not required to fit
|
||||
any of the nine, and forcing one into the nearest-looking type (usually
|
||||
Ledger, because it has an `Integer` key, or Subsidiary, because it has a
|
||||
composite key) produces a harmful redesign recommendation for a table that
|
||||
was never meant to carry that type's guarantees. Apply this rule only when
|
||||
the table's name, fields, or usage genuinely establish it as one of the
|
||||
nine business-record types; when nothing points that way, this rule simply
|
||||
does not apply — that is not the same as an unresolved classification.
|
||||
|
||||
See sample: [`table-design-must-match-bc-table-type-conventions.good.al`](table-design-must-match-bc-table-type-conventions.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
|
|
|||
|
|
@ -1,7 +1,36 @@
|
|||
page 50100 "Sales Line Card"
|
||||
table 50101 "Sample Order Line"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Document No."; Code[20]) { }
|
||||
field(2; "Line No."; Integer) { }
|
||||
field(10; Quantity; Decimal) { }
|
||||
field(11; "Unit Price"; Decimal) { }
|
||||
field(12; "Line Amount"; Decimal) { }
|
||||
}
|
||||
keys
|
||||
{
|
||||
key(PK; "Document No.", "Line No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
||||
page 50100 "Sample Order Line Card"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = "Sales Line";
|
||||
SourceTable = "Sample Order Line";
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(General)
|
||||
{
|
||||
field(quantity; Rec.Quantity) { }
|
||||
field(unitPrice; Rec."Unit Price") { }
|
||||
field(lineAmount; Rec."Line Amount") { }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
actions
|
||||
{
|
||||
|
|
|
|||
|
|
@ -1,16 +1,45 @@
|
|||
codeunit 50100 "Sales Line Management"
|
||||
table 50101 "Sample Order Line"
|
||||
{
|
||||
procedure RecalculateLine(var SalesLine: Record "Sales Line")
|
||||
fields
|
||||
{
|
||||
field(1; "Document No."; Code[20]) { }
|
||||
field(2; "Line No."; Integer) { }
|
||||
field(10; Quantity; Decimal) { }
|
||||
field(11; "Unit Price"; Decimal) { }
|
||||
field(12; "Line Amount"; Decimal) { }
|
||||
}
|
||||
keys
|
||||
{
|
||||
key(PK; "Document No.", "Line No.") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
||||
codeunit 50100 "Sample Order Line Management"
|
||||
{
|
||||
procedure RecalculateLine(var OrderLine: Record "Sample Order Line")
|
||||
begin
|
||||
SalesLine."Line Amount" := SalesLine.Quantity * SalesLine."Unit Price";
|
||||
SalesLine.Modify();
|
||||
OrderLine.Validate("Line Amount", OrderLine.Quantity * OrderLine."Unit Price");
|
||||
OrderLine.Modify(true);
|
||||
end;
|
||||
}
|
||||
|
||||
page 50100 "Sales Line Card"
|
||||
page 50100 "Sample Order Line Card"
|
||||
{
|
||||
PageType = Card;
|
||||
SourceTable = "Sales Line";
|
||||
SourceTable = "Sample Order Line";
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(General)
|
||||
{
|
||||
field(quantity; Rec.Quantity) { }
|
||||
field(unitPrice; Rec."Unit Price") { }
|
||||
field(lineAmount; Rec."Line Amount") { }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
actions
|
||||
{
|
||||
|
|
@ -20,12 +49,12 @@ page 50100 "Sales Line Card"
|
|||
{
|
||||
trigger OnAction()
|
||||
begin
|
||||
SalesLineMgt.RecalculateLine(Rec);
|
||||
OrderLineMgt.RecalculateLine(Rec);
|
||||
end;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
var
|
||||
SalesLineMgt: Codeunit "Sales Line Management";
|
||||
OrderLineMgt: Codeunit "Sample Order Line Management";
|
||||
}
|
||||
|
|
|
|||
|
|
@ -14,26 +14,40 @@ codeunit 50100 "BCPT Create Service Request" implements "BCPT Test Param. Provid
|
|||
var
|
||||
GlobalBCPTTestContext: Codeunit "BCPT Test Context";
|
||||
CustomerNo: Code[20];
|
||||
NextNo: Integer;
|
||||
IsInitialized: Boolean;
|
||||
|
||||
local procedure InitTest()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
begin
|
||||
Customer.FindFirst();
|
||||
// Do not assume a customer already exists: a BCPT run may target an
|
||||
// otherwise-empty environment. Create one if none is found instead
|
||||
// of failing on FindFirst().
|
||||
if not Customer.FindFirst() then begin
|
||||
Customer.Init();
|
||||
Customer."No." := GenerateUniqueCode(MaxStrLen(Customer."No."));
|
||||
Customer.Insert(true);
|
||||
end;
|
||||
CustomerNo := Customer."No.";
|
||||
end;
|
||||
|
||||
local procedure GenerateUniqueCode(Length: Integer): Code[20]
|
||||
begin
|
||||
// A GUID-derived code, not a session-local counter: it stays unique
|
||||
// across concurrent BCPT sessions and repeated runs against the
|
||||
// same environment, which an in-memory counter reset per session
|
||||
// cannot guarantee.
|
||||
exit(CopyStr(DelChr(Format(CreateGuid()), '=', '{}-'), 1, Length));
|
||||
end;
|
||||
|
||||
local procedure CreateServiceRequest(var BCPTTestContext: Codeunit "BCPT Test Context")
|
||||
var
|
||||
ServiceRequestHeader: Record "Service Request Header";
|
||||
ServiceRequestLine: Record "Service Request Line";
|
||||
begin
|
||||
BCPTTestContext.StartScenario('Create Service Request Header');
|
||||
NextNo += 1;
|
||||
ServiceRequestHeader.Init();
|
||||
ServiceRequestHeader."No." := CopyStr(Format(NextNo), 1, MaxStrLen(ServiceRequestHeader."No."));
|
||||
ServiceRequestHeader."No." := GenerateUniqueCode(MaxStrLen(ServiceRequestHeader."No."));
|
||||
ServiceRequestHeader.Validate("Customer No.", CustomerNo);
|
||||
ServiceRequestHeader.Insert(true);
|
||||
BCPTTestContext.EndScenario('Create Service Request Header');
|
||||
|
|
|
|||
|
|
@ -1,14 +1,32 @@
|
|||
local procedure UpgradeCustomerDiscountField()
|
||||
local procedure UpgradeCustomerFields()
|
||||
begin
|
||||
if not UpgradeTag.HasUpgradeTag(GetCustomerDiscountFieldTag()) then begin
|
||||
if Customer.FindSet() then begin
|
||||
Customer.SetLoadFields("Discount %", "Customer Posting Group");
|
||||
if Customer.FindSet() then
|
||||
repeat
|
||||
if Customer."Discount %" = 0 then begin
|
||||
if Customer."Customer Posting Group" <> '' then
|
||||
if (Customer."Discount %" = 0) and (Customer."Customer Posting Group" <> '') then begin
|
||||
Customer."Discount %" := 5;
|
||||
Customer.Modify();
|
||||
end;
|
||||
until Customer.Next() = 0;
|
||||
end;
|
||||
UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag());
|
||||
|
||||
// BUG: a second, unrelated migration's tag check nested inside the
|
||||
// first migration's guarded body. Neither tag can be checked,
|
||||
// skipped, or fixed independently of the other - a failure or a
|
||||
// deliberate skip of the discount migration silently takes the
|
||||
// shipping-agent migration down with it, and nothing in the
|
||||
// Upgrade Tags table records that the second step ran on its own.
|
||||
if not UpgradeTag.HasUpgradeTag(GetCustomerShippingAgentFieldTag()) then begin
|
||||
Customer.SetLoadFields("Shipping Agent Code");
|
||||
if Customer.FindSet() then
|
||||
repeat
|
||||
if Customer."Shipping Agent Code" = '' then begin
|
||||
Customer."Shipping Agent Code" := DefaultShippingAgentCode();
|
||||
Customer.Modify();
|
||||
end;
|
||||
until Customer.Next() = 0;
|
||||
UpgradeTag.SetUpgradeTag(GetCustomerShippingAgentFieldTag());
|
||||
end;
|
||||
end;
|
||||
end;
|
||||
|
|
|
|||
|
|
@ -6,23 +6,35 @@ begin
|
|||
Customer.SetLoadFields("Discount %", "Customer Posting Group");
|
||||
if Customer.FindSet() then
|
||||
repeat
|
||||
SetDefaultDiscountIfEligible(Customer);
|
||||
// A business-data safety condition inside this one migration's
|
||||
// loop is not a second migration hiding inside the first -
|
||||
// Microsoft's own upgrade-tag example nests exactly this shape
|
||||
// (a corruption guard, then a redundant-write guard) inside a
|
||||
// single tagged procedure.
|
||||
if (Customer."Discount %" = 0) and (Customer."Customer Posting Group" <> '') then begin
|
||||
Customer."Discount %" := 5;
|
||||
Customer.Modify();
|
||||
end;
|
||||
until Customer.Next() = 0;
|
||||
|
||||
UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag());
|
||||
end;
|
||||
|
||||
local procedure SetDefaultDiscountIfEligible(var Customer: Record Customer)
|
||||
// A second, genuinely unrelated migration gets its own tag and its own
|
||||
// top-level procedure - not nested inside the first one's guarded body.
|
||||
local procedure UpgradeCustomerShippingAgentField()
|
||||
begin
|
||||
// Both safety conditions from the original logic are preserved, just
|
||||
// flattened into early exits instead of nested ifs: don't overwrite an
|
||||
// already-set discount, and don't touch a customer with no posting
|
||||
// group configured yet.
|
||||
if Customer."Discount %" <> 0 then
|
||||
exit;
|
||||
if Customer."Customer Posting Group" = '' then
|
||||
if UpgradeTag.HasUpgradeTag(GetCustomerShippingAgentFieldTag()) then
|
||||
exit;
|
||||
|
||||
Customer."Discount %" := 5;
|
||||
Customer.SetLoadFields("Shipping Agent Code");
|
||||
if Customer.FindSet() then
|
||||
repeat
|
||||
if Customer."Shipping Agent Code" = '' then begin
|
||||
Customer."Shipping Agent Code" := DefaultShippingAgentCode();
|
||||
Customer.Modify();
|
||||
end;
|
||||
until Customer.Next() = 0;
|
||||
|
||||
UpgradeTag.SetUpgradeTag(GetCustomerShippingAgentFieldTag());
|
||||
end;
|
||||
|
|
|
|||
|
|
@ -7,23 +7,25 @@ countries: [w1]
|
|||
application-area: [all]
|
||||
---
|
||||
|
||||
# Keep upgrade tag conditional logic to at most two levels of nesting
|
||||
# Never nest upgrade tag checks or blend two migrations under one tag
|
||||
|
||||
## Description
|
||||
|
||||
The conditional logic that gates upgrade code behind an upgrade tag should be at most two levels deep: check the tag, exit if already applied, otherwise run the upgrade step. Deeper nesting — tag checks inside tag checks, or a tag check combined with multi-branch business-data conditions — signals that the upgrade step is trying to do more than one thing, or that it is reconstructing decision logic that belongs in the tag structure itself (one tag per distinct upgrade step), not in nested `if` statements inside a single step.
|
||||
Upgrade tag *checks* should stay flat: never nest one tag's existence check inside another tag's guarded body, and never let one tagged procedure quietly perform a second, functionally distinct migration — that turns two upgrade steps into one that can't be tracked, skipped, or fixed independently, which is exactly what separate tags exist to prevent. That is the specific nesting Microsoft's own guidance warns against ("Keep tags simple by limiting nesting tags to two levels").
|
||||
|
||||
Upgrade code runs unattended, once, against production data with no chance to interactively debug a wrong branch. The cost of a nesting-driven mistake here is much higher than in ordinary application code, which is why the ceiling is lower than general AL style would otherwise allow.
|
||||
That is not a limit on how much conditional logic a single migration's own loop body may contain. Microsoft's own worked example for upgrade tags nests a record loop with two business-data safety conditions — a corruption guard, then a redundant-write guard — inside one `if UpgradeTagMgt.HasUpgradeTag(...) then exit;`-guarded procedure, and its own design guidance separately *requires* this: "Implement extra safety checks to avoid data corruption, even though you're using upgrade tags." A business-data guard that protects the single migration a tag represents is not a second migration hiding inside the first, however many `if` levels it takes.
|
||||
|
||||
Upgrade code runs unattended, once, against production data with no chance to interactively debug a wrong branch — which is why mixing two migrations under one tag, or losing track of which tag guards which step, is a genuinely higher-cost mistake here than the equivalent would be in ordinary application code.
|
||||
|
||||
## Best Practice
|
||||
|
||||
One tag check, one exit, one upgrade action — two levels deep at most.
|
||||
One tag, one migration: exit early if the tag is already set, then run the one upgrade step that tag represents — including as many business-data safety conditions as that single step's own correctness requires, nested however deep the logic actually needs. Reach for a second, separately tagged migration only when the nested logic is doing genuinely unrelated work (a different table, a different field, a different concern) that could legitimately be skipped, retried, or fixed on its own.
|
||||
|
||||
See sample: [`upgrade-tag-logic-must-not-nest-deeply.good.al`](upgrade-tag-logic-must-not-nest-deeply.good.al).
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Nesting the tag check, a record loop, and a multi-branch business condition inside one procedure. Split the buried business condition into its own, separately tagged upgrade step instead.
|
||||
Checking one upgrade tag inside the guarded body of another, or writing two functionally unrelated migrations — different tables, different concerns — under a single tag so neither can be tracked, skipped, or fixed independently of the other. A record loop with business-data safety conditions inside one tagged migration's own body is not this anti-pattern, even several `if` levels deep, as long as every condition serves that one migration.
|
||||
|
||||
See sample: [`upgrade-tag-logic-must-not-nest-deeply.bad.al`](upgrade-tag-logic-must-not-nest-deeply.bad.al).
|
||||
|
||||
|
|
|
|||
|
|
@ -48,7 +48,7 @@ The following targeted checks cover every current `data-modeling` article. Treat
|
|||
- A `* Setup` table or its page changes singleton structure, uses a nonblank or generated key, permits insert/delete, uses a List page, or does not ensure the blank-keyed row exists — `setup-table-is-a-singleton`.
|
||||
- A new field is typed `Media`, `MediaSet`, or `BLOB` and the field's caption/name suggests a picture or image — `pictures-must-use-media-not-blob`.
|
||||
- Code outside a test codeunit or a demo-data generator calls `WorkDate(NewDate)` (the assignment form, not a bare `WorkDate()` read) as part of logic whose purpose is unrelated to the work date itself — `code-must-not-change-workdate`. A test deliberately setting a date context, or a demo-data routine that saves, sets, and restores the work date to backdate the data it creates, is not this anti-pattern.
|
||||
- A new or extended table declares its `keys` block, primary-key field list, or naming suffix (`Ledger Entry`, `Journal Line`, `Header`/`Line`, `Setup`) — `table-design-must-match-bc-table-type-conventions`.
|
||||
- A new or extended table's name, fields, or usage positively establish it as one of Business Central's nine business-record types — a name ending `Ledger Entry`/`Register`/`Journal Line`/`Header`/`Line`/`Setup`, an auto-generated `Entry No.`/`No.` key posted from elsewhere, a `Template Name`+`Batch Name`+`Line No.` key, or a singleton `Primary Key` field — `table-design-must-match-bc-table-type-conventions`. Do not worklist it from a bare `keys` block or primary-key declaration alone: a temporary/buffer table, a work queue, a log, a cross-reference/mapping table, or a process-local staging table is not one of the nine types and is out of this rule's scope entirely, not an unresolved case.
|
||||
- A custom master table changes its primary key, `No.`/`No. Series` fields, or `OnInsert` without assigning a blank `No.` from setup through a number series — `master-table-no-from-number-series-in-oninsert`.
|
||||
- BC v22 or later code introduces or retains `NoSeriesManagement`, `InitSeries`, `SelectSeries`, or `SetSeries`, or number assignment/manual-entry checks do not use codeunit `"No. Series"` methods such as `GetNextNo`, `IsManual`, or `TestManual` — `use-no-series-codeunit-not-noseriesmanagement`.
|
||||
- A master gains or changes `Blocked`, or a document line, journal line, reference-field `OnValidate`, or posting routine uses that master without `TestField(Blocked, false)` at the point of use; also cue when the check is placed only in the master's own triggers — `check-blocked-in-referencing-code-not-in-master`.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue