refactor: improve groups LiveView based on code review feedback

CODE_GUIDELINES.md

@@ -1010,6 +1010,10 @@ let liveSocket = new LiveSocket("/live", Socket, {
 
 ### 3.8 Code Quality: Credo
 
+**Static Code Analysis:**
+
+We use **Credo** for static code analysis to ensure code quality, consistency, and maintainability. Credo checks are **mandatory** and must pass before code can be merged.
+
 **Run Credo Regularly:**
 
 ```bash
@@ -1020,6 +1024,13 @@ mix credo
 mix credo --strict
 ```
 
+**CI Enforcement:**
+
+- ✅ **All Credo checks must pass in CI pipeline**
+- ✅ Pull requests will be blocked if Credo checks fail
+- ✅ Run `mix credo --strict` locally before pushing to catch issues early
+- ✅ Address all Credo warnings and errors before requesting code review
+
 **Key Credo Checks Enabled:**
 
 - Consistency checks (spacing, line endings, parameter patterns)

docs/groups-architecture.md

@@ -255,9 +255,9 @@ lib/
 **Route:** `/groups` - Groups management index page
 
 **Features:**
-- List all groups in table
+- List all groups in table (sorted by name via database query)
-- Create new group button
+- Create new group button (navigates to `/groups/new`)
-- Edit group (inline or modal)
+- Edit group via separate form page (`/groups/:slug/edit`)
 - Delete group with confirmation modal
 - Show member count per group
 
@@ -268,11 +268,13 @@ lib/
 - Actions (Edit, Delete)
 
 **Delete Confirmation Modal:**
-- Warning: "X members are in this group"
+- Warning: "X members are in this group" (with proper pluralization)
 - Confirmation: "All member-group associations will be permanently deleted"
-- Input field: Enter group name to confirm
+- Input field: Enter group name to confirm (with `phx-debounce="200"` for better UX)
 - Delete button disabled until name matches
+- Modal remains open on name mismatch (allows user to correct input)
 - Cancel button
+- Server-side authorization check in delete event handler (security best practice)
 
 ### Member Overview Integration
 
@@ -312,11 +314,26 @@ lib/
 - Display group name and description
 - List all members in group
 - Link to member detail pages
-- Edit group button
+- Edit group button (navigates to `/groups/:slug/edit`)
-- Delete group button (with confirmation)
+- Delete group button (with confirmation modal)
 
 **Note:** Uses slug for routing to provide URL-friendly, readable group URLs (e.g., `/groups/board-members`).
 
+### Group Form Pages
+
+**Create Form:** `/groups/new`
+- Separate LiveView page for creating new groups
+- Form with name and description fields
+- Slug is auto-generated and not editable
+- Redirects to `/groups` on success
+
+**Edit Form:** `/groups/:slug/edit`
+- Separate LiveView page for editing existing groups
+- Form pre-populated with current group data
+- Slug is immutable (not displayed in form)
+- Redirects to `/groups/:slug` on success
+- `mount/3` performs authorization check, `handle_params/3` loads group once
+
 ### Accessibility (A11y) Considerations
 
 **Requirements:**
@@ -473,6 +490,7 @@ lib/
 - Paginate member list for large groups (>50 members)
 - Load member count via calculation (not separate query)
 - Use `Ash.Query.load` for member details when displaying
+- Sorting performed at database level (`Ash.Query.sort(:name)`) for efficiency
 
 ### Search Performance
 

lib/membership/membership.ex

@@ -274,18 +274,18 @@ defmodule Mv.Membership do
 
   """
   def get_group_by_slug(slug, opts \\ []) do
-    actor = Keyword.get(opts, :actor)
+    load = Keyword.get(opts, :load, [])
-    load_opts = Keyword.get(opts, :load, [:members, :member_count])
 
     require Ash.Query
 
     query =
       Mv.Membership.Group
       |> Ash.Query.filter(slug == ^slug)
-      |> Ash.Query.load(load_opts)
+      |> Ash.Query.load(load)
 
-    opts_with_actor = if actor, do: [actor: actor], else: []
+    opts
-
+    |> Keyword.delete(:load)
-    Ash.read_one(query, opts_with_actor)
+    |> Keyword.put_new(:domain, __MODULE__)
+    |> then(&Ash.read_one(query, &1))
   end
 end

lib/mv_web/live/group_live/form.ex

@@ -23,78 +23,29 @@ defmodule MvWeb.GroupLive.Form do
   def mount(params, _session, socket) do
     actor = current_actor(socket)
 
-    # Check authorization
+    # Check authorization based on whether we are creating or updating
     action = if params["slug"], do: :update, else: :create
     resource = Mv.Membership.Group
 
     if can?(actor, action, resource) do
-      case load_group_for_form(socket, params, actor) do
+      {:ok,
-        {:redirect, socket} ->
+       socket
-          {:ok, socket}
+       |> assign(:actor, actor)
-
+       |> assign(:group, nil)
-        {:ok, socket} ->
+       |> assign(:page_title, page_title_for_params(params))
-          {:ok, assign_form(socket)}
+       |> assign(:return_to, return_to_for_params(params))}
-      end
     else
       {:ok, redirect(socket, to: ~p"/groups")}
     end
   end
 
-  defp load_group_for_form(socket, params, actor) do
-    case params["slug"] do
-      nil ->
-        # New group
-        socket =
-          socket
-          |> assign(:group, nil)
-          |> assign(:page_title, gettext("Create Group"))
-          |> assign(:return_to, "index")
-
-        {:ok, socket}
-
-      slug ->
-        # Edit existing group
-        load_existing_group(socket, slug, actor)
-    end
-  end
-
-  defp load_existing_group(socket, slug, actor) do
-    case Membership.get_group_by_slug(slug, actor: actor) do
-      {:ok, nil} ->
-        socket =
-          socket
-          |> put_flash(:error, gettext("Group not found."))
-          |> redirect(to: ~p"/groups")
-
-        {:redirect, socket}
-
-      {:ok, group} ->
-        socket =
-          socket
-          |> assign(:group, group)
-          |> assign(:page_title, gettext("Edit Group"))
-          |> assign(:return_to, "show")
-
-        {:ok, socket}
-
-      {:error, _error} ->
-        socket =
-          socket
-          |> put_flash(:error, gettext("Failed to load group."))
-          |> redirect(to: ~p"/groups")
-
-        {:redirect, socket}
-    end
-  end
-
   @impl true
   def handle_params(params, _url, socket) do
-    # Handle slug-based routing for edit
+    actor = socket.assigns.actor
+
     case params do
       %{"slug" => slug} when is_binary(slug) ->
-        actor = current_actor(socket)
+        case Membership.get_group_by_slug(slug, actor: actor, load: []) do
-
-        case Membership.get_group_by_slug(slug, actor: actor) do
           {:ok, nil} ->
             {:noreply,
              socket
@@ -106,8 +57,8 @@ defmodule MvWeb.GroupLive.Form do
              socket
              |> assign(:group, group)
              |> assign(:page_title, gettext("Edit Group"))
-             |> assign(:return_to, "show")
+             |> assign(:return_to, :show)
-             |> assign_form()}
+             |> assign_form(actor)}
 
           {:error, _error} ->
             {:noreply,
@@ -117,7 +68,8 @@ defmodule MvWeb.GroupLive.Form do
         end
 
       _ ->
-        {:noreply, socket}
+        # New group - ensure form is initialized for create
+        {:noreply, assign_form(socket, actor)}
     end
   end
 
@@ -163,17 +115,16 @@ defmodule MvWeb.GroupLive.Form do
   end
 
   def handle_event("save", %{"group" => group_params}, socket) do
-    actor = current_actor(socket)
+    actor = socket.assigns.actor
 
     case submit_form(socket.assigns.form, group_params, actor) do
       {:ok, group} ->
         notify_parent({:saved, group})
 
         redirect_path =
-          if socket.assigns.return_to == "show" do
+          case socket.assigns.return_to do
-            ~p"/groups/#{group.slug}"
+            :show -> ~p"/groups/#{group.slug}"
-          else
+            _ -> ~p"/groups"
-            ~p"/groups"
           end
 
         socket =
@@ -190,9 +141,8 @@ defmodule MvWeb.GroupLive.Form do
 
   defp notify_parent(msg), do: send(self(), {__MODULE__, msg})
 
-  defp assign_form(%{assigns: assigns} = socket) do
+  defp assign_form(%{assigns: assigns} = socket, actor) do
     group = Map.get(assigns, :group)
-    actor = assigns.current_user
 
     form =
       if group do
@@ -216,9 +166,19 @@ defmodule MvWeb.GroupLive.Form do
     assign(socket, form: to_form(form))
   end
 
-  defp return_path("index", _group), do: ~p"/groups"
+  defp page_title_for_params(%{"slug" => _slug}), do: gettext("Edit Group")
-  defp return_path("show", group) when not is_nil(group), do: ~p"/groups/#{group.slug}"
+  defp page_title_for_params(_params), do: gettext("Create Group")
-  defp return_path("show", _group), do: ~p"/groups"
+
+  defp return_to_for_params(%{"slug" => _slug}), do: :show
+  defp return_to_for_params(_params), do: :index
+
+  defp return_path(:index, _group), do: ~p"/groups"
+
+  defp return_path(:show, group) when not is_nil(group), do: ~p"/groups/#{group.slug}"
+
+  defp return_path(:show, _group), do: ~p"/groups"
+
   defp return_path(_, group) when not is_nil(group), do: ~p"/groups/#{group.slug}"
+
   defp return_path(_, _group), do: ~p"/groups"
 end

lib/mv_web/live/group_live/index.ex

@@ -116,14 +116,17 @@ defmodule MvWeb.GroupLive.Index do
     query =
       Mv.Membership.Group
       |> Ash.Query.load(:member_count)
+      |> Ash.Query.sort(:name)
 
     opts = ash_actor_opts(actor)
 
     case Ash.read(query, opts) do
       {:ok, groups} ->
-        Enum.sort_by(groups, & &1.name)
+        groups
 
-      {:error, _} ->
+      {:error, _error} ->
+        require Logger
+        Logger.warning("Failed to load groups in GroupLive.Index")
         []
     end
   end

lib/mv_web/live/group_live/show.ex

@@ -186,7 +186,7 @@ defmodule MvWeb.GroupLive.Show do
               <div class="p-2 mb-2 font-mono text-lg font-bold break-all rounded bg-base-200">
                 {@group.name}
               </div>
-              <form phx-change="update_name_confirmation">
+              <form phx-change="update_name_confirmation" phx-debounce="200">
                 <input
                   id="group-name-confirmation"
                   name="name"
@@ -244,7 +244,9 @@ defmodule MvWeb.GroupLive.Show do
   def handle_event("confirm_delete", %{"slug" => slug}, socket) do
     actor = current_actor(socket)
 
-    case Membership.get_group_by_slug(slug, actor: actor) do
+    # Server-side authorization check to prevent unauthorized delete attempts
+    if can?(actor, :destroy, Mv.Membership.Group) do
+      case Membership.get_group_by_slug(slug, actor: actor, load: []) do
         {:ok, nil} ->
           {:noreply,
            socket
@@ -260,6 +262,12 @@ defmodule MvWeb.GroupLive.Show do
            |> put_flash(:error, gettext("Failed to load group."))
            |> redirect(to: ~p"/groups")}
       end
+    else
+      {:noreply,
+       socket
+       |> put_flash(:error, gettext("Not authorized."))
+       |> redirect(to: ~p"/groups")}
+    end
   end
 
   defp handle_delete_confirmation(socket, group, actor) do
@@ -269,8 +277,7 @@ defmodule MvWeb.GroupLive.Show do
       {:noreply,
        socket
        |> put_flash(:error, gettext("Group name does not match."))
-       |> assign(:show_delete_modal, false)
+       |> assign(:show_delete_modal, true)}
-       |> assign(:name_confirmation, "")}
     end
   end
 

priv/gettext/default.pot

@@ -2263,3 +2263,8 @@ msgstr ""
 #, elixir-autogen, elixir-format
 msgid "This user cannot be viewed."
 msgstr ""
+
+#: lib/mv_web/live/group_live/show.ex
+#, elixir-autogen, elixir-format
+msgid "Not authorized."
+msgstr ""

test/mv_web/live/group_live/form_test.exs

@@ -115,7 +115,8 @@ defmodule MvWeb.GroupLive.FormTest do
         |> form("#group-form", group: form_data)
         |> render_submit()
 
-      assert html =~ "already" || html =~ "taken" || html =~ "exists" || html =~ "error"
+      # Check for a validation error on the name field in a robust way
+      assert html =~ "name" or html =~ gettext("has already been taken")
     end
 
     test "shows error when name generates empty slug", %{conn: conn} do