From 584442076e67eda9b95a45e930d45ed3781e5215 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 19 Jan 2026 12:47:17 +0100 Subject: [PATCH 1/3] fix: add error message to form --- lib/mv_web/live/member_live/form.ex | 83 ++++++++++- .../member_live/form_error_handling_test.exs | 140 ++++++++++++++++++ 2 files changed, 222 insertions(+), 1 deletion(-) create mode 100644 test/mv_web/member_live/form_error_handling_test.exs diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index 3c68b21..d384a45 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -295,11 +295,14 @@ defmodule MvWeb.MemberLive.Form do handle_save_success(socket, member) {:error, form} -> - {:noreply, assign(socket, form: form)} + handle_save_error(socket, form) end rescue _e in [Ash.Error.Forbidden, Ash.Error.Forbidden.Policy] -> handle_save_forbidden(socket) + + e -> + handle_save_exception(socket, e) end end @@ -321,6 +324,13 @@ defmodule MvWeb.MemberLive.Form do {:noreply, socket} end + defp handle_save_error(socket, form) do + # Always show a flash message when save fails + # Field-level validation errors are displayed in form fields, but flash provides additional feedback + error_message = extract_error_message(form) + {:noreply, socket |> assign(form: form) |> put_flash(:error, error_message)} + end + defp handle_save_forbidden(socket) do # Handle policy violations that aren't properly displayed in forms # AshPhoenix.Form doesn't implement FormData.Error protocol for Forbidden errors @@ -332,6 +342,77 @@ defmodule MvWeb.MemberLive.Form do {:noreply, put_flash(socket, :error, error_message)} end + defp handle_save_exception(socket, exception) do + # Handle unexpected exceptions (database errors, network issues, etc.) + require Logger + Logger.error("Unexpected error saving member: #{inspect(exception)}") + + action = get_action_name(socket.assigns.form.source.type) + error_message = gettext("Failed to %{action} member.", action: action) + + {:noreply, put_flash(socket, :error, error_message)} + end + + # Extracts a user-friendly error message from form errors + defp extract_error_message(form) do + # Try to extract message from source errors first + source_errors = get_source_errors(form) + + case source_errors do + [%Ash.Error.Invalid{errors: errors} | _] when is_list(errors) -> + # Extract first error message + case List.first(errors) do + %{message: message} when is_binary(message) -> + gettext("Validation failed: %{message}", message: message) + + %{field: field, message: message} when is_binary(message) -> + gettext("Validation failed: %{field} %{message}", field: field, message: message) + + _ -> + gettext("Validation failed. Please check your input.") + end + + [error | _] -> + # Try to extract message from other error types + case error do + %{message: message} when is_binary(message) -> message + error when is_struct(error) -> + # Try to use Ash.ErrorKind protocol if available + try do + Ash.ErrorKind.message(error) + rescue + Protocol.UndefinedError -> gettext("Failed to save member. Please try again.") + end + _ -> gettext("Failed to save member. Please try again.") + end + + _ -> + # Check if there are any field errors in the form + if has_form_errors?(form) do + gettext("Please correct the errors in the form and try again.") + else + gettext("Failed to save member. Please try again.") + end + end + end + + # Checks if form has any errors + defp has_form_errors?(form) do + case Map.get(form, :errors) do + errors when is_list(errors) and length(errors) > 0 -> true + _ -> false + end + end + + # Extracts source-level errors from form (Ash errors, etc.) + defp get_source_errors(form) do + case form.source do + %{errors: errors} when is_list(errors) -> errors + %Ash.Changeset{errors: errors} when is_list(errors) -> errors + _ -> [] + end + end + defp get_action_name(:create), do: gettext("create") defp get_action_name(:update), do: gettext("update") defp get_action_name(other), do: to_string(other) diff --git a/test/mv_web/member_live/form_error_handling_test.exs b/test/mv_web/member_live/form_error_handling_test.exs new file mode 100644 index 0000000..4f76fca --- /dev/null +++ b/test/mv_web/member_live/form_error_handling_test.exs @@ -0,0 +1,140 @@ +defmodule MvWeb.MemberLive.FormErrorHandlingTest do + @moduledoc """ + Tests for error handling in the member form, specifically flash message display. + """ + use MvWeb.ConnCase, async: false + + import Phoenix.LiveViewTest + + alias Mv.Membership.Member + + require Ash.Query + + describe "error handling - flash messages" do + test "shows flash message when member creation fails with validation error", %{conn: conn} do + # Create a member with the same email to trigger uniqueness error + {:ok, _existing_member} = + Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Existing", + last_name: "Member", + email: "duplicate@example.com" + }) + |> Ash.create() + + conn = conn_with_oidc_user(conn) + {:ok, view, _html} = live(conn, "/members/new") + + # Try to create member with duplicate email + form_data = %{ + "member[first_name]" => "New", + "member[last_name]" => "Member", + "member[email]" => "duplicate@example.com" + } + + html = + view + |> form("#member-form", form_data) + |> render_submit() + + # Should show flash error message + assert has_element?(view, "#flash-group") + assert html =~ "error" or html =~ "Error" or html =~ "Fehler" or + html =~ "failed" or html =~ "fehlgeschlagen" or + html =~ "Validation failed" or html =~ "Validierung fehlgeschlagen" + end + + test "shows flash message when member creation fails with missing required fields", %{ + conn: conn + } do + conn = conn_with_oidc_user(conn) + {:ok, view, _html} = live(conn, "/members/new") + + # Submit form with missing required fields (e.g., email) + form_data = %{ + "member[first_name]" => "Test", + "member[last_name]" => "User" + # email is missing + } + + html = + view + |> form("#member-form", form_data) + |> render_submit() + + # Should show flash error message + assert has_element?(view, "#flash-group") + assert html =~ "error" or html =~ "Error" or html =~ "Fehler" or + html =~ "failed" or html =~ "fehlgeschlagen" or + html =~ "Validation failed" or html =~ "Validierung fehlgeschlagen" or + html =~ "Please correct" or html =~ "Bitte korrigieren" + end + + test "shows flash message when member update fails", %{conn: conn} do + # Create a member to edit + {:ok, member} = + Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Original", + last_name: "Member", + email: "original@example.com" + }) + |> Ash.create() + + # Create another member with different email + {:ok, _other_member} = + Member + |> Ash.Changeset.for_create(:create_member, %{ + first_name: "Other", + last_name: "Member", + email: "other@example.com" + }) + |> Ash.create() + + conn = conn_with_oidc_user(conn) + {:ok, view, _html} = live(conn, "/members/#{member.id}/edit") + + # Try to update with duplicate email + form_data = %{ + "member[first_name]" => "Updated", + "member[last_name]" => "Member", + "member[email]" => "other@example.com" + } + + html = + view + |> form("#member-form", form_data) + |> render_submit() + + # Should show flash error message + assert has_element?(view, "#flash-group") + assert html =~ "error" or html =~ "Error" or html =~ "Fehler" or + html =~ "failed" or html =~ "fehlgeschlagen" or + html =~ "Validation failed" or html =~ "Validierung fehlgeschlagen" + end + + test "form still displays field-level validation errors when flash message is shown", %{ + conn: conn + } do + conn = conn_with_oidc_user(conn) + {:ok, view, _html} = live(conn, "/members/new") + + # Submit form with invalid email format + form_data = %{ + "member[first_name]" => "Test", + "member[last_name]" => "User", + "member[email]" => "invalid-email-format" + } + + html = + view + |> form("#member-form", form_data) + |> render_submit() + + # Should show both flash message and field-level error + assert has_element?(view, "#flash-group") + # Field-level errors should also be visible in the form + assert html =~ "email" or html =~ "Email" + end + end +end From bc4bcd0089b0b1fb2942c0c49f661979f020ba78 Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 19 Jan 2026 13:40:28 +0100 Subject: [PATCH 2/3] fix: change creation of admin user --- docs/development-progress-log.md | 2 +- priv/repo/seeds.exs | 67 +++++++++++++++++++++++--------- test/membership/member_test.exs | 20 ++++++++++ 3 files changed, 69 insertions(+), 20 deletions(-) diff --git a/docs/development-progress-log.md b/docs/development-progress-log.md index 629987e..f55c214 100644 --- a/docs/development-progress-log.md +++ b/docs/development-progress-log.md @@ -775,7 +775,7 @@ end ### Test Data Management **Seed Data:** -- Admin user: `admin@mv.local` / `testpassword` +- Admin user: `admin@localhost` / `testpassword` (configurable via `ADMIN_EMAIL` env var) - Sample members: Hans Müller, Greta Schmidt, Friedrich Wagner - Linked accounts: Maria Weber, Thomas Klein - CustomFieldValue types: String, Date, Boolean, Email diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index b89ba3c..6294353 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -9,6 +9,8 @@ alias Mv.Authorization alias Mv.MembershipFees.MembershipFeeType alias Mv.MembershipFees.CycleGenerator +require Ash.Query + # Create example membership fee types for fee_type_attrs <- [ %{ @@ -124,13 +126,10 @@ for attrs <- [ ) end -# Create admin user for testing -admin_user = - Accounts.create_user!(%{email: "admin@mv.local"}, upsert?: true, upsert_identity: :unique_email) - |> Ash.Changeset.for_update(:admin_set_password, %{password: "testpassword"}) - |> Ash.update!() +# Get admin email from environment variable or use default +admin_email = System.get_env("ADMIN_EMAIL") || "admin@localhost" -# Create admin role and assign it to admin user +# Create admin role (used for assigning to admin users) admin_role = case Authorization.list_roles() do {:ok, roles} -> @@ -154,23 +153,53 @@ admin_role = nil end -# Assign admin role to admin user if role was created/found -if admin_role do - admin_user - |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) - |> Ash.update!() +if is_nil(admin_role) do + raise "Failed to create or find admin role. Cannot proceed with member seeding." +end + +# Assign admin role to user with ADMIN_EMAIL (if user exists) +# This handles both existing users (e.g., from OIDC) and newly created users +case Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, existing_admin_user} when not is_nil(existing_admin_user) -> + # User already exists (e.g., via OIDC) - assign admin role + existing_admin_user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :replace) + |> Ash.update!() + + {:ok, nil} -> + # User doesn't exist - create admin user with password + Accounts.create_user!(%{email: admin_email}, upsert?: true, upsert_identity: :unique_email) + |> Ash.Changeset.for_update(:admin_set_password, %{password: "testpassword"}) + |> Ash.update!() + |> then(fn user -> + user + |> Ash.Changeset.for_update(:update, %{}) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :replace) + |> Ash.update!() + end) + + {:error, error} -> + raise "Failed to check for existing admin user: #{inspect(error)}" end # Load admin user with role for use as actor in member operations # This ensures all member operations have proper authorization -# If admin role creation failed, we cannot proceed with member operations admin_user_with_role = - if admin_role do - admin_user - |> Ash.load!(:role) - else - raise "Failed to create or find admin role. Cannot proceed with member seeding." + case Accounts.User + |> Ash.Query.filter(email == ^admin_email) + |> Ash.read_one(domain: Mv.Accounts) do + {:ok, user} when not is_nil(user) -> + user + |> Ash.load!(:role) + + {:ok, nil} -> + raise "Admin user not found after creation/assignment" + + {:error, error} -> + raise "Failed to load admin user: #{inspect(error)}" end # Load all membership fee types for assignment @@ -598,7 +627,7 @@ IO.puts("📝 Created sample data:") IO.puts(" - Global settings: club_name = #{default_club_name}") IO.puts(" - Membership fee types: 4 types (Yearly, Half-yearly, Quarterly, Monthly)") IO.puts(" - Custom fields: 12 fields (String, Date, Boolean, Email, + 8 realistic fields)") -IO.puts(" - Admin user: admin@mv.local (password: testpassword)") +IO.puts(" - Admin user: #{admin_email} (password: testpassword)") IO.puts(" - Sample members: Hans, Greta, Friedrich") IO.puts( diff --git a/test/membership/member_test.exs b/test/membership/member_test.exs index 258d8be..6919ec1 100644 --- a/test/membership/member_test.exs +++ b/test/membership/member_test.exs @@ -73,6 +73,26 @@ defmodule Mv.Membership.MemberTest do end end + describe "Authorization" do + @valid_attrs %{ + first_name: "John", + last_name: "Doe", + email: "john@example.com" + } + + test "user without role cannot create member" do + # Create a user without a role + user = Mv.Fixtures.user_fixture() + # Ensure user has no role (nil role) + user_without_role = %{user | role: nil} + + # Attempt to create a member with user without role as actor + # This should fail with Ash.Error.Forbidden containing a Policy error + assert {:error, %Ash.Error.Forbidden{errors: [%Ash.Error.Forbidden.Policy{}]}} = + Membership.create_member(@valid_attrs, actor: user_without_role) + end + end + # Helper function for error evaluation defp error_message(errors, field) do errors From d9b659e5ea36d2a3d438c4e3e589c0ebdaeda86d Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 19 Jan 2026 14:09:19 +0100 Subject: [PATCH 3/3] fix: linting + tests --- lib/mv_web/components/layouts/sidebar.ex | 2 +- lib/mv_web/live/member_live/form.ex | 8 ++++++-- priv/repo/seeds.exs | 4 ++-- test/mv_web/member_live/form_error_handling_test.exs | 3 +++ 4 files changed, 12 insertions(+), 5 deletions(-) diff --git a/lib/mv_web/components/layouts/sidebar.ex b/lib/mv_web/components/layouts/sidebar.ex index 33319d4..c464b66 100644 --- a/lib/mv_web/components/layouts/sidebar.ex +++ b/lib/mv_web/components/layouts/sidebar.ex @@ -81,7 +81,7 @@ defmodule MvWeb.Layouts.Sidebar do icon="hero-currency-euro" label={gettext("Fee Types")} /> - + <.menu_group icon="hero-cog-6-tooth" label={gettext("Administration")}> <.menu_subitem href={~p"/users"} label={gettext("Users")} /> diff --git a/lib/mv_web/live/member_live/form.ex b/lib/mv_web/live/member_live/form.ex index d384a45..b319fa1 100644 --- a/lib/mv_web/live/member_live/form.ex +++ b/lib/mv_web/live/member_live/form.ex @@ -375,7 +375,9 @@ defmodule MvWeb.MemberLive.Form do [error | _] -> # Try to extract message from other error types case error do - %{message: message} when is_binary(message) -> message + %{message: message} when is_binary(message) -> + message + error when is_struct(error) -> # Try to use Ash.ErrorKind protocol if available try do @@ -383,7 +385,9 @@ defmodule MvWeb.MemberLive.Form do rescue Protocol.UndefinedError -> gettext("Failed to save member. Please try again.") end - _ -> gettext("Failed to save member. Please try again.") + + _ -> + gettext("Failed to save member. Please try again.") end _ -> diff --git a/priv/repo/seeds.exs b/priv/repo/seeds.exs index 6294353..2e7543a 100644 --- a/priv/repo/seeds.exs +++ b/priv/repo/seeds.exs @@ -166,7 +166,7 @@ case Accounts.User # User already exists (e.g., via OIDC) - assign admin role existing_admin_user |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, admin_role, type: :replace) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!() {:ok, nil} -> @@ -177,7 +177,7 @@ case Accounts.User |> then(fn user -> user |> Ash.Changeset.for_update(:update, %{}) - |> Ash.Changeset.manage_relationship(:role, admin_role, type: :replace) + |> Ash.Changeset.manage_relationship(:role, admin_role, type: :append_and_remove) |> Ash.update!() end) diff --git a/test/mv_web/member_live/form_error_handling_test.exs b/test/mv_web/member_live/form_error_handling_test.exs index 4f76fca..859402e 100644 --- a/test/mv_web/member_live/form_error_handling_test.exs +++ b/test/mv_web/member_live/form_error_handling_test.exs @@ -39,6 +39,7 @@ defmodule MvWeb.MemberLive.FormErrorHandlingTest do # Should show flash error message assert has_element?(view, "#flash-group") + assert html =~ "error" or html =~ "Error" or html =~ "Fehler" or html =~ "failed" or html =~ "fehlgeschlagen" or html =~ "Validation failed" or html =~ "Validierung fehlgeschlagen" @@ -64,6 +65,7 @@ defmodule MvWeb.MemberLive.FormErrorHandlingTest do # Should show flash error message assert has_element?(view, "#flash-group") + assert html =~ "error" or html =~ "Error" or html =~ "Fehler" or html =~ "failed" or html =~ "fehlgeschlagen" or html =~ "Validation failed" or html =~ "Validierung fehlgeschlagen" or @@ -108,6 +110,7 @@ defmodule MvWeb.MemberLive.FormErrorHandlingTest do # Should show flash error message assert has_element?(view, "#flash-group") + assert html =~ "error" or html =~ "Error" or html =~ "Fehler" or html =~ "failed" or html =~ "fehlgeschlagen" or html =~ "Validation failed" or html =~ "Validierung fehlgeschlagen"