From ace59bbae6fce887c79f214c67d3983cbaf05423 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 16 Feb 2026 15:30:16 +0100 Subject: [PATCH] fix: implement review comments --- .../components/member_filter_component.ex | 32 +++++++++++-------- lib/mv_web/live/member_live/index.ex | 16 ++++++---- .../index_custom_fields_sorting_test.exs | 6 ++-- .../index_groups_accessibility_test.exs | 15 ++++----- .../member_live/index_groups_filter_test.exs | 3 +- .../index_groups_integration_test.exs | 2 +- .../index_groups_performance_test.exs | 6 ++-- 7 files changed, 43 insertions(+), 37 deletions(-) diff --git a/lib/mv_web/live/components/member_filter_component.ex b/lib/mv_web/live/components/member_filter_component.ex index 344a0c2..ef6f32e 100644 --- a/lib/mv_web/live/components/member_filter_component.ex +++ b/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 " => "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 != normalized_new + should_send = in_set and current_value != new_value 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.find(groups, fn g -> to_string(g.id) == group_id_str end) - end) - |> Enum.filter(&(&1 != nil)) - |> Enum.map(& &1.name) + |> Enum.map(fn {group_id_str, _} -> Map.get(groups_by_id, group_id_str) end) + |> Enum.reject(&is_nil/1) label = Enum.join(names, ", ") truncate_label(label, 30) diff --git a/lib/mv_web/live/member_live/index.ex b/lib/mv_web/live/member_live/index.ex index aa5f25a..ad84952 100644 --- a/lib/mv_web/live/member_live/index.ex +++ b/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 - custom_field_sort?(field) or - ((atom = safe_member_field_atom_only(field)) != nil and valid_sort_field_db_or_custom?(atom)) + normalized = if field == "groups", do: :groups, else: safe_member_field_atom_only(field) + + (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 || [] - names = Enum.map(groups, & &1.name) |> Enum.sort() - List.first(names) + (member.groups || []) + |> Enum.map(& &1.name) + |> Enum.min(fn -> nil end) end members diff --git a/test/mv_web/member_live/index_custom_fields_sorting_test.exs b/test/mv_web/member_live/index_custom_fields_sorting_test.exs index c8201fd..2f12fcc 100644 --- a/test/mv_web/member_live/index_custom_fields_sorting_test.exs +++ b/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 - assert_patch(view, "/members?query=&sort_field=custom_field_#{field.id}&sort_order=desc") + # Check URL was updated (param order may vary) + 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']") 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 7faad6e..ab9b728 100644 --- a/test/mv_web/member_live/index_groups_accessibility_test.exs +++ b/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 - # Badges should have role="status" and aria-label describing the group - assert html =~ ~r/role=["']status["']/ or html =~ ~r/aria-label=.*#{group1.name}/ + # Verify badges have role="status" and aria-label containing the group name + assert has_element?(view, "span[role='status'][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 - # Sort header should have aria-label describing the sort state - assert html =~ ~r/aria-label=.*[Gg]roup/ or - has_element?(view, "[data-testid='groups'][aria-label]") + # Verify sort header has aria-label describing the sort state + assert 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 diff --git a/test/mv_web/member_live/index_groups_filter_test.exs b/test/mv_web/member_live/index_groups_filter_test.exs index c92a0bd..782ab33 100644 --- a/test/mv_web/member_live/index_groups_filter_test.exs +++ b/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 diff --git a/test/mv_web/member_live/index_groups_integration_test.exs b/test/mv_web/member_live/index_groups_integration_test.exs index 0e0f39c..3075d54 100644 --- a/test/mv_web/member_live/index_groups_integration_test.exs +++ b/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 diff --git a/test/mv_web/member_live/index_groups_performance_test.exs b/test/mv_web/member_live/index_groups_performance_test.exs index daa2e7f..761c4eb 100644 --- a/test/mv_web/member_live/index_groups_performance_test.exs +++ b/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 ->