refactor: fix review blockers

lib/mv_web/live/join_request_live/show.ex

@@ -21,6 +21,7 @@ defmodule MvWeb.JoinRequestLive.Show do
 
   alias Mv.Constants
   alias Mv.Membership
+  alias Mv.Membership.CustomFieldLookup
   alias MvWeb.Helpers.DateFormatter
   alias MvWeb.JoinRequestLive.Helpers, as: JoinRequestHelpers
   alias MvWeb.Translations.MemberFields, as: MemberFieldsTranslations
@@ -31,7 +32,7 @@ defmodule MvWeb.JoinRequestLive.Show do
       {:ok,
        socket
        |> assign(:join_request, nil)
-       |> assign(:custom_field_name_by_id, %{})
+       |> assign(:custom_field_by_id, %{})
        |> assign(:join_form_field_ids, [])
        |> Layouts.assign_page_title(gettext("Join request"))}
     else
@@ -54,13 +55,16 @@ defmodule MvWeb.JoinRequestLive.Show do
         {:ok, request} ->
           field_ids = Membership.get_join_form_allowlist() |> Enum.map(& &1.id)
 
-          custom_field_name_by_id =
-            custom_field_name_map(field_ids ++ Map.keys(request.form_data || %{}), actor)
+          custom_field_by_id =
+            CustomFieldLookup.fetch_map_by_ids(field_ids ++ Map.keys(request.form_data || %{}),
+              actor: actor,
+              select: [:id, :name, :value_type]
+            )
 
           {:noreply,
            socket
            |> assign(:join_request, request)
-           |> assign(:custom_field_name_by_id, custom_field_name_by_id)
+           |> assign(:custom_field_by_id, custom_field_by_id)
            |> assign(:join_form_field_ids, field_ids)
            |> Layouts.assign_page_title(gettext("Join request – %{email}", email: request.email))}
 
@@ -136,59 +140,58 @@ defmodule MvWeb.JoinRequestLive.Show do
           <%!-- Single block: all applicant-provided data in join form order --%>
           <div>
             <h2 class="text-lg font-semibold mb-2">{gettext("Applicant data")}</h2>
-            <div class="border border-base-300 rounded-lg p-4 bg-base-100 space-y-2">
-              <%= for {label, value} <-
+            <div class="border border-base-300 rounded-lg p-4 bg-base-100">
+              <dl class="grid gap-1 md:grid-cols-[14rem_minmax(0,1fr)] md:gap-2">
+                <%= for {label, value} <-
                     applicant_data_rows(
                       @join_request,
                       @join_form_field_ids || [],
-                      @custom_field_name_by_id || %{}
+                      @custom_field_by_id || %{}
                     ) do %>
-                <.field_row label={label} value={value} empty_text={gettext("Not specified")} />
-              <% end %>
+                  <.field_row label={label} value={value} empty_text={gettext("Not specified")} />
+                <% end %>
+              </dl>
             </div>
           </div>
 
           <%!-- Status and review (submitted_at, status; if decided: approved/rejected at, reviewed by) --%>
           <div>
             <h2 class="text-lg font-semibold mb-2">{gettext("Status and review")}</h2>
-            <div class="border border-base-300 rounded-lg p-4 bg-base-100 space-y-2">
-              <.field_row
-                label={gettext("Submitted at")}
-                value={DateFormatter.format_datetime(@join_request.submitted_at, @browser_timezone)}
-              />
+            <div class="border border-base-300 rounded-lg p-4 bg-base-100">
               <dl class="grid gap-1 md:grid-cols-[14rem_minmax(0,1fr)] md:gap-2">
-                <dt class="m-0 text-base-content/60 whitespace-normal break-words">
-                  {gettext("Status")}:
-                </dt>
-                <dd class="m-0 min-w-0">
+                <.field_row
+                  label={gettext("Submitted at")}
+                  value={DateFormatter.format_datetime(@join_request.submitted_at, @browser_timezone)}
+                />
+                <.field_row label={gettext("Status")}>
                   <.badge variant={JoinRequestHelpers.status_badge_variant(@join_request.status)}>
                     {JoinRequestHelpers.format_status(@join_request.status)}
                   </.badge>
