From 08ac76a4b879ef9304d34670cfdbc2574e1b8138 Mon Sep 17 00:00:00 2001
From: Graham McIntire <graham@mcintire.me>
Date: Sun, 3 May 2026 13:01:36 -0500
Subject: [PATCH] refactor(rover): pattern-match-first event handlers + missing
 test branches
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

show.ex (rover-planning) and show.ex (rover-locations) auth + delete
flows go from nested case/case/case to a `with` ladder fed by a single
`authenticated/1` clause that returns `{:ok, user}` or
`{:error, :unauthenticated}`. The else block enumerates the small set
of failure tuples instead of rebuilding nested error returns.

Other idiomatic-Elixir tightening:
- `progress_summary/1` reduces with a `tally_path/2` multi-clause
  helper (status as a head pattern, not three Enum.count passes).
- `paths_by_rover_location/1` extracts `group_to_pair/1`,
  `station_position/1`, and `group_lat/1` so each transformation is a
  pattern match instead of an anonymous fn with `&& fallbacks`.
- `error_summary/1` is two clauses (empty vs populated errors)
  instead of a pipe-into-case.
- Drag-to-edit show.ex consolidates the working-coords assigns into
  `assign_working_coords/3`.

Bug fix uncovered while writing tests: `add_station` on a fresh
/rover-planning/new form was a visual no-op on the first click —
cast_assoc replaced the unseeded default Station struct with the new
params row instead of appending. Now the initial changeset is built
from `%{"stations" => %{"0" => %{"position" => "0"}}}` so add_station
appends from the get-go. Regression test added at
test/microwaveprop_web/live/rover_planning_live_test.exs.

New test branches:
- form: add_station appends on first click + remove_station drops
  the targeted row.
- rover-planning show: add_rover_site whitespace-input rejection,
  delete_rover_site permission denial.
- rover-locations show: save_edit rejects unauthenticated drivers.

Suite: 3228 tests, 0 failures. Credo strict: 0 issues.
---
 .../live/rover_locations_live/show.ex         | 113 ++++++---------
 .../live/rover_planning_live/form.ex          |   9 +-
 .../live/rover_planning_live/show.ex          | 133 ++++++++----------
 .../live/rover_locations_live/show_test.exs   |  13 ++
 .../live/rover_planning_live_test.exs         |  73 ++++++++++
 5 files changed, 198 insertions(+), 143 deletions(-)

diff --git a/lib/microwaveprop_web/live/rover_locations_live/show.ex b/lib/microwaveprop_web/live/rover_locations_live/show.ex
index cbf88005..d57b29d7 100644
--- a/lib/microwaveprop_web/live/rover_locations_live/show.ex
+++ b/lib/microwaveprop_web/live/rover_locations_live/show.ex
@@ -43,83 +43,63 @@ defmodule MicrowavepropWeb.RoverLocationsLive.Show do
   end
 
   def handle_event("cancel_edit", _params, socket) do
-    loc = socket.assigns.location
+    %Location{lat: lat, lon: lon} = socket.assigns.location
 
     {:noreply,
      socket
-     |> assign(
-       editing: false,
-       working_lat: loc.lat,
-       working_lon: loc.lon,
-       grid: Maidenhead.from_latlon(loc.lat, loc.lon, 10)
-     )
-     |> push_event("reset_marker", %{lat: loc.lat, lon: loc.lon})
+     |> assign_working_coords(lat, lon)
+     |> assign(editing: false)
+     |> push_event("reset_marker", %{lat: lat, lon: lon})
      |> push_event("set_marker_draggable", %{draggable: false})}
   end
 
   # Hook fires this on `dragend` with the marker's new coordinates.
   # We only update the working preview — persistence waits for save.
   def handle_event("location_dragged", %{"lat" => lat, "lon" => lon}, socket) when is_number(lat) and is_number(lon) do
-    {:noreply,
-     assign(socket,
-       working_lat: lat,
-       working_lon: lon,
-       grid: Maidenhead.from_latlon(lat, lon, 10)
-     )}
+    {:noreply, assign_working_coords(socket, lat, lon)}
   end
 
   def handle_event("save_edit", _params, socket) do
-    case current_user(socket.assigns[:current_scope]) do
-      %User{} = user ->
-        attrs = %{lat: socket.assigns.working_lat, lon: socket.assigns.working_lon}
+    %{working_lat: lat, working_lon: lon, location: %Location{id: id}} = socket.assigns
 
-        case Rover.update_location(user, socket.assigns.location.id, attrs) do
-          {:ok, updated} ->
-            updated = Repo.preload(updated, :user)
+    with {:ok, user} <- authenticated(socket),
+         {:ok, updated} <- Rover.update_location(user, id, %{lat: lat, lon: lon}) do
+      updated = Repo.preload(updated, :user)
 
-            {:noreply,
-             socket
-             |> assign(
-               location: updated,
-               editing: false,
-               working_lat: updated.lat,
-               working_lon: updated.lon,
-               grid: Maidenhead.from_latlon(updated.lat, updated.lon, 10)
-             )
-             |> put_flash(:info, "Location updated.")
-             |> push_event("set_marker_draggable", %{draggable: false})}
-
-          {:error, :not_found} ->
-            {:noreply, put_flash(socket, :error, "You can only edit your own locations.")}
-
-          {:error, %Ecto.Changeset{}} ->
-            {:noreply, put_flash(socket, :error, "Could not save those coordinates.")}
-        end
-
-      _ ->
-        {:noreply, put_flash(socket, :error, "Sign in required.")}
+      {:noreply,
+       socket
+       |> assign(location: updated, editing: false)
+       |> assign_working_coords(updated.lat, updated.lon)
+       |> put_flash(:info, "Location updated.")
+       |> push_event("set_marker_draggable", %{draggable: false})}
+    else
+      {:error, :unauthenticated} -> {:noreply, put_flash(socket, :error, "Sign in required.")}
+      {:error, :not_found} -> {:noreply, put_flash(socket, :error, "You can only edit your own locations.")}
+      {:error, %Ecto.Changeset{}} -> {:noreply, put_flash(socket, :error, "Could not save those coordinates.")}
     end
   end
 
   def handle_event("delete", _params, socket) do
-    case current_user(socket.assigns[:current_scope]) do
-      %User{} = user ->
-        case Rover.delete_location(user, socket.assigns.location.id) do
-          {:ok, _} ->
-            {:noreply,
-             socket
-             |> put_flash(:info, "Location removed.")
-             |> push_navigate(to: ~p"/rover-locations")}
-
-          {:error, :not_found} ->
-            {:noreply, put_flash(socket, :error, "You can only delete your own locations.")}
-        end
-
-      _ ->
-        {:noreply, put_flash(socket, :error, "Sign in required.")}
+    with {:ok, user} <- authenticated(socket),
+         {:ok, _} <- Rover.delete_location(user, socket.assigns.location.id) do
+      {:noreply,
+       socket
+       |> put_flash(:info, "Location removed.")
+       |> push_navigate(to: ~p"/rover-locations")}
+    else
+      {:error, :unauthenticated} -> {:noreply, put_flash(socket, :error, "Sign in required.")}
+      {:error, :not_found} -> {:noreply, put_flash(socket, :error, "You can only delete your own locations.")}
     end
   end
 
+  defp assign_working_coords(socket, lat, lon) do
+    assign(socket,
+      working_lat: lat,
+      working_lon: lon,
+      grid: Maidenhead.from_latlon(lat, lon, 10)
+    )
+  end
+
   defp load_location(id) do
     case Ecto.UUID.cast(id) do
       {:ok, uuid} -> Location |> Repo.get(uuid) |> Repo.preload(:user)
@@ -127,20 +107,15 @@ defmodule MicrowavepropWeb.RoverLocationsLive.Show do
     end
   end
 
-  defp current_user(scope) do
-    case scope do
-      %{user: %User{} = user} -> user
-      _ -> nil
-    end
-  end
+  defp authenticated(%Phoenix.LiveView.Socket{assigns: %{current_scope: %{user: %User{} = user}}}), do: {:ok, user}
 
