mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
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 <noreply@anthropic.com>
This commit is contained in:
parent
a4d85c3e9e
commit
0120b874b2
22 changed files with 111 additions and 58 deletions
|
|
@ -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`.
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue