Skip to content

V143: consolidate manufacturing/warehouse module tables into core - #632

Merged
ddon merged 6 commits into
BeamLabEU:mainfrom
timujinne:core-v143-module-tables
Jul 13, 2026
Merged

V143: consolidate manufacturing/warehouse module tables into core#632
ddon merged 6 commits into
BeamLabEU:mainfrom
timujinne:core-v143-module-tables

Conversation

@timujinne

@timujinnetimujinne commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds PhoenixKit.Migrations.Postgres.V143, consolidating five tables previously created by the phoenix_kit_manufacturing and phoenix_kit_warehouse packages' own module-owned migration_module/0 migrations into core's numbered chain:

  • phoenix_kit_machines
  • phoenix_kit_machine_type_assignments
  • phoenix_kit_machine_operations
  • phoenix_kit_warehouse_transfers (+ its number sequence)
  • phoenix_kit_warehouse_min_stock

This PR only touches phoenix_kit core (lib/phoenix_kit/migrations/postgres.ex, lib/phoenix_kit/migrations/postgres/v143.ex) and has no dependency on any other open PR. Companion PRs in phoenix_kit_manufacturing and phoenix_kit_warehouse remove their now-redundant migration_module/0 and switch to depending on core owning these tables — see "Depends on / blocks" below.

Why here, not in the module packages

