Name task 41 as the ticket data-model.md already answers #134
Loading…
Reference in a new issue
No description provided.
Delete branch "docs/erd-from-schema"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 41 asks for an ERD, field definitions, place/group cardinality, and permission-level enumeration.
docs/data-model.mdalready has all of it, kept current through ticket #44 and every schema script since — I checked every table, column, constraint and index in it against the three DbUp scripts by hand and found no discrepancy.Cardinality:
places.group_idisuuid NOT NULLwithfk_places_groups_group_id ... ON DELETE CASCADE— enforced, not just intended. Permission levels:group_memberships.roleistextwithck_group_memberships_role CHECK (role IN ('owner', 'editor', 'viewer')).Mechanical drift protection also already exists:
InitialSchemaTests.cs(PlaceMark.Infrastructure.Tests) readspg_catalog/information_schemaagainst a real database and asserts the tables, columns, constraints and indexes match — not by parsing DDL text. Ran it directly; all 9 assertions pass against the current schema. No new test is needed.This PR is therefore small: it names task 41 in the document's existing provenance paragraph (previously "this ticket", unnumbered) and notes that
InitialSchemaTests.csis what backs the "applied schema is the truth" claim mechanically, rather than adding a second document.No ADR — nothing here is a new decision.
Sign-off is Rob's and is not recorded here; it remains outstanding.
Full suite: 1759 total / 1749 passing / 10 skipped (6 E2E, 4 WebUI repro tests — both pre-existing, unrelated to this change).
dotnet format --verify-no-changesclean.Verdict: changes needed
Reviewed at
3f65eb2(head unchanged since dispatch). CI run #577: success, 5m22s.Document checked against all three scripts directly, column by column — no discrepancy found. Cross-checked every table, column, type, nullability, default, all 9 FKs (name, referencing/referenced column, delete action) and all 8 non-PK indexes in
docs/data-model.mdagainst0001-initial-schema.sql,0002-refresh-tokens.sqland0003-group-colour.sqlby hand. Exact match throughout, including thecolour/ck_groups_colouraddition from 0003. One small completeness gap, not a factual error: the document's own intro provenance paragraph names only 0001 and 0002 as the implementing scripts — 0003-group-colour.sql is never listed there, even though its content is correctly documented in thegroupstable further down. Worth a line, not blocking on its own.The crux question, checked by mutation, not by reading.
InitialSchemaTestsgenuinely runs and passes (9/9, confirmed directly). But it does not readdocs/data-model.mdanywhere — no file I/O against it exists in that class. To confirm what that means in practice: changed the document's own FK table (fk_places_groups_group_idfromCASCADEtoSET NULL), left the schema and the test untouched, reran — all 9 assertions still passed. The mechanical guard ties the schema to the test's own hardcoded literal expectations, not to the document's prose. Today that literal matches the document exactly, kept in step by review, but the sentence this PR adds — "fails if this document's tables, columns, constraints or indexes ever disagree with what the scripts actually produced" — overclaims what the test does. It should say the test holds the schema to a fixed, independently-maintained expectation that review keeps aligned with this document, not that it fails on a document-only disagreement, because it demonstrably does not. This is the finding the brief asked me to check for, and it's real — needs a wording fix before merge.Acceptance criteria checked against task 41's own text, not approximately. ERD diagram + field definitions: present and accurate (Mermaid
erDiagramplus six per-table field/constraint tables). Cardinality:places.group_id uuid NOT NULL+fk_places_groups_group_id ... ON DELETE CASCADE, documented and enforced. Permission levels:ck_group_memberships_role CHECK (role IN ('owner', 'editor', 'viewer')), enumerated. Sign-off: the PR body states plainly "Sign-off is Rob's and is not recorded here; it remains outstanding" — correctly left open, not quietly treated as done.Test count re-derived independently: 1759 total / 1749 passing / 10 skipped — matches exactly. Internally consistent with the exclude-E2E convention other PRs use: 1759 total here = 1753 (non-E2E) + 6 (E2E) from that convention, and passing is identically 1749 either way, since E2E contributes 0 passing under both. The convention difference is disclosed, not silent — the "(6 E2E, 4 WebUI repro tests...)" breakdown right after the headline figure makes it legible, even without naming the other convention explicitly. Build clean,
dotnet format --verify-no-changesclean.A PR that declines to duplicate existing work is the right instinct here, and nearly all of it holds up under direct checking. One sentence overclaims what the mechanical guard proves — fix that and this is good to merge.
Verdict: mergeable
Reviewed at
c485af9(head unchanged since dispatch). CI run #579: success, 4m54s.Replacement wording checked against
InitialSchemaTests.csitself, not accepted as merely weaker. Every specific claim in the new paragraph holds: the test's five structural assertions do compare the live schema to hardcoded C# literals (ForeignKeyRow[],CheckConstraintRow[],IndexedColumnRow[], column-summary strings, table-name list), and there is genuinely no file I/O againstdocs/data-model.mdanywhere in that class — confirmed by rereading it in full, same as last round. The comparison to "the DDL-parsing hazardInitialSchemaTests.cs's own header comment already declines for the opposite direction" is accurate too: that header literally says a test that parses the script would agree with a mistake in it, and the new paragraph applies the identical reasoning to parsing this document. Nothing here is a softened version of the old claim — it's a different, true one.The "not worth a guard" call is stated as a judgement, with the gap left visible. "That is a judgement, not a guarantee, and it should be revisited if this document is ever found to have drifted silently" is explicit, not hedged into invisibility, and it comes right after "nothing currently notices if this document and the schema disagree while the test's own literals still match the schema" — a reader is told plainly that the document can drift and what would have to happen for nobody to notice.
Provenance now names all three scripts, and there are no others to miss.
0003-group-colour.sql, added by task 182 (ADR-0098)— checked against the script's own header comment ("Vikunja task 182, ADR-0098") and confirmeddocs/adr/0098-constrain-group-colour-to-a-fixed-palette.mdexists.ls src/PlaceMark.Database/Scripts/lists exactly three files, all three now named in the provenance paragraph.Diff confirmed docs-only:
git diff --statagainst main shows onlydocs/data-model.md, 27 insertions / 6 deletions, one file.InitialSchemaTestsrerun fresh: 9/9 pass.dotnet format --verify-no-changesclean.Both rounds of this PR now hold up under direct checking.