Add a cheap timezone-label accessor that never queries roles - #654
Conversation
get_timezone_label/2 requires the full get_setting_options/0 map as its second argument, and that map's "new_user_default_role" entry queries Roles.list_roles/0 — so every caller that only wanted a timezone label paid for a role query it never uses. phoenix_kit_newsletters worked around this by keeping its own 28-entry copy of the timezone list, a drift risk (core adds a zone, the copy silently falls behind). - Extract the static timezone list into timezone_options/0 (public) — the single source get_setting_options/0's "time_zone" entry and the new accessor both read from. - Add get_timezone_label/1: resolves against timezone_options/0 only, no role query, no full options map. This is what most callers actually need. - get_timezone_label/2 stays for callers that already have a full setting_options map on hand; it now falls back to timezone_options/0 if the map has no "time_zone" key, and shares the same resolution logic as the 1-arity form. - Migrate the three real call sites that only ever wanted the timezone label: Maintenance.Settings' mount/3 (the one flagged), the user settings component's lazy timezone_options assign, and UserForm's (which also had a redundant SECOND Roles.list_roles/0 call — it already loads all_roles separately for its own role-management UI). - Drop Settings.Authorization's setting_options fetch entirely — it was computed and assigned every mount, and never read anywhere in the module or its template. Tests prove positive/negative/zero/half-hour/unknown offsets resolve identically between get_timezone_label/1 and /2, that the two stay in lockstep for every listed timezone, and — the actual point of the change — that get_timezone_label/1 issues zero repo queries, via a :telemetry-based query counter (same pattern already used in phoenix_kit_crm's import_test.exs).
timujinne
left a comment
There was a problem hiding this comment.
Code review
This PR extracts the static 29-entry timezone-offset list out of Settings.get_setting_options/0 into a new public Settings.timezone_options/0, and adds get_timezone_label/1 that resolves a label from it without ever building the full options map — i.e. without get_role_options/0 → Roles.list_roles/0, which the old /2 path forced on every caller that only wanted one label. The /2 arity is retained as a thin backward-compatible wrapper with a defensive fallback, and four call sites of the same class are corrected: the maintenance-settings mount, the user-settings component's lazy assign, UserForm.mount/3 (which was querying roles twice), and Settings.Authorization.mount/3 (which built and assigned an options map nothing read). I verified the single-source claim, the Authorization removal, the unknown-offset behaviour, and — empirically, with a throwaway probe test — that the telemetry-based "zero queries" test is actually sound rather than a no-op. The change is clean, well-scoped, and well-documented; the points below are minor.
Verdict: APPROVE-WITH-NOTES
Findings
-
[NOTE]
test/phoenix_kit/settings/timezone_label_test.exs:88-114— thecount_repo_queries/1helper attaches to[:phoenix_kit, :test, :repo, :query], which looks wrong at a glance (the conventional Ecto event is[otp_app, :repo, :query]). It is in fact correct here: Ecto derives the telemetry prefix from the repo module name, andPhoenixKit.Test.Repoyields[:phoenix_kit, :test, :repo]. I confirmed this by attaching handlers to both event names against a genuineSettings.get_setting/2call — the PR's event fires once, the[otp_app, :repo, :query]form fires zero times — andget_timezone_label("3")fires zero on the PR's event. The test is meaningful. The existing comment is thorough about theasync/self()reasoning but does not explain why the event name carries:test; a maintainer refactoring this (or copying it into a feature module whose repo isFoo.Repo, where the event would be[:foo, :repo, :query]) could easily "correct" the name and silently turn the assertion into a tautology. Suggest one line noting the prefix is derived fromPhoenixKit.Test.Repo. -
[NOTE]
lib/phoenix_kit/settings/settings.ex:1087(and:985) — the newget_timezone_label/1doctest is correct ("UTC+0 (London, Dublin, Lisbon, Accra)"), but the adjacentget_timezone_label/2doctest still asserts the stale"UTC+0 (GMT/London)", and theget_setting_options/0doctest shows a fictional"time_zone" => [{"UTC-12", "-12"}, {"UTC+0 (GMT)", "0"}, {"UTC+8", "8"}]. These are pre-existing drift (nodoctestdirective runs them, so they don't fail) and not introduced by this PR, but since the PR is exactly about making this list canonical and adds a correct example next to the stale ones, it's a cheap pass to reconcile the two sibling examples while the area is open. -
[NOTE]
lib/phoenix_kit/settings/settings.ex:1091-1092— the/2fallbacksetting_options["time_zone"] || timezone_options()is a real behaviour change: the previous body didEnum.find(setting_options["time_zone"], …), which raisedArgumentErrorwhen the key was absent, so a caller passing a restricted map without"time_zone"crashed; it now resolves against the canonical list. This is strictly more lenient and matches the documented intent, and there are no such in-repo callers (the only/2consumer issettings.html.heex:406via theSettings.get_timezone_label/2delegation, which always passes the fullget_setting_options/0map). Worth being aware of for any external caller that may have been relying on the crash as a guard; nothing to change.
No findings on the four focus areas beyond the notes above: timezone_options/0 is genuinely the single source (the suite even asserts timezone_options() == get_setting_options()["time_zone"] and that /1 and /2 agree for every listed offset, including the 5.5/9.5 half-hour entries and the "0"/unknown fallback); the Settings.Authorization assign removal is safe — authorization.html.heex only reads @saved_settings["time_zone"] as a hidden input and never @setting_options; and unknown stored offsets resolve identically to before ("UTC#{value}", with "0" → "UTC").
- count_repo_queries/1: explain why the event is
[:phoenix_kit, :test, :repo, :query] rather than the conventional
[otp_app, :repo, :query] — Ecto derives the prefix from the repo
MODULE's own name (PhoenixKit.Test.Repo), not the OTP app. Without
this, the event name reads like a typo waiting to be "corrected"
into a tautological assert.
- get_setting_options/0 and get_timezone_label/2's doc examples showed
stale/fabricated timezone data ("UTC+0 (GMT)", "UTC+0 (GMT/London)")
that never matched the real canonical list. Since this PR is exactly
about that list, brought both examples in line with it.
|
Review notes addressed in 026b940: the query-counter helper now documents why its telemetry event is |
Post-merge review of the V154 OG migration + admin list-UI bundle, the cheap timezone-label accessor, and the etcher 0.8.2 patch bump. No bugs found; two low-confidence nitpicks recorded on PR #650 (a likely no-op @disable_ddl_transaction on V154, and a possible array/object mismatch on the OG template's canvas JSONB default). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Bumps version and adds the CHANGELOG entry for the merged-but-unpublished PRs #650 (V154 OpenGraph tables + admin list-UI/breadcrumb/sidebar enhancements), #653 (V155 delivery CRM contact id + per-broadcast dedup), #654 (cheap timezone-label accessor), and #655 (etcher 0.8.2). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Summary
Settings.get_timezone_label/2requires the fullget_setting_options/0map as its second argument, and that map's"new_user_default_role"entry queriesRoles.list_roles/0— so every caller that only wanted a timezone label paid for a role query it never uses.phoenix_kit_newslettersworked around this by keeping its own 28-entry copy of the timezone list, a drift risk (core adds a zone, the copy silently falls behind).timezone_options/0(public) — the single sourceget_setting_options/0's"time_zone"entry and the new accessor both read from.get_timezone_label/1: resolves againsttimezone_options/0only, no role query, no full options map. This is what most callers actually need.get_timezone_label/2stays for callers that already have a fullsetting_optionsmap on hand; it now falls back totimezone_options/0if the map has no"time_zone"key, and shares the same resolution logic as the 1-arity form.Other cheap wins found in the same area
Per review request, I checked every other call site that builds the whole
get_setting_options/0map:Maintenance.Settings.mount/3— the flagged call site. Migrated toget_timezone_label/1.user_settings.ex's (component) lazy:timezone_optionsassign — only ever readsetting_options["time_zone"]. Migrated toSettings.timezone_options/0directly.UserForm.mount/3— same pattern, and it also had a redundant secondRoles.list_roles/0call: it already loadsall_rolesseparately (line 45) for its own role-management UI, so building the full options map on top of that queried roles twice. Migrated toSettings.timezone_options/0.Settings.Authorization.mount/3— computed and assignedsetting_optionsevery mount, but neither the module nor its template ever reads it. Removed the fetch and the assign entirely (dead code, not a timezone-specific fix, but the same class of "building the whole map for nothing").Tests
test/phoenix_kit/settings/timezone_label_test.exs(new):timezone_options/0is non-empty, and is the exact listget_setting_options/0's"time_zone"entry uses.get_timezone_label/1resolves positive/negative/zero/half-hour offsets, and falls back to a bare"UTC#{value}"label for values not in the list.get_timezone_label/1and/2agree for every listed offset (round-trip check).get_timezone_label/2still works against a realget_setting_options/0map, and falls back totimezone_options/0if given a map with no"time_zone"key.get_timezone_label/1issues zero repo queries, proven via a:telemetry-based query counter attached to[:phoenix_kit, :test, :repo, :query](same pattern already used inphoenix_kit_crm'simport_test.exs, ported here since core doesn't have an existing helper for this).Verification
MIX_ENV=test mix compile --warnings-as-errors— clean.mix format --check-formatted— clean.mix credo --strict— clean (0 issues, checked against the touched files directly and the full 697-file sweep via the repo's pre-commit hook).mix dialyzer(via the pre-commit hook) — 0 unignored errors.PGHOST=postgres PGPASSWORD=yourrandompassword MIX_ENV=test mix test test/phoenix_kit/settings/— 13 tests, 0 failures (12 new + the existingsetting_test.exs).CHANGELOG.md and the version in
mix.exsare untouched per project convention.