From 91cf7cca6a7dcd0caf8165ec63079ba8493ceefc Mon Sep 17 00:00:00 2001 From: carla Date: Wed, 25 Feb 2026 15:09:37 +0100 Subject: [PATCH] feat: conistent danger zone delete flow --- DESIGN_DUIDELINES.md | 43 ++++++++ .../live/custom_field_live/form_component.ex | 36 +++++++ .../live/custom_field_live/index_component.ex | 60 +++++++----- lib/mv_web/live/datafields_live.ex | 11 +++ lib/mv_web/live/group_live/form.ex | 25 +++++ lib/mv_web/live/group_live/index.ex | 18 ---- lib/mv_web/live/group_live/show.ex | 55 ++++++++--- .../live/member_field_live/index_component.ex | 12 +-- lib/mv_web/live/member_live/form.ex | 84 ++++++++++++++++ lib/mv_web/live/member_live/show.ex | 2 + lib/mv_web/live/role_live/index.ex | 98 +------------------ lib/mv_web/live/role_live/index.html.heex | 42 -------- lib/mv_web/live/role_live/show.ex | 40 ++++++-- lib/mv_web/live/user_live/form.ex | 71 ++++++++++++++ lib/mv_web/live/user_live/index.ex | 45 +-------- lib/mv_web/live/user_live/index.html.heex | 24 ----- lib/mv_web/live/user_live/show.ex | 67 +++++++++++++ .../member_live/form_error_handling_test.exs | 47 +++++++++ test/mv_web/member_live/show_test.exs | 6 +- 19 files changed, 499 insertions(+), 287 deletions(-) diff --git a/DESIGN_DUIDELINES.md b/DESIGN_DUIDELINES.md index 37428a3..e3faf50 100644 --- a/DESIGN_DUIDELINES.md +++ b/DESIGN_DUIDELINES.md @@ -365,3 +365,46 @@ Detail pages should not drift into random layouts. Add to this glossary when new terminology appears. --- + +## 14) Destructive actions: Delete flow (canonical) + +This section defines the canonical delete flow for list/detail/form resources (e.g. members). Use it as the single pattern; do not introduce a second pattern elsewhere. + +### Tables: no row action buttons +- **MUST NOT:** Show Edit or Delete as row action buttons (or dropdown actions) in list/table views. +- **MUST:** Remove any existing edit/delete row actions from tables so that the only way to edit or delete is via the flow below. + +### Navigation: row click → details +- **MUST:** Clicking a table row navigates to the resource details page (e.g. `/members/:id`). +- **MUST NOT:** Use the table for primary edit/delete actions. + +### Edit: from details header, not from table +- **MUST:** Provide a clear primary “Edit” CTA in the details page header (e.g. “Edit member”). +- **MUST:** Edit is reached from the details page (e.g. “Edit member” button in header), not from the list/table. + +### Delete: only via “Danger zone” +- **MUST:** Delete is available only in a dedicated “Danger zone” section at the bottom of the page. +- **MUST:** Use the same “Danger zone” on both the details page and the edit form when the user is authorized to destroy the resource. +- **MUST NOT:** Place delete in the table, in the header next to Edit, or in any other location outside the Danger zone. + +### Danger zone layout and wording (canonical pattern) +- **Heading:** “Danger zone” (H2, `aria-labelledby` for the section, semantic colour e.g. `text-error`). +- **Explanatory text:** One short paragraph stating that the action cannot be undone and mentioning consequences (e.g. related data removed). Use `text-base-content/70` for the text. +- **Layout:** Section with heading outside a bordered box; content inside a single bordered, rounded box (`border border-base-300 rounded-lg p-4 bg-base-100`). +- **Button:** One destructive action only (e.g. “Delete member”). Use CoreComponents `<.button variant="danger">`. No primary or secondary actions mixed inside the Danger zone. + +### Confirmation and button semantics +- **MUST:** Use a single confirmation step (e.g. `data-confirm` / browser confirm or one modal). Do not introduce a second confirmation pattern in this flow. +- **Confirm copy:** Message must include the resource name and state that the action “cannot be undone” (e.g. “Are you sure you want to delete %{name}? This action cannot be undone.”). +- **Button:** Accessible label (visible text + `aria-label` that includes the resource name, e.g. “Delete member %{name}”). Icon (e.g. trash) is optional and must not replace the text label for the primary action. + +### Accessibility +- **MUST:** Button has an accessible name (`aria-label` when icon-only or in addition to visible text as above). +- **MUST:** Focus and keyboard: button is focusable and activatable via keyboard; focus management must not trap the user. +- **MUST:** Contrast and visibility: Danger zone heading and button use semantic danger styling with sufficient contrast (WCAG AA). + +### Authorization visibility +- **MUST:** Show the Danger zone only when the current user is authorized to destroy the resource (e.g. `can?(current_user, :destroy, resource)`). +- **MUST NOT:** Show the Danger zone or the delete button when the user cannot destroy the resource; no “disabled” delete button for unauthorized users. + +--- diff --git a/lib/mv_web/live/custom_field_live/form_component.ex b/lib/mv_web/live/custom_field_live/form_component.ex index 8e59ac9..5f09a8d 100644 --- a/lib/mv_web/live/custom_field_live/form_component.ex +++ b/lib/mv_web/live/custom_field_live/form_component.ex @@ -98,6 +98,33 @@ defmodule MvWeb.CustomFieldLive.FormComponent do label={gettext("Show in overview")} /> + <%= if @custom_field do %> + <%!-- Danger zone: canonical pattern (same as member form) --%> +
+

