refactor: fix review comments

CODE_GUIDELINES.md

@@ -1278,7 +1278,7 @@ mix hex.outdated
 **SMTP configuration:**
 
 - SMTP can be configured via **ENV variables** (`SMTP_HOST`, `SMTP_PORT`, `SMTP_USERNAME`, `SMTP_PASSWORD`, `SMTP_PASSWORD_FILE`, `SMTP_SSL`) or via **Admin Settings** (database: `smtp_host`, `smtp_port`, `smtp_username`, `smtp_password`, `smtp_ssl`).
-- **ENV-only policy:** If `SMTP_HOST` is set, SMTP is treated as environment-managed only. All SMTP fields in Settings are read-only, SMTP save action is hidden, and the UI shows a warning when required ENV values are missing (`SMTP_USERNAME`, and `SMTP_PASSWORD` or `SMTP_PASSWORD_FILE`).
+- **ENV-only policy:** If `SMTP_HOST` is set, SMTP is treated as environment-managed only. All SMTP fields in Settings are read-only, SMTP save action is hidden, and the UI shows a warning when required ENV values are missing (`SMTP_USERNAME`, and `SMTP_PASSWORD` or `SMTP_PASSWORD_FILE`). This keeps one source of truth for transport credentials and avoids mixed ENV/DB SMTP states.
 - **Sensitive settings in DB:** `smtp_password` and `oidc_client_secret` are excluded from the default read of the Setting resource; they are loaded only via explicit select when needed (e.g. `Mv.Config.smtp_password/0`, `Mv.Config.oidc_client_secret/0`). This avoids exposing secrets through `get_settings()`.
 - **Settings cache:** `Mv.Membership.get_settings/0` uses `Mv.Membership.SettingsCache` when the cache process is running (not in test). Cache has a short TTL and is invalidated on every settings update. This avoids repeated DB reads on hot paths (e.g. `RegistrationEnabled` validation, `Layouts.public_page`). In test, the cache is not started so all callers use `get_settings_uncached/0` in the test process (Ecto Sandbox).
 - **Join emails (domain → web):** The domain calls `Mv.Membership.JoinNotifier` (config `:join_notifier`, default `MvWeb.JoinNotifierImpl`) for sending join confirmation, already-member, and already-pending emails. This keeps the domain independent of the web layer; tests can override the notifier.

docs/smtp-configuration-concept.md

@@ -29,6 +29,7 @@ When `SMTP_HOST` is set, SMTP runs in **ENV-only mode**:
 - all SMTP fields in Settings are read-only,
 - saving SMTP settings in the UI is disabled,
 - and the UI shows a warning block if required SMTP ENV values are missing.
+- the UI displays the effective ENV-driven SMTP values in disabled fields so admins can verify what is active.
 
 ---
 

lib/mv/config.ex

@@ -470,11 +470,19 @@ defmodule Mv.Config do
   # ---------------------------------------------------------------------------
 
   @doc """
-  Returns SMTP host. ENV `SMTP_HOST` overrides Settings.
+  Returns SMTP host.
+
+  Policy:
+  - ENV-only mode (`SMTP_HOST` set): read from ENV `SMTP_HOST`
+  - Settings mode: read from Settings only
   """
   @spec smtp_host() :: String.t() | nil
   def smtp_host do
-    smtp_env_or_setting("SMTP_HOST", :smtp_host)
+    if smtp_env_mode?() do
+      System.get_env("SMTP_HOST") |> trim_nil()
+    else
+      get_from_settings(:smtp_host)
+    end
   end
 
   @doc """
@@ -522,7 +530,7 @@ defmodule Mv.Config do
   def smtp_password do
     if smtp_env_mode?() do
       case System.get_env("SMTP_PASSWORD") do
-        nil -> smtp_password_from_file_or_settings()
+        nil -> smtp_password_from_file()
         value -> trim_nil(value)
       end
     else
@@ -530,7 +538,7 @@ defmodule Mv.Config do
     end
   end
 
-  defp smtp_password_from_file_or_settings do
+  defp smtp_password_from_file do
     case System.get_env("SMTP_PASSWORD_FILE") do
       nil -> nil
       path -> read_smtp_password_file(path)
@@ -568,14 +576,6 @@ defmodule Mv.Config do
     present?(smtp_host())
   end
 
-  @doc """
-  Returns true when SMTP ENV mode is active.
-  """
-  @spec smtp_env_configured?() :: boolean()
-  def smtp_env_configured? do
-    smtp_env_mode?()
-  end
-
   @doc """
   Returns true when SMTP is managed by environment variables.
 
@@ -599,6 +599,7 @@ defmodule Mv.Config do
       []
       |> maybe_add_missing("SMTP_USERNAME", smtp_username_env_set?())
       |> maybe_add_missing("SMTP_PASSWORD/SMTP_PASSWORD_FILE", smtp_password_env_set?())
+      |> Enum.reverse()
     else
       []
     end
@@ -665,8 +666,6 @@ defmodule Mv.Config do
   @spec mail_from_email_env_set?() :: boolean()
   def mail_from_email_env_set?, do: env_set?("MAIL_FROM_EMAIL")
 
-  defp parse_smtp_port_env(nil), do: nil
-
   defp parse_smtp_port_env(value) when is_binary(value) do
     case Integer.parse(String.trim(value)) do
       {port, _} when port > 0 -> port
@@ -676,16 +675,8 @@ defmodule Mv.Config do
 
   defp parse_smtp_port_env(_), do: nil
 
-  # Reads a plain string SMTP setting: ENV first, then Settings.
-  defp smtp_env_or_setting(env_key, setting_key) do
-    case System.get_env(env_key) do
-      nil -> get_from_settings(setting_key)
-      value -> trim_nil(value)
-    end
-  end
-
   defp maybe_add_missing(acc, _label, true), do: acc
-  defp maybe_add_missing(acc, label, false), do: acc ++ [label]
+  defp maybe_add_missing(acc, label, false), do: [label | acc]
 
   # Reads an integer setting attribute from Settings.
   defp get_from_settings_integer(key) do

lib/mv_web/components/core_components.ex

@@ -1022,7 +1022,6 @@ defmodule MvWeb.CoreComponents do
             id={@row_id && @row_id.(row)}
             class={[
               table_row_tr_class(
-                @row_click,
                 table_row_selected?(assigns, row),
                 @sticky_first_col
               )
@@ -1142,17 +1141,8 @@ defmodule MvWeb.CoreComponents do
   # Returns CSS classes for table row selection styles.
   # Hover/focus row highlighting is CSS-driven via [data-row-interactive] selectors in app.css.
   # Sticky-first-column zebra tables use CSS accents and omit selected row ring classes.
-  defp table_row_tr_class(_row_click, selected?, sticky_first_col) do
+  defp table_row_tr_class(true, false), do: "ring-2 ring-inset ring-primary"
-    base = []
+  defp table_row_tr_class(_, _), do: ""
-
-    # Sticky-first-col tables: selection/hover accents are CSS-only (orange bar + fills).
-    base =
-      if selected? and not sticky_first_col,
-        do: base ++ ["ring-2", "ring-inset", "ring-primary"],
-        else: base
-
-    Enum.join(base, " ")
-  end
 
   defp table_th_aria_sort(col, sort_field, sort_order) do
     col_sort = Map.get(col, :sort_field)

lib/mv_web/live/global_settings_live.ex

@@ -85,16 +85,8 @@ defmodule MvWeb.GlobalSettingsLive do
       |> assign(:oidc_configured, Mv.Config.oidc_configured?())
       |> assign(:oidc_client_secret_set, Mv.Config.oidc_client_secret_set?())
       |> assign(:registration_enabled, settings.registration_enabled != false)
-      |> assign(:smtp_env_configured, Mv.Config.smtp_env_configured?())
       |> assign(:smtp_env_mode, Mv.Config.smtp_env_mode?())
       |> assign(:smtp_missing_required_env_keys, Mv.Config.smtp_missing_required_env_keys())
-      |> assign(:smtp_host_env_set, Mv.Config.smtp_host_env_set?())
-      |> assign(:smtp_port_env_set, Mv.Config.smtp_port_env_set?())
-      |> assign(:smtp_username_env_set, Mv.Config.smtp_username_env_set?())
-      |> assign(:smtp_password_env_set, Mv.Config.smtp_password_env_set?())
-      |> assign(:smtp_ssl_env_set, Mv.Config.smtp_ssl_env_set?())
-      |> assign(:smtp_from_name_env_set, Mv.Config.mail_from_name_env_set?())
-      |> assign(:smtp_from_email_env_set, Mv.Config.mail_from_email_env_set?())
       |> assign(:smtp_password_set, present?(Mv.Config.smtp_password()))
       |> assign(:smtp_configured, Mv.Config.smtp_configured?())
       |> assign(:smtp_test_result, nil)
@@ -361,19 +353,14 @@ defmodule MvWeb.GlobalSettingsLive do
                   type="text"
                   label={gettext("Host")}
                   disabled={@smtp_env_mode}
-                  placeholder={
+                  placeholder="smtp.example.com"
-                    if(@smtp_host_env_set,
-                      do: gettext("From SMTP_HOST"),
-                      else: "smtp.example.com"
-                    )
-                  }
                 />
                 <.input
                   field={@form[:smtp_port]}
                   type="number"
                   label={gettext("Port")}
                   disabled={@smtp_env_mode}
-                  placeholder={if(@smtp_port_env_set, do: gettext("From SMTP_PORT"), else: "587")}
+                  placeholder="587"
                 />
                 <.input
                   field={@form[:smtp_ssl]}
@@ -385,7 +372,6 @@ defmodule MvWeb.GlobalSettingsLive do
                     {gettext("SSL (port 465)"), "ssl"},
                     {gettext("None (port 25, insecure)"), "none"}
                   ]}
