fix: address review comments

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:**
+**Group Badges and Links in Member Overview / Detail:**
-- Badges must have `role="status"` and appropriate `aria-label` attributes
+- 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 title should indicate group membership
+- Badge/link text or title should indicate group membership for screen readers.
 
 **Clickable Group Badge (for filtering) - Optional:**
 

lib/mv_web/live/member_live/index.html.heex

@@ -322,7 +322,6 @@
       <%= for group <- (member.groups || []) do %>
         <span
           class="badge badge-outline badge-primary"
-          role="status"
           aria-label={gettext("Member of group %{name}", name: group.name)}
         >
           {group.name}

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
                   </div>
                 <% end %>
 
-                <%!-- Groups (in Personal Data, below Linked User) --%>
+                <%!-- Groups (in Personal Data) --%>
+                <% groups = @member.groups || [] %>
                 <div>
                   <.data_field label={gettext("Groups")}>
-                    <%= if Enum.any?(@member.groups || []) do %>
+                    <%= if Enum.empty?(groups) do %>
+                      <span class="text-base-content/70 italic">{gettext("No groups")}</span>
+                    <% else %>
                       <div class="flex flex-wrap gap-2">
-                        <%= 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}
                           </.link>
                         <% end %>
                       </div>
-                    <% else %>
-                      <span class="text-base-content/70 italic">{gettext("No groups")}</span>
                     <% end %>
                   </.data_field>
                 </div>

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}"

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 ""

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
+    # Verify badges have aria-label containing the group name (no role=status on badges)
-    assert has_element?(view, "span[role='status'][aria-label*='#{group1.name}']")
+    assert has_element?(view, "span[aria-label*='#{group1.name}']")
     assert html =~ group1.name
 
     # Verify member1's row contains the badge

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(
+        create_member(actor, %{
-          %{first_name: "Alice", last_name: "Anderson", email: "alice@example.com"},
+          first_name: "Alice",
-          actor: system_actor
+          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} =
+      {:ok, group} = create_group(actor, "Board Members")
-        Group
+      {:ok, _mg} = add_member_to_group(member, group, actor)
-        |> 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)
 
       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} =
+      {:ok, group1} = create_group(actor, "Board Members")
-        Group
+      {:ok, group2} = create_group(actor, "Active Members")
-        |> Ash.Changeset.for_create(:create, %{name: "Board Members"})
+      {:ok, _mg1} = add_member_to_group(member, group1, actor)
-        |> Ash.create(actor: actor)
+      {:ok, _mg2} = add_member_to_group(member, group2, 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)
 
       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} =
+      {:ok, group1} = create_group(actor, "Alpha")
-        Group
+      {:ok, group2} = create_group(actor, "Beta")
-        |> Ash.Changeset.for_create(:create, %{name: "Alpha"})
+      {:ok, _mg1} = add_member_to_group(member, group1, actor)
-        |> Ash.create(actor: actor)
+      {:ok, _mg2} = add_member_to_group(member, group2, 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)
 
       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(
+        create_member(actor, %{first_name: "Bob", last_name: "Brown", email: "bob@example.com"})
-          %{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)
 
+      {: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 =~
+      assert has_element?(view, "a[href*='/groups/#{group.slug}']", group.name)
-               ~r/href="[^"]*\/groups\/#{Regex.escape(group.slug)}"|navigate="[^"]*\/groups\/#{Regex.escape(group.slug)}"/
     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(
+        create_member(actor, %{
-          %{first_name: "Charlie", last_name: "Clark", email: "charlie@example.com"},
+          first_name: "Charlie",
-          actor: system_actor
+          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} =
+      {:ok, group} = create_group(actor, "Solo Group")
-        Group
+      {:ok, _mg} = add_member_to_group(member, group, actor)
-        |> 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)
 
       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} =
+          {:ok, g} = create_group(actor, name)
-            Group
-            |> Ash.Changeset.for_create(:create, %{name: name})
-            |> Ash.create(actor: actor)
-
           g
         end)
 
       for g <- groups do
-        {:ok, _mg} =
+        {:ok, _mg} = add_member_to_group(member, g, actor)
-          MemberGroup
-          |> Ash.Changeset.for_create(:create, %{member_id: member.id, group_id: g.id})
-          |> Ash.create(actor: 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(
+        create_member(actor, %{
-          %{first_name: "Diana", last_name: "Davis", email: "diana@example.com"},
+          first_name: "Diana",
-          actor: system_actor
+          last_name: "Davis",
-        )
+          email: "diana@example.com"
+        })
 
-      {:ok, group} =
+      {:ok, group} = create_group(actor, "Readers")
-        Group
+      {:ok, _mg} = add_member_to_group(member, group, actor)
-        |> 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, _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(
+        create_member(actor, %{first_name: "Eve", last_name: "Evans", email: "eve@example.com"})
-          %{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)
 
+      {: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")
+      # Group link has aria-label indicating group membership for screen readers
-      assert has_element?(view, "[role='status'][aria-label*='#{group.name}']")
+      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