From 911f308a67c93b49dc9a3444542744b91e49fe99 Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 17 Feb 2026 15:30:23 +0100 Subject: [PATCH] fix: address review comments --- docs/groups-architecture.md | 6 +- lib/mv_web/live/member_live/index.html.heex | 1 - lib/mv_web/live/member_live/show.ex | 14 +- priv/gettext/de/LC_MESSAGES/default.po | 9 - priv/gettext/en/LC_MESSAGES/default.po | 5 - .../index_groups_accessibility_test.exs | 8 +- .../member_live/show_groups_display_test.exs | 209 +++++++----------- 7 files changed, 88 insertions(+), 164 deletions(-) diff --git a/docs/groups-architecture.md b/docs/groups-architecture.md index 1557b6f..0e59409 100644 --- a/docs/groups-architecture.md +++ b/docs/groups-architecture.md @@ -356,9 +356,9 @@ lib/ - Screen readers must be able to navigate and understand the interface - ARIA labels and roles must be properly set -**Group Badges in Member Overview:** -- Badges must have `role="status"` and appropriate `aria-label` attributes -- Badge title should indicate group membership +**Group Badges and Links in Member Overview / Detail:** +- Use `aria-label` to indicate group membership (e.g. "Member of group X"). Do not use `role="status"` on badges or links—that role is for live regions (screen reader announcements), not for navigation or static labels. +- Badge/link text or title should indicate group membership for screen readers. **Clickable Group Badge (for filtering) - Optional:** diff --git a/lib/mv_web/live/member_live/index.html.heex b/lib/mv_web/live/member_live/index.html.heex index 311447b..b490618 100644 --- a/lib/mv_web/live/member_live/index.html.heex +++ b/lib/mv_web/live/member_live/index.html.heex @@ -322,7 +322,6 @@ <%= for group <- (member.groups || []) do %> {group.name} diff --git a/lib/mv_web/live/member_live/show.ex b/lib/mv_web/live/member_live/show.ex index cc0b25e..47e8878 100644 --- a/lib/mv_web/live/member_live/show.ex +++ b/lib/mv_web/live/member_live/show.ex @@ -12,7 +12,7 @@ defmodule MvWeb.MemberLive.Show do ## Sections - Personal Data: Name, address, contact information, membership dates, notes - Custom Fields: Dynamic fields in uniform grid layout (sorted by name) - - Groups: Group links (buttons) in Personal Data section, below Linked User + - Groups: Links to group detail pages in Personal Data section - Payment Data: Membership fee type and cycle status - Membership Fees: Tab showing all membership fee cycles with status management (via MembershipFeesComponent) @@ -148,24 +148,24 @@ defmodule MvWeb.MemberLive.Show do <% end %> - <%!-- Groups (in Personal Data, below Linked User) --%> + <%!-- Groups (in Personal Data) --%> + <% groups = @member.groups || [] %>
<.data_field label={gettext("Groups")}> - <%= if Enum.any?(@member.groups || []) do %> + <%= if Enum.empty?(groups) do %> + {gettext("No groups")} + <% else %>
- <%= for group <- (@member.groups || []) do %> + <%= for group <- groups do %> <.link navigate={~p"/groups/#{group.slug}"} class="btn btn-xs btn-outline btn-primary" - role="status" aria-label={gettext("Member of group %{name}", name: group.name)} > {group.name} <% end %>
- <% else %> - {gettext("No groups")} <% end %>
diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 57334d2..e40d053 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2616,12 +2616,3 @@ msgstr "Anzahl Mitglieder:" #, elixir-autogen, elixir-format msgid "PDF" msgstr "PDF" - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Custom Fields in CSV Import" -#~ msgstr "Benutzerdefinierte Felder" - -#~ #, elixir-autogen, elixir-format -#~ msgid "Failed to prepare CSV import: %{error}" -#~ msgstr "Das Vorbereiten des CSV Imports ist gescheitert: %{error}" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 1bf276c..f12d478 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2617,8 +2617,3 @@ msgstr "Member count:" #, elixir-autogen, elixir-format msgid "PDF" msgstr "" - -#~ #: lib/mv_web/live/global_settings_live.ex -#~ #, elixir-autogen, elixir-format, fuzzy -#~ msgid "Custom Fields in CSV Import" -#~ msgstr "" diff --git a/test/mv_web/member_live/index_groups_accessibility_test.exs b/test/mv_web/member_live/index_groups_accessibility_test.exs index ab9b728..d14cd9f 100644 --- a/test/mv_web/member_live/index_groups_accessibility_test.exs +++ b/test/mv_web/member_live/index_groups_accessibility_test.exs @@ -3,7 +3,7 @@ defmodule MvWeb.MemberLive.IndexGroupsAccessibilityTest do Tests for accessibility of groups feature in the member overview. Tests cover: - - Badges have role="status" and aria-label + - Badges have aria-label for group membership (no role="status"; reserved for live regions) - Filter dropdown has aria-label - Sort header has aria-label for screen reader - Keyboard navigation works (Tab through filter, sort header) @@ -44,7 +44,7 @@ defmodule MvWeb.MemberLive.IndexGroupsAccessibilityTest do end @tag :ui - test "group badges have role and aria-label", %{ + test "group badges have aria-label for screen readers", %{ conn: conn, member1: member1, group1: group1 @@ -52,8 +52,8 @@ defmodule MvWeb.MemberLive.IndexGroupsAccessibilityTest do conn = conn_with_oidc_user(conn) {:ok, view, html} = live(conn, "/members") - # Verify badges have role="status" and aria-label containing the group name - assert has_element?(view, "span[role='status'][aria-label*='#{group1.name}']") + # Verify badges have aria-label containing the group name (no role=status on badges) + assert has_element?(view, "span[aria-label*='#{group1.name}']") assert html =~ group1.name # Verify member1's row contains the badge diff --git a/test/mv_web/member_live/show_groups_display_test.exs b/test/mv_web/member_live/show_groups_display_test.exs index bda46b3..f8434b3 100644 --- a/test/mv_web/member_live/show_groups_display_test.exs +++ b/test/mv_web/member_live/show_groups_display_test.exs @@ -4,10 +4,10 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do Tests cover: - Groups in Personal Data (with and without groups) - - Group buttons with correct names and links to group detail pages + - Group links with correct names and links to group detail pages - Edge cases (one group, many groups) - Security: groups visible only when user may view member - - Accessibility: group links have role and aria-label + - Accessibility: group links have aria-label for screen readers ## Note on async async: false to avoid PostgreSQL deadlocks when creating members and groups @@ -22,15 +22,16 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do describe "groups section" do setup do - system_actor = Mv.Helpers.SystemActor.get_system_actor() + actor = Mv.Helpers.SystemActor.get_system_actor() {:ok, member} = - Mv.Membership.create_member( - %{first_name: "Alice", last_name: "Anderson", email: "alice@example.com"}, - actor: system_actor - ) + create_member(actor, %{ + first_name: "Alice", + last_name: "Anderson", + email: "alice@example.com" + }) - %{member: member, actor: system_actor} + %{member: member, actor: actor} end test "displays Groups section when member has at least one group", %{ @@ -38,15 +39,8 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do member: member, actor: actor } do - {:ok, group} = - Group - |> Ash.Changeset.for_create(:create, %{name: "Board Members"}) - |> Ash.create(actor: actor) - - {:ok, _mg} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group.id}) - |> Ash.create(actor: actor) + {:ok, group} = create_group(actor, "Board Members") + {:ok, _mg} = add_member_to_group(member, group, actor) conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, ~p"/members/#{member}") @@ -55,30 +49,15 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do assert html =~ group.name end - test "displays all groups as badges with correct names when member is in multiple groups", %{ + test "displays all group links with correct names when member is in multiple groups", %{ conn: conn, member: member, actor: actor } do - {:ok, group1} = - Group - |> Ash.Changeset.for_create(:create, %{name: "Board Members"}) - |> Ash.create(actor: actor) - - {:ok, group2} = - Group - |> Ash.Changeset.for_create(:create, %{name: "Active Members"}) - |> Ash.create(actor: actor) - - {:ok, _mg1} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group1.id}) - |> Ash.create(actor: actor) - - {:ok, _mg2} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group2.id}) - |> Ash.create(actor: actor) + {:ok, group1} = create_group(actor, "Board Members") + {:ok, group2} = create_group(actor, "Active Members") + {:ok, _mg1} = add_member_to_group(member, group1, actor) + {:ok, _mg2} = add_member_to_group(member, group2, actor) conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, ~p"/members/#{member}") @@ -100,30 +79,15 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do assert html =~ gettext("No groups") end - test "groups are loaded with member (single request returns all group names)", %{ + test "renders all group names when member has multiple groups", %{ conn: conn, member: member, actor: actor } do - {:ok, group1} = - Group - |> Ash.Changeset.for_create(:create, %{name: "Alpha"}) - |> Ash.create(actor: actor) - - {:ok, group2} = - Group - |> Ash.Changeset.for_create(:create, %{name: "Beta"}) - |> Ash.create(actor: actor) - - {:ok, _mg1} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group1.id}) - |> Ash.create(actor: actor) - - {:ok, _mg2} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group2.id}) - |> Ash.create(actor: actor) + {:ok, group1} = create_group(actor, "Alpha") + {:ok, group2} = create_group(actor, "Beta") + {:ok, _mg1} = add_member_to_group(member, group1, actor) + {:ok, _mg2} = add_member_to_group(member, group2, actor) conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, ~p"/members/#{member}") @@ -135,41 +99,29 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do describe "groups section links" do setup do - system_actor = Mv.Helpers.SystemActor.get_system_actor() + actor = Mv.Helpers.SystemActor.get_system_actor() {:ok, member} = - Mv.Membership.create_member( - %{first_name: "Bob", last_name: "Brown", email: "bob@example.com"}, - actor: system_actor - ) - - {:ok, group} = - Group - |> Ash.Changeset.for_create(:create, %{name: "Board Members"}) - |> Ash.create(actor: system_actor) - - {:ok, _mg} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group.id}) - |> Ash.create(actor: system_actor) + create_member(actor, %{first_name: "Bob", last_name: "Brown", email: "bob@example.com"}) + {:ok, group} = create_group(actor, "Board Members") + {:ok, _mg} = add_member_to_group(member, group, actor) %{member: member, group: group} end - test "each group badge links to group detail page with correct slug", %{ + test "each group link goes to group detail page with correct slug", %{ conn: conn, member: member, group: group } do conn = conn_with_oidc_user(conn) - {:ok, _view, html} = live(conn, ~p"/members/#{member}") + {:ok, view, _html} = live(conn, ~p"/members/#{member}") # Link to group detail: /groups/:slug (slug is URL-friendly, e.g. "board-members") - assert html =~ - ~r/href="[^"]*\/groups\/#{Regex.escape(group.slug)}"|navigate="[^"]*\/groups\/#{Regex.escape(group.slug)}"/ + assert has_element?(view, "a[href*='/groups/#{group.slug}']", group.name) end - test "clicking group badge navigates to group detail page", %{ + test "clicking group link navigates to group detail page", %{ conn: conn, member: member, group: group @@ -187,31 +139,25 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do describe "groups section edge cases" do setup do - system_actor = Mv.Helpers.SystemActor.get_system_actor() + actor = Mv.Helpers.SystemActor.get_system_actor() {:ok, member} = - Mv.Membership.create_member( - %{first_name: "Charlie", last_name: "Clark", email: "charlie@example.com"}, - actor: system_actor - ) + create_member(actor, %{ + first_name: "Charlie", + last_name: "Clark", + email: "charlie@example.com" + }) - %{member: member, actor: system_actor} + %{member: member, actor: actor} end - test "member in exactly one group shows single badge", %{ + test "member in exactly one group shows single link", %{ conn: conn, member: member, actor: actor } do - {:ok, group} = - Group - |> Ash.Changeset.for_create(:create, %{name: "Solo Group"}) - |> Ash.create(actor: actor) - - {:ok, _mg} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group.id}) - |> Ash.create(actor: actor) + {:ok, group} = create_group(actor, "Solo Group") + {:ok, _mg} = add_member_to_group(member, group, actor) conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, ~p"/members/#{member}") @@ -220,7 +166,7 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do assert html =~ group.name end - test "member in many groups shows all badges", %{ + test "member in many groups shows all group links", %{ conn: conn, member: member, actor: actor @@ -229,19 +175,12 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do groups = Enum.map(group_names, fn name -> - {:ok, g} = - Group - |> Ash.Changeset.for_create(:create, %{name: name}) - |> Ash.create(actor: actor) - + {:ok, g} = create_group(actor, name) g end) for g <- groups do - {:ok, _mg} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: g.id}) - |> Ash.create(actor: actor) + {:ok, _mg} = add_member_to_group(member, g, actor) end conn = conn_with_oidc_user(conn) @@ -258,23 +197,17 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do describe "groups section with read_only user" do @tag role: :read_only test "user with read permission sees Groups section", %{conn: conn} do - system_actor = Mv.Helpers.SystemActor.get_system_actor() + actor = Mv.Helpers.SystemActor.get_system_actor() {:ok, member} = - Mv.Membership.create_member( - %{first_name: "Diana", last_name: "Davis", email: "diana@example.com"}, - actor: system_actor - ) + create_member(actor, %{ + first_name: "Diana", + last_name: "Davis", + email: "diana@example.com" + }) - {:ok, group} = - Group - |> Ash.Changeset.for_create(:create, %{name: "Readers"}) - |> Ash.create(actor: system_actor) - - {:ok, _mg} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group.id}) - |> Ash.create(actor: system_actor) + {:ok, group} = create_group(actor, "Readers") + {:ok, _mg} = add_member_to_group(member, group, actor) {:ok, _view, html} = live(conn, ~p"/members/#{member}") @@ -285,28 +218,17 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do describe "groups section accessibility" do setup do - system_actor = Mv.Helpers.SystemActor.get_system_actor() + actor = Mv.Helpers.SystemActor.get_system_actor() {:ok, member} = - Mv.Membership.create_member( - %{first_name: "Eve", last_name: "Evans", email: "eve@example.com"}, - actor: system_actor - ) - - {:ok, group} = - Group - |> Ash.Changeset.for_create(:create, %{name: "A11y Group"}) - |> Ash.create(actor: system_actor) - - {:ok, _mg} = - MemberGroup - |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group.id}) - |> Ash.create(actor: system_actor) + create_member(actor, %{first_name: "Eve", last_name: "Evans", email: "eve@example.com"}) + {:ok, group} = create_group(actor, "A11y Group") + {:ok, _mg} = add_member_to_group(member, group, actor) %{member: member, group: group} end - test "group badges have role and aria-label for screen readers", %{ + test "group links have aria-label for screen readers", %{ conn: conn, member: member, group: group @@ -316,8 +238,25 @@ defmodule MvWeb.MemberLive.ShowGroupsDisplayTest do assert html =~ group.name - # Badge has role="status" and aria-label indicating group membership (architecture: "Member of group X") - assert has_element?(view, "[role='status'][aria-label*='#{group.name}']") + # Group link has aria-label indicating group membership for screen readers + assert has_element?(view, "a[aria-label*='#{group.name}']") end end + + # Helpers to reduce setup duplication (create member/group, assign member to group). + defp create_member(actor, attrs) do + Mv.Membership.create_member(attrs, actor: actor) + end + + defp create_group(actor, name) do + Group + |> Ash.Changeset.for_create(:create, %{name: name}) + |> Ash.create(actor: actor) + end + + defp add_member_to_group(member, group, actor) do + MemberGroup + |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: group.id}) + |> Ash.create(actor: actor) + end end