mirror of
https://github.com/microsoft/BCQuality.git
synced 2026-10-05 14:46:55 +01:00
Fix remaining correctness issues from Jesper's 2026-09-15 re-review
- dimension-management-wiring: SaveDefaultDim's third argument is the shortcut dimension number (1-8), not the field's AL field ID; the fixture passed FieldNo(...) = 10. GetDefaultDimID's InheritFromDimSetID must be 0 when recomputing after the linking record changes, not the document's existing Dimension Set ID (which would retain the previous customer's leftover dimensions). Verified against DimensionManagement.Codeunit.al and BankDepositHeader.Table.al in the BCApps reference clone. - check-post-line-batch-pattern: "Post Line writes exactly one line to the ledger" overclaimed - Gen. Jnl.-Post Line alone calls InsertGLEntry from a dozen call sites (balancing entry, VAT, currency rounding, deferrals) and can write several G/L Entries per journal line. Reworded to "posts exactly one journal line" and softened the "distinct, non-overlapping responsibilities" absolute. - namespace-must-be-verified-from-source.bad.al: dropped the "resolves in a local build, fails in VS Code" comment (taught an inherent compiler/language-server disagreement that isn't real); reframed as stale/cached symbols, matching the prose fix already made. - file-datatype-saas.good.al: replaced the deprecated 5-argument UploadIntoStream overload with the current 2-argument one, and actually staged through TempBlob as the article's own Best Practice instructs (the declared TempBlob variable was previously unused). - test-data-must-be-random-and-complete: no longer treats a short-but-valid value as defective merely for being "underfilled" - AL field lengths are maxima, not minimums. Scoped to missing values or a scenario with an explicit length/format requirement (e.g. a truncation test). Updated the al-testing-review.md routing cue to match. - stored-derived-fields-must-not-be-exposed-directly: stopped mandating source-field exposure as part of the core pattern: the good fixture exposed only one of the derived value's two inputs (Hours Used, not Budgeted Hours), making the claimed "so the consumer can verify it" impossible. Reframed as an optional, all-or-nothing addition and fixed the fixture to expose both inputs. Rebased onto upstream/main to resolve conflicts in al-breaking-changes-review.md, al-data-modeling-review.md, al-performance-review.md, and al-style-review.md against merged PRs #148 and #153; all sides' worklist tokens/cues retained.
This commit is contained in:
Michael Dieringer
parent
a28ba1a1d7
commit
faa0bceb86
10 changed files with 27 additions and 14 deletions
|
|
@ -3,11 +3,18 @@ codeunit 50104 "Import File Reader"
|
|||
procedure ImportFile()
|
||||
var
|
||||
TempBlob: Codeunit "Temp Blob";
|
||||
FromInStream: InStream;
|
||||
ToOutStream: OutStream;
|
||||
InStream: InStream;
|
||||
FileName: Text;
|
||||
begin
|
||||
if UploadIntoStream('Import file', '', 'All Files (*.*)|*.*', FileName, InStream) then
|
||||
ParseStream(InStream);
|
||||
if not UploadIntoStream('All Files (*.*)|*.*', FromInStream) then
|
||||
exit;
|
||||
|
||||
TempBlob.CreateOutStream(ToOutStream);
|
||||
CopyStream(ToOutStream, FromInStream);
|
||||
|
||||
TempBlob.CreateInStream(InStream);
|
||||
ParseStream(InStream);
|
||||
end;
|
||||
|
||||
local procedure ParseStream(var InStream: InStream)
|
||||
|
|
|
|||
|
|
@ -14,7 +14,7 @@ codeunit 50101 "Meter Jnl.-Post Line"
|
|||
var
|
||||
MeterLedgEntry: Record "Meter Ledger Entry";
|
||||
begin
|
||||
// Writes exactly one ledger entry; never touches the Journal table.
|
||||
// Posts exactly one journal line; never touches the Journal table.
|
||||
MeterLedgEntry.Init();
|
||||
MeterLedgEntry.TransferFields(MeterJnlLine);
|
||||
MeterLedgEntry.Insert();
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
Business Central's own journal-based posting routines consistently follow a three-codeunit split with distinct, non-overlapping responsibilities — `Codeunit "Gen. Jnl.-Check Line"` / `"Gen. Jnl.-Post Line"` / `"Gen. Jnl.-Post Batch"` for the general journal, and the same `<Journal>-Check Line` / `<Journal>-Post Line` / `<Journal>-Post Batch` shape repeated for Item, Resource, Job, Fixed Asset, Insurance, and Cost Accounting journals: `Check Line` validates one line, `Post Line` writes exactly one line to the ledger, and `Post Batch` loops both across the journal. A document posting routine (posting one document at a time) calls `Post Line` directly and skips `Post Batch`. This is the standard shape to evaluate a new journal-based posting routine against, not a platform-enforced constraint — a routine with a genuinely different transaction/reuse shape may legitimately organize itself differently. But a new routine that blurs this split without a specific reason either misses functionality other code expects to call directly, or exposes an interaction surface it shouldn't.
|
||||
Business Central's own journal-based posting routines consistently follow a three-codeunit split, each with one primary responsibility — `Codeunit "Gen. Jnl.-Check Line"` / `"Gen. Jnl.-Post Line"` / `"Gen. Jnl.-Post Batch"` for the general journal, and the same `<Journal>-Check Line` / `<Journal>-Post Line` / `<Journal>-Post Batch` shape repeated for Item, Resource, Job, Fixed Asset, Insurance, and Cost Accounting journals: `Check Line` validates one line, `Post Line` posts exactly one journal line — `Gen. Jnl.-Post Line` itself can write more than one G/L Entry per call (a balancing entry, VAT, currency rounding, deferrals), so "exactly one line" describes its input, not a one-entry-out guarantee — and `Post Batch` loops both across the journal. A document posting routine (posting one document at a time) calls `Post Line` directly and skips `Post Batch`. This is the standard shape to evaluate a new journal-based posting routine against, not a platform-enforced constraint — a routine with a genuinely different transaction/reuse shape may legitimately organize itself differently, and the three codeunits' responsibilities are a useful default split, not a guarantee that every implementation keeps them non-overlapping. But a new routine that blurs this split without a specific reason either misses functionality other code expects to call directly, or exposes an interaction surface it shouldn't.
|
||||
|
||||
## Best Practice
|
||||
|
||||
|
|
|
|||
|
|
@ -15,7 +15,7 @@ table 50100 "Course"
|
|||
DimMgt: Codeunit DimensionManagement;
|
||||
begin
|
||||
DimMgt.ValidateDimValueCode(1, "Global Dimension 1 Code");
|
||||
DimMgt.SaveDefaultDim(Database::Course, "No.", FieldNo("Global Dimension 1 Code"), "Global Dimension 1 Code");
|
||||
DimMgt.SaveDefaultDim(Database::Course, "No.", 1, "Global Dimension 1 Code");
|
||||
end;
|
||||
}
|
||||
}
|
||||
|
|
@ -74,9 +74,14 @@ table 50101 "Course Registration Header"
|
|||
if not Customer.Get("Customer No.") then
|
||||
exit;
|
||||
|
||||
// Recompute from scratch (InheritFromDimSetID = 0): passing the existing
|
||||
// "Dimension Set ID" here would inherit dimensions from whichever record
|
||||
// the document was previously linked to, retaining them even after the
|
||||
// new customer's defaults have nothing for that dimension.
|
||||
"Shortcut Dimension 1 Code" := '';
|
||||
DimMgt.AddDimSource(DefaultDimSource, Database::Customer, "Customer No.");
|
||||
"Dimension Set ID" :=
|
||||
DimMgt.GetDefaultDimID(
|
||||
DefaultDimSource, '', "Shortcut Dimension 1 Code", GlobalDim2Code, "Dimension Set ID", 0);
|
||||
DefaultDimSource, '', "Shortcut Dimension 1 Code", GlobalDim2Code, 0, 0);
|
||||
end;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -15,7 +15,7 @@ application-area: [all]
|
|||
|
||||
Adding dimension support to a custom table is not just a matter of adding a `Code[20]` field, and master tables and document/transactional tables wire into `Codeunit "Dimension Management"` through two different models — treating them as one mechanism is itself the mistake this article corrects:
|
||||
|
||||
- **Master data** (a custom master table, e.g. "Course") persists **Default Dimension** records: each shortcut dimension field validates through `ValidateDimValueCode`, then the result is saved via `SaveDefaultDim`, and `DeleteDefaultDim` removes them again in `OnDelete`. The master record itself carries no `Dimension Set ID` field.
|
||||
- **Master data** (a custom master table, e.g. "Course") persists **Default Dimension** records: each shortcut dimension field validates through `ValidateDimValueCode`, then the result is saved via `SaveDefaultDim`, and `DeleteDefaultDim` removes them again in `OnDelete`. Both `ValidateDimValueCode` and `SaveDefaultDim` take the shortcut dimension *number* (1-8, matching `General Ledger Setup`'s "Shortcut Dimension N Code" fields) as their first/third argument respectively — not the AL field ID of the table field being validated. The master record itself carries no `Dimension Set ID` field.
|
||||
- **Transactional/document data** (a custom document or journal-line table) carries a single **`Dimension Set ID`** field — a pointer to a shared, deduplicated set of dimension values in `Dimension Set Entry`, assembled from whatever the document inherited plus whatever the user overrode. A document does not acquire that ID by calling `SaveDefaultDim`; it builds a source list with `AddDimSource` (naming the related master table and its key, e.g. `Database::Customer`), then calls `GetDefaultDimID` to compute a new `Dimension Set ID` that inherits the master's Default Dimension records. Editing a shortcut dimension field directly on the document validates through `ValidateShortcutDimValues`, which updates the same `Dimension Set ID` in place rather than writing a separate Default Dimension record.
|
||||
|
||||
Skipping the model that actually matches the table's kind produces a field that looks correct in the designer but silently fails to save, validate, or carry through to postings — or, for a document, one that never picks up the customer's/vendor's own dimensions at all.
|
||||
|
|
@ -24,7 +24,7 @@ Skipping the model that actually matches the table's kind produces a field that
|
|||
|
||||
For a master table, validate each shortcut dimension field through `ValidateDimValueCode`, save the result with `SaveDefaultDim`, and delete the matching Default Dimension records in `OnDelete`.
|
||||
|
||||
For a document table, when the field that attaches the document to a master record changes (e.g. `Customer No.`), call `AddDimSource` naming that master table and key, then `GetDefaultDimID` to compute the document's new `Dimension Set ID`, inheriting the master's Default Dimension records. Validate the document's own Shortcut Dimension fields through `ValidateShortcutDimValues`, which updates that same `Dimension Set ID` rather than persisting a separate Default Dimension record.
|
||||
For a document table, when the field that attaches the document to a master record changes (e.g. `Customer No.`), call `AddDimSource` naming that master table and key, then `GetDefaultDimID` to compute the document's new `Dimension Set ID`, inheriting the master's Default Dimension records. Pass `0` for `GetDefaultDimID`'s `InheritFromDimSetID` argument in this case — passing the document's *existing* `Dimension Set ID` instead inherits whatever dimensions were already in it, so a value the previous linked record supplied can survive into the new one even where the new record has no default for that dimension. Validate the document's own Shortcut Dimension fields through `ValidateShortcutDimValues`, which updates that same `Dimension Set ID` in place rather than persisting a separate Default Dimension record.
|
||||
|
||||
See sample: `dimension-management-wiring.good.al`.
|
||||
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ codeunit 50100 "Tooling Extension"
|
|||
{
|
||||
procedure Run()
|
||||
var
|
||||
ToolingPage: Page "Some Tooling Page"; // resolves in a local build, fails in VS Code
|
||||
ToolingPage: Page "Some Tooling Page"; // guessed namespace; can still resolve against a stale or cached symbol package, then fail once checked against the object's current source or a freshly downloaded one
|
||||
begin
|
||||
ToolingPage.Run();
|
||||
end;
|
||||
|
|
|
|||
|
|
@ -13,7 +13,7 @@ application-area: [all]
|
|||
|
||||
## Description
|
||||
|
||||
A BC test company normally contains initialized system/setup data — an AL test suite should not assume an empty database, but it must be independent of unrelated business records: create the records and setup it owns rather than looking up a specific code, number, or name assumed to already exist, since that makes the test fail for reasons unrelated to the code under test. Every mandatory field on a created record also needs a value that respects its declared length; a partial setup that merely passes validation is not sufficient.
|
||||
A BC test company normally contains initialized system/setup data — an AL test suite should not assume an empty database, but it must be independent of unrelated business records: create the records and setup it owns rather than looking up a specific code, number, or name assumed to already exist, since that makes the test fail for reasons unrelated to the code under test. Every mandatory field on a created record also needs an actual value — leaving one blank because setup-time validation happens to allow it produces a record that doesn't reflect a real one and can fail later, elsewhere in the flow (posting, a report, a later assertion), for a reason unrelated to what the test claims to check. A short-but-valid value is not itself a defect: AL field lengths are maxima, not minimums, so a two-character value in a `Text[100]` field is fine unless the scenario specifically depends on the field's length or shape — for example, a test that verifies truncation or a format check needs a value chosen to exercise that boundary, not an arbitrary short one.
|
||||
|
||||
Not every value should be generated, though. Incidental fixture data — identifiers, names, descriptions — should generally come from the standard library codeunits rather than be tied to specific existing data. But values that materially define the scenario under test — amounts, quantities, percentages, dates, thresholds, rounding precision — should stay explicit and deliberately chosen, not randomized: a rounding test needs values placed deliberately around the rounding boundary, not a random one that might miss it entirely.
|
||||
|
||||
|
|
@ -25,6 +25,6 @@ See sample: `test-data-must-be-random-and-complete.good.al`.
|
|||
|
||||
## Anti Pattern
|
||||
|
||||
Looking up a record assumed to already exist (a hardcoded payment method or customer number) instead of creating it, or leaving mandatory fields empty or underfilled because validation happens to allow it.
|
||||
Looking up a record assumed to already exist (a hardcoded payment method or customer number) instead of creating it, or leaving a mandatory field empty because setup-time validation happens to allow it. Also an anti-pattern, narrower: using a value that doesn't satisfy a scenario's explicit length or format requirement — for example a truncation test that never actually exceeds the field it's meant to overflow.
|
||||
|
||||
See sample: `test-data-must-be-random-and-complete.bad.al`.
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ page 50102 "Project Task API"
|
|||
repeater(General)
|
||||
{
|
||||
field(remainingHours; RemainingHoursCalc) { }
|
||||
field(budgetedHours; Rec."Budgeted Hours") { }
|
||||
field(hoursUsed; Rec."Hours Used") { }
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -17,7 +17,7 @@ A stored field whose value is derived from other fields inside an `OnValidate` t
|
|||
|
||||
## Best Practice
|
||||
|
||||
Recalculate the derived value in `OnAfterGetRecord` from its authoritative source — typically a FlowField — using a page-level variable, and expose both the recalculated value and the source field so the consumer can verify it.
|
||||
Recalculate the derived value in `OnAfterGetRecord` from its authoritative source — typically a FlowField — using a page-level variable, and expose that recalculated value instead of the stale stored field. Whether to also expose the source fields is a separate design decision, not a requirement of this pattern; keep the API contract scoped to what consumers actually need. If letting the consumer verify the recalculation is itself a requirement, expose every field the calculation reads, not just one of them — a derived value with two inputs needs both exposed, or the "verification" is incomplete.
|
||||
|
||||
See sample: `stored-derived-fields-must-not-be-exposed-directly.good.al`.
|
||||
|
||||
|
|
|
|||
|
|
@ -51,7 +51,7 @@ The following targeted checks cover every current `testing` article. Treat each
|
|||
- 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`.
|
||||
- `asserterror` is added or changed without a following `Assert.ExpectedError`, `Assert.ExpectedErrorCode`, or a purpose-built assertion such as `ExpectedTestFieldError` — `asserterror-needs-expectederror-and-code`.
|
||||
- A test path raises UI and `[HandlerFunctions(...)]` does not match the invoked handlers, or the test has no meaningful evidence of the UI result (for example, it treats a Boolean set before the action as proof of success) — `ui-handlers-in-tests`. A capture/reset/assert-after-`RunModal` pattern is valid. Enqueue/dequeue and `AssertEmpty` are required only when order, count, text, replies, or a scripted sequence is part of the contract. Only nonoptional handlers have to execute: a listed handler declared `[SendNotificationHandler(true)]` or `[RecallNotificationHandler(true)]` is optional by design, so do not treat it as unmatched when the run never raises the notification.
|
||||
- A test's `[GIVEN]`/setup looks up a hardcoded code/number/name assumed to already exist instead of creating it, leaves a mandatory field on a created record empty or under-sized, or a scenario-defining value (amount, quantity, percentage, date, threshold, rounding precision) is generated/randomized instead of an explicit chosen value — `test-data-must-be-random-and-complete`. Generating incidental fixture values (identifiers, names, descriptions) via the standard library codeunits is the compliant shape, not the signal to flag.
|
||||
- A test's `[GIVEN]`/setup looks up a hardcoded code/number/name assumed to already exist instead of creating it, leaves a mandatory field on a created record empty, uses a value that doesn't satisfy the scenario's own explicit length/format requirement (for example a truncation test whose value never exceeds the field), or a scenario-defining value (amount, quantity, percentage, date, threshold, rounding precision) is generated/randomized instead of an explicit chosen value — `test-data-must-be-random-and-complete`. Generating incidental fixture values (identifiers, names, descriptions) via the standard library codeunits is the compliant shape, not the signal to flag, and neither is a short-but-valid value in an otherwise-unremarkable field.
|
||||
|
||||
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`.
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue