refactor: apply review changes to joinrequest

docs/development-progress-log.md

@@ -797,8 +797,8 @@ end
 **Subtask 1 – JoinRequest resource and public policies (done):**
 - Resource: `Mv.Membership.JoinRequest` with attributes (status, email, first_name, last_name, form_data, schema_version, confirmation_token_hash, confirmation_token_expires_at, submitted_at, etc.), actions `submit` (create), `get_by_confirmation_token_hash` (read), `confirm` (update). Migration: `20260309141437_add_join_requests.exs`.
 - Policies: Public actions allowed with `actor: nil` via `Mv.Authorization.Checks.ActorIsNil` (submit, get_by_confirmation_token_hash, confirm); default read remains Forbidden for unauthenticated.
-- Domain: `Mv.Membership.submit_join_request/2`, `Mv.Membership.confirm_join_request/2` (token hashing and lookup in domain).
-- Test file: `test/membership/join_request_test.exs` – all tests pass; policy test (read with actor nil → Forbidden) unskipped. Expired-token test still skipped (fixture for expired token to be added in Subtask 2 or later).
+- Domain: `Mv.Membership.submit_join_request/2`, `Mv.Membership.confirm_join_request/2` (token hashing via `JoinRequest.hash_confirmation_token/1`, lookup, expiry check, idempotency for :submitted/:approved/:rejected).
+- Test file: `test/membership/join_request_test.exs` – all tests pass; policy test and expired-token test implemented.
 
 ### Test Data Management
 

lib/membership/join_request.ex

@@ -80,6 +80,7 @@ defmodule Mv.Membership.JoinRequest do
     validate present(:email), on: [:create]
   end
 
+  # Attributes are backend-internal for now; set public? true when exposing via AshJsonApi/AshGraphql
   attributes do
     uuid_primary_key :id
 
@@ -130,4 +131,17 @@ defmodule Mv.Membership.JoinRequest do
     create_timestamp :inserted_at
     update_timestamp :updated_at
   end
+
+  # Public helpers (used by SetConfirmationToken change and domain confirm_join_request)
+
+  @doc """
+  Returns the SHA256 hash of the confirmation token (lowercase hex).
+
+  Used when creating a join request (submit) and when confirming by token.
+  Only one implementation ensures algorithm changes stay in sync.
+  """
+  @spec hash_confirmation_token(String.t()) :: String.t()
+  def hash_confirmation_token(token) when is_binary(token) do
+    :crypto.hash(:sha256, token) |> Base.encode16(case: :lower)
+  end
 end

lib/membership/join_request/changes/confirm_request.ex

@@ -3,15 +3,23 @@ defmodule Mv.Membership.JoinRequest.Changes.ConfirmRequest do
   Sets the join request to submitted (confirmation link clicked).
 
   Used by the confirm action after the user clicks the confirmation link.
-  Token hash is kept so that a second click (idempotent) can still find the record
-  and return success without changing it.
+  Only applies when the current status is `:pending_confirmation`, so that
+  direct calls to the confirm action are idempotent and never overwrite
+  :submitted, :approved, or :rejected. Token hash is kept so a second click
+  can still find the record and return success without changing it.
   """
   use Ash.Resource.Change
 
   @spec change(Ash.Changeset.t(), keyword(), Ash.Resource.Change.context()) :: Ash.Changeset.t()
   def change(changeset, _opts, _context) do
-    changeset
-    |> Ash.Changeset.force_change_attribute(:status, :submitted)
-    |> Ash.Changeset.force_change_attribute(:submitted_at, DateTime.utc_now())
+    current_status = Ash.Changeset.get_data(changeset, :status)
+
+    if current_status == :pending_confirmation do
+      changeset
+      |> Ash.Changeset.force_change_attribute(:status, :submitted)
+      |> Ash.Changeset.force_change_attribute(:submitted_at, DateTime.utc_now())
+    else
+      changeset
+    end
   end
 end

lib/membership/join_request/changes/set_confirmation_token.ex

@@ -2,11 +2,15 @@ defmodule Mv.Membership.JoinRequest.Changes.SetConfirmationToken do
   @moduledoc """
   Hashes the confirmation token and sets expiry for the join request (submit flow).
 
+  Uses `JoinRequest.hash_confirmation_token/1` so hashing logic lives in one place.
+
   Reads the :confirmation_token argument, stores only its SHA256 hash and sets
   confirmation_token_expires_at (e.g. 24h). Raw token is never persisted.
   """
   use Ash.Resource.Change
 
+  alias Mv.Membership.JoinRequest
+
   @confirmation_validity_hours 24
 
   @spec change(Ash.Changeset.t(), keyword(), Ash.Resource.Change.context()) :: Ash.Changeset.t()
@@ -14,7 +18,7 @@ defmodule Mv.Membership.JoinRequest.Changes.SetConfirmationToken do
     token = Ash.Changeset.get_argument(changeset, :confirmation_token)
 
     if is_binary(token) and token != "" do
-      hash = token_hash(token)
+      hash = JoinRequest.hash_confirmation_token(token)
       expires_at = DateTime.utc_now() |> DateTime.add(@confirmation_validity_hours, :hour)
 
       changeset
@@ -25,8 +29,4 @@ defmodule Mv.Membership.JoinRequest.Changes.SetConfirmationToken do
       changeset
     end
   end
-
-  defp token_hash(token) do
-    :crypto.hash(:sha256, token) |> Base.encode16(case: :lower)
-  end
 end

lib/membership/membership.ex

@@ -353,19 +353,20 @@ defmodule Mv.Membership do
   @doc """
   Confirms a join request by token (public confirmation link).
 
-  Hashes the token, finds the JoinRequest by confirmation_token_hash, then updates
-  to status :submitted and invalidates the token. Idempotent: if already submitted,
-  returns the existing record without changing it.
+  Hashes the token, finds the JoinRequest by confirmation_token_hash, checks that
+  the token has not expired, then updates to status :submitted. Idempotent: if
+  already submitted, approved, or rejected, returns the existing record without changing it.
 
   ## Options
     - `:actor` - Must be nil for public confirm (policy allows only unauthenticated).
 
   ## Returns
-    - `{:ok, request}` - Updated or already-submitted JoinRequest
+    - `{:ok, request}` - Updated or already-processed JoinRequest
+    - `{:error, :token_expired}` - Token was found but confirmation_token_expires_at is in the past
     - `{:error, error}` - Token unknown/invalid or authorization error
   """
   def confirm_join_request(token, opts \\ []) when is_binary(token) do
-    hash = confirmation_token_hash(token)
+    hash = JoinRequest.hash_confirmation_token(token)
     actor = Keyword.get(opts, :actor)
 
     query =
@@ -378,20 +379,28 @@ defmodule Mv.Membership do
         {:error, NotFoundError.exception(resource: JoinRequest)}
 
       {:ok, request} ->
-        if request.status == :submitted do
-          {:ok, request}
-        else
-          request
-          |> Ash.Changeset.for_update(:confirm, %{}, domain: __MODULE__)
-          |> Ash.update(domain: __MODULE__, actor: actor)
-        end
+        do_confirm_request(request, actor)
 
       {:error, error} ->
         {:error, error}
     end
   end
 
-  defp confirmation_token_hash(token) do
-    :crypto.hash(:sha256, token) |> Base.encode16(case: :lower)
+  defp do_confirm_request(request, _actor)
+       when request.status in [:submitted, :approved, :rejected] do
+    {:ok, request}
   end
+
+  defp do_confirm_request(request, actor) do
+    if expired?(request.confirmation_token_expires_at) do
+      {:error, :token_expired}
+    else
+      request
+      |> Ash.Changeset.for_update(:confirm, %{}, domain: __MODULE__)
+      |> Ash.update(domain: __MODULE__, actor: actor)
+    end
+  end
+
+  defp expired?(nil), do: true
+  defp expired?(expires_at), do: DateTime.compare(expires_at, DateTime.utc_now()) == :lt
 end

priv/repo/migrations/20260309141437_add_join_requests.exs

@@ -9,6 +9,7 @@ defmodule Mv.Repo.Migrations.AddJoinRequests do
   use Ecto.Migration
 
   def up do
+    # uuid_generate_v7() is provided by initialize_extensions migration (custom function)
     create table(:join_requests, primary_key: false) do
       add :id, :uuid, null: false, default: fragment("uuid_generate_v7()"), primary_key: true
 

test/membership/join_request_test.exs

@@ -98,8 +98,20 @@ defmodule Mv.Membership.JoinRequestTest do
       assert {:error, _} = Membership.confirm_join_request("nonexistent-token", actor: nil)
     end
 
-    @tag :skip
-    test "returns error when token is expired (requires fixture for expired token)"
+    test "returns error when token is expired" do
+      token = "expired-token-#{System.unique_integer([:positive])}"
+      attrs = Map.put(@valid_submit_attrs, :confirmation_token, token)
+
+      {:ok, request} = Membership.submit_join_request(attrs, actor: nil)
+      past = DateTime.add(DateTime.utc_now(), -1, :hour)
+      id_binary = Ecto.UUID.dump!(request.id)
+
+      from(j in "join_requests", where: fragment("id = ?", ^id_binary))
+      |> Repo.update_all(set: [confirmation_token_expires_at: past])
+
+      assert {:error, :token_expired} =
+               Membership.confirm_join_request(token, actor: nil)
+    end
   end
 
   describe "policies (actor: nil)" do