refactor: review remarks

lib/membership/join_notifier.ex

@@ -0,0 +1,13 @@
+defmodule Mv.Membership.JoinNotifier do
+  @moduledoc """
+  Behaviour for sending join-related emails (confirmation, already member, already pending).
+
+  The domain calls this module instead of MvWeb.Emails directly, so the domain layer
+  does not depend on the web layer. The default implementation is set in config
+  (`config :mv, :join_notifier, MvWeb.JoinNotifierImpl`). Tests can override with a mock.
+  """
+  @callback send_confirmation(email :: String.t(), token :: String.t(), opts :: keyword()) ::
+              {:ok, term()} | {:error, term()}
+  @callback send_already_member(email :: String.t()) :: {:ok, term()} | {:error, term()}
+  @callback send_already_pending(email :: String.t()) :: {:ok, term()} | {:error, term()}
+end

lib/membership/join_request/changes/regenerate_confirmation_token.ex

@@ -16,13 +16,16 @@ defmodule Mv.Membership.JoinRequest.Changes.RegenerateConfirmationToken do
     token = Ash.Changeset.get_argument(changeset, :confirmation_token)
 
     if is_binary(token) and token != "" do
-      hash = JoinRequest.hash_confirmation_token(token)
-      expires_at = DateTime.utc_now() |> DateTime.add(@confirmation_validity_hours, :hour)
+      now = DateTime.utc_now()
+      expires_at = DateTime.add(now, @confirmation_validity_hours, :hour)
 
       changeset
-      |> Ash.Changeset.force_change_attribute(:confirmation_token_hash, hash)
+      |> Ash.Changeset.force_change_attribute(
+        :confirmation_token_hash,
+        JoinRequest.hash_confirmation_token(token)
+      )
       |> Ash.Changeset.force_change_attribute(:confirmation_token_expires_at, expires_at)
-      |> Ash.Changeset.force_change_attribute(:confirmation_sent_at, DateTime.utc_now())
+      |> Ash.Changeset.force_change_attribute(:confirmation_sent_at, now)
     else
       changeset
     end

lib/membership/membership.ex

@@ -32,9 +32,7 @@ defmodule Mv.Membership do
   alias Mv.Helpers.SystemActor
   alias Mv.Membership.JoinRequest
   alias Mv.Membership.Member
-  alias MvWeb.Emails.JoinAlreadyMemberEmail
-  alias MvWeb.Emails.JoinAlreadyPendingEmail
-  alias MvWeb.Emails.JoinConfirmationEmail
+  alias Mv.Membership.SettingsCache
   require Logger
 
   admin do
@@ -118,10 +116,16 @@ defmodule Mv.Membership do
 
   """
   def get_settings do
-    # Try to get the first (and only) settings record
+    case Process.whereis(SettingsCache) do
+      nil -> get_settings_uncached()
+      _pid -> SettingsCache.get()
+    end
+  end
+
+  @doc false
+  def get_settings_uncached do
     case Ash.read_one(Mv.Membership.Setting, domain: __MODULE__) do
       {:ok, nil} ->
-        # No settings exist - create as fallback (should normally be created via seed script)
         default_club_name = System.get_env("ASSOCIATION_NAME") || "Club Name"
 
         Mv.Membership.Setting
@@ -162,9 +166,16 @@ defmodule Mv.Membership do
 
   """
   def update_settings(settings, attrs) do
-    settings
-    |> Ash.Changeset.for_update(:update, attrs)
-    |> Ash.update(domain: __MODULE__)
+    case settings
+         |> Ash.Changeset.for_update(:update, attrs)
+         |> Ash.update(domain: __MODULE__) do
+      {:ok, _updated} = result ->
+        SettingsCache.invalidate()
+        result
+
+      error ->
+        error
+    end
   end
 
   @doc """
@@ -228,11 +239,18 @@ defmodule Mv.Membership do
 
   """
   def update_member_field_visibility(settings, visibility_config) do
