From b5588fe66e7467e4a80f4448373f686eb2d2aeb0 Mon Sep 17 00:00:00 2001 From: Moritz Date: Mon, 20 Oct 2025 14:38:00 +0200 Subject: [PATCH] fix validation behaviour --- .../email_not_used_by_other_member.ex | 40 +- .../email_not_used_by_other_user.ex | 31 +- test/accounts/email_uniqueness_test.exs | 357 ++++++++++++++++-- 3 files changed, 369 insertions(+), 59 deletions(-) diff --git a/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex b/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex index d3cb776..d42b2c1 100644 --- a/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex +++ b/lib/mv/accounts/user/validations/email_not_used_by_other_member.ex @@ -1,26 +1,46 @@ defmodule Mv.Accounts.User.Validations.EmailNotUsedByOtherMember do @moduledoc """ Validates that the user's email is not already used by another member. - Allows syncing with linked member (excludes member_id from check). + Only validates when: + - User is already linked to a member (member_id != nil) AND email is changing + - User is being linked to a member (member relationship is changing) + + This allows creating users with the same email as unlinked members. """ use Ash.Resource.Validation @impl true def validate(changeset, _opts, _context) do - case Ash.Changeset.fetch_change(changeset, :email) do - {:ok, new_email} -> - member_id = Ash.Changeset.get_attribute(changeset, :member_id) - check_email_uniqueness(new_email, member_id) + email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) + member_changing? = Ash.Changeset.changing_relationship?(changeset, :member) - :error -> - :ok + member_id = Ash.Changeset.get_attribute(changeset, :member_id) + is_linked? = not is_nil(member_id) + + # Only validate if: + # 1. User is linked AND email is changing + # 2. User is being linked/unlinked (member relationship changing) + should_validate? = (is_linked? and email_changing?) or member_changing? + + if should_validate? do + case Ash.Changeset.fetch_change(changeset, :email) do + {:ok, new_email} -> + check_email_uniqueness(new_email, member_id) + + :error -> + # No email change, get current email + current_email = Ash.Changeset.get_attribute(changeset, :email) + check_email_uniqueness(current_email, member_id) + end + else + :ok end end - defp check_email_uniqueness(new_email, exclude_member_id) do + defp check_email_uniqueness(email, exclude_member_id) do query = Mv.Membership.Member - |> Ash.Query.filter(email == ^to_string(new_email)) + |> Ash.Query.filter(email == ^to_string(email)) |> maybe_exclude_id(exclude_member_id) case Ash.read(query) do @@ -28,7 +48,7 @@ defmodule Mv.Accounts.User.Validations.EmailNotUsedByOtherMember do :ok {:ok, _} -> - {:error, field: :email, message: "is already used by another member", value: new_email} + {:error, field: :email, message: "is already used by another member", value: email} {:error, _} -> :ok diff --git a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex index 6c544b5..54fa243 100644 --- a/lib/mv/membership/member/validations/email_not_used_by_other_user.ex +++ b/lib/mv/membership/member/validations/email_not_used_by_other_user.ex @@ -1,26 +1,37 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do @moduledoc """ Validates that the member's email is not already used by another user. - Allows syncing with linked user (excludes linked user from check). + Only validates when: + - Member is already linked to a user (user != nil) AND email is changing + - Member is being linked to a user (user relationship is changing) + + This allows creating members with the same email as unlinked users. """ use Ash.Resource.Validation @impl true def validate(changeset, _opts, _context) do - case Ash.Changeset.fetch_change(changeset, :email) do - {:ok, new_email} -> - linked_user_id = get_linked_user_id(changeset.data) - check_email_uniqueness(new_email, linked_user_id) + email_changing? = Ash.Changeset.changing_attribute?(changeset, :email) - :error -> - :ok + linked_user_id = get_linked_user_id(changeset.data) + is_linked? = not is_nil(linked_user_id) + + # Only validate if member is already linked AND email is changing + # Do NOT validate when member is being linked (email will be overridden from user) + should_validate? = is_linked? and email_changing? + + if should_validate? do + new_email = Ash.Changeset.get_attribute(changeset, :email) + check_email_uniqueness(new_email, linked_user_id) + else + :ok end end - defp check_email_uniqueness(new_email, exclude_user_id) do + defp check_email_uniqueness(email, exclude_user_id) do query = Mv.Accounts.User - |> Ash.Query.filter(email == ^new_email) + |> Ash.Query.filter(email == ^email) |> maybe_exclude_id(exclude_user_id) case Ash.read(query) do @@ -28,7 +39,7 @@ defmodule Mv.Membership.Member.Validations.EmailNotUsedByOtherUser do :ok {:ok, _} -> - {:error, field: :email, message: "is already used by another user", value: new_email} + {:error, field: :email, message: "is already used by another user", value: email} {:error, _} -> :ok diff --git a/test/accounts/email_uniqueness_test.exs b/test/accounts/email_uniqueness_test.exs index 8665c48..a16ebdd 100644 --- a/test/accounts/email_uniqueness_test.exs +++ b/test/accounts/email_uniqueness_test.exs @@ -4,32 +4,26 @@ defmodule Mv.Accounts.EmailUniquenessTest do alias Mv.Accounts alias Mv.Membership - describe "Email uniqueness validation" do - test "cannot create member with existing unlinked user email" do + describe "Email uniqueness validation - Creation" do + test "CAN create member with existing unlinked user email" do # Create a user with email {:ok, _user} = Accounts.create_user(%{ email: "existing@example.com" }) - # Try to create member with same email - result = + # Create member with same email - should succeed + {:ok, member} = Membership.create_member(%{ first_name: "John", last_name: "Doe", email: "existing@example.com" }) - assert {:error, %Ash.Error.Invalid{} = error} = result - - assert error.errors - |> Enum.any?(fn e -> - e.field == :email and - (String.contains?(e.message, "already") or String.contains?(e.message, "used")) - end) + assert to_string(member.email) == "existing@example.com" end - test "cannot create user with existing unlinked member email" do + test "CAN create user with existing unlinked member email" do # Create a member with email {:ok, _member} = Membership.create_member(%{ @@ -38,29 +32,25 @@ defmodule Mv.Accounts.EmailUniquenessTest do email: "existing@example.com" }) - # Try to create user with same email - result = + # Create user with same email - should succeed + {:ok, user} = Accounts.create_user(%{ email: "existing@example.com" }) - assert {:error, %Ash.Error.Invalid{} = error} = result - - assert error.errors - |> Enum.any?(fn e -> - e.field == :email and - (String.contains?(e.message, "already") or String.contains?(e.message, "used")) - end) + assert to_string(user.email) == "existing@example.com" end + end - test "member email cannot be changed to an existing unlinked user email" do + describe "Email uniqueness validation - Updating unlinked entities" do + test "unlinked member email CAN be changed to an existing unlinked user email" do # Create a user with email - {:ok, user} = + {:ok, _user} = Accounts.create_user(%{ email: "existing_user@example.com" }) - # Create a member with different email + # Create an unlinked member with different email {:ok, member} = Membership.create_member(%{ first_name: "John", @@ -68,42 +58,68 @@ defmodule Mv.Accounts.EmailUniquenessTest do email: "member@example.com" }) - # Try to change member email to existing user email - result = + # Change member email to existing user email - should succeed (member is unlinked) + {:ok, updated_member} = Membership.update_member(member, %{ email: "existing_user@example.com" }) - assert {:error, %Ash.Error.Invalid{} = error} = result - - assert error.errors - |> Enum.any?(fn e -> - e.field == :email and - (String.contains?(e.message, "already") or String.contains?(e.message, "used")) - end) + assert to_string(updated_member.email) == "existing_user@example.com" end - test "user email cannot be changed to an existing unlinked member email" do + test "unlinked user email CAN be changed to an existing unlinked member email" do # Create a member with email - {:ok, member} = + {:ok, _member} = Membership.create_member(%{ first_name: "John", last_name: "Doe", email: "existing_member@example.com" }) - # Create a user with different email + # Create an unlinked user with different email {:ok, user} = Accounts.create_user(%{ email: "user@example.com" }) - # Try to change user email to existing member email - result = + # Change user email to existing member email - should succeed (user is unlinked) + {:ok, updated_user} = Accounts.update_user(user, %{ email: "existing_member@example.com" }) + assert to_string(updated_user.email) == "existing_member@example.com" + end + + test "unlinked member email CANNOT be changed to an existing linked user email" do + # Create a user and link it to a member - this makes the user "linked" + {:ok, user} = + Accounts.create_user(%{ + email: "linked_user@example.com" + }) + + {:ok, _member_a} = + Membership.create_member(%{ + first_name: "Member", + last_name: "A", + email: "temp@example.com", + user: %{id: user.id} + }) + + # Create an unlinked member with different email + {:ok, member_b} = + Membership.create_member(%{ + first_name: "Member", + last_name: "B", + email: "member_b@example.com" + }) + + # Try to change unlinked member's email to linked user's email - should fail + result = + Membership.update_member(member_b, %{ + email: "linked_user@example.com" + }) + assert {:error, %Ash.Error.Invalid{} = error} = result assert error.errors @@ -113,6 +129,269 @@ defmodule Mv.Accounts.EmailUniquenessTest do end) end + test "unlinked user email CANNOT be changed to an existing linked member email" do + # Create a user and link it to a member - this makes the member "linked" + {:ok, user_a} = + Accounts.create_user(%{ + email: "user_a@example.com" + }) + + {:ok, _member_a} = + Membership.create_member(%{ + first_name: "Member", + last_name: "A", + email: "temp@example.com", + user: %{id: user_a.id} + }) + + # Reload user to get updated member_id and linked member email + {:ok, user_a_reloaded} = Ash.get(Mv.Accounts.User, user_a.id) + {:ok, user_a_with_member} = Ash.load(user_a_reloaded, :member) + linked_member_email = to_string(user_a_with_member.member.email) + + # Create an unlinked user with different email + {:ok, user_b} = + Accounts.create_user(%{ + email: "user_b@example.com" + }) + + # Try to change unlinked user's email to linked member's email - should fail + result = + Accounts.update_user(user_b, %{ + email: linked_member_email + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + end + + describe "Email uniqueness validation - Creating with linked emails" do + test "CANNOT create member with existing linked user email" do + # Create a user and link it to a member + {:ok, user} = + Accounts.create_user(%{ + email: "linked@example.com" + }) + + {:ok, _member} = + Membership.create_member(%{ + first_name: "First", + last_name: "Member", + email: "temp@example.com", + user: %{id: user.id} + }) + + # Try to create a new member with the linked user's email - should fail + result = + Membership.create_member(%{ + first_name: "Second", + last_name: "Member", + email: "linked@example.com" + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + + test "CANNOT create user with existing linked member email" do + # Create a user and link it to a member + {:ok, user} = + Accounts.create_user(%{ + email: "user@example.com" + }) + + {:ok, _member} = + Membership.create_member(%{ + first_name: "Member", + last_name: "One", + email: "temp@example.com", + user: %{id: user.id} + }) + + # Reload user to get the linked member's email + {:ok, user_reloaded} = Ash.get(Mv.Accounts.User, user.id) + {:ok, user_with_member} = Ash.load(user_reloaded, :member) + linked_member_email = to_string(user_with_member.member.email) + + # Try to create a new user with the linked member's email - should fail + result = + Accounts.create_user(%{ + email: linked_member_email + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + end + + describe "Email uniqueness validation - Updating linked entities" do + test "linked member email CANNOT be changed to an existing user email" do + # Create a user with email + {:ok, _other_user} = + Accounts.create_user(%{ + email: "other_user@example.com" + }) + + # Create a user and link it to a member + {:ok, user} = + Accounts.create_user(%{ + email: "user@example.com" + }) + + {:ok, member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "temp@example.com", + user: %{id: user.id} + }) + + # Try to change linked member's email to other user's email - should fail + result = + Membership.update_member(member, %{ + email: "other_user@example.com" + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + + test "linked user email CANNOT be changed to an existing member email" do + # Create a member with email + {:ok, _other_member} = + Membership.create_member(%{ + first_name: "Jane", + last_name: "Doe", + email: "other_member@example.com" + }) + + # Create a user and link it to a member + {:ok, user} = + Accounts.create_user(%{ + email: "user@example.com" + }) + + {:ok, _member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "temp@example.com", + user: %{id: user.id} + }) + + # Reload user to get updated member_id + {:ok, user_reloaded} = Ash.get(Mv.Accounts.User, user.id) + + # Try to change linked user's email to other member's email - should fail + result = + Accounts.update_user(user_reloaded, %{ + email: "other_member@example.com" + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + end + + describe "Email uniqueness validation - Linking" do + test "CANNOT link user to member if user email is already used by another unlinked member" do + # Create a member with email + {:ok, _other_member} = + Membership.create_member(%{ + first_name: "Jane", + last_name: "Doe", + email: "duplicate@example.com" + }) + + # Create a user with same email + {:ok, user} = + Accounts.create_user(%{ + email: "duplicate@example.com" + }) + + # Create a member to link with the user + {:ok, member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Smith", + email: "john@example.com" + }) + + # Try to link user to member - should fail because user.email is already used by other_member + result = + Accounts.update_user(user, %{ + member: %{id: member.id} + }) + + assert {:error, %Ash.Error.Invalid{} = error} = result + + assert error.errors + |> Enum.any?(fn e -> + e.field == :email and + (String.contains?(e.message, "already") or String.contains?(e.message, "used")) + end) + end + + test "CAN link member to user even if member email is used by another user (member email gets overridden)" do + # Create a user with email + {:ok, _other_user} = + Accounts.create_user(%{ + email: "duplicate@example.com" + }) + + # Create a member with same email + {:ok, member} = + Membership.create_member(%{ + first_name: "John", + last_name: "Doe", + email: "duplicate@example.com" + }) + + # Create a user to link with the member + {:ok, user} = + Accounts.create_user(%{ + email: "user@example.com" + }) + + # Link member to user - should succeed because member.email will be overridden + {:ok, updated_member} = + Membership.update_member(member, %{ + user: %{id: user.id} + }) + + # Member email should now be the same as user email + {:ok, member_reloaded} = Ash.get(Mv.Membership.Member, updated_member.id) + assert to_string(member_reloaded.email) == "user@example.com" + end + end + + describe "Email syncing" do test "member email syncs to linked user email without validation error" do # Create a user {:ok, user} = @@ -148,7 +427,7 @@ defmodule Mv.Accounts.EmailUniquenessTest do # Create a user linked to this member # The override change will set member.email = user.email automatically - {:ok, user} = + {:ok, _user} = Accounts.create_user(%{ email: "user@example.com", member: %{id: member.id}