Skip to content

Add V162 payment-option linkage, slug transliteration, and sub-permission tab fixes - #682

Merged
ddon merged 7 commits into
BeamLabEU:mainfrom
mdon:main
Aug 5, 2026
Merged

Add V162 payment-option linkage, slug transliteration, and sub-permission tab fixes#682
ddon merged 7 commits into
BeamLabEU:mainfrom
mdon:main

Conversation

@mdon

@mdonmdon commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Merge order

This wave is three PRs across three repos, and they are not
independent:

#PRWhy it must come first
1phoenix_kit#682V162 adds payment_option_uuid to phoenix_kit_orders; Utils.Slug gains the :transliterate option both modules call.
2phoenix_kit_billing#15Owns the Order schema — it casts the column V162 creates.
3phoenix_kit_ecommerce#13Checkout writes the payment-option link through billing's changeset.

Renumbered 2026-08-05: upstream merged its own V161
(case-insensitive username via citext) first, so this migration is now
V162. Its down/1 rolls the marker back to '161', not '160'.

Core must be RELEASED, not merely merged, before the module pins can
move — each module's pk_dep(:phoenix_kit, "~> …") floor should be raised
to that release as part of cutting it.

Nothing breaks if they land out of order, by design:

  • Ecommerce checks the migrated version at runtime before writing the
    payment-option link, so on a host below V162 the order simply records no
    link rather than crashing on a missing column.
  • Slug.slugify/2 ignores an option it does not know, so against an older
    core the Cyrillic slug fix silently no-ops and behaves exactly as it does
    today. The tests that assert it are gated on the capability and start
    running by themselves once the core release lands.

So an out-of-order merge costs the new features, not correctness.


Summary

Adds a nullable payment_option_uuid FK (plus its index) to
phoenix_kit_orders, pointing at phoenix_kit_payment_options.

An order records HOW it is to be paid via payment_method — a small
closed vocabulary (bank, stripe, paypal, razorpay). What the
customer actually chose at checkout is a payment-option ROW: an
operator-configured method with its own name, instructions, provider and
billing-profile requirement. The two are not the same thing. Several
options can share one payment_method ("Bank transfer (EU)" and "Bank
transfer (UK)" are both bank), and an option can be renamed or
deactivated after the order is placed.

Without a link the choice was discarded at conversion: nothing on the
order said which option the customer picked, so an operator processing a
bank transfer could not tell which instructions the customer had been
shown, and payment reconciliation had to guess.

Why ON DELETE SET NULL

Deactivating and deleting a payment option is an ordinary operator action.
It must not be blocked by historical orders, and it must not destroy them.
The order keeps payment_method and its metadata snapshot regardless, so
a deleted option degrades to "we know it was a bank transfer" rather than
to nothing.

Verification

add_if_not_exists / create_if_not_exists and matching down/1, so the
migration is re-runnable and reversible. The table comment moves to '161'
on up and back to '160' on down, per the marker convention.


Also in this PR

Three core changes the ecommerce wave surfaced. Each is independent of the
migration and of each other.

Utils.Slug gains opt-in transliteration

A Cyrillic title slugified to an empty string — every character fell
outside [a-z0-9] and was stripped. Callers read empty as "no slug yet",
so a Ukrainian shop's CSV catalogue could not be matched on re-import and
inserted the whole feed again on every run. The mapping covers Russian and
Ukrainian and also strips Latin diacritics.

It is opt-in: existing callers keep today's ASCII-only behavior, and a
consumer passing the new option against an older phoenix_kit gets today's
result rather than an error. phoenix_kit_ecommerce relies on that — its
Cyrillic tests are gated on the capability and start running once this
ships.

A tab gated on a sub-permission registers correctly

A module tab may be gated on a sub-permission ("shop.manage_settings").
Registration pushed that through register_custom_key/2, which rejects a
dotted key — and the raise aborted the rest of the callback, so the
view → permission mapping the admin gate reads was never cached. The module
silently fell back to the coarser base key for core's gate (its own mount
checks still held), and every boot logged a failure for a key that was
declared correctly. Sub-permissions are already declared through
permission_metadata/0, so registration now skips them.

The automatic view gate requires the base of a dotted key

Caching the dotted key means the admin mount gate now resolves one, and it
was calling has_module_access?/2 — direct membership only, which by
contract leaves the base check to its caller. A scope holding an orphaned
"shop.manage_settings" without "shop" would have passed a gate the
sidebar, can?/2 and every module's own check all refuse.

feature_enabled?/1 resolves a dotted key through its parent and
all_module_keys/0 includes sub-keys, so Owner and a properly-granted
Admin both still pass — pinned by tests.

mdon added 2 commits August 5, 2026 04:08
An order records HOW it is paid via payment_method, a small closed
vocabulary. What the customer actually chose at checkout is a
phoenix_kit_payment_options row - an operator-configured method with its
own name, instructions, provider and billing-profile requirement. Several
options can share one payment_method ("Bank transfer (EU)" and "(UK)" are
both bank), and an option can be renamed or deactivated after the fact.
Without a link the choice was simply discarded at conversion: nothing on
the order said which option the customer picked, so an operator processing
a bank transfer could not tell which instructions the customer had been
shown.
Nullable FK + index, ON DELETE SET NULL: deleting a payment option is an
ordinary operator action and must neither be blocked by nor destroy order
history. The order keeps payment_method and its metadata snapshot, so a
deleted option degrades to "we know it was a bank transfer".
@mdon

mdon commented Aug 5, 2026

Copy link
Copy Markdown
ContributorAuthor

⚠️Migration number collision with #681.

#681 (opened ~5h earlier) also adds lib/phoenix_kit/migrations/postgres/v161.ex, for the citext username change. Both PRs claim V161 and both bump @current_version to 161, so whichever merges second needs renumbering to V162 — the file, the module name, the @current_version bump, the index entry in postgres.ex, and the table-comment marker in both up/1 and down/1.

I'd suggest merging #681 first (it is older and self-contained) and renumbering this one to V162; happy to do that rebase on request. Flagging rather than renumbering pre-emptively, since the order is yours to choose.

The two migrations touch different tables (phoenix_kit_users vs phoenix_kit_orders), so there is no conflict beyond the version number itself.

mdon added 3 commits August 5, 2026 06:05
A Cyrillic title slugified to an empty string: every character fell outside
[a-z0-9] and was stripped. Callers read empty as "no slug yet", so a
Ukrainian shop's CSV catalogue could not be matched on re-import and
inserted the whole feed again on every run.
The mapping covers Russian and Ukrainian and also strips Latin diacritics,
so "Cafe Creme" no longer loses its accented characters to the same strip.
It is opt-in: existing callers keep the current ASCII-only behavior, and a
consumer that passes the new option against an older phoenix_kit simply
gets today's result rather than an error.
Also formats v161 to satisfy --check-formatted.
A module tab may be gated on a sub-permission ("shop.manage_settings").
Registration pushed that key through register_custom_key/2, which rejects a
dotted key outright — and the raise aborted the rest of the callback, so the
view to permission mapping the admin gate reads was never cached. The module
silently fell back to the coarser base key for core's gate (its own mount
checks still held), and every boot logged a failure for a key that was
declared correctly.
Sub-permissions are already declared through permission_metadata/0, so
registration now skips them and goes straight to caching the mapping. The
resolved dotted key is safe on both sides of the gate: feature_enabled?/1
resolves it through its parent, and all_module_keys/0 includes sub-keys, so
the Owner still passes.
Caching the dotted key means the admin mount gate now resolves one, and it
was calling has_module_access?/2 — direct membership only, which by contract
leaves the base check to its caller. A scope holding an orphaned
"shop.manage_settings" without "shop" would have passed a gate the sidebar,
can?/2 and every module's own check all refuse. Dotted keys go through
can?/2 instead.
@mdonmdon changed the title Add V161: payment-option linkage on billing ordersAdd V161 payment-option linkage, slug transliteration, and sub-permission tab fixesAug 5, 2026
The prefix oracle (test/integration/prefix_migration_test.exs) failed on the
full chain into a named schema: Ecto emits an added column and its foreign
key as separate statements, and only the column carries IF NOT EXISTS, so
the constraint went in unguarded.
Rewritten to the pattern the rest of the chain uses - a DO block whose every
existence check is anchored on table_schema, with the index name left bare
on CREATE because an index always lands in its table's schema.
The FK check matches ANY foreign key on the column rather than one name, and
down/1 drops both names, so a database that already ran the first build of
this migration re-runs cleanly instead of collecting a second constraint.
Verified by rolling the marker back to 160 on a database carrying the old
shape and re-running the chain: marker back to 161, still one FK and one
index.
@ddon

ddon commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

@mdon please resolve the conflict

Upstream merged its own V161 (case-insensitive username via citext) while
this branch carried a V161 of its own, so both the version constant and the
migration filename collided.
Theirs keeps 161 because it is the one already released into the chain;
ours becomes V162. Its down/1 marker now rolls back to '161', not '160' —
rolling this migration back lands on their citext migration, and a marker
pointing at the wrong version silently skips every rollback between.
@mdonmdon changed the title Add V161 payment-option linkage, slug transliteration, and sub-permission tab fixesAdd V162 payment-option linkage, slug transliteration, and sub-permission tab fixesAug 5, 2026
@ddon
ddon merged commit acac57e into BeamLabEU:mainAug 5, 2026
ddon pushed a commit that referenced this pull request Aug 5, 2026
Reviews the UrlState / V161-citext / V162-payment-option wave merged on
main, and fixes what it turned up.
Fix: `get_user_by_email_or_username_and_password/3` hand-rolled its case
folding as `fragment("LOWER(?)", u.username)`, which matches no index in
the chain. V161's whole premise is that comparison semantics come from
the column type, so with `username` now `citext` plain equality is both
correct and index-backed via `phoenix_kit_users_username_uidx`. This was
the only username lookup still sequentially scanning the users table, on
the one endpoint reachable without authenticating.
Fix: the `PhoenixKitUrlState` JS hook registered `handleEvent` on the
LiveSocket but only removed its `popstate` listener in `destroyed()`, so
each remount left another live callback behind.
Add: `test/phoenix_kit/migrations/v162_test.exs`. V162 shipped with no
test. Pins `ON DELETE SET NULL` in particular — that is the migration's
whole design decision, and a later refactor reaching for a plain
`references/2` would silently make it `RESTRICT` with nothing failing.
Also renames the V162 PR-draft doc out of `680-v161-…`, which named the
pre-renumber identity and collided with PR #680's own directory.
Full findings, and the two recorded-but-unfixed gaps, in
dev_docs/pull_requests/2026/680-682-post-merge-review/CLAUDE_REVIEW.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request Aug 9, 2026
The 161 slot is taken upstream by two open PRs (#681 our own citext-username
branch, #682 mdon's payment-option linkage), so this repair migration moves to
the first free slot when they land. Records why it cannot be renumbered
pre-emptively (contiguity: a hole makes change(1..N) crash on the missing
module — release_check and its test both catch it, verified empirically) and
the exact renumber ritual incl. manifest regeneration.
ddon pushed a commit that referenced this pull request Aug 9, 2026
Upstream took 161 (our own citext-username PR #681) and 162 (payment-option
linkage #682, itself renumbered off 161), so the flush-bug repair migration is
now V163: file+module, self-stamp '163', down restamp '162',
@current_version 163, moduledoc LATEST marker, and the guard test renamed with
@exempt_version 163. Spec counters: 163 files / 28,000 lines, floor-candidate
remainders recalculated (deleted-line figures unchanged — v01..v147 have never
been touched). The generated manifest is now STALE (its chain_hash pins the
pre-merge file set) and must be regenerated before it is trusted.
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

@mdon@ddon