mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-08-06 17:36:53 +01:00
Promote validated community knowledge
Move eight net-new rules into the Microsoft layer, remove six overlapping articles, and update review skill discovery and references. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 0b130227-d418-4bc0-9e7d-ec6a37adf039
This commit is contained in:
parent
be1b92b624
commit
c9d90ac605
35 changed files with 136 additions and 478 deletions
|
|
@ -0,0 +1,22 @@
|
|||
---
|
||||
bc-version: [27..]
|
||||
domain: appsource
|
||||
keywords: [app-json, help-url, copilot, grounding, documentation, url-depth, contexturl]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# Keep the Copilot help URL to two path levels
|
||||
|
||||
## Description
|
||||
|
||||
The `help` URL declared in `app.json` is what Copilot uses to ground answers about your app. That URL may be at most **two path levels** deep (for example `https://contoso.com/docs/myapp`). If you point it at a deeper path (three or more segments), Copilot does not use the URL as given: it truncates to the first two levels, drops any fragments and query strings, and then grounds on **all** content beneath that two-level path. The failure is silent — there is no build error — and the practical effect is worse answers, because Copilot may ingest sibling apps' documentation that lives under the same two-level parent.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Organize per-app documentation so the canonical help page sits no deeper than two path levels, and confirm during testing that Copilot citations resolve to your app's content rather than a broader parent. If your docs naturally nest deeper, give each app a dedicated two-level path it owns.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
Setting `help` to a deep, tidy-looking docs path such as `https://contoso.com/docs/products/erp/myapp/setup`. Copilot truncates it to `…/docs/products`, then grounds on everything under that node — pulling in unrelated content and degrading answer quality for your users.
|
||||
|
|
@ -0,0 +1,23 @@
|
|||
table 50120 "FieldError Default Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; "Discount %"; Decimal) { }
|
||||
field(3; "Currency Code"; Code[10]) { }
|
||||
}
|
||||
|
||||
procedure ValidateForRelease()
|
||||
begin
|
||||
// This re-tests a field and gives FieldError a fully formed sentence.
|
||||
// The framework already prepends the caption and appends the value,
|
||||
// so this renders as "Currency Code The Currency Code field must have
|
||||
// a value. in ..." — caption repeated, capital letter mid-sentence,
|
||||
// stray trailing clause.
|
||||
if "Currency Code" = '' then
|
||||
FieldError("Currency Code", 'The Currency Code field must have a value.');
|
||||
|
||||
if "Discount %" > 100 then
|
||||
FieldError("Discount %", 'The Discount % must not be greater than 100 percent.');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,21 @@
|
|||
table 50120 "FieldError Default Good"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; "Discount %"; Decimal) { }
|
||||
field(3; "Currency Code"; Code[10]) { }
|
||||
}
|
||||
|
||||
procedure ValidateForRelease()
|
||||
begin
|
||||
// TestField checks this required-field condition and raises the error
|
||||
// with caption and record context supplied by the framework.
|
||||
TestField("Currency Code");
|
||||
|
||||
// Condition already evaluated: pass only a lowercase predicate so it
|
||||
// reads as one sentence after the auto-inserted caption and value.
|
||||
if "Discount %" > 100 then
|
||||
FieldError("Discount %", 'cannot exceed 100');
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: error-handling
|
||||
keywords: [fielderror, testfield, error-message, field-caption, lowercase-convention, record-context, validation]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
# Rely On FieldError's Auto-Generated Context And Pass Only A Lowercase Predicate
|
||||
|
||||
## Description
|
||||
`Rec.FieldError(FieldNo)` does not just print the text you give it. Business Central automatically prepends the field caption, appends the current field value (when non-blank), and suffixes the table name and primary-key values for record identification. The optional second argument is only the middle predicate of that sentence — e.g. `"must be unique"`, not a whole self-contained message. Misunderstanding this leads to messages that duplicate the caption and value or read as broken grammar, because the framework's surrounding text is built to join a lowercase fragment.
|
||||
|
||||
## Best Practice
|
||||
For a plain required-field check, prefer `TestField`, which tests the condition and raises the error in one call. When the condition is non-trivial and has already been evaluated, call `FieldError(FieldNo)` with no message to get the localized default (`must have a value`, `is not valid`, etc.), or pass a short lowercase predicate such as `FieldError(FieldNo, 'must be a positive number')`. Start the custom text with a lowercase letter so it reads as one sentence with the auto-inserted caption, and use a field-number reference (or the field token) rather than a hard-coded field name so captions and translations stay correct. Let the framework supply the caption, value, table, and key context for you.
|
||||
|
||||
## Anti Pattern
|
||||
Re-testing a condition you already evaluated, or passing a fully formed sentence like `'The Amount field must be positive.'` to `FieldError`. The result reads as `Amount The Amount field must be positive. in Gen. Journal Line ...` — capital letter mid-sentence, caption and value repeated, and a stray trailing clause. Reviewer signals: a `FieldError` argument that names the field, restates the current value, starts with a capital letter, or ends with a period. Each is a sign the author treated `FieldError` like `Error` instead of as a predicate slotted into framework-generated context.
|
||||
|
|
@ -0,0 +1,26 @@
|
|||
table 50122 "FieldError vs TestField Bad"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; "Posting Date"; Date) { }
|
||||
field(3; "Amount"; Decimal) { }
|
||||
}
|
||||
|
||||
procedure PostDocument()
|
||||
begin
|
||||
// FieldError performs no comparison and raises as soon as it is
|
||||
// reached, so this "check" terminates PostDocument every time — the
|
||||
// Posting Date is never actually tested, and the amount rule below is
|
||||
// dead code.
|
||||
FieldError("Posting Date", 'must be filled in');
|
||||
|
||||
if IsAmountOutsideAllowedRange("Amount") then
|
||||
Error('Amount is out of range.');
|
||||
end;
|
||||
|
||||
local procedure IsAmountOutsideAllowedRange(Value: Decimal): Boolean
|
||||
begin
|
||||
exit((Value < 0) or (Value > 1000000));
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,27 @@
|
|||
table 50122 "FieldError vs TestField Good"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "No."; Code[20]) { }
|
||||
field(2; "Posting Date"; Date) { }
|
||||
field(3; "Amount"; Decimal) { }
|
||||
}
|
||||
|
||||
procedure PostDocument()
|
||||
begin
|
||||
// TestField performs this simple presence check and raises only when
|
||||
// the field is empty. Self-documenting prerequisite.
|
||||
TestField("Posting Date");
|
||||
|
||||
// Business logic has already determined the value is invalid;
|
||||
// FieldError raises a tailored, record-aware message with no
|
||||
// condition of its own.
|
||||
if IsAmountOutsideAllowedRange("Amount") then
|
||||
FieldError("Amount", 'is outside the approved posting range');
|
||||
end;
|
||||
|
||||
local procedure IsAmountOutsideAllowedRange(Value: Decimal): Boolean
|
||||
begin
|
||||
exit((Value < 0) or (Value > 1000000));
|
||||
end;
|
||||
}
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: error-handling
|
||||
keywords: [fielderror, testfield, field-validation, onvalidate, error-message, mandatory-field, record-context]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
# Choose `TestField` For Conditional Checks And `FieldError` For Already-Failed Validation
|
||||
|
||||
## Description
|
||||
`TestField` and `FieldError` look interchangeable but behave differently, and choosing the wrong one produces either dead code or a check that never fires. `TestField` performs the comparison itself and throws only when the field is empty or does not match the supplied value; `FieldError` performs no comparison and always raises an error the moment it is reached. Both attach the field caption and the record's primary-key context to the message automatically, which is why neither should be replaced by a hand-built `Error` call that interpolates the field name as a literal.
|
||||
|
||||
## Best Practice
|
||||
Use `TestField` when the condition is a simple presence-or-equality check on a single field — mandatory-field gates and prerequisite checks at the top of a procedure read clearly and self-document intent. Use `FieldError` inside an `OnValidate` trigger or a validation procedure where surrounding business logic has already determined the value is invalid and you want a specific, custom message. Rely on the built-in field-and-record context both methods add rather than re-stating the field name in the text.
|
||||
|
||||
## Anti Pattern
|
||||
Calling `FieldError` to "test" a field — placing it on a path that is reached unconditionally and expecting it to validate — terminates execution every time because `FieldError` never evaluates a condition. The inverse smell is reaching for `TestField` when the rule needs a tailored message, then bolting a vague generic string onto a check that cannot express the real business reason. A reviewer can spot the first by a `FieldError` that is not guarded by a preceding `if`, and the second by a `TestField` whose intent comment describes a condition more complex than presence or equality.
|
||||
21
microsoft/knowledge/security/secrets-isolated-storage.bad.al
Normal file
21
microsoft/knowledge/security/secrets-isolated-storage.bad.al
Normal file
|
|
@ -0,0 +1,21 @@
|
|||
table 50134 "Api Setup Bad Sample"
|
||||
{
|
||||
fields
|
||||
{
|
||||
field(1; "Primary Key"; Code[10]) { }
|
||||
|
||||
// A secret in an ordinary Text field is readable by anyone with table
|
||||
// permission, ships in RapidStart packages and Excel exports, and
|
||||
// appears in record snapshots. No DataClassification tag makes it safe;
|
||||
// it belongs in IsolatedStorage instead.
|
||||
field(10; "API Key"; Text[250])
|
||||
{
|
||||
DataClassification = CustomerContent;
|
||||
}
|
||||
}
|
||||
|
||||
keys
|
||||
{
|
||||
key(PK; "Primary Key") { Clustered = true; }
|
||||
}
|
||||
}
|
||||
|
|
@ -0,0 +1,15 @@
|
|||
codeunit 50134 "Api Credential Good Sample"
|
||||
{
|
||||
procedure StoreApiKey(ApiKey: SecretText)
|
||||
begin
|
||||
// Credentials live in IsolatedStorage, invisible to record reads, API
|
||||
// pages, RapidStart packages, and Excel export.
|
||||
IsolatedStorage.SetEncrypted('ExternalApiKey', ApiKey, DataScope::Module);
|
||||
end;
|
||||
|
||||
procedure GetApiKey() ApiKey: SecretText
|
||||
begin
|
||||
if not IsolatedStorage.Get('ExternalApiKey', DataScope::Module, ApiKey) then
|
||||
Error('The external API key has not been configured.');
|
||||
end;
|
||||
}
|
||||
22
microsoft/knowledge/security/secrets-isolated-storage.md
Normal file
22
microsoft/knowledge/security/secrets-isolated-storage.md
Normal file
|
|
@ -0,0 +1,22 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: security
|
||||
keywords: [isolatedstorage, secrets, api-key, oauth-token, connection-string, table-field, credentials]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
|
||||
# A secret belongs in IsolatedStorage, never in a table field
|
||||
|
||||
## Description
|
||||
|
||||
API keys, OAuth tokens, client secrets, and connection strings must not be stored in an ordinary table `Text` field — not even on a hidden setup table. A regular field is exposed through record reads, page display, RapidStart and Excel export, report datasets, and surfaces in `DataClassification` review; anyone with table permission can read it. The correct home is `IsolatedStorage`, which is invisible to database queries, API pages, and configuration packages. The storage-*location* decision is the rule here; how to scope and encrypt the value once it is in IsolatedStorage is covered separately.
|
||||
|
||||
## Best Practice
|
||||
|
||||
Persist every credential in `IsolatedStorage`, write it at the point of capture, and read it only when needed. Prefer `SetEncrypted` when the value fits its documented length limit. On BC24 and later, carry the value through the `SecretText` overloads; on earlier releases, keep any required `Text` handling inside a `[NonDebuggable]` boundary. Choose the `DataScope` that matches the credential's lifetime. See `isolatedstorage-datascope-module-vs-company`, `isolatedstorage-setencrypted-for-sensitive-values`, and `secrettext-for-credentials` for those separate concerns.
|
||||
|
||||
## Anti Pattern
|
||||
|
||||
A "Setup" or "Connection" table carrying a `Text` field named `API Key`, `Password`, or `Client Secret`. The value is now readable by any object with table permission, ships in RapidStart packages and Excel exports, and appears in record snapshots — a credential disclosure that no amount of encryption-in-transit elsewhere makes up for. Reviewer signal: a secret-shaped field declared on a table instead of an `IsolatedStorage` call.
|
||||
18
microsoft/knowledge/ui/fasttab-field-importance.md
Normal file
18
microsoft/knowledge/ui/fasttab-field-importance.md
Normal file
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: ui
|
||||
keywords: [importance, promoted, additional, fasttab, show-more, summary-line, progressive-disclosure, field-visibility]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
# Set Field Importance To Drive FastTab Progressive Disclosure
|
||||
|
||||
## Description
|
||||
A FastTab field's `Importance` property controls whether the field is visible immediately, hidden behind "Show more", or surfaced on the collapsed FastTab header summary line. The three values are `Standard` (the default, shown in the expanded FastTab), `Promoted` (also rendered on the FastTab header when the tab is collapsed), and `Additional` (hidden until the user clicks "Show more"). Misusing these values either clutters the summary line or buries fields users need on every transaction, so reviewers should treat `Importance` as a deliberate layout decision rather than an afterthought.
|
||||
|
||||
## Best Practice
|
||||
Promote only the small set of identifying fields per FastTab that users must read at a glance without expanding — name, status, key amount — so the collapsed header summary line stays scannable. Leave the everyday working fields at `Standard`, and push rarely-touched fields (legacy compatibility fields, system timestamps, seldom-changed configuration) to `Additional`. Note that field-level `Importance = Promoted` is unrelated to action promotion on the page action bar; it governs FastTab field visibility only. Do not rely on initial expand or collapse state, which you cannot set programmatically and which the platform may personalize per user — design assuming any FastTab may be collapsed.
|
||||
|
||||
## Anti Pattern
|
||||
Setting `Importance = Promoted` on most fields of a FastTab so "everything is important" defeats progressive disclosure: the collapsed summary line overflows and conveys nothing at a glance. The opposite failure is marking frequently edited fields `Additional`, forcing users to click "Show more" on every record. A detectable signal is a FastTab whose fields are nearly all `Promoted`, or a FastTab containing only `Additional` fields, which renders as an empty tab until expanded.
|
||||
18
microsoft/knowledge/ui/page-background-tasks.md
Normal file
18
microsoft/knowledge/ui/page-background-tasks.md
Normal file
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
bc-version: [all]
|
||||
domain: ui
|
||||
keywords: [enqueuebackgroundtask, async-calculation, child-session, factbox, cue-tile, onaftergetcurrrecord, responsive-page, read-only]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
# Offload Slow Read-Only Page Calculations To Background Tasks
|
||||
|
||||
## Description
|
||||
Pages that compute statistics, aggregates, or external lookups inline block the page from rendering until the calculation finishes, producing a visible freeze on FactBoxes, cue tiles, and calculated fields. Business Central provides page background tasks: `CurrPage.EnqueueBackgroundTask` runs a dedicated codeunit in a read-only child session and returns values via `OnPageBackgroundTaskCompleted`, so the page opens immediately and fills in computed values as they arrive. This matters because users should never wait on a calculation they may not need. The mechanism has specific rules that are easy to get wrong, which is why it warrants an explicit pattern.
|
||||
|
||||
## Best Practice
|
||||
Move any noticeable read-only computation off the synchronous render path into a background task. Enqueue from `OnAfterGetCurrRecord` so the task is tied to the currently focused record, and pass small payloads through the `Dictionary of [Text, Text]` input/output, converting types with `Format` and `Evaluate`. Keep each task focused on one value or a small related set rather than one large task, and show a placeholder until results land. Because tasks auto-cancel when the page closes, the record changes, or a same-ID task is re-enqueued, always supply sensible defaults and handle the timeout path in `OnPageBackgroundTaskError` — never let critical functionality depend on completion. For tests, drive the task synchronously with `RunPageBackgroundTask`.
|
||||
|
||||
## Anti Pattern
|
||||
Enqueuing from `OnAfterGetRecord` on a list page fires the task for every row, and each cancels the instant the selection moves to the next row — pure wasted child-session churn; a reviewer spots `EnqueueBackgroundTask` called from `OnAfterGetRecord` (or from `OnOpenPage`, where the record context is not yet stable). The other tell is a task codeunit attempting a database write or `Modify`: background tasks run read-only and the write fails at runtime. Inline heavy calculation directly in `OnAfterGetCurrRecord` with no task at all is the baseline smell — it reintroduces the page freeze the feature exists to remove.
|
||||
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
bc-version: [21..]
|
||||
domain: ui
|
||||
keywords: [actionref, promoted-actions, area-promoted, promotedcategory, promotedonly, action-bar, legacy-syntax]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
# Promote Actions With The Modern `actionref` Syntax, Never The Legacy `Promoted` Properties
|
||||
|
||||
## Description
|
||||
Business Central 2022 release wave 2 (v21) introduced the `area(Promoted)` block with `actionref` as the way to promote page actions, separating an action's definition from its promotion. The older approach set `Promoted`, `PromotedCategory`, `PromotedOnly`, and `PromotedIsBig` directly on each action. The two syntaxes cannot be mixed within a single page or page extension, and choosing the legacy one entangles definition with presentation, making the action bar harder to maintain and to extend.
|
||||
|
||||
## Best Practice
|
||||
For new pages and page extensions, define actions in their normal `area`, then promote selected ones with `actionref` inside `area(Promoted)`, grouping them under explicit categories such as `Category_Process` and entity-named groups. This keeps each action defined once and referenced where it should appear, supports split buttons via `ShowAs`, and lets an extension promote a base action without redefining it. When extending a page, you may use modern syntax even if the base page used legacy properties (and vice versa) — the no-mixing rule is per-object, not per-dependency-tree.
|
||||
|
||||
## Anti Pattern
|
||||
Setting `Promoted = true` (with `PromotedCategory`, `PromotedOnly`, or `PromotedIsBig`) on actions in new code, or attempting to combine those properties with an `area(Promoted)` block in the same object — the latter fails to compile. The reviewer signal is any `Promoted`-prefixed property on an action in a newly authored page or page extension; flag it and convert to `actionref` (VS Code offers an automated conversion). Note separately that once an action is promoted in a published app, removing the promotion is a breaking change (AS0031/AW0013), so promote conservatively rather than walking it back later.
|
||||
18
microsoft/knowledge/ui/promoted-action-groups.md
Normal file
18
microsoft/knowledge/ui/promoted-action-groups.md
Normal file
|
|
@ -0,0 +1,18 @@
|
|||
---
|
||||
bc-version: [21..]
|
||||
domain: ui
|
||||
keywords: [action-groups, area-promoted, actionref, showas, split-button, group-caption, navigate-group, entity-group]
|
||||
technologies: [al]
|
||||
countries: [w1]
|
||||
application-area: [all]
|
||||
---
|
||||
# Use Standard Promoted Action Group Names And Placements
|
||||
|
||||
## Description
|
||||
Business Central ships a fixed vocabulary of promoted action groups, and users build muscle memory around where each kind of action lives. When you define `area(Promoted)` groups, reusing the standard caption and placement for a given action class makes the page feel native; inventing your own caption or putting an action in the wrong group forces every user to relearn your page. Frontier models tend to emit plausible-but-nonstandard captions (`Go To`, `Vendor Actions`, `Related`) instead of the established BC names, which is exactly what breaks cross-page consistency.
|
||||
|
||||
## Best Practice
|
||||
Map each action to its conventional group and use the exact standard caption: `Home`/`Process` for data-modifying and workflow actions (entity/card/document pages use `Home`, lists and worksheets use `Process`); an entity-named group (`Customer`, `Item`, `Order`) for navigation tied to the current record (statistics, ledger entries, dimensions); `Navigate` for related pages that are useful regardless of the selected record; `Report` for printing and analysis; and the workflow groups `Posting`, `Release`, `Approve`, `Request Approval`, and `Prepare` for their respective document lifecycle actions. Standard guidance recommends `ShowAs = SplitButton` for `Posting` (Post / Post and Print / Preview) and `Release` (Release / Reopen), while the other common groups normally render as standard groups. Use a split button elsewhere only for closely related alternatives with an obvious primary action. The first enabled and visible action becomes the primary button, so place the expected default first and remember that extensions or personalization can reorder it. Within a common group keep the same action sequence you see on the matching base-app page (for example, mirror Sales Order for a sales document) so order stays predictable.
|
||||
|
||||
## Anti Pattern
|
||||
Custom captions for what is really a standard group (`Vendor Actions` instead of the `Vendor` entity group, `Go To` instead of `Navigate`), posting or statistics actions dropped into the wrong group, or many tiny one-action groups that fragment the ribbon. The reviewer signal is an `area(Promoted)` block whose `group` captions do not match the base-application names for the same page type, or a split button whose actions are unrelated or lack an obvious primary operation.
|
||||
Loading…
Add table
Add a link
Reference in a new issue