mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
Address second round of Jesper Schulz-Wedde's review on PR #156
- dimension-management-wiring.md/.good.al: split into the two distinct
models the article was conflating - master data (Default Dimension
records via ValidateDimValueCode/SaveDefaultDim) vs. transactional/
document data (a single Dimension Set ID assembled via AddDimSource +
GetDefaultDimID, verified against BCApps' ExchRateAdjmtProcess.Codeunit.al).
Added a compiling document-table example alongside the existing master
table one.
- Deleted api-page-flowfields-must-be-calcfields (.md/.good.al/.bad.al):
Microsoft's own FlowFields documentation states a FlowField used as a
control's direct source expression is automatically calculated on any
page - no API-page exception is documented, and none could be
reproduced.
- prefer-email-module.bad.al/.md: Codeunit Mail has no Send/GetErrorDesc
members; fixed to the real current 7-argument CreateMessage signature,
and corrected the claim that the legacy path "still runs" - its base
implementation no longer sends anything, only raises integration events.
- check-post-line-batch-pattern.md/.good.al: reframed from a universal
invariant to the standard shape, naming the real Gen./Item/CA/Res./Job/
Insurance/Mfg. Item/FA Jnl.-Check Line/-Post Line/-Post Batch codeunits
it's based on. Added the missing Check Line companion codeunit so the
good fixture is internally complete.
- test-data-must-be-random-and-complete.good.al: removed leftover
"collision-free" wording contradicting the already-corrected article text.
- fixed-choice-set-must-use-enum-not-integer.md: removed the reintroduced
state-count heuristic ("the line is the state count"), aligned with
binary-choice-must-be-boolean.md's semantics-based distinction.
- namespace-must-be-verified-from-source.md: removed the false claim that
the compiler and AL Language Server use different namespace-resolution
rules.
- intrinsic-al-functions-must-use-modern-casing.md: removed the unverified
claim that PascalCase is the VS Code formatter's default output.
Worklist completeness: added cues for the 8 rules in data-modeling,
testing, performance, and web-services that had none (Jesper's explicit
ask), plus the same gap in all 7 style rules from this PR (not explicitly
named this round, but the identical systemic issue) - 15 cues total across
al-data-modeling-review.md, al-testing-review.md, al-performance-review.md,
al-web-services-review.md, and al-style-review.md.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer
parent
fa3d04c56c
commit
a28ba1a1d7
18 changed files with 98 additions and 94 deletions
|
|
@ -1,13 +1,9 @@
|
|||
codeunit 50103 "Order Confirmation Notifier"
|
||||
{
|
||||
procedure Send(FromName: Text; ToAddress: Text; Subject: Text; Body: Text)
|
||||
procedure Send(ToAddress: Text; Subject: Text; Body: Text)
|
||||
var
|
||||
Mail: Codeunit Mail;
|
||||
MailSent: Boolean;
|
||||
begin
|
||||
Mail.CreateMessage(FromName, ToAddress, '', Subject, Body, true);
|
||||
MailSent := Mail.Send();
|
||||
if not MailSent then
|
||||
Message(Mail.GetErrorDesc());
|
||||
Mail.CreateMessage(ToAddress, '', '', Subject, Body, false, false);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -23,6 +23,6 @@ See sample: `prefer-email-module.good.al`.
|
|||
|
||||
## Anti Pattern
|
||||
|
||||
Calling `Codeunit Mail`'s `CreateMessage`/`Send`/`GetErrorDesc`. It still runs, but it is hard-coupled to whatever SMTP setup exists, and leaves no queryable record of what was sent.
|
||||
Calling `Codeunit Mail`'s `CreateMessage`. It still compiles and runs, but current `Codeunit Mail`'s own implementation of `CreateMessage` no longer sends anything by itself — it only raises integration events for a legacy subscriber to act on — so building new code on it means depending on whatever compatibility shim happens to still be wired up, with no first-class connector selection and no queryable Sent/Outbox/Draft record. `Send` and `GetErrorDesc` are not current members of `Codeunit Mail` at all; do not reference them.
|
||||
|
||||
See sample: `prefer-email-module.bad.al`.
|
||||
|
|
|
|||
|
|
@ -1,3 +1,13 @@
|
|||
codeunit 50100 "Meter Jnl.-Check Line"
|
||||
{
|
||||
procedure CheckLine(var MeterJnlLine: Record "Meter Journal Line")
|
||||
begin
|
||||
// Reads setup/dimension data only, shows no UI beyond errors.
|
||||
if MeterJnlLine.Quantity = 0 then
|
||||
Error('Quantity must not be zero.');
|
||||
end;
|
||||
}
|
||||
|
||||
codeunit 50101 "Meter Jnl.-Post Line"
|
||||
{
|
||||
procedure PostLine(var MeterJnlLine: Record "Meter Journal Line")
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
Every journal-based posting routine in Business Central is split across three companion codeunits with distinct, non-overlapping responsibilities: `Check Line` validates one line, `Post Line` writes exactly one line to the ledger, and `Post Batch` loops both across the journal. A document posting routine (posting one document at a time) calls `Post Line` directly and skips `Post Batch`. A new posting routine that blurs this split either misses functionality other code expects to call directly, or exposes an interaction surface it shouldn't.
|
||||
Business Central's own journal-based posting routines consistently follow a three-codeunit split with distinct, non-overlapping responsibilities — `Codeunit "Gen. Jnl.-Check Line"` / `"Gen. Jnl.-Post Line"` / `"Gen. Jnl.-Post Batch"` for the general journal, and the same `<Journal>-Check Line` / `<Journal>-Post Line` / `<Journal>-Post Batch` shape repeated for Item, Resource, Job, Fixed Asset, Insurance, and Cost Accounting journals: `Check Line` validates one line, `Post Line` writes exactly one line to the ledger, and `Post Batch` loops both across the journal. A document posting routine (posting one document at a time) calls `Post Line` directly and skips `Post Batch`. This is the standard shape to evaluate a new journal-based posting routine against, not a platform-enforced constraint — a routine with a genuinely different transaction/reuse shape may legitimately organize itself differently. But a new routine that blurs this split without a specific reason either misses functionality other code expects to call directly, or exposes an interaction surface it shouldn't.
|
||||
|
||||
## Best Practice
|
||||
|
||||
|
|
|
|||
|
|
@ -1,3 +1,4 @@
|
|||
// Master data: Default Dimension records, no Dimension Set ID field.
|
||||
table 50100 "Course"
|
||||
{
|
||||
fields
|
||||
|
|
@ -26,3 +27,56 @@ table 50100 "Course"
|
|||
DimMgt.DeleteDefaultDim(Database::Course, "No.");
|
||||
end;
|
||||
}
|
||||
|
||||
// Transactional/document data: a single Dimension Set ID, inherited from the
|
||||
// related master record and overridable via shortcut dimension fields.
|
||||
table 50101 "Course Registration Header"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; "Customer No."; Code[20])
|
||||
{
|
||||
TableRelation = Customer;
|
||||
|
||||
trigger OnValidate()
|
||||
begin
|
||||
UpdateDimensionSetID();
|
||||
end;
|
||||
}
|
||||
field(10; "Shortcut Dimension 1 Code"; Code[20])
|
||||
{
|
||||
CaptionClass = '1,1,1';
|
||||
TableRelation = "Dimension Value".Code where(
|
||||
"Global Dimension No." = const(1), Blocked = const(false));
|
||||
|
||||
trigger OnValidate()
|
||||
var
|
||||
DimMgt: Codeunit DimensionManagement;
|
||||
begin
|
||||
DimMgt.ValidateShortcutDimValues(1, "Shortcut Dimension 1 Code", "Dimension Set ID");
|
||||
end;
|
||||
}
|
||||
field(480; "Dimension Set ID"; Integer)
|
||||
{
|
||||
Editable = false;
|
||||
TableRelation = "Dimension Set Entry"."Dimension Set ID";
|
||||
}
|
||||
}
|
||||
|
||||
local procedure UpdateDimensionSetID()
|
||||
var
|
||||
Customer: Record Customer;
|
||||
DimMgt: Codeunit DimensionManagement;
|
||||
DefaultDimSource: List of [Dictionary of [Integer, Code[20]]];
|
||||
GlobalDim2Code: Code[20];
|
||||
begin
|
||||
if not Customer.Get("Customer No.") then
|
||||
exit;
|
||||
|
||||
DimMgt.AddDimSource(DefaultDimSource, Database::Customer, "Customer No.");
|
||||
"Dimension Set ID" :=
|
||||
DimMgt.GetDefaultDimID(
|
||||
DefaultDimSource, '', "Shortcut Dimension 1 Code", GlobalDim2Code, "Dimension Set ID", 0);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -13,11 +13,18 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
Adding dimension support to a custom master or document table is not just a matter of adding a `Code[20]` field. Business Central expects a specific set of hooks into `Codeunit "Dimension Management"` so a dimension value is validated, persisted as a Default Dimension record, and flows through to transactions the same way it does for every standard table. Skipping any one hook produces a field that looks correct in the designer but silently fails to save, validate, or carry through to postings.
|
||||
Adding dimension support to a custom table is not just a matter of adding a `Code[20]` field, and master tables and document/transactional tables wire into `Codeunit "Dimension Management"` through two different models — treating them as one mechanism is itself the mistake this article corrects:
|
||||
|
||||
- **Master data** (a custom master table, e.g. "Course") persists **Default Dimension** records: each shortcut dimension field validates through `ValidateDimValueCode`, then the result is saved via `SaveDefaultDim`, and `DeleteDefaultDim` removes them again in `OnDelete`. The master record itself carries no `Dimension Set ID` field.
|
||||
- **Transactional/document data** (a custom document or journal-line table) carries a single **`Dimension Set ID`** field — a pointer to a shared, deduplicated set of dimension values in `Dimension Set Entry`, assembled from whatever the document inherited plus whatever the user overrode. A document does not acquire that ID by calling `SaveDefaultDim`; it builds a source list with `AddDimSource` (naming the related master table and its key, e.g. `Database::Customer`), then calls `GetDefaultDimID` to compute a new `Dimension Set ID` that inherits the master's Default Dimension records. Editing a shortcut dimension field directly on the document validates through `ValidateShortcutDimValues`, which updates the same `Dimension Set ID` in place rather than writing a separate Default Dimension record.
|
||||
|
||||
Skipping the model that actually matches the table's kind produces a field that looks correct in the designer but silently fails to save, validate, or carry through to postings — or, for a document, one that never picks up the customer's/vendor's own dimensions at all.
|
||||
|
||||
## Best Practice
|
||||
|
||||
A master table should validate its dimension fields through `ValidateDimValueCode` (or `ValidateShortcutDimValues` when a `DimSetID` is also needed) and `SaveDefaultDim`, and create/delete the matching Default Dimension records in `OnInsert`/`OnDelete`. A document table should add Shortcut Dimension fields validated the same way, and call `GetDefaultDimID` to pull inherited dimension values from the related master record whenever the field that attaches the document to that master changes.
|
||||
For a master table, validate each shortcut dimension field through `ValidateDimValueCode`, save the result with `SaveDefaultDim`, and delete the matching Default Dimension records in `OnDelete`.
|
||||
|
||||
For a document table, when the field that attaches the document to a master record changes (e.g. `Customer No.`), call `AddDimSource` naming that master table and key, then `GetDefaultDimID` to compute the document's new `Dimension Set ID`, inheriting the master's Default Dimension records. Validate the document's own Shortcut Dimension fields through `ValidateShortcutDimValues`, which updates that same `Dimension Set ID` rather than persisting a separate Default Dimension record.
|
||||
|
||||
See sample: `dimension-management-wiring.good.al`.
|
||||
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
When a variable or field can only take on a fixed set of more than two named, mutually exclusive states — a difficulty level, a document type, a processing status — it should be typed as `Enum` (or `Option` when extending an object that still uses the legacy type). Representing that same state as a plain `Integer` and tracking the meaning of each value in a comment or in a developer's head is a magic-number anti-pattern: the compiler cannot catch an out-of-range value, and branches read as opaque numbers instead of names. This is distinct from a true two-state choice, which should be `Boolean` rather than an enumeration — the line is the state count.
|
||||
When a variable or field represents a fixed set of named, mutually exclusive states — a difficulty level, a document type, a processing status — it should be typed as `Enum` (or `Option` when extending an object that still uses the legacy type). Representing that same state as a plain `Integer` and tracking the meaning of each value in a comment or in a developer's head is a magic-number anti-pattern: the compiler cannot catch an out-of-range value, and branches read as opaque numbers instead of names. This is about semantics, not member count, matching `binary-choice-must-be-boolean.md`'s own distinction: a domain concept that is genuinely a stable true/false predicate belongs in `Boolean` even if someone represents it as a two-value `Enum`, while a domain concept with exactly two current named states is not automatically a Boolean in disguise — it stays an `Enum` when the states are named alternatives rather than a yes/no flag, or when it needs to implement an interface, preserve an existing contract, or leave room for a future third value.
|
||||
|
||||
## Best Practice
|
||||
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
AL is case-insensitive, so `MESSAGE(...)`, `ERROR(...)`, `CONFIRM(...)`, and `STRSUBSTNO(...)` compile and run identically to `Message(...)`, `Error(...)`, `Confirm(...)`, and `StrSubstNo(...)`. Modern AL — the VS Code tooling's default formatter output, Microsoft's own current samples, and current reference codebases — writes intrinsic/built-in function calls in the casing Microsoft assigns to the function's declared name, typically PascalCase. ALL-CAPS calls are a holdover from classic C/AL and signal code that has not been modernized, even though it compiles and runs correctly. Reserved keywords such as `if`, `begin`, and `for` are a separate, already-tooled concern; intrinsic function names are identifiers, not keywords, so that tooling does not catch ALL-CAPS intrinsic function calls.
|
||||
AL is case-insensitive, so `MESSAGE(...)`, `ERROR(...)`, `CONFIRM(...)`, and `STRSUBSTNO(...)` compile and run identically to `Message(...)`, `Error(...)`, `Confirm(...)`, and `StrSubstNo(...)`. Modern AL — Microsoft's own current samples and current reference codebases — writes intrinsic/built-in function calls in the casing Microsoft assigns to the function's declared name, typically PascalCase. This is a codebase-convention claim, not a claim about what the VS Code formatter enforces: the formatter normalizes particular syntax but is not a mechanism for recasing every intrinsic function call, so do not cite formatter behavior as the reason to follow this convention. ALL-CAPS calls are a holdover from classic C/AL and signal code that has not been modernized, even though it compiles and runs correctly. Reserved keywords such as `if`, `begin`, and `for` are a separate, already-tooled concern; intrinsic function names are identifiers, not keywords, so that tooling does not catch ALL-CAPS intrinsic function calls.
|
||||
|
||||
## Best Practice
|
||||
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
Since Business Central 2024 release wave 1, Microsoft's own objects are organized under a deep `Microsoft.*` namespace tree that has been renamed and restructured repeatedly. When adding a `using` directive for an existing AL object (table, codeunit, page, enum, interface, etc.), guessing its namespace from the object's name, from an older codebase, or from general familiarity produces a statement that can look plausible, compile in isolation, and still resolve to the wrong object or fail in the AL Language Server that VS Code actually uses to report errors. The reliable sources are the object's own source file (its `namespace` declaration) or, for a dependency without accessible source, its AL symbol package — not the object's name or a remembered convention.
|
||||
Since Business Central 2024 release wave 1, Microsoft's own objects are organized under a deep `Microsoft.*` namespace tree that has been renamed and restructured repeatedly. When adding a `using` directive for an existing AL object (table, codeunit, page, enum, interface, etc.), guessing its namespace from the object's name, from an older codebase, or from general familiarity produces a statement that can look plausible and still resolve to the wrong object, or fail to resolve at all, once checked against the object's actual current namespace. The reliable sources are the object's own source file (its `namespace` declaration) or, for a dependency without accessible source, its AL symbol package — not the object's name or a remembered convention.
|
||||
|
||||
## Best Practice
|
||||
|
||||
|
|
@ -23,6 +23,6 @@ See sample: `namespace-must-be-verified-from-source.good.al`.
|
|||
|
||||
## Anti Pattern
|
||||
|
||||
Writing a `using` statement from memory, from an incomplete path, or from a plausible-looking guess. It can compile in one build environment while still failing to resolve in VS Code, because the two use different namespace resolution.
|
||||
Writing a `using` statement from memory, from an incomplete path, or from a plausible-looking guess. It can appear correct while actually resolving to the wrong object, or fail to resolve, once checked against stale or mismatched symbols, a different build configuration, or the object's actual current source — not because the compiler and the AL Language Server apply different namespace-resolution rules; they don't.
|
||||
|
||||
See sample: `namespace-must-be-verified-from-source.bad.al`.
|
||||
|
|
|
|||
|
|
@ -4,7 +4,7 @@ var
|
|||
Customer: Record Customer;
|
||||
SalesHeader: Record "Sales Header";
|
||||
begin
|
||||
// Random, collision-free customer — no assumption about what exists.
|
||||
// Freshly created customer, owned by this test — no assumption about what exists.
|
||||
LibrarySales.CreateCustomer(Customer);
|
||||
LibrarySales.CreateSalesHeader(
|
||||
SalesHeader, SalesHeader."Document Type"::Order, Customer."No.");
|
||||
|
|
|
|||
|
|
@ -1,24 +0,0 @@
|
|||
page 50100 "Item Availability API"
|
||||
{
|
||||
PageType = API;
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'inventory';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'itemAvailability';
|
||||
EntitySetName = 'itemAvailabilities';
|
||||
SourceTable = Item;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(General)
|
||||
{
|
||||
field(itemNo; Rec."No.") { }
|
||||
field(quantityOnHand; Rec.Inventory) { }
|
||||
// No OnAfterGetRecord CalcFields — Inventory is a FlowField
|
||||
// and returns 0 to every consumer.
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
|
@ -1,27 +0,0 @@
|
|||
page 50100 "Item Availability API"
|
||||
{
|
||||
PageType = API;
|
||||
APIPublisher = 'contoso';
|
||||
APIGroup = 'inventory';
|
||||
APIVersion = 'v1.0';
|
||||
EntityName = 'itemAvailability';
|
||||
EntitySetName = 'itemAvailabilities';
|
||||
SourceTable = Item;
|
||||
|
||||
layout
|
||||
{
|
||||
area(content)
|
||||
{
|
||||
repeater(General)
|
||||
{
|
||||
field(itemNo; Rec."No.") { }
|
||||
field(quantityOnHand; Rec.Inventory) { }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
trigger OnAfterGetRecord()
|
||||
begin
|
||||
Rec.CalcFields(Inventory);
|
||||
end;
|
||||
}
|
||||
|
|
@ -1,28 +0,0 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: web-services
|
||||
keywords: [api-page, flowfield, calcfields, odata]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Explicitly Calculate FlowFields on API Pages
|
||||
|
||||
> Contributions welcome — open a PR to refine or extend this article.
|
||||
|
||||
## Description
|
||||
|
||||
FlowFields are not stored in the database — Business Central computes them on demand. Regular pages trigger that calculation automatically while rendering, but API pages do not. A FlowField referenced in an API page's layout returns an empty value to external consumers unless it is calculated explicitly, producing a silent data gap in OData responses that is easy to miss in review.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Call `CalcFields` for every FlowField referenced in the page layout from the `OnAfterGetRecord` trigger, combining multiple fields into a single call.
|
||||
|
||||
See sample: `api-page-flowfields-must-be-calcfields.good.al`.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Relying on the implicit calculation that regular pages perform. Any FlowField left out of the `CalcFields` call returns a blank value to every API consumer with no visible error.
|
||||
|
||||
See sample: `api-page-flowfields-must-be-calcfields.bad.al`.
|
||||
|
|
@ -54,6 +54,10 @@ The following targeted checks cover every current `data-modeling` article. Treat
|
|||
- A `Media` or `MediaSet` field is assigned directly between different table types or different field IDs instead of registering each shared item with `MediaSet.Insert` — `share-mediaset-items-with-insert-not-field-assignment`.
|
||||
- A custom document header assigns defaults outside an `InitRecord` boundary, calls `InitRecord` before assigning its number, or places UI-independent defaults only in a page trigger — `initialize-document-defaults-in-initrecord`.
|
||||
- Directed `Round` calls use `'<'` as mathematical floor or `'>'` as mathematical ceiling, especially where negative amounts are possible — `round-direction-symbols-use-magnitude`.
|
||||
- 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.
|
||||
- An existing, already-published table's `keys` block adds, removes, or reorders a field in its primary key or any `Clustered = true` key — `do-not-change-primary-key`. A new table defining its own key for the first time is not this anti-pattern; requires repository/publication context to know the table has already shipped.
|
||||
- Code reads a setup/configuration-table field inside a branch that has already decided the value is required, and blank/zero is handled with a fallback to a default rather than `TestField`/an equivalent guard — `testfield-required-setup-field`. A read that is genuinely optional in that branch, or one already guarded by `TestField`, is not this anti-pattern.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
|
|
|
|||
|
|
@ -58,6 +58,7 @@ Apply these targeted cues even when simple token overlap would rank the article
|
|||
- Worklist `job-queue-on-hold-does-not-stop-running-work.md` when a running job queue handler polls the entry's `Status` or `On Hold` value as a cancellation signal. Exclude application-owned stop requests that are checked before every bounded unit of work, including the first, when completed work and its checkpoint remain consistent and resume logic clears the request.
|
||||
- Worklist `job-queue-category-code-serializes-conflicting-jobs.md` when two or more job queue entries in the same company are shown by the changed context to require mutual exclusion but have empty or different Job Queue Category Codes. Do not infer a conflict merely because jobs touch the same tables, and do not recommend a category to coordinate across companies, environments, or workers outside the job queue dispatcher.
|
||||
- Worklist `store-scheduled-task-id-to-avoid-duplicate-tasks.md` when `TaskScheduler.CreateTask` runs from initialization, login, setup, or another repeatable path without persisting its returned GUID and checking it with `TaskScheduler.TaskExists` before creating a replacement. Exclude one-shot creation and correctly persisted check-before-create flows; concurrent callers still require serialization around that sequence.
|
||||
- A new or changed report object whose usage/name/caption identifies it as a single-record document (invoice, statement, order confirmation) sets or retains `DefaultRenderingLayout = RDLC` — `document-report-word-layout.md`. Do not worklist this from a tabular/list report with heavy aggregation or calculated columns; RDLC/Excel remains the better fit there.
|
||||
|
||||
These targeted inclusions and exclusions override generic token overlap. Do not retain an excluded article solely because the diff contains one of its keywords.
|
||||
|
||||
|
|
|
|||
|
|
@ -51,6 +51,14 @@ Apply these high-signal mappings before fuzzy topic ranking:
|
|||
|
||||
- A `Label` or `TextConst` contains multiple or ambiguous placeholders but has no `Comment`, or its Comment does not explain every placeholder — `label-comment-explains-placeholders.md`. A single placeholder whose meaning is explicit in the text, such as `Customer %1`, is allowed without a Comment and must not be flagged.
|
||||
- A normal two-argument `Evaluate` has a resolved `DateFormula` destination and a hard-coded non-angle-bracket date-formula literal, directly or through a visible constant — `dateformula-evaluate-needs-language-independent-literals.md`. Do not use this cue for dynamic/localized external input, already invariant `<...>` input, or direct `CalcDate(Text, ...)` calls.
|
||||
- `function-call-parentheses-required.md` applies only to a zero-argument invocation written without `()`. Never worklist it from an invocation that already has parentheses or supplies arguments, including `Error(Label, Arg1, Arg2)`.
|
||||
- A new `.app` build artifact appears at the project root or another unversioned/arbitrary location, or is added to source control alongside the AL source that produced it — `al-build-output-must-not-pollute-project-root.md`. A deliberate `--outfolder`/`outputPath` destination added to `.gitignore` is the compliant shape, not the signal to flag.
|
||||
- A new or renamed AL identifier (variable, procedure, parameter, field, object, enum value, or label identifier) contains non-English words — `al-identifiers-english.md`. A caption, tooltip, or other user-facing text value in a non-English language is not this anti-pattern; only the identifier itself is in scope.
|
||||
- A field or variable is typed `Boolean` and its two possible values are genuinely named domain alternatives (a status pair like Inbound/Outbound, Debit/Credit, Buy/Sell) rather than a true/false predicate, or an `Option`/`Enum`/`Integer` models a domain concept that is intrinsically a yes/no flag — `binary-choice-must-be-boolean.md`. The signal is a semantic mismatch between the type and the domain concept, not the current number of states.
|
||||
- A field or variable is typed `Integer` with the meaning of each value tracked only in a comment, or an `Enum` with more than two members is proposed as `Boolean`-like — `fixed-choice-set-must-use-enum-not-integer.md`. Do not flag a genuinely two-state `Enum`/`Option` for having "too few" members; that overlaps `binary-choice-must-be-boolean.md` instead when the domain is a true/false predicate.
|
||||
- A new `using` directive is added for an existing AL object without the diff also showing that object's own `namespace` declaration or a symbol-package lookup backing the choice — `namespace-must-be-verified-from-source.md`. Require repository/dependency context; a single new `using` line cannot itself prove whether the namespace was verified or guessed.
|
||||
- An intrinsic/built-in AL function call (`MESSAGE`, `ERROR`, `CONFIRM`, `STRSUBSTNO`, etc.) is written in ALL-CAPS or another non-PascalCase form — `intrinsic-al-functions-must-use-modern-casing.md`.
|
||||
- A procedure call passes a literal or a computed expression (not a caller-scope variable) to a parameter position the callee declares `var` — `var-parameters-require-an-addressable-variable.md`. This is a compile-time-guaranteed shape; flag it only when the callee's declared signature is visible in the diff or resolvable from context.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions.
|
||||
|
||||
|
|
|
|||
|
|
@ -51,6 +51,7 @@ The following targeted checks cover every current `testing` article. Treat each
|
|||
- Test fixture code manually calls `Init`/`Insert`, invents keys or prerequisite records, or bypasses available `LibrarySales`, `LibraryPurchase`, `LibraryERM`, `LibraryInventory`, `LibraryRandom`, or equivalent library codeunits — `use-library-codeunits-for-test-fixtures`.
|
||||
- `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`.
|
||||
- A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Only nonoptional handlers have to execute: a listed handler declared `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` is optional by design, so do not treat it as unmatched when the run never raises the notification.
|
||||
- A test's `[GIVEN]`/setup looks up a hardcoded code/number/name assumed to already exist instead of creating it, leaves a mandatory field on a created record empty or under-sized, or a scenario-defining value (amount, quantity, percentage, date, threshold, rounding precision) is generated/randomized instead of an explicit chosen value — `test-data-must-be-random-and-complete`. Generating incidental fixture values (identifiers, names, descriptions) via the standard library codeunits is the compliant shape, not the signal to flag.
|
||||
|
||||
Once the candidate worklist is known, resolve layer-precedence conflicts per READ. Drop lower-precedence files whose normative guidance (`## Best Practice` or `## Anti Pattern`) directly contradicts a higher-precedence candidate, and record each dropped file in `suppressed` with `reason: "layer-precedence"`. Files that would have been candidates but are hidden because their layer is disabled in consumer configuration are recorded with `reason: "configuration"`. Files that never became candidates are NOT recorded in `suppressed`.
|
||||
|
||||
|
|
|
|||
|
|
@ -40,6 +40,8 @@ Narrow the relevant files to the subset that applies to the changes under review
|
|||
- The changed AL object names and types — especially pages declared with `PageType = API`, API page `part` controls, queries declared with `QueryType = API`, and procedures that expose bound actions.
|
||||
- The changed properties and triggers, weighted toward API page metadata (`APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SourceTable`, `SourceTableTemporary`), navigation metadata (`SubPageLink`, `Multiplicity`, and visible singleton or collection semantics), CRUD guards (`InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`), the `OnOpenPage` trigger, and `OnValidate` triggers on exposed fields.
|
||||
- Webhook subscriber handlers and subscription lifecycle code, especially code that creates or renews subscriptions, handles `validationToken`, schedules from `expirationDateTime`, or targets resources whose eligibility is visible in the diff.
|
||||
- An API page (`PageType = API`) with `InsertAllowed = true` lists a field in `ODataKeyFields` and that same field control sets `Editable = false` — `api-page-key-fields-must-be-editable-on-insert.md`. A system-generated key such as `SystemId` marked read-only is not this anti-pattern.
|
||||
- An API page exposes a field via `Rec` directly, and that field is a stored value computed from other fields inside an `OnValidate` trigger rather than a FlowField recalculated in `OnAfterGetRecord` — `stored-derived-fields-must-not-be-exposed-directly.md`. Exposing a genuine FlowField, or a value already recalculated in `OnAfterGetRecord`, is not this anti-pattern.
|
||||
- Tokens extracted from the diff that relate to API surface and behaviour (`PageType`, `QueryType`, `API`, `api-page`, `page-part`, `APIPublisher`, `APIGroup`, `APIVersion`, `EntityName`, `EntitySetName`, `ODataKeyFields`, `SystemId`, `SubPageLink`, `subpagelink`, `Multiplicity`, `multiplicity`, `Many`, `ZeroOrOne`, `SourceTableTemporary`, `Job Queue Entry`, `webhook`, `webhookSupportedResources`, `webhook-supported-resources`, `subscriptions`, `notificationUrl`, `validationToken`, `validationtoken`, `expirationDateTime`, `expirationdatetime`, `ServiceEnabled`, `WebServiceActionContext`, `SetActionResponse`, `ReadIsolation`, `IsolationLevel`, `ReadCommitted`, `InsertAllowed`, `ModifyAllowed`, `DeleteAllowed`, `Editable`, `SourceTable`).
|
||||
|
||||
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.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue