Add locale and priority options to the country selects - #708
Conversation
The country dropdown was English-only and strictly alphabetical: an
Estonian admin picked their own country from 'Estonia' between 'Eritrea'
and 'Eswatini', with 249 others in front of it.
countries_for_select/1 and eu_countries_for_select/1 now take:
* :locale — country names via BeamLabCountries.Translations, defaulting
to the active PhoenixKitWeb.Gettext locale and falling back to the
English name for any locale the data has no translations for, so a
host only ever loses the translation, never the entry
* :priority — alpha-2 codes pinned to the top in the order given,
defaulting to config :phoenix_kit, :country_select_priority
Sorting moves to the localized name, accents folded via NFD — plain byte
order would exile every accented name past 'Z'.
Both functions keep their zero-arity call shape, so existing callers
(including phoenix_kit_billing's core_compat MFA list) are unaffected.
The select list was translated but the single-country lookup was not, so anywhere a country is shown as text rather than picked from a dropdown — phoenix_kit_billing's company address block, format_company_address/0 — stayed English on a translated page. Takes the same :locale option and the same per-country English fallback. The default argument keeps the /1 shape that phoenix_kit_billing's core_compat probes for.
Review follow-ups on this branch. The comment above sort_by_name/1 had the collation backwards. It claimed byte order "would exile every accented name past Z", but Estonian sorts Ü at the very end (after W, Õ, Ä, Ö) and Swedish sorts Å, Ä, Ö after Z — so that position is correct there, and folding is what breaks it, pulling "Ühendkuningriik" into the U block. Folding stays, because it is right for most Latin-script locales and the BEAM ships no ICU, but it is now documented as the approximation it is, in the comment and in the @doc. The @doc also now states what the switch to BeamLabCountries.Translations actually changed: the names differ from the country struct's :name field even in English, for 20 of 250 countries — "Czech Republic" becomes "Czechia", "Turkey" becomes "Türkiye", and North Korea moves from the K's to the N's. A host that never passes :locale still sees new strings and a new order. The supported-locale set is named too, read from Translations.supported_locales/0 rather than from memory. countries_for_select/0 cost 1.27 ms per call and runs in LiveView mount, twice per connection. The localized, sorted entry list is now memoized in :persistent_term per locale and source, which takes a warm call to 0.018 ms. Priority pinning stays outside the cache, so a :country_select_priority change still takes effect immediately. Verified equivalent to the pre-cache implementation across 144 combinations of locale, priority and source. Bad opts raised from inside Keyword.get/3, pointing the caller at Keyword instead of at their own call. `when is_list(opts)` now names this module; get_country_name/2 needed its catch-all narrowed to a non-binary code, or it would have swallowed get_country_name("EE", "ru") — a realistic slip, since the option is :locale. Two tests were doing nothing. The sort test re-derived the implementation's own sort key and compared the output to it, so it could not fail for any collation; it now asserts literal neighbours (Azerbaijan < Åland Islands < Bahamas, and two more pairs), and dropping the fold turns it red. The "keeps every country" test compared lengths, 250 == 250 by construction; it now compares the code sets. Added the missing coverage for the zero-arity contract phoenix_kit_billing's core_compat probes, and for the default-locale branch, which nothing exercised.
34b38c5 to
ff6e891
Compare
timujinne
left a comment
timujinne
left a comment
There was a problem hiding this comment.
Review summary — two independent reviewers
Reviewed twice, independently, from the same brief: Claude Opus (adversarial, A/B against upstream/main, plus the live host app via Tidewave) and GLM-5.2 (elixir-review). Both returned NEEDS-WORK. Everything they raised is now fixed in ff6e8917; the three commits were also reworded to start with Add / Update / Fix per AGENTS.md.
The two findings that mattered
1. The comment above sort_by_name/1 had the collation backwards. It claimed byte order "would exile every accented name past 'Z' — 'Ühendkuningriik' after 'Zimbabwe' — which reads as a bug". In Estonian, Ü belongs at the very end, after W, Õ, Ä, Ö, so that position is correct and the NFD folding is what breaks it: verified on the live app under locale: "et", "Ühendkuningriik" lands at index 240, inside the U block, between "Uus-Meremaa" and "Valgevene". Swedish is worse — folding puts "Åland" at index 14, between "Azerbajdzjan" and "Bahamas", and "Österrike" right after "Oman", when Swedish collation puts Å Ä Ö after Z.
Folding stays — it is right for the majority of Latin-script locales and the BEAM ships no ICU — but it is now documented as the approximation it is, in both the comment and the @doc, naming the locales it gets wrong so a host needing strict collation knows to sort the result itself.
2. The English output changed too, and that was undocumented. Translations.get_name(code, "en") differs from the country struct's :name for 20 of 250 countries, so a monolingual host that never sets a locale gets different strings and a different order out of countries_for_select/0:
GB "United Kingdom of Great Britain and Northern Ireland" -> "United Kingdom"
US "United States of America" -> "United States"
CZ "Czech Republic" -> "Czechia"
TR "Turkey" -> "Türkiye"
KP "Korea (Democratic People's Republic of)" -> "North Korea" # K -> N in the list
Now stated in the @doc, examples verified against the resolved dep rather than quoted from the review.
Also fixed
- Performance.
countries_for_select/0went from 0.902 ms onupstream/mainto 5.754 ms on this branch (300 iterations, warmed) — in a function that runs in LiveView mount, twice per connection. The localized, sorted entry list is now memoized in:persistent_term, keyed by locale and source so the EU subset cannot collide with the full list: 1.266 ms → 0.018 ms per warm call. Priority pinning stays outside the cache, so a:country_select_prioritychange still takes effect immediately — checked at runtime, not just by reading. Equivalence with the pre-cache implementation was verified across 144 combinations (15 supported locales +xx+ru-RU+ nil, × 4 priority lists, × both functions): all identical. - Option guards.
countries_for_select(%{locale: "ru"})andget_country_name("EE", "ru")raisedFunctionClauseErrorfrom insideKeyword.get/3, pointing the caller atKeyword.when is_list(opts)now names this module.get_country_name/2needed more than a guard: its catch-alldef get_country_name(_, _), do: nilwould have swallowed the binary-code-plus-bad-opts case, so it is narrowed towhen not is_binary(country_code).get_country_name("EE", "ru")is the realistic slip here, since the option is:locale. - Two tests were doing nothing. The sort test asserted
names == Enum.sort_by(names, <the implementation's own sort key>)— it re-derived the key it was supposed to be testing and therefore could not fail for any collation, which is exactly why it did not catch finding 1. It now asserts literal neighbours (Azerbaijan < Åland Islands < Bahamas, plus two more pairs); dropping the fold in a scratch copy turns it red (left: 249, right: 15— "Åland Islands" falls to second-to-last). The "keeps every country" test compared lengths, 250 == 250 by construction of the dataset; it now compares the alpha-2 code sets. - Missing coverage. Nothing exercised the zero-arity contract or the
active_locale/0default branch. Added, withCode.ensure_loaded?/1beforefunction_exported?/3— without it the probe answersfalseon a cold VM, which the existing arity test also got wrong.
Verified and dismissed
get_country_name/1's locale sensitivity is deliberate, and its blast radius was checked rather than assumed: no caller in this repo at all; inphoenix_kit_billing,order_form.ex:372andformat_company_address/1both only build strings for display. Nothing persists, compares or caches the value. The one real consequence — the same order printed from a web request and from an Oban job can name the country differently — follows from the feature and is now documented.code in pinned_codesinside a reject is O(n·m) as written but measured harmless: 250-code priority 2.86 ms, 1 code 4.58 ms, empty 4.08 ms (full priority is fastest, since it skips sorting). It is now moot anyway — the sort it fed is cached.- No country vanishes or duplicates in any of the 16 supported locales: 250 entries, 250 unique codes, zero duplicate display names each.
- Two tests do pass against a revert — "still answers nil for an unknown country" and "keeps its one-argument shape". That is their purpose: they are backward-compat guards. Swapping in
upstream/main's implementation fails 13 of 17.
One thing this PR cannot fix
:locale only translates for locales the installed beamlab_countries ships. The pinned 1.1.0 has 15 and Estonian is not among them — in a clean checkout of this branch, get_country_name("GB", locale: "et") still answers "United Kingdom of Great Britain and Northern Ireland". Estonian works in our host app only because it overrides the dep with a local checkout carrying two unpublished commits — and that checkout is still stamped @version "1.1.0", identical to the published release, so a mix deps.get that ever resolves to Hex would silently drop Estonian with no version change to signal it. The fix belongs in beamlab_countries (BeamLabEU/beamlab_countries#9) plus a release; noting it here so the dependency between the two is on record.
mix format --check-formatted, mix credo --strict, mix compile --warnings-as-errors and the module's 17 tests are all green. The full suite could not run in this container (no local PostgreSQL; pk-test dies with an environment fault unrelated to this diff), but a targeted pk-test over this test file and test/integration/users/organization_test.exs — the one in-repo caller — passed 40 tests, 0 failures.
The priority list was compile-time config only, so reordering the dropdown meant editing a file and redeploying — and nothing in the admin UI said the feature existed. `country_select_priority` is now a setting, edited on Admin -> Settings -> Organization next to the company's own country, which is where "where does this business operate" already lives. The setting wins; `config :phoenix_kit, :country_select_priority` stays as the fallback for a blank or unset value, so hosts that never open the page keep the behaviour they configured. It lands where it costs nothing: priority pinning was deliberately left outside the memoized sorted list, so an edit takes effect on the next render with no cache to invalidate. The read goes through get_setting_cached/2, which is consulted before the update-mode short-circuit and therefore resolves without a database; a settings read can raise on an unowned checkout and exit on a dead pool, so both are caught — a country dropdown must not depend on the database being up. Input is normalized on save through the new parse_priority/1 and known_country_codes/1: any separator an operator actually types is accepted, codes are upper-cased and deduplicated, and codes that name no country are dropped, so what the field shows after saving is exactly what the dropdown will do.
Review follow-ups on the setting added in the previous commit.
The edit was discarded whenever an unrelated company field failed
validation: the save ran inside save_company_info/2, which
handle_event("save_company", …) only reaches once validate_company_data/1
returns []. Worse, the error branch skips load_settings/1, so the typed
text stayed on screen with nothing to signal the loss. On a fresh install
it was circular — every required field is empty, so the operator could
not save a priority list until they had filled in the whole company form,
which is exactly when picking a country out of 250 is hardest. The
priority is an independent setting and now persists on every submit.
Total rejection was reported as success: "Estonia, USA" left nothing
after known_country_codes/1, stored blank, silently reverted the dropdown
to the config list, and flashed "saved". Rejected codes are now named in
the flash, and a wholesale rejection says so instead of implying success.
An operator could not turn pinning off at all — blank means "fall back to
config", and a stored empty string is indistinguishable from an absent
key at the settings layer (get_setting_cached/2 answers with the default
for both). So "none" is now an explicit sentinel, honoured over the
config and round-tripped in the field, and it is checked before
known_country_codes/1 — it is not a country code and would otherwise be
dropped like any other unknown one.
Also: the :priority and :locale defaults were evaluated eagerly, which
since the previous commit meant a settings read on every call even when
the caller passed the option; they move behind Keyword.fetch/2. The
rescue/catch around the settings read is gone — get_setting_cached/2 and
the get_setting/2 it falls through to already rescue and catch :exit, so
those handlers were unreachable and a bare rescue would only swallow a
future programmer error. update_setting/2's {:error, changeset} is no
longer discarded: 250 codes joined is 998 characters against a 1000-char
column, so one more country would have turned this into a silent no-save.
Verified against the running app: a submit with a blank company name
still stores "LT, LV"; "Estonia, USA" flashes the rejection and stores
nothing; " NoNe " stores "none" and the dropdown starts at Afghanistan
with the config still set to EE FI LV LT SE.
Second round — the admin setting (
|
Two things were wrong with the first cut of this setting. The compile-time config stayed underneath it as a fallback. That meant a host installing PhoenixKit found country dropdowns already reordered before anyone had chosen anything — whichever countries the config's author happens to serve — and the operator could not tell the difference between "reordered on purpose" and "reordered by a default they never saw". The setting is now the only source: nothing is pinned until someone picks something, and an untouched install is plain alphabetical. Losing the config left the question of where a new host starts, and the answer is in the data rather than in a constant. suggested_priority/2 proposes the organization's own country followed by its nearest neighbours, computed from the country centroids the dataset already carries — Estonia gets Latvia and Finland, Germany gets Luxembourg and the Netherlands, Singapore gets Malaysia and Indonesia. The settings card offers it; nothing is applied until the operator accepts. The free-text field is gone too. It asked an operator to type alpha-2 codes, then silently dropped whatever it did not recognize, so the whole class of "was my input taken?" problems came from the input method. The new card on Admin → Settings → Organization is a list: pick a country from the dropdown, add it, reorder with the arrows, remove with the cross. Every action writes immediately — there is no second field whose validation can hold the list hostage, which is what made the earlier save-behind-validation bug possible. Estonian and Russian translations for the card's strings are included; the config key is removed from the host app in the same change.
Three things the card got wrong. It sat in the right-hand column with bank and tax details, away from the company country it is about. It now stacks directly under the company card. The picker was a plain select over 250 options with no search, and its options came from countries_for_select/1 WITH the pinning applied — so the first thing it offered was whatever is already pinned, and picking "the top one" added a country the operator never chose. It is now the SearchableSelect component, and its options are built with `priority: []` so the list you pick from is never reordered by the list you are editing. Ordering was two arrow buttons per row. This repo already ships DnD for exactly this (`<.draggable_list>` over the SortableGrid hook), so the rows are draggable by a handle instead. `reorder_main_countries` takes the order SortableGrid pushes, but keeps only codes that were already pinned and re-appends anything the payload omitted, so a forged `ordered_ids` can neither add a country nor silently drop one. Verified against the running app: dragging to SE, LV, EE, FI, LT stores that order; a payload of ["ZZ", "LT"] stores "LT, SE, LV, EE, FI" — unknown code ignored, nothing lost. The picker now starts at Afganistan, Ahvenamaa, Albaania rather than at the pinned five.
Review follow-ups on the main-countries card. Two handlers crashed the LiveView on a payload the browser would never send but a WebSocket client can: add_main_country mapped String.upcase/1 over a non-binary code, and reorder_main_countries called Enum.filter/2 on a non-list ordered_ids. Both are guarded now, and reorder and remove gained the catch-all clause add already had — the asymmetry was the bug. Data integrity was never at risk: a forged payload still cannot add an unknown country, drop an omitted one, or duplicate an entry. Dragging was the only way to reorder, which left the list unorderable for keyboard and screen-reader users, and unorderable at all wherever SortableJS cannot be fetched — the hook loads it from a CDN, so a strict CSP or an offline deploy silently disables it. The up/down buttons are back alongside the drag handle, drag stays primary. Their msgids were still in all four catalogues from the earlier iteration, so nothing new needed translating. The card also updated only itself: the Company card's own country select sat directly above it, unchanged until a reload, and a second admin session never learned about the edit at all. The write path now refreshes :countries and broadcasts on the channel this LiveView already subscribes to. It also stops writing when nothing changed — an empty Add, a remove of an absent code, a drag that ends where it started. Smaller: the suggestion went stale after switching the company country; suggested_priority/2 raised on a non-integer :limit, because Erlang term ordering makes "4" > 0; and its docstring promised Estonia Sweden when the data answers Latvia, Åland, Finland, Lithuania. The gap that let both crashers ship was that nothing tested the card at all. 19 integration tests now cover the four handlers against a real database — forged payloads, the reorder invariants, dedup, the move clamps, the no-op writes, the select refresh and the cross-session broadcast.
Third round — the settings card (
|
Merged this sweep: #707 GitHub and Amazon Bedrock integration providers, plus account identity and permission context on the aws_ses test verdict #708 locale and priority options on the country selects, and the Main countries card that stores the pinned list #709 point the editor bundle at the leaf release hex resolves, with a test holding the two together #710 the scheduled-jobs sweep dying at :debug, the queue no upgrade ever added, and an Oban cron-queue check in doctor Post-merge fixes: Every ensure_*_queue helper now shares #710's hardened implementation as ensure_queue/4. The six that predated it each hand-rolled the same string surgery on the host's queues: list and reproduced the same defects, which are worse than the missing queue they were adding -- a bad insert corrupts config.exs rather than leaving a job stuck. Their unanchored guards were also satisfied by a key merely ending in the queue's name, so a host's own push_notifications: 5 silently suppressed the real notifications queue. The Organization settings page no longer concatenates a nilable flag into a country label; that would have raised in load_settings/1 and taken the whole page down. Review docs for all four PRs under dev_docs/pull_requests/2026/. Gate clean, 3463 tests + 38 doctests passing against a real database. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Problem
CountryData.countries_for_select/0returns all 250 countries as English names in one flat alphabetical list. Two consequences for a non-English host:Estoniasits betweenEritreaandEswatini, 60-odd rows down, and the countries they actually invoice are scattered across the list.beamlab_countrieshas carried per-locale country names all along (BeamLabCountries.Translations); nothing in core used them.Change
countries_for_select/1andeu_countries_for_select/1take two options::locale— country names in that locale, defaulting to the activePhoenixKitWeb.Gettextlocale (dialects reduce to their base:ru-RU→ru). A locale with no translation data falls back to the English name per country, so a host only ever loses the translation, never the entry.:priority— alpha-2 codes pinned to the top in the order given, defaulting toconfig :phoenix_kit, :country_select_priority. A host that serves one region configures it once instead of patching call sites.Sorting moves from the English name to the localized one, with accents folded through NFD — plain byte order exiles every accented name past
Z(ÜhendkuningriikafterZimbabwe), which reads as a bug in any locale that uses them.Both functions keep working with no arguments, so every existing caller — including
phoenix_kit_billing'score_compatMFA list, which probes{CountryData, :countries_for_select, 0}— is unaffected.Verification
New
test/phoenix_kit/utils/country_data_test.exs(11 tests, the module had none): translation, dialect reduction, unknown-locale fallback, no country lost, pin order, no duplicate pinned entry, unknown/lower-case codes in:priority, the config default, and that the tail is sorted by the localized name.Also exercised end-to-end in a host app (Andi): the order form's country select renders
Eesti / Soome / Läti / Leedu / Rootsifirst, then the rest alphabetically, inet,ruanden.Notes
doctestwas added: this module's existing examples are written against the bareCountryData.alias, which doctests cannot resolve. The new examples follow the same house style, so they are documentation only.beamlab_countrieswith anetlocale (Add Estonian country name translations beamlab_countries#9). Without it,etsimply falls back to English — this PR does not depend on it.CHANGELOG.mdand@versionare left to the maintainer.