From e7bf777be2a98159582ae3f0573326bc53c6cde9 Mon Sep 17 00:00:00 2001 From: Moritz Date: Sun, 25 Jan 2026 13:39:16 +0100 Subject: [PATCH] refactor: remove AssignDefaultRole change module The attribute-level default solution makes this change module obsolete. All role assignment is now handled via the role_id attribute's default function, which is more robust and works for all creation paths. --- .../user/changes/assign_default_role.ex | 103 ----------- .../user/changes/assign_default_role_test.exs | 169 ------------------ 2 files changed, 272 deletions(-) delete mode 100644 lib/accounts/user/changes/assign_default_role.ex delete mode 100644 test/accounts/user/changes/assign_default_role_test.exs diff --git a/lib/accounts/user/changes/assign_default_role.ex b/lib/accounts/user/changes/assign_default_role.ex deleted file mode 100644 index 65cddca..0000000 --- a/lib/accounts/user/changes/assign_default_role.ex +++ /dev/null @@ -1,103 +0,0 @@ -defmodule Mv.Accounts.User.Changes.AssignDefaultRole do - @moduledoc """ - Assigns the default "Mitglied" role to new users if no role is explicitly provided. - - This change runs during user creation actions (`:create_user`, `:register_with_password`, - `:register_with_rauthy`) and ensures that all users have a role assigned. - - ## Behavior - - - Skips assignment if `role_id` is already set in the changeset, data, or arguments - - Loads the "Mitglied" role without authorization (safe for system operations) - - Returns unchanged changeset if "Mitglied" role doesn't exist (test environments) - - Adds error to changeset on unexpected failures - - ## Important Notes - - - Works with upserts: When combined with `upsert_fields`, only new users get the role - - Uses `authorize?: false` to avoid circular dependencies during user creation - - The "Mitglied" role must exist (created by seeds or migration) - - ## Examples - - # Automatically assigns "Mitglied" role during user creation: - {:ok, user} = - User - |> Ash.Changeset.for_create(:create_user, %{email: "new@example.com"}) - |> Ash.create() - - # User now has "Mitglied" role assigned - {:ok, user_with_role} = Ash.load(user, :role) - assert user_with_role.role.name == "Mitglied" - - # Skips assignment if role is already set: - {:ok, user} = - User - |> Ash.Changeset.for_create(:create_user, %{email: "admin@example.com", role_id: admin_role.id}) - |> Ash.create() - - # User has the explicitly set role, not "Mitglied" - {:ok, user_with_role} = Ash.load(user, :role) - assert user_with_role.role.name == "Admin" - """ - use Ash.Resource.Change - - @impl true - @spec change(Ash.Changeset.t(), keyword(), map()) :: Ash.Changeset.t() - def change(changeset, _opts, _context) do - # Check role_id in changeset attributes (for new assignments) - role_id_in_changeset = Ash.Changeset.get_attribute(changeset, :role_id) - - # Check role_id in existing data (for upserts) - role_id_in_data = Map.get(changeset.data, :role_id) - - # Check if role is being set via argument - role_arg = Ash.Changeset.get_argument(changeset, :role) - - # Check if role relationship is already being managed - # Relationships are stored as a list of tuples: [{record_or_changes, opts}] - role_relationship = Map.get(changeset.relationships || %{}, :role) - - # Skip if role is already set anywhere (changeset, data, argument, or relationship) - has_role = - not is_nil(role_id_in_changeset) or - not is_nil(role_id_in_data) or - not is_nil(role_arg) or - (not is_nil(role_relationship) and role_relationship != []) - - if has_role do - changeset - else - assign_default_role(changeset) - end - end - - @spec assign_default_role(Ash.Changeset.t()) :: Ash.Changeset.t() - defp assign_default_role(changeset) do - # Load the "Mitglied" role without authorization - # This is safe because: - # 1. We're only reading a public system role (no sensitive data) - # 2. This runs during user creation (bootstrap phase) - # 3. Using SystemActor here would create circular dependency (SystemActor needs a user) - case Mv.Authorization.Role.get_mitglied_role() do - {:ok, %Mv.Authorization.Role{} = mitglied_role} -> - # Assign the role using manage_relationship - # Note: :append_and_remove is the correct type for Ash 2.0+ (replaces :replace) - Ash.Changeset.manage_relationship(changeset, :role, mitglied_role, - type: :append_and_remove - ) - - {:ok, nil} -> - # Role doesn't exist - skip assignment (common in test environments) - # In production, the migration will have created the role - changeset - - {:error, error} -> - # Unexpected error during role lookup - Ash.Changeset.add_error(changeset, - field: :role_id, - message: "Failed to load default role: #{inspect(error)}" - ) - end - end -end diff --git a/test/accounts/user/changes/assign_default_role_test.exs b/test/accounts/user/changes/assign_default_role_test.exs deleted file mode 100644 index 1de5e61..0000000 --- a/test/accounts/user/changes/assign_default_role_test.exs +++ /dev/null @@ -1,169 +0,0 @@ -defmodule Mv.Accounts.User.Changes.AssignDefaultRoleTest do - @moduledoc """ - Tests for AssignDefaultRole change module. - - Tests cover: - - Automatic role assignment when no role is set - - Skipping assignment when role is already set (changeset, data, or argument) - - Handling missing "Mitglied" role gracefully - - Error handling for unexpected failures - """ - use Mv.DataCase, async: true - - alias Mv.Accounts.User - alias Mv.Authorization.Role - - setup do - # Ensure "Mitglied" role exists - Mv.DataCase.ensure_default_role() - - # Get "Mitglied" role for assertions - {:ok, mitglied_role} = Role.get_mitglied_role() - - %{mitglied_role: mitglied_role} - end - - describe "change/3" do - test "assigns Mitglied role when no role is set", %{mitglied_role: mitglied_role} do - email = "test#{System.unique_integer([:positive])}@example.com" - - # Create user - AssignDefaultRole change runs automatically via :create_user action - {:ok, user} = - User - |> Ash.Changeset.for_create(:create_user, %{email: email}) - |> Ash.create(authorize?: false, domain: Mv.Accounts) - - # Load user with role - {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) - - # Verify role was assigned - assert user_with_role.role != nil - assert user_with_role.role.id == mitglied_role.id - assert user_with_role.role.name == "Mitglied" - end - - test "skips assignment when role relationship is already set in changeset", %{ - mitglied_role: _mitglied_role - } do - # Create a different role - other_role = - Role - |> Ash.Changeset.for_create(:create_role, %{ - name: "Test Role #{System.unique_integer([:positive])}", - description: "Test role", - permission_set_name: "own_data" - }) - |> Ash.create!(authorize?: false, domain: Mv.Authorization) - - # Test that AssignDefaultRole skips when role relationship is already set - # Create a changeset with role relationship already set BEFORE the change runs - changeset = - User - |> Ash.Changeset.for_create(:create_user, %{ - email: "test#{System.unique_integer([:positive])}@example.com" - }) - |> Ash.Changeset.manage_relationship(:role, other_role, type: :append_and_remove) - - # Manually call the change to test it - result_changeset = Mv.Accounts.User.Changes.AssignDefaultRole.change(changeset, [], %{}) - - # The change should detect that role relationship is already set and skip assignment - # Verify by creating the user and checking the role - {:ok, user} = Ash.create(result_changeset, authorize?: false, domain: Mv.Accounts) - - # Load user with role - {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) - - # Verify the explicitly set role was used, not "Mitglied" - assert user_with_role.role != nil - assert user_with_role.role.id == other_role.id - assert user_with_role.role.name != "Mitglied" - end - - test "skips assignment when role_id is already set in data (upsert scenario)" do - # Create user with role - role = - Role - |> Ash.Changeset.for_create(:create_role, %{ - name: "Test Role #{System.unique_integer([:positive])}", - description: "Test role", - permission_set_name: "own_data" - }) - |> Ash.create!(authorize?: false, domain: Mv.Authorization) - - {:ok, existing_user} = - User - |> Ash.Changeset.for_create(:create_user, %{ - email: "existing#{System.unique_integer([:positive])}@example.com" - }) - |> Ash.create(authorize?: false, domain: Mv.Accounts) - - # Update user to have role - {:ok, user_with_role} = - existing_user - |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, role, type: :append_and_remove) - |> Ash.update(authorize?: false, domain: Mv.Accounts) - - # Reload to get role_id in data - {:ok, user_with_role} = - Ash.load(user_with_role, :role, domain: Mv.Accounts, authorize?: false) - - # Verify user has the explicitly set role - assert user_with_role.role != nil - assert user_with_role.role.id == role.id - - # Now update user again - AssignDefaultRole should skip (role already set) - {:ok, updated_user} = - user_with_role - |> Ash.Changeset.for_update(:update, %{}) - |> Ash.update(authorize?: false, domain: Mv.Accounts) - - # Reload to verify role didn't change - {:ok, updated_user_with_role} = - Ash.load(updated_user, :role, domain: Mv.Accounts, authorize?: false) - - # Role should still be the same (not changed to "Mitglied") - assert updated_user_with_role.role.id == role.id - assert updated_user_with_role.role.name != "Mitglied" - end - - test "handles missing Mitglied role gracefully" do - # Test that change handles nil role gracefully - # Since we can't easily delete the system role, we test the code path - # by verifying that when get_mitglied_role returns nil, changeset is unchanged - changeset = - User - |> Ash.Changeset.for_create(:create_user, %{ - email: "test#{System.unique_integer([:positive])}@example.com" - }) - - # The change should handle nil role gracefully - # If role exists, it will be assigned; if not, changeset remains unchanged - result_changeset = Mv.Accounts.User.Changes.AssignDefaultRole.change(changeset, [], %{}) - - # Changeset should be valid regardless - assert result_changeset.valid? - - # If role exists, it should be assigned (we check this in other tests) - # If role doesn't exist, changeset should be unchanged (no error) - end - - test "assigns role correctly in integration test" do - email = "integration#{System.unique_integer([:positive])}@example.com" - - {:ok, user} = - User - |> Ash.Changeset.for_create(:create_user, %{email: email}) - |> Ash.create(authorize?: false, domain: Mv.Accounts) - - # Load user with role - {:ok, user_with_role} = Ash.load(user, :role, domain: Mv.Accounts, authorize?: false) - - # Verify role was assigned - assert user_with_role.role != nil - assert user_with_role.role.name == "Mitglied" - assert user_with_role.role.permission_set_name == "own_data" - end - end -end