From 01acea68381bc79e7f89e6b11d07522c7ec4f3bb Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 7 May 2026 10:01:19 +0200 Subject: [PATCH 1/5] fix: make sure smtp can be set either via env or ui --- CODE_GUIDELINES.md | 3 +- docs/smtp-configuration-concept.md | 13 ++- lib/mv/config.ex | 107 ++++++++++++++---- lib/mv_web/live/global_settings_live.ex | 40 +++++-- priv/gettext/de/LC_MESSAGES/default.po | 10 ++ priv/gettext/default.pot | 10 ++ priv/gettext/en/LC_MESSAGES/default.po | 10 ++ test/mv/config_smtp_test.exs | 22 +++- .../mv_web/live/global_settings_live_test.exs | 65 +++++++++++ 9 files changed, 238 insertions(+), 42 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index 0d478f9..d721a3a 100644 --- a/CODE_GUIDELINES.md +++ b/CODE_GUIDELINES.md @@ -1277,7 +1277,8 @@ 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 takes priority (same pattern as OIDC/Vereinfacht). +- 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`). - **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. diff --git a/docs/smtp-configuration-concept.md b/docs/smtp-configuration-concept.md index 4ae7760..6668485 100644 --- a/docs/smtp-configuration-concept.md +++ b/docs/smtp-configuration-concept.md @@ -25,7 +25,10 @@ Enable configurable SMTP for sending transactional emails (join confirmation, us | ENV | 1 | Production, Docker, 12-factor | | Settings | 2 | Admin UI, dev without ENV | -When an ENV variable is set, the corresponding Settings field is read-only in the UI (with hint "Set by environment"). +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. --- @@ -63,6 +66,14 @@ Support **SMTP_PASSWORD_FILE** (path to file containing the password), same patt - Show a warning in the Settings UI. - Delivery attempts silently fall back to the Local adapter (no crash). +### 6.1 Behaviour in ENV-only mode (`SMTP_HOST` set) + +- The SMTP source of truth is environment variables only. +- The UI does not allow editing SMTP fields in this mode. +- The Settings page shows a warning block when required values are missing: + - `SMTP_USERNAME` + - `SMTP_PASSWORD` or `SMTP_PASSWORD_FILE` + --- ## 7. Test Email (Settings UI) diff --git a/lib/mv/config.ex b/lib/mv/config.ex index 3494937..c807193 100644 --- a/lib/mv/config.ex +++ b/lib/mv/config.ex @@ -478,48 +478,61 @@ defmodule Mv.Config do end @doc """ - Returns SMTP port as integer. ENV `SMTP_PORT` (parsed) overrides Settings. - Returns nil when neither ENV nor Settings provide a valid port. + Returns SMTP port as integer. + + Policy: + - ENV-only mode (`SMTP_HOST` set): read from ENV `SMTP_PORT` + - Settings mode: read from Settings only """ @spec smtp_port() :: non_neg_integer() | nil def smtp_port do - case System.get_env("SMTP_PORT") do - nil -> - get_from_settings_integer(:smtp_port) - - value when is_binary(value) -> - case Integer.parse(String.trim(value)) do - {port, _} when port > 0 -> port - _ -> nil - end + if smtp_env_mode?() do + parse_smtp_port_env(System.get_env("SMTP_PORT")) + else + get_from_settings_integer(:smtp_port) end end @doc """ - Returns SMTP username. ENV `SMTP_USERNAME` overrides Settings. + Returns SMTP username. + + Policy: + - ENV-only mode (`SMTP_HOST` set): read from ENV `SMTP_USERNAME` + - Settings mode: read from Settings only """ @spec smtp_username() :: String.t() | nil def smtp_username do - smtp_env_or_setting("SMTP_USERNAME", :smtp_username) + if smtp_env_mode?() do + System.get_env("SMTP_USERNAME") |> trim_nil() + else + get_from_settings(:smtp_username) + end end @doc """ Returns SMTP password. - Priority: `SMTP_PASSWORD` ENV > `SMTP_PASSWORD_FILE` (file contents) > Settings. + Policy: + - ENV-only mode (`SMTP_HOST` set): `SMTP_PASSWORD` > `SMTP_PASSWORD_FILE` + - Settings mode: read from Settings only + Strips trailing whitespace/newlines from file contents. """ @spec smtp_password() :: String.t() | nil def smtp_password do - case System.get_env("SMTP_PASSWORD") do - nil -> smtp_password_from_file_or_settings() - value -> trim_nil(value) + if smtp_env_mode?() do + case System.get_env("SMTP_PASSWORD") do + nil -> smtp_password_from_file_or_settings() + value -> trim_nil(value) + end + else + get_smtp_password_from_settings() end end defp smtp_password_from_file_or_settings do case System.get_env("SMTP_PASSWORD_FILE") do - nil -> get_smtp_password_from_settings() + nil -> nil path -> read_smtp_password_file(path) end end @@ -533,11 +546,18 @@ defmodule Mv.Config do @doc """ Returns SMTP TLS/SSL mode string (e.g. 'tls', 'ssl', 'none'). - ENV `SMTP_SSL` overrides Settings. + + Policy: + - ENV-only mode (`SMTP_HOST` set): read from ENV `SMTP_SSL` + - Settings mode: read from Settings only """ @spec smtp_ssl() :: String.t() | nil def smtp_ssl do - smtp_env_or_setting("SMTP_SSL", :smtp_ssl) + if smtp_env_mode?() do + System.get_env("SMTP_SSL") |> trim_nil() + else + get_from_settings(:smtp_ssl) + end end @doc """ @@ -549,12 +569,39 @@ defmodule Mv.Config do end @doc """ - Returns true when any SMTP ENV variable is set (used in Settings UI for hints). + Returns true when SMTP ENV mode is active. """ @spec smtp_env_configured?() :: boolean() def smtp_env_configured? do - smtp_host_env_set?() or smtp_port_env_set?() or smtp_username_env_set?() or - smtp_password_env_set?() or smtp_ssl_env_set?() + smtp_env_mode?() + end + + @doc """ + Returns true when SMTP is managed by environment variables. + + Policy: if `SMTP_HOST` is set, SMTP is treated as ENV-only. + """ + @spec smtp_env_mode?() :: boolean() + def smtp_env_mode? do + smtp_host_env_set?() + end + + @doc """ + Returns missing required SMTP ENV keys for ENV-only mode warnings. + + Required in ENV-only mode: + - `SMTP_USERNAME` + - one of `SMTP_PASSWORD` or `SMTP_PASSWORD_FILE` + """ + @spec smtp_missing_required_env_keys() :: [String.t()] + def smtp_missing_required_env_keys do + if smtp_env_mode?() do + [] + |> maybe_add_missing("SMTP_USERNAME", smtp_username_env_set?()) + |> maybe_add_missing("SMTP_PASSWORD/SMTP_PASSWORD_FILE", smtp_password_env_set?()) + else + [] + end end @doc "Returns true if SMTP_HOST ENV is set." @@ -618,6 +665,17 @@ 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 + _ -> nil + end + end + + 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 @@ -626,6 +684,9 @@ defmodule Mv.Config do end end + defp maybe_add_missing(acc, _label, true), do: acc + defp maybe_add_missing(acc, label, false), do: acc ++ [label] + # Reads an integer setting attribute from Settings. defp get_from_settings_integer(key) do case Mv.Membership.get_settings() do diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 43851db..983f075 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/lib/mv_web/live/global_settings_live.ex @@ -86,6 +86,8 @@ defmodule MvWeb.GlobalSettingsLive do |> 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?()) @@ -321,12 +323,25 @@ defmodule MvWeb.GlobalSettingsLive do <%!-- SMTP / E-Mail Section --%> <.form_section title={gettext("SMTP / E-Mail")}> - <%= if @smtp_env_configured do %> + <%= if @smtp_env_mode do %>

- {gettext("Some values are set via environment variables. Those fields are read-only.")} + {gettext( + "SMTP is fully managed via environment variables. All SMTP fields are read-only." + )}

<% end %> + <%= if @smtp_env_mode and @smtp_missing_required_env_keys != [] do %> +
+ <.icon name="hero-exclamation-triangle" class="size-5 shrink-0 mt-0.5" /> + + {gettext("SMTP environment configuration appears incomplete. Missing: %{keys}", + keys: Enum.join(@smtp_missing_required_env_keys, ", ") + )} + +
+ <% end %> + <%= if @environment == :prod and not @smtp_configured do %>
<.icon name="hero-exclamation-triangle" class="size-5 shrink-0 mt-0.5" /> @@ -345,7 +360,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_host]} type="text" label={gettext("Host")} - disabled={@smtp_host_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_host_env_set, do: gettext("From SMTP_HOST"), @@ -357,14 +372,14 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_port]} type="number" label={gettext("Port")} - disabled={@smtp_port_env_set} + disabled={@smtp_env_mode} placeholder={if(@smtp_port_env_set, do: gettext("From SMTP_PORT"), else: "587")} /> <.input field={@form[:smtp_ssl]} type="select" label={gettext("TLS/SSL")} - disabled={@smtp_ssl_env_set} + disabled={@smtp_env_mode} options={[ {gettext("TLS (port 587, recommended)"), "tls"}, {gettext("SSL (port 465)"), "ssl"}, @@ -379,7 +394,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_username]} type="text" label={gettext("Username")} - disabled={@smtp_username_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_username_env_set, do: gettext("From SMTP_USERNAME"), @@ -391,7 +406,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_password]} type="password" label={gettext("Password")} - disabled={@smtp_password_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_password_env_set, do: gettext("From SMTP_PASSWORD"), @@ -410,7 +425,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_from_email]} type="email" label={gettext("Sender email (From)")} - disabled={@smtp_from_email_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_from_email_env_set, do: gettext("From MAIL_FROM_EMAIL"), @@ -422,7 +437,7 @@ defmodule MvWeb.GlobalSettingsLive do field={@form[:smtp_from_name]} type="text" label={gettext("Sender name (From)")} - disabled={@smtp_from_name_env_set} + disabled={@smtp_env_mode} placeholder={ if(@smtp_from_name_env_set, do: gettext("From MAIL_FROM_NAME"), else: "Mila") } @@ -436,9 +451,10 @@ defmodule MvWeb.GlobalSettingsLive do

<.button :if={ - 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) + 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" diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 52270cc..1482490 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/priv/gettext/de/LC_MESSAGES/default.po @@ -3917,3 +3917,13 @@ msgstr "Offen" #, elixir-autogen, elixir-format msgid "join page URL in a new tab" msgstr "Beitrittslink in einem neuen Tab" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP environment configuration appears incomplete. Missing: %{keys}" +msgstr "Die SMTP-Umgebungs-Konfiguration ist unvollständig. Fehlend: %{keys}" + +#: 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 "SMTP wird vollständig über Umgebungsvariablen verwaltet. Alle SMTP-Felder sind schreibgeschützt." diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 5d48691..21e7b16 100644 --- a/priv/gettext/default.pot +++ b/priv/gettext/default.pot @@ -3917,3 +3917,13 @@ msgstr "" #, elixir-autogen, elixir-format msgid "join page URL in a new tab" msgstr "" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP environment configuration appears incomplete. Missing: %{keys}" +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 "" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index ec6f305..7d94b7c 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/priv/gettext/en/LC_MESSAGES/default.po @@ -3917,3 +3917,13 @@ msgstr "Open" #, elixir-autogen, elixir-format msgid "join page URL in a new tab" msgstr "join page URL in a new tab" + +#: lib/mv_web/live/global_settings_live.ex +#, elixir-autogen, elixir-format +msgid "SMTP environment configuration appears incomplete. Missing: %{keys}" +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 "" diff --git a/test/mv/config_smtp_test.exs b/test/mv/config_smtp_test.exs index 5359366..7a1b895 100644 --- a/test/mv/config_smtp_test.exs +++ b/test/mv/config_smtp_test.exs @@ -23,7 +23,8 @@ defmodule Mv.ConfigSmtpTest do end describe "smtp_port/0" do - test "returns parsed integer when SMTP_PORT ENV is set" do + test "returns parsed integer when SMTP_PORT ENV is set in ENV-only mode" do + set_smtp_env("SMTP_HOST", "smtp.example.com") set_smtp_env("SMTP_PORT", "587") assert Mv.Config.smtp_port() == 587 after @@ -52,13 +53,21 @@ defmodule Mv.ConfigSmtpTest do end describe "smtp_env_configured?/0" do - test "returns true when any SMTP ENV variable is set" 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 after clear_smtp_env() end + 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?() + 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?() @@ -66,15 +75,17 @@ defmodule Mv.ConfigSmtpTest do end describe "smtp_password/0 and SMTP_PASSWORD_FILE" do - test "returns value from SMTP_PASSWORD when set" do + test "returns value from SMTP_PASSWORD when set in ENV-only mode" do + set_smtp_env("SMTP_HOST", "smtp.example.com") set_smtp_env("SMTP_PASSWORD", "env-secret") assert Mv.Config.smtp_password() == "env-secret" after clear_smtp_env() end - test "returns content of file when SMTP_PASSWORD_FILE is set and SMTP_PASSWORD is not" do + test "returns content of file when SMTP_PASSWORD_FILE is set in ENV-only mode and SMTP_PASSWORD is not" do clear_smtp_env() + set_smtp_env("SMTP_HOST", "smtp.example.com") path = Path.join(System.tmp_dir!(), "mv_smtp_test_#{System.unique_integer([:positive])}") File.write!(path, "file-secret\n") Process.put(:smtp_password_file_path, path) @@ -85,7 +96,8 @@ defmodule Mv.ConfigSmtpTest do if path = Process.get(:smtp_password_file_path), do: File.rm(path) end - test "SMTP_PASSWORD overrides SMTP_PASSWORD_FILE when both are set" do + test "SMTP_PASSWORD overrides SMTP_PASSWORD_FILE in ENV-only mode when both are set" do + set_smtp_env("SMTP_HOST", "smtp.example.com") path = Path.join(System.tmp_dir!(), "mv_smtp_test_#{System.unique_integer([:positive])}") File.write!(path, "file-secret") Process.put(:smtp_password_file_path, path) diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index 2edaf74..f90802a 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -124,6 +124,71 @@ defmodule MvWeb.GlobalSettingsLiveTest do {:ok, _view, html} = live(conn, ~p"/settings") assert html =~ "SMTP" or html =~ "E-Mail" or html =~ "Settings" end + + @tag :ui + test "disables all SMTP inputs when SMTP_HOST is set", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-only.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, view, _html} = live(conn, ~p"/settings") + + assert has_element?(view, "#setting_smtp_host[disabled]") + assert has_element?(view, "#setting_smtp_port[disabled]") + assert has_element?(view, "#setting_smtp_ssl[disabled]") + assert has_element?(view, "#setting_smtp_username[disabled]") + assert has_element?(view, "#setting_smtp_password[disabled]") + assert has_element?(view, "#setting_smtp_from_email[disabled]") + assert has_element?(view, "#setting_smtp_from_name[disabled]") + end + + @tag :ui + test "does not render SMTP save action when SMTP_HOST is set", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-only.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, view, _html} = live(conn, ~p"/settings") + refute has_element?(view, "#smtp-form button", "Save SMTP Settings") + end + + @tag :ui + test "shows explicit ENV-only mode hint when SMTP_HOST is set", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-only.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, _view, html} = live(conn, ~p"/settings") + assert html =~ "SMTP is fully managed via environment variables" + end + + @tag :ui + test "shows warning block for missing required SMTP ENV values in ENV-only mode", %{ + conn: conn + } do + clear_smtp_env() + System.put_env("SMTP_HOST", "smtp.env-only.example") + on_exit(fn -> clear_smtp_env() end) + + {:ok, _view, html} = live(conn, ~p"/settings") + assert html =~ "SMTP environment configuration appears incomplete" + assert html =~ "SMTP_USERNAME" + assert html =~ "SMTP_PASSWORD/SMTP_PASSWORD_FILE" + end + + @tag :ui + test "does not enter ENV-only mode when SMTP_HOST is not set", %{conn: conn} do + clear_smtp_env() + System.put_env("SMTP_USERNAME", "leftover@example.com") + on_exit(fn -> clear_smtp_env() end) + + {:ok, view, html} = live(conn, ~p"/settings") + + refute html =~ "SMTP is fully managed via environment variables" + refute html =~ "SMTP environment configuration appears incomplete" + refute has_element?(view, "#setting_smtp_host[disabled]") + refute has_element?(view, "#setting_smtp_username[disabled]") + end end describe "Authentication section when OIDC-only is enabled" do -- 2.47.2 From 3d6081e024fa8c0cf31210e22b0d5beac6e86ec8 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 7 May 2026 13:08:21 +0200 Subject: [PATCH 2/5] fix: make horizontal scrollbars sticky to bottom --- lib/mv_web/components/core_components.ex | 7 +- lib/mv_web/live/member_live/index.html.heex | 1 + .../components/core_components_table_test.exs | 82 +++++++++++++++++++ .../mv_web/live/global_settings_live_test.exs | 14 ++++ test/mv_web/member_live/index_test.exs | 12 +++ 5 files changed, 115 insertions(+), 1 deletion(-) diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex index b5bd763..2eb3051 100644 --- a/lib/mv_web/components/core_components.ex +++ b/lib/mv_web/components/core_components.ex @@ -938,6 +938,11 @@ defmodule MvWeb.CoreComponents do doc: "when true, thead th get lg:sticky lg:top-0 bg-base-100 z-10 for use inside a scroll container on desktop" + attr :wrapper_overflow_class, :string, + default: "overflow-x-auto", + doc: + "overflow class for the table wrapper; set to overflow-visible when outer container owns scrolling" + slot :col, required: true do attr :label, :string attr :class, :string @@ -974,7 +979,7 @@ defmodule MvWeb.CoreComponents do ~H"""
diff --git a/lib/mv_web/live/member_live/index.html.heex b/lib/mv_web/live/member_live/index.html.heex index 4d86a62..92f19b8 100644 --- a/lib/mv_web/live/member_live/index.html.heex +++ b/lib/mv_web/live/member_live/index.html.heex @@ -105,6 +105,7 @@ <.table id="members" rows={@members} + wrapper_overflow_class="overflow-visible" sticky_header={true} row_id={fn member -> "row-#{member.id}" end} row_click={fn member -> JS.push("select_row_and_navigate", value: %{id: member.id}) end} diff --git a/test/mv_web/components/core_components_table_test.exs b/test/mv_web/components/core_components_table_test.exs index 931b42a..45e8c6d 100644 --- a/test/mv_web/components/core_components_table_test.exs +++ b/test/mv_web/components/core_components_table_test.exs @@ -151,4 +151,86 @@ defmodule MvWeb.Components.CoreComponentsTableTest do assert html =~ "ring-primary" end end + + describe "table scroll wrapper contract" do + test "sticky header table uses horizontal-only overflow wrapper" do + rows = [%{id: "1", name: "Alice"}] + + assigns = %{ + id: "test-table", + rows: rows, + row_id: fn r -> "row-#{r.id}" end, + row_click: nil, + sticky_header: true, + row_item: &Function.identity/1, + col: [ + %{ + __slot__: :col, + label: "Name", + inner_block: fn _socket, item -> [item[:name] || ""] end + } + ], + dynamic_cols: [], + action: [] + } + + html = render_component(&CoreComponents.table/1, assigns) + + assert html =~ ~s(class="overflow-x-auto") + refute html =~ ~s(class="overflow-auto") + end + + test "table wrapper does not enable vertical overflow by default" do + rows = [%{id: "1", name: "Alice"}] + + assigns = %{ + id: "test-table", + rows: rows, + row_id: fn r -> "row-#{r.id}" end, + row_click: nil, + row_item: &Function.identity/1, + col: [ + %{ + __slot__: :col, + label: "Name", + inner_block: fn _socket, item -> [item[:name] || ""] end + } + ], + dynamic_cols: [], + action: [] + } + + html = render_component(&CoreComponents.table/1, assigns) + + assert html =~ ~s(class="overflow-x-auto") + refute html =~ ~s(class="overflow-auto") + end + + test "table wrapper overflow class can be overridden by caller" do + rows = [%{id: "1", name: "Alice"}] + + assigns = %{ + id: "test-table", + rows: rows, + row_id: fn r -> "row-#{r.id}" end, + row_click: nil, + wrapper_overflow_class: "overflow-visible", + row_item: &Function.identity/1, + col: [ + %{ + __slot__: :col, + label: "Name", + inner_block: fn _socket, item -> [item[:name] || ""] end + } + ], + dynamic_cols: [], + action: [] + } + + html = render_component(&CoreComponents.table/1, assigns) + + assert html =~ ~s(class="overflow-visible") + refute html =~ ~s(class="overflow-x-auto") + end + end end diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index f90802a..4283ec0 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/test/mv_web/live/global_settings_live_test.exs @@ -3,6 +3,20 @@ defmodule MvWeb.GlobalSettingsLiveTest do import Phoenix.LiveViewTest alias Mv.Membership + defp clear_smtp_env do + [ + "SMTP_HOST", + "SMTP_PORT", + "SMTP_SSL", + "SMTP_USERNAME", + "SMTP_PASSWORD", + "SMTP_PASSWORD_FILE", + "MAIL_FROM_EMAIL", + "MAIL_FROM_NAME" + ] + |> Enum.each(&System.delete_env/1) + end + describe "Global Settings LiveView" do setup %{conn: conn} do user = create_test_user(%{email: "admin@example.com"}) diff --git a/test/mv_web/member_live/index_test.exs b/test/mv_web/member_live/index_test.exs index 686a8e8..95c71ed 100644 --- a/test/mv_web/member_live/index_test.exs +++ b/test/mv_web/member_live/index_test.exs @@ -78,6 +78,18 @@ defmodule MvWeb.MemberLive.IndexTest do assert html =~ "lg:top-0" assert html =~ "bg-base-100" end + + test "members page does not nest a second overflow wrapper inside members-table-scroll", %{ + conn: conn + } do + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/members") + + assert html =~ ~s(id="members-keyboard") + assert html =~ ~s(class="overflow-visible") + refute html =~ ~s(id="members-keyboard" class="overflow-x-auto") + refute html =~ ~s(id="members-keyboard" class="overflow-auto") + end end describe "translations" do -- 2.47.2 From f3043df58b9b46fd7c69f144c4540c283e61ae13 Mon Sep 17 00:00:00 2001 From: Simon Date: Thu, 7 May 2026 13:08:35 +0200 Subject: [PATCH 3/5] docs: update changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index c17ea39..4497a15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed - **Runtime ENV handling** – Empty or invalid environment variables (e.g. `SMTP_PORT=`, `PORT=`, `POOL_SIZE=`, `DATABASE_PORT=`) no longer cause `ArgumentError` at boot. Instead raises clear errors for required vars set but empty (e.g. DATABASE_HOST, PHX_HOST/DOMAIN, SECRET_KEY_BASE). - **PostgreSQL 18 Docker volume path** – Corrected the database volume path to match PostgreSQL 18 expectations. +- **Association name ENV handling** – `ASSOCIATION_NAME` is now treated as source of truth; the field is read-only in Global Settings when managed via ENV. +- **Association name consistency after updates** – Layout now prefers explicitly assigned `club_name` values to avoid stale cached values right after settings changes. +- **SMTP ENV/UI source selection** – SMTP now follows a strict single-source policy: ENV-only when `SMTP_HOST` is set, otherwise Settings-only. +- **SMTP settings UI in ENV mode** – SMTP fields are read-only, save action is hidden, and missing required ENV keys are shown as a warning. ### Dependency updates - Mix dependencies were updated. -- 2.47.2 From 93e1ec741424c482fed414da3a434878f1ec8310 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 8 May 2026 11:37:04 +0200 Subject: [PATCH 4/5] feat: make checkbox column in member view sticky --- CHANGELOG.md | 2 + DESIGN_GUIDELINES.md | 4 +- assets/css/app.css | 65 +++++++++++ lib/mv_web/components/core_components.ex | 55 +++++---- lib/mv_web/live/member_live/index.html.heex | 1 + .../components/core_components_table_test.exs | 110 ++++++++++++++++-- test/mv_web/member_live/index_test.exs | 32 ++++- 7 files changed, 234 insertions(+), 35 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4497a15..e21f4a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] ### Changed +- **Clickable table row highlights** – The new hover/focus-visible row highlight behavior is now the CoreComponents default across clickable tables. Sticky-first-column tables keep zebra striping and show selection through the sticky-column accent stripe (checkboxes keep their default style). +- **Members overview scrolling** – The members table scrollbar now scrolls inside the table container instead of moving with the full page. - **Join request display and settings workflow** – Improved join request rendering and related settings behavior in one cohesive update: - Join request fields now respect their configured field types in the details view. - Custom field labels in join request views were standardized. diff --git a/DESIGN_GUIDELINES.md b/DESIGN_GUIDELINES.md index 0ad562e..34c71b8 100644 --- a/DESIGN_GUIDELINES.md +++ b/DESIGN_GUIDELINES.md @@ -247,11 +247,13 @@ If these cannot be met, use `secondary`/`outline` instead of `ghost`. ### 8.1 Default behavior: row click opens details - **DEFAULT:** Clicking a row navigates to the details page. - **EXCEPTIONS:** Highly interactive rows may disable row-click (document why). -- **Row outline (CoreComponents):** When `row_click` is set, rows get a subtle hover and focus-within ring (theme-friendly). Use `selected_row_id` to show a stronger selected outline (e.g. from URL `?highlight=id` or last selection); the Back link from detail can use `?highlight=id` so the row is visually selected when returning to the index. +- **Row highlight (CoreComponents):** When `row_click` is set, rows use a neutral background highlight on `hover` and `tr:has(:focus-visible)` (see `assets/css/app.css`), so keyboard focus is visible while mouse-only focus does not appear "stuck". For non-sticky tables, `selected_row_id` can still add a stronger selected ring. For sticky-first-column tables, selection emphasis is handled by the sticky-column accent stripe. **IMPORTANT (correctness with our `<.table>` CoreComponent):** Our table implementation attaches the `phx-click` to the **`
`** when `row_click` is set. That means click events bubble from inner elements up to the cell unless we stop propagation. +**LiveStream rows:** Do not enumerate `@rows` with `Enum.with_index` in the table template; streams must be consumed only through `:for`. Sticky-first-column zebra striping for those tables is handled in CSS (`nth-child` under `data-sticky-first-col-rows`), not by assigning odd/even classes from an index. + So, for interactive elements inside a clickable row, you must **stop propagation using `Phoenix.LiveView.JS.stop_propagation/1`**, not a custom attribute. ✅ Correct pattern (one click handler that both stops propagation and triggers an event): diff --git a/assets/css/app.css b/assets/css/app.css index d7f873c..611e9ad 100644 --- a/assets/css/app.css +++ b/assets/css/app.css @@ -708,3 +708,68 @@ background-color: transparent !important; color: inherit; } + +/* + * Default interactive table rows: neutral hover/focus-visible fill for clickable rows. + * Uses :has(:focus-visible) so keyboard navigation highlights the row without sticky mouse-focus artifacts. + */ +.table.table-zebra tbody tr[data-row-interactive="true"]:is(:hover, :has(:focus-visible)) > td { + background-color: var(--color-base-300); +} + +/* + * Sticky first column in zebra tables: opaque backgrounds per row. + * Use nth-child (not HEEx row index) so LiveStream rows stay iterable only via :for (Phoenix LV requirement). + */ +[data-sticky-first-col-rows="true"] .table.table-zebra tbody tr:nth-child(odd) > td.sticky-first-col-cell { + background-color: var(--color-base-100); +} + +[data-sticky-first-col-rows="true"] .table.table-zebra tbody tr:nth-child(even) > td.sticky-first-col-cell { + background-color: var(--color-base-200); +} + +/* + * Checkbox-selected rows: keep zebra backgrounds; only accent the sticky checkbox column. + */ +[data-sticky-first-col-rows="true"] + .table.table-zebra + tbody + tr[data-selected="true"] + > td.sticky-first-col-cell { + box-shadow: inset 2px 0 0 var(--color-primary); +} + +[data-sticky-first-col-rows="true"] + .table.table-zebra + tbody + tr[data-row-interactive="true"]:is(:hover, :has(:focus-visible)) + > td.sticky-first-col-cell { + background-color: var(--color-base-300); + /* Left accent only; keep the familiar orange primary accent. */ + box-shadow: inset 2px 0 0 var(--color-primary); +} + +/* + * Sticky member selection table: drop mouse-only focus outlines that read like a thin frame around the row; + * keyboard :focus-visible keeps DaisyUI control outlines (checkbox / tabindex cell). + */ +[data-sticky-first-col-rows="true"] .table.table-zebra tbody tr { + outline: none; +} + +[data-sticky-first-col-rows="true"] + .table.table-zebra + tbody + tr[data-row-interactive="true"]:is(:hover, :has(:focus-visible)):not(:last-child) { + /* DaisyUI draws a bottom border on each row; hiding it while highlighted avoids a boxy “frame”. */ + border-bottom-color: transparent; +} + +[data-sticky-first-col-rows="true"] .table.table-zebra tbody tr td:focus:not(:focus-visible) { + outline: none; +} + +[data-sticky-first-col-rows="true"] .table.table-zebra tbody tr input.checkbox:focus:not(:focus-visible) { + outline: none; +} diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex index 2eb3051..4e55cf7 100644 --- a/lib/mv_web/components/core_components.ex +++ b/lib/mv_web/components/core_components.ex @@ -943,6 +943,11 @@ defmodule MvWeb.CoreComponents do doc: "overflow class for the table wrapper; set to overflow-visible when outer container owns scrolling" + attr :sticky_first_col, :boolean, + default: false, + doc: + "when true, first header/body column gets sticky left positioning to keep selection controls visible" + slot :col, required: true do attr :label, :string attr :class, :string @@ -980,14 +985,18 @@ defmodule MvWeb.CoreComponents do
@@ -1031,6 +1047,13 @@ defmodule MvWeb.CoreComponents do has_click = col[:col_click] || @row_click classes = ["max-w-xs"] + classes = + if @sticky_first_col && col_idx == 0 do + ["sticky-first-col-cell sticky left-0 z-20" | classes] + else + classes + end + classes = if col_class == nil || (col_class && !String.contains?(col_class, "text-center")) do ["truncate" | classes] @@ -1045,7 +1068,7 @@ defmodule MvWeb.CoreComponents do classes end - # WCAG: no focus ring on the cell itself; row shows focus via focus-within + # WCAG: no focus ring on the cell itself; sticky zebra rows show keyboard focus via CSS :has(:focus-visible) classes = if @row_click && @first_row_click_col_idx == col_idx do [ @@ -1116,28 +1139,18 @@ defmodule MvWeb.CoreComponents do end end - # Returns CSS classes for table row: hover/focus-within outline when row_click is set, - # and stronger selected outline when selected (WCAG: not color-only). - # Hover/focus-within are omitted for the selected row so the selected ring stays visible. - defp table_row_tr_class(row_click, selected?) do - has_row_click? = not is_nil(row_click) + # 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 base = [] + # Sticky-first-col tables: selection/hover accents are CSS-only (orange bar + fills). base = - if has_row_click? and not selected?, - do: - base ++ - [ - "hover:ring-2", - "hover:ring-inset", - "hover:ring-base-content/10", - "focus-within:ring-2", - "focus-within:ring-inset", - "focus-within:ring-base-content/10" - ], + if selected? and not sticky_first_col, + do: base ++ ["ring-2", "ring-inset", "ring-primary"], else: base - base = if selected?, do: base ++ ["ring-2", "ring-inset", "ring-primary"], else: base Enum.join(base, " ") end diff --git a/lib/mv_web/live/member_live/index.html.heex b/lib/mv_web/live/member_live/index.html.heex index 92f19b8..efc1eb7 100644 --- a/lib/mv_web/live/member_live/index.html.heex +++ b/lib/mv_web/live/member_live/index.html.heex @@ -107,6 +107,7 @@ rows={@members} wrapper_overflow_class="overflow-visible" sticky_header={true} + sticky_first_col={true} row_id={fn member -> "row-#{member.id}" end} row_click={fn member -> JS.push("select_row_and_navigate", value: %{id: member.id}) end} row_tooltip={gettext("Click for member details")} diff --git a/test/mv_web/components/core_components_table_test.exs b/test/mv_web/components/core_components_table_test.exs index 45e8c6d..03f1f71 100644 --- a/test/mv_web/components/core_components_table_test.exs +++ b/test/mv_web/components/core_components_table_test.exs @@ -9,7 +9,7 @@ defmodule MvWeb.Components.CoreComponentsTableTest do alias MvWeb.CoreComponents describe "table row_click styling" do - test "when row_click is set, table rows have hover and focus-within ring classes" do + test "when row_click is set, rows are marked interactive and omit ring hover classes" do rows = [%{id: "1", name: "Alice"}, %{id: "2", name: "Bob"}] assigns = %{ @@ -31,12 +31,12 @@ defmodule MvWeb.Components.CoreComponentsTableTest do html = render_component(&CoreComponents.table/1, assigns) - assert html =~ "hover:ring-2" - assert html =~ "focus-within:ring-2" - assert html =~ "hover:ring-base-content/10" + assert html =~ ~s(data-row-interactive="true") + refute html =~ "hover:ring-2" + refute html =~ "focus-within:ring-2" end - test "when row_click is nil, table rows do not have hover ring classes" do + test "when row_click is nil, rows are not marked interactive" do rows = [%{id: "1", name: "Alice"}] assigns = %{ @@ -58,8 +58,7 @@ defmodule MvWeb.Components.CoreComponentsTableTest do html = render_component(&CoreComponents.table/1, assigns) - refute html =~ "hover:ring-2" - refute html =~ "focus-within:ring-2" + refute html =~ ~s(data-row-interactive="true") end end @@ -233,4 +232,101 @@ defmodule MvWeb.Components.CoreComponentsTableTest do refute html =~ ~s(class="overflow-x-auto") end end + + describe "sticky first column contract" do + test "when sticky_first_col is enabled, first header and body cells render sticky-left classes" do + rows = [%{id: "1", selected: true, name: "Alice"}] + + assigns = %{ + id: "test-table", + rows: rows, + row_id: fn r -> "row-#{r.id}" end, + row_click: nil, + sticky_first_col: true, + row_item: &Function.identity/1, + col: [ + %{ + __slot__: :col, + label: "Select", + inner_block: fn _socket, item -> [if(item[:selected], do: "x", else: "")] end + }, + %{ + __slot__: :col, + label: "Name", + inner_block: fn _socket, item -> [item[:name] || ""] end + } + ], + dynamic_cols: [], + action: [] + } + + html = render_component(&CoreComponents.table/1, assigns) + + assert html =~ "sticky" + assert html =~ "left-0" + assert html =~ "z-20" + assert html =~ "z-30" + end + + test "sticky first column marks wrapper and uses CSS row backgrounds instead of row ring classes" do + rows = [%{id: "1", name: "Alice"}] + + assigns = %{ + id: "test-table", + rows: rows, + row_id: fn r -> "row-#{r.id}" end, + row_click: fn _ -> nil end, + sticky_first_col: true, + row_item: &Function.identity/1, + col: [ + %{ + __slot__: :col, + label: "Select", + inner_block: fn _socket, _item -> ["x"] end + }, + %{ + __slot__: :col, + label: "Name", + inner_block: fn _socket, item -> [item[:name] || ""] end + } + ], + dynamic_cols: [], + action: [] + } + + html = render_component(&CoreComponents.table/1, assigns) + + assert html =~ ~s(data-sticky-first-col-rows="true") + assert html =~ "sticky-first-col-cell" + refute html =~ "hover:ring-2" + end + + test "sticky first column with selection sets data-selected without ring-primary" do + rows = [%{id: "one", name: "Alice"}, %{id: "two", name: "Bob"}] + + assigns = %{ + id: "test-table", + rows: rows, + row_id: fn r -> "row-#{r.id}" end, + row_click: fn _ -> nil end, + sticky_first_col: true, + selected_row_id: "two", + row_item: &Function.identity/1, + col: [ + %{ + __slot__: :col, + label: "Name", + inner_block: fn _socket, item -> [item[:name] || ""] end + } + ], + dynamic_cols: [], + action: [] + } + + html = render_component(&CoreComponents.table/1, assigns) + + assert html =~ ~s(data-selected="true") + refute html =~ "ring-primary" + end + end end diff --git a/test/mv_web/member_live/index_test.exs b/test/mv_web/member_live/index_test.exs index 95c71ed..85c3385 100644 --- a/test/mv_web/member_live/index_test.exs +++ b/test/mv_web/member_live/index_test.exs @@ -90,6 +90,25 @@ defmodule MvWeb.MemberLive.IndexTest do refute html =~ ~s(id="members-keyboard" class="overflow-x-auto") refute html =~ ~s(id="members-keyboard" class="overflow-auto") end + + test "members table keeps checkbox column sticky while horizontally scrolling", %{conn: conn} do + system_actor = SystemActor.get_system_actor() + + {:ok, _member} = + Membership.create_member( + %{first_name: "Sticky", last_name: "Column", email: "sticky-column@example.com"}, + actor: system_actor + ) + + conn = conn_with_oidc_user(conn) + {:ok, _view, html} = live(conn, ~p"/members") + + # Contract: first column (select-all header + row checkbox cells) is sticky on the left + assert html =~ "left-0" + assert html =~ "sticky" + assert html =~ "z-30" + assert html =~ "z-20" + end end describe "translations" do @@ -351,10 +370,12 @@ defmodule MvWeb.MemberLive.IndexTest do assert_redirect(view, ~p"/members/#{member}") end - describe "table row outline (hover and selected)" do + describe "table row highlight (hover and selected)" do @describetag :ui - test "clickable rows have hover and focus-within ring classes", %{conn: conn} do + test "clickable rows with sticky first column use hover/focus background highlight", %{ + conn: conn + } do system_actor = SystemActor.get_system_actor() {:ok, _member} = @@ -366,10 +387,9 @@ defmodule MvWeb.MemberLive.IndexTest do conn = conn_with_oidc_user(conn) {:ok, _view, html} = live(conn, "/members") - # CoreComponents table adds hover and focus-within ring when row_click is set - assert html =~ "hover:ring-2" - assert html =~ "focus-within:ring-2" - assert html =~ "hover:ring-base-content/10" + # Sticky-first-column tables: hover/focus fills live in CSS; wrapper is marked for tests. + assert html =~ ~s(data-sticky-first-col-rows="true") + refute html =~ "hover:ring-2" end test "selected outline only from checkbox selection, not from highlight param", %{conn: conn} do -- 2.47.2 From b1740e34355663e5eda3268670f81a89adaee198 Mon Sep 17 00:00:00 2001 From: Simon Date: Fri, 8 May 2026 12:45:57 +0200 Subject: [PATCH 5/5] refactor: fix review comments --- CODE_GUIDELINES.md | 2 +- docs/smtp-configuration-concept.md | 1 + lib/mv/config.ex | 37 +++----- lib/mv_web/components/core_components.ex | 14 +-- lib/mv_web/live/global_settings_live.ex | 90 +++++-------------- priv/gettext/de/LC_MESSAGES/default.po | 30 ------- priv/gettext/default.pot | 30 ------- priv/gettext/en/LC_MESSAGES/default.po | 32 +------ test/mv/config_smtp_test.exs | 10 +-- .../mv_web/live/global_settings_live_test.exs | 16 +++- 10 files changed, 63 insertions(+), 199 deletions(-) diff --git a/CODE_GUIDELINES.md b/CODE_GUIDELINES.md index d721a3a..2b378ef 100644 --- a/CODE_GUIDELINES.md +++ b/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. diff --git a/docs/smtp-configuration-concept.md b/docs/smtp-configuration-concept.md index 6668485..b19f6e4 100644 --- a/docs/smtp-configuration-concept.md +++ b/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. --- diff --git a/lib/mv/config.ex b/lib/mv/config.ex index c807193..870d1d3 100644 --- a/lib/mv/config.ex +++ b/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 diff --git a/lib/mv_web/components/core_components.ex b/lib/mv_web/components/core_components.ex index 4e55cf7..465d41a 100644 --- a/lib/mv_web/components/core_components.ex +++ b/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 - base = [] - - # 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_row_tr_class(true, false), do: "ring-2 ring-inset ring-primary" + defp table_row_tr_class(_, _), do: "" defp table_th_aria_sort(col, sort_field, sort_order) do col_sort = Map.get(col, :sort_field) diff --git a/lib/mv_web/live/global_settings_live.ex b/lib/mv_web/live/global_settings_live.ex index 983f075..6a1c926 100644 --- a/lib/mv_web/live/global_settings_live.ex +++ b/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={ - if(@smtp_host_env_set, - do: gettext("From SMTP_HOST"), - else: "smtp.example.com" - ) - } + placeholder="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)} /> @@ -395,12 +381,7 @@ defmodule MvWeb.GlobalSettingsLive do type="text" label={gettext("Username")} disabled={@smtp_env_mode} - placeholder={ - if(@smtp_username_env_set, - do: gettext("From SMTP_USERNAME"), - else: "user@example.com" - ) - } + placeholder="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, - do: gettext("From SMTP_PASSWORD"), - else: - if(@smtp_password_set, - do: gettext("Leave blank to keep current"), - else: nil - ) - ) + if @smtp_env_mode do + gettext("From SMTP_PASSWORD") + else + if @smtp_password_set, do: gettext("Leave blank to keep current"), else: nil + end } /> @@ -426,21 +404,14 @@ defmodule MvWeb.GlobalSettingsLive do type="email" label={gettext("Sender email (From)")} disabled={@smtp_env_mode} - placeholder={ - if(@smtp_from_email_env_set, - do: gettext("From MAIL_FROM_EMAIL"), - else: "noreply@example.com" - ) - } + placeholder="noreply@example.com" /> <.input field={@form[:smtp_from_name]} type="text" label={gettext("Sender name (From)")} disabled={@smtp_env_mode} - placeholder={ - if(@smtp_from_name_env_set, do: gettext("From MAIL_FROM_NAME"), else: "Mila") - } + placeholder="Mila" /> @@ -450,12 +421,7 @@ defmodule MvWeb.GlobalSettingsLive do )}

<.button - :if={ - 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) - } + :if={not @smtp_env_mode} 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 - |> put_if_env_set(:smtp_host, Mv.Config.smtp_host_env_set?(), Mv.Config.smtp_host()) - |> put_if_env_set(:smtp_port, Mv.Config.smtp_port_env_set?(), Mv.Config.smtp_port()) - |> put_if_env_set( - :smtp_username, - Mv.Config.smtp_username_env_set?(), - Mv.Config.smtp_username() - ) - |> put_if_env_set(:smtp_ssl, Mv.Config.smtp_ssl_env_set?(), Mv.Config.smtp_ssl()) - |> put_if_env_set( - :smtp_from_email, - 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() - ) + if Mv.Config.smtp_env_mode?() do + s + |> Map.put(:smtp_host, Mv.Config.smtp_host()) + |> Map.put(:smtp_port, Mv.Config.smtp_port()) + |> Map.put(:smtp_username, Mv.Config.smtp_username()) + |> Map.put(:smtp_ssl, Mv.Config.smtp_ssl()) + |> Map.put(:smtp_from_email, Mv.Config.mail_from_email()) + |> Map.put(:smtp_from_name, Mv.Config.mail_from_name()) + else + s + end end defp enrich_sync_errors([]), do: [] diff --git a/priv/gettext/de/LC_MESSAGES/default.po b/priv/gettext/de/LC_MESSAGES/default.po index 1482490..a85b4cf 100644 --- a/priv/gettext/de/LC_MESSAGES/default.po +++ b/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" diff --git a/priv/gettext/default.pot b/priv/gettext/default.pot index 21e7b16..b995b1a 100644 --- a/priv/gettext/default.pot +++ b/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" diff --git a/priv/gettext/en/LC_MESSAGES/default.po b/priv/gettext/en/LC_MESSAGES/default.po index 7d94b7c..f4526d1 100644 --- a/priv/gettext/en/LC_MESSAGES/default.po +++ b/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." diff --git a/test/mv/config_smtp_test.exs b/test/mv/config_smtp_test.exs index 7a1b895..ebf2b83 100644 --- a/test/mv/config_smtp_test.exs +++ b/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 diff --git a/test/mv_web/live/global_settings_live_test.exs b/test/mv_web/live/global_settings_live_test.exs index 4283ec0..9be12b9 100644 --- a/test/mv_web/live/global_settings_live_test.exs +++ b/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 -- 2.47.2
{col[:label]} @@ -1011,7 +1020,14 @@ defmodule MvWeb.CoreComponents do