From a0214c09ae1c78da92a2005433af148d93e30a0b Mon Sep 17 00:00:00 2001
From: Cristen Jones <cjones3@mbta.com>
Date: Wed, 5 Feb 2025 17:38:20 -0500
Subject: [PATCH] fix(TripPlan.InputForm): improve location validation + add
 visual error state (#2365)

---
 assets/css/_autocomplete-theme.scss           | 23 +++++++++-------
 assets/ts/ui/autocomplete/config.ts           |  9 +++++++
 lib/dotcom/trip_plan/anti_corruption_layer.ex | 12 ++++-----
 lib/dotcom/trip_plan/input_form.ex            | 27 ++++++++++++++-----
 .../components/trip_planner/input_form.ex     | 18 ++++++++++---
 test/dotcom/trip_plan/input_form_test.exs     | 17 ++++++++++++
 test/dotcom_web/live/trip_planner_test.exs    | 17 ++++++++++++
 7 files changed, 97 insertions(+), 26 deletions(-)

diff --git a/assets/css/_autocomplete-theme.scss b/assets/css/_autocomplete-theme.scss
index d972c64eff..1595339ef8 100644
--- a/assets/css/_autocomplete-theme.scss
+++ b/assets/css/_autocomplete-theme.scss
@@ -1,12 +1,20 @@
 @import '@algolia/autocomplete-theme-classic';
 
+:root {
+  --aa-accent-color: 22, 92, 150; // $brand-primary fallback;
+}
+
 // styles that will be used for every instance
 [phx-hook='AlgoliaAutocomplete'], .aa-Detached {
+  .text-danger & {
+    --aa-accent-color: 179, 0, 15; // matches $error-text
+  }
+
   .aa-Autocomplete {
-    --aa-icon-color-rgb: 22, 92, 150; // $brand-primary;
-    --aa-primary-color-rgb: 22, 92, 150; // $brand-primary;
-    --aa-input-border-color-rgb: 22, 92, 150;
-    --aa-overlay-color-rgb: 22, 92, 150; // $brand-primary;
+    --aa-icon-color-rgb: var(--aa-accent-color);
+    --aa-primary-color-rgb: var(--aa-accent-color);
+    --aa-input-border-color-rgb: var(--aa-accent-color);
+    --aa-overlay-color-rgb: var(--aa-accent-color);
   }
 
   .aa-Label {
@@ -33,11 +41,6 @@
       border-color: $brand-primary-light;
       box-shadow: unset;
     }
-
-    div:has(.text-danger) > & {
-      --aa-input-border-color-rgb: 179, 0, 15;
-      --aa-icon-color-rgb: 179, 0, 15;
-    }
   }
 
   .aa-LoadingIndicator,
@@ -218,7 +221,7 @@
 
     .aa-Label {
       align-content: center;
-      color: $brand-primary;
+      color: rgb(var(--aa-icon-color-rgb));
       font-size: 1.25rem;
       font-weight: bold;
       text-align: center;
diff --git a/assets/ts/ui/autocomplete/config.ts b/assets/ts/ui/autocomplete/config.ts
index c1cde83569..b5e71cff22 100644
--- a/assets/ts/ui/autocomplete/config.ts
+++ b/assets/ts/ui/autocomplete/config.ts
@@ -210,6 +210,15 @@ const TRIP_PLANNER = ({
     onReset: (): void => {
       pushToLiveView({});
     },
+    onStateChange({ prevState, state }) {
+      const isCleared = state.query === "";
+      const changedToClosedState = prevState.isOpen && !state.isOpen;
+      // Send changed input text to LiveView when leaving this input
+      if (changedToClosedState) {
+        const newInputData = isCleared ? {} : { name: state.query };
+        pushToLiveView(newInputData);
+      }
+    },
     getSources({ query }) {
       if (!query)
         return debounced([
diff --git a/lib/dotcom/trip_plan/anti_corruption_layer.ex b/lib/dotcom/trip_plan/anti_corruption_layer.ex
index 656ec75c6d..74ddea9333 100644
--- a/lib/dotcom/trip_plan/anti_corruption_layer.ex
+++ b/lib/dotcom/trip_plan/anti_corruption_layer.ex
@@ -76,16 +76,16 @@ defmodule Dotcom.TripPlan.AntiCorruptionLayer do
   defp copy_params(params) do
     %{
       "from" => %{
-        "latitude" => Map.get(params, "from_latitude"),
-        "longitude" => Map.get(params, "from_longitude"),
-        "name" => Map.get(params, "from"),
+        "latitude" => Map.get(params, "from_latitude", ""),
+        "longitude" => Map.get(params, "from_longitude", ""),
+        "name" => Map.get(params, "from", ""),
         "stop_id" => Map.get(params, "from_stop_id", "")
       },
       "modes" => Map.get(params, "modes") |> convert_modes(),
       "to" => %{
-        "latitude" => Map.get(params, "to_latitude"),
-        "longitude" => Map.get(params, "to_longitude"),
-        "name" => Map.get(params, "to"),
+        "latitude" => Map.get(params, "to_latitude", ""),
+        "longitude" => Map.get(params, "to_longitude", ""),
+        "name" => Map.get(params, "to", ""),
         "stop_id" => Map.get(params, "to_stop_id", "")
       },
       "wheelchair" => Map.get(params, "wheelchair") || "false"
diff --git a/lib/dotcom/trip_plan/input_form.ex b/lib/dotcom/trip_plan/input_form.ex
index f6c8bf5302..670d0e5c2f 100644
--- a/lib/dotcom/trip_plan/input_form.ex
+++ b/lib/dotcom/trip_plan/input_form.ex
@@ -12,9 +12,11 @@ defmodule Dotcom.TripPlan.InputForm do
   alias OpenTripPlannerClient.PlanParams
 
   @error_messages %{
+    from: "Please select an origin location.",
     from_to_same: "Please select a destination at a different location from the origin.",
     modes: "Please select at least one mode of transit.",
-    datetime: "Please specify a date and time in the future or select 'Now'."
+    datetime: "Please specify a date and time in the future or select 'Now'.",
+    to: "Please select a destination location."
   }
 
   @primary_key false
@@ -63,21 +65,34 @@ defmodule Dotcom.TripPlan.InputForm do
     |> cast(params, [:datetime, :datetime_type, :wheelchair])
     |> cast_embed(:from, required: true)
     |> cast_embed(:to, required: true)
+    |> validate_change(:from, &validate_location_change/2)
+    |> validate_change(:to, &validate_location_change/2)
     |> cast_embed(:modes, required: true)
-    |> update_change(:from, &update_location_change/1)
-    |> update_change(:to, &update_location_change/1)
     |> validate_required(:modes, message: error_message(:modes))
     |> validate_required([:datetime_type, :wheelchair])
     |> validate_same_locations()
     |> validate_chosen_datetime()
   end
 
-  # make the parent field blank if the location isn't valid
-  defp update_location_change(%Ecto.Changeset{valid?: false, errors: [_ | _]}), do: nil
-  defp update_location_change(changeset), do: changeset
+  # These are embedded fields, so we need to check the underlying changeset for
+  # validity. If the underlying data has changed (there are four fields, but
+  # checking :name itself suffices), and the changeset is invalid, add an error.
+  defp validate_location_change(
+         field,
+         %Ecto.Changeset{valid?: false, errors: [_ | _]} = changeset
+       ) do
+    if changed?(changeset, :name) do
+      Keyword.new([{field, error_message(field)}])
+    else
+      []
+    end
+  end
+
+  defp validate_location_change(_, _), do: []
 
   defp validate_same_locations(changeset) do
     with from_change when not is_nil(from_change) <- get_change(changeset, :from),
+         true <- from_change.valid?,
          to_change when to_change === from_change <- get_change(changeset, :to) do
       add_error(
         changeset,
diff --git a/lib/dotcom_web/components/trip_planner/input_form.ex b/lib/dotcom_web/components/trip_planner/input_form.ex
index f132683e36..f02480b0ff 100644
--- a/lib/dotcom_web/components/trip_planner/input_form.ex
+++ b/lib/dotcom_web/components/trip_planner/input_form.ex
@@ -43,8 +43,9 @@ defmodule DotcomWeb.Components.TripPlanner.InputForm do
           </div>
           <button
             type="button"
+            disabled={disable_swap?(f)}
             phx-click="swap_direction"
-            class="px-xs bg-transparent fill-brand-primary hover:fill-black"
+            class="px-xs bg-transparent fill-brand-primary disabled:fill-gray-light hover:fill-black"
           >
             <span class="sr-only">Swap origin and destination locations</span>
             <.icon class="h-6 w-6 rotate-90 md:rotate-0" name="right-left" />
@@ -115,10 +116,13 @@ defmodule DotcomWeb.Components.TripPlanner.InputForm do
   defp location_search_box(assigns) do
     assigns =
       assigns
-      |> assign(:location_keys, InputForm.Location.fields())
+      |> assign(%{
+        has_error?: used_input?(assigns.field) && length(assigns.field.errors) > 0,
+        location_keys: InputForm.Location.fields()
+      })
 
     ~H"""
-    <fieldset class="mb-sm -mt-md" id={"#{@name}-wrapper"}>
+    <fieldset class={"mb-sm -mt-md #{if(@has_error?, do: "text-danger")}"} id={"#{@name}-wrapper"}>
       <legend class="text-charcoal-40 m-0 py-sm">{Phoenix.Naming.humanize(@field.field)}</legend>
       <.algolia_autocomplete config_type="trip-planner" placeholder={@placeholder} id={@name}>
         <.inputs_for :let={location_f} field={@field} skip_hidden={true}>
@@ -130,7 +134,7 @@ defmodule DotcomWeb.Components.TripPlanner.InputForm do
             name={location_f[subfield].name}
           />
         </.inputs_for>
-        <.feedback :for={{msg, _} <- @field.errors} :if={used_input?(@field)} kind={:error}>
+        <.feedback :for={{msg, _} <- @field.errors} :if={@has_error?} kind={:error}>
           {msg}
         </.feedback>
       </.algolia_autocomplete>
@@ -147,6 +151,12 @@ defmodule DotcomWeb.Components.TripPlanner.InputForm do
     }
   end
 
+  defp disable_swap?(%{errors: [_ | _] = errors}) do
+    :from in Keyword.keys(errors) or :to in Keyword.keys(errors)
+  end
+
+  defp disable_swap?(_), do: false
+
   defp show_datepicker?(f) do
     input_value(f, :datetime_type) != "now"
   end
diff --git a/test/dotcom/trip_plan/input_form_test.exs b/test/dotcom/trip_plan/input_form_test.exs
index 7b73ed3a4f..c28affcd9e 100644
--- a/test/dotcom/trip_plan/input_form_test.exs
+++ b/test/dotcom/trip_plan/input_form_test.exs
@@ -60,6 +60,23 @@ defmodule Dotcom.TripPlan.InputFormTest do
       refute changeset.errors[:from]
     end
 
+    test "adds error if invalid location" do
+      changeset =
+        InputForm.changeset(%{
+          "to" => %{
+            "latitude" => "",
+            "longitude" => "",
+            "name" => Faker.Cat.breed(),
+            "stop_id" => ""
+          }
+        })
+
+      refute changeset.valid?
+
+      expected_error = InputForm.error_message(:to)
+      assert {^expected_error, _} = changeset.errors[:to]
+    end
+
     test "adds error if from & to are the same" do
       changeset =
         InputForm.changeset(%{
diff --git a/test/dotcom_web/live/trip_planner_test.exs b/test/dotcom_web/live/trip_planner_test.exs
index 4bf87ea9e3..a0a47781f5 100644
--- a/test/dotcom_web/live/trip_planner_test.exs
+++ b/test/dotcom_web/live/trip_planner_test.exs
@@ -144,6 +144,23 @@ defmodule DotcomWeb.Live.TripPlannerTest do
              ]
     end
 
+    test "disabled swap with invalid from/to", %{view: view} do
+      # Setup
+      stub(OpenTripPlannerClient.Mock, :plan, fn _ ->
+        {:ok, %OpenTripPlannerClient.Plan{itineraries: []}}
+      end)
+
+      params = Map.take(@valid_params, ["from"]) |> Map.put("to", %{"name" => Faker.Cat.breed()})
+
+      # Exercise
+      view |> element("form") |> render_change(%{"input_form" => params})
+
+      swap_button = element(view, "button[phx-click='swap_direction']") |> render()
+
+      # Verify
+      assert swap_button =~ "disabled=\"disabled\""
+    end
+
     test "selecting a time other than 'now' shows the datepicker", %{view: view} do
       # Setup
       params = %{