-                  placeholder={if(@smtp_ssl_env_set, do: gettext("From SMTP_SSL"), else: nil)}
                 />
               </div>
 
@@ -395,12 +381,7 @@ defmodule MvWeb.GlobalSettingsLive do
                   type="text"
                   label={gettext("Username")}
                   disabled={@smtp_env_mode}
-                  placeholder={
+                  placeholder="user@example.com"
-                    if(@smtp_username_env_set,
-                      do: gettext("From SMTP_USERNAME"),
-                      else: "user@example.com"
-                    )
-                  }
                 />
                 <.input
                   field={@form[:smtp_password]}
@@ -408,14 +389,11 @@ defmodule MvWeb.GlobalSettingsLive do
                   label={gettext("Password")}
                   disabled={@smtp_env_mode}
                   placeholder={
-                    if(@smtp_password_env_set,
+                    if @smtp_env_mode do
-                      do: gettext("From SMTP_PASSWORD"),
+                      gettext("From SMTP_PASSWORD")
-                      else:
+                    else
-                        if(@smtp_password_set,
+                      if @smtp_password_set, do: gettext("Leave blank to keep current"), else: nil
-                          do: gettext("Leave blank to keep current"),
+                    end
-                          else: nil
-                        )
-                    )
                   }
                 />
               </div>
@@ -426,21 +404,14 @@ defmodule MvWeb.GlobalSettingsLive do
                   type="email"
                   label={gettext("Sender email (From)")}
                   disabled={@smtp_env_mode}
-                  placeholder={
+                  placeholder="noreply@example.com"
-                    if(@smtp_from_email_env_set,
-                      do: gettext("From MAIL_FROM_EMAIL"),
-                      else: "noreply@example.com"
-                    )
-                  }
                 />
                 <.input
                   field={@form[:smtp_from_name]}
                   type="text"
                   label={gettext("Sender name (From)")}
                   disabled={@smtp_env_mode}
-                  placeholder={
+                  placeholder="Mila"
-                    if(@smtp_from_name_env_set, do: gettext("From MAIL_FROM_NAME"), else: "Mila")
-                  }
                 />
               </div>
             </div>
@@ -450,12 +421,7 @@ defmodule MvWeb.GlobalSettingsLive do
               )}
             </p>
             <.button
-              :if={
+              :if={not @smtp_env_mode}
-                not @smtp_env_mode and
-                  not (@smtp_host_env_set and @smtp_port_env_set and @smtp_username_env_set and
-                         @smtp_password_env_set and @smtp_ssl_env_set and @smtp_from_email_env_set and
-                         @smtp_from_name_env_set)
-              }
               phx-disable-with={gettext("Saving...")}
               variant="primary"
               class="mt-2"
@@ -941,9 +907,9 @@ defmodule MvWeb.GlobalSettingsLive do
           |> assign(:oidc_only, Mv.Config.oidc_only?())
           |> assign(:oidc_configured, Mv.Config.oidc_configured?())
           |> assign(:smtp_configured, Mv.Config.smtp_configured?())
