diff --git a/microsoft/knowledge/appsource/release-must-update-app-version.md b/microsoft/knowledge/appsource/release-must-update-app-version.md index d7debe8..fb8f483 100644 --- a/microsoft/knowledge/appsource/release-must-update-app-version.md +++ b/microsoft/knowledge/appsource/release-must-update-app-version.md @@ -19,7 +19,7 @@ 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. 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. +The app's stable identity is its `id` in `app.json`; the version identifies which release — which code state — of that app is deployed. 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 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 daf8259..3e2ea0e 100644 --- a/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md +++ b/microsoft/knowledge/data-modeling/code-must-not-change-workdate.md @@ -42,7 +42,7 @@ 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`. +See sample: [`code-must-not-change-workdate.good.al`](code-must-not-change-workdate.good.al). ## Anti Pattern @@ -55,4 +55,4 @@ 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`. +See sample: [`code-must-not-change-workdate.bad.al`](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 b9db6bd..1767a0d 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 @@ -33,7 +33,7 @@ file attachment blob unrelated to picture rendering). 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`. +See sample: [`pictures-must-use-media-not-blob.good.al`](pictures-must-use-media-not-blob.good.al). ## Anti Pattern @@ -46,4 +46,4 @@ 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`. +See sample: [`pictures-must-use-media-not-blob.bad.al`](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 e3b4efd..77ff09a 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 @@ -72,7 +72,7 @@ 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`. +See sample: [`table-design-must-match-bc-table-type-conventions.good.al`](table-design-must-match-bc-table-type-conventions.good.al). ## Anti Pattern @@ -83,4 +83,4 @@ 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`. +See sample: [`table-design-must-match-bc-table-type-conventions.bad.al`](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.good.al b/microsoft/knowledge/error-handling/defensive-vs-offensive-code-must-match-blast-radius.good.al index 19fa2ad..35f88ee 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,12 @@ // Low blast radius: guard, with an explicit chosen fallback. if Customer.Get(SalesHeader."Sell-to Customer No.") then - CustomerHomePage := Customer."Home Page"; + CustomerHomePage := Customer."Home Page" +else + CustomerHomePage := ''; // Blank is an acceptable, deliberately-considered default here - the field // is purely a display convenience and a reviewer sees it before the document ships. +// It is assigned explicitly, though, not left to whatever the variable +// happened to hold before this lookup ran. // 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 14b5217..91a8aff 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 @@ -17,10 +17,10 @@ Whether code should guard gracefully (defensive) or fail loudly (offensive/fail- 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`. +See sample: [`defensive-vs-offensive-code-must-match-blast-radius.good.al`](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 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`. +See sample: [`defensive-vs-offensive-code-must-match-blast-radius.bad.al`](defensive-vs-offensive-code-must-match-blast-radius.bad.al). 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 e9f94ad..eae1700 100644 --- a/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md +++ b/microsoft/knowledge/error-handling/log-writes-must-survive-rollback.md @@ -17,10 +17,10 @@ Inserting a log record inside the same transaction as the operation it logs look 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`. +See sample: [`log-writes-must-survive-rollback.good.al`](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`. +See sample: [`log-writes-must-survive-rollback.bad.al`](log-writes-must-survive-rollback.bad.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 3b325c4..fd933d7 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 @@ -14,7 +14,7 @@ application-area: [all] 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`. +- A codeunit published through **Web Services** exposes *every* public procedure on it as a SOAP operation automatically — there is no per-method attribute to add. SOAP web service support is deprecated and scheduled for removal; prefer publishing an API page/query for a new integration rather than a new codeunit web service. The permission target for an existing published codeunit 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. @@ -23,10 +23,10 @@ When such an object is left out of every permission set, it becomes both unusabl 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`. +See sample: [`exposed-objects-must-be-in-a-permission-set.good.al`](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`. +See sample: [`exposed-objects-must-be-in-a-permission-set.bad.al`](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.md b/microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md index 2ff120a..09a2ea4 100644 --- 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 @@ -21,10 +21,10 @@ This does not override required structural documentation — feature/scenario te 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`. +See sample: [`al-comments-must-not-restate-what-code-already-shows.good.al`](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`. +See sample: [`al-comments-must-not-restate-what-code-already-shows.bad.al`](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 index 333625d..1a1eece 100644 --- a/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.bad.al @@ -11,7 +11,7 @@ page 50100 "Sales Line Card" { trigger OnAction() begin - Rec."Total Amount" := Rec.Quantity * Rec."Unit Price"; + Rec."Line 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 index a7a40b1..70c6829 100644 --- a/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.good.al @@ -2,7 +2,7 @@ codeunit 50100 "Sales Line Management" { procedure RecalculateLine(var SalesLine: Record "Sales Line") begin - SalesLine."Total Amount" := SalesLine.Quantity * SalesLine."Unit Price"; + SalesLine."Line Amount" := SalesLine.Quantity * SalesLine."Unit Price"; SalesLine.Modify(); end; } 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 f067c23..3326a25 100644 --- a/microsoft/knowledge/style/pages-must-not-contain-business-logic.md +++ b/microsoft/knowledge/style/pages-must-not-contain-business-logic.md @@ -22,10 +22,10 @@ A narrow set of patterns are conventional rather than violations: 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`. +See sample: [`pages-must-not-contain-business-logic.good.al`](pages-must-not-contain-business-logic.good.al). ## Anti Pattern 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`. +See sample: [`pages-must-not-contain-business-logic.bad.al`](pages-must-not-contain-business-logic.bad.al). 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 index 0ca29de..980a4be 100644 --- a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.good.al @@ -13,22 +13,38 @@ codeunit 50100 "BCPT Create Service Request" implements "BCPT Test Param. Provid var GlobalBCPTTestContext: Codeunit "BCPT Test Context"; + CustomerNo: Code[20]; + NextNo: Integer; IsInitialized: Boolean; local procedure InitTest() + var + Customer: Record Customer; begin - // Set up any required configuration + Customer.FindFirst(); + CustomerNo := Customer."No."; end; local procedure CreateServiceRequest(var BCPTTestContext: Codeunit "BCPT Test Context") + var + ServiceRequestHeader: Record "Service Request Header"; + ServiceRequestLine: Record "Service Request Line"; begin BCPTTestContext.StartScenario('Create Service Request Header'); - // ... create the service request + NextNo += 1; + ServiceRequestHeader.Init(); + ServiceRequestHeader."No." := CopyStr(Format(NextNo), 1, MaxStrLen(ServiceRequestHeader."No.")); + ServiceRequestHeader.Validate("Customer No.", CustomerNo); + ServiceRequestHeader.Insert(true); BCPTTestContext.EndScenario('Create Service Request Header'); BCPTTestContext.UserWait(); BCPTTestContext.StartScenario('Add Service Request Line'); - // ... add a line + ServiceRequestLine.Init(); + ServiceRequestLine."Document No." := ServiceRequestHeader."No."; + ServiceRequestLine."Line No." := 10000; + ServiceRequestLine.Description := 'Performance test line'; + ServiceRequestLine.Insert(true); BCPTTestContext.EndScenario('Add Service Request Line'); 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 index 1269b3e..51ebddd 100644 --- a/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md +++ b/microsoft/knowledge/testing/bcpt-scenarios-must-be-app-specific.md @@ -17,10 +17,10 @@ A PerformanceTest app that ships with only the generic Microsoft BCPT samples (c 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`. +See sample: [`bcpt-scenarios-must-be-app-specific.good.al`](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`. +See sample: [`bcpt-scenarios-must-be-app-specific.bad.al`](bcpt-scenarios-must-be-app-specific.bad.al). 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 index 71e63e5..b34adfe 100644 --- a/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md +++ b/microsoft/knowledge/testing/given-blocks-must-cover-full-precondition-chain.md @@ -17,10 +17,10 @@ A `[GIVEN]` block is only correct if it sets up every precondition the code unde 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`. +See sample: [`given-blocks-must-cover-full-precondition-chain.good.al`](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`. +See sample: [`given-blocks-must-cover-full-precondition-chain.bad.al`](given-blocks-must-cover-full-precondition-chain.bad.al). diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al index fdbb76b..b2cbf06 100644 --- a/microsoft/knowledge/testing/test-feature-scenario-tags.good.al +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.good.al @@ -28,6 +28,9 @@ codeunit 50103 "Item Price Testing" LibraryPriceCalculation.CreateSalesPriceLine( PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", "Price Asset Type"::Item, Item."No."); + // CreatePriceHeader leaves the list in Draft status, which price calculation ignores. + PriceListHeader.Validate(Status, PriceListHeader.Status::Active); + PriceListHeader.Modify(true); // [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); diff --git a/microsoft/knowledge/testing/test-feature-scenario-tags.md b/microsoft/knowledge/testing/test-feature-scenario-tags.md index f9a2754..2247c7e 100644 --- a/microsoft/knowledge/testing/test-feature-scenario-tags.md +++ b/microsoft/knowledge/testing/test-feature-scenario-tags.md @@ -17,10 +17,10 @@ Test codeunits are easier to trust and to review when they carry a four-level co 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`. +See sample: [`test-feature-scenario-tags.good.al`](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`. +See sample: [`test-feature-scenario-tags.bad.al`](test-feature-scenario-tags.bad.al). 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 cb61708..6b58c51 100644 --- a/microsoft/knowledge/testing/test-one-when-per-test.good.al +++ b/microsoft/knowledge/testing/test-one-when-per-test.good.al @@ -16,6 +16,9 @@ begin LibraryPriceCalculation.CreateSalesPriceLine( PriceListLine, PriceListHeader.Code, "Price Source Type"::Customer, Customer."No.", "Price Asset Type"::Item, Item."No."); + // CreatePriceHeader leaves the list in Draft status, which price calculation ignores. + PriceListHeader.Validate(Status, PriceListHeader.Status::Active); + PriceListHeader.Modify(true); // [WHEN] LibrarySales.CreateSalesDocumentWithItem( SalesHeader, SalesLine, SalesHeader."Document Type"::Order, Customer."No.", Item."No.", 1, '', 0D); diff --git a/microsoft/knowledge/testing/test-one-when-per-test.md b/microsoft/knowledge/testing/test-one-when-per-test.md index d52a97a..69c3b37 100644 --- a/microsoft/knowledge/testing/test-one-when-per-test.md +++ b/microsoft/knowledge/testing/test-one-when-per-test.md @@ -17,13 +17,13 @@ This is a testing-design practice, not a BC platform requirement — no AL API e 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`. +See sample: [`test-one-when-per-test.good.al`](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`. +See sample: [`test-one-when-per-test.bad.al`](test-one-when-per-test.bad.al). ## Flow tests — a deliberate exception diff --git a/microsoft/knowledge/testing/ui-test-codeunit-naming.md b/microsoft/knowledge/testing/ui-test-codeunit-naming.md index 1cc332a..0790f37 100644 --- a/microsoft/knowledge/testing/ui-test-codeunit-naming.md +++ b/microsoft/knowledge/testing/ui-test-codeunit-naming.md @@ -17,10 +17,10 @@ A test codeunit that drives pages through `TestPage` — opening pages, reading 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`. +See sample: [`ui-test-codeunit-naming.good.al`](ui-test-codeunit-naming.good.al). ## Anti Pattern 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`. +See sample: [`ui-test-codeunit-naming.bad.al`](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 d3628d8..18f7178 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 @@ -12,8 +12,13 @@ application-area: [all] ## 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 +ListPart, Worksheet, Document, ListPlus, plus system dialog/special +types such as `NavigatePage`, `ConfirmationDialog`, `StandardDialog`, +`HeadlinePart`, and `API` (a selected list of conventional types this +article covers design conventions for — not an exhaustive catalogue of +every current `PageType` value; `PromptDialog`, `ConfigurationDialog`, +`UserControlHost`, and `XmlPort` also exist but follow their own +design rules, out of scope here) — 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 @@ -56,7 +61,7 @@ Match the page's design to its type: 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`, +- System dialog types (`NavigatePage`, `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. @@ -66,7 +71,7 @@ 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`. +See sample: [`page-design-must-match-bc-page-type-conventions.good.al`](page-design-must-match-bc-page-type-conventions.good.al). ## Anti Pattern @@ -82,4 +87,4 @@ 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`. +See sample: [`page-design-must-match-bc-page-type-conventions.bad.al`](page-design-must-match-bc-page-type-conventions.bad.al). 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 index a6f09a4..1c0704b 100644 --- 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 @@ -3,12 +3,26 @@ begin if UpgradeTag.HasUpgradeTag(GetCustomerDiscountFieldTag()) then exit; - Customer.SetLoadFields("Discount %"); + Customer.SetLoadFields("Discount %", "Customer Posting Group"); if Customer.FindSet() then repeat - Customer."Discount %" := 5; - Customer.Modify(); + SetDefaultDiscountIfEligible(Customer); until Customer.Next() = 0; UpgradeTag.SetUpgradeTag(GetCustomerDiscountFieldTag()); end; + +local procedure SetDefaultDiscountIfEligible(var Customer: Record Customer) +begin + // Both safety conditions from the original logic are preserved, just + // flattened into early exits instead of nested ifs: don't overwrite an + // already-set discount, and don't touch a customer with no posting + // group configured yet. + if Customer."Discount %" <> 0 then + exit; + if Customer."Customer Posting Group" = '' then + exit; + + Customer."Discount %" := 5; + Customer.Modify(); +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 index f13c4b8..1bc976f 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 @@ -19,13 +19,13 @@ Upgrade code runs unattended, once, against production data with no chance to in One tag check, one exit, one upgrade action — two levels deep at most. -See sample: `upgrade-tag-logic-must-not-nest-deeply.good.al`. +See sample: [`upgrade-tag-logic-must-not-nest-deeply.good.al`](upgrade-tag-logic-must-not-nest-deeply.good.al). ## Anti Pattern Nesting the tag check, a record loop, and a multi-branch business condition inside one procedure. Split the buried business condition into its own, separately tagged upgrade step instead. -See sample: `upgrade-tag-logic-must-not-nest-deeply.bad.al`. +See sample: [`upgrade-tag-logic-must-not-nest-deeply.bad.al`](upgrade-tag-logic-must-not-nest-deeply.bad.al). ## Source 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 index 272fdf6..13c33b4 100644 --- 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 @@ -4,6 +4,8 @@ page 50100 "Vendor Document API" APIPublisher = 'contoso'; APIGroup = 'documents'; APIVersion = 'v1.0'; + EntityName = 'vendorDocument'; + EntitySetName = 'vendorDocuments'; SourceTable = Vendor; // no InsertAllowed/ModifyAllowed override, no Editable = false anywhere 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 e94f7f2..b524b4f 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 @@ -4,6 +4,8 @@ page 50102 "Vendor Contact Info API" APIPublisher = 'contoso'; APIGroup = 'integration'; APIVersion = 'v1.0'; + EntityName = 'vendorContact'; + EntitySetName = 'vendorContacts'; SourceTable = Vendor; DelayedInsert = true; InsertAllowed = false; 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 a06fa35..6f40c87 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 @@ -17,10 +17,10 @@ A general-purpose API page that exposes many fields should not be widened to all 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`. +See sample: [`api-page-least-privilege-write-access.good.al`](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`. +See sample: [`api-page-least-privilege-write-access.bad.al`](api-page-least-privilege-write-access.bad.al). diff --git a/microsoft/skills/review/al-error-handling-review.md b/microsoft/skills/review/al-error-handling-review.md index 778e4e2..17ca28c 100644 --- a/microsoft/skills/review/al-error-handling-review.md +++ b/microsoft/skills/review/al-error-handling-review.md @@ -47,7 +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`. +- New or changed code inserts an error/duration log record around a failed `TryFunction`/`GetLastErrorText`/`GetLastErrorCode` path and then raises, propagates, or rethrows the error — `log-writes-must-survive-rollback`. Do not worklist it when the log insert already happens inside a `Session.StartSession`-targeted codeunit's `OnRun`; that is the compliant shape, not the signal to flag. - 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`.