Localize CRM admin pages and reorder settings tab - #7
Conversation
Move CRM settings tab priority from 650 to 927 so it sits beside Emails (925), Languages (928), and Legal (929) in the settings sidebar instead of jumping to the top of the list. Switch the CRM admin LiveViews from `PhoenixKitWeb.Gettext` to the module-owned `PhoenixKitCRM.Gettext` so user-facing strings live in this package's catalogue: * `settings_live.ex` — replace explicit `Gettext.gettext(PhoenixKitWeb.Gettext, ...)` calls with the local `gettext()` macro * `crm_live.ex` — add `use Gettext, backend: PhoenixKitCRM.Gettext`; wrap the previously-hardcoded "CRM" title and "Enabled" / "Disabled" badge labels * `organizations_view.ex`, `role_view.ex` — add the same backend so the H1, count badge, "Columns" button, error flashes, and empty-state copy resolve through CRM's catalogue * `column_management.ex` — translate the "Columns updated" / "Failed to save columns" flash messages via `Gettext.gettext(PhoenixKitCRM.Gettext, ...)` Extract the new msgids into `priv/gettext/default.pot` and fill English, Russian (3-form plurals), and Estonian (2-form plurals) translations across `default.po`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Note: This was the initial flat-summary review. The actual review with inline comments on each finding is at #pullrequestreview-4357897795 — read that one instead.
Code Review — PR #7
Reviewer: Claude Opus 4.7 (xhigh effort, 5 finder angles + sweep)
Verdict: Request changes — one priority collision is a real bug; the rest are quality/maintenance issues.
The diff is technically sound — mix compile is clean, 81 tests pass, the double-use Gettext correctly shadows the host backend so all gettext/1 macros in the four touched LiveViews resolve to PhoenixKitCRM.Gettext. Translations are present in en/ru/et with correct plural forms (ru nplurals=3, et nplurals=2). But the new tab priority collides with another sibling module that the upstream repo can't see locally, and the localization catalogue has a few drift issues worth fixing before merge.
Issues
1. ⛔ Priority 927 collides with phoenix_kit_ecommerce's settings tab
File: lib/phoenix_kit_crm.ex:155
priority: 927,phoenix_kit_ecommerce/lib/phoenix_kit_ecommerce.ex:279 already uses priority: 927 for :admin_settings_shop (label "E-Commerce", also parent: :admin_settings). Any host app that installs both modules will see non-deterministic ordering of the two tabs in the settings sidebar (the tie-break depends on Dashboard.Registry insertion order).
Full neighborhood, sorted:
| Priority | Module |
|---|---|
| 920 | Referrals |
| 921 | Publishing |
| 923 | Customer Support |
| 925 | Emails |
| 926 | Billing |
| 927 | E-Commerce (existing) |
| 927 | CRM (this PR) |
| 928 | Languages |
| 929 | Legal |
| 930 | SEO |
| 931 | Sitemap |
| 932 | Maintenance |
| 933 | Media |
Suggested fix: Use 924 (free slot between Customer Support 923 and Emails 925) — still satisfies the original "near Emails/Legal" intent.
2. ⚠️ Russian "Enabled" / "Disabled" use masculine adjectival form
File: priv/gettext/ru/LC_MESSAGES/default.po:224, 234
msgid "Enabled"
msgstr "Включён"
msgid "Disabled"
msgstr "Отключён"
Включён / Отключён are masculine short adjectives — fine for the badge sitting next to "CRM" today (CRM is conventionally masculine in Russian), but the same msgid is shared across the backend. If a future caller renders the badge for a feminine subject (e.g. "роль", "функция", "система"), it will read grammatically wrong.
Suggested fix: Use the neuter / status forms which work for any subject:
msgstr "Включено"
msgstr "Отключено"
3. 🟡 column_management.ex could use the gettext/1 macro instead of Gettext.gettext/2
File: lib/phoenix_kit_crm/web/column_management.ex:106, 114
|> Phoenix.LiveView.put_flash(
:info,
Gettext.gettext(PhoenixKitCRM.Gettext, "Columns updated")
)The fully-qualified Gettext.gettext/2 is a runtime function — mix gettext.extract cannot see it, which is why these two msgids had to be added to priv/gettext/default.pot by hand. However, both modules that use PhoenixKitCRM.Web.ColumnManagement (role_view.ex:9, organizations_view.ex:13) now declare use Gettext, backend: PhoenixKitCRM.Gettext themselves, so when the macro expands inside them, a plain gettext("Columns updated") resolves to PhoenixKitCRM.Gettext at compile-time and gets picked up automatically by extraction.
Suggested fix: Replace the two Gettext.gettext(PhoenixKitCRM.Gettext, ...) calls with gettext(...), then re-run mix gettext.extract; remove the manual entries from default.pot. This eliminates the maintenance gotcha where a future contributor adds a third flash and forgets to hand-maintain the .pot.
4. 🟡 priv/gettext/default.pot doc-block is now stale
File: priv/gettext/default.pot:5-12
The preamble still says:
Two groups of msgids are maintained manually (NOT auto-extracted):
- Tab labels — plain strings in
Tab.new!(label: ...).- Column labels — string literals stored in module attributes
This is now untrue:
- The
"CRM"and"Organizations"msgids (still listed under the## Tab labels (manually maintained …)section header at line 23) are now also produced bygettext("CRM")/gettext("Organizations")incrm_live.exandorganizations_view.ex, so the .pot entries carry#, elixir-autogenand line refs. They are no longer manually maintained. - A third group of manually-maintained msgids now exists: the
column_management.exflash messages ("Columns updated", "Failed to save columns"), which sit at lines 222–228 in the middle of the alphabetic autogen block, without#, elixir-autogen.
Two options:
- (Preferred) Adopt fix #3 above to eliminate the third manual group entirely, then refresh the doc-block.
- Otherwise rewrite the doc block to: "Three groups… 3. Flash messages emitted from macros via
Gettext.gettext/2" and move the two new entries up next to the column-config block at lines 41-75, away from the autogen interleave.
5. 🟡 Latent fragility: tab-label "CRM" depends on macro-site existence
File: priv/gettext/default.pot:24-28
The tab label Tab.new!(label: "CRM", …) in phoenix_kit_crm.ex is a plain string — not extracted. The msgid "CRM" survives in the catalogue today because of the macro call gettext("CRM") in crm_live.ex:17/:38. If a future refactor renames/removes those two macro sites, mix gettext.extract will drop the "CRM" msgid (the entry now has #, elixir-autogen), and the sidebar tab label loses its translation.
Suggested fix: Add an explicit manual entry without #, elixir-autogen for "CRM" (and "Organizations", "Overview") under the ## Tab labels header — mirroring how priv/gettext/default.pot:30-32 already does for "Overview". Then either lock the manual ones with #~| style or keep them deduplicated.
6. ℹ️ status_badge strings stay English regardless of locale (upstream, out of scope)
File: lib/phoenix_kit_crm/web/role_view.ex:181-187, lib/phoenix_kit_crm/web/organizations_view.ex:180-187
defp crm_status_html(true), do: ~H|<.status_badge status="active" size={:sm} />|
defp crm_status_html(_), do: ~H|<.status_badge status="inactive" size={:sm} />|PhoenixKitWeb.Components.Core.Badge.status_label/1 does String.replace("_", " ") |> String.capitalize() — no gettext. The rendered "Active"/"Inactive" stays English in ru/et locales. Out of scope for this PR (fix belongs upstream in phoenix_kit), just flagging for a follow-up issue.
7. ℹ️ No CHANGELOG entry / version bump
Files: mix.exs, CHANGELOG.md
mix.exs still pins @version "0.2.2" (set by PR #5). CHANGELOG.md [0.2.2] entry describes ColumnModal/CellFormat strings only — it doesn't mention the LiveView backend swap, the new column_management.ex flashes, or the settings-tab priority change. Per project convention (Prepare 0.2.1 release, Prepare 0.2.2 release commits), this PR should bump to 0.2.3 with a new entry. Not a blocker if you batch the bump into a separate release PR — but worth deciding before merge.
Positive notes
- Double
use Gettextis safe: PhoenixKitWeb's:live_viewmacro injectsuse Gettext, backend: PhoenixKitWeb.Gettextfirst, the PR's explicituse Gettext, backend: PhoenixKitCRM.Gettextoverwrites@__gettext_backend__, and all subsequent macros resolve to the CRM backend. Verified againstdeps/gettext/lib/gettext.ex:611-628. - Plural forms are correct in both Russian (
роль / роли / ролей,пользователь / пользователя / пользователей,организация / организации / организаций) and Estonian (roll / rolli,kasutaja / kasutajat,organisatsioon / organisatsiooni). - No previously-translated msgid is lost. The deleted
Gettext.gettext(PhoenixKitWeb.Gettext, ...)calls referenced msgids that the host PhoenixKit catalogue had no translations for, so switching backends is a strict improvement. - No test breakage. No tests assert on the literal strings the diff localized.
- UTF-8 hygiene clean. The em-dash
—is consistent across.potand all three.pofiles.
* Fix priority collision (`lib/phoenix_kit_crm.ex:155`) — change the CRM settings tab priority from 927 to 924. Priority 927 is already used by `phoenix_kit_ecommerce` for its `:admin_settings_shop` tab; 924 sits in the free slot between Customer Support (923) and Emails (925). * Use neuter Russian forms for the Enabled/Disabled badge (`Включено` / `Отключено` instead of masculine `Включён` / `Отключён`) so the same msgid stays grammatically correct if reused for non-masculine subjects. * Switch the `column_management.ex` flash messages from the fully-qualified `Gettext.gettext(PhoenixKitCRM.Gettext, ...)` runtime call back to the `gettext/1` macro — the consuming modules (`role_view.ex`, `organizations_view.ex`) already declare `use Gettext, backend: PhoenixKitCRM.Gettext`, so the macro expands inside the host with the CRM backend and the msgids are now picked up by `mix gettext.extract` automatically. The manually-maintained entries in `default.pot` are replaced by auto-extracted ones; the doc-block returns to describing exactly two manual groups (tab labels, column labels). * Strip the `#, elixir-autogen` flag from the manually-maintained `CRM` / `Organizations` / `Overview` tab-label entries in `default.pot` so the entries survive even if the macro call sites in `crm_live.ex` / `organizations_view.ex` are ever removed. Add a doc-comment near the entries explaining that future `mix gettext.extract` runs will re-add the flag and the reference must be re-stripped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Review fixes — commit
|
| # | Finding | Fix |
|---|---|---|
| 1 | ⛔ Priority 927 collides with phoenix_kit_ecommerce :admin_settings_shop |
lib/phoenix_kit_crm.ex:155 — priority 927 → 924 (free slot between Customer Support 923 and Emails 925) |
| 2 | priv/gettext/ru/LC_MESSAGES/default.po — switched to neuter "Включено"/"Отключено" so the msgid works for any grammatical gender |
|
| 3 | 🟡 column_management.ex flashes use runtime Gettext.gettext/2 (invisible to extractor) |
Switched both flashes to gettext/1 macro. The consuming modules (role_view.ex, organizations_view.ex) already use Gettext, backend: PhoenixKitCRM.Gettext, so the macro expands against the CRM backend at compile time. mix gettext.extract now picks them up automatically — manually-maintained entries removed from default.pot |
| 4 | 🟡 Stale .pot doc-block ("Two groups… maintained manually") |
Doc-block is accurate again now that fix #3 eliminated the third manual group |
| 5 | 🟡 Latent fragility: tab-label "CRM" depends on macro-site existence | Stripped #, elixir-autogen flag from "CRM" / "Organizations" / "Overview" in default.pot so the entries survive if the macro call sites are ever removed. Added a doc-comment near the block warning that future mix gettext.extract runs will re-add the flag and it must be stripped again |
Verification
mix compile --warnings-as-errors— cleanmix test— 81 tests, 0 failuresmix gettext.merge priv/gettextafter the .pot changes — 0 new, 0 removed, 58 unchanged, 0 reworded (fuzzy) — all existing translations preserved across en/ru/et
⏭️ Deferred (out of scope for this PR)
- Finding 6 —
<.status_badge>in upstreamphoenix_kitrenders untranslated "Active"/"Inactive". Belongs in aphoenix_kitcore PR, not this module. - Finding 7 — No
CHANGELOG.mdentry /@versionbump. Defer to a release-prep PR per the project's convention (Prepare 0.2.x releasecommits).
timujinne
left a comment
There was a problem hiding this comment.
Code review — a4b08e3
Reviewer: Claude Opus 4.7 (xhigh effort: 5 finder angles × verify × sweep).
Verdict: 1 blocker, 2 quality issues, 1 catalogue-drift item — see inline comments. 3 findings without an inline anchor (unchanged lines or doc-block outside the diff hunks) are captured here:
Findings without inline anchor
🟡 priv/gettext/default.pot preamble is now stale (lines 5-12). The doc-block says "Two groups of msgids are maintained manually" but after this PR:
"CRM"and"Organizations"msgids (under the manual## Tab labelssection) now carry#, elixir-autogenand macro line refs — no longer manual.- A third manual group exists at lines 222-228 (
"Columns updated","Failed to save columns") sitting in the middle of the autogen block.
Fixing the inline column_management.ex finding below eliminates group 3, making this doc-block accurate again.
ℹ️ <.status_badge> strings stay English in any locale. role_view.ex:181-187 and organizations_view.ex:180-187 render status="active|inactive" — PhoenixKitWeb.Components.Core.Badge.status_label/1 capitalises the raw atom with no gettext, so ru/et users still see "Active"/"Inactive". Belongs in an upstream phoenix_kit PR, not this module.
ℹ️ No CHANGELOG entry / @version bump. mix.exs still pins @version "0.2.2" from PR #5. The [0.2.2] CHANGELOG entry describes ColumnModal/CellFormat strings only — doesn't mention this PR. Per the project convention (Prepare 0.2.x release commits) this should bump to 0.2.3 — inside this PR or via a release-prep PR before tagging.
Positive notes
- Double
use Gettextis safe — seconduseoverwrites@__gettext_backend__, allgettext/1macros in the four touched LiveViews resolve toPhoenixKitCRM.Gettext. - Plural forms correct in ru (
nplurals=3) and et (nplurals=2). - No previously-translated msgid lost — host
PhoenixKitWeb.Gettexthad no entries for the swapped CRM msgids. mix compile --warnings-as-errorsclean, 81 tests pass, no fuzzy entries.
| icon: "hero-users", | ||
| path: "crm", | ||
| priority: 650, | ||
| priority: 927, |
There was a problem hiding this comment.
⛔ Priority 927 collides with phoenix_kit_ecommerce.
phoenix_kit_ecommerce/lib/phoenix_kit_ecommerce.ex:279 already uses priority: 927 for :admin_settings_shop (same parent: :admin_settings). Any host app installing both modules will see non-deterministic tab ordering — tie-break depends on Dashboard.Registry insertion order.
Full sorted neighborhood: 920 Referrals, 921 Publishing, 923 Customer Support, 925 Emails, 926 Billing, 927 E-Commerce + CRM
| priority: 927, | |
| priority: 924, |
924 is the free slot between Customer Support (923) and Emails (925).
| #: lib/phoenix_kit_crm/web/crm_live.ex:52 | ||
| #, elixir-autogen, elixir-format | ||
| msgid "Enabled" | ||
| msgstr "Включён" |
There was a problem hiding this comment.
Включён is masculine short-form — fine next to "CRM" today, but the same msgid is shared backend-wide. If a future caller wraps a feminine/neuter subject ("роль", "система", "функция"), the badge will read grammatically wrong.
| msgstr "Включён" | |
| msgstr "Включено" |
Neuter status form works for any gender.
| #: lib/phoenix_kit_crm/web/crm_live.ex:52 | ||
| #, elixir-autogen, elixir-format | ||
| msgid "Disabled" | ||
| msgstr "Отключён" |
There was a problem hiding this comment.
Отключён is masculine. Use the neuter status form.
| msgstr "Отключён" | |
| msgstr "Отключено" |
| |> Phoenix.LiveView.put_flash(:info, "Columns updated")} | ||
| |> Phoenix.LiveView.put_flash( | ||
| :info, | ||
| Gettext.gettext(PhoenixKitCRM.Gettext, "Columns updated") |
There was a problem hiding this comment.
🟡 Use the gettext/1 macro instead.
Gettext.gettext/2 is a runtime function — mix gettext.extract cannot see it, which is why the two msgids had to be added to priv/gettext/default.pot by hand. But the consuming modules (role_view.ex:9, organizations_view.ex:13) now declare use Gettext, backend: PhoenixKitCRM.Gettext themselves, so when the macro expands inside them, a plain gettext("...") resolves to PhoenixKitCRM.Gettext at compile-time and gets picked up by extraction.
| Gettext.gettext(PhoenixKitCRM.Gettext, "Columns updated") | |
| |> Phoenix.LiveView.put_flash(:info, gettext("Columns updated"))} |
Then re-run mix gettext.extract and remove the manual entries from default.pot. Eliminates a maintenance gotcha for future flash strings.
| |> Phoenix.LiveView.put_flash(:error, "Failed to save columns")} | ||
| |> Phoenix.LiveView.put_flash( | ||
| :error, | ||
| Gettext.gettext(PhoenixKitCRM.Gettext, "Failed to save columns") |
There was a problem hiding this comment.
🟡 Same as above — switch to the macro form.
| Gettext.gettext(PhoenixKitCRM.Gettext, "Failed to save columns") | |
| |> Phoenix.LiveView.put_flash(:error, gettext("Failed to save columns"))} |
| #: lib/phoenix_kit_crm.ex | ||
| #: lib/phoenix_kit_crm/web/crm_live.ex:17 | ||
| #: lib/phoenix_kit_crm/web/crm_live.ex:38 | ||
| #, elixir-autogen |
There was a problem hiding this comment.
🟡 Latent fragility for tab-label msgids.
The Tab.new!(label: "CRM", …) calls in phoenix_kit_crm.ex are plain strings — not extracted. The msgid "CRM" survives in the catalogue only because of the macro call gettext("CRM") in crm_live.ex:17/:38. If a future refactor renames/removes those sites, mix gettext.extract will drop the msgid (the entry has #, elixir-autogen) and every tab label using it loses its translation.
Fix: strip #, elixir-autogen from "CRM", "Organizations", and "Overview", and replace the macro line refs with a plain #: lib/phoenix_kit_crm.ex so the entries are durable.
The ColumnManagement __using__ macro injects flash messages that call the bare gettext/1 macro, so any host LV must also . Both current hosts already do, but the macro's moduledoc contract didn't list the requirement. Kept as a macro (not a fully-qualified call) so mix gettext.extract still sees the strings. Also records the PR #7 review under dev_docs/pull_requests/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The ColumnManagement __using__ macro injects flash messages that call the bare gettext/1 macro, so any host LV must also `use Gettext, backend: PhoenixKitCRM.Gettext`. Both current hosts already do, but the macro's moduledoc contract didn't list the requirement. Kept as a macro (not a fully-qualified call) so mix gettext.extract still sees the strings. Also records the PR #7 review under dev_docs/pull_requests/. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bump version 0.2.2 -> 0.2.3 and add the CHANGELOG entry covering the unreleased work: PR #6 (Estonian catalogue completed), PR #7 (CRM admin page bodies + ColumnManagement flashes localized onto PhoenixKitCRM.Gettext, settings tab reordered), the dependency refresh (phoenix_kit 1.7.106 -> 1.7.120, ecto 3.13 -> 3.14, others), and the ColumnManagement moduledoc note. Also format the precommit alias in mix.exs that "libs upgraded" left unformatted, restoring `mix format --check-formatted`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Summary
650→927so it sits next to Emails (925), Languages (928), and Legal (929) instead of jumping to the top of the settings sidebar.settings_live.ex,crm_live.ex,organizations_view.ex,role_view.ex) and thecolumn_management.exflash messages fromPhoenixKitWeb.Gettext(host) to module-ownedPhoenixKitCRM.Gettext, so user-facing strings live in this package's catalogue rather than relying on the host app to supply them.priv/gettext/default.potand fillen,ru(3-form plurals), andet(2-form plurals) translations acrossdefault.po.Why
The Overview, Organizations, and Settings pages were rendering English strings in non-EN locales because the macro-bound
gettext()(or explicitGettext.gettext(PhoenixKitWeb.Gettext, ...)) resolved against the host's catalogue, which has no CRM-specific msgids. The tab also rendered first in the settings sidebar, ahead of every core PhoenixKit settings subtab.Notable detail
crm_live.exhad three hardcoded strings that previously weren't translatable at all: the"CRM"H2 title and the"Enabled"/"Disabled"badge labels. They are now wrapped ingettext/1.column_management.exflash msgids ("Columns updated","Failed to save columns") come fromGettext.gettext/2function calls, not the macro, somix gettext.extractcannot see them. They are listed manually indefault.pot(matching the convention already used for tab labels and column-config labels).Test plan
mix compile— cleanmix test— 81 passed, 0 failed?locale=ruand?locale=etin a host app and confirm the new strings render translated.