From 900f322422ecaf3cf489723c0f5ba41ab7b0d4b0 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 12 Feb 2026 15:08:40 +0100 Subject: [PATCH] fix: pr comments --- docs/groups-architecture.md | 21 ++--- lib/mv_web/live/group_live/show.ex | 79 +++++++++++-------- priv/gettext/de/LC_MESSAGES/default.po | 5 ++ priv/gettext/default.pot | 5 ++ priv/gettext/en/LC_MESSAGES/default.po | 5 ++ .../group_live/show_accessibility_test.exs | 4 +- .../live/group_live/show_add_member_test.exs | 49 ++++++------ .../show_add_remove_members_test.exs | 2 +- .../group_live/show_authorization_test.exs | 10 +-- .../group_live/show_member_search_test.exs | 2 +- test/support/group_live_helpers.ex | 59 ++++++++++++++ 11 files changed, 161 insertions(+), 80 deletions(-) create mode 100644 test/support/group_live_helpers.ex diff --git a/docs/groups-architecture.md b/docs/groups-architecture.md index 8251a4b..74784a8 100644 --- a/docs/groups-architecture.md +++ b/docs/groups-architecture.md @@ -314,17 +314,18 @@ lib/ - Display group name and description - List all members in group - Link to member detail pages -- Add members to group (via modal with search/autocomplete) +- Add members to group (via inline combobox with search/autocomplete) - Remove members from group (via remove button per member) - Edit group button (navigates to `/groups/:slug/edit`) - Delete group button (with confirmation modal) **Add Member Functionality:** - "Add Member" button displayed above member table (only for users with `:update` permission) -- Opens modal with member search/autocomplete +- Opens inline add member area with member search/autocomplete (combobox) - Search filters out members already in the group - Selecting a member adds them to the group immediately - Success/error flash messages provide feedback +- "Cancel" button closes the inline add member area without adding **Remove Member Functionality:** - "Remove" button (icon button) for each member in table (only for users with `:update` permission) @@ -818,7 +819,7 @@ Each functional unit can be implemented as a **separate issue**: **Tasks:** 1. Add "Add Member" button above member table in Group Detail View -2. Implement modal with member search/autocomplete +2. Implement inline add member with search/autocomplete 3. Add "Remove" button for each member in table 4. Implement add/remove functionality with flash messages 5. Ensure proper authorization checks @@ -1001,7 +1002,7 @@ Each functional unit can be implemented as a **separate issue**: **Tasks:** - Add "Add Member" button above member table in Group Detail View (`/groups/:slug`) -- Implement modal for member selection with search/autocomplete +- Implement inline add member for member selection with search/autocomplete - Add "Remove" button for each member in the member table - Implement add member functionality (create MemberGroup association) - Implement remove member functionality (destroy MemberGroup association) @@ -1012,7 +1013,7 @@ Each functional unit can be implemented as a **separate issue**: **Acceptance Criteria:** - "Add Member" button is visible above member table (only for users with `:update` permission) -- Clicking "Add Member" opens a modal with member search/autocomplete +- Clicking "Add Member" opens inline add member area with member search/autocomplete - Search filters members and excludes those already in the group - Selecting a member from search adds them to the group - Success flash message is displayed when member is added @@ -1021,7 +1022,7 @@ Each functional unit can be implemented as a **separate issue**: - Clicking "Remove" immediately removes the member from the group (no confirmation dialog) - Success flash message is displayed when member is removed - Group member list and member count update automatically after add/remove -- Modal closes after successful member addition +- Inline add member area closes after successful member addition - Authorization is enforced server-side in event handlers - UI respects permission checks (buttons hidden for unauthorized users) @@ -1031,15 +1032,15 @@ Each functional unit can be implemented as a **separate issue**: - Use `Membership.destroy_member_group/1` for removing members - Filter search results to exclude members already in the group (check `group.members`) - Reload group with `:members` and `:member_count` after operations -- Follow existing modal patterns (similar to delete confirmation modal) +- Use inline combobox pattern (delete group uses a separate confirmation modal) - Ensure accessibility: proper ARIA labels, keyboard navigation, focus management **UI/UX Details:** -- Modal title: "Add Member to Group" +- Inline add member section (no modal; combobox above member table) - Search input placeholder: "Search for a member..." - Search results show member name and email -- "Add" button in modal (disabled until member selected) -- "Cancel" button to close modal +- "Add" button in inline area (disabled until member selected) +- "Cancel" button to close inline add member area - Remove button can be an icon button (trash icon) with tooltip - Flash messages: "Member added successfully" / "Member removed successfully" / error messages diff --git a/lib/mv_web/live/group_live/show.ex b/lib/mv_web/live/group_live/show.ex index af8582f..0251fb6 100644 --- a/lib/mv_web/live/group_live/show.ex +++ b/lib/mv_web/live/group_live/show.ex @@ -15,6 +15,8 @@ defmodule MvWeb.GroupLive.Show do """ use MvWeb, :live_view + require Logger + import MvWeb.LiveHelpers, only: [current_actor: 1] import MvWeb.Authorization @@ -162,7 +164,7 @@ defmodule MvWeb.GroupLive.Show do phx-hook="ComboBox" phx-focus="show_member_dropdown" phx-debounce="300" - phx-window-keydown="member_dropdown_keydown" + phx-keydown="member_dropdown_keydown" phx-mounted={JS.focus()} value={@member_search_query} placeholder={ @@ -231,6 +233,14 @@ defmodule MvWeb.GroupLive.Show do > <.icon name="hero-plus" class="size-5" /> + <% else %> <.button @@ -439,12 +449,7 @@ defmodule MvWeb.GroupLive.Show do @impl true def handle_event("show_member_dropdown", _params, socket) do - # Reload group to ensure we have the latest members list before filtering - actor = current_actor(socket) - group = socket.assigns.group - socket = reload_group(socket, group.slug, actor) - - # Load available members with empty query when input is focused + # Use existing group.members for filtering; reload only on add/remove socket = socket |> load_available_members("") @@ -454,6 +459,19 @@ defmodule MvWeb.GroupLive.Show do {:noreply, socket} end + @impl true + def handle_event("hide_add_member_input", _params, socket) do + {:noreply, + socket + |> assign(:show_add_member_input, false) + |> assign(:member_search_query, "") + |> assign(:available_members, []) + |> assign(:selected_member_ids, []) + |> assign(:selected_members, []) + |> assign(:show_member_dropdown, false) + |> assign(:focused_member_index, nil)} + end + @impl true def handle_event("hide_member_dropdown", _params, socket) do {:noreply, assign(socket, show_member_dropdown: false, focused_member_index: nil)} @@ -514,11 +532,7 @@ defmodule MvWeb.GroupLive.Show do @impl true def handle_event("search_members", %{"member_search" => query}, socket) do - # Reload group to ensure we have the latest members list before filtering - actor = current_actor(socket) - group = socket.assigns.group - socket = reload_group(socket, group.slug, actor) - + # Use existing group.members for filtering; reload only on add/remove socket = socket |> assign(:member_search_query, query) @@ -574,7 +588,8 @@ defmodule MvWeb.GroupLive.Show do # Server-side authorization check if can?(actor, :update, group) do - perform_add_members(socket, group, socket.assigns.selected_member_ids, actor) + member_ids = Enum.uniq(socket.assigns.selected_member_ids) + perform_add_members(socket, group, member_ids, actor) else {:noreply, socket @@ -648,23 +663,29 @@ defmodule MvWeb.GroupLive.Show do defp load_available_members(socket, query) do require Ash.Query + current_member_ids = group_member_ids_set(socket.assigns.group) base_query = available_members_base_query(query) - limited_query = Ash.Query.limit(base_query, 10) + + # Fetch more than 10, then exclude already-in-group and take 10 (avoids empty dropdown when first N are all in group) + fetch_limit = 50 + limited_query = Ash.Query.limit(base_query, fetch_limit) actor = current_actor(socket) case Ash.read(limited_query, actor: actor, domain: Mv.Membership) do {:ok, members} -> - current_member_ids = group_member_ids_set(socket.assigns.group) + available = + members + |> Enum.reject(fn m -> MapSet.member?(current_member_ids, m.id) end) + |> Enum.take(10) - filtered_members = - Enum.reject(members, fn member -> - MapSet.member?(current_member_ids, member.id) - end) + assign(socket, available_members: available) - assign(socket, available_members: filtered_members) + {:error, error} -> + Logger.warning("Failed to load available members for group: #{inspect(error)}") - {:error, _} -> - assign(socket, available_members: []) + socket + |> put_flash(:error, gettext("Could not load member search. Please try again.")) + |> assign(:available_members, []) end end @@ -681,18 +702,8 @@ defmodule MvWeb.GroupLive.Show do end defp group_member_ids_set(group) do - cond do - is_list(group.members) and group.members != [] -> - group.members - |> Enum.map(& &1.id) - |> MapSet.new() - - is_list(group.members) -> - MapSet.new() - - true -> - MapSet.new() - end + members = group.members || [] + members |> Enum.map(& &1.id) |> MapSet.new() end defp perform_add_members(socket, group, member_ids, actor) when is_list(member_ids) do diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 2644994..d6cd9da 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2260,6 +2260,11 @@ msgstr "Nicht berechtigt." msgid "Could not load data fields. Please check your permissions." msgstr "Datenfelder konnten nicht geladen werden. Bitte überprüfen Sie Ihre Berechtigungen." +#: lib/mv_web/live/group_live/show.ex +#, elixir-autogen, elixir-format +msgid "Could not load member search. Please try again." +msgstr "Mitgliedersuche konnte nicht geladen werden. Bitte versuchen Sie es erneut." + #: lib/mv_web/live/group_live/show.ex #, elixir-autogen, elixir-format msgid "Add Member" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 5c1c4a5..608f935 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2261,6 +2261,11 @@ msgstr "" msgid "Could not load data fields. Please check your permissions." msgstr "" +#: lib/mv_web/live/group_live/show.ex +#, elixir-autogen, elixir-format +msgid "Could not load member search. Please try again." +msgstr "" + #: lib/mv_web/live/group_live/show.ex #, elixir-autogen, elixir-format msgid "Add Member" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 3fe9ce3..18dbd04 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2259,6 +2259,11 @@ msgstr "" msgid "Could not load data fields. Please check your permissions." msgstr "" +#: lib/mv_web/live/group_live/show.ex +#, elixir-autogen, elixir-format +msgid "Could not load member search. Please try again." +msgstr "" + #: lib/mv_web/live/global_settings_live.ex #, elixir-autogen, elixir-format, fuzzy msgid "CSV files only, maximum %{size} MB" diff --git a/test/mv_web/live/group_live/show_accessibility_test.exs b/test/mv_web/live/group_live/show_accessibility_test.exs index c1be3af..97ce469 100644 --- a/test/mv_web/live/group_live/show_accessibility_test.exs +++ b/test/mv_web/live/group_live/show_accessibility_test.exs @@ -93,7 +93,7 @@ defmodule MvWeb.GroupLive.ShowAccessibilityTest do end describe "keyboard navigation" do - test "tab navigation works in modal", %{conn: conn} do + test "tab navigation works in inline add member area", %{conn: conn} do # This test verifies that keyboard navigation is possible # Actual tab order testing would require more complex setup group = Fixtures.group_fixture() @@ -107,7 +107,7 @@ defmodule MvWeb.GroupLive.ShowAccessibilityTest do html = render(view) - # Modal should have focusable elements + # Inline add member area should have focusable elements assert html =~ ~r/input|button/ || html =~ "#member-search-input" end diff --git a/test/mv_web/live/group_live/show_add_member_test.exs b/test/mv_web/live/group_live/show_add_member_test.exs index a8bb7bd..783db9d 100644 --- a/test/mv_web/live/group_live/show_add_member_test.exs +++ b/test/mv_web/live/group_live/show_add_member_test.exs @@ -1,11 +1,12 @@ defmodule MvWeb.GroupLive.ShowAddMemberTest do @moduledoc """ - Tests for adding members to groups via the Add Member modal. + Tests for adding members to groups via the inline Add Member combobox. Tests successful add, error handling, and edge cases. """ use MvWeb.ConnCase, async: false import Phoenix.LiveViewTest + import MvWeb.GroupLiveHelpers use Gettext, backend: MvWeb.Gettext alias Mv.Membership @@ -28,32 +29,11 @@ defmodule MvWeb.GroupLive.ShowAddMemberTest do {:ok, view, _html} = live(conn, "/groups/#{group.slug}") - # Open inline input - view - |> element("button", "Add Member") - |> render_click() + open_add_member(view) + search_member(view, "Alice") + select_member(view, member) + add_selected(view) - # Search and select member - view - |> element("#member-search-input") - |> render_focus() - - # phx-change is on the form, so we need to trigger it via the form - view - |> element("form[phx-change='search_members']") - |> render_change(%{"member_search" => "Alice"}) - - # Select member - view - |> element("[data-member-id='#{member.id}']") - |> render_click() - - # Click Add button - view - |> element("button[phx-click='add_selected_members']") - |> render_click() - - # Verify member appears in group list (no success flash message) html = render(view) assert html =~ "Alice" assert html =~ "Johnson" @@ -198,7 +178,7 @@ defmodule MvWeb.GroupLive.ShowAddMemberTest do assert new_count == initial_count + 1 end - test "modal closes after successful member addition", %{conn: conn} do + test "inline add member area closes after successful member addition", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() group = Fixtures.group_fixture() @@ -242,6 +222,21 @@ defmodule MvWeb.GroupLive.ShowAddMemberTest do # Inline input should be closed (Add Member button should be visible again) refute has_element?(view, "#member-search-input") end + + test "Cancel button closes inline add member area without adding", %{conn: conn} do + group = Fixtures.group_fixture() + + {:ok, view, _html} = live(conn, "/groups/#{group.slug}") + + open_add_member(view) + assert has_element?(view, "#member-search-input") + assert has_element?(view, "button[phx-click='hide_add_member_input']") + + cancel_add_member(view) + + refute has_element?(view, "#member-search-input") + assert has_element?(view, "button", "Add Member") + end end describe "error handling" do diff --git a/test/mv_web/live/group_live/show_add_remove_members_test.exs b/test/mv_web/live/group_live/show_add_remove_members_test.exs index 6c140c3..c014372 100644 --- a/test/mv_web/live/group_live/show_add_remove_members_test.exs +++ b/test/mv_web/live/group_live/show_add_remove_members_test.exs @@ -1,6 +1,6 @@ defmodule MvWeb.GroupLive.ShowAddRemoveMembersTest do @moduledoc """ - UI tests for Add/Remove Member buttons visibility and modal display. + UI tests for Add/Remove Member buttons visibility and inline add member display. Tests UI rendering and permission-based visibility. """ diff --git a/test/mv_web/live/group_live/show_authorization_test.exs b/test/mv_web/live/group_live/show_authorization_test.exs index 744b9ad..f121b36 100644 --- a/test/mv_web/live/group_live/show_authorization_test.exs +++ b/test/mv_web/live/group_live/show_authorization_test.exs @@ -217,7 +217,7 @@ defmodule MvWeb.GroupLive.ShowAuthorizationTest do end @tag role: :read_only - test "modal cannot be opened for unauthorized users", %{conn: conn} do + test "inline add member area cannot be opened for unauthorized users", %{conn: conn} do group = Fixtures.group_fixture() {:ok, _view, html} = live(conn, "/groups/#{group.slug}") @@ -262,14 +262,14 @@ defmodule MvWeb.GroupLive.ShowAuthorizationTest do match?({:error, {:live_redirect, %{to: "/groups"}}}, result) end + @tag :skip test "non-existent member IDs are handled", %{conn: conn} do + # Future: test add_selected_members with invalid ID (would require pushing event with forged selected_member_ids) group = Fixtures.group_fixture() - {:ok, _view, _html} = live(conn, "/groups/#{group.slug}") + {:ok, view, _html} = live(conn, "/groups/#{group.slug}") - # Try to add non-existent member (if possible) - # Implementation should handle this gracefully - # This tests error handling for invalid IDs + assert has_element?(view, "button", "Add Member") end test "non-existent group IDs are handled", %{conn: conn} do diff --git a/test/mv_web/live/group_live/show_member_search_test.exs b/test/mv_web/live/group_live/show_member_search_test.exs index 8dff33e..75d1803 100644 --- a/test/mv_web/live/group_live/show_member_search_test.exs +++ b/test/mv_web/live/group_live/show_member_search_test.exs @@ -1,6 +1,6 @@ defmodule MvWeb.GroupLive.ShowMemberSearchTest do @moduledoc """ - UI tests for member search functionality in Add Member modal. + UI tests for member search functionality in inline Add Member combobox. Tests search behavior and filtering of members already in group. """ diff --git a/test/support/group_live_helpers.ex b/test/support/group_live_helpers.ex new file mode 100644 index 0000000..50e2f9e --- /dev/null +++ b/test/support/group_live_helpers.ex @@ -0,0 +1,59 @@ +defmodule MvWeb.GroupLiveHelpers do + @moduledoc """ + Helpers for Group LiveView tests (e.g. group show add/remove member flow). + + Use these to reduce duplication in tests that open the add member area, + search, select, and add members. + """ + + import Phoenix.LiveViewTest + + @doc """ + Opens the inline add member area by clicking "Add Member". + """ + def open_add_member(view) do + view + |> element("button", "Add Member") + |> render_click() + end + + @doc """ + Triggers member search by focusing the input and sending a form change with the given query. + """ + def search_member(view, query) do + view + |> element("#member-search-input") + |> render_focus() + + view + |> element("form[phx-change='search_members']") + |> render_change(%{"member_search" => query}) + end + + @doc """ + Clicks the option for the given member in the dropdown (by data-member-id). + """ + def select_member(view, member) do + view + |> element("[data-member-id='#{member.id}']") + |> render_click() + end + + @doc """ + Clicks the "Add" button (add_selected_members). + """ + def add_selected(view) do + view + |> element("button[phx-click='add_selected_members']") + |> render_click() + end + + @doc """ + Clicks the "Cancel" button to close the inline add member area. + """ + def cancel_add_member(view) do + view + |> element("button[phx-click='hide_add_member_input']") + |> render_click() + end +end