-  defp can_modify?(scope, %Location{user_id: user_id}) do
-    case current_user(scope) do
-      %User{is_admin: true} -> true
-      %User{id: ^user_id} when not is_nil(user_id) -> true
-      _ -> false
-    end
-  end
+  defp authenticated(_), do: {:error, :unauthenticated}
+
+  defp can_modify?(%{user: %User{is_admin: true}}, _), do: true
+
+  defp can_modify?(%{user: %User{id: id}}, %Location{user_id: id}) when not is_nil(id), do: true
+
+  defp can_modify?(_, _), do: false
 
   defp status_label(:good), do: "Good"
   defp status_label(:bad), do: "Bad"
diff --git a/lib/microwaveprop_web/live/rover_planning_live/form.ex b/lib/microwaveprop_web/live/rover_planning_live/form.ex
index ffd28158..e1a85710 100644
--- a/lib/microwaveprop_web/live/rover_planning_live/form.ex
+++ b/lib/microwaveprop_web/live/rover_planning_live/form.ex
@@ -5,7 +5,6 @@ defmodule MicrowavepropWeb.RoverPlanningLive.Form do
   alias Microwaveprop.Accounts.User
   alias Microwaveprop.RoverPlanning
   alias Microwaveprop.RoverPlanning.Mission