-    settings
-    |> Ash.Changeset.for_update(:update_member_field_visibility, %{
-      member_field_visibility: visibility_config
-    })
-    |> Ash.update(domain: __MODULE__)
+    case settings
+         |> Ash.Changeset.for_update(:update_member_field_visibility, %{
+           member_field_visibility: visibility_config
+         })
+         |> Ash.update(domain: __MODULE__) do
+      {:ok, _} = result ->
+        SettingsCache.invalidate()
+        result
+
+      error ->
+        error
+    end
   end
 
   @doc """
@@ -265,12 +283,19 @@ defmodule Mv.Membership do
         field: field,
         show_in_overview: show_in_overview
       ) do
-    settings
-    |> Ash.Changeset.new()
-    |> Ash.Changeset.set_argument(:field, field)
-    |> Ash.Changeset.set_argument(:show_in_overview, show_in_overview)
-    |> Ash.Changeset.for_update(:update_single_member_field_visibility, %{})
-    |> Ash.update(domain: __MODULE__)
+    case settings
+         |> Ash.Changeset.new()
+         |> Ash.Changeset.set_argument(:field, field)
+         |> Ash.Changeset.set_argument(:show_in_overview, show_in_overview)
+         |> Ash.Changeset.for_update(:update_single_member_field_visibility, %{})
+         |> Ash.update(domain: __MODULE__) do
+      {:ok, _} = result ->
+        SettingsCache.invalidate()
+        result
+
+      error ->
+        error
+    end
   end
 
   @doc """
@@ -304,13 +329,20 @@ defmodule Mv.Membership do
         show_in_overview: show_in_overview,
         required: required
       ) do
