From 9cc90c16ab9535f1a778cb8737d3370719eaefe9 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Fri, 4 Sep 2026 20:59:54 +0200 Subject: [PATCH 1/3] Add 18 more community AL/BC patterns across appsource, data-modeling, error-handling, security, style, testing, ui, upgrade, and web-services Second contribution from CURABIS ApS, generalized from patterns observed across real AppSource/PTE development. Cross-checked against the current microsoft/knowledge corpus before opening; several originally-drafted candidates were dropped as duplicates of existing files. --- .../release-must-update-app-version.md | 37 +++++++++ .../code-must-not-change-workdate.bad.al | 9 +++ .../code-must-not-change-workdate.good.al | 11 +++ .../code-must-not-change-workdate.md | 40 +++++++++ .../pictures-must-use-media-not-blob.bad.al | 11 +++ .../pictures-must-use-media-not-blob.good.al | 11 +++ .../pictures-must-use-media-not-blob.md | 44 ++++++++++ ...ust-match-bc-table-type-conventions.bad.al | 17 ++++ ...st-match-bc-table-type-conventions.good.al | 16 ++++ ...gn-must-match-bc-table-type-conventions.md | 81 +++++++++++++++++++ ...ensive-code-must-match-blast-radius.bad.al | 11 +++ ...nsive-code-must-match-blast-radius.good.al | 10 +++ ...-offensive-code-must-match-blast-radius.md | 26 ++++++ .../log-writes-must-survive-rollback.bad.al | 24 ++++++ .../log-writes-must-survive-rollback.good.al | 26 ++++++ .../log-writes-must-survive-rollback.md | 26 ++++++ ...objects-must-be-in-a-permission-set.bad.al | 12 +++ ...bjects-must-be-in-a-permission-set.good.al | 10 +++ ...sed-objects-must-be-in-a-permission-set.md | 26 ++++++ ...not-restate-what-code-already-shows.bad.al | 18 +++++ ...ot-restate-what-code-already-shows.good.al | 20 +++++ ...ust-not-restate-what-code-already-shows.md | 30 +++++++ ...ges-must-not-contain-business-logic.bad.al | 20 +++++ ...es-must-not-contain-business-logic.good.al | 31 +++++++ .../pages-must-not-contain-business-logic.md | 31 +++++++ ...ce-organized-by-feature-not-object-type.md | 36 +++++++++ ...bcpt-scenarios-must-be-app-specific.bad.al | 29 +++++++ ...cpt-scenarios-must-be-app-specific.good.al | 43 ++++++++++ .../bcpt-scenarios-must-be-app-specific.md | 26 ++++++ ...-must-cover-full-precondition-chain.bad.al | 12 +++ ...must-cover-full-precondition-chain.good.al | 15 ++++ ...ocks-must-cover-full-precondition-chain.md | 26 ++++++ .../testing/test-feature-scenario-tags.bad.al | 21 +++++ .../test-feature-scenario-tags.good.al | 26 ++++++ .../testing/test-feature-scenario-tags.md | 26 ++++++ .../testing/test-one-when-per-test.bad.al | 15 ++++ .../testing/test-one-when-per-test.good.al | 29 +++++++ .../testing/test-one-when-per-test.md | 34 ++++++++ .../testing/ui-test-codeunit-naming.bad.al | 24 ++++++ .../testing/ui-test-codeunit-naming.good.al | 33 ++++++++ .../testing/ui-test-codeunit-naming.md | 26 ++++++ ...must-match-bc-page-type-conventions.bad.al | 21 +++++ ...ust-match-bc-page-type-conventions.good.al | 20 +++++ ...ign-must-match-bc-page-type-conventions.md | 76 +++++++++++++++++ ...rade-tag-logic-must-not-nest-deeply.bad.al | 14 ++++ ...ade-tag-logic-must-not-nest-deeply.good.al | 14 ++++ .../upgrade-tag-logic-must-not-nest-deeply.md | 28 +++++++ ...i-page-least-privilege-write-access.bad.al | 23 ++++++ ...-page-least-privilege-write-access.good.al | 21 +++++ .../api-page-least-privilege-write-access.md | 26 ++++++ 50 files changed, 1262 insertions(+) create mode 100644 microsoft/knowledge/appsource/release-must-update-app-version.md create mode 100644 microsoft/knowledge/data-modeling/code-must-not-change-workdate.bad.al create mode 100644 microsoft/knowledge/data-modeling/code-must-not-change-workdate.good.al create mode 100644 microsoft/knowledge/data-modeling/code-must-not-change-workdate.md create mode 100644 microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.bad.al create mode 100644 microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.good.al create mode 100644 microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md create mode 100644 microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.bad.al create mode 100644 microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.good.al create mode 100644 microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md create mode 100644 microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al create mode 100644 microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al create mode 100644 microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md create mode 100644 microsoft/knowledge/error-handling/log-writes-must-survive-rollback.bad.al create mode 100644 microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al create mode 100644 microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md create mode 100644 microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.bad.al create mode 100644 microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.good.al create mode 100644 microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md create mode 100644 microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.bad.al create mode 100644 microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.good.al create mode 100644 microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md create mode 100644 microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al create mode 100644 microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al create mode 100644 microsoft/knowledge/style/pages-must-not-contain-business-logic.md create mode 100644 microsoft/knowledge/style/source-organized-by-feature-not-object-type.md create mode 100644 microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.bad.al create mode 100644 microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al create mode 100644 microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md create mode 100644 microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al create mode 100644 microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al create mode 100644 microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md create mode 100644 microsoft/knowledge/testing/test-feature-scenario-tags.bad.al create mode 100644 microsoft/knowledge/testing/test-feature-scenario-tags.good.al create mode 100644 microsoft/knowledge/testing/test-feature-scenario-tags.md create mode 100644 microsoft/knowledge/testing/test-one-when-per-test.bad.al create mode 100644 microsoft/knowledge/testing/test-one-when-per-test.good.al create mode 100644 microsoft/knowledge/testing/test-one-when-per-test.md create mode 100644 microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al create mode 100644 microsoft/knowledge/testing/ui-test-codeunit-naming.good.al create mode 100644 microsoft/knowledge/testing/ui-test-codeunit-naming.md create mode 100644 microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.bad.al create mode 100644 microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.good.al create mode 100644 microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md create mode 100644 microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al create mode 100644 microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al create mode 100644 microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md create mode 100644 microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al create mode 100644 microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al create mode 100644 microsoft/knowledge/web-services/api-page-least-privilege-write-access.md diff --git a/microsoft/knowledge/appsource/release-must-update-app-version.md b/microsoft/knowledge/appsource/release-must-update-app-version.md new file mode 100644 index 00000000..91fb0938 --- /dev/null +++ b/microsoft/knowledge/appsource/release-must-update-app-version.md @@ -0,0 +1,37 @@ +--- +bc-version: [all] +domain: appsource +keywords: [version, release, app-json, semver, al-go, appsource] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Update the app version at every release + +## Description + +At every release — a branch merged to `main`, a tagged release build, or an AppSource submission — the app's version is consciously updated, not left to the pipeline alone. + +| Version part | Owner | When | +|---|---|---| +| Major | Developer decision | Breaking change (schema, API, removed objects) | +| Minor | Developer decision | Every release with new functionality | +| Build / Revision | AL-Go pipeline | Automatic — never hand-edited | + +The version number is the only identity a deployed app has. Two customer environments running "the same" version with different code is an undiagnosable support case; an AppSource submission with an unchanged major.minor is a rejected submission. AL-Go increments build numbers on every CI run, which creates the illusion that versioning is handled — but major.minor is a human statement about compatibility, and no pipeline can make it. + +## Best Practice + + Before the release merge: + app.json: "version": "1.3.0.0" (new functionality -> minor bump) + AL-Go settings: "repoVersion": "1.3" (where used) + Then: feature branch -> main via PR, tag, release. + +Feature branches never touch the version; only the release does. + +## Anti Pattern + + Branch merged to main and released. + app.json still says "version": "1.2.0.0" -- same as the previous release. + Two different code states now share one version identity. diff --git a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.bad.al b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.bad.al new file mode 100644 index 00000000..0c265e9d --- /dev/null +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.bad.al @@ -0,0 +1,9 @@ +codeunit 50101 "Batch Job Runner" +{ + procedure AdvanceToNextBusinessDay() + begin + // Anti-pattern: repurposes the user's session WorkDate as a + // scratch variable for unrelated business logic. + WorkDate(CalcDate('<1D>', WorkDate())); + end; +} diff --git a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.good.al b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.good.al new file mode 100644 index 00000000..f1dcf5ed --- /dev/null +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.good.al @@ -0,0 +1,11 @@ +codeunit 50100 "Posting Date Helper" +{ + procedure GetDefaultPostingDate(): Date + var + PostingDate: Date; + begin + // Read the work date to default a value; never write to it. + PostingDate := WorkDate(); + exit(PostingDate); + end; +} diff --git a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md new file mode 100644 index 00000000..428e6dbd --- /dev/null +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md @@ -0,0 +1,40 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [workdate, session-setting, user-control, side-effect] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Application code must not change the WorkDate + +## Description + +The work date is a per-user session setting the user controls from the +client (the date shown in the top-right corner, used to default posting +dates and date filters). Application code must never call the `WorkDate` +function to set a new value. Doing so changes what the user sees and +defaults to for the rest of their session, as a side effect of running +unrelated business logic — a surprising, hard-to-trace behavior change the +user never asked for and has no visibility into. + +This is a call-direction distinction: reading the current work date via +`WorkDate` (or `WorkDate()` with no argument) is fine and common — it is +only the assignment form, `WorkDate(NewDate)`, that is the anti-pattern. + +## Best Practice + +Read the work date to default a value; never write to it. + +See sample: `code-must-not-change-workdate.good.al`. + +## Anti Pattern + +Setting the work date from within a codeunit, report, or page action +changes session state the user owns, for the duration of a call that has +nothing to do with the user's date preference. If a scenario genuinely +needs a specific date for a calculation, pass or compute that date as a +local variable — never repurpose the session's `WorkDate`. + +See sample: `code-must-not-change-workdate.bad.al`. diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.bad.al b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.bad.al new file mode 100644 index 00000000..e20775a6 --- /dev/null +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.bad.al @@ -0,0 +1,11 @@ +table 50111 "Sample Item Card" +{ + fields + { + field(1; "No."; Code[20]) { } + field(50; Picture; BLOB) + { + Caption = 'Picture'; + } + } +} diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.good.al b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.good.al new file mode 100644 index 00000000..4fec633b --- /dev/null +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.good.al @@ -0,0 +1,11 @@ +table 50110 "Sample Item Card" +{ + fields + { + field(1; "No."; Code[20]) { } + field(50; Picture; Media) + { + Caption = 'Picture'; + } + } +} diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md new file mode 100644 index 00000000..3786d471 --- /dev/null +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md @@ -0,0 +1,44 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [blob, media, mediaset, picture-field, image-field, table-design] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Pictures must be stored in a Media/MediaSet field, not BLOB + +## Description + +`BLOB` is still a valid AL field type for arbitrary binary data, but it is +not the right choice for storing pictures or images. The current +recommendation is the `Media` field type for a single image, or +`MediaSet` when a record needs several image variants (e.g. multiple +sizes). Media/MediaSet integrate with the platform's picture control, the +media repository, image caching, and thumbnail generation — none of which +a plain `BLOB` field gets. A `BLOB` field storing a picture works, but it +is the legacy approach: no caching, no thumbnail support, and no +integration with the standard picture controls used across Business +Central pages. + +`BLOB` remains the correct choice for genuinely arbitrary binary payloads +that are not images and don't benefit from the media pipeline (e.g. a raw +file attachment blob unrelated to picture rendering). + +## Best Practice + +Use `Media` (or `MediaSet` for multiple image variants) for any field that +holds a picture. + +See sample: `pictures-must-use-media-not-blob.good.al`. + +## Anti Pattern + +A `BLOB` field named "Picture" compiles and stores the image bytes, but +it misses the picture control integration, caching, and thumbnail +generation that a `Media`/`MediaSet` field provides for free — the anti +pattern is choosing `BLOB` for image storage out of habit rather than +recognizing that the field is holding a picture, not generic binary data. + +See sample: `pictures-must-use-media-not-blob.bad.al`. diff --git a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.bad.al b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.bad.al new file mode 100644 index 00000000..c2416403 --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.bad.al @@ -0,0 +1,17 @@ +table 50121 "Sample Ledger Entry" +{ + fields + { + // Anti-pattern: a Ledger table's key must never be user-editable. + field(1; "Entry No."; Integer) { } + field(2; "Posting Date"; Date) { } + field(3; Amount; Decimal) { } + } + keys + { + key(PK; "Entry No.") { Clustered = true; } + } + // No AutoIncrement, no guard against manual insert/delete — a user or + // integration can renumber or remove entries, breaking the Ledger + // type's audit-trail guarantee. +} diff --git a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.good.al b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.good.al new file mode 100644 index 00000000..825ca45c --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.good.al @@ -0,0 +1,16 @@ +table 50120 "Sample Ledger Entry" +{ + fields + { + // Ledger primary key: Integer "Entry No.", set only by posting. + field(1; "Entry No."; Integer) { AutoIncrement = true; } + field(2; "Posting Date"; Date) { } + field(3; Amount; Decimal) { } + } + keys + { + key(PK; "Entry No.") { Clustered = true; } + } + // No user-facing Insert/Delete/Modify path is exposed; rows are + // created exclusively by the posting routine. +} diff --git a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md new file mode 100644 index 00000000..01f27937 --- /dev/null +++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md @@ -0,0 +1,81 @@ +--- +bc-version: [all] +domain: data-modeling +keywords: [tables, table-design, naming-conventions, primary-key, master-table, ledger-table, journal-table, register-table, document-table, setup-table, subsidiary-table, supplemental-table] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Tables must match one of Business Central's nine table-type conventions + +## Description + +Business Central's Base Application follows nine recurring table types — +Master, Supplemental, Subsidiary, Ledger, Register, Journal, Document, +Document History, and Setup. Each type fixes a naming pattern, a +primary-key shape, and a set of associated pages. A new or extended table +whose design doesn't match the conventions of its own type is either +misclassified or built inconsistently with the rest of the application, +and should be flagged in review even if it compiles. Before assigning a +primary key or naming a new table, first identify which of the nine types +it is — that answer fixes almost every other design decision. + +## Best Practice + +Match the table's design to its type: + +1. **Master** (Customer, Item) — one record is the subject; primary key + `Code[20]` named `No.`; description field in `DataCaptionFields`; Card + + List (+ Statistics) pages. +2. **Supplemental** (Currency, Language) — used across functional areas; + primary key `Code[10]` named `Code`; one List page, plural name, set as + `LookupPageID`. +3. **Subsidiary** (Item Vendor) — subsidiary to a Master/Supplemental + table; primary key is the parent key field(s), optionally + `Line No.`; + Worksheet or Tabular page, always filtered by the calling page. +4. **Ledger** (Cust. Ledger Entry) — transactional record of a functional + area; primary key `Integer` `Entry No.`, always auto-generated by + posting, never user-editable, no free add/delete; List page as + `LookupPageID`/`DrillDownPageID`. +5. **Register** (G/L Register) — table of contents for its Ledger, one row + per posting run; primary key `Integer` `No.`, auto-generated; carries + `From Entry No.`/`To Entry No.`; List page with a link to the Ledger. +6. **Journal** (Resource Journal Line) — where users enter data before + posting to a Ledger; primary key Template + Batch + `Integer` `Line No.`; + Worksheet page with `AutoSplitKey`, a Posting action, and a link to the + Ledger. +7. **Document** (Sales Header/Line) — posts to Ledgers via Journals, not + directly; Header primary key `Code[20]` `No.` (or + `Option Document + Type`); Line primary key = Header key renamed ` No.` + + `Integer Line No.`; Document/Card page with a Posting action and a lines + subpage. +8. **Document History** (Posted Sales Invoice Header/Line) — posted copy of + a Document table, created during posting; mirrors the source table's + fields; never user-editable; same page shape but the Line-equivalent is + a List page, not a Worksheet. +9. **Setup** (General Ledger Setup) — exactly one record for a functional + area; primary key `Code[10]` named `Primary Key`, always blank; one page + with the key field hidden, whose `OnOpenPage` creates the singleton the + first time it's opened (`Reset()` → `Get()` → if not found, `Init()` → + `Insert()`) rather than assuming the record pre-exists. + +A table named "Setup" that holds more than one record follows Subsidiary +rules instead — the name alone is not proof of type. When a table's +identity can't be resolved from its definition alone (e.g. a "Setup"-named +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. + +See sample: `table-design-must-match-bc-table-type-conventions.good.al`. + +## Anti Pattern + +A table that mixes conventions from two types — for example, a "Ledger" +table with a user-editable primary key that lets users freely insert or +delete rows — is not "flexible", it is either misclassified or has skipped +a design step. A Ledger table's `Entry No.` must come only from the +posting routine; exposing it as an editable field breaks the type's core +guarantee that entries are an immutable, sequential audit trail. + +See sample: `table-design-must-match-bc-table-type-conventions.bad.al`. diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al new file mode 100644 index 00000000..fb822b3f --- /dev/null +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al @@ -0,0 +1,11 @@ +// Both fields guarded the same way, out of habit rather than analysis. +if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then + VATRegNo := SalesHeader."VAT Registration No."; // low blast radius - fine + +// but the same pattern, unexamined, was also applied here: +if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then + VATBusPostingGroup := SalesHeader."VAT Bus. Posting Group" +else + VATBusPostingGroup := ''; + // High blast radius: silently wrong VAT posting group reaches posting + // with no error, no TestField, and no reviewer in the loop. diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al new file mode 100644 index 00000000..38fbc887 --- /dev/null +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al @@ -0,0 +1,10 @@ +// Low blast radius: guard, with an explicit chosen fallback. +if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then + VATRegNo := SalesHeader."VAT Registration No."; +// Blank is an acceptable, deliberately-considered default here - the field +// is informational and a reviewer sees it before the document ships. + +// High blast radius: let it fail loud, because this feeds posted VAT. +SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo); +SalesHeader.TestField("VAT Bus. Posting Group"); +VATBusPostingGroup := SalesHeader."VAT Bus. Posting Group"; diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md new file mode 100644 index 00000000..fab6850d --- /dev/null +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [defensive-programming, offensive-programming, fail-fast, blast-radius, guarded-lookup] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Match defensive vs. offensive error handling to the blast radius of being wrong + +## Description + +Whether code should guard gracefully (defensive) or fail loudly (offensive/fail-fast) is not a matter of habit or a blanket house style — it depends on what happens downstream if the guarded condition is silently defaulted or skipped. Treating every missing value the same way, defensively or offensively, is itself the anti-pattern: uniform defensiveness hides the failures that matter most, while uniform fail-fast turns ordinary, expected absence into unnecessary crashes. Two fields can look structurally identical — both read from a related record, both potentially missing — and still deserve opposite treatment depending on what they feed. + +## Best Practice + +Trace what a silently-defaulted or skipped value actually reaches before deciding how to guard it. If it reaches a posted ledger amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output, code offensively: let the lookup fail loud (`TestField`, an unguarded `Get()` expected to always succeed, or an explicit `Error`) so a human sees the problem before anything posts. If it is cosmetic, informational, or easily corrected after the fact (a display field, an optional UI enhancement, a report not yet run), code defensively — but the fallback must be an explicit, deliberately-chosen, named business value, never a blank or zero that is merely the datatype default. When genuinely unsure which category a field falls into, that is a question to resolve explicitly with whoever owns the requirement, not a coin flip. + +See sample: `defensive-vs-offensive-code-must-match-blast-radius.good.al`. + +## Anti Pattern + +Guarding two fields the same way purely out of habit, without analyzing what each one feeds. A low-blast-radius field, such as a VAT registration number shown only on a printed document, and a high-blast-radius field, such as the VAT posting group that determines VAT actually applied to a posted transaction, are both wrapped in the same `if Header.Get(...) then ... else` pattern with a blank/zero fallback — leaving the posting-critical field free to post with a silently wrong value. + +See sample: `defensive-vs-offensive-code-must-match-blast-radius.bad.al`. diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.bad.al b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.bad.al new file mode 100644 index 00000000..6cc28767 --- /dev/null +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.bad.al @@ -0,0 +1,24 @@ +codeunit 50101 "Sample Web Service Caller" +{ + procedure CallExternalService() + var + ErrorLogEntry: Record "Sample Error Log"; + begin + // BUG: the log write happens inside the same transaction as the + // risky call, using the same Record instance as the caller. + if not TryCallService() then begin + ErrorLogEntry.Init(); + ErrorLogEntry."Error Message" := CopyStr(GetLastErrorText(), 1, 250); + ErrorLogEntry.Insert(); + Error(GetLastErrorText()); + // Error() above rolls back this transaction - including the + // Insert() just made. The failure is never actually logged. + end; + end; + + [TryFunction] + local procedure TryCallService() + begin + // ... external call that may fail ... + end; +} diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al new file mode 100644 index 00000000..34b22eca --- /dev/null +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al @@ -0,0 +1,26 @@ +codeunit 50100 "Sample Error Log Writer" +{ + // Started via Session.StartSession so its commit is independent of the + // caller's transaction. Does one thing: insert the log entry, commit. + trigger OnRun() + var + ErrorLogEntry: Record "Sample Error Log"; + begin + ErrorLogEntry.Init(); + ErrorLogEntry."Call Duration (ms)" := CallDurationMs; + ErrorLogEntry."Error Message" := + CopyStr(ErrorMessageText, 1, MaxStrLen(ErrorLogEntry."Error Message")); + ErrorLogEntry.Insert(true); + Commit(); + end; + + procedure SetParameters(Duration: Integer; ErrorText: Text) + begin + CallDurationMs := Duration; + ErrorMessageText := ErrorText; + end; + + var + CallDurationMs: Integer; + ErrorMessageText: Text; +} diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md new file mode 100644 index 00000000..6939d01c --- /dev/null +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: error-handling +keywords: [logging, rollback, session, transaction, isolated-session, telemetry] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Log writes that must capture failures must survive transaction rollback + +## Description + +Inserting a log record inside the same transaction as the operation it logs looks correct until the operation errors: the transaction rolls back and takes the log entry with it. The result is a log that faithfully records every success and silently loses exactly the failures it exists to capture. This is a common blind spot in error/duration logging around web-service calls, background jobs, and other operations expected to fail sometimes. + +## Best Practice + +Write any log whose purpose includes capturing failures from a transaction that is independent of the operation being logged: start an isolated session (`StartSession` on a codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. Capture duration and other telemetry values in the caller and pass them as parameters — the isolated session must not re-read state that a rollback may have erased. Logs that only record successful, committed work can safely stay in the main transaction; this pattern targets error and diagnostic logs specifically. + +See sample: `log-writes-must-survive-rollback.good.al`. + +## Anti Pattern + +Inserting the error-log record in the same transaction as the risky operation, so a rollback deletes the very entry meant to explain the failure. Adding a stray `Commit` before the risky call is not a fix either — it breaks the caller's atomicity and can violate posting-routine rules. Swallowing the error to keep the log alive (running a codeunit without checking or re-raising its result) is equally wrong: the log must observe the failure, not suppress it. + +See sample: `log-writes-must-survive-rollback.bad.al`. diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.bad.al b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.bad.al new file mode 100644 index 00000000..1a93861c --- /dev/null +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.bad.al @@ -0,0 +1,12 @@ +permissionset 50100 "Sample - Integration" +{ + Access = Public; + Assignable = false; + Caption = 'Sample Integration'; + Permissions = + tabledata "Sample Order" = RIMD; + // BUG: "Sample Order API" (PageType = API) and "Sample Order Query" + // (a published API query) have no "= X" entry anywhere in this app. + // Both endpoints are unreachable even though the table looks fully + // granted - nobody decided who may call them. +} diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.good.al b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.good.al new file mode 100644 index 00000000..de8d2abc --- /dev/null +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.good.al @@ -0,0 +1,10 @@ +permissionset 50100 "Sample - Integration" +{ + Access = Public; + Assignable = false; + Caption = 'Sample Integration'; + Permissions = + tabledata "Sample Order" = RIMD, + page "Sample Order API" = X, // exposed API page: execute granted + query "Sample Order Query" = X; // exposed API query: execute granted +} diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md new file mode 100644 index 00000000..75f13789 --- /dev/null +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: security +keywords: [permission-set, api-page, web-service, exposure, access-control] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Every exposed object must belong to a permission set + +## Description + +An object that is reachable from outside the app's own UI — an API page (`PageType = API`), a web-service-enabled page or query (`ServiceEnabled = true`), or a published API query — is only usable if it is also granted execute access through a permission set. When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing. + +## Best Practice + +Give every exposed object an explicit execute entry (`page "..." = X`, `query "..." = X`) in a permission set shipped by the app. Route sensitive endpoints into a dedicated, non-default admin permission set so reaching them requires a deliberate grant rather than being included by default. If an object should never be reachable from outside the app, remove the exposure itself (drop `PageType = API` / `ServiceEnabled`) rather than leaving an orphaned endpoint with no permission-set membership. + +See sample: `exposed-objects-must-be-in-a-permission-set.good.al`. + +## Anti Pattern + +Granting access to the underlying table data while forgetting to grant execute access to the exposed page or query itself. The table looks fully covered by a permission set, but the API/service layer in front of it has no `= X` entry anywhere, so the endpoint silently fails for every caller even though the data permissions look complete. + +See sample: `exposed-objects-must-be-in-a-permission-set.bad.al`. diff --git a/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.bad.al b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.bad.al new file mode 100644 index 00000000..869e398c --- /dev/null +++ b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.bad.al @@ -0,0 +1,18 @@ +codeunit 50101 "Credit Memo Routing" +{ + procedure PostSalesLine(var SalesHeader: Record "Sales Header"; var SalesLine: Record "Sales Line"; CustomerNo: Code[20]; Amount: Decimal) + begin + // Set the customer number + SalesHeader.Validate("Sell-to Customer No.", CustomerNo); + // Insert the line + SalesLine.Insert(true); + // Check if the amount is positive + if Amount > 0 then + // Post the entry + PostEntry(Amount); + end; + + local procedure PostEntry(Amount: Decimal) + begin + end; +} diff --git a/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.good.al b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.good.al new file mode 100644 index 00000000..6bd39b1d --- /dev/null +++ b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.good.al @@ -0,0 +1,20 @@ +codeunit 50101 "Credit Memo Routing" +{ + procedure PostSalesLine(var SalesHeader: Record "Sales Header"; var SalesLine: Record "Sales Line"; CustomerNo: Code[20]; Amount: Decimal) + begin + SalesHeader.Validate("Sell-to Customer No.", CustomerNo); + SalesLine.Insert(true); + + // Negative amounts arrive from credit memos routed through this + // codeunit; PostEntry() rejects them, so they're filtered here. + if Amount < 0 then + exit; + + if Amount > 0 then + PostEntry(Amount); + end; + + local procedure PostEntry(Amount: Decimal) + begin + end; +} diff --git a/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md new file mode 100644 index 00000000..2ff120a2 --- /dev/null +++ b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md @@ -0,0 +1,30 @@ +--- +bc-version: [all] +domain: style +keywords: [comments, verbosity, self-documenting, restate, tutorial-style] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Comments must not restate what the code already shows + +## Description + +A comment above nearly every statement that just narrates what the statement already says (`// Validate the customer number` above `SalesHeader.Validate("Sell-to Customer No.", CustomerNo)`) adds noise without adding information. Production AL — the Base Application, mature partner codebases — is comment-sparse by comparison: identifiers do the explaining, and a comment appears only when the code alone can't carry the reason. + +A comment earns its place only when it captures something the code cannot: a non-obvious business rule, a workaround for a specific platform limitation, or a constraint that would surprise the next reader. If removing the comment would leave the reader no worse off, the comment should not have been written. + +This does not override required structural documentation — feature/scenario test tags and XML-doc summaries on public library procedures remain required where they apply; those are structural markers, not narrative comments. + +## Best Practice + +Let the code speak for itself; reserve comments for the reason a reader could not otherwise infer. + +See sample: `al-comments-must-not-restate-what-code-already-shows.good.al`. + +## Anti Pattern + +A comment line before every statement, repeating in English what the statement's own identifiers already say. + +See sample: `al-comments-must-not-restate-what-code-already-shows.bad.al`. diff --git a/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al b/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al new file mode 100644 index 00000000..333625d4 --- /dev/null +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al @@ -0,0 +1,20 @@ +page 50100 "Sales Line Card" +{ + PageType = Card; + SourceTable = "Sales Line"; + + actions + { + area(Processing) + { + action(Recalculate) + { + trigger OnAction() + begin + Rec."Total Amount" := Rec.Quantity * Rec."Unit Price"; + Rec.Modify(); + end; + } + } + } +} diff --git a/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al b/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al new file mode 100644 index 00000000..a7a40b14 --- /dev/null +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al @@ -0,0 +1,31 @@ +codeunit 50100 "Sales Line Management" +{ + procedure RecalculateLine(var SalesLine: Record "Sales Line") + begin + SalesLine."Total Amount" := SalesLine.Quantity * SalesLine."Unit Price"; + SalesLine.Modify(); + end; +} + +page 50100 "Sales Line Card" +{ + PageType = Card; + SourceTable = "Sales Line"; + + actions + { + area(Processing) + { + action(Recalculate) + { + trigger OnAction() + begin + SalesLineMgt.RecalculateLine(Rec); + end; + } + } + } + + var + SalesLineMgt: Codeunit "Sales Line Management"; +} diff --git a/microsoft/knowledge/style/pages-must-not-contain-business-logic.md b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md new file mode 100644 index 00000000..d113a793 --- /dev/null +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md @@ -0,0 +1,31 @@ +--- +bc-version: [all] +domain: style +keywords: [pages, business-logic, codeunit, separation-of-concerns, presentation-layer] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep business logic out of page objects + +## Description + +A page procedure that calculates a value and assigns it to a field, calls `Rec.Modify()` directly, or implements a business rule is an architecture violation even when it compiles. Pages are a presentation layer: they bind data to the UI and invoke actions. Calculations, validations, and record mutations belong in codeunits, where they can be tested, reused, and called consistently regardless of which page (or API, or batch job) triggers them. When logic lives on a page, it only applies when a user opens that specific page — the same business rule silently doesn't run through any other entry point. + +A narrow set of patterns are conventional rather than violations: +- A setup page reading and writing its own singleton setup record. +- A dedicated "Run Conversion" page invoking a conversion codeunit directly. +- The standard singleton-initialization idiom on `OnOpenPage` (`if not Rec.Get() then begin Rec.Init(); Rec.Insert(); end`) used by cue/activities pages to bootstrap their own presentation-state record — this is not business logic, it is the same pattern used throughout base-app cue pages. + +## Best Practice + +Delegate all business operations to a codeunit: the page owns presentation, the codeunit owns logic. A calculation or validation triggered from a page action should call a codeunit procedure rather than compute the result inline. + +See sample: `pages-must-not-contain-business-logic.good.al`. + +## Anti Pattern + +Direct calculations in a page trigger (e.g. `Rec."Total Amount" := Rec.Quantity * Rec."Unit Price"`), calls to `Rec.Modify()` from a page trigger, or business-rule validation embedded in `OnValidate`/`OnAction` instead of routed through a codeunit. + +See sample: `pages-must-not-contain-business-logic.bad.al`. diff --git a/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md new file mode 100644 index 00000000..eb1e5745 --- /dev/null +++ b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md @@ -0,0 +1,36 @@ +--- +bc-version: [all] +domain: style +keywords: [folder-structure, feature-organization, source-layout, maintainability] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Organize AL source by business feature, not object type + +## Description + +Source folders inside an AL app should group files by the business feature or module they belong to (`src/Sales/Invoice/`, `src/NoSeries/`), not by which kind of AL object they are (`src/Tables/`, `src/Pages/`, `src/Codeunits/`). Object-type folders scatter everything belonging to one feature across half a dozen directories, so a developer picking up a feature has to jump between folders that share nothing but object type to see the whole picture. Feature folders keep a table, its pages, its codeunits, and its test setup physically together. + +Code genuinely shared across multiple features (utility codeunits, common interfaces, shared enums) belongs in a `Common` or `Shared` folder, not duplicated per feature and not left in a catch-all root. + +## Best Practice + + src/ + ├── NoSeries/ + ├── Sales/ + │ ├── Invoice/ + │ └── Order/ + └── Common/ + +Each feature folder holds every object type it needs; shared code has one dedicated home. + +## Anti Pattern + + src/ + ├── Tables/ + ├── Pages/ + └── Codeunits/ + +Finding everything related to one feature now requires searching multiple folders and mentally reassembling it from scattered pieces. diff --git a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.bad.al b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.bad.al new file mode 100644 index 00000000..d6a29d6f --- /dev/null +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.bad.al @@ -0,0 +1,29 @@ +// Only wraps Microsoft's own generic scenario — measures BC, not this extension +codeunit 50101 "BCPT Create Sales Order" implements "BCPT Test Param. Provider" +{ + SingleInstance = true; + + trigger OnRun() + begin + CreateStandardSalesOrder(GlobalBCPTTestContext); + end; + + var + GlobalBCPTTestContext: Codeunit "BCPT Test Context"; + + local procedure CreateStandardSalesOrder(var BCPTTestContext: Codeunit "BCPT Test Context") + begin + BCPTTestContext.StartScenario('Create Sales Order With N Lines'); + // ... standard sales order creation, no reference to the extension's own logic + BCPTTestContext.EndScenario('Create Sales Order With N Lines'); + end; + + procedure GetDefaultParameters(): Text[1000] + begin + exit(''); + end; + + procedure ValidateParameters(Parameters: Text[1000]) + begin + end; +} diff --git a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al new file mode 100644 index 00000000..0ca29de1 --- /dev/null +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al @@ -0,0 +1,43 @@ +codeunit 50100 "BCPT Create Service Request" implements "BCPT Test Param. Provider" +{ + SingleInstance = true; + + trigger OnRun() + begin + if not IsInitialized then begin + InitTest(); + IsInitialized := true; + end; + CreateServiceRequest(GlobalBCPTTestContext); + end; + + var + GlobalBCPTTestContext: Codeunit "BCPT Test Context"; + IsInitialized: Boolean; + + local procedure InitTest() + begin + // Set up any required configuration + end; + + local procedure CreateServiceRequest(var BCPTTestContext: Codeunit "BCPT Test Context") + begin + BCPTTestContext.StartScenario('Create Service Request Header'); + // ... create the service request + BCPTTestContext.EndScenario('Create Service Request Header'); + BCPTTestContext.UserWait(); + + BCPTTestContext.StartScenario('Add Service Request Line'); + // ... add a line + BCPTTestContext.EndScenario('Add Service Request Line'); + end; + + procedure GetDefaultParameters(): Text[1000] + begin + exit(''); + end; + + procedure ValidateParameters(Parameters: Text[1000]) + begin + end; +} diff --git a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md new file mode 100644 index 00000000..8dae0b70 --- /dev/null +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [bcpt, performance-test, scenarios, app-specific, regression] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Include app-specific scenarios in a PerformanceTest app's BCPT suite + +## Description + +A PerformanceTest app that ships with only the generic Microsoft BCPT samples (creating sales orders, purchase orders, posting item journals) measures Business Central's own baseline performance, not the extension it was built to test. Those samples are starting points, not coverage. Without a scenario that exercises the extension's own business flow — its own codeunits, its own FlowFields, its own page rendering — a performance regression introduced by the extension has no test that would ever detect it. + +## Best Practice + +For every major business flow the extension adds, create a matching `BCPT*` scenario codeunit: `SingleInstance = true`, implementing `"BCPT Test Param. Provider"`, wrapping the operation under test in `BCPTTestContext.StartScenario()` / `EndScenario()`, and building its own test data in a local `InitTest()` procedure rather than depending on hardcoded records. Give each distinct step its own named scenario so a regression in one step doesn't hide inside a coarser measurement. + +See sample: `bcpt-scenarios-must-be-app-specific.good.al`. + +## Anti Pattern + +A PerformanceTest app whose only scenario codeunits are copies of Microsoft's shipped samples (creating a standard sales order, opening the standard customer list) tests the platform, not the extension. Any regression in the extension's own posting logic, calculations, or pages goes unmeasured and unnoticed. + +See sample: `bcpt-scenarios-must-be-app-specific.bad.al`. diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al new file mode 100644 index 00000000..4357b409 --- /dev/null +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al @@ -0,0 +1,12 @@ +[Test] +procedure PostSalesOrder_CreatesInvoice() +var + SalesHeader: Record "Sales Header"; +begin + // [GIVEN] a sales order — posting groups left to whatever exists in the test company + LibrarySales.CreateSalesOrder(SalesHeader); + // [WHEN] + LibrarySales.PostSalesOrder(SalesHeader, false, true); + // [THEN] + Assert.RecordIsNotEmpty(SalesInvoiceHeader); +end; diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al new file mode 100644 index 00000000..1a509fc7 --- /dev/null +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al @@ -0,0 +1,15 @@ +[Test] +procedure PostSalesOrder_CreatesInvoice() +var + Customer: Record Customer; + SalesHeader: Record "Sales Header"; +begin + // [GIVEN] a customer with a full posting-group chain and VAT setup + LibrarySales.CreateCustomerWithPostingSetup(Customer); + // [GIVEN] a sales order for that customer, dated explicitly + LibrarySales.CreateSalesOrderForCustomer(SalesHeader, Customer."No.", WorkDate()); + // [WHEN] + LibrarySales.PostSalesOrder(SalesHeader, false, true); + // [THEN] + Assert.RecordIsNotEmpty(SalesInvoiceHeader); +end; diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md new file mode 100644 index 00000000..71e63e59 --- /dev/null +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [given, test-setup, posting, report, request-page, precondition, completeness] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Cover the full precondition chain in GIVEN, not just the primary record + +## Description + +A `[GIVEN]` block is only correct if it sets up every precondition the code under test actually reads, not just the record the scenario is "about." For most master-data tests, creating the primary record is enough. For posting routines and reports it usually is not: an incomplete `[GIVEN]` produces a test that either fails with a setup error unrelated to the scenario, or worse, passes without ever reaching the logic it claims to verify. + +## Best Practice + +For a posting test, set up the full posting-group chain the document requires (e.g. customer/vendor posting group, gen. business/product posting group, VAT posting setup), the setup records the specific posting path reads, and an explicit date when the path is date-sensitive — a missing link surfaces as an unrelated G/L error, not a meaningful test failure. For a report test that claims to verify filtering or dataset logic, include both a record that should be included and one that should be excluded, plus any request-page parameter or FlowField the report's logic branches on. A report test that only claims to run without error is exempt from the include/exclude pairing, but it must say so in its scenario name or comment — an unlabelled single-record `[GIVEN]` is ambiguous about which claim it is making, and that ambiguity is itself the defect. + +See sample: `given-blocks-must-cover-full-precondition-chain.good.al`. + +## Anti Pattern + +A posting test whose `[GIVEN]` creates only the sales header, relying on whatever posting groups happen to exist in the test company. A report test whose `[GIVEN]` creates only matching records, so the report "passes" whether or not its filter logic does anything at all. + +See sample: `given-blocks-must-cover-full-precondition-chain.bad.al`. diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al b/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al new file mode 100644 index 00000000..fa9d2c01 --- /dev/null +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al @@ -0,0 +1,21 @@ +codeunit 50102 "Item Price Testing" +{ + Subtype = Test; + + var + ItemPriceMgt: Codeunit "Item Price Mgt."; + Assert: Codeunit "Library Assert"; + + [Test] + procedure Test1() + var + Customer: Record Customer; + Item: Record Item; + Price, Disc: Decimal; + begin + // setup mixed with assertions, no clear layers + Customer.Insert(false); + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', Price, Disc); + Assert.AreEqual(100, Price, ''); + end; +} diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al new file mode 100644 index 00000000..6866403a --- /dev/null +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al @@ -0,0 +1,26 @@ +// [FEATURE] Item Price — price cascade (Customer -> Price Group -> All Customers) +codeunit 50103 "Item Price Testing" +{ + Subtype = Test; + + var + LibrarySales: Codeunit "Library - Sales"; + ItemPriceMgt: Codeunit "Item Price Mgt."; + Assert: Codeunit "Library Assert"; + + // [SCENARIO] Customer with a specific price list line gets that unit price + [Test] + procedure GetPrice_CustomerPrice_ReturnsUnitPrice() + var + Customer: Record Customer; + Item: Record Item; + UnitPrice, LineDiscPct: Decimal; + begin + // [GIVEN] a customer with a price list line at 100 LCY + LibrarySales.CreateCustomerWithPrice(Customer, Item, '', 100); + // [WHEN] + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + // [THEN] + Assert.AreEqual(100, UnitPrice, 'Unit price must match customer price list'); + end; +} diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.md b/microsoft/knowledge/testing/test-feature-scenario-tags.md new file mode 100644 index 00000000..5842beee --- /dev/null +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [feature, scenario, given, when, then, tags, bdd, atdd, comments] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Tag test codeunits with FEATURE, SCENARIO, GIVEN, WHEN, and THEN comments + +## Description + +Test codeunits are easier to trust and to review when they carry a four-level comment structure taken from Behaviour-/Acceptance-Test-Driven Development: `[FEATURE]` once at the top of the codeunit naming the functional area under test, `[SCENARIO]` above each test procedure stating one falsifiable business claim in plain language, and `[GIVEN]`/`[WHEN]`/`[THEN]` marking the precondition, action, and assertion inside the test body. Without these tags a test procedure is an opaque block of AL that only reveals its intent by being read line by line; a reviewer or product owner cannot scan a codeunit and know what business behaviour it covers. + +## Best Practice + +Put `[FEATURE]` as a comment before the codeunit's opening brace, naming the domain rather than the object. Put `[SCENARIO]` immediately above each `[Test]` attribute, describing the scenario in business language that complements — not duplicates — the procedure name. Inside the body, mark the precondition setup as `[GIVEN]`, the single action under test as `[WHEN]`, and the assertions as `[THEN]`. The procedure name stays the machine-readable identity shown in test-runner output; the `[SCENARIO]` comment stays the human-readable one. Neither replaces the other. + +See sample: `test-feature-scenario-tags.good.al`. + +## Anti Pattern + +A test procedure with no `[FEATURE]`/`[SCENARIO]`/`[GIVEN]`/`[WHEN]`/`[THEN]` structure, setup mixed freely with assertions, and a procedure name like `Test1` that says nothing about what is being verified. Nothing in the codeunit tells a reader what business rule it exists to protect. + +See sample: `test-feature-scenario-tags.bad.al`. diff --git a/microsoft/knowledge/testing/test-one-when-per-test.bad.al b/microsoft/knowledge/testing/test-one-when-per-test.bad.al new file mode 100644 index 00000000..d24da5db --- /dev/null +++ b/microsoft/knowledge/testing/test-one-when-per-test.bad.al @@ -0,0 +1,15 @@ +[Test] +procedure GetPrice_ThenGetDiscount_ReturnsCorrectValues() +var + TempBuffer: Record "Item Price Tier Buffer" temporary; + UnitPrice, LineDiscPct: Decimal; +begin + // [GIVEN] ... + // [WHEN] first action + ItemPriceMgt.GetSalesPrice(CustomerNo, ItemNo, '', UnitPrice, LineDiscPct); + // [WHEN] second action — this is a second test in disguise + ItemPriceMgt.GetSalesPriceTiers(CustomerNo, ItemNo, '', TempBuffer); + // [THEN] asserting two unrelated things + Assert.AreEqual(100, UnitPrice, ''); + Assert.IsFalse(TempBuffer.IsEmpty(), ''); +end; diff --git a/microsoft/knowledge/testing/test-one-when-per-test.good.al b/microsoft/knowledge/testing/test-one-when-per-test.good.al new file mode 100644 index 00000000..320bffde --- /dev/null +++ b/microsoft/knowledge/testing/test-one-when-per-test.good.al @@ -0,0 +1,29 @@ +[Test] +procedure GetPrice_CustomerPrice_ReturnsCorrectUnitPrice() +var + Customer: Record Customer; + Item: Record Item; + UnitPrice, LineDiscPct: Decimal; +begin + // [GIVEN] a customer with a price list line at 100 + LibrarySales.CreateCustomerWithPrice(Customer, Item, '', 100); + // [WHEN] + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + // [THEN] + Assert.AreEqual(100, UnitPrice, 'Unit price must match price list'); +end; + +[Test] +procedure GetPriceTiers_CustomerTier_ReturnsOneTierLine() +var + Customer: Record Customer; + Item: Record Item; + TempBuffer: Record "Item Price Tier Buffer" temporary; +begin + // [GIVEN] a customer with a tier price at min qty 10 + LibrarySales.CreateCustomerWithTierPrice(Customer, Item, '', 10, 90); + // [WHEN] + ItemPriceMgt.GetSalesPriceTiers(Customer."No.", Item."No.", '', TempBuffer); + // [THEN] + Assert.AreEqual(1, TempBuffer.Count(), 'Exactly one tier line expected'); +end; diff --git a/microsoft/knowledge/testing/test-one-when-per-test.md b/microsoft/knowledge/testing/test-one-when-per-test.md new file mode 100644 index 00000000..8aa6ca8b --- /dev/null +++ b/microsoft/knowledge/testing/test-one-when-per-test.md @@ -0,0 +1,34 @@ +--- +bc-version: [all] +domain: testing +keywords: [when, single-action, bdd, atdd, given-when-then, flow-test, regression-test] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep exactly one WHEN per test, with narrow exceptions for flow and defect-then-fix tests + +## Description + +Each test procedure should contain exactly one `[WHEN]` block: one action that triggers the behaviour under test. A test with multiple WHENs — "do A, then do B, then check C" — is two or more tests in disguise. Splitting them gives failure isolation (a failing test points at one action, not an ambiguous sequence) and keeps each test readable as a single, falsifiable claim. A precondition action, such as posting a document so a ledger entry exists to assert against, belongs in `[GIVEN]`; only the action actually being asserted belongs in `[WHEN]`. + +## Best Practice + +Give each test one `[WHEN]` and one focused claim. A procedure name containing "And" or "Then" in the middle (`GetPrice_AndDiscount_ReturnsValues`) is a strong signal the test should be split. + +See sample: `test-one-when-per-test.good.al`. + +## Anti Pattern + +A test that performs a first action, then a second unrelated action, then asserts on both — mixing two falsifiable claims into one procedure so a failure can't tell you which action broke. + +See sample: `test-one-when-per-test.bad.al`. + +## Flow tests — a deliberate exception + +A flow test verifies the accumulated outcome of a genuinely multi-round business process (partial receipt then invoicing, several posting rounds against one document), where the sequence itself is the scenario — splitting it would lose the interaction under test. Multiple `[WHEN]` blocks are allowed only when the procedure name declares the flow, each `[WHEN]` is labelled as one round of a single scenario rather than an unrelated action, and the `[THEN]` asserts the accumulated end-state rather than assertions that decompose cleanly per action (if they do decompose cleanly, it is still two tests in disguise). Outside this shape, unit-level tests keep the strict one-WHEN rule. + +## Defect-then-fix tests — a second, narrower exception + +A test that reproduces a specific broken state and then verifies a subsequent action corrects it is not the same shape as an unrelated-action test, even though its `[THEN]` assertions decompose cleanly per step — clean decomposition is expected here, not a sign of two unrelated tests. This shape is permitted only when the second `[WHEN]` cannot be meaningfully tested without the first (the fix only affects the exact stale state the first action produced, so splitting would just re-run the first action inside a second test's `[GIVEN]`), and the procedure name communicates the before/after relationship. diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al b/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al new file mode 100644 index 00000000..98fc2b26 --- /dev/null +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al @@ -0,0 +1,24 @@ +codeunit 50104 "Item Price Testing" +{ + Subtype = Test; + + [Test] + procedure GetPrice_LogicTest() + var + Customer: Record Customer; + Item: Record Item; + UnitPrice, LineDiscPct: Decimal; + begin + // logic test — fine on its own, but not paired with a UI test below + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + end; + + [Test] + procedure Page_ShowsPrice_UT() + var + ItemPricePage: TestPage "Item Price"; + begin + // UI test mixed into a logic-test codeunit, and the codeunit lacks the _UT suffix + ItemPricePage.OpenNew(); + end; +} diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al b/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al new file mode 100644 index 00000000..5f216b0f --- /dev/null +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al @@ -0,0 +1,33 @@ +codeunit 50105 "Item Price Testing" +{ + Subtype = Test; + + [Test] + procedure GetPrice_CustomerPrice_ReturnsUnitPrice() + var + Customer: Record Customer; + Item: Record Item; + UnitPrice, LineDiscPct: Decimal; + begin + ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + end; +} + +codeunit 50106 "Item Price Testing_UT" +{ + Subtype = Test; + + [Test] + procedure Page_EnterCustomerAndItem_FactBoxShowsPrice() + var + Customer: Record Customer; + Item: Record Item; + ItemPricePage: TestPage "Item Price"; + Assert: Codeunit "Library Assert"; + begin + ItemPricePage.OpenNew(); + ItemPricePage.CustomerNo.SetValue(Customer."No."); + ItemPricePage.ItemNo.SetValue(Item."No."); + Assert.AreEqual('100.00', ItemPricePage.PriceInfo.UnitPrice.Value(), ''); + end; +} diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.md b/microsoft/knowledge/testing/ui-test-codeunit-naming.md new file mode 100644 index 00000000..4166cd74 --- /dev/null +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: testing +keywords: [ui-test, testpage, naming, suffix, codeunit, page-testing] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Suffix UI-layer test codeunits with _UT and never mix layers in one codeunit + +## Description + +A test codeunit that drives pages through `TestPage` — opening pages, reading FactBox parts, triggering field `OnValidate` through the page — is testing a different layer than a codeunit that calls business-logic procedures directly. Readers need to know which layer a given test exercises without opening it, and a single codeunit that mixes both kinds of test hides that distinction: a failure could mean the logic broke, the page broke, or both. + +## Best Practice + +Give any test codeunit that uses `TestPage` a `_UT` (Unit Test — UI layer) suffix, and keep it free of tests that call codeunit/table procedures directly. Keep the corresponding logic-only codeunit unsuffixed. Allocate the two codeunits adjacent object IDs so their relationship is visible in the object list. + +See sample: `ui-test-codeunit-naming.good.al`. + +## Anti Pattern + +A codeunit named without the `_UT` suffix that nonetheless contains `TestPage` calls, or — worse — one codeunit that mixes a direct logic-call test and a `TestPage`-driven test side by side. Either way, the codeunit's name no longer tells a reader which layer a failing test actually broke. + +See sample: `ui-test-codeunit-naming.bad.al`. diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.bad.al b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.bad.al new file mode 100644 index 00000000..fd371f5c --- /dev/null +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.bad.al @@ -0,0 +1,21 @@ +page 50131 "Sample Item List" +{ + PageType = List; + SourceTable = "Sample Item"; + // Anti-pattern: no CardPageID even though a Card page exists for + // this table, and no UsageCategory, so the page is invisible to + // Tell Me search. + ApplicationArea = All; + + layout + { + area(content) + { + repeater(Group) + { + field(Description; Rec.Description) { } + field("No."; Rec."No.") { } // primary key buried, not left-most + } + } + } +} diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.good.al b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.good.al new file mode 100644 index 00000000..7b53ac0e --- /dev/null +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.good.al @@ -0,0 +1,20 @@ +page 50130 "Sample Item List" +{ + PageType = List; + SourceTable = "Sample Item"; + CardPageID = "Sample Item Card"; // links back to its Card page + UsageCategory = Lists; + ApplicationArea = All; + + layout + { + area(content) + { + repeater(Group) + { + field("No."; Rec."No.") { } // primary key, left-most + field(Description; Rec.Description) { } + } + } + } +} diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md new file mode 100644 index 00000000..5f925d29 --- /dev/null +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md @@ -0,0 +1,76 @@ +--- +bc-version: [all] +domain: ui +keywords: [pages, page-design, naming-conventions, page-type, card-page, list-page, factbox, worksheet-page, document-page, rolecenter, cardpageid, autosplitkey] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Pages must match one of Business Central's page-type conventions + +## Description + +Business Central's page types — RoleCenter, Card, List, CardPart, +ListPart, Worksheet, Document, ListPlus, plus the system dialog types +(Navigate, ConfirmationDialog, StandardDialog, HeadlinePart, API) — each +fix a naming pattern and a structural constraint, not just a visual +layout. A page whose name, primary-key handling, or linkage +(`CardPageID`, `SubPageLink`, `AutoSplitKey`) doesn't match its own type's +conventions is either the wrong page type for the job or built +inconsistently with the rest of the application, and should be flagged in +review even if it compiles and renders. Before naming a new page or +wiring its links, first ask which page type it is, and whether the source +table actually fits that type's structural requirement — the type fixes +the naming suffix, which fields are visible, and which other page it must +link back to. + +## Best Practice + +Match the page's design to its type: + +- **RoleCenter** — tailored home page for a role; named role + `Role + Center`; links to List pages, shows Cues/Activities. +- **Card** — view/edit one record; named table + `Card`; FastTabs only, + first FastTab named `General`. Requires a single-field primary key — a + multi-field key needs a List/Worksheet/Tabular page instead. +- **List** — view multiple records, also the lookup/drilldown surface; + named table + `List` if read-only, or the plural table name if + editable; primary-key fields shown left-most; `CardPageID` must point + at the associated Card page when one exists. +- **CardPart** — single-column FactBox; named for its content + + `FactBox`. +- **ListPart** — multi-column FactBox or subpage (e.g. document lines); + named for its content + `FactBox`/`SubPage`; `SubPageLink` must + actually filter to the host record. +- **Worksheet** — multi-record entry for a Journal-like table, insertion + order preserved; primary-key fields never shown; uses `AutoSplitKey` + with a trailing `Integer` key field. +- **Document** — FastTabs plus a lines subpage, lines filtered to the + header; named for the document (`Sales Invoice`). +- **ListPlus** — like Document but with multiple lists instead of one; + named like the record/report it summarizes. +- System dialog types (`Navigate`, `ConfirmationDialog`, + `StandardDialog`, `HeadlinePart`) are fixed shapes with no page-name + suffix convention. `API` pages follow their own property rules and are + extended by adding a new API page, never a page extension. + +Before wiring controls, the design step should fix: which users and +tasks the page serves, the concrete fields/commands/links those tasks +need, the page type that matches the content (chosen before the source +table), and the source table that actually holds the page's primary data. + +See sample: `page-design-must-match-bc-page-type-conventions.good.al`. + +## Anti Pattern + +A page that mixes conventions from two types — for example, a "Card" +page built on a table with a two-field primary key, or a "List" page +with no `CardPageID` even though a Card page exists for the same table — +signals a design step was skipped, not a stylistic choice. Also watch +for: a Worksheet or List page showing primary-key fields it shouldn't (or +hiding them when it should show them), and a page with no +`UsageCategory` set, which makes it invisible to Tell Me search even +though it otherwise works. + +See sample: `page-design-must-match-bc-page-type-conventions.bad.al`. diff --git a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al new file mode 100644 index 00000000..c6eaebad --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.bad.al @@ -0,0 +1,14 @@ +local procedure UpgradeCustomerDiscountField() +begin + if not UpgradeTag.HasUpgradeTag(GetCustomerDiscountFieldTag()) then begin + if Customer.FindSet() then begin + repeat + if Customer."Discount %" = 0 then begin + if Customer."Customer Posting Group" <> '' then + Customer."Discount %" := 5; + end; + until Customer.Next() = 0; + end; + UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag()); + end; +end; diff --git a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al new file mode 100644 index 00000000..a6f09a4c --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.good.al @@ -0,0 +1,14 @@ +local procedure UpgradeCustomerDiscountField() +begin + if UpgradeTag.HasUpgradeTag(GetCustomerDiscountFieldTag()) then + exit; + + Customer.SetLoadFields("Discount %"); + if Customer.FindSet() then + repeat + Customer."Discount %" := 5; + Customer.Modify(); + until Customer.Next() = 0; + + UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag()); +end; diff --git a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md new file mode 100644 index 00000000..3bd7e972 --- /dev/null +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md @@ -0,0 +1,28 @@ +--- +bc-version: [all] +domain: upgrade +keywords: [upgrade-tag, nesting, complexity, upgrade-per-company, upgrade-per-database] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Keep upgrade tag conditional logic to at most two levels of nesting + +## 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 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. + +## Best Practice + +One tag check, one exit, one upgrade action — two levels deep at most. + +See sample: `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. + +See sample: `upgrade-tag-logic-must-not-nest-deeply.bad.al`. diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al new file mode 100644 index 00000000..272fdf64 --- /dev/null +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.bad.al @@ -0,0 +1,23 @@ +page 50100 "Vendor Document API" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'documents'; + APIVersion = 'v1.0'; + SourceTable = Vendor; + // no InsertAllowed/ModifyAllowed override, no Editable = false anywhere + + layout + { + area(content) + { + repeater(GroupName) + { + field(no; Rec."No.") { } + field(vatRegNo; Rec."VAT Registration No.") { } + field(contactEmail; Rec."E-Mail") { } + // ...dozens more fields, none marked Editable = false + } + } + } +} diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al new file mode 100644 index 00000000..35af72be --- /dev/null +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al @@ -0,0 +1,21 @@ +page 50102 "Vendor Contact Info API" +{ + PageType = API; + APIPublisher = 'contoso'; + APIGroup = 'integration'; + APIVersion = 'v1.0'; + SourceTable = Vendor; + DelayedInsert = true; + + layout + { + area(content) + { + repeater(GroupName) + { + field(no; Rec."No.") { Editable = false; } + field(contactEmail; Rec."E-Mail") { } + } + } + } +} diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md new file mode 100644 index 00000000..5289fad8 --- /dev/null +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md @@ -0,0 +1,26 @@ +--- +bc-version: [all] +domain: web-services +keywords: [api-page, least-privilege, write-access, odata, security, external-api, identity-fields] +technologies: [al] +countries: [w1] +application-area: [all] +--- + +# Give API pages least-privilege write access + +## Description + +A general-purpose API page that exposes many fields should not be widened to allow writes on one additional field. A `PageType = API` page consumed by an external integration, an automation agent, or a partner system carries the same risk regardless of caller: a write-enabled page with no per-field restriction is a wide-open surface. The most common real-world shape of this problem is not a page that started narrow and got widened — it is a page that was never restricted at all: with `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` left at their defaults and no `Editable = false` on any field, every field on the source table — including identity fields and financially significant ones — is fully writable, with nothing marking that as deliberate. + +## Best Practice + +Create a separate, minimal API page that exposes only the key and the specific field the consumer needs to write, with everything else `Editable = false` or simply absent from the page. + +See sample: `api-page-least-privilege-write-access.good.al`. + +## Anti Pattern + +Widening an existing general-purpose API page with write access to one field, leaving every other field on the page (including identity and posting fields) writable by default because no one added `Editable = false`. + +See sample: `api-page-least-privilege-write-access.bad.al`. From 81845a57db435a1aeeefdde21282357754883873 Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Mon, 7 Sep 2026 21:02:51 +0200 Subject: [PATCH 2/3] Address Jesper Schulz-Wedde's review on PR #157 - release-must-update-app-version.md: reframe around AppSource's actual strict full-version-ordering requirement; scope branching-policy claims as team convention, not platform rule. - pictures-must-use-media-not-blob.md: MediaSet is a collection of independent media objects, not automatic image variants/thumbnails. - log-writes-must-survive-rollback.{md,good.al}: StartSession's only data channel into the new session is its Record parameter to a TableNo-scoped codeunit; a setter called on a local instance before starting the session populates nothing in the new session. - exposed-objects-must-be-in-a-permission-set.md: correct the three exposure mechanisms (Web Services config, PageType/QueryType=API, ServiceEnabled as a method-only attribute). - pages-must-not-contain-business-logic.md: scope to persisted mutations and cross-entry-point rules; presentation-only calculations and table-owned invariants are not violations. - given-blocks-must-cover-full-precondition-chain.good.al: replace invented LibrarySales calls with the real API (CreateCustomer/CreateSalesOrderForCustomerNo/PostSalesDocument). - test-feature-scenario-tags.{md,good.al}: move [SCENARIO] inside the test procedure body to match the current BCApps corpus; keep [FEATURE] at codeunit level per Microsoft's own documented option. - ui-test-codeunit-naming.md: scope the _UT suffix and adjacent-ID pairing as an explicit team convention, not a BCApps-wide standard. - page-design-must-match-bc-page-type-conventions.md / table-design-must-match-bc-table-type-conventions.md: Card's single-key primary-key claim is a contextual heuristic, not a mandatory constraint (Ship-to Address, Customer/Vendor Bank Account are real composite-key Card pages); a Subsidiary table with its own identity commonly gets List+Card, not Worksheet/Tabular. - api-page-least-privilege-write-access.{md,good.al}: only page-placed fields are ever exposed; set InsertAllowed/DeleteAllowed=false in the good sample so a narrow field set can't still create/delete records. - source-organized-by-feature-not-object-type.md, test-one-when-per-test.md: scope as team/testing-design conventions, not Microsoft platform requirements. - upgrade-tag-logic-must-not-nest-deeply.md: add the Microsoft Learn citation that already backs the two-level nesting limit. - Wire the new articles into the testing/data-modeling/error-handling/ security/ui review skills' candidate-selection signals. Co-Authored-By: Claude Sonnet 5 --- .../release-must-update-app-version.md | 6 +-- .../pictures-must-use-media-not-blob.md | 27 +++++++----- ...gn-must-match-bc-table-type-conventions.md | 7 ++- .../log-writes-must-survive-rollback.good.al | 43 +++++++++++++------ .../log-writes-must-survive-rollback.md | 2 +- ...sed-objects-must-be-in-a-permission-set.md | 2 +- .../pages-must-not-contain-business-logic.md | 4 +- ...ce-organized-by-feature-not-object-type.md | 2 +- ...must-cover-full-precondition-chain.good.al | 15 ++++--- .../test-feature-scenario-tags.good.al | 2 +- .../testing/test-feature-scenario-tags.md | 2 +- .../testing/test-one-when-per-test.md | 2 +- .../testing/ui-test-codeunit-naming.md | 10 ++--- ...ign-must-match-bc-page-type-conventions.md | 19 +++++--- .../upgrade-tag-logic-must-not-nest-deeply.md | 4 ++ ...-page-least-privilege-write-access.good.al | 2 + .../api-page-least-privilege-write-access.md | 4 +- .../skills/review/al-data-modeling-review.md | 2 + .../skills/review/al-error-handling-review.md | 1 + microsoft/skills/review/al-security-review.md | 2 +- microsoft/skills/review/al-testing-review.md | 3 ++ microsoft/skills/review/al-ui-review.md | 2 +- 22 files changed, 108 insertions(+), 55 deletions(-) diff --git a/microsoft/knowledge/appsource/release-must-update-app-version.md b/microsoft/knowledge/appsource/release-must-update-app-version.md index 91fb0938..d7debe8f 100644 --- a/microsoft/knowledge/appsource/release-must-update-app-version.md +++ b/microsoft/knowledge/appsource/release-must-update-app-version.md @@ -19,16 +19,16 @@ At every release — a branch merged to `main`, a tagged release build, or an Ap | Minor | Developer decision | Every release with new functionality | | Build / Revision | AL-Go pipeline | Automatic — never hand-edited | -The version number is the only identity a deployed app has. Two customer environments running "the same" version with different code is an undiagnosable support case; an AppSource submission with an unchanged major.minor is a rejected submission. AL-Go increments build numbers on every CI run, which creates the illusion that versioning is handled — but major.minor is a human statement about compatibility, and no pipeline can make it. +The version number is the only identity a deployed app has. Two customer environments running "the same" version with different code is an undiagnosable support case. AppSource's actual requirement is strict full-version ordering — the complete version must be greater than the previously submitted version — which an AL-Go-generated build/revision increment can satisfy on its own; AppSource does not require major.minor itself to change. Treating major.minor as a deliberate, human-decided compatibility signal is still valuable practice — it is a statement about what changed that no pipeline can make on its own — just not a platform-enforced requirement. ## Best Practice Before the release merge: - app.json: "version": "1.3.0.0" (new functionality -> minor bump) + app.json: "version": "1.3.0.0" (new functionality -> minor bump, by team convention) AL-Go settings: "repoVersion": "1.3" (where used) Then: feature branch -> main via PR, tag, release. -Feature branches never touch the version; only the release does. +"Feature branches never touch the version" and "every merge to main is a release" are workflow choices your team can adopt for compatibility clarity — not something AppSource itself requires. ## Anti Pattern diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md index 3786d471..195c4a8f 100644 --- a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md @@ -14,13 +14,15 @@ application-area: [all] `BLOB` is still a valid AL field type for arbitrary binary data, but it is not the right choice for storing pictures or images. The current recommendation is the `Media` field type for a single image, or -`MediaSet` when a record needs several image variants (e.g. multiple -sizes). Media/MediaSet integrate with the platform's picture control, the -media repository, image caching, and thumbnail generation — none of which -a plain `BLOB` field gets. A `BLOB` field storing a picture works, but it -is the legacy approach: no caching, no thumbnail support, and no -integration with the standard picture controls used across Business -Central pages. +`MediaSet` when a record needs several independent images (e.g. multiple +product photos) — `MediaSet` is a collection of separately-imported media +objects, each with its own identity; it does not generate resized variants +or thumbnails on its own, and displaying more than one item still requires +custom page handling. Media/MediaSet integrate with the platform's +picture control and media repository, which a plain `BLOB` field does not +— but any derived preview or thumbnail image still has to be generated +explicitly and stored in its own field, regardless of which type holds the +source image. `BLOB` remains the correct choice for genuinely arbitrary binary payloads that are not images and don't benefit from the media pipeline (e.g. a raw @@ -36,9 +38,12 @@ See sample: `pictures-must-use-media-not-blob.good.al`. ## Anti Pattern A `BLOB` field named "Picture" compiles and stores the image bytes, but -it misses the picture control integration, caching, and thumbnail -generation that a `Media`/`MediaSet` field provides for free — the anti -pattern is choosing `BLOB` for image storage out of habit rather than -recognizing that the field is holding a picture, not generic binary data. +it misses the picture control integration and media repository that a +`Media`/`MediaSet` field provides for free — the anti pattern is choosing +`BLOB` for image storage out of habit rather than recognizing that the +field is holding a picture, not generic binary data. A related anti +pattern: assuming `MediaSet` gives automatic image variants or thumbnails +because it sounds like a collection with derived versions — it is only a +collection of independently-imported media objects. See sample: `pictures-must-use-media-not-blob.bad.al`. diff --git a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md index 01f27937..e3b4efd8 100644 --- a/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md +++ b/microsoft/knowledge/data-modeling/table-design-must-match-bc-table-type-conventions.md @@ -33,7 +33,12 @@ Match the table's design to its type: `LookupPageID`. 3. **Subsidiary** (Item Vendor) — subsidiary to a Master/Supplemental table; primary key is the parent key field(s), optionally + `Line No.`; - Worksheet or Tabular page, always filtered by the calling page. + page shape depends on whether the table carries its own identity: a + pure parent-join table (Item Vendor) typically gets a plain List page + filtered by the calling page, while a subsidiary table that supplements + a master record with its own identity — parent key + own code, e.g. + Ship-to Address, Customer/Vendor Bank Account — commonly gets a + List+Card pair instead, for direct editing of that record. 4. **Ledger** (Cust. Ledger Entry) — transactional record of a functional area; primary key `Integer` `Entry No.`, always auto-generated by posting, never user-editable, no free add/delete; List page as diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al index 34b22eca..5c455fdb 100644 --- a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al @@ -1,26 +1,45 @@ +table 50100 "Sample Error Log Buffer" +{ + TableType = Temporary; + fields + { + field(1; "Call Duration (ms)"; Integer) { } + field(2; "Error Message"; Text[250]) { } + } +} + codeunit 50100 "Sample Error Log Writer" { - // Started via Session.StartSession so its commit is independent of the - // caller's transaction. Does one thing: insert the log entry, commit. - trigger OnRun() + // TableNo makes OnRun receive the Record that Session.StartSession + // passes to the new session. This is the only data channel into that + // session — there is no shared memory with the caller's instance. + TableNo = "Sample Error Log Buffer"; + + trigger OnRun(var Rec: Record "Sample Error Log Buffer") var ErrorLogEntry: Record "Sample Error Log"; begin ErrorLogEntry.Init(); - ErrorLogEntry."Call Duration (ms)" := CallDurationMs; + ErrorLogEntry."Call Duration (ms)" := Rec."Call Duration (ms)"; ErrorLogEntry."Error Message" := - CopyStr(ErrorMessageText, 1, MaxStrLen(ErrorLogEntry."Error Message")); + CopyStr(Rec."Error Message", 1, MaxStrLen(ErrorLogEntry."Error Message")); ErrorLogEntry.Insert(true); Commit(); end; +} - procedure SetParameters(Duration: Integer; ErrorText: Text) +// Caller side: populate the buffer record, then hand it to StartSession. +// The insert-and-commit above happens inside the started session, so it +// survives even if the caller's own transaction rolls back afterward. +codeunit 50101 "Sample Error Log Caller Excerpt" +{ + procedure LogFailure(Duration: Integer; ErrorText: Text) + var + LogBuffer: Record "Sample Error Log Buffer" temporary; + SessionId: Integer; begin - CallDurationMs := Duration; - ErrorMessageText := ErrorText; + LogBuffer."Call Duration (ms)" := Duration; + LogBuffer."Error Message" := CopyStr(ErrorText, 1, MaxStrLen(LogBuffer."Error Message")); + Session.StartSession(SessionId, Codeunit::"Sample Error Log Writer", CompanyName, LogBuffer); end; - - var - CallDurationMs: Integer; - ErrorMessageText: Text; } diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md index 6939d01c..e9f94ad4 100644 --- a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md @@ -15,7 +15,7 @@ Inserting a log record inside the same transaction as the operation it logs look ## Best Practice -Write any log whose purpose includes capturing failures from a transaction that is independent of the operation being logged: start an isolated session (`StartSession` on a codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. Capture duration and other telemetry values in the caller and pass them as parameters — the isolated session must not re-read state that a rollback may have erased. Logs that only record successful, committed work can safely stay in the main transaction; this pattern targets error and diagnostic logs specifically. +Write any log whose purpose includes capturing failures from a transaction that is independent of the operation being logged: start an isolated session (`Session.StartSession` on a `TableNo`-scoped codeunit that only inserts the log record and commits) so the entry persists regardless of what happens to the caller's transaction. `StartSession`'s only channel for getting data into that new session is its optional `Record` parameter, delivered to the target codeunit's `OnRun` trigger — a separate session gets a fresh instantiation of the codeunit, so calling a setter procedure on a local object variable before starting the session does not populate anything in the new session's instance. Capture duration and other telemetry values in the caller, place them into the `Record` passed to `StartSession`, and do the insert-and-commit entirely inside that session's own `OnRun`. Logs that only record successful, committed work can safely stay in the main transaction; this pattern targets error and diagnostic logs specifically. See sample: `log-writes-must-survive-rollback.good.al`. diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md index 75f13789..f7a05ffb 100644 --- a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md @@ -11,7 +11,7 @@ application-area: [all] ## Description -An object that is reachable from outside the app's own UI — an API page (`PageType = API`), a web-service-enabled page or query (`ServiceEnabled = true`), or a published API query — is only usable if it is also granted execute access through a permission set. When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing. +An object that is reachable from outside the app's own UI is only usable if it is also granted execute access through a permission set. Three distinct mechanisms make an object reachable this way, and each needs to be checked on its own terms: a page or query published through the **Web Services** configuration page; a custom REST endpoint declared with `PageType = API` / `QueryType = API`; or an individual codeunit method exposed with the `[ServiceEnabled]` attribute (a method-level attribute — it does not apply to pages or queries as a property). When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing. ## Best Practice diff --git a/microsoft/knowledge/style/pages-must-not-contain-business-logic.md b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md index d113a793..f067c23b 100644 --- a/microsoft/knowledge/style/pages-must-not-contain-business-logic.md +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md @@ -11,7 +11,7 @@ application-area: [all] ## Description -A page procedure that calculates a value and assigns it to a field, calls `Rec.Modify()` directly, or implements a business rule is an architecture violation even when it compiles. Pages are a presentation layer: they bind data to the UI and invoke actions. Calculations, validations, and record mutations belong in codeunits, where they can be tested, reused, and called consistently regardless of which page (or API, or batch job) triggers them. When logic lives on a page, it only applies when a user opens that specific page — the same business rule silently doesn't run through any other entry point. +A page procedure that persists a business mutation directly (`Rec.Modify()` outside the standard record-bound save, or a cross-entry-point business rule implemented only in a page trigger) is an architecture violation even when it compiles: the rule only applies when a user opens that specific page, and silently doesn't run through any other entry point (API, batch job, another page). This is narrower than "no calculation may live on a page" — a presentation-specific calculation (formatting, a derived display value) is fine on the page that shows it, and a reusable data invariant commonly belongs on the table itself (a field's own validation/trigger), not forced into a codeunit merely to keep it off the page. The actual line is entry-point independence: a business operation or invariant that must hold regardless of which entry point touches the record belongs in a codeunit or the table, not solely in one page's trigger. A narrow set of patterns are conventional rather than violations: - A setup page reading and writing its own singleton setup record. @@ -26,6 +26,6 @@ See sample: `pages-must-not-contain-business-logic.good.al`. ## Anti Pattern -Direct calculations in a page trigger (e.g. `Rec."Total Amount" := Rec.Quantity * Rec."Unit Price"`), calls to `Rec.Modify()` from a page trigger, or business-rule validation embedded in `OnValidate`/`OnAction` instead of routed through a codeunit. +A cross-entry-point business rule or persisted mutation implemented only in a page trigger — calling `Rec.Modify()` to save a computed business value from `OnValidate`/`OnAction`, or a validation that must hold regardless of caller, instead of routed through a codeunit or the table's own field validation. A presentation-only calculation or a table-owned field invariant is not an instance of this anti-pattern. See sample: `pages-must-not-contain-business-logic.bad.al`. diff --git a/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md index eb1e5745..990a93be 100644 --- a/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md +++ b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md @@ -11,7 +11,7 @@ application-area: [all] ## Description -Source folders inside an AL app should group files by the business feature or module they belong to (`src/Sales/Invoice/`, `src/NoSeries/`), not by which kind of AL object they are (`src/Tables/`, `src/Pages/`, `src/Codeunits/`). Object-type folders scatter everything belonging to one feature across half a dozen directories, so a developer picking up a feature has to jump between folders that share nothing but object type to see the whole picture. Feature folders keep a table, its pages, its codeunits, and its test setup physically together. +Folder structure inside an AL app has no effect on compilation or runtime behavior — this is a repository-organization convention, not a platform requirement, and different projects reasonably choose differently. Grouping files by business feature or module (`src/Sales/Invoice/`, `src/NoSeries/`) rather than by AL object type (`src/Tables/`, `src/Pages/`, `src/Codeunits/`) keeps everything belonging to one feature physically together, which many teams find easier to navigate than jumping between object-type folders that share nothing but their AL object kind. Adopt this consistently on a project rather than mixing both schemes, but treat it as a team convention to apply deliberately, not a Microsoft-mandated structure. Code genuinely shared across multiple features (utility codeunits, common interfaces, shared enums) belongs in a `Common` or `Shared` folder, not duplicated per feature and not left in a catch-all root. diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al index 1a509fc7..224f0dcf 100644 --- a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.good.al @@ -3,13 +3,18 @@ procedure PostSalesOrder_CreatesInvoice() var Customer: Record Customer; SalesHeader: Record "Sales Header"; + SalesInvoiceHeader: Record "Sales Invoice Header"; + InvoiceNo: Code[20]; begin - // [GIVEN] a customer with a full posting-group chain and VAT setup - LibrarySales.CreateCustomerWithPostingSetup(Customer); - // [GIVEN] a sales order for that customer, dated explicitly - LibrarySales.CreateSalesOrderForCustomer(SalesHeader, Customer."No.", WorkDate()); + // [GIVEN] a customer + LibrarySales.CreateCustomer(Customer); + // [GIVEN] a sales order for that customer + LibrarySales.CreateSalesOrderForCustomerNo(SalesHeader, Customer."No."); + SalesHeader.Validate("Posting Date", WorkDate()); + SalesHeader.Modify(true); // [WHEN] - LibrarySales.PostSalesOrder(SalesHeader, false, true); + InvoiceNo := LibrarySales.PostSalesDocument(SalesHeader, false, true); // [THEN] + SalesInvoiceHeader.Get(InvoiceNo); Assert.RecordIsNotEmpty(SalesInvoiceHeader); end; diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al index 6866403a..5079b6d6 100644 --- a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al @@ -8,7 +8,6 @@ codeunit 50103 "Item Price Testing" ItemPriceMgt: Codeunit "Item Price Mgt."; Assert: Codeunit "Library Assert"; - // [SCENARIO] Customer with a specific price list line gets that unit price [Test] procedure GetPrice_CustomerPrice_ReturnsUnitPrice() var @@ -16,6 +15,7 @@ codeunit 50103 "Item Price Testing" Item: Record Item; UnitPrice, LineDiscPct: Decimal; begin + // [SCENARIO] Customer with a specific price list line gets that unit price // [GIVEN] a customer with a price list line at 100 LCY LibrarySales.CreateCustomerWithPrice(Customer, Item, '', 100); // [WHEN] diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.md b/microsoft/knowledge/testing/test-feature-scenario-tags.md index 5842beee..f9a27549 100644 --- a/microsoft/knowledge/testing/test-feature-scenario-tags.md +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.md @@ -15,7 +15,7 @@ Test codeunits are easier to trust and to review when they carry a four-level co ## Best Practice -Put `[FEATURE]` as a comment before the codeunit's opening brace, naming the domain rather than the object. Put `[SCENARIO]` immediately above each `[Test]` attribute, describing the scenario in business language that complements — not duplicates — the procedure name. Inside the body, mark the precondition setup as `[GIVEN]`, the single action under test as `[WHEN]`, and the assertions as `[THEN]`. The procedure name stays the machine-readable identity shown in test-runner output; the `[SCENARIO]` comment stays the human-readable one. Neither replaces the other. +Put `[FEATURE]` as a comment before the codeunit's opening brace, naming the domain rather than the object — Microsoft's own guidance allows setting it once for the whole codeunit, inherited by every test in it. Put `[SCENARIO]`, matching the current BCApps corpus, as the first comment inside each test procedure's body (after `begin`), describing the scenario in business language that complements — not duplicates — the procedure name, followed by `[GIVEN]` marking the precondition setup, `[WHEN]` marking the single action under test, and `[THEN]` marking the assertions. The procedure name stays the machine-readable identity shown in test-runner output; the `[SCENARIO]` comment stays the human-readable one. Neither replaces the other. See sample: `test-feature-scenario-tags.good.al`. diff --git a/microsoft/knowledge/testing/test-one-when-per-test.md b/microsoft/knowledge/testing/test-one-when-per-test.md index 8aa6ca8b..d52a97a4 100644 --- a/microsoft/knowledge/testing/test-one-when-per-test.md +++ b/microsoft/knowledge/testing/test-one-when-per-test.md @@ -11,7 +11,7 @@ application-area: [all] ## Description -Each test procedure should contain exactly one `[WHEN]` block: one action that triggers the behaviour under test. A test with multiple WHENs — "do A, then do B, then check C" — is two or more tests in disguise. Splitting them gives failure isolation (a failing test points at one action, not an ambiguous sequence) and keeps each test readable as a single, falsifiable claim. A precondition action, such as posting a document so a ledger entry exists to assert against, belongs in `[GIVEN]`; only the action actually being asserted belongs in `[WHEN]`. +This is a testing-design practice, not a BC platform requirement — no AL API enforces it, and it should not gate a change the way a platform-contradicted claim would. Each test procedure should contain exactly one `[WHEN]` block: one action that triggers the behaviour under test. A test with multiple WHENs — "do A, then do B, then check C" — is two or more tests in disguise. Splitting them gives failure isolation (a failing test points at one action, not an ambiguous sequence) and keeps each test readable as a single, falsifiable claim. A precondition action, such as posting a document so a ledger entry exists to assert against, belongs in `[GIVEN]`; only the action actually being asserted belongs in `[WHEN]`. ## Best Practice diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.md b/microsoft/knowledge/testing/ui-test-codeunit-naming.md index 4166cd74..1cc332a4 100644 --- a/microsoft/knowledge/testing/ui-test-codeunit-naming.md +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.md @@ -1,26 +1,26 @@ --- bc-version: [all] domain: testing -keywords: [ui-test, testpage, naming, suffix, codeunit, page-testing] +keywords: [ui-test, testpage, naming, suffix, codeunit, page-testing, team-convention] technologies: [al] countries: [w1] application-area: [all] --- -# Suffix UI-layer test codeunits with _UT and never mix layers in one codeunit +# Separate UI-layer and logic-layer tests into different codeunits ## Description -A test codeunit that drives pages through `TestPage` — opening pages, reading FactBox parts, triggering field `OnValidate` through the page — is testing a different layer than a codeunit that calls business-logic procedures directly. Readers need to know which layer a given test exercises without opening it, and a single codeunit that mixes both kinds of test hides that distinction: a failure could mean the logic broke, the page broke, or both. +A test codeunit that drives pages through `TestPage` — opening pages, reading FactBox parts, triggering field `OnValidate` through the page — is testing a different layer than a codeunit that calls business-logic procedures directly. Readers need to know which layer a given test exercises without opening it, and a single codeunit that mixes both kinds of test hides that distinction: a failure could mean the logic broke, the page broke, or both. The `_UT` suffix and adjacent-object-ID pairing below are one team's naming convention for making that split visible, not a BCApps-wide naming standard — BCApps itself uses `UT` for unit tests generally, not specifically to mean "UI layer," and does not treat adjacent object IDs as a semantic pairing mechanism. Apply the suffix only on a project that has explicitly adopted this convention. ## Best Practice -Give any test codeunit that uses `TestPage` a `_UT` (Unit Test — UI layer) suffix, and keep it free of tests that call codeunit/table procedures directly. Keep the corresponding logic-only codeunit unsuffixed. Allocate the two codeunits adjacent object IDs so their relationship is visible in the object list. +Keep UI-layer (`TestPage`-driven) and logic-layer tests in separate codeunits regardless of naming. Projects that adopt a `_UT`-style suffix convention should apply it consistently to every UI-layer test codeunit, keep the corresponding logic-only codeunit unsuffixed, and document the convention where the team's other naming rules live. See sample: `ui-test-codeunit-naming.good.al`. ## Anti Pattern -A codeunit named without the `_UT` suffix that nonetheless contains `TestPage` calls, or — worse — one codeunit that mixes a direct logic-call test and a `TestPage`-driven test side by side. Either way, the codeunit's name no longer tells a reader which layer a failing test actually broke. +One codeunit that mixes a direct logic-call test and a `TestPage`-driven test side by side — a failing test no longer tells a reader which layer actually broke. On a project that has adopted the `_UT` convention, a UI-layer codeunit missing the suffix is also an instance of this anti-pattern; on a project that has not adopted it, the suffix itself is not required. See sample: `ui-test-codeunit-naming.bad.al`. diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md index 5f925d29..b579f698 100644 --- a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md @@ -32,8 +32,14 @@ Match the page's design to its type: - **RoleCenter** — tailored home page for a role; named role + `Role Center`; links to List pages, shows Cues/Activities. - **Card** — view/edit one record; named table + `Card`; FastTabs only, - first FastTab named `General`. Requires a single-field primary key — a - multi-field key needs a List/Worksheet/Tabular page instead. + first FastTab named `General`. A single-field primary key is typical, + but not a hard requirement: a subsidiary table that supplements a + master record with its own identity (parent key + own code — Ship-to + Address, Customer/Vendor Bank Account) commonly gets its own Card page + over a composite key too. Treat the key shape as a contextual signal, + not a mandatory constraint — a composite-key table with no such + supplementing relationship to a master record is the actual signal a + List/Worksheet/Tabular page fits better. - **List** — view multiple records, also the lookup/drilldown surface; named table + `List` if read-only, or the plural table name if editable; primary-key fields shown left-most; `CardPageID` must point @@ -64,10 +70,11 @@ See sample: `page-design-must-match-bc-page-type-conventions.good.al`. ## Anti Pattern -A page that mixes conventions from two types — for example, a "Card" -page built on a table with a two-field primary key, or a "List" page -with no `CardPageID` even though a Card page exists for the same table — -signals a design step was skipped, not a stylistic choice. Also watch +A page that mixes conventions from two types — for example, a "List" +page with no `CardPageID` even though a Card page exists for the same +table — signals a design step was skipped, not a stylistic choice. A +Card page over a composite-key table is not automatically this anti +pattern; check whether the table supplements a master record first. Also watch for: a Worksheet or List page showing primary-key fields it shouldn't (or hiding them when it should show them), and a page with no `UsageCategory` set, which makes it invisible to Tell Me search even diff --git a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md index 3bd7e972..f13c4b8f 100644 --- a/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md +++ b/microsoft/knowledge/upgrade/upgrade-tag-logic-must-not-nest-deeply.md @@ -26,3 +26,7 @@ See sample: `upgrade-tag-logic-must-not-nest-deeply.good.al`. 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. See sample: `upgrade-tag-logic-must-not-nest-deeply.bad.al`. + +## Source + +Microsoft's own "Upgrading Extensions" guidance, Design considerations: "Keep tags simple by limiting nesting tags to two levels. Complicated if statements can lead to problems." — https://learn.microsoft.com/dynamics365/business-central/dev-itpro/developer/devenv-upgrading-extensions#using-upgrade-tags-to-control-upgrade-code diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al index 35af72be..e94f7f2d 100644 --- a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.good.al @@ -6,6 +6,8 @@ page 50102 "Vendor Contact Info API" APIVersion = 'v1.0'; SourceTable = Vendor; DelayedInsert = true; + InsertAllowed = false; + DeleteAllowed = false; layout { diff --git a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md index 5289fad8..a06fa35b 100644 --- a/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md +++ b/microsoft/knowledge/web-services/api-page-least-privilege-write-access.md @@ -11,11 +11,11 @@ application-area: [all] ## Description -A general-purpose API page that exposes many fields should not be widened to allow writes on one additional field. A `PageType = API` page consumed by an external integration, an automation agent, or a partner system carries the same risk regardless of caller: a write-enabled page with no per-field restriction is a wide-open surface. The most common real-world shape of this problem is not a page that started narrow and got widened — it is a page that was never restricted at all: with `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` left at their defaults and no `Editable = false` on any field, every field on the source table — including identity fields and financially significant ones — is fully writable, with nothing marking that as deliberate. +A general-purpose API page that exposes many fields should not be widened to allow writes on one additional field. A `PageType = API` page consumed by an external integration, an automation agent, or a partner system carries the same risk regardless of caller: a write-enabled page with no per-field restriction is a wide-open surface. Least privilege has to cover both dimensions of exposure: which fields are on the page, and which operations the page allows. Only a field actually placed on the page is reachable at all — but a page that includes many fields, with `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` left at their defaults and no `Editable = false` on most of them, leaves every one of those included fields — identity fields and financially significant ones among them — fully writable, with nothing marking that as deliberate. Restricting fields alone is not enough either: a page with only two fields on it can still let a caller insert brand-new records or delete existing ones if `InsertAllowed`/`DeleteAllowed` are left at their true defaults (both `true`). ## Best Practice -Create a separate, minimal API page that exposes only the key and the specific field the consumer needs to write, with everything else `Editable = false` or simply absent from the page. +Create a separate, minimal API page that exposes only the key and the specific field the consumer needs to write, with everything else `Editable = false` or simply absent from the page — and set `InsertAllowed`/`DeleteAllowed` to `false` unless the consumer's use case genuinely needs to create or delete records through that page. See sample: `api-page-least-privilege-write-access.good.al`. diff --git a/microsoft/skills/review/al-data-modeling-review.md b/microsoft/skills/review/al-data-modeling-review.md index 2386613a..8e773902 100644 --- a/microsoft/skills/review/al-data-modeling-review.md +++ b/microsoft/skills/review/al-data-modeling-review.md @@ -46,6 +46,8 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `data-modeling` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action. - 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`. +- 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 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`. diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index 39851ac8..479319b2 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -47,6 +47,7 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `error-handling` article: - `[ErrorBehavior(ErrorBehavior::Collect)]`, `ErrorInfo.Collectible`, `HasCollectedErrors`, `GetCollectedErrors`, or `ClearCollectedErrors` is added or changed, especially when errors are collected without later surfacing/clearing them — `collect-validation-errors-with-errorbehavior`. +- New or changed code calls `Session.StartSession` from within error/duration logging around a web-service call, background job, or other operation expected to fail — `log-writes-must-survive-rollback`. - Developer-only invariant text is raised with default client visibility, or a user-actionable validation is hidden as `ErrorType::Internal` — `errortype-internal-vs-client-for-diagnostics`. - `FieldError` receives a complete capitalized sentence, repeats the field caption/value, or ends the predicate with punctuation — `fielderror-default-message-logic`. - An unguarded `FieldError` is used as though it performed a comparison, or `TestField` is forced onto a complex rule needing a tailored predicate — `fielderror-vs-testfield`. diff --git a/microsoft/skills/review/al-security-review.md b/microsoft/skills/review/al-security-review.md index 8472afea..31f274ec 100644 --- a/microsoft/skills/review/al-security-review.md +++ b/microsoft/skills/review/al-security-review.md @@ -39,7 +39,7 @@ Narrow the relevant files to the subset that applies to the changes under review - The changed AL object names and types — especially permission sets, codeunits handling authentication or authorization, objects touching `Isolated Storage`, `OAuth2` flows, web service endpoints, API pages, event publishers, and RecordRef helpers. - The changed procedures and triggers, weighted toward those that call `HttpClient`, validate or compose URLs, write to telemetry, read or write secrets, unwrap SecretText, manipulate record-level security, expose var Boolean guard parameters, or bypass the permission model (for example, `RecordRef.Open`, `Record.WritePermission`, direct table access from a non-owning app). -- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`). +- Tokens extracted from the diff that relate to security concerns (`IsolatedStorage`, `SetEncrypted`, `OAuth2`, `SecretText`, `Unwrap`, `NonDebuggable`, `Password`, `Token`, `HttpClient`, `Uri`, `AreURIsHaveSameHost`, `IsValidURIPattern`, `RecordRef`, `RecordId`, `TransferFields`, `Codeunit.Run`, `Access = Internal`, `internalsVisibleTo`, `Open`, `IntegrationEvent`, `SkipValidation`, `HasAccess`, `Permission`, `UserSecurityId`, `Commit`, `ServiceEnabled`, `PageType = API`, `QueryType = API`, `permissionset`, `Web Services`). 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. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 8bad7345..37243d4d 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -46,6 +46,9 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `testing` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action. - A method in a `Subtype = Test` codeunit adds or changes `[TransactionModel(...)]`, exercises code that calls `Commit` under `AutoRollback`, defaults broadly to `AutoCommit`, or chooses `None` for a writing test — `transactionmodel-attribute-governs-test-transactions`. +- A test procedure or comment adds `[FEATURE]`/`[SCENARIO]`/`[GIVEN]`/`[WHEN]`/`[THEN]` tags — `test-feature-scenario-tags`. +- A test codeunit calls `TestPage` methods (`OpenNew`, `OpenView`, `OpenEdit`) — `ui-test-codeunit-naming`. +- A `[GIVEN]`-tagged setup precedes a posting call or report execution — `given-blocks-must-cover-full-precondition-chain`. - An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it. - A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`. - 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`. diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index c0af5af8..c3113a5a 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -41,7 +41,7 @@ Narrow the relevant files to the subset that applies to the changes under review - **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`, and to JavaScript/CSS/HTML that implements a control add-in's rendering or Business Central communication. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files. - For each relevant knowledge file, compute overlap against changed page declarations and control add-in files, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, `ShowCaption`, `InstructionalText`, `GridLayout`, `Style`, `StyleExpr`, promoted action definitions, field importance, page background tasks, DOM creation, ARIA attributes, keyboard/focus handlers, packaged-resource AJAX, and calls from JavaScript into AL. -- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions). +- Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`). A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed page element. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. From 30abf077be7f776f44319dccec80965df945738f Mon Sep 17 00:00:00 2001 From: Michael Dieringer <65093775+MichaelDieringer@users.noreply.github.com> Date: Tue, 8 Sep 2026 20:05:56 +0200 Subject: [PATCH 3/3] Address second round of Jesper Schulz-Wedde's review on PR #157 - log-writes-must-survive-rollback.good.al: fixed invalid trigger OnRun(var Rec: ...) declaration; Rec is implicit when TableNo is set. - exposed-objects-must-be-in-a-permission-set.md: distinguished the three exposure mechanisms (page/query web service or API, codeunit published as a web service, [ServiceEnabled] bound action on a page) and their actual permission targets (page/query "..." = X vs codeunit "..." = X). - code-must-not-change-workdate.md: scoped from an absolute "never" to "not as a side effect of unrelated logic" - verified real WorkDate(x) setter usage in BCApps demo-data generators and test codeunits. - bcpt-scenarios-must-be-app-specific.md: SingleInstance and StartScenario/EndScenario reframed as context-dependent patterns, not mandatory requirements - BCPT Create Customer uses neither. - test-feature-scenario-tags.good.al/.bad.al: replaced the invented LibrarySales.CreateCustomerWithPrice/"Item Price Mgt." calls with a real, verified price-list-line test using Library - Sales/Library - Inventory/ Library - Price Calculation. - page-design-must-match-bc-page-type-conventions.md: scoped the missing UsageCategory anti-pattern to pages intended as searchable entry points. - defensive-vs-offensive-code-must-match-blast-radius.md/.good.al/.bad.al: replaced the VAT registration number "low blast radius" example with a genuinely cosmetic field (customer home page URL). - source-organized-by-feature-not-object-type.md: anti-pattern reframed as inconsistency with a repo's own convention, not the object-type scheme itself. - pictures-must-use-media-not-blob.md: removed leftover "image variants" wording contradicting the already-corrected MediaSet description. Proactively fixed while sweeping all fixtures for invented APIs: - given-blocks-must-cover-full-precondition-chain.bad.al: PostSalesOrder called with wrong arity and referenced an undeclared variable. - ui-test-codeunit-naming.good.al/.bad.al: replaced the same fake "Item Price Mgt."/TestPage "Item Price" with real Library - Sales calls and the real Customer Card TestPage. Worklist completeness: added review-skill cues for the 12 of 18 new rules that had none (al-appsource-review.md, al-data-modeling-review.md, al-error-handling-review.md, al-security-review.md, al-style-review.md x3, al-testing-review.md x2, al-ui-review.md, al-upgrade-review.md, al-web-services-review.md), and fixed test-feature-scenario-tags' cue, which only matched the compliant (tagged) shape instead of the anti-pattern (untagged/generic-named test). Co-Authored-By: Claude Sonnet 5 --- .../code-must-not-change-workdate.md | 48 +++++++++++++------ .../pictures-must-use-media-not-blob.md | 4 +- ...ensive-code-must-match-blast-radius.bad.al | 4 +- ...nsive-code-must-match-blast-radius.good.al | 6 +-- ...-offensive-code-must-match-blast-radius.md | 2 +- .../log-writes-must-survive-rollback.good.al | 2 +- ...sed-objects-must-be-in-a-permission-set.md | 10 +++- ...ce-organized-by-feature-not-object-type.md | 18 +++++-- .../bcpt-scenarios-must-be-app-specific.md | 2 +- ...-must-cover-full-precondition-chain.bad.al | 5 +- .../testing/test-feature-scenario-tags.bad.al | 24 +++++++--- .../test-feature-scenario-tags.good.al | 27 +++++++---- .../testing/test-one-when-per-test.bad.al | 31 +++++++++--- .../testing/test-one-when-per-test.good.al | 46 +++++++++++++----- .../testing/ui-test-codeunit-naming.bad.al | 19 ++++---- .../testing/ui-test-codeunit-naming.good.al | 33 ++++++++----- ...ign-must-match-bc-page-type-conventions.md | 10 ++-- .../skills/review/al-appsource-review.md | 1 + .../skills/review/al-data-modeling-review.md | 1 + .../skills/review/al-error-handling-review.md | 1 + microsoft/skills/review/al-security-review.md | 1 + microsoft/skills/review/al-style-review.md | 3 ++ microsoft/skills/review/al-testing-review.md | 8 ++-- microsoft/skills/review/al-ui-review.md | 1 + microsoft/skills/review/al-upgrade-review.md | 1 + .../skills/review/al-web-services-review.md | 1 + 26 files changed, 219 insertions(+), 90 deletions(-) diff --git a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md index 428e6dbd..daf8259c 100644 --- a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md @@ -13,28 +13,46 @@ application-area: [all] The work date is a per-user session setting the user controls from the client (the date shown in the top-right corner, used to default posting -dates and date filters). Application code must never call the `WorkDate` -function to set a new value. Doing so changes what the user sees and -defaults to for the rest of their session, as a side effect of running -unrelated business logic — a surprising, hard-to-trace behavior change the -user never asked for and has no visibility into. - -This is a call-direction distinction: reading the current work date via -`WorkDate` (or `WorkDate()` with no argument) is fine and common — it is -only the assignment form, `WorkDate(NewDate)`, that is the anti-pattern. +dates and date filters). Business logic unrelated to that setting must not +call `WorkDate(NewDate)` as a side effect of doing something else — that +silently changes what the user sees and defaults to for the rest of their +session, a surprising, hard-to-trace behavior change the user never asked +for and has no visibility into. This is not a blanket ban on the setter +itself: BCApps' own demo-data generators legitimately save the current +work date, set a specific one to backdate the data they create, and +restore it afterward (see `CreateDemoEDocsBE.Codeunit.al`'s +`WorkDate(SampleInvoiceDate)` / `WorkDate(SavedWorkDate)` pair), and test +codeunits routinely set `WorkDate` deliberately to control the date context +a test runs under (hundreds of calls across BCApps' test suite, for +example `SustainabilityPostingTest.Codeunit.al`). Both are the code's +*actual purpose*, not a side effect of something unrelated. + +This is a call-direction distinction for the read side: reading the +current work date via `WorkDate` (or `WorkDate()` with no argument) is +always fine. ## Best Practice -Read the work date to default a value; never write to it. +Read the work date to default a value. Only write to it when changing it +*is* the operation being performed — implementing the user's own +work-date/settings action, or a test or demo-data routine that deliberately +establishes a date context (saving and restoring the prior value if the +routine must leave the session as it found it). Business logic that exists +to do something else must never write `WorkDate` as an incidental side +effect; if a calculation needs a specific date, pass or compute that date +as a local variable instead. See sample: `code-must-not-change-workdate.good.al`. ## Anti Pattern -Setting the work date from within a codeunit, report, or page action -changes session state the user owns, for the duration of a call that has -nothing to do with the user's date preference. If a scenario genuinely -needs a specific date for a calculation, pass or compute that date as a -local variable — never repurpose the session's `WorkDate`. +Setting the work date from within a codeunit, report, or page action whose +purpose is unrelated to the user's date preference — for example, a +posting or calculation routine that calls `WorkDate(SomeDate)` to make its +own logic simpler. This changes session state the user owns for the +duration of a call that was never about the work date, and never restores +it. This is a different case from a test or demo-data routine explicitly +declaring a date context: the anti-pattern is unrelated logic silently +mutating state it does not own, not the setter form itself. See sample: `code-must-not-change-workdate.bad.al`. diff --git a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md index 195c4a8f..b9db6bd8 100644 --- a/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md +++ b/microsoft/knowledge/data-modeling/pictures-must-use-media-not-blob.md @@ -30,8 +30,8 @@ file attachment blob unrelated to picture rendering). ## Best Practice -Use `Media` (or `MediaSet` for multiple image variants) for any field that -holds a picture. +Use `Media` for a single image, or `MediaSet` for multiple independent +images, for any field that holds a picture. See sample: `pictures-must-use-media-not-blob.good.al`. diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al index fb822b3f..6f2835f8 100644 --- a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.bad.al @@ -1,6 +1,6 @@ // Both fields guarded the same way, out of habit rather than analysis. -if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then - VATRegNo := SalesHeader."VAT Registration No."; // low blast radius - fine +if Customer.Get(SalesHeader."Sell-to Customer No.") then + CustomerHomePage := Customer."Home Page"; // low blast radius - fine // but the same pattern, unexamined, was also applied here: if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al index 38fbc887..19fa2ad3 100644 --- a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al @@ -1,8 +1,8 @@ // Low blast radius: guard, with an explicit chosen fallback. -if SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo) then - VATRegNo := SalesHeader."VAT Registration No."; +if Customer.Get(SalesHeader."Sell-to Customer No.") then + CustomerHomePage := Customer."Home Page"; // Blank is an acceptable, deliberately-considered default here - the field -// is informational and a reviewer sees it before the document ships. +// is purely a display convenience and a reviewer sees it before the document ships. // High blast radius: let it fail loud, because this feeds posted VAT. SalesHeader.Get(SalesHeader."Document Type"::Order, DocumentNo); diff --git a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md index fab6850d..14b52174 100644 --- a/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md +++ b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.md @@ -21,6 +21,6 @@ See sample: `defensive-vs-offensive-code-must-match-blast-radius.good.al`. ## Anti Pattern -Guarding two fields the same way purely out of habit, without analyzing what each one feeds. A low-blast-radius field, such as a VAT registration number shown only on a printed document, and a high-blast-radius field, such as the VAT posting group that determines VAT actually applied to a posted transaction, are both wrapped in the same `if Header.Get(...) then ... else` pattern with a blank/zero fallback — leaving the posting-critical field free to post with a silently wrong value. +Guarding two fields the same way purely out of habit, without analyzing what each one feeds. A low-blast-radius field, such as a customer's home page URL shown only for convenience on a printed document, and a high-blast-radius field, such as the VAT posting group that determines VAT actually applied to a posted transaction, are both wrapped in the same `if Header.Get(...) then ... else` pattern with a blank/zero fallback — leaving the posting-critical field free to post with a silently wrong value. A VAT registration number is not a safe stand-in for the low-risk side of this example: it is legally relevant, often validated, and can feed external VAT services or mandated document output, so it belongs on the offensive/fail-fast side alongside the posting group, not next to it as the "safe" contrast. See sample: `defensive-vs-offensive-code-must-match-blast-radius.bad.al`. diff --git a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al index 5c455fdb..c0f7d65f 100644 --- a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.good.al @@ -15,7 +15,7 @@ codeunit 50100 "Sample Error Log Writer" // session — there is no shared memory with the caller's instance. TableNo = "Sample Error Log Buffer"; - trigger OnRun(var Rec: Record "Sample Error Log Buffer") + trigger OnRun() var ErrorLogEntry: Record "Sample Error Log"; begin diff --git a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md index f7a05ffb..3b325c4c 100644 --- a/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md +++ b/microsoft/knowledge/security/exposed-objects-must-be-in-a-permission-set.md @@ -11,11 +11,17 @@ application-area: [all] ## Description -An object that is reachable from outside the app's own UI is only usable if it is also granted execute access through a permission set. Three distinct mechanisms make an object reachable this way, and each needs to be checked on its own terms: a page or query published through the **Web Services** configuration page; a custom REST endpoint declared with `PageType = API` / `QueryType = API`; or an individual codeunit method exposed with the `[ServiceEnabled]` attribute (a method-level attribute — it does not apply to pages or queries as a property). When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing. +An object that is reachable from outside the app's own UI is only usable if it is also granted execute access through a permission set. Three distinct mechanisms make an object reachable this way, each with its own permission target: + +- A page or query published through the **Web Services** configuration page, or a custom REST endpoint declared with `PageType = API` / `QueryType = API` — both need a `page "..." = X` / `query "..." = X` entry for that object. +- A codeunit published through **Web Services** exposes *every* public procedure on it as an OData/SOAP operation automatically — there is no per-method attribute to add. The permission target is the codeunit itself: `codeunit "..." = X`. +- `[ServiceEnabled]` is a method-level attribute used on a *page* procedure to expose it as an OData v4 bound action (for example a `Post` action on an invoice page) — it does not apply to pages, queries, or codeunits as an object-level property, and it does not create its own permission target. The action is still a call into that page object, so the page's own `page "..." = X` entry is what governs it. + +When such an object is left out of every permission set, it becomes both unusable (no caller, human or service, can reach it) and invisible in review: nobody deliberately decided who may call it. Exposure without a matching grant is not a safe default; it is an endpoint nobody is governing. ## Best Practice -Give every exposed object an explicit execute entry (`page "..." = X`, `query "..." = X`) in a permission set shipped by the app. Route sensitive endpoints into a dedicated, non-default admin permission set so reaching them requires a deliberate grant rather than being included by default. If an object should never be reachable from outside the app, remove the exposure itself (drop `PageType = API` / `ServiceEnabled`) rather than leaving an orphaned endpoint with no permission-set membership. +Give every exposed object an explicit execute entry in a permission set shipped by the app: `page "..." = X` / `query "..." = X` for a published or API page/query (including one that exposes a `[ServiceEnabled]` bound action), and `codeunit "..." = X` for a codeunit published as a web service. Route sensitive endpoints into a dedicated, non-default admin permission set so reaching them requires a deliberate grant rather than being included by default. If an object should never be reachable from outside the app, remove the exposure itself (drop `PageType = API` / the Web Services registration) rather than leaving an orphaned endpoint with no permission-set membership. See sample: `exposed-objects-must-be-in-a-permission-set.good.al`. diff --git a/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md index 990a93be..c6980939 100644 --- a/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md +++ b/microsoft/knowledge/style/source-organized-by-feature-not-object-type.md @@ -28,9 +28,21 @@ Each feature folder holds every object type it needs; shared code has one dedica ## Anti Pattern +A repository that documents or has established feature-based organization +as its convention, but then mixes in object-type folders for new work +anyway: + src/ - ├── Tables/ - ├── Pages/ + ├── Sales/ + │ └── Invoice/ + ├── Tables/ <- new objects land here instead of a feature folder └── Codeunits/ -Finding everything related to one feature now requires searching multiple folders and mentally reassembling it from scattered pieces. +The anti-pattern is inconsistency with the project's own chosen convention, +not the object-type scheme itself — a repository that deliberately and +consistently organizes by object type throughout is exercising the other +reasonable choice described above, not violating this rule. What actually +costs a reader time is a codebase where some features live under their own +folder and others are scattered across type folders, so finding everything +related to one feature means checking both schemes and reassembling it from +wherever each object happened to land. diff --git a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md index 8dae0b70..1269b3e9 100644 --- a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md @@ -15,7 +15,7 @@ A PerformanceTest app that ships with only the generic Microsoft BCPT samples (c ## Best Practice -For every major business flow the extension adds, create a matching `BCPT*` scenario codeunit: `SingleInstance = true`, implementing `"BCPT Test Param. Provider"`, wrapping the operation under test in `BCPTTestContext.StartScenario()` / `EndScenario()`, and building its own test data in a local `InitTest()` procedure rather than depending on hardcoded records. Give each distinct step its own named scenario so a regression in one step doesn't hide inside a coarser measurement. +For every major business flow the extension adds, create a matching `BCPT*` scenario codeunit implementing `"BCPT Test Param. Provider"`, building its own test data in a local `InitTest()` procedure rather than depending on hardcoded records. Beyond that shared shape, the interface details are context-dependent, not fixed requirements: most of Microsoft's own shipped BCPT samples declare `SingleInstance = true`, but `codeunit "BCPT Create Customer"` does not, relying instead on `OnRun` calling `InitTest()` unconditionally every run. Likewise, wrapping the operation under test in `BCPTTestContext.StartScenario()` / `EndScenario()` is a real, available pattern for splitting one codeunit's run into several separately measured steps — useful when a regression in one step should not hide inside a coarser, whole-`OnRun` measurement — but it is not what every sample does; `"BCPT Create Customer"` measures its entire `OnRun` as a single implicit scenario and never calls `StartScenario`/`EndScenario` at all. Choose per-step scenarios when step-level granularity matters to the flow being tested; otherwise a single measured `OnRun` is a legitimate, simpler choice. See sample: `bcpt-scenarios-must-be-app-specific.good.al`. diff --git a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al index 4357b409..0181fdb2 100644 --- a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.bad.al @@ -2,11 +2,14 @@ procedure PostSalesOrder_CreatesInvoice() var SalesHeader: Record "Sales Header"; + SalesInvoiceHeader: Record "Sales Invoice Header"; + InvoiceNo: Code[20]; begin // [GIVEN] a sales order — posting groups left to whatever exists in the test company LibrarySales.CreateSalesOrder(SalesHeader); // [WHEN] - LibrarySales.PostSalesOrder(SalesHeader, false, true); + InvoiceNo := LibrarySales.PostSalesDocument(SalesHeader, false, true); // [THEN] + SalesInvoiceHeader.Get(InvoiceNo); Assert.RecordIsNotEmpty(SalesInvoiceHeader); end; diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al b/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al index fa9d2c01..06e5d54f 100644 --- a/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.bad.al @@ -3,7 +3,9 @@ codeunit 50102 "Item Price Testing" Subtype = Test; var - ItemPriceMgt: Codeunit "Item Price Mgt."; + LibrarySales: Codeunit "Library - Sales"; + LibraryInventory: Codeunit "Library - Inventory"; + LibraryPriceCalculation: Codeunit "Library - Price Calculation"; Assert: Codeunit "Library Assert"; [Test] @@ -11,11 +13,21 @@ codeunit 50102 "Item Price Testing" var Customer: Record Customer; Item: Record Item; - Price, Disc: Decimal; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; + SalesHeader: Record "Sales Header"; + SalesLine: Record "Sales Line"; begin - // setup mixed with assertions, no clear layers - Customer.Insert(false); - ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', Price, Disc); - Assert.AreEqual(100, Price, ''); + // setup mixed with assertions, no clear layers, no FEATURE/SCENARIO/GIVEN/WHEN/THEN tags + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + LibrarySales.CreateSalesDocumentWithItem( + SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); + Assert.AreEqual(PriceListLine."Unit Price", SalesLine."Unit Price", ''); end; } diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al index 5079b6d6..fdbb76b4 100644 --- a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al @@ -5,7 +5,8 @@ codeunit 50103 "Item Price Testing" var LibrarySales: Codeunit "Library - Sales"; - ItemPriceMgt: Codeunit "Item Price Mgt."; + LibraryInventory: Codeunit "Library - Inventory"; + LibraryPriceCalculation: Codeunit "Library - Price Calculation"; Assert: Codeunit "Library Assert"; [Test] @@ -13,14 +14,24 @@ codeunit 50103 "Item Price Testing" var Customer: Record Customer; Item: Record Item; - UnitPrice, LineDiscPct: Decimal; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; + SalesHeader: Record "Sales Header"; + SalesLine: Record "Sales Line"; begin // [SCENARIO] Customer with a specific price list line gets that unit price - // [GIVEN] a customer with a price list line at 100 LCY - LibrarySales.CreateCustomerWithPrice(Customer, Item, '', 100); - // [WHEN] - ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); - // [THEN] - Assert.AreEqual(100, UnitPrice, 'Unit price must match customer price list'); + // [GIVEN] a customer and an item with a customer-specific sales price list line + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + // [WHEN] a sales line is created for that customer and item + LibrarySales.CreateSalesDocumentWithItem( + SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); + // [THEN] the sales line picks up the customer's price list line + Assert.AreEqual(PriceListLine."Unit Price", SalesLine."Unit Price", 'Unit price must match customer price list'); end; } diff --git a/microsoft/knowledge/testing/test-one-when-per-test.bad.al b/microsoft/knowledge/testing/test-one-when-per-test.bad.al index d24da5db..c49696cb 100644 --- a/microsoft/knowledge/testing/test-one-when-per-test.bad.al +++ b/microsoft/knowledge/testing/test-one-when-per-test.bad.al @@ -1,15 +1,32 @@ [Test] -procedure GetPrice_ThenGetDiscount_ReturnsCorrectValues() +procedure GetPrice_ThenGetPriceLines_ReturnsCorrectValues() var - TempBuffer: Record "Item Price Tier Buffer" temporary; - UnitPrice, LineDiscPct: Decimal; + Customer: Record Customer; + Item: Record Item; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; + SalesHeader: Record "Sales Header"; + SalesLine: Record "Sales Line"; begin // [GIVEN] ... + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); // [WHEN] first action - ItemPriceMgt.GetSalesPrice(CustomerNo, ItemNo, '', UnitPrice, LineDiscPct); + LibrarySales.CreateSalesDocumentWithItem( + SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); // [WHEN] second action — this is a second test in disguise - ItemPriceMgt.GetSalesPriceTiers(CustomerNo, ItemNo, '', TempBuffer); + PriceListLine.Validate("Minimum Quantity", 10); + PriceListLine.Modify(true); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); // [THEN] asserting two unrelated things - Assert.AreEqual(100, UnitPrice, ''); - Assert.IsFalse(TempBuffer.IsEmpty(), ''); + Assert.AreEqual(PriceListLine."Unit Price", SalesLine."Unit Price", ''); + PriceListLine.SetRange("Price List Code", PriceListHeader.Code); + Assert.AreEqual(2, PriceListLine.Count(), ''); end; diff --git a/microsoft/knowledge/testing/test-one-when-per-test.good.al b/microsoft/knowledge/testing/test-one-when-per-test.good.al index 320bffde..cb617088 100644 --- a/microsoft/knowledge/testing/test-one-when-per-test.good.al +++ b/microsoft/knowledge/testing/test-one-when-per-test.good.al @@ -3,27 +3,51 @@ procedure GetPrice_CustomerPrice_ReturnsCorrectUnitPrice() var Customer: Record Customer; Item: Record Item; - UnitPrice, LineDiscPct: Decimal; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; + SalesHeader: Record "Sales Header"; + SalesLine: Record "Sales Line"; begin - // [GIVEN] a customer with a price list line at 100 - LibrarySales.CreateCustomerWithPrice(Customer, Item, '', 100); + // [GIVEN] a customer with a price list line for the item + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); // [WHEN] - ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + LibrarySales.CreateSalesDocumentWithItem( + SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); // [THEN] - Assert.AreEqual(100, UnitPrice, 'Unit price must match price list'); + Assert.AreEqual(PriceListLine."Unit Price", SalesLine."Unit Price", 'Unit price must match price list'); end; [Test] -procedure GetPriceTiers_CustomerTier_ReturnsOneTierLine() +procedure GetPriceLines_TwoMinimumQuantityLines_ReturnsBoth() var Customer: Record Customer; Item: Record Item; - TempBuffer: Record "Item Price Tier Buffer" temporary; + PriceListHeader: Record "Price List Header"; + PriceListLine: Record "Price List Line"; begin - // [GIVEN] a customer with a tier price at min qty 10 - LibrarySales.CreateCustomerWithTierPrice(Customer, Item, '', 10, 90); + // [GIVEN] a customer price list with two minimum-quantity price lines for the same item + LibrarySales.CreateCustomer(Customer); + LibraryInventory.CreateItem(Item); + LibraryPriceCalculation.CreatePriceHeader( + PriceListHeader, PriceListHeader."Price Type"::Sale, "Price Source Type"::Customer, Customer."No."); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + PriceListLine.Validate("Minimum Quantity", 10); + PriceListLine.Modify(true); + LibraryPriceCalculation.CreateSalesPriceLine( + PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", + "Price Asset Type"::Item, Item."No."); + PriceListLine.Validate("Minimum Quantity", 50); + PriceListLine.Modify(true); // [WHEN] - ItemPriceMgt.GetSalesPriceTiers(Customer."No.", Item."No.", '', TempBuffer); + PriceListLine.SetRange("Price List Code", PriceListHeader.Code); // [THEN] - Assert.AreEqual(1, TempBuffer.Count(), 'Exactly one tier line expected'); + Assert.AreEqual(2, PriceListLine.Count(), 'Exactly two price lines expected'); end; diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al b/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al index 98fc2b26..2acba18e 100644 --- a/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.bad.al @@ -3,22 +3,25 @@ codeunit 50104 "Item Price Testing" Subtype = Test; [Test] - procedure GetPrice_LogicTest() + procedure ApplyDiscount_LogicTest() var - Customer: Record Customer; - Item: Record Item; - UnitPrice, LineDiscPct: Decimal; + Assert: Codeunit "Library Assert"; begin // logic test — fine on its own, but not paired with a UI test below - ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + Assert.AreEqual(90, ApplyDiscount(100, 10), 'A 10% discount on 100 must yield 90'); + end; + + local procedure ApplyDiscount(UnitPrice: Decimal; DiscountPct: Decimal): Decimal + begin + exit(UnitPrice - (UnitPrice * DiscountPct / 100)); end; [Test] - procedure Page_ShowsPrice_UT() + procedure CustomerCard_Opens_UT() var - ItemPricePage: TestPage "Item Price"; + CustomerCard: TestPage "Customer Card"; begin // UI test mixed into a logic-test codeunit, and the codeunit lacks the _UT suffix - ItemPricePage.OpenNew(); + CustomerCard.OpenNew(); end; } diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al b/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al index 5f216b0f..6c343d87 100644 --- a/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.good.al @@ -3,13 +3,18 @@ codeunit 50105 "Item Price Testing" Subtype = Test; [Test] - procedure GetPrice_CustomerPrice_ReturnsUnitPrice() + procedure ApplyDiscount_ReducesUnitPrice() var - Customer: Record Customer; - Item: Record Item; - UnitPrice, LineDiscPct: Decimal; + Assert: Codeunit "Library Assert"; + DiscountedPrice: Decimal; + begin + DiscountedPrice := ApplyDiscount(100, 10); + Assert.AreEqual(90, DiscountedPrice, 'A 10% discount on 100 must yield 90'); + end; + + local procedure ApplyDiscount(UnitPrice: Decimal; DiscountPct: Decimal): Decimal begin - ItemPriceMgt.GetSalesPrice(Customer."No.", Item."No.", '', UnitPrice, LineDiscPct); + exit(UnitPrice - (UnitPrice * DiscountPct / 100)); end; } @@ -18,16 +23,20 @@ codeunit 50106 "Item Price Testing_UT" Subtype = Test; [Test] - procedure Page_EnterCustomerAndItem_FactBoxShowsPrice() + procedure CustomerCard_SetName_UpdatesField() var Customer: Record Customer; - Item: Record Item; - ItemPricePage: TestPage "Item Price"; + CustomerCard: TestPage "Customer Card"; Assert: Codeunit "Library Assert"; + LibrarySales: Codeunit "Library - Sales"; begin - ItemPricePage.OpenNew(); - ItemPricePage.CustomerNo.SetValue(Customer."No."); - ItemPricePage.ItemNo.SetValue(Item."No."); - Assert.AreEqual('100.00', ItemPricePage.PriceInfo.UnitPrice.Value(), ''); + LibrarySales.CreateCustomer(Customer); + CustomerCard.OpenEdit(); + CustomerCard.GoToRecord(Customer); + CustomerCard.Name.SetValue('Updated Name'); + CustomerCard.Close(); + + Customer.Get(Customer."No."); + Assert.AreEqual('Updated Name', Customer.Name, 'Name must be updated through the page'); end; } diff --git a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md index b579f698..d3628d84 100644 --- a/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md +++ b/microsoft/knowledge/ui/page-design-must-match-bc-page-type-conventions.md @@ -75,9 +75,11 @@ page with no `CardPageID` even though a Card page exists for the same table — signals a design step was skipped, not a stylistic choice. A Card page over a composite-key table is not automatically this anti pattern; check whether the table supplements a master record first. Also watch -for: a Worksheet or List page showing primary-key fields it shouldn't (or -hiding them when it should show them), and a page with no -`UsageCategory` set, which makes it invisible to Tell Me search even -though it otherwise works. +for a Worksheet or List page showing primary-key fields it shouldn't (or +hiding them when it should show them). A page with no `UsageCategory` set +is not automatically a defect either: supporting pages, subpages, dialogs, +and pages intended only to be reached through another workflow correctly +have no `UsageCategory` — flag its absence only on a page intended as a +searchable entry point in its own right. See sample: `page-design-must-match-bc-page-type-conventions.bad.al`. diff --git a/microsoft/skills/review/al-appsource-review.md b/microsoft/skills/review/al-appsource-review.md index 43a2e205..9b760a4c 100644 --- a/microsoft/skills/review/al-appsource-review.md +++ b/microsoft/skills/review/al-appsource-review.md @@ -51,6 +51,7 @@ The following targeted checks cover every current `appsource` article across the Before emitting an affix finding, compare every owned object name and every member added to another publisher's object against the configured `mandatoryAffixes`/`mandatoryPrefix`. A matching prefix or suffix is compliant. Do not flag an `ABC`-prefixed object or an `ABC`-suffixed extension member when `ABC` is the configured affix. - For BC v27 or later, `app.json` adds or changes the `help` URL to a path deeper than two levels, or a changed Copilot/context-sensitive help arrangement would ground the app under an overly broad truncated parent — `keep-copilot-help-url-to-two-path-levels`. +- A release/submission pipeline change (`AL-Go-Settings.json`, a publish/release workflow) or an `app.json` version bump is present without the new complete version being strictly greater than the previously submitted one, or the change asserts a hand-edited build/revision or every-merge-is-a-release policy as a universal AppSource rule rather than a project-specific workflow choice — `release-must-update-app-version`. Require repository/pipeline context to know the previously submitted version; a single `app.json` diff cannot prove ordering on its own. 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`. diff --git a/microsoft/skills/review/al-data-modeling-review.md b/microsoft/skills/review/al-data-modeling-review.md index 8e773902..366fee65 100644 --- a/microsoft/skills/review/al-data-modeling-review.md +++ b/microsoft/skills/review/al-data-modeling-review.md @@ -47,6 +47,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 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`. diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index 479319b2..844198a7 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -48,6 +48,7 @@ The following targeted checks cover every current `error-handling` article: - `[ErrorBehavior(ErrorBehavior::Collect)]`, `ErrorInfo.Collectible`, `HasCollectedErrors`, `GetCollectedErrors`, or `ClearCollectedErrors` is added or changed, especially when errors are collected without later surfacing/clearing them — `collect-validation-errors-with-errorbehavior`. - New or changed code calls `Session.StartSession` from within error/duration logging around a web-service call, background job, or other operation expected to fail — `log-writes-must-survive-rollback`. +- A guarded lookup (`if Record.Get(...) then ... else` or similar) sets a value used later, and the same guard shape (with the same blank/zero fallback style) is applied to a field that feeds a posted amount, a tax/VAT calculation, a quantity or price actually used in a transaction, or a legally/compliance-facing output — `defensive-vs-offensive-code-must-match-blast-radius`. The signal is a posting-critical or compliance-facing field guarded defensively with a silent fallback, not the mere presence of a guarded lookup. - Developer-only invariant text is raised with default client visibility, or a user-actionable validation is hidden as `ErrorType::Internal` — `errortype-internal-vs-client-for-diagnostics`. - `FieldError` receives a complete capitalized sentence, repeats the field caption/value, or ends the predicate with punctuation — `fielderror-default-message-logic`. - An unguarded `FieldError` is used as though it performed a comparison, or `TestField` is forced onto a complex rule needing a tailored predicate — `fielderror-vs-testfield`. diff --git a/microsoft/skills/review/al-security-review.md b/microsoft/skills/review/al-security-review.md index 31f274ec..5fcbe78b 100644 --- a/microsoft/skills/review/al-security-review.md +++ b/microsoft/skills/review/al-security-review.md @@ -49,6 +49,7 @@ For secret values, select the most specific sink owner: - When a `Text`/`Code` credential is declared, passed, returned, or unwrapped without a visible HTTP URI/header/body sink, use `secrettext-for-credentials.md`. - When that value is interpolated into a URI, authorization header, or HTTP body and sent through `HttpClient`, use `secrettext-with-httpclient.md` as the primary finding. It supersedes the generic credential-type article at that location; keep the latter only as a supporting reference when useful. +- When a page/query is registered in Web Services, declares `PageType = API`/`QueryType = API`, a codeunit is registered in Web Services, or `[ServiceEnabled]` is added to a page procedure, and no permission set in the app grants a matching `page "..." = X` / `query "..." = X` / `codeunit "..." = X` entry for that specific object — use `exposed-objects-must-be-in-a-permission-set.md`. The anti-pattern is the exposed object missing its own execute entry, even when the underlying table's `tabledata` permissions look complete; require repository-level permission-set context, since one file cannot prove an entry is absent elsewhere in the app. 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`. diff --git a/microsoft/skills/review/al-style-review.md b/microsoft/skills/review/al-style-review.md index bffef4de..31fcf12a 100644 --- a/microsoft/skills/review/al-style-review.md +++ b/microsoft/skills/review/al-style-review.md @@ -51,6 +51,9 @@ 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. - `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 or changed comment restates what the adjacent code already makes obvious from its own names and structure (a comment that just repeats a variable/field/method name in prose) rather than explaining a non-obvious constraint, invariant, or workaround — `al-comments-must-not-restate-what-code-already-shows.md`. A comment absent entirely is not this anti-pattern; only a present-but-redundant comment is. +- A `page`/`pageextension` adds or changes a procedure body that performs a calculation, validation, or record mutation belonging to a business operation reused across entry points, rather than presentation-specific state or a call into a codeunit — `pages-must-not-contain-business-logic.md`. A page calling a codeunit procedure, or a page's own presentation-only state and formatting, is not this anti-pattern; nor is a data invariant that belongs on the table itself. +- Changed source files are added under an object-type folder (`Tables/`, `Pages/`, `Codeunits/`, etc.) in a repository whose existing structure is predominantly feature-based, or vice versa — `source-organized-by-feature-not-object-type.md`. The anti-pattern is inconsistency with the repository's own established convention, not the choice of either scheme; a repository consistently organized by object type throughout is not a violation. Require repository-level folder context; a single new file's path cannot prove the project's convention alone. Once the candidate worklist is known, resolve layer-precedence conflicts per READ and record suppressions. diff --git a/microsoft/skills/review/al-testing-review.md b/microsoft/skills/review/al-testing-review.md index 37243d4d..fff6abd8 100644 --- a/microsoft/skills/review/al-testing-review.md +++ b/microsoft/skills/review/al-testing-review.md @@ -46,9 +46,11 @@ A file enters the candidate worklist when its `keywords` intersect the extracted The following targeted checks cover every current `testing` article. Treat each as a candidate-selection cue: when the signal appears in changed code, add the named article to the worklist and evaluate it in Action. - A method in a `Subtype = Test` codeunit adds or changes `[TransactionModel(...)]`, exercises code that calls `Commit` under `AutoRollback`, defaults broadly to `AutoCommit`, or chooses `None` for a writing test — `transactionmodel-attribute-governs-test-transactions`. -- A test procedure or comment adds `[FEATURE]`/`[SCENARIO]`/`[GIVEN]`/`[WHEN]`/`[THEN]` tags — `test-feature-scenario-tags`. -- A test codeunit calls `TestPage` methods (`OpenNew`, `OpenView`, `OpenEdit`) — `ui-test-codeunit-naming`. -- A `[GIVEN]`-tagged setup precedes a posting call or report execution — `given-blocks-must-cover-full-precondition-chain`. +- A new or changed `[Test]` procedure is added, whether or not it already carries `[FEATURE]`/`[SCENARIO]`/`[GIVEN]`/`[WHEN]`/`[THEN]` tags — `test-feature-scenario-tags`. A procedure with no tags at all, or a generic name like `Test1`, is the anti-pattern signal; presence of the tags is the compliant shape, not the thing to search for. +- A test codeunit calls `TestPage` methods (`OpenNew`, `OpenView`, `OpenEdit`) alongside `[Test]` procedures in the same codeunit that call business-logic procedures directly with no `TestPage` involved — `ui-test-codeunit-naming`. The anti-pattern signal is both kinds of test mixed into one codeunit (or, on a project using the `_UT` convention, a UI-layer codeunit missing the suffix); a codeunit containing only `TestPage`-driven tests is not itself a violation. +- A `[GIVEN]`-tagged setup precedes a posting call or report execution and does not visibly set up posting-group/VAT setup records, an explicit date, or (for a report test) both an included and an excluded record — `given-blocks-must-cover-full-precondition-chain`. +- A test procedure contains more than one `[WHEN]` block, or more than one distinct action not labelled `[GIVEN]`, without the procedure name declaring a flow/defect-then-fix shape — `test-one-when-per-test`. +- A `BCPT*` scenario codeunit is added and the PerformanceTest app's only other scenario codeunits are copies of Microsoft's shipped BCPT samples (`BCPT Create Customer`, `BCPT Create Item Journal`, `BCPT Post GL Entries`, etc.) with no scenario exercising the extension's own codeunits, FlowFields, or pages — `bcpt-scenarios-must-be-app-specific`. - An `AutoCommit` test runs under a `Subtype = TestRunner` codeunit that omits `TestIsolation` or sets it to `Disabled`, leaving committed data between tests — `testisolation-belongs-on-the-test-runner`. Require runner/repository context; a standalone test file cannot prove which runner executes it. - A permission-sensitive test uses `TestPermissions = Disabled`, claims to test a restricted user without `"Permissions Mock"`/`"Library - Lower Permissions"`, or declares `[TestPermissions(...)]` without applying that context — `permission-tests-must-lower-the-execution-context`. - 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`. diff --git a/microsoft/skills/review/al-ui-review.md b/microsoft/skills/review/al-ui-review.md index c3113a5a..78b86c3a 100644 --- a/microsoft/skills/review/al-ui-review.md +++ b/microsoft/skills/review/al-ui-review.md @@ -40,6 +40,7 @@ Discard files that are not applicable. Retain conditionally applicable files onl Narrow the relevant files to the subset that applies to the changes under review. - **UI-file filter.** UI review applies to files declaring `page`, `pageextension`, or `pagecustomization`, and to JavaScript/CSS/HTML that implements a control add-in's rendering or Business Central communication. When the diff contains no such files, return `outcome: "not-applicable"` without evaluating knowledge files. +- A new or changed page's name/suffix, primary-key handling, `CardPageID`, `SubPageLink`, `AutoSplitKey`, or `UsageCategory` doesn't match the conventions of its own declared `PageType` — `page-design-must-match-bc-page-type-conventions.md`. A Card page over a composite-key table that supplements a master record, or a supporting/subpage/dialog page intended only to be reached through another workflow and correctly omitting `UsageCategory`, is not this anti-pattern on its own; check whether the page is actually mixing conventions or is meant as a searchable entry point before flagging. - For each relevant knowledge file, compute overlap against changed page declarations and control add-in files, weighted toward `Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `OptionCaption`, `ShowCaption`, `InstructionalText`, `GridLayout`, `Style`, `StyleExpr`, promoted action definitions, field importance, page background tasks, DOM creation, ARIA attributes, keyboard/focus handlers, packaged-resource AJAX, and calls from JavaScript into AL. - Tokens extracted from the diff (`Caption`, `ToolTip`, `AboutTitle`, `AboutText`, `PageType`, `ShowCaption`, `InstructionalText`, `grid`, `fixed`, `GridLayout`, `Style`, `StyleExpr`, `Importance`, `Promoted`, `Additional`, `area(Promoted)`, `actionref`, `PromotedCategory`, `PromotedOnly`, `PromotedIsBig`, `ShowAs`, `SplitButton`, `EnqueueBackgroundTask`, `OnAfterGetCurrRecord`, `OnAfterGetRecord`, `OnPageBackgroundTaskCompleted`, `OnPageBackgroundTaskError`, `RunPageBackgroundTask`, `Favorable`, `Unfavorable`, `Ambiguous`, `cuegroup`, `controladdin`, `control-add-in`, `usercontrol`, `aria-`, `tabindex`, `keydown`, `focus`, `innerHTML`, `createElement`, `packaged-resource`, `ajax`, `$.get`, `$.ajax`, `XMLHttpRequest`, `xhrFields`, `withCredentials`, `withcredentials`, `InvokeExtensibilityMethod`, `invokeextensibilitymethod`, `skipIfBusy`, `successCallback`, `success-callback`, `errorCallback`, `setInterval`, `JSON.stringify`, `payload`, `throttling`, `reduced-functionality`, `ClientServicesMaxUploadSize`, `&`, `Specifies`, `Message(`, `Confirm(`, `Error(` in a page context, `Disabled`, `Invalid`, `Whitelist`, `Blacklist`, trailing punctuation patterns on captions, `CardPageID`, `AutoSplitKey`, `PageType = Card`, `PageType = List`, `PageType = Worksheet`, `PageType = Document`). diff --git a/microsoft/skills/review/al-upgrade-review.md b/microsoft/skills/review/al-upgrade-review.md index 667ea32e..01887974 100644 --- a/microsoft/skills/review/al-upgrade-review.md +++ b/microsoft/skills/review/al-upgrade-review.md @@ -43,6 +43,7 @@ Narrow the relevant files to the subset that applies to the changes under review - For each `OnCheckPreconditions...` and `OnValidateUpgrade...` trigger, build the best available call graph from surrounding unchanged source as well as changed hunks, tracing resolved calls through reachable local or internal helpers. Worklist the check-only rule when a database write occurs either directly in the trigger or in any helper procedure reachable from it. Writes include `Insert`, `Modify`, `ModifyAll`, `Delete`, `DeleteAll`, `Rename`, and `DataTransfer`. Also perform the reverse check when a PR changes a writing helper body: worklist the rule when that helper is invoked directly or transitively by an unchanged check or validation trigger. - Treat a direct write or a fully resolved call chain as high-confidence evidence. When cross-object dispatch, unavailable declarations, or an incomplete call graph prevents proving the complete chain, cap confidence at `medium`, name the unresolved edge in the finding, and do not claim a violation without a resolved path from a check or validation trigger to a write. - Worklist the install-versus-upgrade rule when migration helpers are reachable only from an install codeunit. +- An upgrade-tag procedure nests a record loop or a multi-branch business-data condition inside the tag-check/exit guard, going past the tag-check-then-exit-then-single-upgrade-action shape, or one procedure mixes the gated logic for more than one distinct upgrade tag — `upgrade-tag-logic-must-not-nest-deeply.md`. A single `if UpgradeTag.HasUpgradeTag(...) then exit;` guard followed by one flat upgrade action (even one that loops over records to apply that single action) is the compliant shape, not the signal to flag — the anti-pattern is a buried, separately-conditioned business decision nested inside that action. A file enters the candidate worklist when its `keywords` intersect the extracted tokens or its topic (derived from the index entry's `path`, `title`, and `description`) matches a changed object type. Read an article's full file — its `## Best Practice` / `## Anti Pattern` bodies — only after it makes the worklist; candidate selection uses the index alone. When the diff contains no upgrade-related changes by any of the above signals, return `outcome: "not-applicable"` without evaluating files. diff --git a/microsoft/skills/review/al-web-services-review.md b/microsoft/skills/review/al-web-services-review.md index cfa6fdda..f31f258b 100644 --- a/microsoft/skills/review/al-web-services-review.md +++ b/microsoft/skills/review/al-web-services-review.md @@ -40,6 +40,7 @@ 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`) is added or changed and its `InsertAllowed`/`ModifyAllowed`/`DeleteAllowed` properties are left at their default `true` (or explicitly set `true`) for an operation the endpoint's stated purpose does not need — `api-page-least-privilege-write-access.md`. The signal is an operation left enabled beyond what the endpoint's own described purpose requires, not the mere presence of these properties; a page that genuinely needs full CRUD and grants it deliberately is not a violation. - 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.