-  alias Microwaveprop.RoverPlanning.Station
 
   @band_options [
     {"902 MHz", 902},
@@ -40,8 +39,12 @@ defmodule MicrowavepropWeb.RoverPlanningLive.Form do
   end
 
   defp apply_action(socket, :new, _params) do
-    mission = %Mission{stations: [%Station{position: 0}]}
-    cs = Mission.changeset(mission, %{})
+    # Seed the initial changeset PARAMS (not just the struct) with one
+    # station row so add_station / remove_station have something to work
+    # with. Without this seed, cast_assoc treats the first add_station
+    # click as a replacement of the unseeded default — visually a no-op.
+    mission = %Mission{}
+    cs = Mission.changeset(mission, %{"stations" => %{"0" => %{"position" => "0"}}})
 
     assign(socket,
       mission: mission,
diff --git a/lib/microwaveprop_web/live/rover_planning_live/show.ex b/lib/microwaveprop_web/live/rover_planning_live/show.ex
index 70f37808..53b8497c 100644
--- a/lib/microwaveprop_web/live/rover_planning_live/show.ex
+++ b/lib/microwaveprop_web/live/rover_planning_live/show.ex
@@ -51,22 +51,13 @@ defmodule MicrowavepropWeb.RoverPlanningLive.Show do
   end
 
   def handle_event("add_rover_site", %{"input" => input}, socket) do
-    with %User{} = user <- current_user(socket) || :anonymous,
+    with {:ok, user} <- authenticated(socket),
          {:ok, %{lat: lat, lon: lon}} <- LocationResolver.resolve(input),
          attrs = %{lat: lat, lon: lon, status: :good, notes: String.trim(input)},
          {:ok, _loc} <- Rover.create_location(user, attrs) do
-      :ok = RoverPlanning.replace_mission_paths(socket.assigns.mission)
-
-      {:noreply,
-       socket
-       |> assign(
-         rover_sites: RoverPlanning.candidate_rover_locations(socket.assigns.mission),
-         paths: RoverPlanning.list_paths(socket.assigns.mission),
-         rover_site_input: ""
-       )
-       |> put_flash(:info, "Rover site added — recomputing paths.")}
+      {:noreply, refresh_after_site_change(socket, "Rover site added — recomputing paths.")}
     else
-      :anonymous -> {:noreply, put_flash(socket, :error, "Sign in to add rover sites.")}
+      {:error, :unauthenticated} -> {:noreply, put_flash(socket, :error, "Sign in to add rover sites.")}
       :empty -> {:noreply, put_flash(socket, :error, "Enter a callsign, grid, or lat,lon.")}
       {:error, msg} when is_binary(msg) -> {:noreply, put_flash(socket, :error, msg)}
       {:error, %Ecto.Changeset{} = cs} -> {:noreply, put_flash(socket, :error, error_summary(cs))}
@@ -74,55 +65,47 @@ defmodule MicrowavepropWeb.RoverPlanningLive.Show do
   end
 
   def handle_event("delete_rover_site", %{"id" => id}, socket) do
-    case current_user(socket) do
-      %User{} = user ->
-        case Rover.delete_location(user, id) do
-          {:ok, _} ->
-            :ok = RoverPlanning.replace_mission_paths(socket.assigns.mission)
-
-            {:noreply,
-             socket
-             |> assign(
-               rover_sites: RoverPlanning.candidate_rover_locations(socket.assigns.mission),
-               paths: RoverPlanning.list_paths(socket.assigns.mission)
-             )
-             |> put_flash(:info, "Rover site removed — recomputing paths.")}
-
-          {:error, :not_found} ->
-            {:noreply, put_flash(socket, :error, "You can only delete rover sites you created.")}
-        end
-
-      _ ->
-        {:noreply, put_flash(socket, :error, "Sign in to remove rover sites.")}
+    with {:ok, user} <- authenticated(socket),
+         {:ok, _} <- Rover.delete_location(user, id) do
+      {:noreply, refresh_after_site_change(socket, "Rover site removed — recomputing paths.")}
+    else
+      {:error, :unauthenticated} -> {:noreply, put_flash(socket, :error, "Sign in to remove rover sites.")}
+      {:error, :not_found} -> {:noreply, put_flash(socket, :error, "You can only delete rover sites you created.")}
     end
   end
 
   def handle_event("delete", _params, socket) do
-    case current_user(socket) do
-      %User{} = user ->
-        case RoverPlanning.delete_mission(user, socket.assigns.mission.id) do
-          {:ok, _} ->
-            {:noreply,
-             socket
-             |> put_flash(:info, "Mission deleted.")
-             |> push_navigate(to: ~p"/rover-planning")}
-
-          {:error, :not_found} ->
-            {:noreply, put_flash(socket, :error, "You can only delete your own missions.")}
-        end
-
-      _ ->
-        {:noreply, put_flash(socket, :error, "Sign in required.")}
+    with {:ok, user} <- authenticated(socket),
+         {:ok, _} <- RoverPlanning.delete_mission(user, socket.assigns.mission.id) do
+      {:noreply,
+       socket
+       |> put_flash(:info, "Mission deleted.")
+       |> push_navigate(to: ~p"/rover-planning")}
+    else
+      {:error, :unauthenticated} -> {:noreply, put_flash(socket, :error, "Sign in required.")}
+      {:error, :not_found} -> {:noreply, put_flash(socket, :error, "You can only delete your own missions.")}
     end
   end
 
-  defp current_user(%Phoenix.LiveView.Socket{assigns: assigns}) do
-    case assigns[:current_scope] do
-      %{user: %User{} = user} -> user
-      _ -> nil
-    end
+  # Re-enqueues the mission's path matrix and reloads the show-page assigns
+  # that depend on the rover-site set.
+  defp refresh_after_site_change(socket, flash_message) do
+    mission = socket.assigns.mission
+    :ok = RoverPlanning.replace_mission_paths(mission)
+
+    socket
+    |> assign(
+      rover_sites: RoverPlanning.candidate_rover_locations(mission),
+      paths: RoverPlanning.list_paths(mission),
+      rover_site_input: ""
+    )
+    |> put_flash(:info, flash_message)
   end
 
+  defp authenticated(%Phoenix.LiveView.Socket{assigns: %{current_scope: %{user: %User{} = user}}}), do: {:ok, user}
+
+  defp authenticated(_), do: {:error, :unauthenticated}
+
   defp can_modify?(%{current_scope: %{user: %User{is_admin: true}}}, _), do: true
 
   defp can_modify?(%{current_scope: %{user: %User{id: id}}}, %Mission{user_id: id}) when not is_nil(id), do: true
@@ -135,14 +118,10 @@ defmodule MicrowavepropWeb.RoverPlanningLive.Show do
 
   defp can_delete_site?(_, _), do: false
 
-  defp error_summary(%Ecto.Changeset{} = cs) do
-    cs.errors
-    |> Enum.map_join("; ", fn {field, {msg, _}} -> "#{field} #{msg}" end)
-    |> case do
-      "" -> "Could not save rover site."
-      summary -> summary
-    end
-  end
+  defp error_summary(%Ecto.Changeset{errors: []}), do: "Could not save rover site."
+
+  defp error_summary(%Ecto.Changeset{errors: errors}),
+    do: Enum.map_join(errors, "; ", fn {field, {msg, _}} -> "#{field} #{msg}" end)
 
   defp band_label(mhz) when is_integer(mhz) and mhz >= 1000 do
     ghz = mhz / 1000
@@ -189,13 +168,17 @@ defmodule MicrowavepropWeb.RoverPlanningLive.Show do
   defp verdict_badge(_), do: ""
 
   defp progress_summary(paths) do
-    total = length(paths)
-    complete = Enum.count(paths, &(&1.status == :complete))
-    pending = Enum.count(paths, &(&1.status in [:pending, :computing]))
-    failed = Enum.count(paths, &(&1.status == :failed))
-    %{total: total, complete: complete, pending: pending, failed: failed}
+    Enum.reduce(paths, %{total: 0, complete: 0, pending: 0, failed: 0}, &tally_path/2)
   end
 
+  defp tally_path(%{status: :complete}, acc), do: %{acc | total: acc.total + 1, complete: acc.complete + 1}
+
+  defp tally_path(%{status: status}, acc) when status in [:pending, :computing],
+    do: %{acc | total: acc.total + 1, pending: acc.pending + 1}
+
+  defp tally_path(%{status: :failed}, acc), do: %{acc | total: acc.total + 1, failed: acc.failed + 1}
+  defp tally_path(_, acc), do: %{acc | total: acc.total + 1}
+
   # Bucket paths by their rover-location and sort each bucket so stations
   # appear in the order the user defined on the mission. The outer order
   # is by rover-location lat (north → south) so the same rover location
@@ -203,14 +186,22 @@ defmodule MicrowavepropWeb.RoverPlanningLive.Show do
   defp paths_by_rover_location(paths) do
     paths
     |> Enum.group_by(& &1.rover_location_id)
-    |> Enum.map(fn {_id, group} ->
-      location = (List.first(group) || %{}).rover_location
-      sorted = Enum.sort_by(group, &(&1.station && &1.station.position))
-      {location, sorted}
-    end)
-    |> Enum.sort_by(fn {loc, _} -> -((loc && loc.lat) || 0) end)
+    |> Enum.map(&group_to_pair/1)
+    |> Enum.sort_by(&group_lat/1, :desc)
   end
 
+  defp group_to_pair({_id, [%{rover_location: location} | _] = group}) do
+    {location, Enum.sort_by(group, &station_position/1)}
+  end
+
+  defp group_to_pair({_id, []}), do: {nil, []}
+
+  defp station_position(%{station: %{position: pos}}) when is_integer(pos), do: pos
+  defp station_position(_), do: 0
+
+  defp group_lat({%{lat: lat}, _}) when is_number(lat), do: lat
+  defp group_lat(_), do: 0
+
   # JSONB round-trips bare zeros as integers (e.g. `0` not `0.0`), and
   # `Float.round/2` only accepts floats. Coerce with `* 1.0` everywhere
   # we round a value that came out of a `:map` column.
diff --git a/test/microwaveprop_web/live/rover_locations_live/show_test.exs b/test/microwaveprop_web/live/rover_locations_live/show_test.exs
index 6c1cf620..60de4b20 100644
--- a/test/microwaveprop_web/live/rover_locations_live/show_test.exs
+++ b/test/microwaveprop_web/live/rover_locations_live/show_test.exs
@@ -139,6 +139,19 @@ defmodule MicrowavepropWeb.RoverLocationsLive.ShowTest do
       assert reloaded.lon == -97.5
     end
 
+    test "save_edit fails closed when the user is signed out mid-session", %{conn: conn} do
+      user = Microwaveprop.AccountsFixtures.user_fixture()
+      {:ok, loc} = Rover.create_location(user, %{lat: 32.5, lon: -97.5, status: :good})
+
+      # Anonymous conn — driving the event directly through render_hook
+      # exercises the unauthenticated branch of save_edit.
+      {:ok, lv, _html} = live(conn, ~p"/rover-locations/#{loc.id}")
+
+      html = render_hook(lv, "save_edit", %{})
+      assert html =~ "Sign in required"
+      assert Repo.reload(loc).lat == 32.5
+    end
+
     test "non-owner does not see the Edit button", %{conn: conn} do
       owner = Microwaveprop.AccountsFixtures.user_fixture()
       visitor = Microwaveprop.AccountsFixtures.user_fixture()
diff --git a/test/microwaveprop_web/live/rover_planning_live_test.exs b/test/microwaveprop_web/live/rover_planning_live_test.exs
index db5ee8c5..686d9ac5 100644
--- a/test/microwaveprop_web/live/rover_planning_live_test.exs
+++ b/test/microwaveprop_web/live/rover_planning_live_test.exs
@@ -244,6 +244,46 @@ defmodule MicrowavepropWeb.RoverPlanningLiveTest do
       assert Repo.aggregate(Location, :count) == 0
     end
 
+    test "add_rover_site rejects whitespace-only input with a flash error",
+         %{conn: conn} do
+      user = AccountsFixtures.user_fixture()
+      mission = create_mission(user, "Empty-input mission")
+      conn = log_in_user(conn, user)
+
+      {:ok, lv, _} = live(conn, ~p"/rover-planning/#{mission.id}")
+
+      html =
+        lv
+        |> form("form[phx-submit='add_rover_site']", %{"input" => "   "})
+        |> render_submit()
+
+      # `   ` trims to "" → LocationResolver returns :empty, which the
+      # handler maps to this flash. No new location was created.
+      assert html =~ "Enter a callsign, grid, or lat,lon"
+      assert Repo.aggregate(Location, :count) == 1
+    end
+
+    test "delete_rover_site shows error when the user does not own the site",
+         %{conn: conn} do
+      owner = AccountsFixtures.user_fixture()
+      mission = create_mission(owner, "Foreign-site mission")
+
+      [site] = Repo.all(Location)
+
+      visitor = AccountsFixtures.user_fixture()
+      conn = log_in_user(conn, visitor)
+
+      {:ok, lv, _html} = live(conn, ~p"/rover-planning/#{mission.id}")
+
+      # Visitor doesn't own the site, so the trash button is hidden;
+      # but we exercise the server-side guard directly via render_hook
+      # to confirm the not_found branch.
+      html = render_hook(lv, "delete_rover_site", %{"id" => site.id})
+
+      assert html =~ "You can only delete rover sites you created"
+      assert Repo.aggregate(Location, :count) == 1
+    end
+
     test "anonymous visitor sees a sign-in prompt instead of the add form", %{conn: conn} do
       user = AccountsFixtures.user_fixture()
       mission = create_mission(user, "Anon-view mission")
@@ -308,6 +348,39 @@ defmodule MicrowavepropWeb.RoverPlanningLiveTest do
                ~s(name="mission[only_known_good]" value="true" checked)
     end
 
+    test "remove_station drops the targeted station from the form", %{conn: conn} do
+      user = AccountsFixtures.user_fixture()
+      conn = log_in_user(conn, user)
+      {:ok, lv, _html} = live(conn, ~p"/rover-planning/new")
+
+      # The new form seeds one station; click Add to get a second.
+      _ = lv |> element("button[phx-click='add_station']") |> render_click()
+      assert render(lv) =~ ~s(phx-value-index="1")
+
+      html =
+        lv
+        |> element("button[phx-click='remove_station'][phx-value-index='1']")
+        |> render_click()
+
+      refute html =~ ~s(phx-value-index="1")
+      assert html =~ ~s(phx-value-index="0")
+    end
+
+    test "add_station appends a new row on the very first click", %{conn: conn} do
+      # Regression: before seeding initial params with one station, the
+      # first add_station click was a no-op (cast_assoc replaced the
+      # default Station struct with the new params row instead of
+      # appending). Both indices must render after a single click.
+      user = AccountsFixtures.user_fixture()
+      conn = log_in_user(conn, user)
+      {:ok, lv, _html} = live(conn, ~p"/rover-planning/new")
+
+      html = lv |> element("button[phx-click='add_station']") |> render_click()
+
+      assert html =~ ~s(phx-value-index="0")
+      assert html =~ ~s(phx-value-index="1")
+    end
+
     test "logged-in user can create a mission with a callsign-input station",
          %{conn: conn} do
       user = AccountsFixtures.user_fixture()