-                </dd>
+                </.field_row>
+                <%= if @join_request.status in [:approved, :rejected] do %>
+                  <%= if @join_request.approved_at do %>
+                    <.field_row
+                      label={gettext("Approved at")}
+                      value={
+                        DateFormatter.format_datetime(@join_request.approved_at, @browser_timezone)
+                      }
+                    />
+                  <% end %>
+                  <%= if @join_request.rejected_at do %>
+                    <.field_row
+                      label={gettext("Rejected at")}
+                      value={
+                        DateFormatter.format_datetime(@join_request.rejected_at, @browser_timezone)
+                      }
+                    />
+                  <% end %>
+                  <.field_row
+                    label={gettext("Review by")}
+                    value={JoinRequestHelpers.reviewer_display(@join_request)}
+                    empty_text="-"
+                  />
+                <% end %>
               </dl>
-              <%= if @join_request.status in [:approved, :rejected] do %>
-                <%= if @join_request.approved_at do %>
-                  <.field_row
-                    label={gettext("Approved at")}
-                    value={
-                      DateFormatter.format_datetime(@join_request.approved_at, @browser_timezone)
-                    }
-                  />
-                <% end %>
-                <%= if @join_request.rejected_at do %>
-                  <.field_row
-                    label={gettext("Rejected at")}
-                    value={
-                      DateFormatter.format_datetime(@join_request.rejected_at, @browser_timezone)
-                    }
-                  />
-                <% end %>
-                <.field_row
-                  label={gettext("Review by")}
-                  value={JoinRequestHelpers.reviewer_display(@join_request)}
-                  empty_text="-"
-                />
-              <% end %>
             </div>
           </div>
 
@@ -221,28 +224,30 @@ defmodule MvWeb.JoinRequestLive.Show do
   attr :label, :string, required: true
   attr :value, :any, default: nil
   attr :empty_text, :string, default: nil
+  slot :inner_block
 
   defp field_row(assigns) do
     ~H"""
-    <dl class="grid gap-1 md:grid-cols-[14rem_minmax(0,1fr)] md:gap-2">
-      <dt class="m-0 text-base-content/60 whitespace-normal break-words">{@label}:</dt>
-      <dd class="m-0 min-w-0">
-        <%= if @value && @value != "" do %>
+    <dt class="m-0 text-base-content/60 whitespace-normal break-words">{@label}:</dt>
+    <dd class="m-0 min-w-0">
+      <%= cond do %>
+        <% @inner_block != [] -> %>
+          {render_slot(@inner_block)}
+        <% @value && @value != "" -> %>
           {@value}
-        <% else %>
+        <% true -> %>
           <span class="text-base-content/40 italic">
             {@empty_text || gettext("Not specified")}
           </span>
-        <% end %>
-      </dd>
-    </dl>
+      <% end %>
+    </dd>
     """
   end
 
   # Builds a single list of {label, display_value} for all applicant-provided data in join form
   # order. Typed fields (email, first_name, last_name) and form_data are merged; legacy
   # form_data keys (not in current join form config) are appended at the end.
-  defp applicant_data_rows(join_request, ordered_field_ids, custom_field_name_by_id) do
+  defp applicant_data_rows(join_request, ordered_field_ids, custom_field_by_id) do
     member_field_strings = Constants.member_fields() |> Enum.map(&Atom.to_string/1)
     form_data = join_request.form_data || %{}
 
@@ -256,8 +261,9 @@ defmodule MvWeb.JoinRequestLive.Show do
       ordered_field_ids
       |> Enum.map(fn key ->
         value = Map.get(typed, key) || Map.get(form_data, key)
-        label = field_key_to_label(key, member_field_strings, custom_field_name_by_id)
-        {label, format_applicant_value(value)}
+        label = field_key_to_label(key, member_field_strings, custom_field_by_id)
+        value_type = field_key_to_value_type(key, member_field_strings, custom_field_by_id)
+        {label, format_applicant_value(value, value_type)}
       end)
 
     legacy_keys =
@@ -270,31 +276,32 @@ defmodule MvWeb.JoinRequestLive.Show do
 
     legacy_entries =
       Enum.map(legacy_keys, fn key ->
-        label = field_key_to_label(key, member_field_strings, custom_field_name_by_id)
-        {label, format_applicant_value(form_data[key])}
+        label = field_key_to_label(key, member_field_strings, custom_field_by_id)
+        value_type = field_key_to_value_type(key, member_field_strings, custom_field_by_id)
+        {label, format_applicant_value(form_data[key], value_type)}
       end)
 
     in_order ++ legacy_entries
   end
 
-  defp format_applicant_value(nil), do: nil
-  defp format_applicant_value(""), do: nil
-  defp format_applicant_value(%Date{} = date), do: DateFormatter.format_date(date)
+  defp format_applicant_value(nil, _type), do: nil
+  defp format_applicant_value("", _type), do: nil
+  defp format_applicant_value(%Date{} = date, _type), do: DateFormatter.format_date(date)
 
