fix: implement review comments

lib/mv_web/live/components/member_filter_component.ex

@@ -17,7 +17,8 @@ defmodule MvWeb.Components.MemberFilterComponent do
   ## Props
   - `:cycle_status_filter` - Current payment filter state: `nil` (all), `:paid`, or `:unpaid`
   - `:groups` - List of groups (for per-group filter rows)
-  - `:group_filters` - Map of active group filters: `%{group_id => :in | :not_in}` (nil = All for that group)
+  - `:group_filters` - Map of active group filters: `%{group_id => :in | :not_in}` (nil = All for that group).
+    Multiple active filters combine with AND (member must match all selected group conditions).
   - `:boolean_custom_fields` - List of boolean custom fields to display
   - `:boolean_filters` - Map of active boolean filters: `%{custom_field_id => true | false}`
   - `:id` - Component ID (required)
@@ -30,6 +31,8 @@ defmodule MvWeb.Components.MemberFilterComponent do
   """
   use MvWeb, :live_component
 
+  @group_filter_prefix "group_"
+
   @impl true
   def mount(socket) do
     {:ok, assign(socket, :open, false)}
@@ -43,6 +46,7 @@ defmodule MvWeb.Components.MemberFilterComponent do
       |> assign(:cycle_status_filter, assigns[:cycle_status_filter])
       |> assign(:groups, assigns[:groups] || [])
       |> assign(:group_filters, assigns[:group_filters] || %{})
+      |> assign(:group_filter_prefix, @group_filter_prefix)
       |> assign(:boolean_custom_fields, assigns[:boolean_custom_fields] || [])
       |> assign(:boolean_filters, assigns[:boolean_filters] || %{})
       |> assign(:member_count, assigns[:member_count] || 0)
@@ -199,7 +203,7 @@ defmodule MvWeb.Components.MemberFilterComponent do
                     <input
                       type="radio"
                       id={"group-filter-#{group.id}-all"}
-                      name={"group_#{group.id}"}
+                      name={"#{@group_filter_prefix}#{group.id}"}
                       value="all"
                       class="absolute opacity-0 w-0 h-0 pointer-events-none"
                       checked={Map.get(@group_filters, to_string(group.id)) == nil}
@@ -215,7 +219,7 @@ defmodule MvWeb.Components.MemberFilterComponent do
                     <input
                       type="radio"
                       id={"group-filter-#{group.id}-in"}
-                      name={"group_#{group.id}"}
+                      name={"#{@group_filter_prefix}#{group.id}"}
                       value="in"
                       class="absolute opacity-0 w-0 h-0 pointer-events-none"
                       checked={Map.get(@group_filters, to_string(group.id)) == :in}
@@ -232,7 +236,7 @@ defmodule MvWeb.Components.MemberFilterComponent do
                     <input
                       type="radio"
                       id={"group-filter-#{group.id}-not-in"}
-                      name={"group_#{group.id}"}
+                      name={"#{@group_filter_prefix}#{group.id}"}
                       value="not_in"
                       class="absolute opacity-0 w-0 h-0 pointer-events-none"
                       checked={Map.get(@group_filters, to_string(group.id)) == :not_in}
@@ -358,11 +362,13 @@ defmodule MvWeb.Components.MemberFilterComponent do
       end
 
     # Parse per-group filters (params keys "group_<uuid>" => "all"|"in"|"not_in")
+    prefix_len = String.length(@group_filter_prefix)
+
     group_filters_parsed =
       params
-      |> Enum.filter(fn {key, _} -> String.starts_with?(key, "group_") end)
+      |> Enum.filter(fn {key, _} -> String.starts_with?(key, @group_filter_prefix) end)
       |> Enum.reduce(%{}, fn {key, value_str}, acc ->
-        group_id_str = String.slice(key, 6, String.length(key) - 6)
+        group_id_str = String.slice(key, prefix_len, String.length(key) - prefix_len)
         filter_value = parse_group_filter_value(value_str)
         Map.put(acc, group_id_str, filter_value)
       end)
@@ -388,11 +394,10 @@ defmodule MvWeb.Components.MemberFilterComponent do
     Enum.each(group_filters_parsed, fn {group_id_str, new_value} ->
       in_set = MapSet.member?(all_group_ids, group_id_str)
       current_value = Map.get(current_group_filters, group_id_str)
-      normalized_new = if new_value == nil, do: nil, else: new_value
+      should_send = in_set and current_value != new_value
-      should_send = in_set and current_value != normalized_new
 
       if should_send do
-        send(self(), {:group_filter_changed, group_id_str, normalized_new})
+        send(self(), {:group_filter_changed, group_id_str, new_value})
       end
     end)
 
@@ -461,13 +466,12 @@ defmodule MvWeb.Components.MemberFilterComponent do
     do: gettext("All")
 
   defp group_filters_label(groups, group_filters) do
+    groups_by_id = Map.new(groups, fn g -> {to_string(g.id), g.name} end)
+
     names =
       group_filters
-      |> Enum.map(fn {group_id_str, _} ->
+      |> Enum.map(fn {group_id_str, _} -> Map.get(groups_by_id, group_id_str) end)
-        Enum.find(groups, fn g -> to_string(g.id) == group_id_str end)
+      |> Enum.reject(&is_nil/1)
-      end)
-      |> Enum.filter(&(&1 != nil))
-      |> Enum.map(& &1.name)
 
     label = Enum.join(names, ", ")
     truncate_label(label, 30)

lib/mv_web/live/member_live/index.ex

@@ -977,6 +977,7 @@ defmodule MvWeb.MemberLive.Index do
     end
   end
 
+  # Multiple group filters combine with AND: member must match all selected group conditions.
   defp apply_group_filters(query, group_filters, _groups) when group_filters == %{}, do: query
 
   defp apply_group_filters(query, group_filters, groups) do
@@ -1103,9 +1104,10 @@ defmodule MvWeb.MemberLive.Index do
   end
 
   defp valid_sort_field_db_or_custom?(field) when is_binary(field) do
-    field == "groups" or
+    normalized = if field == "groups", do: :groups, else: safe_member_field_atom_only(field)
-      custom_field_sort?(field) or
+
-      ((atom = safe_member_field_atom_only(field)) != nil and valid_sort_field_db_or_custom?(atom))
+    (normalized != nil and valid_sort_field_db_or_custom?(normalized)) or
+      custom_field_sort?(field)
   end
 
   defp safe_member_field_atom_only(str) do
@@ -1161,11 +1163,11 @@ defmodule MvWeb.MemberLive.Index do
   end
 
   defp sort_members_by_groups(members, order) do
-    # Members with groups first, then by first group name alphabetically
+    # Members with groups first, then by first group name alphabetically (min = first by sort order)
     first_group_name = fn member ->
-      groups = member.groups || []
+      (member.groups || [])
-      names = Enum.map(groups, & &1.name) |> Enum.sort()
+      |> Enum.map(& &1.name)
-      List.first(names)
+      |> Enum.min(fn -> nil end)
     end
 
     members

test/mv_web/member_live/index_custom_fields_sorting_test.exs

@@ -145,8 +145,10 @@ defmodule MvWeb.MemberLive.IndexCustomFieldsSortingTest do
     |> element("[data-testid='custom_field_#{field.id}']")
     |> render_click()
 
-    # Check URL was updated
+    # Check URL was updated (param order may vary)
-    assert_patch(view, "/members?query=&sort_field=custom_field_#{field.id}&sort_order=desc")
+    path = assert_patch(view)
+    assert path =~ "sort_order=desc"
+    assert path =~ "sort_field=custom_field_#{field.id}"
 
     # Verify sort state
     assert has_element?(view, "[data-testid='custom_field_#{field.id}'][aria-label='descending']")

test/mv_web/member_live/index_groups_accessibility_test.exs

@@ -52,9 +52,8 @@ defmodule MvWeb.MemberLive.IndexGroupsAccessibilityTest do
     conn = conn_with_oidc_user(conn)
     {:ok, view, html} = live(conn, "/members")
 
-    # Verify badges have accessibility attributes
+    # Verify badges have role="status" and aria-label containing the group name
-    # Badges should have role="status" and aria-label describing the group
+    assert has_element?(view, "span[role='status'][aria-label*='#{group1.name}']")
-    assert html =~ ~r/role=["']status["']/ or html =~ ~r/aria-label=.*#{group1.name}/
     assert html =~ group1.name
 
     # Verify member1's row contains the badge
@@ -84,12 +83,10 @@ defmodule MvWeb.MemberLive.IndexGroupsAccessibilityTest do
     conn: conn
   } do
     conn = conn_with_oidc_user(conn)
-    {:ok, view, html} = live(conn, "/members")
+    {:ok, view, _html} = live(conn, "/members")
 
-    # Verify sort header has aria-label
+    # Verify sort header has aria-label describing the sort state
-    # Sort header should have aria-label describing the sort state
+    assert has_element?(view, "[data-testid='groups'][aria-label]")
-    assert html =~ ~r/aria-label=.*[Gg]roup/ or
-             has_element?(view, "[data-testid='groups'][aria-label]")
   end
 
   @tag :ui
@@ -170,7 +167,7 @@ defmodule MvWeb.MemberLive.IndexGroupsAccessibilityTest do
       |> Ash.create(actor: system_actor)
 
     conn = conn_with_oidc_user(conn)
-    {:ok, view, html} = live(conn, "/members")
+    {:ok, _view, html} = live(conn, "/members")
 
     # Verify multiple badges are present
     assert html =~ member1.first_name

test/mv_web/member_live/index_groups_filter_test.exs

@@ -3,7 +3,8 @@ defmodule MvWeb.MemberLive.IndexGroupsFilterTest do
   Tests for filtering members by group in the member overview.
 
   Uses the filter dropdown (MemberFilterComponent) with one row per group:
-  All / Yes / No (per group).
+  All / Yes / No (per group). Multiple active group filters combine with AND
+  (member must match all selected group conditions).
   """
   # async: false to prevent PostgreSQL deadlocks when creating members and groups
   use MvWeb.ConnCase, async: false

