fix email settings: missing providers, a11y, no-JS support

show all 10 providers in three groups (popular, transactional, advanced) with category headings. fix phx-change clobbering text fields, async test email sending state, integer parse crash on bad port. add keyboard focus on card radios, fieldset legend, WCAG-compliant badge contrast, responsive grid. extract shared save_config into Mailer, add no-JS controller fallback with configured_adapter hidden field for adapter change detection. remove CardRadioScroll JS hook (no longer needed). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
2026-03-04 21:26:59 +00:00
parent dd659e4c61
commit dd20ea824f
9 changed files with 637 additions and 235 deletions
--- a/test/berrypod/mailer_test.exs
+++ b/test/berrypod/mailer_test.exs
@@ -207,6 +207,75 @@ defmodule Berrypod.MailerTest do
    end
  end

+  describe "save_config/3" do
+    test "saves valid adapter config" do
+      assert {:ok, _} =
+               Mailer.save_config("brevo", %{"api_key" => "xkeysib-abc123def456"}, "a@b.com")
+
+      assert Settings.get_setting("email_adapter") == "brevo"
+      assert Settings.has_secret?("email_brevo_api_key")
+    end
+
+    test "returns error for missing required fields" do
+      assert {:error, errors} = Mailer.save_config("brevo", %{"api_key" => ""}, "a@b.com")
+      assert errors["api_key"] =~ "required"
+    end
+
+    test "returns error for invalid key format" do
+      assert {:error, errors} =
+               Mailer.save_config("sendgrid", %{"api_key" => "not-a-sendgrid-key"}, "a@b.com")
+
+      assert errors["api_key"] =~ "SG."
+    end
+
+    test "returns error for invalid integer" do
+      assert {:error, errors} =
+               Mailer.save_config(
+                 "smtp",
+                 %{"relay" => "smtp.example.com", "port" => "abc"},
+                 "a@b.com"
+               )
+
+      assert errors["port"] =~ "number"
+    end
+
+    test "returns error for unknown adapter" do
+      assert {:error, errors} = Mailer.save_config("unknown", %{}, "a@b.com")
+      assert errors["_base"] =~ "select"
+    end
+
+    test "auto-sets from address when not configured" do
+      Settings.delete_setting("email_from_address")
+      Mailer.save_config("brevo", %{"api_key" => "xkeysib-abc123def456"}, "admin@shop.com")
+      assert Settings.get_setting("email_from_address") == "admin@shop.com"
+    end
+
+    test "clears email verified flag" do
+      Application.put_env(:berrypod, Mailer, adapter: Swoosh.Adapters.Postmark, api_key: "test")
+      Mailer.mark_email_verified()
+      assert Mailer.email_verified?()
+
+      Mailer.save_config("brevo", %{"api_key" => "xkeysib-abc123def456"}, "a@b.com")
+      refute Mailer.email_verified?()
+    end
+
+    test "clears settings from other providers" do
+      Settings.put_secret("email_mailjet_api_key", "old-key")
+      Settings.put_secret("email_mailjet_secret", "old-secret")
+
+      Mailer.save_config("brevo", %{"api_key" => "xkeysib-abc123def456"}, "a@b.com")
+
+      refute Settings.has_secret?("email_mailjet_api_key")
+      refute Settings.has_secret?("email_mailjet_secret")
+    end
+
+    test "trims whitespace from values" do
+      Mailer.save_config("brevo", %{"api_key" => "  xkeysib-abc123def456  "}, "a@b.com")
+      # Key should be saved trimmed (verified via successful save — no format error)
+      assert Settings.get_setting("email_adapter") == "brevo"
+    end
+  end
+
  describe "current_config/0" do
    test "returns {nil, %{}} when no adapter configured" do
      Application.put_env(:berrypod, Mailer, adapter: Swoosh.Adapters.Local)
--- a/test/berrypod_web/controllers/email_settings_controller_test.exs
+++ b/test/berrypod_web/controllers/email_settings_controller_test.exs
@@ -0,0 +1,76 @@
+defmodule BerrypodWeb.EmailSettingsControllerTest do
+  use BerrypodWeb.ConnCase, async: false
+
+  import Berrypod.AccountsFixtures
+
+  alias Berrypod.Settings
+
+  setup do
+    original = Application.get_env(:berrypod, Berrypod.Mailer)
+    Application.put_env(:berrypod, Berrypod.Mailer, adapter: Swoosh.Adapters.Test)
+
+    on_exit(fn ->
+      Application.put_env(:berrypod, Berrypod.Mailer, original)
+    end)
+
+    user = user_fixture()
+    conn = build_conn() |> log_in_user(user)
+    %{conn: conn, user: user}
+  end
+
+  describe "POST /admin/settings/email" do
+    test "saves adapter config and redirects", %{conn: conn} do
+      conn =
+        post(conn, ~p"/admin/settings/email", %{
+          email: %{
+            adapter: "brevo",
+            configured_adapter: "brevo",
+            api_key: "xkeysib-abc123def456"
+          }
+        })
+
+      assert redirected_to(conn) == ~p"/admin/settings/email"
+      assert Phoenix.Flash.get(conn.assigns.flash, :info) =~ "Settings saved"
+      assert Settings.get_setting("email_adapter") == "brevo"
+    end
+
+    test "redirects with adapter param when adapter changed", %{conn: conn} do
+      conn =
+        post(conn, ~p"/admin/settings/email", %{
+          email: %{adapter: "resend", configured_adapter: "brevo", api_key: ""}
+        })
+
+      assert redirected_to(conn) == ~p"/admin/settings/email?adapter=resend"
+    end
+
+    test "redirects with error on validation failure", %{conn: conn} do
+      conn =
+        post(conn, ~p"/admin/settings/email", %{
+          email: %{adapter: "brevo", configured_adapter: "brevo", api_key: ""}
+        })
+
+      assert redirected_to(conn) =~ ~p"/admin/settings/email"
+      assert Phoenix.Flash.get(conn.assigns.flash, :error) =~ "required"
+    end
+  end
+
+  describe "POST /admin/settings/email/test" do
+    test "sends test email and redirects", %{conn: conn} do
+      Settings.put_setting("email_adapter", "postmark")
+      Settings.put_secret("email_postmark_api_key", "pm_test_abc")
+
+      conn = post(conn, ~p"/admin/settings/email/test")
+
+      assert redirected_to(conn) == ~p"/admin/settings/email"
+      assert Phoenix.Flash.get(conn.assigns.flash, :info) =~ "Test email sent"
+    end
+  end
+
+  describe "unauthenticated" do
+    test "redirects to login", %{conn: _conn} do
+      conn = build_conn()
+      conn = post(conn, ~p"/admin/settings/email", %{email: %{adapter: "brevo"}})
+      assert redirected_to(conn) =~ ~p"/users/log-in"
+    end
+  end
+end
--- a/test/berrypod_web/live/admin/email_settings_test.exs
+++ b/test/berrypod_web/live/admin/email_settings_test.exs
@@ -37,11 +37,33 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do
      assert html =~ "Postal"
    end

+    test "shows all three provider groups", %{conn: conn} do
+      {:ok, _view, html} = live(conn, ~p"/admin/settings/email")
+
+      # All-email providers
+      assert html =~ "Popular providers"
+      assert html =~ "Brevo"
+      assert html =~ "SendGrid"
+      assert html =~ "Mailjet"
+      assert html =~ "MailerSend"
+
+      # Transactional providers
+      assert html =~ "Transactional only"
+      assert html =~ "Resend"
+      assert html =~ "Postmark"
+      assert html =~ "Mailgun"
+      assert html =~ "MailPace"
+
+      # Advanced in details
+      assert html =~ "Already have your own email server?"
+      assert html =~ "SMTP"
+      assert html =~ "Postal"
+    end
+
    test "shows setup guidance", %{conn: conn} do
      {:ok, _view, html} = live(conn, ~p"/admin/settings/email")

      assert html =~ "needs an email provider"
-      assert html =~ "Paste your API key"
      assert html =~ "300 emails/day free"
    end

@@ -51,7 +73,7 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do
      # Select SMTP via form change (radio inputs fire phx-change)
      html =
        view
-        |> form("form[phx-change=\"change_adapter\"]", %{email: %{adapter: "smtp"}})
+        |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "smtp"}})
        |> render_change()

      assert html =~ "Server host"
@@ -66,19 +88,31 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do
      # Select Brevo which needs just an api_key
      html =
        view
-        |> form("form[phx-change=\"change_adapter\"]", %{email: %{adapter: "brevo"}})
+        |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "brevo"}})
        |> render_change()

      assert html =~ "API key"
      assert html =~ "Brevo"
    end

+    test "selecting a transactional provider shows its config", %{conn: conn} do
+      {:ok, view, _html} = live(conn, ~p"/admin/settings/email")
+
+      html =
+        view
+        |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "resend"}})
+        |> render_change()
+
+      assert html =~ "API key"
+      assert html =~ "Resend"
+    end
+
    test "saving config persists settings", %{conn: conn} do
      {:ok, view, _html} = live(conn, ~p"/admin/settings/email")

      # Select Brevo via form change
      view
-      |> form("form[phx-change=\"change_adapter\"]", %{email: %{adapter: "brevo"}})
+      |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "brevo"}})
      |> render_change()

      # Submit with an API key
@@ -98,7 +132,7 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do

      # Select Brevo
      view
-      |> form("form[phx-change=\"change_adapter\"]", %{email: %{adapter: "brevo"}})
+      |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "brevo"}})
      |> render_change()

      # Submit without API key
@@ -110,6 +144,23 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do
      assert html =~ "API key is required"
    end

+    test "saving with invalid port shows error", %{conn: conn} do
+      {:ok, view, _html} = live(conn, ~p"/admin/settings/email")
+
+      view
+      |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "smtp"}})
+      |> render_change()
+
+      html =
+        view
+        |> form("form[phx-submit=\"save\"]", %{
+          email: %{adapter: "smtp", relay: "smtp.example.com", port: "abc"}
+        })
+        |> render_submit()
+
+      assert html =~ "must be a number"
+    end
+
    test "shows test email section when configured", %{conn: conn} do
      Settings.put_setting("email_adapter", "postmark")
      Settings.put_secret("email_postmark_api_key", "pm_test_abc")
@@ -136,7 +187,10 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do

      {:ok, view, _html} = live(conn, ~p"/admin/settings/email")

-      html = render_click(view, "send_test")
+      # send_test is now async — click triggers the event, then handle_info completes it
+      render_click(view, "send_test")
+      # Wait for the async handle_info to complete
+      html = render(view)

      assert html =~ "Email is working"
      assert html =~ "Send again"
@@ -153,7 +207,7 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do

      # Switch to Brevo and save
      view
-      |> form("form[phx-change=\"change_adapter\"]", %{email: %{adapter: "brevo"}})
+      |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "brevo"}})
      |> render_change()

      view
@@ -177,7 +231,7 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do
      {:ok, view, _html} = live(conn, ~p"/admin/settings/email")

      view
-      |> form("form[phx-change=\"change_adapter\"]", %{email: %{adapter: "brevo"}})
+      |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "brevo"}})
      |> render_change()

      view
@@ -188,6 +242,46 @@ defmodule BerrypodWeb.Admin.EmailSettingsTest do

      refute Mailer.email_verified?()
    end
+
+    test "phx-change is no-op when adapter hasn't changed", %{conn: conn} do
+      {:ok, view, _html} = live(conn, ~p"/admin/settings/email")
+
+      # Select Brevo
+      view
+      |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "brevo"}})
+      |> render_change()
+
+      # Trigger another change with the same adapter (simulates text field input)
+      html =
+        view
+        |> form("form[phx-change=\"form_change\"]", %{email: %{adapter: "brevo"}})
+        |> render_change()
+
+      # Should still show Brevo config
+      assert html =~ "Brevo"
+      assert html =~ "API key"
+    end
+
+    test "adapter query param preselects provider", %{conn: conn} do
+      {:ok, _view, html} = live(conn, ~p"/admin/settings/email?adapter=resend")
+
+      assert html =~ "Resend"
+      assert html =~ "API key"
+    end
+
+    test "from_checklist param shows checklist banner", %{conn: conn} do
+      {:ok, _view, html} = live(conn, ~p"/admin/settings/email?from=checklist")
+
+      assert html =~ "setting up email"
+      assert html =~ "Back to checklist"
+    end
+
+    test "has fieldset legend for accessibility", %{conn: conn} do
+      {:ok, _view, html} = live(conn, ~p"/admin/settings/email")
+
+      assert html =~ "Email provider"
+      assert html =~ "sr-only"
+    end
  end

  describe "unauthenticated" do