+          |> assign(:smtp_env_mode, Mv.Config.smtp_env_mode?())
+          |> assign(:smtp_missing_required_env_keys, Mv.Config.smtp_missing_required_env_keys())
           |> assign(:smtp_password_set, present?(Mv.Config.smtp_password()))
-          |> assign(:smtp_from_name_env_set, Mv.Config.mail_from_name_env_set?())
-          |> assign(:smtp_from_email_env_set, Mv.Config.mail_from_email_env_set?())
           |> assign(:vereinfacht_test_result, test_result)
           |> put_flash(:success, gettext("Settings updated successfully"))
           |> assign_form()
@@ -1283,25 +1249,17 @@ defmodule MvWeb.GlobalSettingsLive do
   end
 
   defp merge_smtp_env_values(s) do
-    s
+    if Mv.Config.smtp_env_mode?() do
-    |> put_if_env_set(:smtp_host, Mv.Config.smtp_host_env_set?(), Mv.Config.smtp_host())
+      s
-    |> put_if_env_set(:smtp_port, Mv.Config.smtp_port_env_set?(), Mv.Config.smtp_port())
+      |> Map.put(:smtp_host, Mv.Config.smtp_host())
-    |> put_if_env_set(
+      |> Map.put(:smtp_port, Mv.Config.smtp_port())
-      :smtp_username,
+      |> Map.put(:smtp_username, Mv.Config.smtp_username())
-      Mv.Config.smtp_username_env_set?(),
+      |> Map.put(:smtp_ssl, Mv.Config.smtp_ssl())
-      Mv.Config.smtp_username()
+      |> Map.put(:smtp_from_email, Mv.Config.mail_from_email())
-    )
+      |> Map.put(:smtp_from_name, Mv.Config.mail_from_name())
-    |> put_if_env_set(:smtp_ssl, Mv.Config.smtp_ssl_env_set?(), Mv.Config.smtp_ssl())
+    else
-    |> put_if_env_set(
+      s
-      :smtp_from_email,
+    end
-      Mv.Config.mail_from_email_env_set?(),
-      Mv.Config.mail_from_email()
-    )
-    |> put_if_env_set(
-      :smtp_from_name,
-      Mv.Config.mail_from_name_env_set?(),
-      Mv.Config.mail_from_name()
-    )
   end
 
   defp enrich_sync_errors([]), do: []

priv/gettext/de/LC_MESSAGES/default.po

@@ -1384,16 +1384,6 @@ msgstr "Festgelegt nach der Erstellung. Mitglieder können nur zwischen Beitrags
 msgid "From %{first} to %{last} (relevant years with membership data)"
 msgstr "Von %{first} bis %{last} (Jahre mit Mitgliederdaten)"
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From MAIL_FROM_EMAIL"
-msgstr "Aus MAIL_FROM_EMAIL"
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From MAIL_FROM_NAME"
-msgstr "Aus MAIL_FROM_NAME"
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From OIDC_ADMIN_GROUP_NAME"
@@ -1429,31 +1419,11 @@ msgstr "Aus OIDC_ONLY"
 msgid "From OIDC_REDIRECT_URI"
 msgstr "Aus OIDC_REDIRECT_URI"
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_HOST"
-msgstr "Von SMTP_HOST"
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From SMTP_PASSWORD"
 msgstr "Von SMTP_PASSWORD"
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_PORT"
-msgstr "Von SMTP_PORT"
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_SSL"
-msgstr "Von SMTP_SSL"
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_USERNAME"
-msgstr "Von SMTP_USERNAME"
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From VEREINFACHT_API_KEY"

priv/gettext/default.pot

@@ -1385,16 +1385,6 @@ msgstr ""
 msgid "From %{first} to %{last} (relevant years with membership data)"
 msgstr ""
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From MAIL_FROM_EMAIL"
-msgstr ""
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From MAIL_FROM_NAME"
-msgstr ""
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From OIDC_ADMIN_GROUP_NAME"
@@ -1430,31 +1420,11 @@ msgstr ""
 msgid "From OIDC_REDIRECT_URI"
 msgstr ""
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_HOST"
-msgstr ""
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From SMTP_PASSWORD"
 msgstr ""
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_PORT"
-msgstr ""
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_SSL"
-msgstr ""
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_USERNAME"
-msgstr ""
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From VEREINFACHT_API_KEY"