+ {gettext("Danger zone")} +

+
+

+ {gettext( + "Deleting this data field cannot be undone. All custom field values for this field will be permanently removed." + )} +

+ <.button + type="button" + variant="danger" + phx-click="request_delete" + phx-target={@myself} + data-testid="custom-field-delete" + aria-label={gettext("Delete data field %{name}", name: @custom_field.name)} + > + <.icon name="hero-trash" class="size-4" /> + {gettext("Delete data field")} + +
+
+ <% end %> +
<.button type="button" variant="neutral" phx-click="cancel" phx-target={@myself}> {gettext("Cancel")} @@ -170,6 +197,15 @@ defmodule MvWeb.CustomFieldLive.FormComponent do {:noreply, socket} end + @impl true + def handle_event("request_delete", _params, socket) do + if custom_field = socket.assigns[:custom_field] do + send(self(), {:open_delete_modal_for, custom_field}) + end + + {:noreply, socket} + end + defp assign_form(%{assigns: %{custom_field: custom_field}} = socket) do form = if custom_field do diff --git a/lib/mv_web/live/custom_field_live/index_component.ex b/lib/mv_web/live/custom_field_live/index_component.ex index ebc4930..b0e9862 100644 --- a/lib/mv_web/live/custom_field_live/index_component.ex +++ b/lib/mv_web/live/custom_field_live/index_component.ex @@ -59,7 +59,7 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do JS.push("edit_custom_field", value: %{id: custom_field.id}, target: @myself) end } - row_tooltip={gettext("Click for dataield details")} + row_tooltip={gettext("Click to edit datafield")} > <:col :let={{_id, custom_field}} label={gettext("Name")}>{custom_field.name} @@ -96,22 +96,6 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do {gettext("No")} - - <:action :let={{_id, custom_field}}> - <.link phx-click={ - JS.push("edit_custom_field", value: %{id: custom_field.id}, target: @myself) - }> - {gettext("Edit datafield")} - - - - <:action :let={{_id, custom_field}}> - <.link phx-click={ - JS.push("prepare_delete", value: %{id: custom_field.id}, target: @myself) - }> - {gettext("Delete")} - -
@@ -223,16 +207,38 @@ defmodule MvWeb.CustomFieldLive.IndexComponent do # Get actor from assigns or fall back to socket assigns actor = Map.get(assigns, :actor, socket.assigns[:actor]) - {:ok, - socket - |> assign(assigns) - |> assign_new(:show_form, fn -> false end) - |> assign_new(:form_id, fn -> "custom-field-form-new" end) - |> assign_new(:editing_custom_field, fn -> nil end) - |> assign_new(:show_delete_modal, fn -> false end) - |> assign_new(:custom_field_to_delete, fn -> nil end) - |> assign_new(:slug_confirmation, fn -> "" end) - |> stream(:custom_fields, stream_custom_fields(actor, self()), reset: true)} + socket = + socket + |> assign(assigns) + |> assign_new(:show_form, fn -> false end) + |> assign_new(:form_id, fn -> "custom-field-form-new" end) + |> assign_new(:editing_custom_field, fn -> nil end) + |> assign_new(:show_delete_modal, fn -> false end) + |> assign_new(:custom_field_to_delete, fn -> nil end) + |> assign_new(:slug_confirmation, fn -> "" end) + |> stream(:custom_fields, stream_custom_fields(actor, self()), reset: true) + + # Open delete modal when requested from form (e.g. Danger zone in FormComponent) + socket = + case Map.get(assigns, :open_delete_for_id) do + nil -> + socket + + id -> + custom_field = + Ash.get!(Mv.Membership.CustomField, id, + load: [:assigned_members_count], + actor: actor + ) + + socket + |> assign(:show_delete_modal, true) + |> assign(:custom_field_to_delete, custom_field) + |> assign(:slug_confirmation, "") + |> assign(:open_delete_for_id, nil) + end + + {:ok, socket} end @impl true diff --git a/lib/mv_web/live/datafields_live.ex b/lib/mv_web/live/datafields_live.ex index f922d22..0fc4c3c 100644 --- a/lib/mv_web/live/datafields_live.ex +++ b/lib/mv_web/live/datafields_live.ex @@ -101,6 +101,17 @@ defmodule MvWeb.DatafieldsLive do {:noreply, assign(socket, :active_editing_section, section)} end + # Open delete modal for custom field (triggered from Danger zone in FormComponent) + @impl true + def handle_info({:open_delete_modal_for, custom_field}, socket) do + send_update(MvWeb.CustomFieldLive.IndexComponent, + id: "custom-fields-component", + open_delete_for_id: custom_field.id + ) + + {:noreply, socket} + end + @impl true def handle_info({:member_field_saved, _member_field, action}, socket) do {:ok, updated_settings} = Membership.get_settings() diff --git a/lib/mv_web/live/group_live/form.ex b/lib/mv_web/live/group_live/form.ex index d9999d3..490214f 100644 --- a/lib/mv_web/live/group_live/form.ex +++ b/lib/mv_web/live/group_live/form.ex @@ -101,6 +101,31 @@ defmodule MvWeb.GroupLive.Form do rows="4" /> + + <%!-- Danger zone: canonical pattern (same as member form) --%> + <%= if @group && can?(@current_user, :destroy, @group) do %> +
+

