3 AL/BC patterns from CURABIS's internal automated-testing training material (#158)

* Add 3 more AL/BC patterns from CURABIS Academy testing course material

Third batch from CURABIS ApS: item-ledger-entry document-no lookup after Ship-and-Invoice posting, TestPage.Visible()/.Enabled() as the mechanism for verifying field UI state, and LibraryUtility.GenerateGUID() for collision-free test fixture values.

* Address Jesper Schulz-Wedde's review on PR #158

- use-generateguid-for-unique-test-fixture-values.md: GenerateGUID()
  is a Code[10] number-series value, not a real GUID; truncating it
  with CopyStr for a shorter field cuts off the changing digits. Point
  to GenerateRandomCode/GenerateRandomCodeWithLength/GenerateRandomXMLText
  instead, which verify uniqueness against the actual table.
- Split use-testpage-visible-enabled-to-verify-field-ui-state.md: drop
  its editability claim (the sample opens with OpenView() and asserts
  Enabled(), which verifies enabled state, not editability — Editable()
  and Enabled() are distinct TestField methods). New companion article
  use-testpage-editable-to-verify-field-editability.md covers Editable()
  with OpenEdit() specifically.
- Wire GenerateGUID/CopyStr and TestPage Visible/Enabled/Editable cues
  into al-testing-review.md, and the Item Ledger Entry/Last Shipping No.
  posting cue into al-data-modeling-review.md.

The Item Ledger Entry article itself was independently verified against
current BCApps source and needs no changes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Address second round of Jesper Schulz-Wedde's review on PR #158

- use-generateguid-for-unique-test-fixture-values.md/.good.al: documented
  each LibraryUtility helper's actual behavior, verified against
  LibraryUtility.Codeunit.al. GenerateRandomCode opens the target table as
  a temporary RecordRef, so despite taking TableNo it never checks real
  data. GenerateRandomXMLText performs no table lookup at all. Only
  GenerateRandomCodeWithLength/GenerateRandomCode20 (capped at Code[10]/
  Code[20]) genuinely verify against the real table. Fixture switched to
  GenerateRandomCodeWithLength where the comment claims verified
  uniqueness.
- al-testing-review.md: rewired the cue to catch the actual anti-pattern
  (hardcoded literals, hand-built uniqueness, short-field GUID truncation)
  instead of only matching the compliant GenerateGUID()+CopyStr shape;
  broadened tokens to include TestPage, Library - Utility, and
  .Visible()/.Enabled()/.Editable().
- al-data-modeling-review.md: restricted the Item Ledger Entry
  Last-Shipping-No. cue to sales combined posting; purchase combined
  posting is Receive+Invoice and uses different fields entirely.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>

* Fix remaining correctness issues from Jesper's 2026-09-15 re-review

- use-generateguid-for-unique-test-fixture-values.md: narrowed the
  collision rule to primary-key/unique-lookup fields - an ordinary
  descriptive field carries no uniqueness constraint, so a hardcoded
  or deterministic value there isn't the anti-pattern (the article and
  its al-testing-review.md worklist cue both said "primary-key or
  descriptive field"). Also corrected GenerateRandomCode: it opens its
  target table as a temporary RecordRef that starts and stays empty,
  so its repeat/until loop always exits after one iteration and never
  retries even within a single test run - the "non-colliding within a
  test run" claim was false. It's the rightmost N characters of
  GenerateGUID()'s sequential series, so a short field's value cycles
  (Code[1] repeats every 10 calls, Code[2] every 100). Verified against
  LibraryUtility.Codeunit.al in the BCApps reference clone.
- item-ledger-entry-document-no-follows-last-shipping-no: both
  fixtures called FindSet() without consuming its optional Boolean,
  which raises a runtime error on an empty result set - the opposite
  of the article's own claimed "silently matches zero rows, no error"
  behavior. Wrapped in `if ... then;` per the existing
  guard-database-reads.good.al idiom.
- al-data-modeling-review.md: widened both not-applicable scope
  clauses (intro and outcome) to include dimension wiring, posting-
  routine structure, and Item Ledger Entry document-number lookups -
  the leaf declared itself not-applicable outside setup/master/key/
  numbering/block/audit surfaces despite having a targeted cue for
  this PR's own new article.
- Converted this PR's 8 plain-backtick "See sample: `x.good.al`."
  references (across all 4 new articles) to the READ-convention
  markdown-link form required by Knowledge-Retrieval.ps1.

Rebased onto upstream/main (one conflict in al-data-modeling-review.md
intro wording, merged).

* Stop routing GenerateRandomCode20 as compliant for shorter fields

al-testing-review.md's cue presented GenerateRandomCodeWithLength and
GenerateRandomCode20 as interchangeable options for "a shorter field
needing real verified uniqueness." They aren't: verified against
LibraryUtility.Codeunit.al, GenerateRandomCode20 truncates
GenerateGUID()'s sequential value down to the target field's length by
keeping the leftmost (slowest-changing) characters via PadStr, so its
retry loop against a field shorter than 20 can churn through the same
truncated prefix for a long time. GenerateRandomCodeWithLength has no
such problem (it generates exactly the requested length of random
text). The knowledge article itself already scoped GenerateRandomCode20
to Code[20] correctly - only the skill cue needed narrowing to match.

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
Michael Dieringer 2026-09-29 12:49:11 +02:00 • committed by GitHub
parent 0a8c9a8556
commit 186691f815
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
14 changed files with 255 additions and 3 deletions

View file

@ -16,7 +16,7 @@ application-area: [all]
Reviews AL source changes against the `data-modeling` knowledge domain in BCQuality and emits a findings report. This is a leaf action skill: it invokes no sub-skills. It is one of the skills composed by `al-code-review`.
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, or audit fields. The skill returns `not-applicable` when none of those apply.
An orchestrator invokes this skill with a `pr-diff`, `file-path`, or `folder-path`. Data-modeling findings are narrow by design — they apply when the review scope contains setup or master tables, their card pages, primary keys, number-series assignment, block enforcement, audit fields, dimension wiring, journal-based posting-routine structure, or Item Ledger Entry document-number lookups after a combined sales post. The skill returns `not-applicable` when none of those apply.
## Source
@ -46,6 +46,7 @@ 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`.
- Code reads `Item Ledger Entry."Document No."` (or `"Last Shipping No."`/`"Last Posting No."`) after a combined Ship+Invoice **sales** post — `item-ledger-entry-document-no-follows-last-shipping-no`. This is a sales-specific rule: purchase combined posting is Receive+Invoice and uses receiving fields such as `"Last Receiving No."`, not the shipment/document-number behavior this article describes. Do not worklist it from purchase posting code.
- 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`.
@ -87,7 +88,7 @@ Outcome selection:
- `completed` — the skill evaluated every worklist item.
- `no-knowledge` — no applicable data-modeling knowledge survived filtering.
- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, or audit-field surface.
- `not-applicable` — the diff touches no setup/master table, page, key, numbering, block-check, audit-field, dimension-wiring, posting-routine-structure, or Item-Ledger-Entry-document-number surface.
- `partial` — a budget was hit before the worklist was exhausted.
- `failed` — an unrecoverable error occurred.