Skip to content

Add ensure_current/2 helper + V110 language column on doc templates#515

Merged
ddon merged 2 commits into
BeamLabEU:devfrom
mdon:dev
May 5, 2026
Merged

Add ensure_current/2 helper + V110 language column on doc templates#515
ddon merged 2 commits into
BeamLabEU:devfrom
mdon:dev

Conversation

@mdon
Copy link
Copy Markdown
Contributor

@mdon mdon commented May 5, 2026

Summary

Two changes bundled here:

  1. PhoenixKit.Migration.ensure_current/2 — re-runnable analog of
    mix ecto.migrate for test helpers and any boot path that runs against
    a long-lived DB. Fixes a latent staleness bug where the documented
    canonical pattern stops applying newly-shipped Vxxx migrations after
    the first boot.

  2. V110 — language column on phoenix_kit_doc_templates — adds a
    nullable VARCHAR(10) column so Document Creator templates can be
    tagged with a single locale (full code, e.g. en-US, et-EE). Parent
    apps will read this back via the public listing API to fill template
    variables in the matching language regardless of the admin's UI
    locale. Resolves the design ask in
    BeamLabEU/phoenix_kit_document_creator#13.


(1) ensure_current/2 — the bug

The pattern shipped in dev_docs/migration_cleanup.md and copied into
every consuming module's test_helper.exs:

Ecto.Migrator.run(repo, [{0, PhoenixKit.Migration}], :up, all: true)

is idempotent at the outer layer — Ecto.Migrator records "version 0
applied" in schema_migrations after the first call and filters
{0, PhoenixKit.Migration} out of pending on every subsequent boot.
PhoenixKit.Migration.up/1 is never re-invoked, so newly-shipped Vxxx
migrations never apply even though PhoenixKit's own marker (the comment
on the phoenix_kit table) is itself idempotent.

Symptom: column ... does not exist after mix deps.update phoenix_kit
pulls in new migrations but the test DB stays at the old marker. The
two layers of idempotency cancel each other out.

Verified empirically — core's own phoenix_kit_test was at marker 107
even though Hex 1.7.103 ships V108 + V109. The original wrapper at
test/support/postgres/migrations/20260316000000_add_phoenix_kit.exs
hadn't re-run since first DB creation.

The fix

ensure_current/2 passes a fresh wall-clock version
(:os.system_time(:microsecond)) to Ecto.Migrator.up/4 on every
call. Ecto sees a "new" migration each time and invokes the inner
runner; PhoenixKit's marker short-circuits internally if there's
nothing new to apply. Microsecond precision keeps the collision /
clock-skew window vanishingly small.

# In test/test_helper.exs
PhoenixKit.Migration.ensure_current(MyApp.Test.Repo, log: false)

Forwards :prefix from the Ecto.Migration runner context via the
internal Runner module's runner_opts/0 so callers passing
prefix: "auth" aren't silently routed to "public".

The schema_migrations table accumulates one row per call (with a
microsecond-precision timestamp as the version) — cosmetic noise
acceptable for the test-DB use case.


(2) V110 — language on document templates

Document Creator templates are inherently single-language (one Google
Doc = one language; translations live in separate templates). Today
parent apps fall back to either a global hardcoded value or the admin's
session locale when filling template variables — both wrong, since an
Estonian-language template should always be filled with Estonian
values regardless of which UI locale the admin happens to use.

V110 adds:

ALTER TABLE phoenix_kit_doc_templates ADD COLUMN language VARCHAR(10);
  • Nullable, so existing rows survive without a backfill. The form
    in phoenix_kit_document_creator will pre-select the project's
    primary language (sourced from PhoenixKit.Modules.Languages) when
    creating new templates and let users change it from the admin UI.
  • Full locale codes (en-US, et-EE, ja) — matches what
    Languages.get_enabled_languages/0 returns, lossless. Consumers
    that want bare base codes can derive them via
    DialectMapper.dialect_to_base/1.
  • Templates only. Documents inherit language from
    template_uuid → templates.language — they don't get their own
    column.

Schema/API changes in phoenix_kit_document_creator will land
separately on that repo.


What's NOT changed

  • Production migrations via mix ecto.migrate /
    mix phoenix_kit.update continue to work the same way; V110 is just
    the next versioned migration in line.
  • Existing consumer modules' test_helper.exs files are untouched.
    They can migrate to ensure_current/2 at their own pace; the old
    pattern still works (just with the latent staleness bug). Workspace
    dev_docs/migration_cleanup.md documents the cutover.

Test plan

  • mix format --check-formatted clean
  • mix credo --strict — 7313 mods, no issues
  • mix dialyzer — 0 errors
  • mix test — 1049 tests; 3 pre-existing V107 failures verified on stock dev (stale-DB drift unrelated to this PR)
  • Empirical: marker advanced from 107 → 109 on the first test run after the change, confirming new migrations actually apply
  • V110 applied via mix phoenix_kit.update against phoenix_kit_parent_dev; column exists, nullable, VARCHAR(10), marker advanced to 110
  • Browser-verified end-to-end through phoenix_kit_parent admin pages (catalogue list, smart catalogue detail, item edit form, search, paged list with 311 items) — 0 console errors
  • Phase 2 quality review applied — 4 fixes (CRITICAL prefix-loss bug + HIGH clock-skew hardening + spec/test cleanup)

Followup

dev_docs/migration_cleanup.md and dev_docs/quality_sweep.md updates
land alongside (in the workspace, not this repo). Consumer modules
adopting ensure_current/2 come as separate PRs — first is
phoenix_kit_catalogue (companion to this PR, paired merge).

mdon added 2 commits May 5, 2026 04:10
The pattern shipped in dev_docs/migration_cleanup.md as the canonical
test_helper schema setup —

    Ecto.Migrator.run(repo, [{0, PhoenixKit.Migration}], :up, all: true)

— silently goes stale once `0` is in `schema_migrations`. Ecto.Migrator
filters that pending entry out, the inner runner is never re-invoked,
and newly-shipped Vxxx migrations don't apply on subsequent boots even
though PhoenixKit's own marker (the comment on `phoenix_kit` table) is
itself idempotent. Two layers of idempotency cancel each other out.

Symptom: `column ... does not exist` after `mix deps.update phoenix_kit`
brings in new migrations but the test DB stays at the old marker.

ensure_current/2 sidesteps this by passing a fresh wall-clock version
(:os.system_time(:microsecond)) to Ecto.Migrator.up/4 on every call.
Ecto sees a "new" migration each time and invokes the inner runner;
PhoenixKit's marker short-circuits internally if there's nothing new
to apply. Microsecond precision keeps the collision and clock-skew
windows small enough that an NTP correction would have to rewind the
clock by µs at exactly the wrong moment to hide a newly-shipped
migration.

Forward `:prefix` from the Ecto.Migration runner context inside
PhoenixKit.Migration.Runner so callers passing `prefix: "auth"` to
ensure_current/2 don't silently run migrations against `"public"`.

Update core's own test_helper.exs to use ensure_current/2 as the
canonical example. Delete the now-redundant
test/support/postgres/migrations/ wrapper migration.

The schema_migrations table accumulates one row per call (with a
microsecond-precision timestamp as the version) — cosmetic noise
acceptable for the test-DB use case. Production migrations via
mix ecto.migrate / mix phoenix_kit.update remain unchanged.
Adds a nullable VARCHAR(10) language column to phoenix_kit_doc_templates
so each Document Creator template can be tagged with a single locale
(full code, e.g. en-US, et-EE). Parent apps will read it back via the
public listing API to fill template variables in the matching language
regardless of the admin's UI locale.

Documents intentionally do not get a language column — they inherit
language from template_uuid → templates.language.

