refactor: fix review issues - member_count aggregate, migration down, docs, actor handling

lib/membership/group.ex

@@ -67,12 +67,12 @@ defmodule Mv.Membership.Group do
     validate present(:name)
 
     # Case-insensitive name uniqueness validation
-    validate fn changeset, _context ->
+    validate fn changeset, context ->
       name = Ash.Changeset.get_attribute(changeset, :name)
       current_id = Ash.Changeset.get_attribute(changeset, :id)
 
       if name do
-        check_name_uniqueness(name, current_id)
+        check_name_uniqueness(name, current_id, context)
       else
         :ok
       end
@@ -115,24 +115,8 @@ defmodule Mv.Membership.Group do
     many_to_many :members, Mv.Membership.Member, through: Mv.Membership.MemberGroup
   end
 
-  calculations do
-    calculate :member_count, :integer do
-      description "Number of members in this group"
-
-      calculation fn [group], _context ->
-        system_actor = SystemActor.get_system_actor()
-        opts = Helpers.ash_actor_opts(system_actor)
-
-        query =
-          Mv.Membership.MemberGroup
-          |> Ash.Query.filter(group_id == ^group.id)
-
-        case Ash.read(query, opts) do
-          {:ok, member_groups} -> [length(member_groups)]
-          {:error, _} -> [0]
-        end
-      end
-    end
+  aggregates do
+    count :member_count, :member_groups
   end
 
   identities do
@@ -140,14 +124,21 @@ defmodule Mv.Membership.Group do
   end
 
   # Private helper function for case-insensitive name uniqueness check
-  defp check_name_uniqueness(name, exclude_id) do
+  # Uses context actor if available (respects policies), falls back to system actor
+  defp check_name_uniqueness(name, exclude_id, context) do
+    # Use context actor if available (respects user permissions), otherwise fall back to system actor
+    actor =
+      case context do
+        %{actor: actor} when not is_nil(actor) -> actor
+        _ -> SystemActor.get_system_actor()
+      end
+
     query =
       Mv.Membership.Group
       |> Ash.Query.filter(fragment("LOWER(?) = LOWER(?)", name, ^name))
       |> maybe_exclude_id(exclude_id)
 
-    system_actor = SystemActor.get_system_actor()
-    opts = Helpers.ash_actor_opts(system_actor)
+    opts = Helpers.ash_actor_opts(actor)
 
     case Ash.read(query, opts) do
       {:ok, []} ->

lib/membership/member_group.ex

@@ -24,11 +24,18 @@ defmodule Mv.Membership.MemberGroup do
 
   ## Examples
       # Add member to group
-      MemberGroup.create!(%{member_id: member.id, group_id: group.id})
+      {:ok, member_group} =
+        Membership.create_member_group(%{member_id: member.id, group_id: group.id})
 
       # Remove member from group
-      member_group = MemberGroup.get_by_member_and_group!(member.id, group.id)
-      MemberGroup.destroy!(member_group)
+      {:ok, [member_group]} =
+        Ash.read(
+          Mv.Membership.MemberGroup
+          |> Ash.Query.filter(member_id == ^member.id and group_id == ^group.id),
+          domain: Mv.Membership
+        )
+
+      :ok = Membership.destroy_member_group(member_group)
   """
   use Ash.Resource,
     domain: Mv.Membership,
@@ -54,13 +61,13 @@ defmodule Mv.Membership.MemberGroup do
     validate present(:group_id)
 
     # Prevent duplicate associations
-    validate fn changeset, _context ->
+    validate fn changeset, context ->
       member_id = Ash.Changeset.get_attribute(changeset, :member_id)
       group_id = Ash.Changeset.get_attribute(changeset, :group_id)
       current_id = Ash.Changeset.get_attribute(changeset, :id)
 
       if member_id && group_id do
-        check_duplicate_association(member_id, group_id, current_id)
+        check_duplicate_association(member_id, group_id, current_id, context)
       else
         :ok
       end
@@ -96,17 +103,24 @@ defmodule Mv.Membership.MemberGroup do
   end
 
   # Private helper function to check for duplicate associations
-  defp check_duplicate_association(member_id, group_id, exclude_id) do
+  # Uses context actor if available (respects policies), falls back to system actor
+  defp check_duplicate_association(member_id, group_id, exclude_id, context) do
     alias Mv.Helpers
     alias Mv.Helpers.SystemActor
 
+    # Use context actor if available (respects user permissions), otherwise fall back to system actor
+    actor =
+      case context do
+        %{actor: actor} when not is_nil(actor) -> actor
+        _ -> SystemActor.get_system_actor()
+      end
+
     query =
       Mv.Membership.MemberGroup
       |> Ash.Query.filter(member_id == ^member_id and group_id == ^group_id)
       |> maybe_exclude_id(exclude_id)
 
-    system_actor = SystemActor.get_system_actor()
-    opts = Helpers.ash_actor_opts(system_actor)
+    opts = Helpers.ash_actor_opts(actor)
 
     case Ash.read(query, opts) do
       {:ok, []} ->

priv/repo/migrations/20260127141620_add_groups_and_member_groups.exs

@@ -82,7 +82,7 @@ defmodule Mv.Repo.Migrations.AddGroupsAndMemberGroups do
   end
 
   def down do
-    execute("DROP INDEX IF EXISTS groups_unique_name_lower_index", "")
+    execute("DROP INDEX IF EXISTS groups_unique_name_lower_index")
 
     drop_if_exists unique_index(:groups, [:slug], name: "groups_unique_slug_index")