From 40a4461d2367b714b73c0ac7d21657f0d4fed490 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 13 Mar 2026 09:34:56 +0100 Subject: [PATCH] fix: join confirmation mail configuration --- CODE_GUIDELINES.md | 4 ++ docs/development-progress-log.md | 2 +- docs/smtp-configuration-concept.md | 15 ++++-- lib/membership/membership.ex | 6 +-- lib/mv_web/emails/join_confirmation_email.ex | 22 ++++---- lib/mv_web/live/join_live.ex | 18 ++++++- priv/gettext/de/LC_MESSAGES/default.po | 21 +++++--- priv/gettext/default.pot | 5 ++ priv/gettext/en/LC_MESSAGES/default.po | 5 ++ ...join_request_submit_email_failure_test.exs | 33 ++++++++++++ .../live/join_live_email_failure_test.exs | 54 +++++++++++++++++++ test/support/failing_mail_adapter.ex | 10 ++++ 12 files changed, 167 insertions(+), 28 deletions(-) create mode 100644 test/membership/join_request_submit_email_failure_test.exs create mode 100644 test/mv_web/live/join_live_email_failure_test.exs create mode 100644 test/support/failing_mail_adapter.ex diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 0cb8d65..898fdd2 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1290,6 +1290,10 @@ mix hex.outdated - `SendPasswordResetEmail` and `SendNewUserConfirmationEmail` use `Mv.Mailer.deliver/1` (not `deliver!/1`). Errors are logged via `Logger.error` and not re-raised so they never crash the caller process. +**Join confirmation email:** + +- `MvWeb.Emails.JoinConfirmationEmail` uses `Mailer.deliver(email, Mailer.smtp_config())` so it uses the same SMTP configuration as the test mail (Settings or boot ENV). On delivery failure, `Mv.Membership.submit_join_request/2` returns `{:error, :email_delivery_failed}` (and logs via `Logger.error`); the JoinLive shows an error message and no success UI. + **Unified layout (transactional emails):** - All transactional emails (join confirmation, user confirmation, password reset) use the same layout: `MvWeb.EmailLayoutView` (layout) and `MvWeb.EmailsView` (body templates). diff --git a/docs/development-progress-log.md b/docs/development-progress-log.md index a6297ba..6d8e523 100644 --- a/docs/development-progress-log.md +++ b/docs/development-progress-log.md @@ -806,7 +806,7 @@ end - **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; 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. +- **PR review follow-ups (Join confirmation):** Join confirmation email uses `Mailer.deliver/2` with `Mailer.smtp_config/0` (same config as test mail). On delivery failure the domain returns `{:error, :email_delivery_failed}` (logged via `Logger.error`), and the JoinLive shows an error message (no success UI). 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. **Subtask 3 – Admin: Join form settings (done):** diff --git a/docs/smtp-configuration-concept.md b/docs/smtp-configuration-concept.md index 30fd7de..c60a0e2 100644 --- a/docs/smtp-configuration-concept.md +++ b/docs/smtp-configuration-concept.md @@ -82,13 +82,19 @@ Provided by `Mv.Config.mail_from_name/0` and `Mv.Config.mail_from_email/0`. --- -## 9. AshAuthentication Senders +## 9. Join Confirmation Email + +`MvWeb.Emails.JoinConfirmationEmail` uses the same SMTP configuration as the test email: `Mailer.deliver(email, Mailer.smtp_config())`. This ensures Settings-based SMTP is used when not configured via ENV at boot. On delivery failure the domain returns `{:error, :email_delivery_failed}` (and logs via `Logger.error`); the JoinLive shows an error message and no success UI. + +--- + +## 10. AshAuthentication Senders Both `SendPasswordResetEmail` and `SendNewUserConfirmationEmail` use `Mv.Mailer.deliver/1` (not `deliver!/1`). Delivery failures are logged (`Logger.error`) and not re-raised, so they never crash the caller process. AshAuthentication ignores the return value of `send/3`. --- -## 10. TLS / SSL in OTP 27 +## 11. TLS / SSL in OTP 27 OTP 26+ enforces `verify_peer` by default, which fails for self-signed or internal SMTP server certificates. @@ -101,7 +107,7 @@ Both `tls_options` (STARTTLS, port 587) and `sockopts` (direct SSL, port 465) us --- -## 11. Summary Checklist +## 12. Summary Checklist - [x] ENV: `SMTP_HOST`, `SMTP_PORT`, `SMTP_USERNAME`, `SMTP_PASSWORD`, `SMTP_PASSWORD_FILE`, `SMTP_SSL`. - [x] ENV: `MAIL_FROM_NAME`, `MAIL_FROM_EMAIL` for sender identity. @@ -112,13 +118,14 @@ Both `tls_options` (STARTTLS, port 587) and `sockopts` (direct SSL, port 465) us - [x] TLS certificate validation relaxed for OTP 27 (tls_options + sockopts). - [x] Prod warning: clear message in Settings when SMTP is not configured. - [x] Test email: form with recipient field, translatable content, classified success/error messages. +- [x] Join confirmation email: uses `Mailer.smtp_config/0` (same as test mail); on failure returns `{:error, :email_delivery_failed}`, error shown in JoinLive, logged for admin. - [x] AshAuthentication senders: graceful error handling (no crash on delivery failure). - [x] Gettext for all new UI strings, translated to German. - [x] Docs and code guidelines updated. --- -## 12. Follow-up / Future Work +## 13. Follow-up / Future Work - **SMTP password at-rest encryption:** The `smtp_password` attribute is currently stored in plaintext in the `settings` table. It is excluded from default reads (same pattern as `oidc_client_secret`); both are read only via explicit select when needed. For production systems at-rest encryption (e.g. with [Cloak](https://hexdocs.pm/cloak)) should be considered and tracked as a follow-up issue. - **Error classification:** SMTP error categorization currently uses substring matching on server messages (e.g. "535", "authentication"). A more robust approach would be to pattern-match on `gen_smtp` error tuples first where possible, and fall back to string analysis only when needed. Server wording varies; consider extending patterns as new providers are used. diff --git a/lib/membership/membership.ex b/lib/membership/membership.ex index 2f18f90..24bf27b 100644 --- a/lib/membership/membership.ex +++ b/lib/membership/membership.ex @@ -364,7 +364,8 @@ defmodule Mv.Membership do - `:actor` - Must be nil for public submit (policy allows only unauthenticated). ## Returns - - `{:ok, request}` - Created JoinRequest in status pending_confirmation + - `{:ok, request}` - Created JoinRequest in status pending_confirmation, email sent + - `{:error, :email_delivery_failed}` - Request created but confirmation email could not be sent (logged) - `{:error, error}` - Validation or authorization error """ def submit_join_request(attrs, opts \\ []) do @@ -390,8 +391,7 @@ defmodule Mv.Membership do "Join confirmation email failed for #{request.email}: #{inspect(reason)}" ) - # Request was created; return success so the user sees the confirmation message - {:ok, request} + {:error, :email_delivery_failed} end error -> diff --git a/lib/mv_web/emails/join_confirmation_email.ex b/lib/mv_web/emails/join_confirmation_email.ex index 781a205..9bd3c5a 100644 --- a/lib/mv_web/emails/join_confirmation_email.ex +++ b/lib/mv_web/emails/join_confirmation_email.ex @@ -15,11 +15,11 @@ defmodule MvWeb.Emails.JoinConfirmationEmail do @doc """ Sends the join confirmation email to the given address with the confirmation link. + Uses the same SMTP configuration as the test mail (Settings or boot ENV) via + `Mailer.deliver/2` with `Mailer.smtp_config/0` for consistency. + 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}") @@ -32,12 +32,14 @@ defmodule MvWeb.Emails.JoinConfirmationEmail do locale: Gettext.get_locale(MvWeb.Gettext) } - new() - |> from(Mailer.mail_from()) - |> to(email_address) - |> subject(subject) - |> put_view(MvWeb.EmailsView) - |> render_body("join_confirmation.html", assigns) - |> Mailer.deliver() + email = + new() + |> from(Mailer.mail_from()) + |> to(email_address) + |> subject(subject) + |> put_view(MvWeb.EmailsView) + |> render_body("join_confirmation.html", assigns) + + Mailer.deliver(email, Mailer.smtp_config()) end end diff --git a/lib/mv_web/live/join_live.ex b/lib/mv_web/live/join_live.ex index 99a7df9..7489331 100644 --- a/lib/mv_web/live/join_live.ex +++ b/lib/mv_web/live/join_live.ex @@ -142,8 +142,22 @@ defmodule MvWeb.JoinLive do case build_submit_attrs(params, socket.assigns.join_fields) do {:ok, attrs} -> case Membership.submit_join_request(attrs, actor: nil) do - {:ok, _} -> {:noreply, assign(socket, :submitted, true)} - {:error, _} -> validation_error_reply(socket, params) + {:ok, _} -> + {:noreply, assign(socket, :submitted, true)} + + {:error, :email_delivery_failed} -> + {:noreply, + socket + |> put_flash( + :error, + gettext( + "We could not send the confirmation email. Please try again later or contact support." + ) + ) + |> assign(:form, to_form(params, as: "join"))} + + {:error, _} -> + validation_error_reply(socket, params) end {:error, message} -> diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 1b163d4..a0d73fb 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -1556,17 +1556,17 @@ msgstr "Hausnummer" #: 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 "Wenn Sie kein Konto angelegt haben, können Sie diese E-Mail ignorieren." +msgstr "Wenn du kein Konto angelegt hast, kannst du diese E-Mail ignorieren." #: 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 "Wenn Sie das nicht angefordert haben, können Sie diese E-Mail ignorieren. Ihr Passwort bleibt unverändert." +msgstr "Wenn du das nicht angefordert hast, kannst du diese E-Mail ignorieren. Dein Passwort bleibt unverändert." #: 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 "Wenn Sie diese Anfrage nicht gestellt haben, können Sie diese E-Mail ignorieren." +msgstr "Wenn du diese Anfrage nicht gestellt hast, kannst du diese E-Mail ignorieren." #: lib/mv_web/components/layouts/sidebar.ex #: lib/mv_web/live/import_live.ex @@ -2542,7 +2542,7 @@ msgstr "Bitte bestätige zuerst die Betragsänderung" #: 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 "Bitte bestätigen Sie Ihre E-Mail-Adresse, indem Sie auf den folgenden Link klicken." +msgstr "Bitte bestätige deine E-Mail-Adresse, indem du auf den folgenden Link klickst." #: lib/mv_web/live/member_live/form.ex #, elixir-autogen, elixir-format @@ -3200,7 +3200,7 @@ msgstr "Textfeld" #: lib/mv_web/controllers/join_confirm_controller.ex #, elixir-autogen, elixir-format msgid "Thank you, we have received your request." -msgstr "Vielen Dank, wir haben Ihre Anfrage erhalten." +msgstr "Vielen Dank, wir haben deine Anfrage erhalten." #: lib/mv_web/controllers/auth_controller.ex #, elixir-autogen, elixir-format @@ -3273,7 +3273,7 @@ msgstr "Dies ist eine Test-E-Mail von Mila. Wenn du diese erhalten hast, funktio #: lib/mv_web/controllers/join_confirm_controller.ex #, elixir-autogen, elixir-format msgid "This link has expired. Please submit the form again." -msgstr "Dieser Link ist abgelaufen. Bitte senden Sie das Formular erneut ab." +msgstr "Dieser Link ist abgelaufen. Bitte sende das Formular erneut ab." #: lib/mv_web/live/user_live/form.ex #, elixir-autogen, elixir-format @@ -3517,7 +3517,7 @@ msgstr "Keine Internetverbindung gefunden" #: 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 "Wir haben Ihre Mitgliedschaftsanfrage erhalten. Bitte klicken Sie zur Bestätigung auf den folgenden Link." +msgstr "Wir haben deine Mitgliedschaftsanfrage erhalten. Bitte klicke zur Bestätigung auf den folgenden Link." #: lib/mv_web/live/join_live.ex #, elixir-autogen, elixir-format @@ -3635,7 +3635,7 @@ msgstr "Du hast dich bereits auf andere Weise angemeldet, aber dein Konto noch n #: 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 "Sie haben die Zurücksetzung Ihres Passworts angefordert. Klicken Sie auf den folgenden Link, um ein neues Passwort zu setzen." +msgstr "Du hast die Zurücksetzung deines Passworts angefordert. Klicke auf den folgenden Link, um ein neues Passwort zu setzen." #: lib/mv_web/live/join_live.ex #, elixir-autogen, elixir-format @@ -3795,3 +3795,8 @@ msgstr "Link zur öffentlichen Beitrittsseite (diesen Link mit Interessent*innen #, elixir-autogen, elixir-format msgid "Status and review" msgstr "Status und Prüfung" + +#: lib/mv_web/live/join_live.ex +#, elixir-autogen, elixir-format +msgid "We could not send the confirmation email. Please try again later or contact support." +msgstr "Die Bestätigungs-E-Mail konnte nicht versendet werden. Bitte versuche es später erneut oder wende dich an den Support." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 60e77c1..d20a604 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -3795,3 +3795,8 @@ msgstr "" #, elixir-autogen, elixir-format msgid "Status and review" msgstr "" + +#: lib/mv_web/live/join_live.ex +#, elixir-autogen, elixir-format +msgid "We could not send the confirmation email. Please try again later or contact support." +msgstr "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 4e4f87b..7a42e63 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -3795,3 +3795,8 @@ msgstr "Link to the public join page (share this with applicants):" #, elixir-autogen, elixir-format msgid "Status and review" msgstr "Status and review" + +#: lib/mv_web/live/join_live.ex +#, elixir-autogen, elixir-format +msgid "We could not send the confirmation email. Please try again later or contact support." +msgstr "" diff --git a/test/membership/join_request_submit_email_failure_test.exs b/test/membership/join_request_submit_email_failure_test.exs new file mode 100644 index 0000000..2587628 --- /dev/null +++ b/test/membership/join_request_submit_email_failure_test.exs @@ -0,0 +1,33 @@ +defmodule Mv.Membership.JoinRequestSubmitEmailFailureTest do + @moduledoc """ + Tests that when join confirmation email delivery fails, the domain returns + {:error, :email_delivery_failed} (and the LiveView shows an error). Uses + FailingMailAdapter to simulate delivery failure; async: false to avoid config races. + """ + use Mv.DataCase, async: false + + alias Mv.Membership + + @valid_submit_attrs %{ + email: "fail#{System.unique_integer([:positive])}@example.com" + } + + test "submit_join_request returns {:error, :email_delivery_failed} when mail delivery fails" do + saved = Application.get_env(:mv, Mv.Mailer) + + Application.put_env( + :mv, + Mv.Mailer, + Keyword.put(saved || [], :adapter, Mv.TestSupport.FailingMailAdapter) + ) + + on_exit(fn -> + Application.put_env(:mv, Mv.Mailer, saved) + end) + + token = "fail-token-#{System.unique_integer([:positive])}" + attrs = Map.put(@valid_submit_attrs, :confirmation_token, token) + + assert {:error, :email_delivery_failed} = Membership.submit_join_request(attrs, actor: nil) + end +end diff --git a/test/mv_web/live/join_live_email_failure_test.exs b/test/mv_web/live/join_live_email_failure_test.exs new file mode 100644 index 0000000..cc4e756 --- /dev/null +++ b/test/mv_web/live/join_live_email_failure_test.exs @@ -0,0 +1,54 @@ +defmodule MvWeb.JoinLiveEmailFailureTest do + @moduledoc """ + When join confirmation email delivery fails, the user sees an error message + and no success copy. Uses FailingMailAdapter; async: false to avoid config races. + """ + use MvWeb.ConnCase, async: false + import Phoenix.LiveViewTest + + alias Mv.Membership + + @tag role: :unauthenticated + test "when confirmation email fails, user sees error flash and no success message", %{ + conn: conn + } do + enable_join_form_for_test() + + saved = Application.get_env(:mv, Mv.Mailer) + + Application.put_env( + :mv, + Mv.Mailer, + Keyword.put(saved || [], :adapter, Mv.TestSupport.FailingMailAdapter) + ) + + on_exit(fn -> + Application.put_env(:mv, Mv.Mailer, saved) + end) + + {:ok, view, _html} = live(conn, "/join") + + view + |> form("#join-form", %{ + "email" => "fail#{System.unique_integer([:positive])}@example.com", + "first_name" => "Jane", + "last_name" => "Doe", + "website" => "" + }) + |> render_submit() + + html = render(view) + assert html =~ "could not send" or html =~ "confirmation email" + refute view |> element("[data-testid='join-success-message']") |> has_element?() + end + + defp enable_join_form_for_test do + {:ok, settings} = Membership.get_settings() + + Membership.update_settings(settings, %{ + join_form_enabled: true, + join_form_field_ids: ["email", "first_name", "last_name"], + join_form_field_required: %{"email" => true, "first_name" => false, "last_name" => false} + }) + end +end diff --git a/test/support/failing_mail_adapter.ex b/test/support/failing_mail_adapter.ex new file mode 100644 index 0000000..59bb4c0 --- /dev/null +++ b/test/support/failing_mail_adapter.ex @@ -0,0 +1,10 @@ +defmodule Mv.TestSupport.FailingMailAdapter do + @moduledoc """ + Swoosh adapter that always returns delivery failure. Used in tests to assert + that join confirmation email failure is handled (error shown to user, no success UI). + """ + use Swoosh.Adapter + + @impl true + def deliver(_email, _config), do: {:error, :forced} +end