Both packages previously shipped their own migration_module/0, discovered and run by mix phoenix_kit.update on the host app. That worked, but split "does this host's schema match this module's expectations" across independently-versioned migration chains instead of one. This moves both packages onto the pattern already used for phoenix_kit_locations (V90/V122) and the standalone-warehouse-table precedent already in core (V140, PR #624) — one chain, one @current_version, one mix phoenix_kit.update.

Scope per table

  • phoenix_kit_machinesCREATE TABLE IF NOT EXISTS with the V1 identity columns plus the V2 passport/soft-location columns added via ADD COLUMN IF NOT EXISTS — i.e. the module's current (V5-equivalent) shape in one step. Idempotent against a V1-V5 host and a fresh host alike.
  • phoenix_kit_machine_type_assignments / phoenix_kit_machine_operationsmachine_type_uuid/operation_uuid are soft references (no FK): those directories now live in the separate, optional phoenix_kit_entities package, which this core migration correctly has no dependency on. Upgrade path: a host on the published phoenix_kit_manufacturing 0.2.0 (module schema V1) already has machine_type_assignments with a live FK on machine_type_uuid; this migration drops that FK unconditionally via a catalog lookup by table+column (not a guessed constraint name — see fk_constraint_name/3, ported verbatim from the module's own already-shipped code). phoenix_kit_warehouse 0.1.0 never published any migrations at all (verified against the published Hex tarball — no migrations/ directory exists in it), so warehouse's two tables are fresh-install-only DDL, no upgrade branch needed or written.
  • Legacy directory tables (phoenix_kit_machine_types, phoenix_kit_operations, phoenix_kit_defect_reasons — all pre-V5 module-owned) are not re-created; they aren't one of the five objects this migration owns. Each is dropped when it exists and is empty, and left in place (with a RAISE NOTICE) when it still holds rows, so no host's real directory data is silently destroyed. See "Non-empty legacy tables" below for hosts that hit that branch.

What this PR does not do

  • Data conversion. The module's own (never-published-to-Hex) local V5 used to also copy rows out of the three legacy directory tables into phoenix_kit_entities records and rewrite the join-table columns to point at them. V143 does not reproduce that step: a core schema migration has no business depending on the optional phoenix_kit_entities package, and silently converting someone's business data as a side effect of a schema migration is the wrong default. See "Non-empty legacy tables" below for the manual path.
  • CHANGELOG.md. Left untouched, as usual — release notes are the maintainer's call. Note: mix.exs@version is already 1.7.189 (bumped upstream before this branch was rebased onto it), which is exactly the version the companion warehouse/manufacturing PRs pin (>= 1.7.189) — so only the CHANGELOG entry is pending, not a version bump. Suggested for the release notes: a warning that down(version: 142) on a host upgraded from phoenix_kit_manufacturing 0.2.0 drops the pre-existing phoenix_kit_machine_type_assignments table (documented caveat in the migration docstring).

Non-empty legacy tables on upgrade hosts

Only relevant to a host that (a) installed phoenix_kit_manufacturing from the published 0.2.0 (module schema V1) and (b) has real rows in phoenix_kit_machine_types / phoenix_kit_operations / phoenix_kit_defect_reasons when it upgrades phoenix_kit core past V143. Fresh installs, and any host where those three tables are already empty or gone, need nothing further — V143 handles that case by itself.

Quick check after upgrading:

SELECT to_regclass('public.phoenix_kit_machine_types') IS NOT NULLAS machine_types_left,
to_regclass('public.phoenix_kit_operations') IS NOT NULLAS operations_left,
to_regclass('public.phoenix_kit_defect_reasons') IS NOT NULLAS defect_reasons_left;

If any of those come back true, the conversion algorithm to finish by hand is:

  1. Idempotently ensure three phoenix_kit_entities blueprint entities exist (machine_type, operation, defect_reason) via get_entity_by_name/1create_entity/2 (+ translations).
  2. For every row in each legacy table, insert a phoenix_kit_entity_data record (title/description into the primary-language block of data; machine_type's field_template into metadata), stamping metadata["legacy_uuid"] with the source row's uuid for idempotency.
  3. UPDATE phoenix_kit_machine_type_assignments SET machine_type_uuid = <new uuid> WHERE machine_type_uuid = <old uuid> (and the phoenix_kit_machine_operations.operation_uuid equivalent) for every mapped pair — the FK constraints on these columns are already gone by this point (V143 drops them unconditionally in up/1), so there's nothing to drop yourself first.
  4. DROP TABLE the three legacy tables once every row is confirmed migrated.

The exact, previously-shipped implementation of this algorithm (migrate_legacy_directories_to_entities/2 and its private helpers, including the blueprint entity definitions and multilang reshaping) is preserved verbatim in phoenix_kit_manufacturing's git history and walked step by step — with the exact commit to pull it from — in that repo's dev_docs/LEGACY_DATA_MIGRATION.md (added by its own companion PR removing migration_module/0).

Rollback

down/1 mirrors the five creates (drops in FK-safe order, restores the phoenix_kit table comment to '142'). One caveat, documented in V143.down/1's own moduledoc: on a host that started from the published manufacturing 0.2.0, phoenix_kit_machine_type_assignments pre-dates V143 (created by that package's own old migration_module/0, not by this migration) — down/1 can't distinguish the two provenances, so it drops that table unconditionally, which is a stricter rollback than "undo only what V143 did" on such a host. down/1 never touches the three legacy directory tables in either direction — they're never owned by V143 regardless of whether up/1 dropped them or left them in place.

Rolling back is PhoenixKit.Migrations.down(prefix: "public", version: 142) — the target version is exclusive of the down range (only V143's own down/1 runs; V142 and below are untouched).

Heads-up: unrelated dangling v143.ex on feature/v143-crm-party-roles

This fork also carries a stale, never-opened-as-a-PR branch feature/v143-crm-party-roles (fb24bda0, branched from 1.7.186) with its own, unrelated v143.ex — a CRM party-roles migration (supplier/client roles on companies/contacts). It does not block this PR: upstream/main never merged it (@current_version is still 142 there, and no v143.ex exists on upstream/main, as of this PR), so there is no numbering collision today. If/when that CRM work is revived, it renumbers to V144+ — an intentional owner call, made so this consolidation didn't have to wait on an unopened, unrelated branch.

Depends on / blocks

  • No dependency on any other open PR.
  • Companion PRs in phoenix_kit_manufacturing and phoenix_kit_warehouse remove their now-redundant migration_module/0 and bump their pk_dep(:phoenix_kit, ...) pin to whatever patch version ships this V143 (placeholder in both mix.exs files until this is published — upstream phoenix_kit is at 1.7.189 as of this PR).

Testing

  • mix format
  • mix compile --warnings-as-errors
  • mix test test/phoenix_kit/migration_test.exs — module-shape tests, no DB needed, 6/6 passing
  • mix test test/integration/prefix_migration_test.exs — runs the full versioned chain (now including V143) into a named schema; not run in this environment (no reachable PostgreSQL). This is exactly the test that exercises V143 end to end once a DB-backed environment picks up this branch.
  • Every DDL statement in v143.ex was diffed by hand against its pre-consolidation source (phoenix_kit_manufacturing's module machines.ex V1/V2 shape, phoenix_kit_warehouse's v01.ex/v02.ex) — see CLAUDE_REVIEW.md in this PR's doc folder for the full verification pass.

Related

  • Migration: lib/phoenix_kit/migrations/postgres/v143.ex
  • Dispatcher: lib/phoenix_kit/migrations/postgres.ex
  • Precedent: V140 (phoenix_kit_warehouse's first core-owned tables, PR Add V140 migration: phoenix_kit_warehouse tables #624), V138/V122/V90 (locations — same core-owns-the-tables pattern)
  • Review: dev_docs/pull_requests/2026/143-manufacturing-warehouse-tables-consolidation/CLAUDE_REVIEW.md

Ports phoenix_kit_manufacturing's and phoenix_kit_warehouse's own
migration_module/0 tables into core's numbered migration chain:
phoenix_kit_machines, phoenix_kit_machine_type_assignments,
phoenix_kit_machine_operations (soft-referenced, FK dropped on upgrade
from published manufacturing 0.2.0), phoenix_kit_warehouse_transfers,
and phoenix_kit_warehouse_min_stock (fresh-install only). Legacy
directory tables (machine_types/operations/defect_reasons) are dropped
when empty and left in place with a notice when they still hold rows.
Bumps @current_version from 142 to 143 in PhoenixKit.Migrations.Postgres
so ensure_current/2 and the Module.concat dispatch pick up V143
(manufacturing/warehouse module tables consolidation, added in the prior
commit) automatically — no separate version registry to update. Adds the
matching V143 entry to the moduledoc's version list ahead of V142, moving
the "LATEST" marker over.
Verifies DDL fidelity against the pre-consolidation manufacturing/
warehouse sources, the wave-C plan's mandatory review fixes (8 indexes,
FK-drop ordering, down/1 upgrade-host caveat, rollback terminology), and
checks two known bug classes (schema-qualified index names, unscoped
constraint guards) fixed in prior PRs. No issues found.
Schema-qualifies the five uuid_generate_v7() DEFAULT calls
(phoenix_kit_machines, phoenix_kit_machine_type_assignments,
phoenix_kit_machine_operations, phoenix_kit_warehouse_transfers,
phoenix_kit_warehouse_min_stock) to #{p}uuid_generate_v7() — matching
the fix applied to V136/V138/V140/V141 by the low-privilege-install
hardening (0268450). Postgres.up/1 already re-ensures the function at
the target schema for any chain starting >= V40, so this is a
mechanical qualification with no behavior change on public installs.
Every other prefix-safety convention was already satisfied: index
names stay bare on CREATE, the information_schema.tables existence
check in maybe_drop_if_empty/1 is anchored on table_schema, and
fk_constraint_name/3 anchors its catalog lookup on tc.table_schema via
a parameterized query.
@timujinne

Copy link
Copy Markdown
ContributorAuthor

Code review — V143 consolidation (GLM-5.2 @ max thinking, full-source review)

Reviewed the full text of both changed files (v143.ex, postgres.ex), the neighboring migrations (V140/V141/V142), helpers.ex, and the prefix-migration oracle test.

Verdict: NEEDS-WORK

The migration is conceptually correct — idempotency, prefix-hardening, mirrored down/1, the '143' marker, and safety with respect to V140/V17 were all checked and hold — but one major error-handling issue in the catalog-lookup FK drop contradicts the stated "drops unconditionally" contract and should be fixed before merge.

Findings

[major] lib/phoenix_kit/migrations/postgres/v143.ex:381-384fk_constraint_name/3 swallows query errors.
case RepoHelper.repo().query(...) do {:ok, %{rows: [[name] | _]}} -> name; _ -> nil end treats {:error, _} the same as "no FK found". Scenario: upgrading a phoenix_kit_manufacturing 0.2.0 host with a non-emptyphoenix_kit_machine_types; a transient repo().query failure (lock timeout / connection blip) silently skips dropping the live FK on machine_type_assignments.machine_type_uuid. The surviving FK will then reject new assignment rows carrying entity_data UUIDs — precisely the violation of the soft-reference contract this migration promises to establish, surfacing later as puzzling application-level insert failures.
Fix: distinguish "no rows" from "error" — {:ok, %{rows: []}} -> nil; {:error, r} -> raise("FK lookup failed for #{table}.#{column}: #{inspect(r)}") — or use query! as V116:94 does.

[minor] v143.ex:290warehouse_transfers.source_refs has no GIN index.
V140 adds USING GIN (source_refs) on each of its five source_refs tables and documents it as the reverse-lookup index replacing the dropped FK column. V143 ships the same source_refs JSONB column on transfers without the GIN — any source_refs @> query goes to a seq scan.
Fix: add CREATE INDEX IF NOT EXISTS phoenix_kit_warehouse_transfers_source_refs_index ... USING GIN (source_refs) if the module looks up transfers by source ref; otherwise drop the column or document the omission.

[minor] v143.ex:349warehouse_min_stock.min_quantity lacks a CHECK (>= 0).
The sibling warehouse_stock.quantity (V140:76-84) carries CHECK (quantity >= 0). A negative min-stock is meaningless; the invariant is enforced on the sibling table but missing here.
Fix: add an idempotent CHECK in the V140 style (pg_constraint existence guard in a DO $$ block).

[suggestion] v143.ex:368-385 — document the immediate-query ↔ queued-execute interaction.
fk_constraint_name runs immediately via RepoHelper.repo().query/4 while the preceding CREATE TABLE IF NOT EXISTS is queued by Ecto. This is safe exactly because the FK being sought pre-exists (created by module 0.2.0), not by V143's own queued DDL — but this same trap (immediate repo().query over queued DDL) broke V40/V61. A short comment at fk_constraint_name ("reads pre-V143 state; the V40/V61 flush trap doesn't apply because no preceding queued DDL creates the FK sought here") will prevent a future "corrective" flush().

[suggestion] v143.ex:394-395 — local prefix_str/1 instead of Helpers.qualify_table/2.
helpers.ex moduledoc says new migration code should use qualify_table/2; V140–V142 also keep a local helper, so this is consistency-with-neighbors rather than a violation, and semantically equivalent. Optional: switch new code to the shared Helpers.

Verified clean (no findings)

Idempotency ✓ (all CREATE/ADD COLUMN/CREATE INDEX/CREATE SEQUENCE guarded with IF NOT EXISTS; drop_fk_constraintnil → no-op; maybe_drop_if_empty has IF EXISTS guards). Upgrade branches ✓ (ADD COLUMN IF NOT EXISTS for passport columns deliberately separated from CREATE TABLE so a no-op CREATE on a 0.2.0 host still gets the V2 columns; catalog-lookup FK drop is parameterized and schema-anchored via tc.table_schema = $1; conditional legacy drops only at COUNT=0, otherwise RAISE NOTICE; CASCADE is a valid resilience net since the inbound FK is removed earlier). Marker '143' ✓ (single-step up writes its own COMMENT; down restores '142'; multi-step path consistent). down/1 mirrored and safe ✓ (drops exactly the 5 owned objects + sequence in FK-safe order; does not touch V140 or V17 tables). Prefix-hardening ✓ (#{p}uuid_generate_v7() schema-qualified; table_schema anchors; bare index names; schema-qualified FK targets). Style matches V140–V142 ✓. The documented down caveat on 0.2.0 hosts (drops the pre-existing machine_type_assignments) — accepted, not a bug.

Required testing

The development environment cannot host scratch databases, so these must be run from scratch before merge — please test carefully on a clean PostgreSQL:

  1. Fresh install 0→143 (public schema): full chain via mix test.reset. Verify: phoenix_kit table COMMENT = '143'; the 5 tables + phoenix_kit_warehouse_transfers_number_seq exist; machine_type_assignments.machine_type_uuid has no FK (pg_constraint); warehouse_transfers.performed_by_uuid FK → phoenix_kit_usersON DELETE SET NULL; a repeat PhoenixKit.Migration.ensure_current/2 is a clean no-op.
  2. Fresh install 0→143 (named prefix schema): run the test/integration/prefix_migration_test.exs oracle against a real PG. Assert: marker = to_string(Postgres.current_version()); uuid_generate_v7() lives in the prefix schema; phoenix_kit_machines.uuid DEFAULT contains the schema-qualified call; no CREATE INDEX fails on a qualified-name bug. This has not been run yet (per PR body) — mandatory.
  3. 0.2.0 upgrade path: on a DB with the module-0.2.0 tables plus rows in machine_types, apply V143. Verify: passport columns added; the live machine_type_assignments.machine_type_uuid_fkey dropped; non-empty machine_types left in place with a NOTICE; empty operations/defect_reasons dropped. Separately compare the join-table unique-index names and V1 machines PK/columns against the 0.2.0 schema so the no-op CREATE TABLE IF NOT EXISTS leaves no divergence.
  4. down(..., version: 142): the 5 tables + sequence dropped; V140/V17 untouched; marker = '142'. On a 0.2.0 host, machine_type_assignments is dropped entirely (documented caveat) — confirm this is acceptable.
  5. mix precommit / mix quality.ci: format, credo --strict, compile with warnings-as-errors, dialyzer.
  6. Cross-repo:PHOENIX_KIT_PATH=../phoenix_kit mix test in phoenix_kit_manufacturing and phoenix_kit_warehouse (companion PRs) — their schemas must work against the V143 tables. Merge order per PR body: this PR → locations → warehouse/manufacturing.

@timujinne

Copy link
Copy Markdown
ContributorAuthor

Cross-repo package review — V143 consolidation + module PRs (GLM-5.2 @ max thinking)

Holistic review of the four-PR package: this PR (core V143), BeamLabEU/phoenix_kit_locations#9, BeamLabEU/phoenix_kit_warehouse#3, BeamLabEU/phoenix_kit_manufacturing#3.

Structural checks passed: V143 owns exactly 3 manufacturing + 2 warehouse tables; all 5 module schemas match the DDL column-for-column and index-for-index; the other 6 warehouse tables are owned by core V140 (verified — no orphan tables, no schema/DDL mismatch). V122's @kinds is a superset of the locations PR's kinds. No live code references the removed module migration modules (only historical comments and dev_docs); test helpers use core ensure_current/2. Dependencies resolve across the package.

Verdict: NEEDS-WORK

The package is structurally sound (no blockers), but not merge-ready due to documentation/coordination defects — two majors genuinely break builds/onboarding if followed literally.

Findings

[major] phoenix_kit_manufacturing/AGENTS.md ("Local cross-repo development", "Database & migrations") — test instructions omit PHOENIX_KIT_LOCATIONS_PATH.
The doc gives only PHOENIX_KIT_PATH=../phoenix_kit mix test, but machine_form_live.ex:148 imports PhoenixKitLocations.Web.Components.PlacePicker and machines.ex:532 calls Spaces.full_path/2 — both exist only in the unreleased locations version. Without PHOENIX_KIT_LOCATIONS_PATH, Mix resolves locations ~> 0.2 → 0.2.0 from Hex and compilation fails. The PR body and mix.exs:86-87 state the variable correctly — the doc diverges from both.
Fix: add PHOENIX_KIT_LOCATIONS_PATH=../phoenix_kit_locations to both AGENTS.md sections.

[major] phoenix_kit_warehouse/AGENTS.md ("Project Overview", "Database & migrations") — document sharply diverges from reality.
It claims "Status: scaffold … the module code — schemas, contexts, admin UI, and migrations — is not implemented yet" and "both runtime database tables (transfers, min_stock) are created by V143. This module only defines Ecto schemas that map to those tables". In fact lib/ has ~60 files and 8 schemas; 6 tables (stock, goods_receipts, goods_issues, internal_orders, supplier_orders, inventory_documents) are created by core V140, 2 (transfers, min_stock) by V143. A maintainer reading AGENTS.md learns neither about the implementation nor about V140.
Fix: rewrite both sections to the implemented state and describe the V140/V143 split explicitly.

[major] phoenix_kit_warehouse/lib/phoenix_kit_warehouse.ex:2-17 (moduledoc) — wave-1 features not described.
The moduledoc lists only "stock, stocktakes, internal orders, supplier orders, goods receipt, and goods issue" — no transfers, turnover, deficit control, or multi-warehouse, all present in admin_tabs/0 (lines 210-236) and headlining the PR. Additionally "Hard-depends on phoenix_kit_catalogue … and phoenix_kit_locations" omits phoenix_kit_billing (mix.exs:93, application/0) — an undocumented hard dependency.
Fix: align the moduledoc with admin_tabs and the actual dependency graph.

[minor] phoenix_kit_warehouse/mix.exs:53-55 and phoenix_kit_manufacturing/mix.exs:45-47 — stale comments referencing the removed mechanism.
The test.setup alias comment says schema is applied "via ensure_current/2 plus the module's own migration_module/0", but migration_module/0 was removed from both modules.
Fix: drop the "plus the module's own migration_module/0" phrase.

[minor] phoenix_kit_warehouse/lib/phoenix_kit_warehouse/schemas/{stock,goods_receipt,goods_issue,internal_order,supplier_order,inventory_document}.ex:7Ecto.UUID primary keys vs the UUIDv7 convention.
AGENTS.md and the 2 V143-owned schemas (transfer.ex, min_stock.ex) use UUIDv7, while the 6 V140-backed schemas use Ecto.UUID with autogenerate: true — client-side v4 generation, so the DB's uuid_generate_v7() DEFAULT is dead for Ecto inserts and the warehouse table family mixes v4/v7. Not a bug (both are valid UUIDs), but a stated-convention violation.
Fix: align on UUIDv7 or document the deviation.

[minor] Core PR body vs phoenix_kit/mix.exs:4 — inaccuracy about the version bump.
The core PR body says "CHANGELOG/version bump intentionally left to you", but @version is already "1.7.189" — exactly what warehouse/manufacturing pin (>= 1.7.189). Only the CHANGELOG entry is pending.
Fix: correct the PR body wording.

[minor] Warehouse PR body hides the V140 dependency.
It mentions only "transfers/min_stock tables now ship in core V143"; that the other 6 warehouse tables live in core V140 appears nowhere in the body — the maintainer can't see the full table-ownership picture from the PR alone.
Fix: add a V140 line for the 6 document tables.

[suggestion] Package-wide — no release coordination artifact.
Warehouse (0.1.0) and manufacturing (0.2.0) got no version/CHANGELOG updates (maintainer-owned per project convention), and the required publish order (core 1.7.189 → locations → bump warehouse/manufacturing) that turns the placeholder pins into resolvable ones is recorded nowhere central.
Fix: a single release checklist (publish order + which versions to bump) and CHANGELOG stubs in the three modules.

[suggestion] v143.ex:94-106down/1 unconditionally drops phoenix_kit_machine_type_assignments on upgrade hosts.
Documented in the docstring and PR body, but an operator rolling back V143 on a 0.2.0/V1 host loses type-assignment data (the table's provenance — module vs V143 — is indistinguishable). Acceptable, but worth surfacing in the core release notes, not just the migration docstring.

Required testing

Tests were not executed in the dev environment (no scratch DB). Please test carefully from scratch on a real PostgreSQL before merging the package:

  1. Clean install, public schema: fresh DB, run core chain 1→143; verify all 8 warehouse + 3 manufacturing tables and the V143-owned columns/indexes against the DDL; then down(version: 142) — the 5 V143-owned objects + sequence gone, marker COMMENT = '142', the 6 V140 tables and legacy directories untouched.
  2. Named-schema (prefix) install: repeat feat: Add user creation and editing functionality with LiveView #1 in a named schema — V143 is hand-written execute DDL and core AGENTS.md flags prefix-safety as a regression zone; verify objects and COMMENT land in the right schema, uuid_generate_v7() schema-qualified, users FK resolves.
  3. Idempotent re-run: apply up(143) twice on an already-migrated DB — clean no-op.
  4. Manufacturing 0.2.0/V1 upgrade path: pre-create legacy machine_types with rows + machine_type_assignments with a live FK; run V143 → FK dropped, legacy tables left with a NOTICE, passport columns added idempotently; then walk the LEGACY_DATA_MIGRATION.md runbook and confirm type/operation badges stop reading "Unknown".
  5. Warehouse wave-1 lifecycle: receipt → transfer → issue full cycle, including cancel-from-in_transit reverse posting (ledger atomicity — DB-only check); deficit → supplier order from a deficit row; multi-warehouse scope; turnover report.
  6. Manufacturing lifecycle: machine card end-to-end; entities directory lifecycle (create/translate/trash + "used by N" counters); EntitiesRegistry bootstrap — blueprint provisioning on first start and with no users (graceful defer).
  7. Dependency compile gates: manufacturing mix compilemust fail without the locations PR (PlacePicker/Spaces.full_path) — confirms the declared hard dependency; warehouse must compile against published locations 0.2.0.
  8. Full module suites from scratch:PHOENIX_KIT_PATH=<core #632> (+ PHOENIX_KIT_LOCATIONS_PATH=<locations #9> for manufacturing) mix test on real PostgreSQL — the integration suites have never been run; then, after publishing, resolve the placeholder pins and re-run against Hex pins.

@timujinne

Copy link
Copy Markdown
ContributorAuthor

Review findings addressed in 8f39a50:

  • [major] fk_constraint_name/3 error swallowing — now three explicit clauses; {:error, reason} raises "FK lookup failed for table.column" instead of silently mapping to nil.
  • [minor] GIN index on warehouse_transfers.source_refs — added CREATE INDEX IF NOT EXISTS ... USING GIN (source_refs) in V140 style.
  • [minor] min_quantity CHECK — idempotent CHECK (min_quantity >= 0) via the same DO $$/pg_constraint idiom as V140's warehouse_stock_quantity_non_negative.
  • [suggestion] immediate-query safety comment — added above fk_constraint_name/3 (why the V40/V61 flush trap doesn't apply here).

The PR body has been corrected per the package review: @version is already 1.7.189 (bumped upstream) — only the CHANGELOG entry is pending; a release-notes rollback warning for the down caveat is suggested there as well. Compile clean, no warnings. The "Required testing" checklist from the review above still applies in full — the prefix-schema oracle and the 0.2.0 upgrade path have not been executed in this environment.

@timujinne
timujinne marked this pull request as ready for review July 13, 2026 11:01
# Conflicts:
#	lib/phoenix_kit/migrations/postgres.ex
#	lib/phoenix_kit/migrations/postgres/v143.ex
@ddon
ddon merged commit ff8b739 into BeamLabEU:mainJul 13, 2026
@ddon

ddon commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Renumbered V143 → V144

This PR originally claimed V143. A separate same-day core change (new-login security alerts, phoenix_kit_user_known_devices) landed on main as V143 first, so I rebased this branch onto current main and renumbered your migration V143 → V144 throughout: module name, moduledoc prose, COMMENT ON TABLE version markers on both up/1/down/1, and postgres.ex's moduledoc entry + @current_version. Also renamed the review doc directory to dev_docs/pull_requests/2026/632-manufacturing-warehouse-tables-consolidation/ to match this repo's {pr_number}-{slug} convention (it had used the migration number). Pure renumbering — no DDL, ordering, or logic touched.

Independent verification (second pass)

Re-derived (not just re-read) the two claims in the existing review's "Known bug classes checked" that most benefit from independent eyes:

  • maybe_drop_if_empty/3's raw prefix/table interpolation inside the DO $$ ... END $$ block isn't parameterized.table is always one of three hardcoded literals. prefix is validated before any version module's up/1 runs — PhoenixKit.Migrations.Postgres.with_defaults/2 (postgres.ex:1629) calls Helpers.validate_prefix!(opts.prefix) ([a-z_][a-z0-9_]* only) ahead of dispatch, confirmed by reading with_defaults/2 directly. Safe.
  • fk_constraint_name/3's immediate repo().query/3 vs. the documented "queued execute/1, flushes late" gotcha — reasoned through both flush-timing outcomes rather than taking the moduledoc's claim at face value: on a fresh install, machine_type_uuid/operation_uuid have no REFERENCES clause in this migration's own CREATE TABLE, so the lookup correctly finds nothing regardless of flush timing. On an upgrade host, the table and its FK both pre-date this migration entirely — no flush-ordering question arises either way.
  • Migration-chain shape, re-run fresh post-renumber: mix test test/phoenix_kit/migration_test.exs — 6 tests, 0 failures.
  • Full gate, re-run fresh on the merged branch: mix precommit — clean.

No new findings. Concur with the original review's "Clean, no bugs" verdict. Full write-up (including this note) is in dev_docs/pull_requests/2026/632-manufacturing-warehouse-tables-consolidation/CLAUDE_REVIEW.md on my branch update — maintainerCanModify got toggled off after my renumbering push landed, so I couldn't push that doc update; posting it here instead.

Proceeding to merge.

Sign up for freeto 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

@timujinne@ddon