diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 4905b79..28c454b 100644 --- a/CODE_GUIDELINES.md +++ b/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) diff --git a/docs/groups-architecture.md b/docs/groups-architecture.md index 860d308..b2316d8 100644 --- a/docs/groups-architecture.md +++ b/docs/groups-architecture.md @@ -255,9 +255,9 @@ lib/ **Route:** `/groups` - Groups management index page **Features:** -- List all groups in table -- Create new group button -- Edit group (inline or modal) +- List all groups in table (sorted by name via database query) +- Create new group button (navigates to `/groups/new`) +- 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 -- Delete group button (with confirmation) +- Edit group button (navigates to `/groups/:slug/edit`) +- 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 diff --git a/lib/membership/membership.ex b/lib/membership/membership.ex index 0f865e8..47d2f0e 100644 --- a/lib/membership/membership.ex +++ b/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_opts = Keyword.get(opts, :load, [:members, :member_count]) + load = Keyword.get(opts, :load, []) 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: [] - - Ash.read_one(query, opts_with_actor) + opts + |> Keyword.delete(:load) + |> Keyword.put_new(:domain, __MODULE__) + |> then(&Ash.read_one(query, &1)) end end diff --git a/lib/mv_web/live/group_live/form.ex b/lib/mv_web/live/group_live/form.ex index aef6fc6..0ffba09 100644 --- a/lib/mv_web/live/group_live/form.ex +++ b/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 - {:redirect, socket} -> - {:ok, socket} - - {:ok, socket} -> - {:ok, assign_form(socket)} - end + {:ok, + socket + |> assign(:actor, actor) + |> assign(:group, nil) + |> assign(:page_title, page_title_for_params(params)) + |> assign(:return_to, return_to_for_params(params))} 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) do + case Membership.get_group_by_slug(slug, actor: actor, load: []) 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_form()} + |> assign(:return_to, :show) + |> 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 - ~p"/groups/#{group.slug}" - else - ~p"/groups" + case socket.assigns.return_to do + :show -> ~p"/groups/#{group.slug}" + _ -> ~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 return_path("show", group) when not is_nil(group), do: ~p"/groups/#{group.slug}" - defp return_path("show", _group), do: ~p"/groups" + defp page_title_for_params(%{"slug" => _slug}), do: gettext("Edit Group") + defp page_title_for_params(_params), do: gettext("Create Group") + + 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 diff --git a/lib/mv_web/live/group_live/index.ex b/lib/mv_web/live/group_live/index.ex index 0122733..b0d428d 100644 --- a/lib/mv_web/live/group_live/index.ex +++ b/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 diff --git a/lib/mv_web/live/group_live/show.ex b/lib/mv_web/live/group_live/show.ex index 676fa50..6b98da6 100644 --- a/lib/mv_web/live/group_live/show.ex +++ b/lib/mv_web/live/group_live/show.ex @@ -186,7 +186,7 @@ defmodule MvWeb.GroupLive.Show do
{@group.name}
-
+ slug}, socket) do actor = current_actor(socket) - case Membership.get_group_by_slug(slug, actor: actor) do - {:ok, nil} -> - {:noreply, - socket - |> put_flash(:error, gettext("Group not found.")) - |> redirect(to: ~p"/groups")} + # 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 + |> put_flash(:error, gettext("Group not found.")) + |> redirect(to: ~p"/groups")} - {:ok, group} -> - handle_delete_confirmation(socket, group, actor) + {:ok, group} -> + handle_delete_confirmation(socket, group, actor) - {:error, _error} -> - {:noreply, - socket - |> put_flash(:error, gettext("Failed to load group.")) - |> redirect(to: ~p"/groups")} + {:error, _error} -> + {:noreply, + socket + |> 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 @@ -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(:name_confirmation, "")} + |> assign(:show_delete_modal, true)} end end diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index b59085f..13b488c 100644 --- a/priv/gettext/default.pot +++ b/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 "" diff --git a/test/mv_web/live/group_live/form_test.exs b/test/mv_web/live/group_live/form_test.exs index 7da8da4..9169dfe 100644 --- a/test/mv_web/live/group_live/form_test.exs +++ b/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