From 2ebf28911260fce62e10e61f158145ab32cd83d5 Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 27 Jan 2026 13:41:25 +0100 Subject: [PATCH 01/28] 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 02/28] 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 03/28] 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 04/28] 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 63377717e451097967690f816980e9c4dab78dd0 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:01 +0100 Subject: [PATCH 05/28] Ensure system actor user exists via migration Creates user system@mila.local with Admin role if missing. Idempotent; guarantees system actor in production without relying on seeds. --- ...124937_ensure_system_actor_user_exists.exs | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs diff --git a/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs b/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs new file mode 100644 index 0000000..b0ee775 --- /dev/null +++ b/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs @@ -0,0 +1,59 @@ +defmodule Mv.Repo.Migrations.EnsureSystemActorUserExists do + @moduledoc """ + Ensures the system actor user always exists. + + The system actor is used for systemic operations (email sync, cycle generation, + background jobs). It is created by seeds in development; in production seeds + may not run, so this migration guarantees the user exists. + + Creates a user with email "system@mila.local" (default from Mv.Helpers.SystemActor) + and the Admin role. The user has no password and no OIDC ID, so it cannot log in. + """ + use Ecto.Migration + import Ecto.Query + + @system_user_email "system@mila.local" + + def up do + admin_role_id = ensure_admin_role_exists() + ensure_system_actor_user_exists(admin_role_id) + end + + def down do + # Not reversible - do not delete system user on rollback + :ok + end + + defp ensure_admin_role_exists do + case repo().one(from(r in "roles", where: r.name == "Admin", select: r.id)) do + nil -> + execute(""" + INSERT INTO roles (id, name, description, permission_set_name, is_system_role, inserted_at, updated_at) + VALUES (uuid_generate_v7(), 'Admin', 'Administrator with full access', 'admin', false, (now() AT TIME ZONE 'utc'), (now() AT TIME ZONE 'utc')) + """) + + role_id = repo().one(from(r in "roles", where: r.name == "Admin", select: r.id)) + IO.puts("✅ Created 'Admin' role (was missing)") + role_id + + role_id -> + role_id + end + end + + defp ensure_system_actor_user_exists(_admin_role_id) do + case repo().one(from(u in "users", where: u.email == ^@system_user_email, select: u.id)) do + nil -> + execute(""" + INSERT INTO users (id, email, hashed_password, oidc_id, member_id, role_id) + SELECT gen_random_uuid(), '#{@system_user_email}', NULL, NULL, NULL, r.id + FROM roles r WHERE r.name = 'Admin' LIMIT 1 + """) + + IO.puts("✅ Created system actor user (#{@system_user_email})") + + _ -> + :ok + end + end +end From 55f322a09b25ea58a156ee07229dd060e4c99a86 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:07 +0100 Subject: [PATCH 06/28] Prevent deletion of system actor user Add destroy validation and explicit destroy action (primary, require_atomic? false). Validation blocks destroy when email == SystemActor.system_user_email(). --- lib/accounts/user.ex | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index bcaf506..65eef35 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -86,7 +86,13 @@ defmodule Mv.Accounts.User do # - :create_user (for manual user creation with optional member link) # - :register_with_password (for password-based registration) # - :register_with_rauthy (for OIDC-based registration) - defaults [:read, :destroy] + defaults [:read] + + destroy :destroy do + primary? true + # Required because custom validation (system actor protection) cannot run atomically + require_atomic? false + end # Primary generic update action: # - Selected by AshAdmin's generated "Edit" UI and generic AshPhoenix @@ -359,6 +365,19 @@ defmodule Mv.Accounts.User do :ok end end + + # Prevent deletion of the system actor user (required for internal operations) + validate fn changeset, _context -> + if to_string(changeset.data.email) == Mv.Helpers.SystemActor.system_user_email() do + {:error, + field: :email, + message: + "Cannot delete system actor user. This user is required for internal operations."} + else + :ok + end + end, + on: [:destroy] end def validate_oidc_id_present(changeset, _context) do From 56bf411756820aa89911b38807fc30732353ae29 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:13 +0100 Subject: [PATCH 07/28] Hide system actor from user list and block show/edit Index: filter out SystemActor.system_user_email() in query. Show/Form: redirect to /users with flash when viewing or editing system actor user. Index format_error: handle Ash errors without :message field. --- lib/mv_web/live/user_live/form.ex | 25 +++++++++++++++++++++++-- lib/mv_web/live/user_live/index.ex | 16 +++++++++++++--- lib/mv_web/live/user_live/show.ex | 15 +++++++++++---- 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index 6cf3f0f..28af7c4 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -266,10 +266,31 @@ defmodule MvWeb.UserLive.Form do user = case params["id"] do - nil -> nil - id -> Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) + nil -> + nil + + id -> + loaded = + Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) + + if to_string(loaded.email) == Mv.Helpers.SystemActor.system_user_email() do + {:redirect, loaded} + else + loaded + end end + if match?({:redirect, _}, user) do + {:ok, + socket + |> put_flash(:error, gettext("This user cannot be edited.")) + |> push_navigate(to: ~p"/users")} + else + mount_continue(user, params, socket) + end + end + + defp mount_continue(user, params, socket) do action = if is_nil(user), do: gettext("New"), else: gettext("Edit") page_title = action <> " " <> gettext("User") diff --git a/lib/mv_web/live/user_live/index.ex b/lib/mv_web/live/user_live/index.ex index 2062ae6..ef3583e 100644 --- a/lib/mv_web/live/user_live/index.ex +++ b/lib/mv_web/live/user_live/index.ex @@ -25,10 +25,17 @@ defmodule MvWeb.UserLive.Index do import MvWeb.LiveHelpers, only: [current_actor: 1] + require Ash.Query + @impl true def mount(_params, _session, socket) do actor = current_actor(socket) - users = Ash.read!(Mv.Accounts.User, domain: Mv.Accounts, load: [:member], actor: actor) + + users = + Mv.Accounts.User + |> Ash.Query.filter(email != ^Mv.Helpers.SystemActor.system_user_email()) + |> Ash.read!(domain: Mv.Accounts, load: [:member], actor: actor) + sorted = Enum.sort_by(users, & &1.email) {:ok, @@ -138,8 +145,11 @@ defmodule MvWeb.UserLive.Index do defp sort_fun(:asc), do: &<=/2 defp sort_fun(:desc), do: &>=/2 - defp format_error(%Ash.Error.Invalid{errors: errors}) do - Enum.map_join(errors, ", ", fn %{message: message} -> message end) + defp format_error(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do + Enum.map_join(errors, ", ", fn + %{message: message} when is_binary(message) -> message + other -> inspect(other) + end) end defp format_error(error) do diff --git a/lib/mv_web/live/user_live/show.ex b/lib/mv_web/live/user_live/show.ex index 641e091..fe2dd24 100644 --- a/lib/mv_web/live/user_live/show.ex +++ b/lib/mv_web/live/user_live/show.ex @@ -75,9 +75,16 @@ defmodule MvWeb.UserLive.Show do actor = current_actor(socket) user = Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) - {:ok, - socket - |> assign(:page_title, gettext("Show User")) - |> assign(:user, user)} + if to_string(user.email) == Mv.Helpers.SystemActor.system_user_email() do + {:ok, + socket + |> put_flash(:error, gettext("This user cannot be viewed.")) + |> push_navigate(to: ~p"/users")} + else + {:ok, + socket + |> assign(:page_title, gettext("Show User")) + |> assign(:user, user)} + end end end From 86c1ab846258f9ea8f7c14de958ae173a27ec41e Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:26 +0100 Subject: [PATCH 08/28] Add tests for system actor protection and hiding Index: system actor not in list, destroy returns Ash.Error.Invalid. Show/Form: redirect to /users when viewing or editing system actor user. --- test/mv/helpers/system_actor_test.exs | 24 ++++++++++++++++-------- test/mv_web/live/user_live/show_test.exs | 10 ++++++++++ test/mv_web/user_live/form_test.exs | 10 ++++++++++ test/mv_web/user_live/index_test.exs | 21 +++++++++++++++++++++ 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/test/mv/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs index af28443..e676130 100644 --- a/test/mv/helpers/system_actor_test.exs +++ b/test/mv/helpers/system_actor_test.exs @@ -10,6 +10,14 @@ defmodule Mv.Helpers.SystemActorTest do require Ash.Query + # Deletes a user row directly via SQL, bypassing Ash validations. + # Use only in tests when setting up "no system user" / "no users" scenarios; + # Ash.destroy! forbids deleting the system actor user. + defp delete_user_bypass_ash(user) do + id = Ecto.UUID.dump!(user.id) + Ecto.Adapters.SQL.query!(Mv.Repo, "DELETE FROM users WHERE id = $1", [id]) + end + # Helper function to ensure admin role exists defp ensure_admin_role do case Authorization.list_roles() do @@ -124,7 +132,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -163,7 +171,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -177,7 +185,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^admin_email) |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -227,7 +235,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -241,7 +249,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^admin_email) |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -275,7 +283,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -314,7 +322,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -328,7 +336,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^admin_email) |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok diff --git a/test/mv_web/live/user_live/show_test.exs b/test/mv_web/live/user_live/show_test.exs index 3551fdf..9518106 100644 --- a/test/mv_web/live/user_live/show_test.exs +++ b/test/mv_web/live/user_live/show_test.exs @@ -154,4 +154,14 @@ defmodule MvWeb.UserLive.ShowTest do assert html =~ gettext("Show User") || html =~ to_string(user.email) end end + + describe "system actor user" do + test "redirects to user list when viewing system actor user", %{conn: conn} do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + conn = conn_with_oidc_user(conn) + + assert {:error, {:live_redirect, %{to: "/users"}}} = + live(conn, ~p"/users/#{system_actor.id}") + end + end end diff --git a/test/mv_web/user_live/form_test.exs b/test/mv_web/user_live/form_test.exs index ed309fb..4b76c19 100644 --- a/test/mv_web/user_live/form_test.exs +++ b/test/mv_web/user_live/form_test.exs @@ -420,4 +420,14 @@ defmodule MvWeb.UserLive.FormTest do assert is_nil(updated_user.member) end end + + describe "system actor user" do + test "redirects to user list when editing system actor user", %{conn: conn} do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + conn = conn_with_oidc_user(conn, %{email: "admin@example.com"}) + + assert {:error, {:live_redirect, %{to: "/users"}}} = + live(conn, "/users/#{system_actor.id}/edit") + end + end end diff --git a/test/mv_web/user_live/index_test.exs b/test/mv_web/user_live/index_test.exs index 41c198d..a1a02ea 100644 --- a/test/mv_web/user_live/index_test.exs +++ b/test/mv_web/user_live/index_test.exs @@ -405,6 +405,27 @@ defmodule MvWeb.UserLive.IndexTest do end end + describe "system actor user" do + test "does not show system actor user in list", %{conn: conn} do + # Ensure system actor exists (e.g. via get_system_actor in conn_with_oidc_user) + _system_actor = Mv.Helpers.SystemActor.get_system_actor() + system_email = Mv.Helpers.SystemActor.system_user_email() + + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, "/users") + + refute html =~ system_email, + "System actor user (#{system_email}) must not appear in the user list" + end + + test "destroying system actor user returns error", %{current_user: current_user} do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + assert {:error, %Ash.Error.Invalid{}} = + Ash.destroy(system_actor, domain: Mv.Accounts, actor: current_user) + end + end + describe "member linking display" do test "displays linked member name in user list", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() From b5b2317d69a845a7747fda59c6c8e63d0bfa028f Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:32 +0100 Subject: [PATCH 09/28] Add gettext strings for system actor show/edit redirect messages German: Dieser Benutzer kann nicht angezeigt/bearbeitet werden. --- priv/gettext/de/LC_MESSAGES/default.po | 10 ++++++++++ priv/gettext/default.pot | 10 ++++++++++ priv/gettext/en/LC_MESSAGES/default.po | 10 ++++++++++ 3 files changed, 30 insertions(+) diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 5496213..865dff4 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2125,3 +2125,13 @@ msgstr "E-Mail" #, elixir-autogen, elixir-format msgid "email %{email} has already been taken" msgstr "E-Mail %{email} wurde bereits verwendet" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be edited." +msgstr "Dieser Benutzer kann nicht bearbeitet werden." + +#: lib/mv_web/live/user_live/show.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be viewed." +msgstr "Dieser Benutzer kann nicht angezeigt werden." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index fc3a78c..e63621d 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2126,3 +2126,13 @@ msgstr "" #, elixir-autogen, elixir-format msgid "email %{email} has already been taken" msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be edited." +msgstr "" + +#: lib/mv_web/live/user_live/show.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be viewed." +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 9432a47..15c6f9a 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2126,3 +2126,13 @@ msgstr "" #, elixir-autogen, elixir-format msgid "email %{email} has already been taken" msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be edited." +msgstr "" + +#: lib/mv_web/live/user_live/show.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be viewed." +msgstr "" From fc8306cfeead5133b25b3d9c1de6a0e77917e8ca Mon Sep 17 00:00:00 2001 From: Simon Date: Tue, 27 Jan 2026 16:38:17 +0100 Subject: [PATCH 10/28] 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 11/28] 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") From 92b12af022381f9e7d9b0773e39f7eaf4d66f60d Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:23 +0100 Subject: [PATCH 12/28] refactor(web): extract format_ash_error to MvWeb.ErrorHelpers Use shared ErrorHelpers in UserLive.Index for consistent Ash error formatting. --- lib/mv_web/error_helpers.ex | 25 +++++++++++++++++++++++++ lib/mv_web/live/user_live/index.ex | 16 +++------------- 2 files changed, 28 insertions(+), 13 deletions(-) create mode 100644 lib/mv_web/error_helpers.ex diff --git a/lib/mv_web/error_helpers.ex b/lib/mv_web/error_helpers.ex new file mode 100644 index 0000000..dbbc89c --- /dev/null +++ b/lib/mv_web/error_helpers.ex @@ -0,0 +1,25 @@ +defmodule MvWeb.ErrorHelpers do + @moduledoc """ + Shared helpers for formatting errors in the web layer. + + Use `format_ash_error/1` for Ash errors so behaviour stays consistent + (e.g. handling Invalid errors whose entries may lack a `:message` field). + """ + + @doc """ + Formats an Ash error for display to the user. + + Handles `Ash.Error.Invalid` by joining error messages; entries without + a `:message` field are inspected to avoid FunctionClauseError. + Other errors are inspected. + """ + @spec format_ash_error(Ash.Error.t() | term()) :: String.t() + def format_ash_error(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do + Enum.map_join(errors, ", ", fn + %{message: message} when is_binary(message) -> message + other -> inspect(other) + end) + end + + def format_ash_error(error), do: inspect(error) +end diff --git a/lib/mv_web/live/user_live/index.ex b/lib/mv_web/live/user_live/index.ex index ef3583e..4526e4c 100644 --- a/lib/mv_web/live/user_live/index.ex +++ b/lib/mv_web/live/user_live/index.ex @@ -26,6 +26,7 @@ defmodule MvWeb.UserLive.Index do import MvWeb.LiveHelpers, only: [current_actor: 1] require Ash.Query + import MvWeb.ErrorHelpers, only: [format_ash_error: 1] @impl true def mount(_params, _session, socket) do @@ -71,7 +72,7 @@ defmodule MvWeb.UserLive.Index do )} {:error, error} -> - {:noreply, put_flash(socket, :error, format_error(error))} + {:noreply, put_flash(socket, :error, format_ash_error(error))} end {:error, %Ash.Error.Query.NotFound{}} -> @@ -82,7 +83,7 @@ defmodule MvWeb.UserLive.Index do put_flash(socket, :error, gettext("You do not have permission to access this user"))} {:error, error} -> - {:noreply, put_flash(socket, :error, format_error(error))} + {:noreply, put_flash(socket, :error, format_ash_error(error))} end end @@ -144,15 +145,4 @@ defmodule MvWeb.UserLive.Index do defp toggle_order(:desc), do: :asc defp sort_fun(:asc), do: &<=/2 defp sort_fun(:desc), do: &>=/2 - - defp format_error(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do - Enum.map_join(errors, ", ", fn - %{message: message} when is_binary(message) -> message - other -> inspect(other) - end) - end - - defp format_error(error) do - inspect(error) - end end From b5da5774f50a12c91a9da0ab91df7c68e83efc89 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:28 +0100 Subject: [PATCH 13/28] feat(system_actor): add system_user?/1 and normalize email Case-insensitive email comparison for system-actor detection. --- lib/mv/helpers/system_actor.ex | 32 ++++++++++++++++++++++++++- test/mv/helpers/system_actor_test.exs | 8 +++---- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lib/mv/helpers/system_actor.ex b/lib/mv/helpers/system_actor.ex index 565c2ef..8cd93d2 100644 --- a/lib/mv/helpers/system_actor.ex +++ b/lib/mv/helpers/system_actor.ex @@ -172,6 +172,31 @@ defmodule Mv.Helpers.SystemActor do end end + @doc """ + Returns whether the given user is the system actor user (case-insensitive email match). + + Use this instead of ad-hoc `to_string(user.email) == system_user_email()` so + comparisons are consistent and case-insensitive everywhere. + + ## Returns + + - `boolean()` - true if user's email matches system user email (case-insensitive) + + ## Examples + + iex> Mv.Helpers.SystemActor.system_user?(user_with_system_email) + true + iex> Mv.Helpers.SystemActor.system_user?(other_user) + false + + """ + @spec system_user?(Mv.Accounts.User.t() | map() | nil) :: boolean() + def system_user?(%{email: email}) when not is_nil(email) do + normalized_email(to_string(email)) == normalized_system_user_email() + end + + def system_user?(_), do: false + @doc """ Returns the email address of the system user. @@ -191,6 +216,11 @@ defmodule Mv.Helpers.SystemActor do @spec system_user_email() :: String.t() def system_user_email, do: system_user_email_config() + # Case-insensitive normalized form for comparisons + defp normalized_system_user_email, do: normalized_email(system_user_email_config()) + + defp normalized_email(email) when is_binary(email), do: String.downcase(email) + # Returns the system user email from environment variable or default # This allows configuration via SYSTEM_ACTOR_EMAIL env var @spec system_user_email_config() :: String.t() @@ -368,7 +398,7 @@ defmodule Mv.Helpers.SystemActor do upsert_identity: :unique_email, authorize?: false ) - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) |> Ash.load!(:role, domain: Mv.Accounts, authorize?: false) diff --git a/test/mv/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs index e676130..c2715ae 100644 --- a/test/mv/helpers/system_actor_test.exs +++ b/test/mv/helpers/system_actor_test.exs @@ -57,7 +57,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.read_one(domain: Mv.Accounts, authorize?: false) do {:ok, user} when not is_nil(user) -> user - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) |> Ash.load!(:role, domain: Mv.Accounts, authorize?: false) @@ -68,7 +68,7 @@ defmodule Mv.Helpers.SystemActorTest do upsert_identity: :unique_email, authorize?: false ) - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) |> Ash.load!(:role, domain: Mv.Accounts, authorize?: false) @@ -373,9 +373,9 @@ defmodule Mv.Helpers.SystemActorTest do system_actor = SystemActor.get_system_actor() - # Assign wrong role to system user + # Assign wrong role to system user (use :update_internal so bootstrap-style update is allowed) system_user - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, read_only_role, type: :append_and_remove) |> Ash.update!(actor: system_actor) From 62b9a82045c623a2b3b25a089a4ab52be3e4172c Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:31 +0100 Subject: [PATCH 14/28] feat(accounts): block update/destroy on system-actor user Validation prevents modifying system actor user (required for internal ops). --- lib/accounts/user.ex | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 65eef35..875925b 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -175,6 +175,13 @@ defmodule Mv.Accounts.User do end end + # Internal update used only by SystemActor/bootstrap and tests to assign role to system user. + # Not protected by system-user validation so bootstrap can run. + update :update_internal do + accept [] + require_atomic? false + end + # Admin action for direct password changes in admin panel # Uses the official Ash Authentication HashPasswordChange with correct context update :admin_set_password do @@ -366,18 +373,20 @@ defmodule Mv.Accounts.User do end end - # Prevent deletion of the system actor user (required for internal operations) + # Prevent modification of the system actor user (required for internal operations). + # Block update/destroy on UI-exposed actions only; :update_internal is used by bootstrap/tests. validate fn changeset, _context -> - if to_string(changeset.data.email) == Mv.Helpers.SystemActor.system_user_email() do + if Mv.Helpers.SystemActor.system_user?(changeset.data) do {:error, field: :email, message: - "Cannot delete system actor user. This user is required for internal operations."} + "Cannot modify system actor user. This user is required for internal operations."} else :ok end end, - on: [:destroy] + on: [:update, :destroy], + where: [action_is([:update, :update_user, :admin_set_password, :destroy])] end def validate_oidc_id_present(changeset, _context) do From 0df90c369baf2c053d0b1d5481db4ccde549dbaa Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:35 +0100 Subject: [PATCH 15/28] chore(migration): ensure_system_actor_user_exists Use admin_role_id, consistent UUID and timestamps. --- ...0127124937_ensure_system_actor_user_exists.exs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs b/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs index b0ee775..b675cf3 100644 --- a/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs +++ b/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs @@ -41,13 +41,15 @@ defmodule Mv.Repo.Migrations.EnsureSystemActorUserExists do end end - defp ensure_system_actor_user_exists(_admin_role_id) do + defp ensure_system_actor_user_exists(admin_role_id) do case repo().one(from(u in "users", where: u.email == ^@system_user_email, select: u.id)) do nil -> + # Use uuid_generate_v7() for consistency with roles insert in this migration + role_id_str = uuid_to_string(admin_role_id) + execute(""" INSERT INTO users (id, email, hashed_password, oidc_id, member_id, role_id) - SELECT gen_random_uuid(), '#{@system_user_email}', NULL, NULL, NULL, r.id - FROM roles r WHERE r.name = 'Admin' LIMIT 1 + VALUES (uuid_generate_v7(), '#{@system_user_email}', NULL, NULL, NULL, '#{role_id_str}'::uuid) """) IO.puts("✅ Created system actor user (#{@system_user_email})") @@ -56,4 +58,11 @@ defmodule Mv.Repo.Migrations.EnsureSystemActorUserExists do :ok end end + + defp uuid_to_string(id) when is_binary(id) do + {:ok, str} = Ecto.UUID.load(id) + str + end + + defp uuid_to_string(id), do: to_string(id) end From c9d6e0c644282686c7d7b64a6c413207eee0f1c5 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:40 +0100 Subject: [PATCH 16/28] feat(user_live): handle system user in form and show Early return / load_user_or_redirect, use system_user? to avoid editing system actor. --- lib/mv_web/live/user_live/form.ex | 30 ++++++++++++++---------------- lib/mv_web/live/user_live/show.ex | 2 +- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index 28af7c4..0a286c9 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -264,29 +264,27 @@ defmodule MvWeb.UserLive.Form do def mount(params, _session, socket) do actor = current_actor(socket) - user = - case params["id"] do - nil -> - nil + case load_user_or_redirect(params["id"], actor, socket) do + {:redirect, socket} -> + {:ok, socket} - id -> - loaded = - Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) + {:ok, user} -> + mount_continue(user, params, socket) + end + end - if to_string(loaded.email) == Mv.Helpers.SystemActor.system_user_email() do - {:redirect, loaded} - else - loaded - end - end + defp load_user_or_redirect(nil, _actor, _socket), do: {:ok, nil} - if match?({:redirect, _}, user) do - {:ok, + defp load_user_or_redirect(id, actor, socket) do + user = Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) + + if Mv.Helpers.SystemActor.system_user?(user) do + {:redirect, socket |> put_flash(:error, gettext("This user cannot be edited.")) |> push_navigate(to: ~p"/users")} else - mount_continue(user, params, socket) + {:ok, user} end end diff --git a/lib/mv_web/live/user_live/show.ex b/lib/mv_web/live/user_live/show.ex index fe2dd24..e961d84 100644 --- a/lib/mv_web/live/user_live/show.ex +++ b/lib/mv_web/live/user_live/show.ex @@ -75,7 +75,7 @@ defmodule MvWeb.UserLive.Show do actor = current_actor(socket) user = Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) - if to_string(user.email) == Mv.Helpers.SystemActor.system_user_email() do + if Mv.Helpers.SystemActor.system_user?(user) do {:ok, socket |> put_flash(:error, gettext("This user cannot be viewed.")) From ebbb4144a55e27c9aee67acda5abe1428bea7361 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:43 +0100 Subject: [PATCH 17/28] fix(seeds): use :update_internal for system user admin-role :update is blocked for system-actor user; use :update_internal in bootstrap. --- priv/repo/seeds.exs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 1a1f80f..43ffcaf 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -268,9 +268,9 @@ case Accounts.User |> Ash.read_one(domain: Mv.Accounts, authorize?: false) do {:ok, existing_system_user} when not is_nil(existing_system_user) -> # System user already exists - ensure it has admin role - # Use authorize?: false for bootstrap + # Use authorize?: false for bootstrap; :update_internal bypasses system-user modification block existing_system_user - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) @@ -287,7 +287,7 @@ case Accounts.User upsert_identity: :unique_email, authorize?: false ) - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) From acb33b9f3b71b45f6a99446301dba6069baea2f3 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:01 +0100 Subject: [PATCH 18/28] Ensure system actor user exists via migration Creates user system@mila.local with Admin role if missing. Idempotent; guarantees system actor in production without relying on seeds. --- ...124937_ensure_system_actor_user_exists.exs | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) create mode 100644 priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs diff --git a/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs b/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs new file mode 100644 index 0000000..b0ee775 --- /dev/null +++ b/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs @@ -0,0 +1,59 @@ +defmodule Mv.Repo.Migrations.EnsureSystemActorUserExists do + @moduledoc """ + Ensures the system actor user always exists. + + The system actor is used for systemic operations (email sync, cycle generation, + background jobs). It is created by seeds in development; in production seeds + may not run, so this migration guarantees the user exists. + + Creates a user with email "system@mila.local" (default from Mv.Helpers.SystemActor) + and the Admin role. The user has no password and no OIDC ID, so it cannot log in. + """ + use Ecto.Migration + import Ecto.Query + + @system_user_email "system@mila.local" + + def up do + admin_role_id = ensure_admin_role_exists() + ensure_system_actor_user_exists(admin_role_id) + end + + def down do + # Not reversible - do not delete system user on rollback + :ok + end + + defp ensure_admin_role_exists do + case repo().one(from(r in "roles", where: r.name == "Admin", select: r.id)) do + nil -> + execute(""" + INSERT INTO roles (id, name, description, permission_set_name, is_system_role, inserted_at, updated_at) + VALUES (uuid_generate_v7(), 'Admin', 'Administrator with full access', 'admin', false, (now() AT TIME ZONE 'utc'), (now() AT TIME ZONE 'utc')) + """) + + role_id = repo().one(from(r in "roles", where: r.name == "Admin", select: r.id)) + IO.puts("✅ Created 'Admin' role (was missing)") + role_id + + role_id -> + role_id + end + end + + defp ensure_system_actor_user_exists(_admin_role_id) do + case repo().one(from(u in "users", where: u.email == ^@system_user_email, select: u.id)) do + nil -> + execute(""" + INSERT INTO users (id, email, hashed_password, oidc_id, member_id, role_id) + SELECT gen_random_uuid(), '#{@system_user_email}', NULL, NULL, NULL, r.id + FROM roles r WHERE r.name = 'Admin' LIMIT 1 + """) + + IO.puts("✅ Created system actor user (#{@system_user_email})") + + _ -> + :ok + end + end +end From b7f37c80bdf92923358106c9b34c4ae7d8c17cae Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:07 +0100 Subject: [PATCH 19/28] Prevent deletion of system actor user Add destroy validation and explicit destroy action (primary, require_atomic? false). Validation blocks destroy when email == SystemActor.system_user_email(). --- lib/accounts/user.ex | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index bcaf506..65eef35 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -86,7 +86,13 @@ defmodule Mv.Accounts.User do # - :create_user (for manual user creation with optional member link) # - :register_with_password (for password-based registration) # - :register_with_rauthy (for OIDC-based registration) - defaults [:read, :destroy] + defaults [:read] + + destroy :destroy do + primary? true + # Required because custom validation (system actor protection) cannot run atomically + require_atomic? false + end # Primary generic update action: # - Selected by AshAdmin's generated "Edit" UI and generic AshPhoenix @@ -359,6 +365,19 @@ defmodule Mv.Accounts.User do :ok end end + + # Prevent deletion of the system actor user (required for internal operations) + validate fn changeset, _context -> + if to_string(changeset.data.email) == Mv.Helpers.SystemActor.system_user_email() do + {:error, + field: :email, + message: + "Cannot delete system actor user. This user is required for internal operations."} + else + :ok + end + end, + on: [:destroy] end def validate_oidc_id_present(changeset, _context) do From 8ad5201e1a004f22ed77bf9c2f3ca2e9575be653 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:13 +0100 Subject: [PATCH 20/28] Hide system actor from user list and block show/edit Index: filter out SystemActor.system_user_email() in query. Show/Form: redirect to /users with flash when viewing or editing system actor user. Index format_error: handle Ash errors without :message field. --- lib/mv_web/live/user_live/form.ex | 25 +++++++++++++++++++++++-- lib/mv_web/live/user_live/index.ex | 16 +++++++++++++--- lib/mv_web/live/user_live/show.ex | 15 +++++++++++---- 3 files changed, 47 insertions(+), 9 deletions(-) diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index 6cf3f0f..28af7c4 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -266,10 +266,31 @@ defmodule MvWeb.UserLive.Form do user = case params["id"] do - nil -> nil - id -> Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) + nil -> + nil + + id -> + loaded = + Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) + + if to_string(loaded.email) == Mv.Helpers.SystemActor.system_user_email() do + {:redirect, loaded} + else + loaded + end end + if match?({:redirect, _}, user) do + {:ok, + socket + |> put_flash(:error, gettext("This user cannot be edited.")) + |> push_navigate(to: ~p"/users")} + else + mount_continue(user, params, socket) + end + end + + defp mount_continue(user, params, socket) do action = if is_nil(user), do: gettext("New"), else: gettext("Edit") page_title = action <> " " <> gettext("User") diff --git a/lib/mv_web/live/user_live/index.ex b/lib/mv_web/live/user_live/index.ex index 2062ae6..ef3583e 100644 --- a/lib/mv_web/live/user_live/index.ex +++ b/lib/mv_web/live/user_live/index.ex @@ -25,10 +25,17 @@ defmodule MvWeb.UserLive.Index do import MvWeb.LiveHelpers, only: [current_actor: 1] + require Ash.Query + @impl true def mount(_params, _session, socket) do actor = current_actor(socket) - users = Ash.read!(Mv.Accounts.User, domain: Mv.Accounts, load: [:member], actor: actor) + + users = + Mv.Accounts.User + |> Ash.Query.filter(email != ^Mv.Helpers.SystemActor.system_user_email()) + |> Ash.read!(domain: Mv.Accounts, load: [:member], actor: actor) + sorted = Enum.sort_by(users, & &1.email) {:ok, @@ -138,8 +145,11 @@ defmodule MvWeb.UserLive.Index do defp sort_fun(:asc), do: &<=/2 defp sort_fun(:desc), do: &>=/2 - defp format_error(%Ash.Error.Invalid{errors: errors}) do - Enum.map_join(errors, ", ", fn %{message: message} -> message end) + defp format_error(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do + Enum.map_join(errors, ", ", fn + %{message: message} when is_binary(message) -> message + other -> inspect(other) + end) end defp format_error(error) do diff --git a/lib/mv_web/live/user_live/show.ex b/lib/mv_web/live/user_live/show.ex index 641e091..fe2dd24 100644 --- a/lib/mv_web/live/user_live/show.ex +++ b/lib/mv_web/live/user_live/show.ex @@ -75,9 +75,16 @@ defmodule MvWeb.UserLive.Show do actor = current_actor(socket) user = Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) - {:ok, - socket - |> assign(:page_title, gettext("Show User")) - |> assign(:user, user)} + if to_string(user.email) == Mv.Helpers.SystemActor.system_user_email() do + {:ok, + socket + |> put_flash(:error, gettext("This user cannot be viewed.")) + |> push_navigate(to: ~p"/users")} + else + {:ok, + socket + |> assign(:page_title, gettext("Show User")) + |> assign(:user, user)} + end end end From 9c31f0c16c726313e42831c80d28aa3b0d16a686 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:26 +0100 Subject: [PATCH 21/28] Add tests for system actor protection and hiding Index: system actor not in list, destroy returns Ash.Error.Invalid. Show/Form: redirect to /users when viewing or editing system actor user. --- test/mv/helpers/system_actor_test.exs | 24 ++++++++++++++++-------- test/mv_web/live/user_live/show_test.exs | 10 ++++++++++ test/mv_web/user_live/form_test.exs | 10 ++++++++++ test/mv_web/user_live/index_test.exs | 21 +++++++++++++++++++++ 4 files changed, 57 insertions(+), 8 deletions(-) diff --git a/test/mv/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs index af28443..e676130 100644 --- a/test/mv/helpers/system_actor_test.exs +++ b/test/mv/helpers/system_actor_test.exs @@ -10,6 +10,14 @@ defmodule Mv.Helpers.SystemActorTest do require Ash.Query + # Deletes a user row directly via SQL, bypassing Ash validations. + # Use only in tests when setting up "no system user" / "no users" scenarios; + # Ash.destroy! forbids deleting the system actor user. + defp delete_user_bypass_ash(user) do + id = Ecto.UUID.dump!(user.id) + Ecto.Adapters.SQL.query!(Mv.Repo, "DELETE FROM users WHERE id = $1", [id]) + end + # Helper function to ensure admin role exists defp ensure_admin_role do case Authorization.list_roles() do @@ -124,7 +132,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -163,7 +171,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -177,7 +185,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^admin_email) |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -227,7 +235,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -241,7 +249,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^admin_email) |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -275,7 +283,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -314,7 +322,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^"system@mila.local") |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok @@ -328,7 +336,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.Query.filter(email == ^admin_email) |> Ash.read_one(domain: Mv.Accounts, actor: system_actor) do {:ok, user} when not is_nil(user) -> - Ash.destroy!(user, domain: Mv.Accounts, actor: system_actor) + delete_user_bypass_ash(user) _ -> :ok diff --git a/test/mv_web/live/user_live/show_test.exs b/test/mv_web/live/user_live/show_test.exs index 3551fdf..9518106 100644 --- a/test/mv_web/live/user_live/show_test.exs +++ b/test/mv_web/live/user_live/show_test.exs @@ -154,4 +154,14 @@ defmodule MvWeb.UserLive.ShowTest do assert html =~ gettext("Show User") || html =~ to_string(user.email) end end + + describe "system actor user" do + test "redirects to user list when viewing system actor user", %{conn: conn} do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + conn = conn_with_oidc_user(conn) + + assert {:error, {:live_redirect, %{to: "/users"}}} = + live(conn, ~p"/users/#{system_actor.id}") + end + end end diff --git a/test/mv_web/user_live/form_test.exs b/test/mv_web/user_live/form_test.exs index ed309fb..4b76c19 100644 --- a/test/mv_web/user_live/form_test.exs +++ b/test/mv_web/user_live/form_test.exs @@ -420,4 +420,14 @@ defmodule MvWeb.UserLive.FormTest do assert is_nil(updated_user.member) end end + + describe "system actor user" do + test "redirects to user list when editing system actor user", %{conn: conn} do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + conn = conn_with_oidc_user(conn, %{email: "admin@example.com"}) + + assert {:error, {:live_redirect, %{to: "/users"}}} = + live(conn, "/users/#{system_actor.id}/edit") + end + end end diff --git a/test/mv_web/user_live/index_test.exs b/test/mv_web/user_live/index_test.exs index 41c198d..a1a02ea 100644 --- a/test/mv_web/user_live/index_test.exs +++ b/test/mv_web/user_live/index_test.exs @@ -405,6 +405,27 @@ defmodule MvWeb.UserLive.IndexTest do end end + describe "system actor user" do + test "does not show system actor user in list", %{conn: conn} do + # Ensure system actor exists (e.g. via get_system_actor in conn_with_oidc_user) + _system_actor = Mv.Helpers.SystemActor.get_system_actor() + system_email = Mv.Helpers.SystemActor.system_user_email() + + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, "/users") + + refute html =~ system_email, + "System actor user (#{system_email}) must not appear in the user list" + end + + test "destroying system actor user returns error", %{current_user: current_user} do + system_actor = Mv.Helpers.SystemActor.get_system_actor() + + assert {:error, %Ash.Error.Invalid{}} = + Ash.destroy(system_actor, domain: Mv.Accounts, actor: current_user) + end + end + describe "member linking display" do test "displays linked member name in user list", %{conn: conn} do system_actor = Mv.Helpers.SystemActor.get_system_actor() From eb8d78f834f116b75a073dd87561aece346412cf Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 14:29:32 +0100 Subject: [PATCH 22/28] Add gettext strings for system actor show/edit redirect messages German: Dieser Benutzer kann nicht angezeigt/bearbeitet werden. --- priv/gettext/de/LC_MESSAGES/default.po | 10 ++++++++++ priv/gettext/default.pot | 10 ++++++++++ priv/gettext/en/LC_MESSAGES/default.po | 10 ++++++++++ 3 files changed, 30 insertions(+) diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 5496213..865dff4 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -2125,3 +2125,13 @@ msgstr "E-Mail" #, elixir-autogen, elixir-format msgid "email %{email} has already been taken" msgstr "E-Mail %{email} wurde bereits verwendet" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be edited." +msgstr "Dieser Benutzer kann nicht bearbeitet werden." + +#: lib/mv_web/live/user_live/show.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be viewed." +msgstr "Dieser Benutzer kann nicht angezeigt werden." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index fc3a78c..e63621d 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -2126,3 +2126,13 @@ msgstr "" #, elixir-autogen, elixir-format msgid "email %{email} has already been taken" msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be edited." +msgstr "" + +#: lib/mv_web/live/user_live/show.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be viewed." +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 9432a47..15c6f9a 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -2126,3 +2126,13 @@ msgstr "" #, elixir-autogen, elixir-format msgid "email %{email} has already been taken" msgstr "" + +#: lib/mv_web/live/user_live/form.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be edited." +msgstr "" + +#: lib/mv_web/live/user_live/show.ex +#, elixir-autogen, elixir-format +msgid "This user cannot be viewed." +msgstr "" From 41bc031cc6ac93396a9447ef0367afbd1d9a6404 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:23 +0100 Subject: [PATCH 23/28] refactor(web): extract format_ash_error to MvWeb.ErrorHelpers Use shared ErrorHelpers in UserLive.Index for consistent Ash error formatting. --- lib/mv_web/error_helpers.ex | 25 +++++++++++++++++++++++++ lib/mv_web/live/user_live/index.ex | 16 +++------------- 2 files changed, 28 insertions(+), 13 deletions(-) create mode 100644 lib/mv_web/error_helpers.ex diff --git a/lib/mv_web/error_helpers.ex b/lib/mv_web/error_helpers.ex new file mode 100644 index 0000000..dbbc89c --- /dev/null +++ b/lib/mv_web/error_helpers.ex @@ -0,0 +1,25 @@ +defmodule MvWeb.ErrorHelpers do + @moduledoc """ + Shared helpers for formatting errors in the web layer. + + Use `format_ash_error/1` for Ash errors so behaviour stays consistent + (e.g. handling Invalid errors whose entries may lack a `:message` field). + """ + + @doc """ + Formats an Ash error for display to the user. + + Handles `Ash.Error.Invalid` by joining error messages; entries without + a `:message` field are inspected to avoid FunctionClauseError. + Other errors are inspected. + """ + @spec format_ash_error(Ash.Error.t() | term()) :: String.t() + def format_ash_error(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do + Enum.map_join(errors, ", ", fn + %{message: message} when is_binary(message) -> message + other -> inspect(other) + end) + end + + def format_ash_error(error), do: inspect(error) +end diff --git a/lib/mv_web/live/user_live/index.ex b/lib/mv_web/live/user_live/index.ex index ef3583e..4526e4c 100644 --- a/lib/mv_web/live/user_live/index.ex +++ b/lib/mv_web/live/user_live/index.ex @@ -26,6 +26,7 @@ defmodule MvWeb.UserLive.Index do import MvWeb.LiveHelpers, only: [current_actor: 1] require Ash.Query + import MvWeb.ErrorHelpers, only: [format_ash_error: 1] @impl true def mount(_params, _session, socket) do @@ -71,7 +72,7 @@ defmodule MvWeb.UserLive.Index do )} {:error, error} -> - {:noreply, put_flash(socket, :error, format_error(error))} + {:noreply, put_flash(socket, :error, format_ash_error(error))} end {:error, %Ash.Error.Query.NotFound{}} -> @@ -82,7 +83,7 @@ defmodule MvWeb.UserLive.Index do put_flash(socket, :error, gettext("You do not have permission to access this user"))} {:error, error} -> - {:noreply, put_flash(socket, :error, format_error(error))} + {:noreply, put_flash(socket, :error, format_ash_error(error))} end end @@ -144,15 +145,4 @@ defmodule MvWeb.UserLive.Index do defp toggle_order(:desc), do: :asc defp sort_fun(:asc), do: &<=/2 defp sort_fun(:desc), do: &>=/2 - - defp format_error(%Ash.Error.Invalid{errors: errors}) when is_list(errors) do - Enum.map_join(errors, ", ", fn - %{message: message} when is_binary(message) -> message - other -> inspect(other) - end) - end - - defp format_error(error) do - inspect(error) - end end From 7d33acde9fe6cfe83363f773bd84e3928beac097 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:28 +0100 Subject: [PATCH 24/28] feat(system_actor): add system_user?/1 and normalize email Case-insensitive email comparison for system-actor detection. --- lib/mv/helpers/system_actor.ex | 32 ++++++++++++++++++++++++++- test/mv/helpers/system_actor_test.exs | 8 +++---- 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/lib/mv/helpers/system_actor.ex b/lib/mv/helpers/system_actor.ex index 565c2ef..8cd93d2 100644 --- a/lib/mv/helpers/system_actor.ex +++ b/lib/mv/helpers/system_actor.ex @@ -172,6 +172,31 @@ defmodule Mv.Helpers.SystemActor do end end + @doc """ + Returns whether the given user is the system actor user (case-insensitive email match). + + Use this instead of ad-hoc `to_string(user.email) == system_user_email()` so + comparisons are consistent and case-insensitive everywhere. + + ## Returns + + - `boolean()` - true if user's email matches system user email (case-insensitive) + + ## Examples + + iex> Mv.Helpers.SystemActor.system_user?(user_with_system_email) + true + iex> Mv.Helpers.SystemActor.system_user?(other_user) + false + + """ + @spec system_user?(Mv.Accounts.User.t() | map() | nil) :: boolean() + def system_user?(%{email: email}) when not is_nil(email) do + normalized_email(to_string(email)) == normalized_system_user_email() + end + + def system_user?(_), do: false + @doc """ Returns the email address of the system user. @@ -191,6 +216,11 @@ defmodule Mv.Helpers.SystemActor do @spec system_user_email() :: String.t() def system_user_email, do: system_user_email_config() + # Case-insensitive normalized form for comparisons + defp normalized_system_user_email, do: normalized_email(system_user_email_config()) + + defp normalized_email(email) when is_binary(email), do: String.downcase(email) + # Returns the system user email from environment variable or default # This allows configuration via SYSTEM_ACTOR_EMAIL env var @spec system_user_email_config() :: String.t() @@ -368,7 +398,7 @@ defmodule Mv.Helpers.SystemActor do upsert_identity: :unique_email, authorize?: false ) - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) |> Ash.load!(:role, domain: Mv.Accounts, authorize?: false) diff --git a/test/mv/helpers/system_actor_test.exs b/test/mv/helpers/system_actor_test.exs index e676130..c2715ae 100644 --- a/test/mv/helpers/system_actor_test.exs +++ b/test/mv/helpers/system_actor_test.exs @@ -57,7 +57,7 @@ defmodule Mv.Helpers.SystemActorTest do |> Ash.read_one(domain: Mv.Accounts, authorize?: false) do {:ok, user} when not is_nil(user) -> user - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) |> Ash.load!(:role, domain: Mv.Accounts, authorize?: false) @@ -68,7 +68,7 @@ defmodule Mv.Helpers.SystemActorTest do upsert_identity: :unique_email, authorize?: false ) - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) |> Ash.load!(:role, domain: Mv.Accounts, authorize?: false) @@ -373,9 +373,9 @@ defmodule Mv.Helpers.SystemActorTest do system_actor = SystemActor.get_system_actor() - # Assign wrong role to system user + # Assign wrong role to system user (use :update_internal so bootstrap-style update is allowed) system_user - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, read_only_role, type: :append_and_remove) |> Ash.update!(actor: system_actor) From d98b32af8d66cf377b9037003507219b518d8c73 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:31 +0100 Subject: [PATCH 25/28] feat(accounts): block update/destroy on system-actor user Validation prevents modifying system actor user (required for internal ops). --- lib/accounts/user.ex | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/accounts/user.ex b/lib/accounts/user.ex index 65eef35..875925b 100644 --- a/lib/accounts/user.ex +++ b/lib/accounts/user.ex @@ -175,6 +175,13 @@ defmodule Mv.Accounts.User do end end + # Internal update used only by SystemActor/bootstrap and tests to assign role to system user. + # Not protected by system-user validation so bootstrap can run. + update :update_internal do + accept [] + require_atomic? false + end + # Admin action for direct password changes in admin panel # Uses the official Ash Authentication HashPasswordChange with correct context update :admin_set_password do @@ -366,18 +373,20 @@ defmodule Mv.Accounts.User do end end - # Prevent deletion of the system actor user (required for internal operations) + # Prevent modification of the system actor user (required for internal operations). + # Block update/destroy on UI-exposed actions only; :update_internal is used by bootstrap/tests. validate fn changeset, _context -> - if to_string(changeset.data.email) == Mv.Helpers.SystemActor.system_user_email() do + if Mv.Helpers.SystemActor.system_user?(changeset.data) do {:error, field: :email, message: - "Cannot delete system actor user. This user is required for internal operations."} + "Cannot modify system actor user. This user is required for internal operations."} else :ok end end, - on: [:destroy] + on: [:update, :destroy], + where: [action_is([:update, :update_user, :admin_set_password, :destroy])] end def validate_oidc_id_present(changeset, _context) do From a10c770ca7d72c56aa929c22e85a445f8ce9d448 Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:35 +0100 Subject: [PATCH 26/28] chore(migration): ensure_system_actor_user_exists Use admin_role_id, consistent UUID and timestamps. --- ...0127124937_ensure_system_actor_user_exists.exs | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs b/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs index b0ee775..b675cf3 100644 --- a/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs +++ b/priv/repo/migrations/20260127124937_ensure_system_actor_user_exists.exs @@ -41,13 +41,15 @@ defmodule Mv.Repo.Migrations.EnsureSystemActorUserExists do end end - defp ensure_system_actor_user_exists(_admin_role_id) do + defp ensure_system_actor_user_exists(admin_role_id) do case repo().one(from(u in "users", where: u.email == ^@system_user_email, select: u.id)) do nil -> + # Use uuid_generate_v7() for consistency with roles insert in this migration + role_id_str = uuid_to_string(admin_role_id) + execute(""" INSERT INTO users (id, email, hashed_password, oidc_id, member_id, role_id) - SELECT gen_random_uuid(), '#{@system_user_email}', NULL, NULL, NULL, r.id - FROM roles r WHERE r.name = 'Admin' LIMIT 1 + VALUES (uuid_generate_v7(), '#{@system_user_email}', NULL, NULL, NULL, '#{role_id_str}'::uuid) """) IO.puts("✅ Created system actor user (#{@system_user_email})") @@ -56,4 +58,11 @@ defmodule Mv.Repo.Migrations.EnsureSystemActorUserExists do :ok end end + + defp uuid_to_string(id) when is_binary(id) do + {:ok, str} = Ecto.UUID.load(id) + str + end + + defp uuid_to_string(id), do: to_string(id) end From cbcb93418ea13963c3d7f5b023696c45ff813a2f Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:40 +0100 Subject: [PATCH 27/28] feat(user_live): handle system user in form and show Early return / load_user_or_redirect, use system_user? to avoid editing system actor. --- lib/mv_web/live/user_live/form.ex | 30 ++++++++++++++---------------- lib/mv_web/live/user_live/show.ex | 2 +- 2 files changed, 15 insertions(+), 17 deletions(-) diff --git a/lib/mv_web/live/user_live/form.ex b/lib/mv_web/live/user_live/form.ex index 28af7c4..0a286c9 100644 --- a/lib/mv_web/live/user_live/form.ex +++ b/lib/mv_web/live/user_live/form.ex @@ -264,29 +264,27 @@ defmodule MvWeb.UserLive.Form do def mount(params, _session, socket) do actor = current_actor(socket) - user = - case params["id"] do - nil -> - nil + case load_user_or_redirect(params["id"], actor, socket) do + {:redirect, socket} -> + {:ok, socket} - id -> - loaded = - Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) + {:ok, user} -> + mount_continue(user, params, socket) + end + end - if to_string(loaded.email) == Mv.Helpers.SystemActor.system_user_email() do - {:redirect, loaded} - else - loaded - end - end + defp load_user_or_redirect(nil, _actor, _socket), do: {:ok, nil} - if match?({:redirect, _}, user) do - {:ok, + defp load_user_or_redirect(id, actor, socket) do + user = Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) + + if Mv.Helpers.SystemActor.system_user?(user) do + {:redirect, socket |> put_flash(:error, gettext("This user cannot be edited.")) |> push_navigate(to: ~p"/users")} else - mount_continue(user, params, socket) + {:ok, user} end end diff --git a/lib/mv_web/live/user_live/show.ex b/lib/mv_web/live/user_live/show.ex index fe2dd24..e961d84 100644 --- a/lib/mv_web/live/user_live/show.ex +++ b/lib/mv_web/live/user_live/show.ex @@ -75,7 +75,7 @@ defmodule MvWeb.UserLive.Show do actor = current_actor(socket) user = Ash.get!(Mv.Accounts.User, id, domain: Mv.Accounts, load: [:member], actor: actor) - if to_string(user.email) == Mv.Helpers.SystemActor.system_user_email() do + if Mv.Helpers.SystemActor.system_user?(user) do {:ok, socket |> put_flash(:error, gettext("This user cannot be viewed.")) From 92ee7fcc6306eb10491474883165235452ff6b9a Mon Sep 17 00:00:00 2001 From: Moritz Date: Tue, 27 Jan 2026 17:37:43 +0100 Subject: [PATCH 28/28] fix(seeds): use :update_internal for system user admin-role :update is blocked for system-actor user; use :update_internal in bootstrap. --- priv/repo/seeds.exs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 1a1f80f..43ffcaf 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -268,9 +268,9 @@ case Accounts.User |> Ash.read_one(domain: Mv.Accounts, authorize?: false) do {:ok, existing_system_user} when not is_nil(existing_system_user) -> # System user already exists - ensure it has admin role - # Use authorize?: false for bootstrap + # Use authorize?: false for bootstrap; :update_internal bypasses system-user modification block existing_system_user - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false) @@ -287,7 +287,7 @@ case Accounts.User upsert_identity: :unique_email, authorize?: false ) - |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.for_update(:update_internal, %{}) |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!(authorize?: false)