V117 migration: document composition tables

[timujinne]

Adds the V117 PhoenixKit migration introducing the three tables required by the `phoenix_kit_document_creator` document-composition feature.

V117 — Document composition tables

• `phoenix_kit_doc_templates`: adds `category` string column + index. Lets templates self-classify (financial / technical / etc.) so callers can filter the template grid by scope.
• `phoenix_kit_doc_document_sections` (new): snapshots `(document_uuid, template_uuid, position, variable_values, image_params)` for every section of every generated document. `document_uuid: :delete_all`, `template_uuid: :nilify_all`. Unique on `(document_uuid, position)` + index on `(document_uuid)`.
• `phoenix_kit_doc_template_presets` (new): named reusable recipes filtered by `(scope_type, scope_id, category)`. `sections JSONB DEFAULT '[]'::jsonb`.

Rationale

The composition flow needs to (a) compose multiple templates into a single Google Doc with per-section variable substitution, and (b) remember the exact recipe used so a user can later "create another like this" or save the recipe as a named preset. Both require server-side state alongside the existing single-row `Document` shape, which is why the link tables live here in PhoenixKit core (the schemas + business logic ship in `phoenix_kit_document_creator`).

Compatibility

• The legacy `Document.template_uuid` column is retained — no existing data is touched. New code in `phoenix_kit_document_creator` writes only into `document_sections`; `template_uuid` is left null on composed docs and continues to work for single-template (legacy path) docs.
• Down migration drops the two new tables and removes the `category` column.

File counts

```
21 commits (mostly merge-forward from upstream into our dev fork);
1 substantive change: lib/phoenix_kit/migrations/postgres/v117.ex + version registry entry.
```

[timujinne]

PR #539 Review — V117 migration: document composition tables

• Author:@timujinne
• Base ← Head:BeamLabEU/dev ← timujinne/dev
• Stats: 4 files, +151 / −38
• Mergeable:CLEAN (git sees no conflict — but see Critical findings)
• Reviewer: CLAUDE (Opus 4.7)

Overview

The PR introduces V117 migration for the phoenix_kit_document_creator module:

1. Adds category VARCHAR + index to phoenix_kit_doc_templates
2. Creates phoenix_kit_doc_document_sections — join table for multi-section composed documents (document_uuid, template_uuid, position, variable_values, image_params)
3. Creates phoenix_kit_doc_template_presets — named reusable section compositions with optional scope

PR body explicitly claims: "1 substantive change: lib/phoenix_kit/migrations/postgres/v117.ex + version registry entry." — but the diff includes two additional files that revert recent upstream sitemap work. See BUG - CRITICAL below.

---

Critical Findings

BUG - CRITICAL: Unintentional sitemap regression bundled into the PR

The PR also modifies:

• lib/modules/sitemap/sources/publishing.ex (+1 / −15)
• lib/modules/sitemap/sources/static.ex (+2 / −21)

These changes delete latest_post_date/2 and static_lastmod/1 — functions that currently exist and work on BeamLabEU/dev. They are not reverts of something already reverted upstream; this is silent fork drift coming back to bite.

Upstream history confirming these are live functions on dev:

dd94ebb1 Unify admin and frontend language systems into single source of truth
74b91f07 Suppress warnings for optional external modules with @compile no_warn_undefined
53cec970 Guard all Publishing references behind Code.ensure_loaded? for external module support
e7b0ef60 Add lastmod to sitemap group listings and homepage ← function added here
54b9b899 Add lastmod to sitemap router-discovered and static entries

The user's fork branched before these upstream commits landed, and the diff against BeamLabEU/dev is undoing them.

Concrete behavior regressions if merged:

Surface	Before merge	After merge
Publishing group listing URLs (/blog, /posts, …)	<lastmod> = latest published-post date in that group	<lastmod> element omitted (lastmod = nil → falsy in url_entry.ex:118)
Static homepage /	<lastmod> derived cross-source from latest publishing content	<lastmod> = today's date, every regeneration
Other static pages	<lastmod> = today's date	<lastmod> = today's date (unchanged)

