From 5deb102e459b6f735f368e9b979a1057558abfcc Mon Sep 17 00:00:00 2001 From: Simon Date: Mon, 9 Mar 2026 18:54:40 +0100 Subject: [PATCH] refactor: adress review comments --- CODE_GUIDELINES.md | 2 +- config/runtime.exs | 14 +++--- docs/development-progress-log.md | 3 +- lib/membership/membership.ex | 20 +++++++-- .../tasks/join_requests.cleanup_expired.ex | 43 +++++++++++++------ .../send_new_user_confirmation_email.ex | 10 ++++- .../user/senders/send_password_reset_email.ex | 10 ++++- lib/mv_web/emails/email_layout_view.ex | 3 ++ lib/mv_web/emails/join_confirmation_email.ex | 18 ++++++-- .../templates/emails/layouts/layout.html.heex | 10 +++-- priv/gettext/en/LC_MESSAGES/default.po | 30 ++++++------- 11 files changed, 111 insertions(+), 52 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 77098d9..66a93a5 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1260,7 +1260,7 @@ mix hex.outdated **Mailer and from address:** - `Mv.Mailer` (Swoosh) and `Mv.Mailer.mail_from/0` return the configured sender `{name, email}`. -- Config: `config :mv, :mail_from, {"Mila", "noreply@example.com"}` in config.exs; override in runtime.exs from ENV for production. +- Config: `config :mv, :mail_from, {"Mila", "noreply@example.com"}` in config.exs. In production, runtime.exs overrides from ENV (`MAIL_FROM_NAME`, `MAIL_FROM_EMAIL`). **Unified layout (transactional emails):** diff --git a/config/runtime.exs b/config/runtime.exs index d502cfa..b8570d8 100644 --- a/config/runtime.exs +++ b/config/runtime.exs @@ -217,13 +217,13 @@ if config_env() == :prod do # # Check `Plug.SSL` for all available options in `force_ssl`. - # ## Configuring the mailer - # - # Transactional emails (join confirmation, user confirmation, password reset) use - # the sender from config :mv, :mail_from. Override in production if needed: - # config :mv, :mail_from, {System.get_env("MAIL_FROM_NAME", "Mila"), System.get_env("MAIL_FROM_EMAIL", "noreply@example.com")} - # - # In production you need to configure the mailer to use a different adapter. + # Transactional emails use the sender from config :mv, :mail_from (overridable via ENV). + config :mv, + :mail_from, + {System.get_env("MAIL_FROM_NAME", "Mila"), + System.get_env("MAIL_FROM_EMAIL", "noreply@example.com")} + + # In production you may need to configure the mailer to use a different adapter. # Also, you may need to configure the Swoosh API client of your choice if you # are not using SMTP. Here is an example of the configuration: # diff --git a/docs/development-progress-log.md b/docs/development-progress-log.md index 4e95ff4..a86efe6 100644 --- a/docs/development-progress-log.md +++ b/docs/development-progress-log.md @@ -805,7 +805,8 @@ end - **Join confirmation:** Domain wrapper `submit_join_request/2` generates token (or uses optional `:confirmation_token` in attrs for tests), creates JoinRequest via action `:submit`, then sends one email via `MvWeb.Emails.JoinConfirmationEmail`. Route `GET /confirm_join/:token` (JoinConfirmController) updates to `submitted`; idempotent; expired/invalid handled. - **Senders migrated:** `SendNewUserConfirmationEmail`, `SendPasswordResetEmail` use layout + `Mv.Mailer.mail_from/0`. - **Cleanup:** Mix task `mix join_requests.cleanup_expired` hard-deletes JoinRequests in `pending_confirmation` with expired `confirmation_token_expires_at` (authorize?: false). For cron/Oban. -- **Gettext:** New email strings in default domain; German translations in de/LC_MESSAGES/default.po. +- **Gettext:** New email strings in default domain; German translations in de/LC_MESSAGES/default.po; English msgstr filled for email-related strings. +- **PR review follow-ups (Join confirmation):** Join confirmation email uses `Mailer.deliver/1` and returns `{:ok, email}` \| `{:error, reason}`; domain logs delivery errors but still returns `{:ok, request}` so the user sees success. Comment in `submit_join_request/2` clarifies that the raw token is hashed by `JoinRequest.Changes.SetConfirmationToken`. Cleanup task uses `Ash.bulk_destroy` and logs partial errors without halting. Layout uses assigns `app_name` and `locale` (from config/Gettext) instead of hardcoded "Mila" and `lang="de"`. Production `runtime.exs` sets `:mail_from` from ENV (`MAIL_FROM_NAME`, `MAIL_FROM_EMAIL`). Layout reference unified to `"layout.html"`; redundant `put_layout` removed from senders. - Tests: `join_request_test.exs`, `join_request_submit_email_test.exs`, `join_confirm_controller_test.exs` – all pass. ### Test Data Management diff --git a/lib/membership/membership.ex b/lib/membership/membership.ex index ffca356..5e01a6a 100644 --- a/lib/membership/membership.ex +++ b/lib/membership/membership.ex @@ -31,6 +31,7 @@ defmodule Mv.Membership do alias Ash.Error.Query.NotFound, as: NotFoundError alias Mv.Membership.JoinRequest alias MvWeb.Emails.JoinConfirmationEmail + require Logger admin do show? true @@ -369,8 +370,9 @@ defmodule Mv.Membership do actor = Keyword.get(opts, :actor) token = Map.get(attrs, :confirmation_token) || generate_confirmation_token() - attrs_with_token = - attrs |> Map.drop([:confirmation_token]) |> Map.put(:confirmation_token, token) + # Raw token is passed to the submit action; JoinRequest.Changes.SetConfirmationToken + # hashes it before persist. Only the hash is stored; the raw token is sent in the email link. + attrs_with_token = Map.put(attrs, :confirmation_token, token) case Ash.create(JoinRequest, attrs_with_token, action: :submit, @@ -378,8 +380,18 @@ defmodule Mv.Membership do domain: __MODULE__ ) do {:ok, request} -> - JoinConfirmationEmail.send(request.email, token) - {:ok, request} + case JoinConfirmationEmail.send(request.email, token) do + {:ok, _email} -> + {:ok, request} + + {:error, reason} -> + Logger.error( + "Join confirmation email failed for #{request.email}: #{inspect(reason)}" + ) + + # Request was created; return success so the user sees the confirmation message + {:ok, request} + end error -> error diff --git a/lib/mix/tasks/join_requests.cleanup_expired.ex b/lib/mix/tasks/join_requests.cleanup_expired.ex index bc9ea2a..0d1823c 100644 --- a/lib/mix/tasks/join_requests.cleanup_expired.ex +++ b/lib/mix/tasks/join_requests.cleanup_expired.ex @@ -17,6 +17,7 @@ defmodule Mix.Tasks.JoinRequests.CleanupExpired do use Mix.Task require Ash.Query + require Logger alias Mv.Membership.JoinRequest @@ -34,23 +35,41 @@ defmodule Mix.Tasks.JoinRequests.CleanupExpired do |> Ash.Query.filter(confirmation_token_expires_at < ^now) # Bypass authorization: cleanup is a system maintenance task (cron/Oban). - case Ash.read(query, domain: Mv.Membership, authorize?: false) do - {:ok, requests} -> - count = delete_expired_requests(requests) + # Use bulk_destroy so the data layer can delete in one pass when supported. + opts = [domain: Mv.Membership, authorize?: false] + + count = + case Ash.count(query, opts) do + {:ok, n} -> n + {:error, _} -> 0 + end + + do_run(query, opts, count) + end + + defp do_run(_query, _opts, 0) do + Mix.shell().info("No expired join requests to delete.") + 0 + end + + defp do_run(query, opts, count) do + case Ash.bulk_destroy(query, :destroy, %{}, opts) do + %{status: status, errors: errors} when status in [:success, :partial_success] -> + maybe_log_errors(errors) Mix.shell().info("Deleted #{count} expired join request(s).") count - {:error, error} -> - Mix.raise("Failed to list expired join requests: #{inspect(error)}") + %{status: :error, errors: errors} -> + Mix.raise("Failed to delete expired join requests: #{inspect(errors)}") end end - defp delete_expired_requests(requests) do - Enum.reduce_while(requests, 0, fn request, acc -> - case Ash.destroy(request, domain: Mv.Membership, authorize?: false) do - :ok -> {:cont, acc + 1} - {:error, _} -> {:halt, acc} - end - end) + defp maybe_log_errors(nil), do: :ok + defp maybe_log_errors([]), do: :ok + + defp maybe_log_errors(errors) do + Logger.warning( + "Join requests cleanup: #{length(errors)} error(s) while deleting expired requests: #{inspect(errors)}" + ) end end diff --git a/lib/mv/accounts/user/senders/send_new_user_confirmation_email.ex b/lib/mv/accounts/user/senders/send_new_user_confirmation_email.ex index 7c5fa0c..393a220 100644 --- a/lib/mv/accounts/user/senders/send_new_user_confirmation_email.ex +++ b/lib/mv/accounts/user/senders/send_new_user_confirmation_email.ex @@ -37,13 +37,19 @@ defmodule Mv.Accounts.User.Senders.SendNewUserConfirmationEmail do confirm_url = url(~p"/confirm_new_user/#{token}") subject = gettext("Confirm your email address") + assigns = %{ + confirm_url: confirm_url, + subject: subject, + app_name: Mailer.mail_from() |> elem(0), + locale: Gettext.get_locale(MvWeb.Gettext) + } + new() |> from(Mailer.mail_from()) |> to(to_string(user.email)) |> subject(subject) |> put_view(MvWeb.EmailsView) - |> put_layout({MvWeb.EmailLayoutView, "layout.html"}) - |> render_body("user_confirmation.html", %{confirm_url: confirm_url, subject: subject}) + |> render_body("user_confirmation.html", assigns) |> Mailer.deliver!() end end diff --git a/lib/mv/accounts/user/senders/send_password_reset_email.ex b/lib/mv/accounts/user/senders/send_password_reset_email.ex index a4bb489..74d5d47 100644 --- a/lib/mv/accounts/user/senders/send_password_reset_email.ex +++ b/lib/mv/accounts/user/senders/send_password_reset_email.ex @@ -37,13 +37,19 @@ defmodule Mv.Accounts.User.Senders.SendPasswordResetEmail do reset_url = url(~p"/password-reset/#{token}") subject = gettext("Reset your password") + assigns = %{ + reset_url: reset_url, + subject: subject, + app_name: Mailer.mail_from() |> elem(0), + locale: Gettext.get_locale(MvWeb.Gettext) + } + new() |> from(Mailer.mail_from()) |> to(to_string(user.email)) |> subject(subject) |> put_view(MvWeb.EmailsView) - |> put_layout({MvWeb.EmailLayoutView, "layout.html"}) - |> render_body("password_reset.html", %{reset_url: reset_url, subject: subject}) + |> render_body("password_reset.html", assigns) |> Mailer.deliver!() end end diff --git a/lib/mv_web/emails/email_layout_view.ex b/lib/mv_web/emails/email_layout_view.ex index a0cf03f..da76656 100644 --- a/lib/mv_web/emails/email_layout_view.ex +++ b/lib/mv_web/emails/email_layout_view.ex @@ -4,6 +4,9 @@ defmodule MvWeb.EmailLayoutView do Renders a single layout template that wraps all email body content. See docs/email-layout-mockup.md for the layout structure. + + Uses Phoenix.View (legacy API) for compatibility with phoenix_swoosh email rendering. + Layout expects assigns :app_name and :locale (passed from each email sender). """ use Phoenix.View, root: "lib/mv_web", diff --git a/lib/mv_web/emails/join_confirmation_email.ex b/lib/mv_web/emails/join_confirmation_email.ex index f72ec45..781a205 100644 --- a/lib/mv_web/emails/join_confirmation_email.ex +++ b/lib/mv_web/emails/join_confirmation_email.ex @@ -4,7 +4,7 @@ defmodule MvWeb.Emails.JoinConfirmationEmail do """ use Phoenix.Swoosh, view: MvWeb.EmailsView, - layout: {MvWeb.EmailLayoutView, :layout} + layout: {MvWeb.EmailLayoutView, "layout.html"} use MvWeb, :verified_routes import Swoosh.Email @@ -16,18 +16,28 @@ defmodule MvWeb.Emails.JoinConfirmationEmail do Sends the join confirmation email to the given address with the confirmation link. Called from the domain after a JoinRequest is created (submit flow). + Returns `{:ok, email}` on success, `{:error, reason}` on delivery failure. + Callers should log errors and may still return success for the overall operation + (e.g. join request created) so the user is not shown a generic error when only + the email failed. """ def send(email_address, token) when is_binary(email_address) and is_binary(token) do confirm_url = url(~p"/confirm_join/#{token}") subject = gettext("Confirm your membership request") + assigns = %{ + confirm_url: confirm_url, + subject: subject, + app_name: Mailer.mail_from() |> elem(0), + locale: Gettext.get_locale(MvWeb.Gettext) + } + new() |> from(Mailer.mail_from()) |> to(email_address) |> subject(subject) |> put_view(MvWeb.EmailsView) - |> put_layout({MvWeb.EmailLayoutView, "layout.html"}) - |> render_body("join_confirmation.html", %{confirm_url: confirm_url, subject: subject}) - |> Mailer.deliver!() + |> render_body("join_confirmation.html", assigns) + |> Mailer.deliver() end end diff --git a/lib/mv_web/templates/emails/layouts/layout.html.heex b/lib/mv_web/templates/emails/layouts/layout.html.heex index 63bc5c7..4b5535f 100644 --- a/lib/mv_web/templates/emails/layouts/layout.html.heex +++ b/lib/mv_web/templates/emails/layouts/layout.html.heex @@ -1,9 +1,9 @@ - + - {assigns[:subject] || "Mila"} + {assigns[:subject] || assigns[:app_name] || "Mila"} @@ -25,7 +27,7 @@
-
Mila
+
+ {assigns[:app_name] || "Mila"} +
- © {DateTime.utc_now().year} Mila · Mitgliederverwaltung + © {DateTime.utc_now().year} {assigns[:app_name] || "Mila"} · Mitgliederverwaltung
diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index bd9bb0c..7dc5068 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -3243,77 +3243,77 @@ msgstr "without %{name}" #: lib/mv_web/templates/emails/user_confirmation.html.heex #, elixir-autogen, elixir-format msgid "Confirm my email" -msgstr "" +msgstr "Confirm my email" #: lib/mv_web/templates/emails/join_confirmation.html.heex #, elixir-autogen, elixir-format msgid "Confirm my request" -msgstr "" +msgstr "Confirm my request" #: lib/mv/accounts/user/senders/send_new_user_confirmation_email.ex #, elixir-autogen, elixir-format msgid "Confirm your email address" -msgstr "" +msgstr "Confirm your email address" #: lib/mv_web/emails/join_confirmation_email.ex #, elixir-autogen, elixir-format msgid "Confirm your membership request" -msgstr "" +msgstr "Confirm your membership request" #: lib/mv_web/templates/emails/user_confirmation.html.heex #, elixir-autogen, elixir-format msgid "If you did not create an account, you can ignore this email." -msgstr "" +msgstr "If you did not create an account, you can ignore this email." #: lib/mv_web/templates/emails/password_reset.html.heex #, elixir-autogen, elixir-format msgid "If you did not request this, you can ignore this email. Your password will remain unchanged." -msgstr "" +msgstr "If you did not request this, you can ignore this email. Your password will remain unchanged." #: lib/mv_web/templates/emails/join_confirmation.html.heex #, elixir-autogen, elixir-format msgid "If you did not submit this request, you can ignore this email." -msgstr "" +msgstr "If you did not submit this request, you can ignore this email." #: lib/mv_web/controllers/join_confirm_controller.ex #, elixir-autogen, elixir-format msgid "Invalid or expired link." -msgstr "" +msgstr "Invalid or expired link." #: lib/mv_web/templates/emails/user_confirmation.html.heex #, elixir-autogen, elixir-format msgid "Please confirm your email address by clicking the link below." -msgstr "" +msgstr "Please confirm your email address by clicking the link below." #: lib/mv_web/templates/emails/password_reset.html.heex #, elixir-autogen, elixir-format msgid "Reset password" -msgstr "" +msgstr "Reset password" #: lib/mv/accounts/user/senders/send_password_reset_email.ex #, elixir-autogen, elixir-format msgid "Reset your password" -msgstr "" +msgstr "Reset your password" #: lib/mv_web/controllers/join_confirm_controller.ex #, elixir-autogen, elixir-format msgid "Thank you, we have received your request." -msgstr "" +msgstr "Thank you, we have received your request." #: lib/mv_web/controllers/join_confirm_controller.ex #, elixir-autogen, elixir-format msgid "This link has expired. Please submit the form again." -msgstr "" +msgstr "This link has expired. Please submit the form again." #: lib/mv_web/templates/emails/join_confirmation.html.heex #, elixir-autogen, elixir-format msgid "We have received your membership request. To complete it, please click the link below." -msgstr "" +msgstr "We have received your membership request. To complete it, please click the link below." #: lib/mv_web/templates/emails/password_reset.html.heex #, elixir-autogen, elixir-format msgid "You requested a password reset. Click the link below to set a new password." -msgstr "" +msgstr "You requested a password reset. Click the link below to set a new password." #: lib/mv_web/live/member_live/show/membership_fees_component.ex #, elixir-autogen, elixir-format, fuzzy