refactor: adress review comments

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):**
 

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 use the sender from config :mv, :mail_from (overridable via ENV).
-  #
+  config :mv,
-  # Transactional emails (join confirmation, user confirmation, password reset) use
+         :mail_from,
-  # the sender from config :mv, :mail_from. Override in production if needed:
+         {System.get_env("MAIL_FROM_NAME", "Mila"),
-  #   config :mv, :mail_from, {System.get_env("MAIL_FROM_NAME", "Mila"), System.get_env("MAIL_FROM_EMAIL", "noreply@example.com")}
+          System.get_env("MAIL_FROM_EMAIL", "noreply@example.com")}
-  #
+
-  # In production you need to configure the mailer to use a different adapter.
+  # 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:
   #

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

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 =
+    # Raw token is passed to the submit action; JoinRequest.Changes.SetConfirmationToken
-      attrs |> Map.drop([:confirmation_token]) |> Map.put(:confirmation_token, token)
+    # 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)
+        case JoinConfirmationEmail.send(request.email, token) do
-        {:ok, request}
+          {: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

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
+    # Use bulk_destroy so the data layer can delete in one pass when supported.
-      {:ok, requests} ->
+    opts = [domain: Mv.Membership, authorize?: false]
-        count = delete_expired_requests(requests)
+
+    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} ->
+      %{status: :error, errors: errors} ->
-        Mix.raise("Failed to list expired join requests: #{inspect(error)}")
+        Mix.raise("Failed to delete expired join requests: #{inspect(errors)}")
     end
   end
 
-  defp delete_expired_requests(requests) do
+  defp maybe_log_errors(nil), do: :ok
-    Enum.reduce_while(requests, 0, fn request, acc ->
+  defp maybe_log_errors([]), do: :ok
-      case Ash.destroy(request, domain: Mv.Membership, authorize?: false) do
+
-        :ok -> {:cont, acc + 1}
+  defp maybe_log_errors(errors) do
-        {:error, _} -> {:halt, acc}
+    Logger.warning(
-      end
+      "Join requests cleanup: #{length(errors)} error(s) while deleting expired requests: #{inspect(errors)}"
-    end)
+    )
   end
 end

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", assigns)
-    |> render_body("user_confirmation.html", %{confirm_url: confirm_url, subject: subject})
     |> Mailer.deliver!()
   end
 end

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", assigns)
-    |> render_body("password_reset.html", %{reset_url: reset_url, subject: subject})
     |> Mailer.deliver!()
   end
 end

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",

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", assigns)
-    |> render_body("join_confirmation.html", %{confirm_url: confirm_url, subject: subject})
+    |> Mailer.deliver()
-    |> Mailer.deliver!()
   end
 end

lib/mv_web/templates/emails/layouts/layout.html.heex

@@ -1,9 +1,9 @@
 <!DOCTYPE html>
-<html lang="de">
+<html lang={assigns[:locale] || "de"}>
   <head>
     <meta charset="utf-8" />
     <meta name="viewport" content="width=device-width, initial-scale=1.0" />
-    <title>{assigns[:subject] || "Mila"}</title>
+    <title>{assigns[:subject] || assigns[:app_name] || "Mila"}</title>
   </head>
   <body style="margin: 0; padding: 0; font-family: system-ui, -apple-system, sans-serif; background-color: #f3f4f6;">
     <table
@@ -15,7 +15,9 @@
     >
       <tr>
         <td style="padding: 24px 16px 16px;">
-          <div style="font-size: 18px; font-weight: 600; color: #111827;">Mila</div>
+          <div style="font-size: 18px; font-weight: 600; color: #111827;">
+            {assigns[:app_name] || "Mila"}
+          </div>
         </td>
       </tr>
       <tr>
@@ -25,7 +27,7 @@
       </tr>
       <tr>
         <td style="padding: 16px 24px; font-size: 12px; color: #6b7280;">
-          © {DateTime.utc_now().year} Mila · Mitgliederverwaltung
+          © {DateTime.utc_now().year} {assigns[:app_name] || "Mila"} · Mitgliederverwaltung
         </td>
       </tr>
     </table>

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