Why this is critical:

1. The PR title and body claim only V117. A reviewer skimming the title will merge unaware of the regression.
2. mergeStateStatus: CLEAN — git sees no conflict because the fork's deletion silently overwrites the upstream addition. CI won't catch it.
3. Setting lastmod: Date.utc_today() on every static URL on every sitemap regeneration is a known SEO anti-pattern (search engines penalize/ignore sitemaps that claim daily freshness across every page).
4. The user has explicitly said "we didn't make changes to Phoenix Kit Core module here" — but this PR does, by accident.

Required action before merge — pick one:

Option A (recommended): rebase the branch on current upstream/dev so the V117 commit lands cleanly without the sitemap drift:

git fetch upstream dev
git checkout dev
git reset --hard origin/dev # or rebase, depending on local commits
git rebase upstream/dev
# resolve any conflicts on V117 numbering only; sitemap files should be untouched
git push --force-with-lease origin dev

Option B: drop only the sitemap files from the PR:

git checkout upstream/dev -- lib/modules/sitemap/sources/publishing.ex lib/modules/sitemap/sources/static.ex
git commit --amend --no-edit
git push --force-with-lease origin dev

After either, the PR diff should show exactly two files: lib/phoenix_kit/migrations/postgres.ex and lib/phoenix_kit/migrations/postgres/v117.ex.

---

V117 Migration Review

Assuming the sitemap regression is fixed, the V117 migration itself is well-structured. Findings below.

IMPROVEMENT - MEDIUM: Redundant index on document_uuid

v117.ex:69-71:

create_if_not_exists(index(:phoenix_kit_doc_document_sections,[:document_uuid],prefix: prefix))

Two lines earlier (v117.ex:62-67), unique_index is created on (document_uuid, position). Postgres B-tree composite indexes serve queries on the leftmost column prefix — a query filtering on document_uuid alone will already use the composite. The standalone single-column index is redundant.

Impact: ~24 bytes per row write overhead + a second index to maintain on every INSERT/UPDATE/DELETE of sections. With many composed documents this adds up.

Fix: drop the redundant create_if_not_exists(index(..., [:document_uuid], ...)) lines. Don't forget to mirror in any down-migration cleanup if you add one (current down/1 drops the table, which transitively drops both indexes — so no action needed there).

IMPROVEMENT - MEDIUM: Inconsistent null: false on created_by_uuid

V117 tightens the existing convention without documenting why:

Migration	Column	Null?	FK?
V86 phoenix_kit_doc_templates	created_by_uuid	nullable	none
V86 phoenix_kit_doc_documents	created_by_uuid	nullable	none
V117 phoenix_kit_doc_document_sections	created_by_uuid	NOT NULL	none
V117 phoenix_kit_doc_template_presets	created_by_uuid	NOT NULL	none

Making it NOT NULL on new tables is defensible (no legacy rows to backfill), and arguably correct. But if a future PhoenixKit feature creates sections/presets system-internally (e.g. seed data, auto-generated default presets), this will crash. Worth a one-line moduledoc note: "creator is always required; system-generated rows must use a sentinel uuid."

Also: V86 doesn't add an FK constraint to phoenix_kit_users.uuid. V117 doesn't either — consistent. If the team wants tighter constraints, that's a separate effort across all created_by_uuid columns, not piecemeal here.

IMPROVEMENT - LOW: template_uuid lacks an index

phoenix_kit_doc_document_sections.template_uuid has an FK with :nilify_all but no index. Queries like "all sections using template X" (e.g. for an admin "where is this template used?" feature) will full-scan. Likely fine at low row counts; worth adding if cross-template stats become a feature.

create_if_not_exists(index(:phoenix_kit_doc_document_sections,[:template_uuid],prefix: prefix))

IMPROVEMENT - LOW: Preset scope index column order

v117.ex:89-92:

CREATEINDEXIF NOT EXISTS phoenix_kit_doc_template_presets_scope_index
ON#{p}phoenix_kit_doc_template_presets (scope_type, scope_id, category)