-  defp format_applicant_value(value) when is_map(value),
-    do: format_applicant_value_from_map(value)
+  defp format_applicant_value(value, type) when is_map(value),
+    do: format_applicant_value_from_map(value, type)
 
-  defp format_applicant_value(value) when is_boolean(value),
+  defp format_applicant_value(value, _type) when is_boolean(value),
     do: if(value, do: gettext("Yes"), else: gettext("No"))
 
-  defp format_applicant_value(value) when is_binary(value),
-    do: format_binary_applicant_value(value)
+  defp format_applicant_value(value, type) when is_binary(value),
+    do: format_binary_applicant_value(value, type)
 
-  defp format_applicant_value(value) when is_number(value), do: to_string(value)
+  defp format_applicant_value(value, _type) when is_number(value), do: to_string(value)
 
-  defp format_applicant_value(value), do: to_string(value)
+  defp format_applicant_value(value, _type), do: to_string(value)
 
-  defp format_binary_applicant_value(value) do
+  defp format_binary_applicant_value(value, type) do
     trimmed_value = String.trim(value)
 
     cond do
@@ -307,8 +314,11 @@ defmodule MvWeb.JoinRequestLive.Show do
       String.downcase(trimmed_value) in ["off", "false", "0"] ->
         gettext("No")
 
-      true ->
+      type in [:date, Ash.Type.Date] ->
         format_iso_date_string(trimmed_value)
+
+      true ->
+        trimmed_value
     end
   end
 
@@ -319,12 +329,13 @@ defmodule MvWeb.JoinRequestLive.Show do
     end
   end
 
-  defp format_applicant_value_from_map(value) do
+  defp format_applicant_value_from_map(value, fallback_type) do
     raw = Map.get(value, "_union_value") || Map.get(value, "value")
     type = Map.get(value, "_union_type") || Map.get(value, "type")
+    effective_type = type || fallback_type
 
-    if raw && type in ["date", :date] do
-      format_applicant_value(raw)
+    if raw && effective_type in ["date", :date, Ash.Type.Date] do
+      format_applicant_value(raw, :date)
     else
       format_applicant_value_simple(raw, value)
     end
@@ -338,44 +349,39 @@ defmodule MvWeb.JoinRequestLive.Show do
   defp format_applicant_value_simple(raw, _value) when is_integer(raw), do: to_string(raw)
   defp format_applicant_value_simple(_raw, value), do: to_string(value)
 
-  defp field_key_to_label(key, member_field_strings, custom_field_name_by_id)
+  defp field_key_to_label(key, member_field_strings, custom_field_by_id)
        when is_binary(key) do
     if key in member_field_strings do
       MemberFieldsTranslations.label(String.to_existing_atom(key))
     else
-      Map.get(custom_field_name_by_id, key, key)
+      case Map.get(custom_field_by_id, key) do
+        %{name: name} -> name
+        _ -> key
+      end
     end
   end
 
   defp field_key_to_label(key, _, _), do: to_string(key)
 
-  defp custom_field_name_map(field_keys, actor) do
-    member_field_strings = Constants.member_fields() |> Enum.map(&Atom.to_string/1)
+  defp field_key_to_value_type("email", _member_field_strings, _custom_field_by_id), do: :string
 
-    custom_field_ids =
-      field_keys
-      |> Enum.uniq()
-      |> Enum.reject(&(&1 in member_field_strings))
+  defp field_key_to_value_type("first_name", _member_field_strings, _custom_field_by_id),
+    do: :string
 
-    case custom_field_ids do
-      [] ->
-        %{}
+  defp field_key_to_value_type("last_name", _member_field_strings, _custom_field_by_id),
+    do: :string
 
-      ids ->
-        Mv.Membership.CustomField
-        |> Ash.Query.select([:id, :name])
-        |> Ash.read(actor: actor, domain: Mv.Membership)
-        |> case do
-          {:ok, fields} ->
-            allowed_ids = MapSet.new(ids)
-
-            fields
-            |> Enum.filter(&MapSet.member?(allowed_ids, &1.id))
-            |> Map.new(&{&1.id, &1.name})
-
-          {:error, _} ->
-            %{}
-        end
+  defp field_key_to_value_type(key, member_field_strings, custom_field_by_id)
+       when is_binary(key) do
+    if key in member_field_strings do
+      :string
+    else
+      case Map.get(custom_field_by_id, key) do
+        %{value_type: value_type} -> value_type
+        _ -> nil
+      end
     end
   end
+
+  defp field_key_to_value_type(_key, _member_field_strings, _custom_field_by_id), do: nil
 end