test/mv_web/member_live/index_groups_integration_test.exs

@@ -78,7 +78,7 @@ defmodule MvWeb.MemberLive.IndexGroupsIntegrationTest do
     group1: group1
   } do
     conn = conn_with_oidc_user(conn)
-    {:ok, view, html} = live(conn, "/members")
+    {:ok, _view, html} = live(conn, "/members")
 
     # Verify groups column is visible by default
     assert html =~ group1.name

test/mv_web/member_live/index_groups_performance_test.exs

@@ -6,7 +6,7 @@ defmodule MvWeb.MemberLive.IndexGroupsPerformanceTest do
   - Groups are loaded with members in a single query (preloading)
   - No N+1 queries when loading members with groups
   - Filter works at database level (not in-memory)
-  - Sort works at database level
+  - Sort runs in-memory but uses preloaded group data (no extra DB queries)
   """
   # async: false to prevent PostgreSQL deadlocks when creating members and groups
   use MvWeb.ConnCase, async: false
@@ -73,7 +73,7 @@ defmodule MvWeb.MemberLive.IndexGroupsPerformanceTest do
     # For now, we verify the functionality works correctly
 
     conn = conn_with_oidc_user(conn)
-    {:ok, view, html} = live(conn, "/members")
+    {:ok, _view, html} = live(conn, "/members")
 
     # Verify all members are loaded
     Enum.each(1..10, fn i ->
@@ -193,7 +193,7 @@ defmodule MvWeb.MemberLive.IndexGroupsPerformanceTest do
     end)
 
     conn = conn_with_oidc_user(conn)
-    {:ok, view, html} = live(conn, "/members")
+    {:ok, _view, html} = live(conn, "/members")
 
     # Verify all members are loaded efficiently
     Enum.each(11..30, fn i ->