Composite index on (scope_type, scope_id, category) serves these query shapes:

• WHERE scope_type = ? ✅
• WHERE scope_type = ? AND scope_id = ? ✅
• WHERE scope_type = ? AND scope_id = ? AND category = ? ✅
• WHERE category = ? ❌ (won't use this index — left-most rule)
• WHERE scope_id = ? ❌

If category-only browsing is a UI affordance (e.g. "all 'financial' presets across all scopes"), a separate single-column index on category is needed. Defer this until the access pattern is known — premature otherwise.

NITPICK: Raw SQL for phoenix_kit_doc_template_presets

The moduledoc explains it: :map DSL can't express '[]'::jsonb default cleanly. Fair. But V86's phoenix_kit_doc_templates does it via Ecto DSL: add(:variables, :map, default: fragment("'[]'::jsonb")). Same trick would work here:

add(:sections,:map,default: fragment("'[]'::jsonb"),null: false)

That removes ~12 lines of raw SQL and keeps schema definitions consistent across the document_creator tables. Tradeoff: raw SQL is more explicit; Ecto DSL is more uniform. Not a blocker — author's judgment call.

NITPICK: Down migration version comment

v117.ex:105:

execute("COMMENT ON TABLE #{p}phoenix_kit IS '116'")

Correctly rewinds to 116. Matches pattern used elsewhere. ✅ No action needed; flagging as positive.

NITPICK: Moduledoc accurate but verbose

The 18-line moduledoc on V117 is fine but unusual for migration modules — V86 has a similar prose moduledoc, so it follows precedent. Continue the pattern; future readers benefit.

---

Style & Convention Compliance

Check	Status
@current_version bumped to 117	✅
Changelog table entry in postgres.ex moduledoc	✅ — also correctly demotes V116 from ⚡ LATEST
IF NOT EXISTS / create_if_not_exists idempotency	✅
UUIDv7 primary keys via uuid_generate_v7()	✅
prefix: plumbing	✅ — both Ecto DSL and raw SQL paths
Down migration	✅ — drops in FK-safe order, restores version comment
timestamps(type: :utc_datetime) consistency	✅ — matches V86
Commit messages use approved verbs (Add, …)	✅
@version / CHANGELOG.md untouched	✅ (per project rule: maintainer-owned)

---

Test Coverage

No migration test file in the PR. V114 has test/phoenix_kit/migrations/v114_test.exs as a recent example pattern. Worth adding test/phoenix_kit/migrations/v117_test.exs verifying:

• Tables exist after up/1
• Columns + indexes are present
• down/1 cleanly removes everything
• Re-running up/1 is idempotent (the whole point of IF NOT EXISTS)

Not a blocker — but the V114 test sets a precedent and the test infrastructure (PhoenixKit.Test.Repo) is in place.

---

Security Considerations

No security surface added.

• All new columns are server-controlled; no user-input string concatenation in raw SQL paths (the only string interpolation is prefix, which comes from migration config, not user input).
• created_by_uuid is null: false but unconstrained by FK — same as V86. Acceptable.
• JSONB variable_values / image_params / sections accept arbitrary user content but that's an application-layer validation concern, not a migration concern.

---

Final Disposition

Item	Severity	Must fix before merge?
Sitemap regression (publishing.ex, static.ex)	BUG - CRITICAL	YES
Redundant document_uuid index	IMPROVEMENT - MEDIUM	No (low-effort, recommend)
Inconsistent null: false on created_by_uuid	IMPROVEMENT - MEDIUM	No (defensible, document)
Missing template_uuid index	IMPROVEMENT - LOW	No
Preset scope index column order	IMPROVEMENT - LOW	No
Raw SQL vs Ecto DSL for presets	NITPICK	No
No V117 migration test	NITPICK	No (precedent exists)

Recommendation: Do not merge as-is. Strip the sitemap files (Option B above) or rebase on current upstream/dev (Option A). Once the diff is exactly V117 + version registry, the migration is ready to ship with the medium/low items as optional follow-ups.