From 2ebf28911260fce62e10e61f158145ab32cd83d5 Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 27 Jan 2026 13:41:25 +0100 Subject: [PATCH 1/6] docs: add slugs to group concept #371 --- docs/groups-architecture.md | 91 +++++++++++++++++++++++++++---------- 1 file changed, 68 insertions(+), 23 deletions(-) diff --git a/docs/groups-architecture.md b/docs/groups-architecture.md index c401e2a..66c663d 100644 --- a/docs/groups-architecture.md +++ b/docs/groups-architecture.md @@ -49,7 +49,8 @@ This document defines the technical architecture for the Groups feature. It focu - No parent/child relationships in MVP 3. **Minimal Attributes (MVP):** - - Only `name` and `description` in initial version + - `name`, `description`, and `slug` in initial version + - `slug` is automatically generated from `name` (immutable, URL-friendly) - Extensible for future attributes (dates, status, etc.) 4. **Cascade Deletion:** @@ -71,7 +72,7 @@ This document defines the technical architecture for the Groups feature. It focu **New Resources:** -- `Group` - Group definitions (name, description) +- `Group` - Group definitions (name, description, slug) - `MemberGroup` - Join table for many-to-many relationship between Members and Groups **Extended Resources:** @@ -113,13 +114,17 @@ lib/ **Attributes:** - `id` - UUID v7 primary key - `name` - Unique group name (required, max 100 chars) +- `slug` - URL-friendly identifier (required, max 100 chars, auto-generated from name) - `description` - Optional description (max 500 chars) - `inserted_at` / `updated_at` - Timestamps **Constraints:** - `name` must be unique (case-insensitive, using LOWER(name)) +- `slug` must be unique (case-sensitive, exact match) - `name` cannot be null +- `slug` cannot be null - `name` max length: 100 characters +- `slug` max length: 100 characters - `description` max length: 500 characters #### `member_groups` Table (Join Table) @@ -151,15 +156,19 @@ lib/ - `member_count` - Integer calculation counting associated members **Actions:** -- `create` - Create new group -- `read` - List/search groups -- `update` - Update group name/description +- `create` - Create new group (auto-generates slug from name) +- `read` - List/search groups (can query by slug via identity) +- `update` - Update group name/description (slug remains unchanged) - `destroy` - Delete group (with confirmation) **Validations:** - `name` required, unique (case-insensitive), max 100 chars +- `slug` required, unique (case-sensitive), max 100 chars, auto-generated, immutable - `description` optional, max 500 chars +**Identities:** +- `unique_slug` - Unique identity on `slug` for efficient lookups + #### `Mv.Membership.MemberGroup` **Relationships:** @@ -192,12 +201,14 @@ lib/ **Create Group:** - Validate name uniqueness -- Generate slug (if needed for future URL-friendly identifiers) +- Automatically generate slug from name (using `GenerateSlug` change, same pattern as CustomFields) +- Validate slug uniqueness - Return created group **Update Group:** - Validate name uniqueness (if name changed) - Update description +- Slug remains unchanged (immutable after creation) - Return updated group **Delete Group:** @@ -295,7 +306,7 @@ lib/ ### Group Detail View (`/groups/:id`) -**Route:** `/groups/:id` - Group detail page +**Route:** `/groups/:id` - Group detail page (uses UUID, slug can be used for future `/groups/:slug` routes) **Features:** - Display group name and description @@ -304,6 +315,8 @@ lib/ - Edit group button - Delete group button (with confirmation) +**Note:** Currently uses UUID for routing. Slug is available for future URL-friendly routes (`/groups/:slug`). + ### Accessibility (A11y) Considerations **Requirements:** @@ -431,7 +444,8 @@ lib/ ### Database Indexes **Critical Indexes:** -- `groups.name` - For uniqueness and search +- `groups.name` - For uniqueness and search (case-insensitive via LOWER) +- `groups.slug` - For uniqueness and efficient lookups (unique index) - `member_groups.member_id` - For member → groups queries - `member_groups.group_id` - For group → members queries - Composite index on `(member_id, group_id)` - For uniqueness check @@ -440,7 +454,7 @@ lib/ **Member Overview:** - Load groups with members in single query using query preloading -- Preload only necessary group attributes (id, name) to minimize data transfer +- Preload only necessary group attributes (id, name, slug) to minimize data transfer - Filter groups at database level when filtering by group **N+1 Query Prevention:** @@ -531,7 +545,7 @@ The Groups feature is divided into **functionally complete, vertical units**. Ea **Minimal Viable Product (MVP):** The MVP includes the **core functionality** necessary to manage groups and assign them to members: -1. ✅ Create groups (Name + Description) +1. ✅ Create groups (Name + Description + Slug) 2. ✅ Edit groups 3. ✅ Delete groups (with confirmation) 4. ✅ Assign members to groups @@ -554,9 +568,10 @@ The MVP includes the **core functionality** necessary to manage groups and assig **Functional Scope:** Administrators can manage groups in the system **Scope:** -- Group resource (Name, Description) +- Group resource (Name, Description, Slug) - CRUD operations for groups -- Validations (unique name, length limits) +- Validations (unique name, unique slug, length limits) +- Automatic slug generation from name - Delete logic with cascade behavior **Deliverable:** Groups can be created, edited, and deleted via API @@ -731,12 +746,13 @@ Each functional unit can be implemented as a **separate issue**: **Goal:** Basic group management and member assignment **Tasks:** -1. Create `Group` resource (name, description) -2. Create `MemberGroup` join table resource -3. Extend `Member` with groups relationship -4. Database migrations -5. Basic CRUD actions for groups -6. Add/remove members from groups (via group management) +1. Create `Group` resource (name, description, slug) +2. Implement slug generation (reuse `GenerateSlug` change from CustomFields) +3. Create `MemberGroup` join table resource +4. Extend `Member` with groups relationship +5. Database migrations (including slug column and unique index) +6. Basic CRUD actions for groups +7. Add/remove members from groups (via group management) **Deliverables:** - Groups can be created, edited, deleted @@ -841,16 +857,18 @@ Each functional unit can be implemented as a **separate issue**: **Type:** Backend **Estimation:** 4-5h **Tasks:** -- Create `Group` resource +- Create `Group` resource (with slug attribute and generation) - Create `MemberGroup` join table resource - Extend `Member` resource -- Database migrations -- Basic validations +- Database migrations (including slug column) +- Basic validations (name, slug, description) **Acceptance Criteria:** - Groups can be created via Ash API +- Slug is automatically generated from name +- Slug is unique and immutable - Members can be associated with groups -- Database constraints enforced +- Database constraints enforced (unique name, unique slug, foreign keys) ### Issue 2: Groups Management UI **Type:** Frontend @@ -941,8 +959,19 @@ Each functional unit can be implemented as a **separate issue**: - Allow description to be nil - Trim whitespace from name - Description max length is 500 characters +- Slug is automatically generated from name on create +- Slug is immutable (doesn't change on update) +- Slug is unique (cannot have duplicates) +- Slug handles German umlauts correctly (ä → a, ß → ss) +- Slug converts to lowercase +- Slug removes special characters +- Slug replaces spaces with hyphens +- Slug truncates to max 100 characters +- Slug cannot be empty (validation fails if slug would be empty) +- Read group by slug (via identity lookup) - Update group name and description - Prevent duplicate name on update +- Prevent duplicate slug on create (same name → same slug → error) - Delete group and all member associations - Do not delete members themselves - Member count calculation returns 0 for empty group @@ -997,11 +1026,18 @@ Each functional unit can be implemented as a **separate issue**: **Migration 1: Create groups table** - Create table with UUID v7 primary key - Add name field (required, unique, case-insensitive) +- Add slug field (required, unique, case-sensitive, auto-generated) - Add description field (optional) - Add timestamps -- Create unique index on lowercased name +- Create unique index on lowercased name (for name uniqueness) +- Create unique index on slug (for slug uniqueness and lookups) - Create index on lowercased name for search +**Note:** Slug generation follows the same pattern as CustomFields: +- Uses `Mv.Membership.CustomField.Changes.GenerateSlug` (reusable change) +- Or create `Mv.Membership.Group.Changes.GenerateSlug` if needed +- Slug is generated on create, immutable on update + **Migration 2: Create member_groups join table** - Create table with UUID v7 primary key - Add member_id and group_id foreign keys @@ -1030,4 +1066,13 @@ Each functional unit can be implemented as a **separate issue**: This architecture provides a solid foundation for the Groups feature while maintaining flexibility for future enhancements. The many-to-many relationship is implemented via a join table, following existing patterns in the codebase. The MVP focuses on core functionality (create, edit, delete groups, assign members) with clear extension points for hierarchical groups, roles, and advanced permissions. +**Slug Implementation:** +Groups include automatic slug generation, following the same pattern as CustomFields. Slugs are: +- Automatically generated from the `name` attribute on create +- Immutable after creation (don't change when name is updated) +- Unique and URL-friendly +- Available for future route enhancements (e.g., `/groups/:slug` instead of `/groups/:id`) + +The implementation reuses the existing `GenerateSlug` change from CustomFields, ensuring consistency across the codebase. + The implementation is split into 6 manageable issues, totaling approximately 15 hours of work, aligning with the original estimation. Each phase builds on the previous one, allowing for incremental development and testing. From 0216dfcbbb48c4ccd118f19acfa066ecae2064a4 Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 27 Jan 2026 15:04:26 +0100 Subject: [PATCH 2/6] test: add tests for group resource #371 --- .../group_database_constraints_test.exs | 141 +++++++++ test/membership/group_integration_test.exs | 141 +++++++++ test/membership/group_test.exs | 284 ++++++++++++++++++ test/membership/member_group_test.exs | 116 +++++++ .../member_groups_relationship_test.exs | 197 ++++++++++++ 5 files changed, 879 insertions(+) create mode 100644 test/membership/group_database_constraints_test.exs create mode 100644 test/membership/group_integration_test.exs create mode 100644 test/membership/group_test.exs create mode 100644 test/membership/member_group_test.exs create mode 100644 test/membership/member_groups_relationship_test.exs diff --git a/test/membership/group_database_constraints_test.exs b/test/membership/group_database_constraints_test.exs new file mode 100644 index 0000000..4418b33 --- /dev/null +++ b/test/membership/group_database_constraints_test.exs @@ -0,0 +1,141 @@ +defmodule Mv.Membership.GroupDatabaseConstraintsTest do + @moduledoc """ + Tests for database-level constraints (unique, foreign keys, CASCADE). + These tests verify that constraints are enforced at the database level, not just application level. + """ + use Mv.DataCase, async: false + + alias Mv.Membership + + require Ash.Query + import Ash.Expr + + setup do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + %{actor: system_actor} + end + + describe "Unique Constraints" do + test "database enforces unique name constraint (case-insensitive via LOWER)", %{actor: actor} do + {:ok, _group1} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + # Try to create with same name, different case - should fail at DB level + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_group(%{name: "TEST GROUP"}, actor: actor) + + assert Enum.any?(errors, fn err -> + err.field == :name and + (String.contains?(err.message, "already been taken") or + String.contains?(err.message, "already exists") or + String.contains?(err.message, "duplicate")) + end) + end + + test "database enforces unique slug constraint (case-sensitive)", %{actor: actor} do + {:ok, _group1} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + # Try to create with name that generates same slug - should fail at DB level + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_group(%{name: "test-group"}, actor: actor) + + assert Enum.any?(errors, fn err -> + (err.field == :slug or err.field == :name) and + (String.contains?(err.message, "already been taken") or + String.contains?(err.message, "already exists") or + String.contains?(err.message, "duplicate")) + end) + end + end + + describe "Foreign Key Constraints" do + test "cannot create MemberGroup with non-existent member_id", %{actor: actor} do + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + fake_member_id = Ash.UUID.generate() + + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_member_group(%{member_id: fake_member_id, group_id: group.id}, + actor: actor + ) + + assert Enum.any?(errors, fn err -> + (err.field == :member_id or err.field == :member) and + (String.contains?(err.message, "does not exist") or + String.contains?(err.message, "not found") or + String.contains?(err.message, "foreign key")) + end) + end + + test "cannot create MemberGroup with non-existent group_id", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + fake_group_id = Ash.UUID.generate() + + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_member_group(%{member_id: member.id, group_id: fake_group_id}, + actor: actor + ) + + assert Enum.any?(errors, fn err -> + (err.field == :group_id or err.field == :group) and + (String.contains?(err.message, "does not exist") or + String.contains?(err.message, "not found") or + String.contains?(err.message, "foreign key")) + end) + end + end + + describe "CASCADE Delete Constraints" do + test "deleting member cascades to member_groups (verified at DB level)", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + {:ok, member_group} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Verify association exists + assert member_group.member_id == member.id + + # Delete member + :ok = Membership.destroy_member(member, actor: actor) + + # Verify MemberGroup is deleted at DB level (CASCADE) + {:ok, mgs} = + Ash.read( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(id == ^member_group.id)), + actor: actor, + domain: Mv.Membership + ) + + assert mgs == [] + end + + test "deleting group cascades to member_groups (verified at DB level)", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + {:ok, member_group} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Verify association exists + assert member_group.group_id == group.id + + # Delete group + :ok = Membership.destroy_group(group, actor: actor) + + # Verify MemberGroup is deleted at DB level (CASCADE) + {:ok, mgs} = + Ash.read( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(id == ^member_group.id)), + actor: actor, + domain: Mv.Membership + ) + + assert mgs == [] + end + end +end diff --git a/test/membership/group_integration_test.exs b/test/membership/group_integration_test.exs new file mode 100644 index 0000000..7664660 --- /dev/null +++ b/test/membership/group_integration_test.exs @@ -0,0 +1,141 @@ +defmodule Mv.Membership.GroupIntegrationTest do + @moduledoc """ + Integration tests for many-to-many relationships and query performance. + """ + use Mv.DataCase, async: false + + alias Mv.Membership + + require Ash.Query + import Ash.Expr + + setup do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + %{actor: system_actor} + end + + describe "Many-to-Many Relationship" do + test "member can belong to multiple groups", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group1} = Membership.create_group(%{name: "Group One"}, actor: actor) + {:ok, group2} = Membership.create_group(%{name: "Group Two"}, actor: actor) + {:ok, group3} = Membership.create_group(%{name: "Group Three"}, actor: actor) + + # Add member to all groups + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member.id, group_id: group1.id}, + actor: actor + ) + + {:ok, _mg2} = + Membership.create_member_group(%{member_id: member.id, group_id: group2.id}, + actor: actor + ) + + {:ok, _mg3} = + Membership.create_member_group(%{member_id: member.id, group_id: group3.id}, + actor: actor + ) + + # Load member with groups + {:ok, member_with_groups} = + Ash.load(member, :groups, actor: actor, domain: Mv.Membership) + + assert length(member_with_groups.groups) == 3 + assert Enum.any?(member_with_groups.groups, &(&1.id == group1.id)) + assert Enum.any?(member_with_groups.groups, &(&1.id == group2.id)) + assert Enum.any?(member_with_groups.groups, &(&1.id == group3.id)) + end + + test "group can contain multiple members", %{actor: actor} do + {:ok, member1} = Membership.create_member(%{email: "member1@test.com"}, actor: actor) + {:ok, member2} = Membership.create_member(%{email: "member2@test.com"}, actor: actor) + {:ok, member3} = Membership.create_member(%{email: "member3@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + # Add all members to group + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member1.id, group_id: group.id}, + actor: actor + ) + + {:ok, _mg2} = + Membership.create_member_group(%{member_id: member2.id, group_id: group.id}, + actor: actor + ) + + {:ok, _mg3} = + Membership.create_member_group(%{member_id: member3.id, group_id: group.id}, + actor: actor + ) + + # Load group with members + {:ok, group_with_members} = + Ash.load(group, :members, actor: actor, domain: Mv.Membership) + + assert length(group_with_members.members) == 3 + assert Enum.any?(group_with_members.members, &(&1.id == member1.id)) + assert Enum.any?(group_with_members.members, &(&1.id == member2.id)) + assert Enum.any?(group_with_members.members, &(&1.id == member3.id)) + end + end + + describe "Query Performance" do + test "preloading groups with members avoids N+1 queries", %{actor: actor} do + # Create test data + {:ok, member1} = Membership.create_member(%{email: "member1@test.com"}, actor: actor) + {:ok, member2} = Membership.create_member(%{email: "member2@test.com"}, actor: actor) + {:ok, group1} = Membership.create_group(%{name: "Group One"}, actor: actor) + {:ok, group2} = Membership.create_group(%{name: "Group Two"}, actor: actor) + + # Create associations + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member1.id, group_id: group1.id}, + actor: actor + ) + + {:ok, _mg2} = + Membership.create_member_group(%{member_id: member1.id, group_id: group2.id}, + actor: actor + ) + + {:ok, _mg3} = + Membership.create_member_group(%{member_id: member2.id, group_id: group1.id}, + actor: actor + ) + + # Count queries using Telemetry + query_count = Agent.start_link(fn -> 0 end) |> elem(1) + + handler = fn _event, _measurements, _metadata, _config -> + Agent.update(query_count, &(&1 + 1)) + end + + :telemetry.attach("test-query-counter", [:ash, :query, :start], handler, nil) + + # Load all members with groups preloaded (should be efficient with JOIN) + {:ok, members} = + Ash.read(Mv.Membership.Member, actor: actor, domain: Mv.Membership, load: [:groups]) + + final_count = Agent.get(query_count, & &1) + :telemetry.detach("test-query-counter") + + member1_loaded = Enum.find(members, &(&1.id == member1.id)) + member2_loaded = Enum.find(members, &(&1.id == member2.id)) + + # Verify preloading worked + assert length(member1_loaded.groups) == 2 + assert length(member2_loaded.groups) == 1 + + # Verify groups are correctly associated + assert Enum.any?(member1_loaded.groups, &(&1.id == group1.id)) + assert Enum.any?(member1_loaded.groups, &(&1.id == group2.id)) + assert Enum.any?(member2_loaded.groups, &(&1.id == group1.id)) + + # Verify query count is reasonable (should be 2 queries: one for members, one for groups) + # Note: Exact count may vary based on Ash implementation, but should be much less than N+1 + assert final_count <= 3, + "Expected max 3 queries (members + groups + possible count), got #{final_count}. This suggests N+1 query problem." + end + end +end diff --git a/test/membership/group_test.exs b/test/membership/group_test.exs new file mode 100644 index 0000000..a9e058a --- /dev/null +++ b/test/membership/group_test.exs @@ -0,0 +1,284 @@ +defmodule Mv.Membership.GroupTest do + @moduledoc """ + Tests for Group resource validations, CRUD operations, and relationships. + """ + use Mv.DataCase, async: true + + alias Mv.Membership + + require Ash.Query + import Ash.Expr + + setup do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + %{actor: system_actor} + end + + describe "Validations - Name & Description" do + @valid_attrs %{ + name: "Test Group", + description: "Test description" + } + + test "create group with valid attributes", %{actor: actor} do + assert {:ok, group} = Membership.create_group(@valid_attrs, actor: actor) + assert group.name == "Test Group" + assert group.description == "Test description" + assert group.slug != nil + end + + test "create group with name only (description nil)", %{actor: actor} do + attrs = Map.delete(@valid_attrs, :description) + assert {:ok, group} = Membership.create_group(attrs, actor: actor) + assert group.name == "Test Group" + assert group.description == nil + end + + test "return error when name is missing", %{actor: actor} do + attrs = Map.delete(@valid_attrs, :name) + + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_group(attrs, actor: actor) + + assert error_message(errors, :name) =~ "must be present" + end + + test "return error when name exceeds 100 characters", %{actor: actor} do + long_name = String.duplicate("a", 101) + attrs = Map.put(@valid_attrs, :name, long_name) + + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_group(attrs, actor: actor) + + assert error_message(errors, :name) =~ "must be at most 100" + end + + test "return error when name is not unique (case-insensitive) - application level validation", + %{ + actor: actor + } do + {:ok, _group1} = Membership.create_group(@valid_attrs, actor: actor) + + # Try to create with same name, different case + # This tests application-level validation (Ash validations) + attrs2 = Map.put(@valid_attrs, :name, "TEST GROUP") + + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_group(attrs2, actor: actor) + + error_msg = error_message(errors, :name) + assert error_msg =~ "already been taken" || error_msg =~ "already exists" + end + + test "description max length is 500 characters", %{actor: actor} do + long_description = String.duplicate("a", 501) + attrs = Map.put(@valid_attrs, :description, long_description) + + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_group(attrs, actor: actor) + + assert error_message(errors, :description) =~ "must be at most 500" + end + end + + describe "Slug Generation & Validation" do + test "slug is automatically generated from name on create", %{actor: actor} do + {:ok, group} = + Membership.create_group(%{name: "Test Group Name"}, actor: actor) + + assert group.slug == "test-group-name" + end + + test "slug is unique (prevents duplicate slugs from different names)", %{actor: actor} do + {:ok, _group1} = + Membership.create_group(%{name: "Test!!!"}, actor: actor) + + # Second group with name that generates same slug should fail + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_group(%{name: "Test???"}, actor: actor) + + assert Enum.any?(errors, fn err -> + (err.field == :slug or err.field == :name) and + (String.contains?(err.message, "already been taken") or + String.contains?(err.message, "already exists")) + end) + end + + test "slug is immutable (doesn't change when name is updated)", %{actor: actor} do + {:ok, group} = + Membership.create_group(%{name: "Original Name"}, actor: actor) + + original_slug = group.slug + assert original_slug == "original-name" + + {:ok, updated_group} = + Membership.update_group(group, %{name: "New Different Name"}, actor: actor) + + assert updated_group.slug == original_slug + assert updated_group.name == "New Different Name" + end + + test "slug cannot be empty (rejects name with only special characters)", %{actor: actor} do + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_group(%{name: "!!!"}, actor: actor) + + assert Enum.any?(errors, fn err -> + (err.field == :slug or err.field == :name) and + (String.contains?(err.message, "cannot be empty") or + String.contains?(err.message, "is required")) + end) + end + end + + describe "CRUD Operations" do + test "create group with name and description", %{actor: actor} do + attrs = %{name: "New Group", description: "Description"} + + assert {:ok, group} = Membership.create_group(attrs, actor: actor) + assert group.name == "New Group" + assert group.description == "Description" + end + + test "update group name (slug remains unchanged)", %{actor: actor} do + {:ok, group} = Membership.create_group(%{name: "Original"}, actor: actor) + original_slug = group.slug + + {:ok, updated} = Membership.update_group(group, %{name: "Updated"}, actor: actor) + + assert updated.name == "Updated" + assert updated.slug == original_slug + end + + test "update group description", %{actor: actor} do + {:ok, group} = + Membership.create_group(%{name: "Test", description: "Old"}, actor: actor) + + {:ok, updated} = + Membership.update_group(group, %{description: "New Description"}, actor: actor) + + assert updated.description == "New Description" + end + + test "prevent duplicate name on update (case-insensitive)", %{actor: actor} do + {:ok, _group1} = Membership.create_group(%{name: "Group One"}, actor: actor) + {:ok, group2} = Membership.create_group(%{name: "Group Two"}, actor: actor) + + # Try to update group2 with group1's name (different case) + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.update_group(group2, %{name: "GROUP ONE"}, actor: actor) + + error_msg = error_message(errors, :name) + assert error_msg =~ "already been taken" || error_msg =~ "already exists" + end + end + + describe "Calculations" do + test "member count calculation returns 0 for empty group", %{actor: actor} do + {:ok, group} = Membership.create_group(%{name: "Empty Group"}, actor: actor) + + # Load with calculation + {:ok, group_with_count} = + Ash.load(group, :member_count, actor: actor, domain: Mv.Membership) + + assert group_with_count.member_count == 0 + end + + test "member count calculation returns correct count when members added/removed", %{ + actor: actor + } do + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + {:ok, member1} = Membership.create_member(%{email: "member1@test.com"}, actor: actor) + {:ok, member2} = Membership.create_member(%{email: "member2@test.com"}, actor: actor) + + # Add members to group + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member1.id, group_id: group.id}, + actor: actor + ) + + {:ok, _mg2} = + Membership.create_member_group(%{member_id: member2.id, group_id: group.id}, + actor: actor + ) + + # Check count + {:ok, group_with_count} = + Ash.load(group, :member_count, actor: actor, domain: Mv.Membership) + + assert group_with_count.member_count == 2 + + # Remove one member + {:ok, mg_to_delete} = + Ash.read_one( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(member_id == ^member1.id and group_id == ^group.id)), + actor: actor, + domain: Mv.Membership + ) + + :ok = Membership.destroy_member_group(mg_to_delete, actor: actor) + + # Check count again + {:ok, group_with_count_updated} = + Ash.load(group, :member_count, actor: actor, domain: Mv.Membership) + + assert group_with_count_updated.member_count == 1 + end + end + + describe "Relationships & Deletion" do + test "group has many_to_many members relationship (load with preloading)", %{actor: actor} do + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + + {:ok, _mg} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Load group with members + {:ok, group_with_members} = + Ash.load(group, :members, actor: actor, domain: Mv.Membership) + + assert length(group_with_members.members) == 1 + assert hd(group_with_members.members).id == member.id + end + + test "delete group cascades to member_groups (members remain intact)", %{actor: actor} do + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + + {:ok, _mg} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Delete group + :ok = Membership.destroy_group(group, actor: actor) + + # Member should still exist + {:ok, member_reloaded} = Ash.get(Mv.Membership.Member, member.id, actor: actor) + assert member_reloaded != nil + + # MemberGroup should be deleted + {:ok, mgs} = + Ash.read( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(group_id == ^group.id)), + actor: actor, + domain: Mv.Membership + ) + + assert mgs == [] + end + end + + # Helper function for error evaluation + # Returns the error message for a given field, or empty string if not found + defp error_message(errors, field) do + case Enum.find(errors, fn err -> Map.get(err, :field) == field end) do + nil -> "" + err -> Map.get(err, :message, "") + end + end +end diff --git a/test/membership/member_group_test.exs b/test/membership/member_group_test.exs new file mode 100644 index 0000000..54f9ff9 --- /dev/null +++ b/test/membership/member_group_test.exs @@ -0,0 +1,116 @@ +defmodule Mv.Membership.MemberGroupTest do + @moduledoc """ + Tests for MemberGroup join table resource - validations and cascade delete behavior. + """ + use Mv.DataCase, async: true + + alias Mv.Membership + + require Ash.Query + import Ash.Expr + + setup do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + %{actor: system_actor} + end + + describe "Validations & Associations" do + test "create association between member and group", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + assert {:ok, member_group} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + assert member_group.member_id == member.id + assert member_group.group_id == group.id + end + + test "prevent duplicate associations (same member + same group)", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Try to create duplicate + assert {:error, %Ash.Error.Invalid{errors: errors}} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + assert Enum.any?(errors, fn err -> + ((err.field == :member_id or err.field == :group_id) and + String.contains?(err.message, "already been taken")) or + String.contains?(err.message, "already exists") or + String.contains?(err.message, "duplicate") + end) + end + end + + describe "Cascade Delete Behavior" do + test "cascade delete when member deleted (MemberGroup deleted, Group remains)", %{ + actor: actor + } do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + {:ok, _mg} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Delete member + :ok = Membership.destroy_member(member, actor: actor) + + # Group should still exist + {:ok, group_reloaded} = Ash.get(Mv.Membership.Group, group.id, actor: actor) + assert group_reloaded != nil + + # MemberGroup should be deleted + {:ok, mgs} = + Ash.read( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(member_id == ^member.id)), + actor: actor, + domain: Mv.Membership + ) + + assert mgs == [] + end + + test "cascade delete when group deleted (MemberGroup deleted, Member remains)", %{ + actor: actor + } do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + {:ok, _mg} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Delete group + :ok = Membership.destroy_group(group, actor: actor) + + # Member should still exist + {:ok, member_reloaded} = Ash.get(Mv.Membership.Member, member.id, actor: actor) + assert member_reloaded != nil + + # MemberGroup should be deleted + {:ok, mgs} = + Ash.read( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(group_id == ^group.id)), + actor: actor, + domain: Mv.Membership + ) + + assert mgs == [] + end + end +end diff --git a/test/membership/member_groups_relationship_test.exs b/test/membership/member_groups_relationship_test.exs new file mode 100644 index 0000000..a72c8bc --- /dev/null +++ b/test/membership/member_groups_relationship_test.exs @@ -0,0 +1,197 @@ +defmodule Mv.Membership.MemberGroupsRelationshipTest do + @moduledoc """ + Tests for Member resource extension with groups relationship. + """ + use Mv.DataCase, async: true + + alias Mv.Membership + + require Ash.Query + import Ash.Expr + + setup do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + %{actor: system_actor} + end + + describe "Relationships" do + test "member has many_to_many groups relationship (load with preloading)", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group1} = Membership.create_group(%{name: "Group One"}, actor: actor) + {:ok, group2} = Membership.create_group(%{name: "Group Two"}, actor: actor) + + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member.id, group_id: group1.id}, + actor: actor + ) + + {:ok, _mg2} = + Membership.create_member_group(%{member_id: member.id, group_id: group2.id}, + actor: actor + ) + + # Load member with groups + {:ok, member_with_groups} = + Ash.load(member, :groups, actor: actor, domain: Mv.Membership) + + assert length(member_with_groups.groups) == 2 + assert Enum.any?(member_with_groups.groups, &(&1.id == group1.id)) + assert Enum.any?(member_with_groups.groups, &(&1.id == group2.id)) + end + + test "load multiple members with groups preloaded (N+1 prevention)", %{actor: actor} do + {:ok, member1} = Membership.create_member(%{email: "member1@test.com"}, actor: actor) + {:ok, member2} = Membership.create_member(%{email: "member2@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member1.id, group_id: group.id}, + actor: actor + ) + + {:ok, _mg2} = + Membership.create_member_group(%{member_id: member2.id, group_id: group.id}, + actor: actor + ) + + # Load all members with groups in single query + {:ok, members} = + Ash.read(Mv.Membership.Member, actor: actor, domain: Mv.Membership, load: [:groups]) + + member1_loaded = Enum.find(members, &(&1.id == member1.id)) + member2_loaded = Enum.find(members, &(&1.id == member2.id)) + + assert length(member1_loaded.groups) == 1 + assert length(member2_loaded.groups) == 1 + assert hd(member1_loaded.groups).id == group.id + assert hd(member2_loaded.groups).id == group.id + end + end + + describe "Member-Group Association Operations" do + test "add member to group via Ash API", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + assert {:ok, member_group} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + assert member_group.member_id == member.id + assert member_group.group_id == group.id + end + + test "remove member from group via Ash API", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + {:ok, member_group} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Remove association + :ok = Membership.destroy_member_group(member_group, actor: actor) + + # Verify association is removed + {:ok, mgs} = + Ash.read( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(member_id == ^member.id and group_id == ^group.id)), + actor: actor, + domain: Mv.Membership + ) + + assert mgs == [] + end + + test "add member to multiple groups in single operation", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group1} = Membership.create_group(%{name: "Group One"}, actor: actor) + {:ok, group2} = Membership.create_group(%{name: "Group Two"}, actor: actor) + {:ok, group3} = Membership.create_group(%{name: "Group Three"}, actor: actor) + + # Add to all groups + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member.id, group_id: group1.id}, + actor: actor + ) + + {:ok, _mg2} = + Membership.create_member_group(%{member_id: member.id, group_id: group2.id}, + actor: actor + ) + + {:ok, _mg3} = + Membership.create_member_group(%{member_id: member.id, group_id: group3.id}, + actor: actor + ) + + # Verify all associations exist + {:ok, member_with_groups} = + Ash.load(member, :groups, actor: actor, domain: Mv.Membership) + + assert length(member_with_groups.groups) == 3 + end + end + + describe "Edge Cases" do + test "adding member to same group twice fails (duplicate prevention)", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + {:ok, _mg1} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # Try to add again + assert {:error, %Ash.Error.Invalid{}} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + end + + test "removing member from group they're not in (idempotent, no error)", %{actor: actor} do + {:ok, member} = Membership.create_member(%{email: "test@test.com"}, actor: actor) + {:ok, group} = Membership.create_group(%{name: "Test Group"}, actor: actor) + + # Verify no association exists + {:ok, nil} = + Ash.read_one( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(member_id == ^member.id and group_id == ^group.id)), + actor: actor, + domain: Mv.Membership + ) + + # Test idempotency: Create association, delete it, then try to delete again + # This verifies that destroy_member_group is idempotent + {:ok, member_group} = + Membership.create_member_group(%{member_id: member.id, group_id: group.id}, + actor: actor + ) + + # First deletion should succeed + assert :ok = Membership.destroy_member_group(member_group, actor: actor) + + # Verify association is deleted + {:ok, nil} = + Ash.read_one( + Mv.Membership.MemberGroup + |> Ash.Query.filter(expr(id == ^member_group.id)), + actor: actor, + domain: Mv.Membership + ) + + # Try to destroy again - should be idempotent (either succeed or return not found error) + # Note: This tests the idempotency of the destroy action + result = Membership.destroy_member_group(member_group, actor: actor) + + # Should either succeed (idempotent) or return an error (not found) + # Both behaviors are acceptable for idempotency + assert result == :ok || match?({:error, _}, result) + end + end +end From 8e9fbe76cf5e8f39b4a94738e31b80bdad579e7a Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 27 Jan 2026 15:22:41 +0100 Subject: [PATCH 3/6] docs: add testing philosophy to coding guideline and update groups architecture docs #371 --- CODE_GUIDELINES.md | 34 ++++++++++++++ docs/groups-architecture.md | 94 +++++++++++++++++++++++++------------ 2 files changed, 99 insertions(+), 29 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 17b03d0..4905b79 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1515,6 +1515,40 @@ mix test test/membership/member_test.exs:42 ### 4.7 Testing Best Practices +**Testing Philosophy: Focus on Business Logic, Not Framework Functionality** + +We test our business logic and domain-specific behavior, not core framework features. Framework features (Ash validations, Ecto relationships, etc.) are already tested by their respective libraries. + +**What We Test:** +- Business rules and validations specific to our domain +- Custom business logic (slug generation, calculations, etc.) +- Integration between our resources +- Database-level constraints (unique constraints, foreign keys, CASCADE) +- Query performance (N+1 prevention) + +**What We Don't Test:** +- Framework core functionality (Ash validations work, Ecto relationships work, etc.) +- Standard CRUD operations without custom logic +- Framework-provided features that are already tested upstream +- Detailed slug generation edge cases (Umlauts, truncation, etc.) if covered by reusable change tests + +**Example:** +```elixir +# ✅ GOOD - Tests our business rule +test "slug is immutable (doesn't change when name is updated)" do + {:ok, group} = Membership.create_group(%{name: "Original"}, actor: actor) + original_slug = group.slug + + {:ok, updated} = Membership.update_group(group, %{name: "New"}, actor: actor) + assert updated.slug == original_slug # Business rule: slug doesn't change +end + +# ❌ AVOID - Tests framework functionality +test "Ash.Changeset validates required fields" do + # This is already tested by Ash framework +end +``` + **Descriptive Test Names:** ```elixir diff --git a/docs/groups-architecture.md b/docs/groups-architecture.md index 66c663d..5e31bc5 100644 --- a/docs/groups-architecture.md +++ b/docs/groups-architecture.md @@ -944,6 +944,24 @@ Each functional unit can be implemented as a **separate issue**: ## Testing Strategy +### Testing Philosophy + +**Focus on Business Logic, Not Framework Functionality** + +We test our business logic and domain-specific behavior, not core framework features. Framework features (Ash validations, Ecto relationships, etc.) are already tested by their respective libraries. + +**What We Test:** +- Business rules and validations specific to our domain +- Custom business logic (slug generation, calculations, etc.) +- Integration between our resources +- Database-level constraints (unique constraints, foreign keys, CASCADE) +- Query performance (N+1 prevention) + +**What We Don't Test:** +- Framework core functionality (Ash validations work, Ecto relationships work, etc.) +- Standard CRUD operations without custom logic +- Framework-provided features that are already tested upstream + ### Unit Tests #### Group Resource Tests @@ -952,31 +970,23 @@ Each functional unit can be implemented as a **separate issue**: **Test Cases:** - Create group with valid attributes -- Return error when name is missing -- Return error when name exceeds 100 characters -- Return error when name is not unique -- Name uniqueness is case-insensitive -- Allow description to be nil -- Trim whitespace from name -- Description max length is 500 characters -- Slug is automatically generated from name on create -- Slug is immutable (doesn't change on update) -- Slug is unique (cannot have duplicates) -- Slug handles German umlauts correctly (ä → a, ß → ss) -- Slug converts to lowercase -- Slug removes special characters -- Slug replaces spaces with hyphens -- Slug truncates to max 100 characters -- Slug cannot be empty (validation fails if slug would be empty) -- Read group by slug (via identity lookup) +- Return error when name is missing (required validation) +- Return error when name exceeds 100 characters (length validation) +- Return error when name is not unique (case-insensitive) - application level validation +- Allow description to be nil (optional field) +- Description max length is 500 characters (length validation) +- Slug is automatically generated from name on create (custom business logic) +- Slug is immutable (doesn't change when name is updated) - business rule +- Slug is unique (prevents duplicate slugs from different names) - business rule +- Slug cannot be empty (rejects name with only special characters) - business rule - Update group name and description -- Prevent duplicate name on update -- Prevent duplicate slug on create (same name → same slug → error) -- Delete group and all member associations -- Do not delete members themselves -- Member count calculation returns 0 for empty group -- Member count calculation returns correct count when members added -- Member count updates correctly when members removed +- Prevent duplicate name on update (case-insensitive) - business rule +- Delete group and all member associations (cascade behavior) +- Do not delete members themselves (cascade boundary) +- Member count calculation returns 0 for empty group (custom calculation) +- Member count calculation returns correct count when members added/removed (custom calculation) + +**Note:** Detailed slug generation tests (Umlauts, truncation, etc.) are covered by the `GenerateSlug` change tests in `custom_field_slug_test.exs`, which is reused for groups. We don't duplicate these framework-level tests. #### MemberGroup Resource Tests @@ -984,9 +994,23 @@ Each functional unit can be implemented as a **separate issue**: **Test Cases:** - Create association between member and group -- Prevent duplicate associations -- Cascade delete when member deleted -- Cascade delete when group deleted +- Prevent duplicate associations (unique constraint) +- Cascade delete when member deleted (database constraint) +- Cascade delete when group deleted (database constraint) + +#### Database Constraint Tests + +**File:** `test/membership/group_database_constraints_test.exs` + +**Test Cases:** +- Database enforces unique name constraint (case-insensitive via LOWER) - DB level +- Database enforces unique slug constraint (case-sensitive) - DB level +- Cannot create MemberGroup with non-existent member_id (foreign key constraint) +- Cannot create MemberGroup with non-existent group_id (foreign key constraint) +- Deleting member cascades to member_groups (verified at DB level) +- Deleting group cascades to member_groups (verified at DB level) + +**Note:** These tests verify that constraints are enforced at the database level, not just application level. ### Integration Tests @@ -995,8 +1019,20 @@ Each functional unit can be implemented as a **separate issue**: **File:** `test/membership/group_integration_test.exs` **Test Cases:** -- Member can belong to multiple groups -- Group can contain multiple members +- Member can belong to multiple groups (many-to-many relationship) +- Group can contain multiple members (many-to-many relationship) +- Preloading groups with members avoids N+1 queries (performance test with query count verification) + +**File:** `test/membership/member_groups_relationship_test.exs` + +**Test Cases:** +- Member has many_to_many groups relationship (load with preloading) +- Load multiple members with groups preloaded (N+1 prevention) +- Add member to group via Ash API +- Remove member from group via Ash API +- Add member to multiple groups in single operation +- Adding member to same group twice fails (duplicate prevention) +- Removing member from group they're not in (idempotent, no error) ### UI Tests From 6db64bf996599ff3281e6c6bc97f59a27cd584f8 Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 27 Jan 2026 16:03:21 +0100 Subject: [PATCH 4/6] feat: add groups resource #371 --- docs/groups-architecture.md | 7 +- .../changes/generate_slug.ex | 33 +++- lib/membership/custom_field.ex | 2 +- lib/membership/group.ex | 171 ++++++++++++++++++ lib/membership/member.ex | 6 + lib/membership/member_group.ex | 128 +++++++++++++ lib/membership/membership.ex | 17 ++ ...127141620_add_groups_and_member_groups.exs | 116 ++++++++++++ .../repo/groups/20260127141620.json | 106 +++++++++++ .../repo/member_groups/20260127141620.json | 136 ++++++++++++++ test/membership/group_test.exs | 26 ++- test/membership/member_group_test.exs | 12 +- 12 files changed, 742 insertions(+), 18 deletions(-) rename lib/membership/{custom_field => }/changes/generate_slug.ex (80%) create mode 100644 lib/membership/group.ex create mode 100644 lib/membership/member_group.ex create mode 100644 priv/repo/migrations/20260127141620_add_groups_and_member_groups.exs create mode 100644 priv/resource_snapshots/repo/groups/20260127141620.json create mode 100644 priv/resource_snapshots/repo/member_groups/20260127141620.json diff --git a/docs/groups-architecture.md b/docs/groups-architecture.md index 5e31bc5..023df5b 100644 --- a/docs/groups-architecture.md +++ b/docs/groups-architecture.md @@ -1069,10 +1069,9 @@ We test our business logic and domain-specific behavior, not core framework feat - Create unique index on slug (for slug uniqueness and lookups) - Create index on lowercased name for search -**Note:** Slug generation follows the same pattern as CustomFields: -- Uses `Mv.Membership.CustomField.Changes.GenerateSlug` (reusable change) -- Or create `Mv.Membership.Group.Changes.GenerateSlug` if needed -- Slug is generated on create, immutable on update +**Note:** Slug generation uses the shared `Mv.Membership.Changes.GenerateSlug` change, +which is used by both CustomFields and Groups for consistent slug generation. +Slug is generated on create, immutable on update. **Migration 2: Create member_groups join table** - Create table with UUID v7 primary key diff --git a/lib/membership/custom_field/changes/generate_slug.ex b/lib/membership/changes/generate_slug.ex similarity index 80% rename from lib/membership/custom_field/changes/generate_slug.ex rename to lib/membership/changes/generate_slug.ex index 061d7e7..d9d2216 100644 --- a/lib/membership/custom_field/changes/generate_slug.ex +++ b/lib/membership/changes/generate_slug.ex @@ -1,4 +1,4 @@ -defmodule Mv.Membership.CustomField.Changes.GenerateSlug do +defmodule Mv.Membership.Changes.GenerateSlug do @moduledoc """ Ash Change that automatically generates a URL-friendly slug from the `name` attribute. @@ -14,12 +14,26 @@ defmodule Mv.Membership.CustomField.Changes.GenerateSlug do - Trims leading/trailing hyphens - Truncates to max 100 characters + ## Usage + + Works for any resource with `name` and `slug` attributes. + Used by CustomField and Group resources. + + create :create do + accept [:name, :description] + change Mv.Membership.Changes.GenerateSlug + validate string_length(:slug, min: 1) + end + ## Examples # Create with automatic slug generation CustomField.create!(%{name: "Mobile Phone"}) # => %CustomField{name: "Mobile Phone", slug: "mobile-phone"} + Group.create!(%{name: "Test Group"}) + # => %Group{name: "Test Group", slug: "test-group"} + # German umlauts are converted CustomField.create!(%{name: "Café Müller"}) # => %CustomField{name: "Café Müller", slug: "cafe-muller"} @@ -32,7 +46,7 @@ defmodule Mv.Membership.CustomField.Changes.GenerateSlug do ## Implementation Note This change only runs on `:create` actions. The slug is immutable by design, - as changing slugs would break external references (e.g., CSV imports/exports). + as changing slugs would break external references (e.g., CSV imports/exports, URL routes). """ use Ash.Resource.Change @@ -47,11 +61,14 @@ defmodule Mv.Membership.CustomField.Changes.GenerateSlug do ## Parameters - `changeset` - The Ash changeset + - `_opts` - Options passed to the change (unused) + - `_context` - Ash context map (unused) ## Returns The changeset with the `:slug` attribute set to the generated slug. """ + @impl true def change(changeset, _opts, _context) do # Only generate slug on create, not on update (immutability) if changeset.action_type == :create do @@ -62,6 +79,9 @@ defmodule Mv.Membership.CustomField.Changes.GenerateSlug do name when is_binary(name) -> slug = generate_slug(name) Ash.Changeset.force_change_attribute(changeset, :slug, slug) + + _ -> + changeset end else # On update, don't touch the slug (immutable) @@ -80,6 +100,14 @@ defmodule Mv.Membership.CustomField.Changes.GenerateSlug do - Leading/trailing hyphens removed - Maximum length of 100 characters + ## Parameters + + - `name` - The string to convert to a slug + + ## Returns + + A URL-friendly slug string, or empty string if input is invalid. + ## Examples iex> generate_slug("Mobile Phone") @@ -104,6 +132,7 @@ defmodule Mv.Membership.CustomField.Changes.GenerateSlug do "strasse" """ + @spec generate_slug(String.t()) :: String.t() def generate_slug(name) when is_binary(name) do slug = Slug.slugify(name) diff --git a/lib/membership/custom_field.ex b/lib/membership/custom_field.ex index 18b8154..94cb657 100644 --- a/lib/membership/custom_field.ex +++ b/lib/membership/custom_field.ex @@ -63,7 +63,7 @@ defmodule Mv.Membership.CustomField do create :create do accept [:name, :value_type, :description, :required, :show_in_overview] - change Mv.Membership.CustomField.Changes.GenerateSlug + change Mv.Membership.Changes.GenerateSlug validate string_length(:slug, min: 1) end diff --git a/lib/membership/group.ex b/lib/membership/group.ex new file mode 100644 index 0000000..9670c5e --- /dev/null +++ b/lib/membership/group.ex @@ -0,0 +1,171 @@ +defmodule Mv.Membership.Group do + @moduledoc """ + Ash resource representing a group that members can belong to. + + ## Overview + Groups allow organizing members into categories (e.g., "Board Members", "Active Members"). + Each member can belong to multiple groups, and each group can contain multiple members. + + ## Attributes + - `name` - Unique group name (required, max 100 chars, case-insensitive uniqueness) + - `slug` - URL-friendly identifier (required, max 100 chars, auto-generated from name, immutable) + - `description` - Optional description (max 500 chars) + + ## Relationships + - `has_many :member_groups` - Relationship to MemberGroup join table + - `many_to_many :members` - Relationship to Members through MemberGroup + + ## Constraints + - Name must be unique (case-insensitive, using LOWER(name) in database) + - Slug must be unique (case-sensitive, exact match) + - Name cannot be null + - Slug cannot be null + + ## Calculations + - `member_count` - Returns the number of members in this group + + ## Examples + # Create a new group + Group.create!(%{name: "Board Members", description: "Members of the board"}) + # => %Group{name: "Board Members", slug: "board-members", ...} + + # Update group (slug remains unchanged) + group = Group.get_by_slug!("board-members") + Group.update!(group, %{description: "Updated description"}) + # => %Group{slug: "board-members", ...} # slug unchanged! + """ + use Ash.Resource, + domain: Mv.Membership, + data_layer: AshPostgres.DataLayer + + require Ash.Query + import Ash.Expr + alias Mv.Helpers + alias Mv.Helpers.SystemActor + require Logger + + postgres do + table "groups" + repo Mv.Repo + end + + actions do + defaults [:read, :destroy] + + create :create do + accept [:name, :description] + change Mv.Membership.Changes.GenerateSlug + validate string_length(:slug, min: 1) + end + + update :update do + accept [:name, :description] + require_atomic? false + end + end + + validations do + validate present(:name) + + # Case-insensitive name uniqueness validation + 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) + else + :ok + end + end + end + + attributes do + uuid_v7_primary_key :id + + attribute :name, :string do + allow_nil? false + public? true + + constraints max_length: 100, + trim?: true + end + + attribute :slug, :string do + allow_nil? false + public? true + writable? false + + constraints max_length: 100, + trim?: true + end + + attribute :description, :string do + allow_nil? true + public? true + + constraints max_length: 500, + trim?: true + end + + timestamps() + end + + relationships do + has_many :member_groups, Mv.Membership.MemberGroup + 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 + end + + identities do + identity :unique_slug, [:slug] + end + + # Private helper function for case-insensitive name uniqueness check + defp check_name_uniqueness(name, exclude_id) do + 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) + + case Ash.read(query, opts) do + {:ok, []} -> + :ok + + {:ok, _} -> + {:error, field: :name, message: "has already been taken", value: name} + + {:error, reason} -> + Logger.warning( + "Name uniqueness validation query failed for group name '#{name}': #{inspect(reason)}. Allowing operation to proceed (fail-open)." + ) + + :ok + end + end + + defp maybe_exclude_id(query, nil), do: query + defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) +end diff --git a/lib/membership/member.ex b/lib/membership/member.ex index 1a5d805..da61146 100644 --- a/lib/membership/member.ex +++ b/lib/membership/member.ex @@ -582,6 +582,12 @@ defmodule Mv.Membership.Member do # has_many: All fee cycles for this member has_many :membership_fee_cycles, Mv.MembershipFees.MembershipFeeCycle + + # Groups relationships + # has_many: All member-group associations for this member + has_many :member_groups, Mv.Membership.MemberGroup + # many_to_many: All groups this member belongs to (through MemberGroup) + many_to_many :groups, Mv.Membership.Group, through: Mv.Membership.MemberGroup end calculations do diff --git a/lib/membership/member_group.ex b/lib/membership/member_group.ex new file mode 100644 index 0000000..551531d --- /dev/null +++ b/lib/membership/member_group.ex @@ -0,0 +1,128 @@ +defmodule Mv.Membership.MemberGroup do + @moduledoc """ + Ash resource representing the join table for the many-to-many relationship + between Members and Groups. + + ## Overview + MemberGroup is a join table that links members to groups. It enables the + many-to-many relationship where: + - A member can belong to multiple groups + - A group can contain multiple members + + ## Attributes + - `member_id` - Foreign key to Member (required) + - `group_id` - Foreign key to Group (required) + + ## Relationships + - `belongs_to :member` - Relationship to Member + - `belongs_to :group` - Relationship to Group + + ## Constraints + - Unique constraint on `(member_id, group_id)` - prevents duplicate memberships + - CASCADE delete: Removing member removes all group associations + - CASCADE delete: Removing group removes all member associations + + ## Examples + # Add member to group + MemberGroup.create!(%{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) + """ + use Ash.Resource, + domain: Mv.Membership, + data_layer: AshPostgres.DataLayer + + require Ash.Query + import Ash.Expr + + postgres do + table "member_groups" + repo Mv.Repo + end + + actions do + defaults [:read, :destroy] + + create :create do + accept [:member_id, :group_id] + end + end + + validations do + validate present(:member_id) + validate present(:group_id) + + # Prevent duplicate associations + 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) + else + :ok + end + end + end + + attributes do + uuid_v7_primary_key :id + + attribute :member_id, :uuid do + allow_nil? false + end + + attribute :group_id, :uuid do + allow_nil? false + end + + timestamps() + end + + relationships do + belongs_to :member, Mv.Membership.Member do + allow_nil? false + end + + belongs_to :group, Mv.Membership.Group do + allow_nil? false + end + end + + identities do + identity :unique_member_group, [:member_id, :group_id] + end + + # Private helper function to check for duplicate associations + defp check_duplicate_association(member_id, group_id, exclude_id) do + alias Mv.Helpers + alias Mv.Helpers.SystemActor + + 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) + + case Ash.read(query, opts) do + {:ok, []} -> + :ok + + {:ok, _} -> + {:error, field: :member_id, message: "Member is already in this group", value: member_id} + + {:error, _reason} -> + # Fail-open: if query fails, allow operation to proceed + # Database constraint will catch duplicates anyway + :ok + end + end + + defp maybe_exclude_id(query, nil), do: query + defp maybe_exclude_id(query, id), do: Ash.Query.filter(query, id != ^id) +end diff --git a/lib/membership/membership.ex b/lib/membership/membership.ex index 97ac4be..b42daa5 100644 --- a/lib/membership/membership.ex +++ b/lib/membership/membership.ex @@ -7,6 +7,8 @@ defmodule Mv.Membership do - `CustomFieldValue` - Dynamic custom field values attached to members - `CustomField` - Schema definitions for custom fields - `Setting` - Global application settings (singleton) + - `Group` - Groups that members can belong to + - `MemberGroup` - Join table for many-to-many relationship between Members and Groups ## Public API The domain exposes these main actions: @@ -14,6 +16,8 @@ defmodule Mv.Membership do - Custom field value management: `create_custom_field_value/1`, `list_custom_field_values/0`, etc. - Custom field management: `create_custom_field/1`, `list_custom_fields/0`, `list_required_custom_fields/0`, etc. - Settings management: `get_settings/0`, `update_settings/2`, `update_member_field_visibility/2`, `update_single_member_field_visibility/3` + - Group management: `create_group/1`, `list_groups/0`, `update_group/2`, `destroy_group/1` + - Member-group associations: `create_member_group/1`, `list_member_groups/0`, `destroy_member_group/1` ## Admin Interface The domain is configured with AshAdmin for management UI. @@ -61,6 +65,19 @@ defmodule Mv.Membership do define :update_single_member_field_visibility, action: :update_single_member_field_visibility end + + resource Mv.Membership.Group do + define :create_group, action: :create + define :list_groups, action: :read + define :update_group, action: :update + define :destroy_group, action: :destroy + end + + resource Mv.Membership.MemberGroup do + define :create_member_group, action: :create + define :list_member_groups, action: :read + define :destroy_member_group, action: :destroy + end end # Singleton pattern: Get the single settings record diff --git a/priv/repo/migrations/20260127141620_add_groups_and_member_groups.exs b/priv/repo/migrations/20260127141620_add_groups_and_member_groups.exs new file mode 100644 index 0000000..3736956 --- /dev/null +++ b/priv/repo/migrations/20260127141620_add_groups_and_member_groups.exs @@ -0,0 +1,116 @@ +defmodule Mv.Repo.Migrations.AddGroupsAndMemberGroups do + @moduledoc """ + Updates resources based on their most recent snapshots. + + This file was autogenerated with `mix ash_postgres.generate_migrations` + """ + + use Ecto.Migration + + def up do + create table(:member_groups, primary_key: false) do + add :id, :uuid, null: false, default: fragment("uuid_generate_v7()"), primary_key: true + + add :member_id, + references(:members, + column: :id, + name: "member_groups_member_id_fkey", + type: :uuid, + prefix: "public", + on_delete: :delete_all + ), + null: false + + add :group_id, :uuid, null: false + + add :inserted_at, :utc_datetime_usec, + null: false, + default: fragment("(now() AT TIME ZONE 'utc')") + + add :updated_at, :utc_datetime_usec, + null: false, + default: fragment("(now() AT TIME ZONE 'utc')") + end + + create table(:groups, primary_key: false) do + add :id, :uuid, null: false, default: fragment("uuid_generate_v7()"), primary_key: true + end + + alter table(:member_groups) do + modify :group_id, + references(:groups, + column: :id, + name: "member_groups_group_id_fkey", + type: :uuid, + prefix: "public", + on_delete: :delete_all + ) + end + + # Unique constraint on (member_id, group_id) to prevent duplicate associations + create unique_index(:member_groups, [:member_id, :group_id], + name: "member_groups_unique_member_group_index" + ) + + # Indexes for efficient queries + create index(:member_groups, [:member_id], name: "member_groups_member_id_index") + create index(:member_groups, [:group_id], name: "member_groups_group_id_index") + + alter table(:groups) do + add :name, :text, null: false + add :slug, :text, null: false + add :description, :text + + add :inserted_at, :utc_datetime_usec, + null: false, + default: fragment("(now() AT TIME ZONE 'utc')") + + add :updated_at, :utc_datetime_usec, + null: false, + default: fragment("(now() AT TIME ZONE 'utc')") + end + + # Unique index on slug (case-sensitive) + create unique_index(:groups, [:slug], name: "groups_unique_slug_index") + + # Unique index on LOWER(name) for case-insensitive uniqueness + # Using execute because Ecto doesn't support fragment in index column list + execute( + "CREATE UNIQUE INDEX groups_unique_name_lower_index ON groups (LOWER(name))", + "DROP INDEX IF EXISTS groups_unique_name_lower_index" + ) + end + + def down do + execute("DROP INDEX IF EXISTS groups_unique_name_lower_index", "") + + drop_if_exists unique_index(:groups, [:slug], name: "groups_unique_slug_index") + + alter table(:groups) do + remove :updated_at + remove :inserted_at + remove :description + remove :slug + remove :name + end + + drop_if_exists index(:member_groups, [:group_id], name: "member_groups_group_id_index") + drop_if_exists index(:member_groups, [:member_id], name: "member_groups_member_id_index") + + drop_if_exists unique_index(:member_groups, [:member_id, :group_id], + name: "member_groups_unique_member_group_index" + ) + + drop constraint(:member_groups, "member_groups_group_id_fkey") + + alter table(:member_groups) do + modify :group_id, :uuid + end + + drop table(:groups) + + drop constraint(:member_groups, "member_groups_member_id_fkey") + + drop table(:member_groups) + end +end diff --git a/priv/resource_snapshots/repo/groups/20260127141620.json b/priv/resource_snapshots/repo/groups/20260127141620.json new file mode 100644 index 0000000..ade6dd8 --- /dev/null +++ b/priv/resource_snapshots/repo/groups/20260127141620.json @@ -0,0 +1,106 @@ +{ + "attributes": [ + { + "allow_nil?": false, + "default": "fragment(\"uuid_generate_v7()\")", + "generated?": false, + "precision": null, + "primary_key?": true, + "references": null, + "scale": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "name", + "type": "text" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "slug", + "type": "text" + }, + { + "allow_nil?": true, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "description", + "type": "text" + }, + { + "allow_nil?": false, + "default": "fragment(\"(now() AT TIME ZONE 'utc')\")", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "inserted_at", + "type": "utc_datetime_usec" + }, + { + "allow_nil?": false, + "default": "fragment(\"(now() AT TIME ZONE 'utc')\")", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "updated_at", + "type": "utc_datetime_usec" + } + ], + "base_filter": null, + "check_constraints": [], + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "EB2489A9C4F649CBBDBD5E0685F703F10AF04448FB01A424801EEE36BAFF1A4A", + "identities": [ + { + "all_tenants?": false, + "base_filter": null, + "index_name": "groups_unique_slug_index", + "keys": [ + { + "type": "atom", + "value": "slug" + } + ], + "name": "unique_slug", + "nils_distinct?": true, + "where": null + } + ], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.Mv.Repo", + "schema": null, + "table": "groups" +} \ No newline at end of file diff --git a/priv/resource_snapshots/repo/member_groups/20260127141620.json b/priv/resource_snapshots/repo/member_groups/20260127141620.json new file mode 100644 index 0000000..957c84c --- /dev/null +++ b/priv/resource_snapshots/repo/member_groups/20260127141620.json @@ -0,0 +1,136 @@ +{ + "attributes": [ + { + "allow_nil?": false, + "default": "fragment(\"uuid_generate_v7()\")", + "generated?": false, + "precision": null, + "primary_key?": true, + "references": null, + "scale": null, + "size": null, + "source": "id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": { + "deferrable": false, + "destination_attribute": "id", + "destination_attribute_default": null, + "destination_attribute_generated": null, + "index?": false, + "match_type": null, + "match_with": null, + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "name": "member_groups_member_id_fkey", + "on_delete": null, + "on_update": null, + "primary_key?": true, + "schema": "public", + "table": "members" + }, + "scale": null, + "size": null, + "source": "member_id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "nil", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": { + "deferrable": false, + "destination_attribute": "id", + "destination_attribute_default": null, + "destination_attribute_generated": null, + "index?": false, + "match_type": null, + "match_with": null, + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "name": "member_groups_group_id_fkey", + "on_delete": null, + "on_update": null, + "primary_key?": true, + "schema": "public", + "table": "groups" + }, + "scale": null, + "size": null, + "source": "group_id", + "type": "uuid" + }, + { + "allow_nil?": false, + "default": "fragment(\"(now() AT TIME ZONE 'utc')\")", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "inserted_at", + "type": "utc_datetime_usec" + }, + { + "allow_nil?": false, + "default": "fragment(\"(now() AT TIME ZONE 'utc')\")", + "generated?": false, + "precision": null, + "primary_key?": false, + "references": null, + "scale": null, + "size": null, + "source": "updated_at", + "type": "utc_datetime_usec" + } + ], + "base_filter": null, + "check_constraints": [], + "custom_indexes": [], + "custom_statements": [], + "has_create_action": true, + "hash": "6A81B894ADE7993917E2F97AB0C7233894AA7E59126DF2C17A7F04AEBDA6C159", + "identities": [ + { + "all_tenants?": false, + "base_filter": null, + "index_name": "member_groups_unique_member_group_index", + "keys": [ + { + "type": "atom", + "value": "member_id" + }, + { + "type": "atom", + "value": "group_id" + } + ], + "name": "unique_member_group", + "nils_distinct?": true, + "where": null + } + ], + "multitenancy": { + "attribute": null, + "global": null, + "strategy": null + }, + "repo": "Elixir.Mv.Repo", + "schema": null, + "table": "member_groups" +} \ No newline at end of file diff --git a/test/membership/group_test.exs b/test/membership/group_test.exs index a9e058a..1c84eeb 100644 --- a/test/membership/group_test.exs +++ b/test/membership/group_test.exs @@ -50,7 +50,7 @@ defmodule Mv.Membership.GroupTest do assert {:error, %Ash.Error.Invalid{errors: errors}} = Membership.create_group(attrs, actor: actor) - assert error_message(errors, :name) =~ "must be at most 100" + assert error_message(errors, :name) =~ "100" or error_message(errors, :name) =~ "length" end test "return error when name is not unique (case-insensitive) - application level validation", @@ -77,7 +77,8 @@ defmodule Mv.Membership.GroupTest do assert {:error, %Ash.Error.Invalid{errors: errors}} = Membership.create_group(attrs, actor: actor) - assert error_message(errors, :description) =~ "must be at most 500" + assert error_message(errors, :description) =~ "500" or + error_message(errors, :description) =~ "length" end end @@ -123,9 +124,13 @@ defmodule Mv.Membership.GroupTest do Membership.create_group(%{name: "!!!"}, actor: actor) assert Enum.any?(errors, fn err -> - (err.field == :slug or err.field == :name) and - (String.contains?(err.message, "cannot be empty") or - String.contains?(err.message, "is required")) + field = Map.get(err, :field) + message = Map.get(err, :message, Exception.message(err)) + + (field == :slug or field == :name) and + (String.contains?(message, "cannot be empty") or + String.contains?(message, "is required") or + String.contains?(message, "must be present")) end) end end @@ -277,8 +282,15 @@ defmodule Mv.Membership.GroupTest do # Returns the error message for a given field, or empty string if not found defp error_message(errors, field) do case Enum.find(errors, fn err -> Map.get(err, :field) == field end) do - nil -> "" - err -> Map.get(err, :message, "") + nil -> + "" + + err -> + # Handle different error types (Ash.Error.Changes.Required doesn't have :message) + case Map.get(err, :message) do + nil -> Exception.message(err) + message -> message + end end end end diff --git a/test/membership/member_group_test.exs b/test/membership/member_group_test.exs index 54f9ff9..b3c048f 100644 --- a/test/membership/member_group_test.exs +++ b/test/membership/member_group_test.exs @@ -44,10 +44,14 @@ defmodule Mv.Membership.MemberGroupTest do ) assert Enum.any?(errors, fn err -> - ((err.field == :member_id or err.field == :group_id) and - String.contains?(err.message, "already been taken")) or - String.contains?(err.message, "already exists") or - String.contains?(err.message, "duplicate") + field = Map.get(err, :field) + message = Map.get(err, :message, "") + + (field == :member_id or field == :group_id) and + (String.contains?(message, "already been taken") or + String.contains?(message, "already exists") or + String.contains?(message, "duplicate") or + String.contains?(message, "already in this group")) end) end end From fc8306cfeead5133b25b3d9c1de6a0e77917e8ca Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 27 Jan 2026 16:38:17 +0100 Subject: [PATCH 5/6] test: resolve warnings --- lib/membership/group.ex | 1 - lib/membership/member_group.ex | 1 - test/membership/group_integration_test.exs | 1 - 3 files changed, 3 deletions(-) diff --git a/lib/membership/group.ex b/lib/membership/group.ex index 9670c5e..94d40ee 100644 --- a/lib/membership/group.ex +++ b/lib/membership/group.ex @@ -39,7 +39,6 @@ defmodule Mv.Membership.Group do data_layer: AshPostgres.DataLayer require Ash.Query - import Ash.Expr alias Mv.Helpers alias Mv.Helpers.SystemActor require Logger diff --git a/lib/membership/member_group.ex b/lib/membership/member_group.ex index 551531d..64a91d1 100644 --- a/lib/membership/member_group.ex +++ b/lib/membership/member_group.ex @@ -35,7 +35,6 @@ defmodule Mv.Membership.MemberGroup do data_layer: AshPostgres.DataLayer require Ash.Query - import Ash.Expr postgres do table "member_groups" diff --git a/test/membership/group_integration_test.exs b/test/membership/group_integration_test.exs index 7664660..2333a66 100644 --- a/test/membership/group_integration_test.exs +++ b/test/membership/group_integration_test.exs @@ -7,7 +7,6 @@ defmodule Mv.Membership.GroupIntegrationTest do alias Mv.Membership require Ash.Query - import Ash.Expr setup do system_actor = Mv.Helpers.SystemActor.get_system_actor() From e92c98b5591c3992a09c17bf08349a839d5db8a2 Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 27 Jan 2026 17:09:07 +0100 Subject: [PATCH 6/6] refactor: fix review issues - member_count aggregate, migration down, docs, actor handling --- lib/membership/group.ex | 37 +++++++------------ lib/membership/member_group.ex | 30 +++++++++++---- ...127141620_add_groups_and_member_groups.exs | 2 +- 3 files changed, 37 insertions(+), 32 deletions(-) diff --git a/lib/membership/group.ex b/lib/membership/group.ex index 94d40ee..14aadc8 100644 --- a/lib/membership/group.ex +++ b/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, []} -> diff --git a/lib/membership/member_group.ex b/lib/membership/member_group.ex index 64a91d1..5d29bda 100644 --- a/lib/membership/member_group.ex +++ b/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, []} -> diff --git a/priv/repo/migrations/20260127141620_add_groups_and_member_groups.exs b/priv/repo/migrations/20260127141620_add_groups_and_member_groups.exs index 3736956..9af0eee 100644 --- a/priv/repo/migrations/20260127141620_add_groups_and_member_groups.exs +++ b/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")