From bcd38712452a1ba69be01d25e363fe4b5545e57a Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 14:25:32 +0100 Subject: [PATCH 1/6] fix: correct German translations for role management Fix incorrect translations: - 'Listing Roles' -> 'Rollen auflisten' (was 'Benutzer*innen auflisten') - 'Custom' -> 'Benutzerdefiniert' (was 'Benutzerdefinierte Felder') --- lib/mv_web/live/role_live/index.ex | 8 +++++--- lib/mv_web/live/role_live/show.ex | 1 - priv/gettext/de/LC_MESSAGES/default.po | 8 +++----- priv/gettext/default.pot | 4 +--- priv/gettext/en/LC_MESSAGES/default.po | 4 +--- 5 files changed, 10 insertions(+), 15 deletions(-) diff --git a/lib/mv_web/live/role_live/index.ex b/lib/mv_web/live/role_live/index.ex index 0099929..d34cb22 100644 --- a/lib/mv_web/live/role_live/index.ex +++ b/lib/mv_web/live/role_live/index.ex @@ -126,7 +126,9 @@ defmodule MvWeb.RoleLive.Index do end # Loads all user counts for roles in a single query to avoid N+1 queries - @spec load_user_counts([Mv.Authorization.Role.t()], map() | nil) :: %{Ecto.UUID.t() => non_neg_integer()} + @spec load_user_counts([Mv.Authorization.Role.t()], map() | nil) :: %{ + Ecto.UUID.t() => non_neg_integer() + } defp load_user_counts(roles, actor) do role_ids = Enum.map(roles, & &1.id) @@ -153,7 +155,8 @@ defmodule MvWeb.RoleLive.Index do end # Gets user count from preloaded assigns map - @spec get_user_count(Mv.Authorization.Role.t(), %{Ecto.UUID.t() => non_neg_integer()}) :: non_neg_integer() + @spec get_user_count(Mv.Authorization.Role.t(), %{Ecto.UUID.t() => non_neg_integer()}) :: + non_neg_integer() defp get_user_count(role, user_counts) do Map.get(user_counts, role.id, 0) end @@ -169,5 +172,4 @@ defmodule MvWeb.RoleLive.Index do _ -> 0 end end - end diff --git a/lib/mv_web/live/role_live/show.ex b/lib/mv_web/live/role_live/show.ex index 8400728..3f15155 100644 --- a/lib/mv_web/live/role_live/show.ex +++ b/lib/mv_web/live/role_live/show.ex @@ -213,5 +213,4 @@ defmodule MvWeb.RoleLive.Show do """ end - end diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 5a9c32f..229b180 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1449,9 +1449,7 @@ msgstr "Bereits bezahlte Zyklen bleiben mit dem alten Betrag." #: lib/mv_web/live/member_live/show/membership_fees_component.ex #: lib/mv_web/live/membership_fee_type_live/index.ex -#: lib/mv_web/live/role_live/form.ex -#: lib/mv_web/live/role_live/index.ex -#: lib/mv_web/live/role_live/show.ex +#: lib/mv_web/live/role_live/helpers.ex #, elixir-autogen, elixir-format msgid "An error occurred" msgstr "Ein Fehler ist aufgetreten" @@ -1862,7 +1860,7 @@ msgstr "System-Rolle kann nicht gelöscht werden" #: lib/mv_web/live/role_live/index.html.heex #, elixir-autogen, elixir-format, fuzzy msgid "Custom" -msgstr "Benutzerdefinierte Felder" +msgstr "Benutzerdefiniert" #: lib/mv_web/live/role_live/show.ex #, elixir-autogen, elixir-format, fuzzy @@ -1879,7 +1877,7 @@ msgstr "Rolle konnte nicht gelöscht werden: %{error}" #: lib/mv_web/live/role_live/index.html.heex #, elixir-autogen, elixir-format, fuzzy msgid "Listing Roles" -msgstr "Benutzer*innen auflisten" +msgstr "Rollen auflisten" #: lib/mv_web/live/role_live/index.html.heex #, elixir-autogen, elixir-format diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index cdf1895..d720511 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -1450,9 +1450,7 @@ msgstr "" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #: lib/mv_web/live/membership_fee_type_live/index.ex -#: lib/mv_web/live/role_live/form.ex -#: lib/mv_web/live/role_live/index.ex -#: lib/mv_web/live/role_live/show.ex +#: lib/mv_web/live/role_live/helpers.ex #, elixir-autogen, elixir-format msgid "An error occurred" msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 724c991..bb75e6b 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -1450,9 +1450,7 @@ msgstr "" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #: lib/mv_web/live/membership_fee_type_live/index.ex -#: lib/mv_web/live/role_live/form.ex -#: lib/mv_web/live/role_live/index.ex -#: lib/mv_web/live/role_live/show.ex +#: lib/mv_web/live/role_live/helpers.ex #, elixir-autogen, elixir-format msgid "An error occurred" msgstr "" From 583c2b17663bc8ae89f3003e0de3d2f2ab39e102 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 14:25:32 +0100 Subject: [PATCH 2/6] fix: correct German translations for role management Fix incorrect translations: - 'Listing Roles' -> 'Rollen auflisten' (was 'Benutzer*innen auflisten') - 'Custom' -> 'Benutzerdefiniert' (was 'Benutzerdefinierte Felder') --- lib/mv_web/live/role_live/index.ex | 8 +++++--- lib/mv_web/live/role_live/show.ex | 1 - priv/gettext/de/LC_MESSAGES/default.po | 8 +++----- priv/gettext/default.pot | 4 +--- priv/gettext/en/LC_MESSAGES/default.po | 4 +--- 5 files changed, 10 insertions(+), 15 deletions(-) diff --git a/lib/mv_web/live/role_live/index.ex b/lib/mv_web/live/role_live/index.ex index 0099929..d34cb22 100644 --- a/lib/mv_web/live/role_live/index.ex +++ b/lib/mv_web/live/role_live/index.ex @@ -126,7 +126,9 @@ defmodule MvWeb.RoleLive.Index do end # Loads all user counts for roles in a single query to avoid N+1 queries - @spec load_user_counts([Mv.Authorization.Role.t()], map() | nil) :: %{Ecto.UUID.t() => non_neg_integer()} + @spec load_user_counts([Mv.Authorization.Role.t()], map() | nil) :: %{ + Ecto.UUID.t() => non_neg_integer() + } defp load_user_counts(roles, actor) do role_ids = Enum.map(roles, & &1.id) @@ -153,7 +155,8 @@ defmodule MvWeb.RoleLive.Index do end # Gets user count from preloaded assigns map - @spec get_user_count(Mv.Authorization.Role.t(), %{Ecto.UUID.t() => non_neg_integer()}) :: non_neg_integer() + @spec get_user_count(Mv.Authorization.Role.t(), %{Ecto.UUID.t() => non_neg_integer()}) :: + non_neg_integer() defp get_user_count(role, user_counts) do Map.get(user_counts, role.id, 0) end @@ -169,5 +172,4 @@ defmodule MvWeb.RoleLive.Index do _ -> 0 end end - end diff --git a/lib/mv_web/live/role_live/show.ex b/lib/mv_web/live/role_live/show.ex index 8400728..3f15155 100644 --- a/lib/mv_web/live/role_live/show.ex +++ b/lib/mv_web/live/role_live/show.ex @@ -213,5 +213,4 @@ defmodule MvWeb.RoleLive.Show do """ end - end diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 5a9c32f..229b180 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1449,9 +1449,7 @@ msgstr "Bereits bezahlte Zyklen bleiben mit dem alten Betrag." #: lib/mv_web/live/member_live/show/membership_fees_component.ex #: lib/mv_web/live/membership_fee_type_live/index.ex -#: lib/mv_web/live/role_live/form.ex -#: lib/mv_web/live/role_live/index.ex -#: lib/mv_web/live/role_live/show.ex +#: lib/mv_web/live/role_live/helpers.ex #, elixir-autogen, elixir-format msgid "An error occurred" msgstr "Ein Fehler ist aufgetreten" @@ -1862,7 +1860,7 @@ msgstr "System-Rolle kann nicht gelöscht werden" #: lib/mv_web/live/role_live/index.html.heex #, elixir-autogen, elixir-format, fuzzy msgid "Custom" -msgstr "Benutzerdefinierte Felder" +msgstr "Benutzerdefiniert" #: lib/mv_web/live/role_live/show.ex #, elixir-autogen, elixir-format, fuzzy @@ -1879,7 +1877,7 @@ msgstr "Rolle konnte nicht gelöscht werden: %{error}" #: lib/mv_web/live/role_live/index.html.heex #, elixir-autogen, elixir-format, fuzzy msgid "Listing Roles" -msgstr "Benutzer*innen auflisten" +msgstr "Rollen auflisten" #: lib/mv_web/live/role_live/index.html.heex #, elixir-autogen, elixir-format diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index cdf1895..d720511 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -1450,9 +1450,7 @@ msgstr "" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #: lib/mv_web/live/membership_fee_type_live/index.ex -#: lib/mv_web/live/role_live/form.ex -#: lib/mv_web/live/role_live/index.ex -#: lib/mv_web/live/role_live/show.ex +#: lib/mv_web/live/role_live/helpers.ex #, elixir-autogen, elixir-format msgid "An error occurred" msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 724c991..bb75e6b 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -1450,9 +1450,7 @@ msgstr "" #: lib/mv_web/live/member_live/show/membership_fees_component.ex #: lib/mv_web/live/membership_fee_type_live/index.ex -#: lib/mv_web/live/role_live/form.ex -#: lib/mv_web/live/role_live/index.ex -#: lib/mv_web/live/role_live/show.ex +#: lib/mv_web/live/role_live/helpers.ex #, elixir-autogen, elixir-format msgid "An error occurred" msgstr "" From ce395ec112036f2ba42ded896eef180485397728 Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 15:54:46 +0100 Subject: [PATCH 3/6] fix: add ensure_user_role_loaded to router live_session globally --- lib/mv_web/live_helpers.ex | 10 ++++++++-- lib/mv_web/router.ex | 5 ++++- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/lib/mv_web/live_helpers.ex b/lib/mv_web/live_helpers.ex index 1835cba..f217ee2 100644 --- a/lib/mv_web/live_helpers.ex +++ b/lib/mv_web/live_helpers.ex @@ -49,8 +49,14 @@ defmodule MvWeb.LiveHelpers do opts = [domain: Mv.Accounts, actor: user] case Ash.load(user, :role, opts) do - {:ok, loaded_user} -> loaded_user - {:error, _} -> user + {:ok, loaded_user} -> + loaded_user + + {:error, error} -> + # Log warning if role loading fails - this can cause authorization issues + require Logger + Logger.warning("Failed to load role for user #{user.id}: #{inspect(error)}") + user end end end diff --git a/lib/mv_web/router.ex b/lib/mv_web/router.ex index e73c926..682b672 100644 --- a/lib/mv_web/router.ex +++ b/lib/mv_web/router.ex @@ -46,7 +46,10 @@ defmodule MvWeb.Router do AshAuthentication-specific: We define that all routes can only be accessed when the user is signed in. """ ash_authentication_live_session :authentication_required, - on_mount: {MvWeb.LiveUserAuth, :live_user_required} do + on_mount: [ + {MvWeb.LiveUserAuth, :live_user_required}, + {MvWeb.LiveHelpers, :ensure_user_role_loaded} + ] do live "/", MemberLive.Index, :index live "/members", MemberLive.Index, :index From ddb4da266dc72645adc96f2176e9ac5d9407918f Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 15:54:47 +0100 Subject: [PATCH 4/6] fix: use verified routes in navbar and improve can_access_page? Use ~p verified routes instead of string paths in navbar template. Update can_access_page? to handle both string and verified route paths for better type safety. --- lib/mv_web/authorization.ex | 8 ++++++-- lib/mv_web/components/layouts/navbar.ex | 4 ++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/lib/mv_web/authorization.ex b/lib/mv_web/authorization.ex index 18ecd70..95a8524 100644 --- a/lib/mv_web/authorization.ex +++ b/lib/mv_web/authorization.ex @@ -106,14 +106,18 @@ defmodule MvWeb.Authorization do iex> can_access_page?(mitglied, "/members") false """ - @spec can_access_page?(map() | nil, String.t()) :: boolean() + @spec can_access_page?(map() | nil, String.t() | Phoenix.VerifiedRoutes.unverified_path()) :: + boolean() def can_access_page?(nil, _page_path), do: false def can_access_page?(user, page_path) do + # Convert verified route to string if needed + page_path_str = if is_binary(page_path), do: page_path, else: to_string(page_path) + with %{role: %{permission_set_name: ps_name}} when not is_nil(ps_name) <- user, {:ok, ps_atom} <- PermissionSets.permission_set_name_to_atom(ps_name), permissions <- PermissionSets.get_permissions(ps_atom) do - page_matches?(permissions.pages, page_path) + page_matches?(permissions.pages, page_path_str) else _ -> false end diff --git a/lib/mv_web/components/layouts/navbar.ex b/lib/mv_web/components/layouts/navbar.ex index 692f949..e3e9319 100644 --- a/lib/mv_web/components/layouts/navbar.ex +++ b/lib/mv_web/components/layouts/navbar.ex @@ -34,9 +34,9 @@ defmodule MvWeb.Layouts.Navbar do
  • <.link navigate="/settings">{gettext("Global Settings")}
    • - <%= if can_access_page?(@current_user, "/admin/roles") do %> + <%= if can_access_page?(@current_user, ~p"/admin/roles") do %>
  • - <.link navigate="/admin/roles">{gettext("Roles")} + <.link navigate={~p"/admin/roles"}>{gettext("Roles")}
    • <% end %> From 08182300b9e1b5f0ba998faf7f1b965e01398e5f Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 15:54:49 +0100 Subject: [PATCH 5/6] refactor: add opts_with_actor helper and improve error formatting Add opts_with_actor helper function to reduce duplication when building Ash options with actor and domain. Improve format_error documentation and ensure consistent error message formatting. --- lib/mv_web/live/role_live/form.ex | 2 +- lib/mv_web/live/role_live/helpers.ex | 17 +++++++++++++++++ lib/mv_web/live/role_live/index.ex | 10 +++++----- lib/mv_web/live/role_live/show.ex | 6 +++--- 4 files changed, 26 insertions(+), 9 deletions(-) diff --git a/lib/mv_web/live/role_live/form.ex b/lib/mv_web/live/role_live/form.ex index 6388ec1..7b74c7e 100644 --- a/lib/mv_web/live/role_live/form.ex +++ b/lib/mv_web/live/role_live/form.ex @@ -15,7 +15,7 @@ defmodule MvWeb.RoleLive.Form do alias Mv.Authorization.PermissionSets - import MvWeb.RoleLive.Helpers + import MvWeb.RoleLive.Helpers, only: [format_error: 1] on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} diff --git a/lib/mv_web/live/role_live/helpers.ex b/lib/mv_web/live/role_live/helpers.ex index 9d4e77d..8fbc544 100644 --- a/lib/mv_web/live/role_live/helpers.ex +++ b/lib/mv_web/live/role_live/helpers.ex @@ -6,6 +6,7 @@ defmodule MvWeb.RoleLive.Helpers do @doc """ Formats an error for display to the user. + Extracts error messages from Ash.Error.Invalid and joins them. """ @spec format_error(Ash.Error.Invalid.t() | String.t() | any()) :: String.t() def format_error(%Ash.Error.Invalid{} = error) do @@ -24,4 +25,20 @@ defmodule MvWeb.RoleLive.Helpers do def permission_set_badge_class("normal_user"), do: "badge badge-success badge-sm" def permission_set_badge_class("admin"), do: "badge badge-error badge-sm" def permission_set_badge_class(_), do: "badge badge-ghost badge-sm" + + @doc """ + Builds Ash options with actor and domain, ensuring actor is never nil in real paths. + """ + @spec opts_with_actor(keyword(), map() | nil, atom()) :: keyword() + def opts_with_actor(base_opts \\ [], actor, domain) do + opts = Keyword.put(base_opts, :domain, domain) + + if actor do + Keyword.put(opts, :actor, actor) + else + require Logger + Logger.warning("opts_with_actor called with nil actor - this may bypass policies") + opts + end + end end diff --git a/lib/mv_web/live/role_live/index.ex b/lib/mv_web/live/role_live/index.ex index d34cb22..718aa34 100644 --- a/lib/mv_web/live/role_live/index.ex +++ b/lib/mv_web/live/role_live/index.ex @@ -21,7 +21,8 @@ defmodule MvWeb.RoleLive.Index do require Ash.Query - import MvWeb.RoleLive.Helpers + import MvWeb.RoleLive.Helpers, + only: [format_error: 1, permission_set_badge_class: 1, opts_with_actor: 3] on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} @@ -126,6 +127,7 @@ defmodule MvWeb.RoleLive.Index do end # Loads all user counts for roles in a single query to avoid N+1 queries + # TODO: Optimize to use DB-side aggregation instead of loading all users @spec load_user_counts([Mv.Authorization.Role.t()], map() | nil) :: %{ Ecto.UUID.t() => non_neg_integer() } @@ -133,8 +135,7 @@ defmodule MvWeb.RoleLive.Index do role_ids = Enum.map(roles, & &1.id) # Load all users with role_id in a single query - opts = [domain: Mv.Accounts] - opts = if actor, do: Keyword.put(opts, :actor, actor), else: opts + opts = opts_with_actor([], actor, Mv.Accounts) users = case Ash.read( @@ -164,8 +165,7 @@ defmodule MvWeb.RoleLive.Index do # Recalculates user count for a specific role (used before deletion) @spec recalculate_user_count(Mv.Authorization.Role.t(), map() | nil) :: non_neg_integer() defp recalculate_user_count(role, actor) do - opts = [domain: Mv.Accounts] - opts = if actor, do: Keyword.put(opts, :actor, actor), else: opts + opts = opts_with_actor([], actor, Mv.Accounts) case Ash.count(Accounts.User |> Ash.Query.filter(role_id == ^role.id), opts) do {:ok, count} -> count diff --git a/lib/mv_web/live/role_live/show.ex b/lib/mv_web/live/role_live/show.ex index 3f15155..7184b68 100644 --- a/lib/mv_web/live/role_live/show.ex +++ b/lib/mv_web/live/role_live/show.ex @@ -16,7 +16,8 @@ defmodule MvWeb.RoleLive.Show do require Ash.Query - import MvWeb.RoleLive.Helpers + import MvWeb.RoleLive.Helpers, + only: [format_error: 1, permission_set_badge_class: 1, opts_with_actor: 3] on_mount {MvWeb.LiveHelpers, :ensure_user_role_loaded} @@ -143,8 +144,7 @@ defmodule MvWeb.RoleLive.Show do # Recalculates user count for a specific role (used before deletion) @spec recalculate_user_count(Mv.Authorization.Role.t(), map() | nil) :: non_neg_integer() defp recalculate_user_count(role, actor) do - opts = [domain: Mv.Accounts] - opts = if actor, do: Keyword.put(opts, :actor, actor), else: opts + opts = opts_with_actor([], actor, Mv.Accounts) case Ash.count(Accounts.User |> Ash.Query.filter(role_id == ^role.id), opts) do {:ok, count} -> count From 5998fb643db174104513387bb235fdbc34f34efc Mon Sep 17 00:00:00 2001 From: Moritz Date: Thu, 8 Jan 2026 15:58:53 +0100 Subject: [PATCH 6/6] perf: optimize load_user_counts with DB-side aggregation Replace Elixir-side counting with Ecto GROUP BY COUNT query for better performance. This avoids loading all users into memory and performs the aggregation directly in the database. --- lib/mv_web/live/role_live/index.ex | 35 +++++++++++++----------------- 1 file changed, 15 insertions(+), 20 deletions(-) diff --git a/lib/mv_web/live/role_live/index.ex b/lib/mv_web/live/role_live/index.ex index 718aa34..9d75da6 100644 --- a/lib/mv_web/live/role_live/index.ex +++ b/lib/mv_web/live/role_live/index.ex @@ -126,33 +126,28 @@ defmodule MvWeb.RoleLive.Index do end end - # Loads all user counts for roles in a single query to avoid N+1 queries - # TODO: Optimize to use DB-side aggregation instead of loading all users + # Loads all user counts for roles using DB-side aggregation for better performance @spec load_user_counts([Mv.Authorization.Role.t()], map() | nil) :: %{ Ecto.UUID.t() => non_neg_integer() } - defp load_user_counts(roles, actor) do + defp load_user_counts(roles, _actor) do role_ids = Enum.map(roles, & &1.id) - # Load all users with role_id in a single query - opts = opts_with_actor([], actor, Mv.Accounts) + # Use Ecto directly for efficient GROUP BY COUNT query + # This is much more performant than loading all users and counting in Elixir + # Note: We bypass Ash here for performance, but this is a simple read-only query + import Ecto.Query - users = - case Ash.read( - Accounts.User - |> Ash.Query.filter(role_id in ^role_ids) - |> Ash.Query.select([:role_id]), - opts - ) do - {:ok, users_list} -> users_list - {:error, _} -> [] - end + query = + from u in Accounts.User, + where: u.role_id in ^role_ids, + group_by: u.role_id, + select: {u.role_id, count(u.id)} - # Group by role_id and count - users - |> Enum.group_by(& &1.role_id) - |> Enum.map(fn {role_id, users_list} -> {role_id, length(users_list)} end) - |> Map.new() + results = Mv.Repo.all(query) + + results + |> Enum.into(%{}, fn {role_id, count} -> {role_id, count} end) end # Gets user count from preloaded assigns map