priv/gettext/en/LC_MESSAGES/default.po

@@ -1385,16 +1385,6 @@ msgstr ""
 msgid "From %{first} to %{last} (relevant years with membership data)"
 msgstr ""
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From MAIL_FROM_EMAIL"
-msgstr ""
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From MAIL_FROM_NAME"
-msgstr ""
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From OIDC_ADMIN_GROUP_NAME"
@@ -1430,31 +1420,11 @@ msgstr ""
 msgid "From OIDC_REDIRECT_URI"
 msgstr ""
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_HOST"
-msgstr ""
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From SMTP_PASSWORD"
 msgstr ""
 
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_PORT"
-msgstr ""
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_SSL"
-msgstr ""
-
-#: lib/mv_web/live/global_settings_live.ex
-#, elixir-autogen, elixir-format
-msgid "From SMTP_USERNAME"
-msgstr ""
-
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "From VEREINFACHT_API_KEY"
@@ -3926,4 +3896,4 @@ msgstr ""
 #: lib/mv_web/live/global_settings_live.ex
 #, elixir-autogen, elixir-format
 msgid "SMTP is fully managed via environment variables. All SMTP fields are read-only."
-msgstr ""
+msgstr "SMTP is fully managed via environment variables. All SMTP fields are read-only."

test/mv/config_smtp_test.exs

@@ -2,7 +2,7 @@ defmodule Mv.ConfigSmtpTest do
   @moduledoc """
   Unit tests for Mv.Config SMTP-related helpers.
 
-  ENV overrides Settings (same pattern as OIDC/Vereinfacht). Uses real ENV and
+  SMTP uses ENV-only mode when SMTP_HOST is set. Uses real ENV and
   Settings; no mocking so we test the actual precedence. async: false because
   we mutate ENV.
   """
@@ -52,10 +52,10 @@ defmodule Mv.ConfigSmtpTest do
     end
   end
 
-  describe "smtp_env_configured?/0" do
+  describe "smtp_env_mode?/0" do
     test "returns true when SMTP_HOST is set" do
       set_smtp_env("SMTP_HOST", "smtp.example.com")
-      assert Mv.Config.smtp_env_configured?() == true
+      assert Mv.Config.smtp_env_mode?() == true
     after
       clear_smtp_env()
     end
@@ -63,14 +63,14 @@ defmodule Mv.ConfigSmtpTest do
     test "returns false when SMTP_HOST is not set even if other SMTP ENV variables are set" do
       set_smtp_env("SMTP_USERNAME", "user@example.com")
       set_smtp_env("SMTP_PASSWORD", "secret")
-      refute Mv.Config.smtp_env_configured?()
+      refute Mv.Config.smtp_env_mode?()
     after
       clear_smtp_env()
     end
 
     test "returns false when no SMTP ENV variables are set" do
       clear_smtp_env()
-      refute Mv.Config.smtp_env_configured?()
+      refute Mv.Config.smtp_env_mode?()
     end
   end
 

test/mv_web/live/global_settings_live_test.exs

@@ -1,5 +1,5 @@
 defmodule MvWeb.GlobalSettingsLiveTest do
-  use MvWeb.ConnCase, async: true
+  use MvWeb.ConnCase, async: false
   import Phoenix.LiveViewTest
   alias Mv.Membership
 
@@ -203,6 +203,20 @@ defmodule MvWeb.GlobalSettingsLiveTest do
       refute has_element?(view, "#setting_smtp_host[disabled]")
       refute has_element?(view, "#setting_smtp_username[disabled]")
     end
+
+    @tag :ui
+    test "shows effective ENV SMTP host value in disabled field", %{conn: conn} do
+      clear_smtp_env()
+      System.put_env("SMTP_HOST", "smtp.env-active.example")
+      on_exit(fn -> clear_smtp_env() end)
+
+      {:ok, settings} = Membership.get_settings()
+      {:ok, _} = Membership.update_settings(settings, %{smtp_host: "smtp.db-legacy.example"})
+
+      {:ok, _view, html} = live(conn, ~p"/settings")
+      assert html =~ ~s(value="smtp.env-active.example")
+      refute html =~ ~s(value="smtp.db-legacy.example")
+    end
   end
 
   describe "Authentication section when OIDC-only is enabled" do