+ {gettext("Danger zone")} +

+
+

+ {gettext( + "Deleting this group cannot be undone. All member-group associations will be permanently removed." + )} +

+ <.button + variant="danger" + navigate={~p"/groups/#{@group.slug}?confirm_delete=1"} + data-testid="group-form-delete-btn" + aria-label={gettext("Delete group %{name}", name: @group.name)} + > + <.icon name="hero-trash" class="size-4" /> + {gettext("Delete group")} + +
+
+ <% end %> diff --git a/lib/mv_web/live/group_live/index.ex b/lib/mv_web/live/group_live/index.ex index 76663fd..ff22b91 100644 --- a/lib/mv_web/live/group_live/index.ex +++ b/lib/mv_web/live/group_live/index.ex @@ -77,24 +77,6 @@ defmodule MvWeb.GroupLive.Index do <:col :let={group} label={gettext("Members")} class="text-right"> {group.member_count || 0} - <:action :let={group}> - <.button - variant="ghost" - size="sm" - navigate={~p"/groups/#{group.slug}"} - > - {gettext("View")} - - <%= if can?(@current_user, :update, Mv.Membership.Group) do %> - <.button - variant="ghost" - size="sm" - navigate={~p"/groups/#{group.slug}/edit"} - > - {gettext("Edit group")} - - <% end %> - <% end %> diff --git a/lib/mv_web/live/group_live/show.ex b/lib/mv_web/live/group_live/show.ex index 7e2d57f..d970f2a 100644 --- a/lib/mv_web/live/group_live/show.ex +++ b/lib/mv_web/live/group_live/show.ex @@ -39,18 +39,18 @@ defmodule MvWeb.GroupLive.Show do end @impl true - def handle_params(%{"slug" => slug}, _url, socket) do + def handle_params(%{"slug" => slug} = params, _url, socket) do actor = current_actor(socket) # Check if user can read groups if can?(actor, :read, Mv.Membership.Group) do - load_group_by_slug(socket, slug, actor) + load_group_by_slug(socket, slug, actor, params) else {:noreply, redirect(socket, to: ~p"/members")} end end - defp load_group_by_slug(socket, slug, actor) do + defp load_group_by_slug(socket, slug, actor, params \\ %{}) do # Load group with members and member_count # Using explicit load ensures efficient preloading of members relationship require Ash.Query @@ -68,10 +68,16 @@ defmodule MvWeb.GroupLive.Show do |> redirect(to: ~p"/groups")} {:ok, group} -> - {:noreply, - socket - |> assign(:page_title, group.name) - |> assign(:group, group)} + open_delete = params["confirm_delete"] == "1" && can?(actor, :destroy, group) + + socket = + socket + |> assign(:page_title, group.name) + |> assign(:group, group) + |> assign(:show_delete_modal, open_delete) + |> assign(:name_confirmation, "") + + {:noreply, socket} {:error, _error} -> {:noreply, @@ -105,15 +111,6 @@ defmodule MvWeb.GroupLive.Show do {gettext("Edit group")} <% end %> - <%= if can?(@current_user, :destroy, @group) do %> - <.button - variant="danger" - phx-click="open_delete_modal" - data-testid="group-show-delete-btn" - > - {gettext("Delete")} - - <% end %> @@ -339,6 +336,32 @@ defmodule MvWeb.GroupLive.Show do + <%!-- Danger zone: canonical pattern (same as member show) --%> + <%= if can?(@current_user, :destroy, @group) do %> +
+

+ {gettext("Danger zone")} +

+
+

+ {gettext( + "Deleting this group cannot be undone. All member-group associations will be permanently removed." + )} +

+ <.button + variant="danger" + type="button" + phx-click="open_delete_modal" + data-testid="group-show-delete-btn" + aria-label={gettext("Delete group %{name}", name: @group.name)} + > + <.icon name="hero-trash" class="size-4" /> + {gettext("Delete group")} + +
+
+ <% end %> + <%!-- Delete Confirmation Modal --%> <%= if assigns[:show_delete_modal] do %> diff --git a/lib/mv_web/live/member_field_live/index_component.ex b/lib/mv_web/live/member_field_live/index_component.ex index 419b585..28384b5 100644 --- a/lib/mv_web/live/member_field_live/index_component.ex +++ b/lib/mv_web/live/member_field_live/index_component.ex @@ -57,7 +57,7 @@ defmodule MvWeb.MemberFieldLive.IndexComponent do JS.push("edit_member_field", value: %{"field" => field_name}, target: @myself) end } - row_tooltip={gettext("Click for datafield details")} + row_tooltip={gettext("Click to edit datafield")} > <:col :let={{_field_name, field_data}} label={gettext("Name")}> {MemberFields.label(field_data.field)} @@ -92,16 +92,6 @@ defmodule MvWeb.MemberFieldLive.IndexComponent do {gettext("No")} - - <:action :let={{_field_name, field_data}}> - <.link - phx-click="edit_member_field" - phx-value-field={Atom.to_string(field_data.field)} - phx-target={@myself} - > - {gettext("Edit datafield")} - - """ diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 9138236..1875205 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -20,6 +20,7 @@ defmodule MvWeb.MemberLive.Form do """ use MvWeb, :live_view + require Logger import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] alias Mv.Membership @@ -246,6 +247,42 @@ defmodule MvWeb.MemberLive.Form do {gettext("Save Member")} + + <%!-- Danger zone: same section pattern as MemberLive.Show (canonical) --%> + <%= if @member && can?(@current_user, :destroy, @member) do %> +
+

+ {gettext("Danger zone")} +

+
+

+ {gettext( + "Deleting this member cannot be undone. All related data (e.g. membership fee cycles) will be removed." + )} +

+ <.button + variant="danger" + type="button" + phx-click="delete" + phx-value-id={@member.id} + data-confirm={ + gettext("Are you sure you want to delete %{name}? This action cannot be undone.", + name: MvWeb.Helpers.MemberHelpers.display_name(@member) + ) + } + data-testid="member-delete" + aria-label={ + gettext("Delete member %{name}", + name: MvWeb.Helpers.MemberHelpers.display_name(@member) + ) + } + > + <.icon name="hero-trash" class="size-4" /> + {gettext("Delete member")} + +
+
+ <% end %> @@ -366,6 +403,40 @@ defmodule MvWeb.MemberLive.Form do end end + @impl true + def handle_event("delete", %{"id" => id}, socket) do + member = socket.assigns.member + actor = current_actor(socket) + + if is_nil(member) do + {:noreply, put_flash(socket, :error, gettext("Member not found"))} + else + if to_string(id) != to_string(member.id) do + {:noreply, put_flash(socket, :error, gettext("Member not found"))} + else + case Ash.destroy(member, actor: actor) do + :ok -> + {:noreply, + socket + |> put_flash(:success, gettext("Member deleted successfully")) + |> push_navigate(to: ~p"/members")} + + {:error, %Ash.Error.Forbidden{}} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to delete this member") + )} + + {:error, error} -> + Logger.warning("Member delete failed: member_id=#{member.id} error=#{inspect(error)}") + {:noreply, put_flash(socket, :error, format_destroy_error(error))} + end + end + end + end + defp handle_save_success(socket, member) do notify_parent({:saved, member}) @@ -413,6 +484,19 @@ defmodule MvWeb.MemberLive.Form do end end + defp format_destroy_error(%Ash.Error.Invalid{errors: errors}) do + error_messages = + Enum.map(errors, fn + %{field: field, message: message} -> "#{field}: #{message}" + %{message: message} -> message + _ -> inspect(errors) + end) + + Enum.join(error_messages, ", ") + end + + defp format_destroy_error(error), do: inspect(error) + defp handle_save_error(socket, form) do # Always show a flash message when save fails # Field-level validation errors are displayed in form fields, but flash provides additional feedback diff --git a/lib/mv_web/live/member_live/show.ex b/lib/mv_web/live/member_live/show.ex index 6757646..63349be 100644 --- a/lib/mv_web/live/member_live/show.ex +++ b/lib/mv_web/live/member_live/show.ex @@ -418,6 +418,8 @@ defmodule MvWeb.MemberLive.Show do )} {:error, error} -> + require Logger + Logger.warning("Member delete failed: member_id=#{member.id} error=#{inspect(error)}") {:noreply, put_flash(socket, :error, format_error(error))} end end diff --git a/lib/mv_web/live/role_live/index.ex b/lib/mv_web/live/role_live/index.ex index 2169400..ed64eb7 100644 --- a/lib/mv_web/live/role_live/index.ex +++ b/lib/mv_web/live/role_live/index.ex @@ -5,11 +5,8 @@ defmodule MvWeb.RoleLive.Index do ## Features - List all roles with name, description, permission_set_name, is_system_role - Create new roles - - Navigate to role details and edit forms - - Delete non-system roles - - ## Events - - `delete` - Remove a role from the database (only non-system roles) + - Navigate to role details (row click) and edit from details header + - Delete only via Danger zone on role show page ## Security Only admins can access this page (enforced by authorization). @@ -21,8 +18,7 @@ defmodule MvWeb.RoleLive.Index do require Ash.Query - import MvWeb.RoleLive.Helpers, - only: [format_error: 1, permission_set_badge_class: 1, opts_with_actor: 3] + import MvWeb.RoleLive.Helpers, only: [permission_set_badge_class: 1] @impl true def mount(_params, _session, socket) do @@ -37,83 +33,6 @@ defmodule MvWeb.RoleLive.Index do |> assign(:user_counts, user_counts)} end - @impl true - def handle_event("delete", %{"id" => id}, socket) do - case Authorization.get_role(id, actor: socket.assigns.current_user) do - {:ok, role} -> - handle_delete_role(role, id, socket) - - {:error, %Ash.Error.Query.NotFound{}} -> - {:noreply, - put_flash( - socket, - :error, - gettext("Role not found.") - )} - - {:error, error} -> - error_message = format_error(error) - - {:noreply, - put_flash( - socket, - :error, - gettext("Failed to delete role: %{error}", error: error_message) - )} - end - end - - defp handle_delete_role(role, id, socket) do - if role.is_system_role do - {:noreply, - put_flash( - socket, - :error, - gettext("System roles cannot be deleted.") - )} - else - user_count = recalculate_user_count(role, socket.assigns.current_user) - - if user_count > 0 do - {:noreply, - put_flash( - socket, - :error, - gettext( - "Cannot delete role. %{count} user(s) are still assigned to this role. Please assign them to another role first.", - count: user_count - ) - )} - else - perform_role_deletion(role, id, socket) - end - end - end - - defp perform_role_deletion(role, id, socket) do - case Authorization.destroy_role(role, actor: socket.assigns.current_user) do - :ok -> - updated_roles = Enum.reject(socket.assigns.roles, &(&1.id == id)) - updated_counts = Map.delete(socket.assigns.user_counts, id) - - {:noreply, - socket - |> assign(:roles, updated_roles) - |> assign(:user_counts, updated_counts) - |> put_flash(:success, gettext("Role deleted successfully."))} - - {:error, error} -> - error_message = format_error(error) - - {:noreply, - put_flash( - socket, - :error, - gettext("Failed to delete role: %{error}", error: error_message) - )} - end - end - @spec load_roles(map() | nil) :: [Mv.Authorization.Role.t()] defp load_roles(actor) do opts = MvWeb.LiveHelpers.ash_actor_opts(actor) @@ -154,15 +73,4 @@ defmodule MvWeb.RoleLive.Index do defp get_user_count(role, user_counts) do Map.get(user_counts, role.id, 0) end - - # Recalculates user count for a specific role (used before deletion) - @spec recalculate_user_count(Mv.Authorization.Role.t(), map() | nil) :: non_neg_integer() - defp recalculate_user_count(role, actor) do - opts = opts_with_actor([], actor, Mv.Accounts) - - case Ash.count(Accounts.User |> Ash.Query.filter(role_id == ^role.id), opts) do - {:ok, count} -> count - _ -> 0 - end - end end diff --git a/lib/mv_web/live/role_live/index.html.heex b/lib/mv_web/live/role_live/index.html.heex index 5947472..43f2fc7 100644 --- a/lib/mv_web/live/role_live/index.html.heex +++ b/lib/mv_web/live/role_live/index.html.heex @@ -53,47 +53,5 @@ <:col :let={role} label={gettext("Users")}> {get_user_count(role, @user_counts)} - - <:action :let={role}> -
- <.link navigate={~p"/admin/roles/#{role}"}>{gettext("Show")} -
- - <%= if can?(@current_user, :update, Mv.Authorization.Role) do %> - <.button variant="ghost" size="sm" navigate={~p"/admin/roles/#{role}/edit"}> - <.icon name="hero-pencil" class="size-4" /> - {gettext("Edit role")} - - <% end %> - - - <:action :let={role}> - <%= if can?(@current_user, :destroy, Mv.Authorization.Role) and not role.is_system_role do %> - <.button - variant="danger" - size="sm" - phx-click={JS.push("delete", value: %{id: role.id}) |> hide("#row-#{role.id}")} - data-confirm={gettext("Are you sure?")} - > - <.icon name="hero-trash" class="size-4" /> - {gettext("Delete")} - - <% else %> - <.tooltip - :if={role.is_system_role} - content={gettext("System roles cannot be deleted")} - position="left" - > - - - <% end %> - diff --git a/lib/mv_web/live/role_live/show.ex b/lib/mv_web/live/role_live/show.ex index b5a25ce..8b615b6 100644 --- a/lib/mv_web/live/role_live/show.ex +++ b/lib/mv_web/live/role_live/show.ex @@ -182,15 +182,6 @@ defmodule MvWeb.RoleLive.Show do {gettext("Edit role")} <% end %> - <%= if can?(@current_user, :destroy, Mv.Authorization.Role) and not @role.is_system_role do %> - <.button - variant="danger" - phx-click={JS.push("delete", value: %{id: @role.id})} - data-confirm={gettext("Are you sure?")} - > - <.icon name="hero-trash" /> {gettext("Delete Role")} - - <% end %> @@ -216,6 +207,37 @@ defmodule MvWeb.RoleLive.Show do <% end %> + + <%!-- Danger zone: canonical pattern (same as member show) --%> + <%= if can?(@current_user, :destroy, Mv.Authorization.Role) and not @role.is_system_role do %> +
+