-    settings
-    |> Ash.Changeset.new()
-    |> Ash.Changeset.set_argument(:field, field)
-    |> Ash.Changeset.set_argument(:show_in_overview, show_in_overview)
-    |> Ash.Changeset.set_argument(:required, required)
-    |> Ash.Changeset.for_update(:update_single_member_field, %{})
-    |> Ash.update(domain: __MODULE__)
+    case settings
+         |> Ash.Changeset.new()
+         |> Ash.Changeset.set_argument(:field, field)
+         |> Ash.Changeset.set_argument(:show_in_overview, show_in_overview)
+         |> Ash.Changeset.set_argument(:required, required)
+         |> Ash.Changeset.for_update(:update_single_member_field, %{})
+         |> Ash.update(domain: __MODULE__) do
+      {:ok, _} = result ->
+        SettingsCache.invalidate()
+        result
+
+      error ->
+        error
+    end
   end
 
   @doc """
@@ -427,12 +459,12 @@ defmodule Mv.Membership do
 
   defp pending_join_request_with_email(_), do: nil
 
-  defp apply_anti_enumeration_delay do
-    Process.sleep(100 + :rand.uniform(200))
+  defp join_notifier do
+    Application.get_env(:mv, :join_notifier, MvWeb.JoinNotifierImpl)
   end
 
   defp send_already_member_and_return(email) do
-    case JoinAlreadyMemberEmail.send(email) do
+    case join_notifier().send_already_member(email) do
       {:ok, _} ->
         :ok
 
@@ -440,7 +472,7 @@ defmodule Mv.Membership do
         Logger.error("Join already-member email failed for #{email}: #{inspect(reason)}")
     end
 
-    apply_anti_enumeration_delay()
+    # Delay is applied by the caller (e.g. JoinLive) to avoid blocking the process.
     {:ok, :notified_already_member}
   end
 
@@ -461,7 +493,7 @@ defmodule Mv.Membership do
          })
          |> Ash.update(domain: __MODULE__, authorize?: false) do
       {:ok, _updated} ->
-        case JoinConfirmationEmail.send(email, new_token, resend: true) do
+        case join_notifier().send_confirmation(email, new_token, resend: true) do
           {:ok, _} ->
             :ok
 
@@ -469,7 +501,7 @@ defmodule Mv.Membership do
             Logger.error("Join resend confirmation email failed for #{email}: #{inspect(reason)}")
         end
 
-        apply_anti_enumeration_delay()
+        # Delay is applied by the caller (e.g. JoinLive) to avoid blocking the process.
         {:ok, :notified_already_pending}
 
       {:error, _} ->
@@ -479,7 +511,7 @@ defmodule Mv.Membership do
   end
 
   defp send_already_pending_and_return(email) do
-    case JoinAlreadyPendingEmail.send(email) do
+    case join_notifier().send_already_pending(email) do
       {:ok, _} ->
         :ok
 
@@ -487,7 +519,7 @@ defmodule Mv.Membership do
         Logger.error("Join already-pending email failed for #{email}: #{inspect(reason)}")
     end
 
-    apply_anti_enumeration_delay()
+    # Delay is applied by the caller (e.g. JoinLive) to avoid blocking the process.
     {:ok, :notified_already_pending}
   end
 
@@ -501,9 +533,9 @@ defmodule Mv.Membership do
            domain: __MODULE__
          ) do
       {:ok, request} ->
-        case JoinConfirmationEmail.send(request.email, token) do
+        case join_notifier().send_confirmation(request.email, token, []) do
           {:ok, _email} ->
-            apply_anti_enumeration_delay()
+            # Delay is applied by the caller (e.g. JoinLive) to avoid blocking the process.
             {:ok, request}
 
           {:error, reason} ->

lib/membership/settings_cache.ex

@@ -0,0 +1,85 @@
+defmodule Mv.Membership.SettingsCache do
+  @moduledoc """
+  Process-based cache for global settings to avoid repeated DB reads on hot paths
+  (e.g. RegistrationEnabled validation, Layouts.public_page, Plugs).
+
+  Uses a short TTL (default 60 seconds). Cache is invalidated on every settings
+  update so that changes take effect quickly. If no settings process exists
+  (e.g. in tests), get/1 falls back to direct read.
+  """
+  use GenServer
+
+  @default_ttl_seconds 60
+
+  def start_link(opts \\ []) do
+    GenServer.start_link(__MODULE__, opts, name: __MODULE__)
+  end
+
+  @doc """
+  Returns cached settings or fetches and caches them. Uses TTL; invalidate on update.
+  """
+  def get do
+    case Process.whereis(__MODULE__) do
+      nil ->
+        # No cache process (e.g. test) – read directly
+        do_fetch()
+
+      _pid ->
+        GenServer.call(__MODULE__, :get, 10_000)
+    end
+  end
+
+  @doc """
+  Invalidates the cache so the next get/0 will refetch from the database.
+  Call after update_settings and any other path that mutates settings.
+  """
+  def invalidate do
+    case Process.whereis(__MODULE__) do
+      nil -> :ok
+      _pid -> GenServer.cast(__MODULE__, :invalidate)
+    end
+  end
+
+  @impl true
+  def init(opts) do
+    ttl = Keyword.get(opts, :ttl_seconds, @default_ttl_seconds)
+    state = %{ttl_seconds: ttl, cached: nil, expires_at: nil}
+    {:ok, state}
+  end
+
+  @impl true
+  def handle_call(:get, _from, state) do
+    now = System.monotonic_time(:second)
+    expired? = state.expires_at == nil or state.expires_at <= now
+
+    {result, new_state} =
+      if expired? do
+        fetch_and_cache(now, state)
+      else
+        {{:ok, state.cached}, state}
+      end
+
+    {:reply, result, new_state}
+  end
+
+  defp fetch_and_cache(now, state) do
+    case do_fetch() do
+      {:ok, settings} = ok ->
+        expires = now + state.ttl_seconds
+        {ok, %{state | cached: settings, expires_at: expires}}
+
+      err ->
+        result = if state.cached, do: {:ok, state.cached}, else: err
+        {result, state}
+    end
+  end
+
+  @impl true
+  def handle_cast(:invalidate, state) do
+    {:noreply, %{state | cached: nil, expires_at: nil}}
+  end
+
+  defp do_fetch do
+    Mv.Membership.get_settings_uncached()
+  end
+end