18 more AL/BC patterns: data-modeling, testing, style, security, error-handling, ui, upgrade, web-services, appsource - #157
Conversation
… 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.
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
There are useful principles in this batch, but it is not ready for the Microsoft layer yet. I verified several concrete platform/source contradictions inline.
There are also two cross-cutting blockers:
- None of the 18 articles is wired into the corresponding
microsoft/skills/review/al-*-review.mdworklist. The current contracts say their targeted checks cover every current article, but no changed skill names any new slug and several domains' relevance gates would returnnot-applicablebefore considering these topics. Please add explicit signals/targeted checks and coordinate with #155's coverage contract so agents can reliably consume the knowledge. - Several entries are CURABIS house conventions presented as universal Microsoft requirements: feature-vs-object folder layout, exactly one WHEN,
_UTmeaning UI-layer tests, adjacent object IDs, and a hard two-level nesting limit. These can be useful team guidance, but they need authoritative BC-wide grounding and careful scoping before an agent may emit gating findings from the Microsoft layer; otherwise they belong in a custom/community layer.
The structural validators pass (with two keyword-count warnings), but they do not compile the AL fixtures or validate the claims. The CLA is still pending; because this is generalized from CURABIS work, the contributor must use the applicable employer/company declaration and confirm authorization to contribute it.
|
@microsoft-github-policy-service agree [company="CURABIS ApS"] |
|
@microsoft-github-policy-service agree company="CURABIS ApS" |
- 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 <noreply@anthropic.com>
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Follow-up after 81845a5: the version ordering, API restrictions, FEATURE/SCENARIO placement, _UT/object-ID claims, composite-key Card guidance, and several earlier fixture issues are improved. The remaining correctness issues are inline below.
The deterministic testing worklist is also incomplete: only a subset of the 18 new rules has an explicit cue, and several cues recognize compliant code rather than the anti-pattern. Please ensure every new rule can be selected from the changed-code signals that indicate a possible violation. The invented API comment below applies to all new good fixtures; they should be compiled against the declared dependencies rather than treated as pseudocode.
- 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 <noreply@anthropic.com>
|
Jesper Schulz-Wedde (@JesperSchulz) Addressed this round in 30abf07 (pushed 2026-09-08): the 9 file-level issues above (individual replies inline), plus two more invented-API fixtures found in the same sweep that weren't in your review ( |
Jesper Schulz-Wedde (JesperSchulz)
left a comment
Jesper Schulz-Wedde (JesperSchulz)
left a comment
There was a problem hiding this comment.
Thank you for another substantial round of corrections. Most of the previous findings are now resolved, and this PR is much closer.
During verification of the updated examples, we found a small number of correctness issues that the structural validators cannot detect—for example, AL that does not compile, test setup that never activates the data it intends to exercise, or guidance that differs from the current platform behavior. We recognize that another round is inconvenient, and we are not trying to prolong the review. These articles will be consumed as authoritative instructions by automated agents, so an incorrect “good” fixture or overly broad rule can be repeated across many future changes. We have therefore limited this round to the material issues below and omitted optional editorial cleanup.
| @@ -0,0 +1,23 @@ | |||
| page 50102 "Vendor Contact Info API" | |||
| { | |||
| PageType = API; | |||
There was a problem hiding this comment.
The API-page examples are still missing mandatory EntityName and EntitySetName properties. Please add distinct singular and plural camelCase values to both the good and bad fixtures and compile them against the normal Base Application symbols. This matters because the files are presented as complete API page objects, not abbreviated fragments.
| { | ||
| procedure RecalculateLine(var SalesLine: Record "Sales Line") | ||
| begin | ||
| SalesLine."Total Amount" := SalesLine.Quantity * SalesLine."Unit Price"; |
There was a problem hiding this comment.
Sales Line has no field named "Total Amount" in the current Base Application, so both this good fixture and the corresponding bad fixture do not compile. Please use an actual field appropriate to the example—such as Amount, "Line Amount", or a declared custom field/table—while preserving the intended page-versus-domain-logic lesson.
| // [GIVEN] a customer and an item with a customer-specific sales price list line | ||
| LibrarySales.CreateCustomer(Customer); | ||
| LibraryInventory.CreateItem(Item); | ||
| LibraryPriceCalculation.CreatePriceHeader( |
There was a problem hiding this comment.
The replacement APIs now exist, thanks. One behavioral step is still missing: CreatePriceHeader creates a Draft price list, and Draft lists are not used for price calculation. Please validate the header to "Price Status"::Active and persist it before creating the sales line. The same setup is duplicated in test-one-when-per-test.good.al and needs the same correction; otherwise the “good” tests do not exercise the price list they assert against.
| 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`. |
There was a problem hiding this comment.
The execute-permission correction is good, but a published codeunit is not an OData service. Current Business Central supports Codeunit and Page for SOAP publication, while Page and Query are the OData object types. Please describe published codeunits as SOAP services and recommend API pages/queries for new REST integrations. This distinction is especially important now that SOAP is deprecated.
| 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`. |
There was a problem hiding this comment.
This cue still selects the article from Session.StartSession, which is the compliant remedy, while the bad fixture contains no such call. Please cue on the actual risk: inserting a failure/error log in the current transaction around a failed TryFunction/GetLastError* path and then raising, propagating, or rethrowing the error. StartSession can then be treated as a compliant discriminator rather than the entry condition.
|
|
||
| Business Central's page types — RoleCenter, Card, List, CardPart, | ||
| ListPart, Worksheet, Document, ListPlus, plus the system dialog types | ||
| (Navigate, ConfirmationDialog, StandardDialog, HeadlinePart, API) — each |
There was a problem hiding this comment.
Because this is marked [all] and reads as a page-type catalogue, please use the exact current enum name NavigatePage, not Navigate, and either include current types such as PromptDialog, ConfigurationDialog, and UserControlHost or state clearly that this is only a selected list of conventional types. Otherwise an agent can reject a valid current PageType or suggest an invalid one.
2 participants
Summary
Second batch from CURABIS ApS — 18 more knowledge articles generalized from patterns observed across real AppSource/PTE Business Central development, follow-up to #156. Same format (6-key frontmatter, Description/Best Practice/Anti Pattern, sibling
.good.al/.bad.alsamples where a code contrast helps, no fenced code in the .md), targeted at existing Microsoft-owned domains rather than/community/._UTsuffix for UI-layer test codeunits, GIVEN-block precondition completeness for posting/report scenarios, one WHEN per test (with flow-test and defect-then-fix exceptions).Checked against the current
microsoft/knowledge/corpus before opening — four originally-drafted articles were dropped as duplicates or substantial overlaps of existing files (xrec-is-a-before-image-only-in-some-triggers.md,use-library-codeunits-for-test-fixtures.md+asserterror-needs-expectederror-and-code.md,choose-telemetry-scope-by-audience.md,compose-permission-sets-with-included-sets.md).Test plan
🤖 Generated with Claude Code