+ {gettext("Danger zone")} +

+
+

+ {gettext( + "Deleting this role cannot be undone. Users assigned to this role must be reassigned first." + )} +

+ <.button + variant="danger" + phx-click={JS.push("delete", value: %{id: @role.id})} + data-confirm={ + gettext( + "Are you sure you want to delete the role %{name}? This action cannot be undone.", + name: @role.name + ) + } + data-testid="role-delete" + aria-label={gettext("Delete role %{name}", name: @role.name)} + > + <.icon name="hero-trash" class="size-4" /> + {gettext("Delete role")} + +
+
+ <% end %> """ end diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index 8ff5966..34defe1 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -39,6 +39,7 @@ defmodule MvWeb.UserLive.Form do import MvWeb.LiveHelpers, only: [current_actor: 1, submit_form: 3] import MvWeb.Authorization, only: [can?: 3] + import MvWeb.ErrorHelpers, only: [format_ash_error: 1] @impl true def render(assigns) do @@ -281,6 +282,38 @@ defmodule MvWeb.UserLive.Form do <% end %> + <%!-- Danger zone: canonical pattern (same as member form) --%> + <%= if @user && can?(@current_user, :destroy, @user) && !Mv.Helpers.SystemActor.system_user?(@user) do %> +
+

+ {gettext("Danger zone")} +

+
+

+ {gettext( + "Deleting this user cannot be undone. The user account and any linked member association will be affected." + )} +

+ <.button + type="button" + variant="danger" + phx-click="delete" + phx-value-id={@user.id} + data-confirm={ + gettext("Are you sure you want to delete the user %{email}? This action cannot be undone.", + email: @user.email + ) + } + data-testid="user-delete" + aria-label={gettext("Delete user %{email}", email: @user.email)} + > + <.icon name="hero-trash" class="size-4" /> + {gettext("Delete user")} + +
+
+ <% end %> +
<.button navigate={return_path(@return_to, @user)} variant="neutral"> {gettext("Cancel")} @@ -404,6 +437,44 @@ defmodule MvWeb.UserLive.Form do end end + @impl true + def handle_event("delete", %{"id" => id}, socket) do + user = socket.assigns.user + actor = current_actor(socket) + + if is_nil(user) do + {:noreply, put_flash(socket, :error, gettext("User not found"))} + else + if to_string(id) != to_string(user.id) do + {:noreply, put_flash(socket, :error, gettext("User not found"))} + else + if Mv.Helpers.SystemActor.system_user?(user) do + {:noreply, + put_flash(socket, :error, gettext("System user cannot be deleted."))} + else + case Ash.destroy(user, domain: Mv.Accounts, actor: actor) do + :ok -> + {:noreply, + socket + |> put_flash(:success, gettext("User deleted successfully")) + |> push_navigate(to: ~p"/users")} + + {:error, %Ash.Error.Forbidden{}} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to delete this user") + )} + + {:error, error} -> + {:noreply, put_flash(socket, :error, format_ash_error(error))} + end + end + end + end + end + @impl true def handle_event("show_member_dropdown", _params, socket) do {:noreply, assign(socket, show_member_dropdown: true)} diff --git a/lib/mv_web/live/user_live/index.ex b/lib/mv_web/live/user_live/index.ex index ba36605..d72c1fd 100644 --- a/lib/mv_web/live/user_live/index.ex +++ b/lib/mv_web/live/user_live/index.ex @@ -5,15 +5,14 @@ defmodule MvWeb.UserLive.Index do ## Features - List all users with email and linked member - Sort users by email (default) - - Delete users - - Navigate to user details and edit forms + - Navigate to user details (row click) and edit from details header + - Delete only via Danger zone on user show/edit - Bulk selection for future batch operations ## Relationships Displays linked member information when a user is connected to a member account. ## Events - - `delete` - Remove a user from the database - `select_user` - Toggle individual user selection - `select_all` - Toggle selection of all visible users @@ -26,7 +25,6 @@ defmodule MvWeb.UserLive.Index do import MvWeb.LiveHelpers, only: [current_actor: 1] require Ash.Query - import MvWeb.ErrorHelpers, only: [format_ash_error: 1] @impl true def mount(_params, _session, socket) do @@ -48,45 +46,6 @@ defmodule MvWeb.UserLive.Index do |> assign(:selected_users, [])} end - @impl true - def handle_event("delete", %{"id" => id}, socket) do - actor = current_actor(socket) - - case Ash.get(Mv.Accounts.User, id, domain: Mv.Accounts, actor: actor) do - {:ok, user} -> - case Ash.destroy(user, domain: Mv.Accounts, actor: actor) do - :ok -> - updated_users = Enum.reject(socket.assigns.users, &(&1.id == id)) - - {:noreply, - socket - |> assign(:users, updated_users) - |> put_flash(:success, gettext("User deleted successfully"))} - - {:error, %Ash.Error.Forbidden{}} -> - {:noreply, - put_flash( - socket, - :error, - gettext("You do not have permission to delete this user") - )} - - {:error, error} -> - {:noreply, put_flash(socket, :error, format_ash_error(error))} - end - - {:error, %Ash.Error.Query.NotFound{}} -> - {:noreply, put_flash(socket, :error, gettext("User not found"))} - - {:error, %Ash.Error.Forbidden{} = _error} -> - {:noreply, - put_flash(socket, :error, gettext("You do not have permission to access this user"))} - - {:error, error} -> - {:noreply, put_flash(socket, :error, format_ash_error(error))} - end - end - # Selects one user in the list of users @impl true def handle_event("select_user", %{"id" => id}, socket) do diff --git a/lib/mv_web/live/user_live/index.html.heex b/lib/mv_web/live/user_live/index.html.heex index 858e784..7ffa0e3 100644 --- a/lib/mv_web/live/user_live/index.html.heex +++ b/lib/mv_web/live/user_live/index.html.heex @@ -84,29 +84,5 @@ — <% end %> - - <:action :let={user}> -
- <.link navigate={~p"/users/#{user}"}>{gettext("Show")} -
- - <%= if can?(@current_user, :update, user) do %> - <.link navigate={~p"/users/#{user}/edit"} data-testid="user-edit"> - {gettext("Edit user")} - - <% end %> - - - <:action :let={user}> - <%= if can?(@current_user, :destroy, user) do %> - <.link - phx-click={JS.push("delete", value: %{id: user.id}) |> hide("#row-#{user.id}")} - data-confirm={gettext("Are you sure?")} - data-testid="user-delete" - > - {gettext("Delete")} - - <% end %> - diff --git a/lib/mv_web/live/user_live/show.ex b/lib/mv_web/live/user_live/show.ex index 3530b36..a77a1c4 100644 --- a/lib/mv_web/live/user_live/show.ex +++ b/lib/mv_web/live/user_live/show.ex @@ -27,6 +27,7 @@ defmodule MvWeb.UserLive.Show do use MvWeb, :live_view import MvWeb.LiveHelpers, only: [current_actor: 1] + import MvWeb.ErrorHelpers, only: [format_ash_error: 1] @impl true def render(assigns) do @@ -80,6 +81,37 @@ defmodule MvWeb.UserLive.Show do <% end %> + + <%!-- Danger zone: canonical pattern (same as member show) --%> + <%= if can?(@current_user, :destroy, @user) and not Mv.Helpers.SystemActor.system_user?(@user) do %> +
+

+ {gettext("Danger zone")} +

+
+

+ {gettext( + "Deleting this user cannot be undone. The user account and any linked member association will be affected." + )} +

+ <.button + variant="danger" + phx-click="delete" + phx-value-id={@user.id} + data-confirm={ + gettext("Are you sure you want to delete the user %{email}? This action cannot be undone.", + email: @user.email + ) + } + data-testid="user-delete" + aria-label={gettext("Delete user %{email}", email: @user.email)} + > + <.icon name="hero-trash" class="size-4" /> + {gettext("Delete user")} + +
+
+ <% end %> """ end @@ -103,4 +135,39 @@ defmodule MvWeb.UserLive.Show do |> assign(:user, user)} end end + + @impl true + def handle_event("delete", %{"id" => id}, socket) do + user = socket.assigns.user + actor = current_actor(socket) + + if to_string(id) != to_string(user.id) do + {:noreply, put_flash(socket, :error, gettext("User not found"))} + else + if Mv.Helpers.SystemActor.system_user?(user) do + {:noreply, + put_flash(socket, :error, gettext("System user cannot be deleted."))} + else + case Ash.destroy(user, domain: Mv.Accounts, actor: actor) do + :ok -> + {:noreply, + socket + |> put_flash(:success, gettext("User deleted successfully")) + |> push_navigate(to: ~p"/users")} + + {:error, %Ash.Error.Forbidden{}} -> + {:noreply, + put_flash( + socket, + :error, + gettext("You do not have permission to delete this user") + )} + + {:error, error} -> + {:noreply, + put_flash(socket, :error, format_ash_error(error))} + end + end + end + end end diff --git a/test/mv_web/member_live/form_error_handling_test.exs b/test/mv_web/member_live/form_error_handling_test.exs index fec7df4..0ec142e 100644 --- a/test/mv_web/member_live/form_error_handling_test.exs +++ b/test/mv_web/member_live/form_error_handling_test.exs @@ -9,6 +9,53 @@ defmodule MvWeb.MemberLive.FormErrorHandlingTest do require Ash.Query + describe "danger zone on edit" do + @tag :ui + test "edit form shows Danger zone and delete button when user can destroy member", %{ + conn: conn + } do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + {:ok, member} = + Mv.Membership.create_member( + %{first_name: "Delete", last_name: "FromEdit", email: "delete.from.edit@example.com"}, + actor: system_actor + ) + + conn = conn_with_oidc_user(conn) + {:ok, view, html} = live(conn, ~p"/members/#{member}/edit") + + assert html =~ gettext("Danger zone") + assert has_element?(view, "[data-testid='member-delete']") + end + + test "delete event from edit form removes member and redirects to /members", %{conn: conn} do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + {:ok, member} = + Mv.Membership.create_member( + %{ + first_name: "ToDelete", + last_name: "FromForm", + email: "todelete.from.form.#{System.unique_integer([:positive])}@example.com" + }, + actor: system_actor + ) + + conn = conn_with_oidc_user(conn) + {:ok, view, _html} = live(conn, ~p"/members/#{member}/edit") + + view + |> render_click("delete", %{"id" => member.id}) + + assert_redirect(view, ~p"/members") + + refute Mv.Membership.Member + |> Ash.Query.filter(id == ^member.id) + |> Ash.exists?() + end + end + describe "tab visibility" do @tag :ui test "Payments tab is not visible on new member form", %{conn: conn} do diff --git a/test/mv_web/member_live/show_test.exs b/test/mv_web/member_live/show_test.exs index 8c7a23a..54829de 100644 --- a/test/mv_web/member_live/show_test.exs +++ b/test/mv_web/member_live/show_test.exs @@ -135,14 +135,16 @@ defmodule MvWeb.MemberLive.ShowTest do end describe "delete action" do - test "renders Delete button when user can destroy member", %{ + test "renders Danger zone section and Delete button when user can destroy member", %{ conn: conn, member: member } do conn = conn_with_oidc_user(conn) - {:ok, view, _html} = live(conn, ~p"/members/#{member}") + {:ok, view, html} = live(conn, ~p"/members/#{member}") assert has_element?(view, "[data-testid='member-delete']") + assert html =~ gettext("Danger zone") + assert has_element?(view, "section[aria-labelledby='danger-zone-heading']") end test "delete event removes member and redirects to index", %{