Existing rows survive without a backfill; the form pre-selects the
project's primary language for new templates.
@mdon mdon changed the title Add PhoenixKit.Migration.ensure_current/2 for re-runnable migrations Add ensure_current/2 helper + V110 language column on doc templates May 5, 2026
mdon added a commit to mdon/phoenix_kit_publishing that referenced this pull request May 5, 2026
Drop the `Ecto.Migrator.run([{0, PhoenixKit.Migration}], :up, all: true)`
shape — once `0` lands in `schema_migrations`, Ecto.Migrator silently
filters it out forever, so newly-shipped Vxxx migrations never re-apply.
`ensure_current/2` (core 1.7.105+ / phoenix_kit#515) sidesteps this by
passing a fresh wall-clock version on each boot.

Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core
1.7.105 publishes.
@ddon ddon merged commit 86256b6 into BeamLabEU:dev May 5, 2026
ddon pushed a commit that referenced this pull request May 5, 2026
Splits PhoenixKit.Migration.Runner.runner_opts/0 (closure reading the
runner-context prefix()) into a pure runner_opts/1 that takes the
prefix as an argument. Runner.up/0 and Runner.down/0 now call
runner_opts(prefix()).

The previous closure shape couldn't be unit-tested without spinning up
an Ecto.Migration.Runner process — which conflicts with the Ecto
sandbox per the existing moduledoc. The pure transform is testable in
isolation: three assertions pin the contract for nil (drop, so the
"public" default in with_defaults/2 isn't clobbered) and non-nil
prefixes (forwarded verbatim, so multi-tenant migrations land in the
caller's schema instead of silently in public).

If someone "simplifies" runner_opts to always return [] (the regression
the helper was originally added to prevent — see PR #515 review),
CI now fails.

Also adds a "Return contract" section to ensure_current/2's moduledoc
clarifying that failures raise from Ecto.Migrator.up/4 rather than
being wrapped in {:error, _}. Spec was already accurate (:: :ok); this
just calls out the failure-mode contract for callers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 5, 2026
Bumps @Version 1.7.104 → 1.7.105 covering the migration.ex follow-up
commit (Runner.runner_opts/1 refactor + regression tests +
ensure_current/2 raise-on-failure docstring clarification).

CHANGELOG entry deferred to the maintainer per AGENTS.md ownership
rules — flagging the gap, not auto-writing.

Adds the CLAUDE_REVIEW.md for PR #515 (ensure_current/2 helper + V110
language column on doc templates) including the post-merge follow-up
section documenting the runner_opts/1 refactor and tests.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 5, 2026
Covers PR #515 (ensure_current/2 helper + V110 language column on doc
templates) and the post-merge follow-up (runner_opts/1 refactor +
regression tests + ensure_current/2 raise-on-failure docstring note).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit to BeamLabEU/phoenix_kit_entities that referenced this pull request May 5, 2026
Drop the `Ecto.Migrator.run([{0, PhoenixKit.Migration}], :up, all: true)`
shape — once `0` lands in `schema_migrations`, Ecto.Migrator silently
filters it out forever, so newly-shipped Vxxx migrations never re-apply.
`ensure_current/2` (core 1.7.105+ / phoenix_kit#515) sidesteps this by
passing a fresh wall-clock version on each boot.

Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core
1.7.105 publishes.
ddon pushed a commit to BeamLabEU/phoenix_kit_hello_world that referenced this pull request May 5, 2026
…rrent/2

This module's `test/support/postgres/migrations/20260425000000_setup_phoenix_kit.exs`
recreated `phoenix_kit_settings`, `phoenix_kit_activities`, and the
`uuid_generate_v7()` PL/pgSQL function — all owned by core's V03 / V40 /
V90 migrations. Since hello_world is the canonical template module that
others copy from, the bucket-A drift here propagates across every
module that lifted its scaffold.

Swap to `PhoenixKit.Migration.ensure_current/2` (added in core 1.7.105
via BeamLabEU/phoenix_kit#515). On every test boot, ensure_current/2
re-applies any newly-shipped Vxxx migrations idempotently — schema
drift impossible by construction.

  * `test/test_helper.exs` — single-line call to `ensure_current/2`
    after Repo start
  * `mix.exs` `test.setup` alias — drop the `ecto.migrate` step;
    test_helper handles migration on every boot now
  * Delete `test/support/postgres/migrations/` entirely (~80 lines of
    hand-rolled DDL gone)
  * `AGENTS.md` — refresh test-setup section + file-layout list

Depends on BeamLabEU/phoenix_kit#515 (the helper) being merged and
1.7.105 published. Until then, `mix test` falls into the helper's
existing rescue branch — unit tests still run, integration tests
auto-exclude as documented. After deps.update bumps the Hex pin past
1.7.105, integration tests resume cleanly.

Closes the bucket-A row in workspace `dev_docs/migration_cleanup.md`.
ddon pushed a commit to BeamLabEU/phoenix_kit_locations that referenced this pull request May 5, 2026
Delete `test/support/postgres/migrations/20260403000000_setup_phoenix_kit.exs`
(143 lines duplicating core's V40 / V03 / V90 DDL) and call
`PhoenixKit.Migration.ensure_current/2` from `test/test_helper.exs`
instead. Schema drift between test and prod is impossible by
construction.

Surfaced one fixture bug: `destructive_rescue_test.exs` `DROP TABLE
phoenix_kit_activities` no longer works because V90 ships
`phoenix_kit_notifications` with an FK to activities; switched to
`DROP TABLE … CASCADE` to match production's actual constraint graph.

Companion edits:
- `config/test.exs`: drop `priv: "test/support/postgres"` (no longer needed)
- `mix.exs`: drop `ecto.migrate` from the `test.setup` alias (helper
  applies migrations on every boot)
- `AGENTS.md`: refresh Database & Migrations / Testing sections to
  reference `ensure_current/2`

Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core
1.7.105 publishes.
ddon pushed a commit to BeamLabEU/phoenix_kit_sync that referenced this pull request May 5, 2026
Replace the inline DDL (uuid_generate_v7 fn, phoenix_kit_activities
stub, phoenix_kit_settings stub) plus the
`Ecto.Migrator.up(TestRepo, 0, PhoenixKitSync.Migration, ...)` call
in `test/test_helper.exs` with `PhoenixKit.Migration.ensure_current/2`
(core 1.7.105+ / phoenix_kit#515).

Sync tables are owned by core (V37 creates them as `phoenix_kit_db_sync_*`;
V44 renames to `phoenix_kit_sync_*`; V56/V58/V61/V73/V74 evolve them).
phoenix_kit_settings (V03), phoenix_kit_activities (V90), and the
uuid_generate_v7 function (V40) are also owned by core. Schema drift
between test and prod is impossible by construction.

Two pre-existing test failures surfaced — both committed in 7916940
("PR #5 follow-up bundle (F1–F4)") whose own message admits "DB
tests will be exercised by the next full mix test run on a host with
the test DB available". The old inline DDL had been masking the
real LV behavior. Both skipped with `@tag :skip` and clear comments
naming the bug + intended fix; out of migration-cleanup scope:

- F1 Iron Law dead-render test — `handle_params/3`'s catch-all
  `handle_action` clause calls `load_connections/1` unconditionally,
  bypassing the `connected?` gate that F1 added to `mount/3`. Needs
  a one-line LV fix: swap `load_connections()` →
  `maybe_load_connections()` in `connections_live.ex:106`.
- F4 revoke gettext test — looks for `[phx-click='revoke_connection']`
  in the list view, but the revoke button only exists in the connection
  detail view (`connections_live.ex:1940`). Test needs a navigate-then-
  click flow or a context-layer pin.

Companion edits:
- `AGENTS.md`: Database & Migrations / Testing sections updated.

Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core
1.7.105 publishes.
ddon pushed a commit to BeamLabEU/phoenix_kit_ai that referenced this pull request May 5, 2026
Drop the `Ecto.Migrator.run([{0, PhoenixKit.Migration}], :up, all: true)`
shape — once `0` lands in `schema_migrations`, Ecto.Migrator silently
filters it out forever, so newly-shipped Vxxx migrations never re-apply.
`ensure_current/2` (core 1.7.105+ / phoenix_kit#515) sidesteps this by
passing a fresh wall-clock version on each boot.

Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core
1.7.105 publishes.
ddon pushed a commit to BeamLabEU/phoenix_kit_staff that referenced this pull request May 5, 2026
Drop the transition migration shim at
`test/support/postgres/migrations/20260427000000_setup_phoenix_kit.exs`
(145 lines that called `PhoenixKit.Migrations.up()` for V01..V96
prereqs and inlined V100 staff DDL with `IF NOT EXISTS` guards) and
call `PhoenixKit.Migration.ensure_current/2` from `test/test_helper.exs`
instead. V100 is now in core, so the inline shim is no longer needed.

Schema drift between test and prod is impossible by construction.

Companion edits:
- `mix.exs`: drop `ecto.migrate` from the `test.setup` alias (helper
  applies migrations on every boot)
- `config/test.exs`: drop `priv: "test/support/postgres"` (no longer needed)
- `AGENTS.md`: refresh Testing section to reference `ensure_current/2`

Two pre-existing tests skipped — both submit `status: "bogus_status"`
to test the failure-side audit row, but Phoenix LiveView 1.1.29
(pulled in by `mix deps.get`) added stricter `<select>` option
validation to `render_submit/2` that blocks the bogus value before
the LV sees it. The audit-row failure path needs a context-layer pin
or a different invalid-input vector. Out of migration-cleanup scope.

Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core
1.7.105 publishes.
ddon pushed a commit to BeamLabEU/phoenix_kit_projects that referenced this pull request May 5, 2026
Drop the transition migration shim at
`test/support/postgres/migrations/20260427000000_setup_phoenix_kit.exs`
(323 lines that called `PhoenixKit.Migrations.up()` for V01..V96
prereqs and inlined V100 staff + V101 projects + V105 partial-index
DDL with `IF NOT EXISTS` guards) and call
`PhoenixKit.Migration.ensure_current/2` from `test/test_helper.exs`
instead. V100, V101, and V105 are all in core, so the inline shim is
no longer needed.

Schema drift between test and prod is impossible by construction.

Companion edits:
- `mix.exs`: drop `ecto.migrate` from the `test.setup` alias (helper
  applies migrations on every boot)
- `config/test.exs`: drop `priv: "test/support/postgres"` (no longer needed)
- `AGENTS.md`: refresh Testing section to reference `ensure_current/2`

Depends on BeamLabEU/phoenix_kit#515 — CI will be red until core
1.7.105 publishes.
mdon added a commit to mdon/phoenix_kit_catalogue that referenced this pull request May 5, 2026
Adds the test batch from the Phase 2 quality sweep against the PDF
library feature. Lands in two parts: pure-function tests that run
locally today (schema, worker, paths) plus DB-gated test files that
run once `BeamLabEU/phoenix_kit#515` publishes 1.7.105 with V111.

## New test files

- `test/schemas_test.exs` — appended 25 tests for the four PDF
  schemas (Pdf, PdfExtraction, PdfPage, PdfPageContent). Required
  fields, validations, status transitions, edge cases. **113 / 113
  green** (the file's full count after additions).
- `test/workers/pdf_extractor_test.exs` — new file, 22 tests
  covering `normalize/1` (ligatures, soft-hyphen, line-break
  hyphenation, whitespace), `parse_page_count/1` (typical / edge /
  malformed), `inspect_reason/1` (all four shapes + fallback), and
  the Oban worker definition (queue, max_attempts). **22 / 22 green**.
- `test/paths_test.exs` — new file, 12 tests for the PDF Paths
  helpers — pins the `URI.encode_www_form` fix from the sweep and
  the page-fragment positioning in `pdf_viewer/2`. **12 / 12 green**.
- `test/catalogue/pdf_library_test.exs` — new file, ~25 context
  tests for list/count/get/trash/restore/permanently_delete +
  worker callbacks + `insert_page` (content dedup) +
  `prune_orphan_page_contents` + search. DB-gated.
- `test/web/pdf_library_live_test.exs` — new file, ~6 LV smoke
  tests for mount + filter switch + trash/restore events with
  `assert_activity_logged` actor_uuid pinning + PdfDetailLive
  not-found redirect. DB-gated.

## Worker testability change

Three pure helpers in `Workers.PdfExtractor` (`normalize/1`,
`parse_page_count/1`, `inspect_reason/1`) exposed as `@doc false def`
(was `defp`). Lets the test file call them directly without
shenanigans. No production behavior change.

## Sweep documentation

`dev_docs/pull_requests/2026/24-pdf-library-quality-sweep/FOLLOW_UP.md`
records the full Phase 2 audit + every classified finding (Fixed
Batch 1 / Skipped with rationale / Open). Format mirrors PR BeamLabEU#23's
follow-up.

## DB-gated tests

The two integration test files cannot run on the standalone
catalogue test DB today — it's at V110 until 1.7.105 publishes
with V111. The `test_helper.exs`'s `:integration` auto-exclude
keeps CI green; tests light up automatically when
`mix deps.update phoenix_kit` lands in `mix.lock`.
mdon added a commit to mdon/phoenix_kit that referenced this pull request May 5, 2026
Triage of CLAUDE_REVIEW.md against current code: every actionable item
was addressed in the reviewer's own post-merge follow-up commit
(`Runner.runner_opts/1` + prefix-forwarding tests + `## Return contract`
moduledoc + `@version 1.7.105`). FOLLOW_UP.md records the audit so the
PR folder isn't ambiguously "untriaged".
ddon added a commit that referenced this pull request May 6, 2026
V111 PDF library tables + #511/#512/#515 follow-ups
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants