Merge pull request #1 from mbta/cbj/errors

feat(OpenTripPlannerClient.Error): more detailed erroring

.credo.exs

@@ -141,7 +141,11 @@
           {Credo.Check.Refactor.MapJoin, []},
           {Credo.Check.Refactor.MapMap, []},
           {Credo.Check.Refactor.MatchInCondition, []},
-          {Credo.Check.Refactor.ModuleDependencies, [excluded_namespaces: ["OpenTripPlannerClient.Schema"], max_deps: 10]},
+          {Credo.Check.Refactor.ModuleDependencies,
+           [
+             excluded_namespaces: ["OpenTripPlannerClient.Schema", "OpenTripPlannerClient.Util"],
+             max_deps: 15
+           ]},
           {Credo.Check.Refactor.NegatedConditionsInUnless, []},
           {Credo.Check.Refactor.NegatedConditionsWithElse, []},
           {Credo.Check.Refactor.NegatedIsNil, []},
@@ -196,7 +200,7 @@
           {Credo.Check.Readability.MultiAlias, []},
           {Credo.Check.Readability.SinglePipe, []},
           {Credo.Check.Warning.LazyLogging, []},
-          {Credo.Check.Refactor.MapInto, []},
+          {Credo.Check.Refactor.MapInto, []}
           #
           # Custom checks can be created using `mix credo.gen.check`.
           #

config/config.exs

@@ -3,7 +3,8 @@ import Config
 config :open_trip_planner_client,
   otp_url: "http://otp2-local.mbtace.com",
   # otp_url: "http://localhost:8080",
-  timezone: "America/New_York"
+  timezone: "America/New_York",
+  fallback_error_message: "Something went wrong."
 
 config :logger, :default_formatter,
   format: "[$level] $message $metadata\n",

lib/open_trip_planner_client/error.ex

@@ -0,0 +1,98 @@
+defmodule OpenTripPlannerClient.Error do
+  @moduledoc """
+  Describes errors from OpenTripPlanner, including routing errors from the /plan
+  endpoint for now. For routing errors, generates custom human-friendly error
+  messages based on routing error code and plan details.
+  """
+
+  alias OpenTripPlannerClient.Plan
+  alias Timex.{Duration, Format.Duration.Formatter}
+
+  require Logger
+
+  defstruct [:details, :message, :type]
+
+  @type t :: %__MODULE__{
+          details: map(),
+          message: String.t(),
+          type: :graphql_error | :routing_error
+        }
+
+  @spec from_graphql_error(map()) :: t()
+  def from_graphql_error(error) do
+    _ = log_error(error)
+
+    {message, details} = Map.pop(error, :message)
+    %__MODULE__{details: details, message: message, type: :graphql_error}
+  end
+
+  @spec from_routing_errors(Plan.t()) :: t()
+  def from_routing_errors(%Plan{routing_errors: routing_errors} = plan) do
+    _ = log_error(routing_errors)
+
+    for %{code: code, description: description} <- routing_errors do
+      %__MODULE__{
+        details: %{code: code, description: description},
+        message: code_to_message(code, description, plan),
+        type: :routing_error
+      }
+    end
+  end
+
+  defp code_to_message(code, description, _)
+       when code in ["LOCATION_NOT_FOUND", "NO_STOPS_IN_RANGE"] do
+    case description do
+      "Origin" <> _ ->
+        "Origin location is not close enough to any transit stops"
+
+      "Destination" <> _ ->
+        "Destination location is not close enough to any transit stops"
+
+      _ ->
+        "Location is not close enough to any transit stops"
+    end
+  end
+
+  defp code_to_message("NO_TRANSIT_CONNECTION", _, _) do
+    "No transit connection was found between the origin and destination on this date and time"
+  end
+
+  defp code_to_message("OUTSIDE_BOUNDS", _, _) do
+    "Origin or destination location is outside of our service area"
+  end
+
+  defp code_to_message("NO_TRANSIT_CONNECTION_IN_SEARCH_WINDOW", _, %Plan{} = plan) do
+    with window when is_binary(window) <- humanized_search_window(plan.search_window_used),
+         {:ok, formatted_datetime} <- humanized_full_date(plan.date) do
+      "No transit routes found within #{window} of #{formatted_datetime}. Routes may be available at other times."
+    else
+      _ ->
+        fallback_error_message()
+    end
+  end
+
+  defp code_to_message(_, _, _), do: fallback_error_message()
+
+  defp humanized_search_window(number) do
+    number
+    |> Duration.from_seconds()
+    |> Formatter.format(:humanized)
+  end
+
+  defp humanized_full_date(datetime) when is_integer(datetime) do
+    datetime
+    |> OpenTripPlannerClient.Util.to_local_time()
+    |> Timex.format("{h12}:{m}{am} on {WDfull}, {Mfull} {D}")
+  end
+
+  defp fallback_error_message do
+    Application.get_env(
+      :open_trip_planner_client,
+      :fallback_error_message
+    )
+  end
+
+  defp log_error(errors) when is_list(errors), do: Enum.each(errors, &log_error/1)
+
+  defp log_error(error), do: Logger.error(error)
+end

lib/open_trip_planner_client/parser.ex

@@ -4,10 +4,7 @@ defmodule OpenTripPlannerClient.Parser do
   errors and trip planner errors into standard formats for logging and testing.
   """
 
-  require Logger
-
-  @type parse_error ::
-          :graphql_field_error | :graphql_request_error | OpenTripPlannerClient.Behaviour.error()
+  alias OpenTripPlannerClient.{Error, Plan}
 
   @walking_better_than_transit "WALKING_BETTER_THAN_TRANSIT"
 
@@ -28,53 +25,39 @@ defmodule OpenTripPlannerClient.Parser do
   null), the errors entry must be present if and only if one or more field error
   was raised during execution.
   """
-  @spec validate_body(%{}) :: {:ok, OpenTripPlannerClient.Plan.t()} | {:error, any()}
-  def validate_body(body) do
-    body
-    |> validate_graphql()
-    |> drop_walking_errors()
-    |> validate_routing()
-  end
+  @spec validate_body(map()) :: {:ok, Plan.t()} | {:error, term()}
 
-  defp validate_graphql(%{errors: [_ | _] = errors} = body) do
-    log_error(errors)
-
-    case body do
-      %{data: _} ->
-        {:error, :graphql_field_error}
+  def validate_body(%{errors: [_ | _] = errors}) do
+    {:error, Enum.map(errors, &Error.from_graphql_error/1)}
+  end
 
-      _ ->
-        {:error, :graphql_request_error}
+  def validate_body(body) do
+    with {:ok, plan} <- valid_plan(body),
+         {:ok, %Plan{} = decoded_plan} <- Nestru.decode(plan, Plan) do
+      decoded_plan
+      |> drop_nonfatal_errors()
+      |> valid_plan()
+    else
+      error ->
+        error
     end
   end
 
-  defp validate_graphql(body), do: body
+  defp valid_plan(%Plan{routing_errors: []} = plan), do: {:ok, plan}
 
-  defp drop_walking_errors(%{data: %{plan: %{routing_errors: routing_errors}}} = body)
-       when is_list(routing_errors) do
-    update_in(body, [:data, :plan, :routing_errors], &reject_walking_errors/1)
-  end
-
-  defp drop_walking_errors(body), do: body
+  defp valid_plan(%Plan{} = plan),
+    do: {:error, Error.from_routing_errors(plan)}
 
-  defp reject_walking_errors(routing_errors) do
-    Enum.reject(routing_errors, &(&1.code == @walking_better_than_transit))
-  end
+  defp valid_plan(%{data: %{plan: nil}}), do: {:error, :no_plan}
+  defp valid_plan(%{data: %{plan: plan}}), do: {:ok, plan}
 
-  defp validate_routing(%{
-         data: %{plan: %{routing_errors: [%{code: code} | _] = routing_errors}}
-       }) do
-    log_error(routing_errors)
-    {:error, code}
+  defp valid_plan(_) do
+    {:error, :no_data}
   end
 
-  defp validate_routing(%{data: %{plan: plan}}) do
-    Nestru.decode(plan, OpenTripPlannerClient.Plan)
+  defp drop_nonfatal_errors(plan) do
+    plan.routing_errors
+    |> Enum.reject(&(&1.code == @walking_better_than_transit))
+    |> then(&%Plan{plan | routing_errors: &1})
   end
-
-  defp validate_routing(body), do: body
-
-  defp log_error(errors) when is_list(errors), do: Enum.each(errors, &log_error/1)
-
-  defp log_error(error), do: Logger.error(error)
 end

lib/open_trip_planner_client/plan.ex

@@ -18,8 +18,7 @@ defmodule OpenTripPlannerClient.Plan do
         |> update_in([:itineraries], &replace_nil_with_list/1)
         |> update_in([:date], fn
           dt when is_integer(dt) ->
-            Timex.from_unix(dt, :milliseconds)
-            |> OpenTripPlannerClient.Util.to_local_time()
+            OpenTripPlannerClient.Util.to_local_time(dt)
 
           dt ->
             dt

lib/open_trip_planner_client/util.ex

@@ -20,6 +20,12 @@ defmodule OpenTripPlannerClient.Util do
   def to_uppercase_atom(other), do: other
 
   @spec to_local_time(Timex.Types.valid_datetime()) :: DateTime.t()
+  def to_local_time(datetime) when is_integer(datetime) do
+    datetime
+    |> Timex.from_unix(:milliseconds)
+    |> to_local_time()
+  end
+
   def to_local_time(datetime) do
     Timex.to_datetime(
       datetime,

mix.exs

@@ -17,7 +17,9 @@ defmodule OpenTripPlannerClient.MixProject do
       test_coverage: [
         ignore_modules: [
           Mix.Tasks.UpdateFixture,
-          ~r/OpenTripPlannerClient.Schema\./
+          ~r/Jason.Encoder/,
+          ~r/OpenTripPlannerClient.Schema\./,
+          ~r/Nestru/
         ]
       ],
 

test/open_trip_planner_client/error_test.exs

@@ -0,0 +1,88 @@
+defmodule OpenTripPlannerClient.ErrorTest do
+  use ExUnit.Case, async: true
+
+  import OpenTripPlannerClient.Error
+  import OpenTripPlannerClient.Test.Support.Factory
+
+  alias OpenTripPlannerClient.Error
+
+  describe "from_graphql_error/2" do
+    test "handles one error" do
+      message = Faker.Lorem.sentence(3, ".")
+      error = %{message: message}
+
+      assert %Error{details: %{}, type: :graphql_error, message: ^message} =
+               from_graphql_error(error)
+    end
+  end
+
+  describe "from_routing_errors/1" do
+    test "shows a configured fallback message" do
+      assert [%Error{message: custom_fallback}] =
+               from_routing_errors(
+                 build(:plan, routing_errors: [build(:routing_error, %{code: "Fake"})])
+               )
+
+      assert custom_fallback ==
+               Application.get_env(:open_trip_planner_client, :fallback_error_message)
+    end
+
+    test "displays differing message based on error described for origin vs destination" do
+      plan =
+        build(:plan,
+          routing_errors: [
+            %{code: "LOCATION_NOT_FOUND", description: "Origin location not found"},
+            %{code: "LOCATION_NOT_FOUND", description: "Destination location not found"},
+            %{code: "LOCATION_NOT_FOUND", description: "Some other message"}
+          ]
+        )
+
+      [origin_message, destination_message, message] =
+        from_routing_errors(plan) |> Enum.map(& &1.message)
+
+      assert origin_message != destination_message
+      assert origin_message =~ "is not close enough to any transit stops"
+      assert destination_message =~ "is not close enough to any transit stops"
+      assert message =~ "Location is not close enough to any transit stops"
+    end
+
+    test "message for NO_TRANSIT_CONNECTION" do
+      assert [%Error{message: message}] =
+               from_routing_errors(plan_with_error_code("NO_TRANSIT_CONNECTION"))
+
+      assert message =~ "No transit connection was found"
+    end
+
+    test "message for OUTSIDE_BOUNDS" do
+      assert [%Error{message: message}] =
+               from_routing_errors(plan_with_error_code("OUTSIDE_BOUNDS"))
+
+      assert message =~ "is outside of our service area"
+    end
+
+    test "detailed message for NO_TRANSIT_CONNECTION_IN_SEARCH_WINDOW" do
+      search_window_used = Faker.random_between(600, 7200)
+      date = Faker.DateTime.forward(2)
+
+      plan =
+        build(:plan, %{
+          date: Timex.to_unix(date) * 1000,
+          itineraries: [],
+          routing_errors: [
+            build(:routing_error, %{code: "NO_TRANSIT_CONNECTION_IN_SEARCH_WINDOW"})
+          ],
+          search_window_used: search_window_used
+        })
+
+      assert [%Error{message: message}] = from_routing_errors(plan)
+
+      assert message =~ "Routes may be available at other times"
+    end
+  end
+
+  defp plan_with_error_code(code) do
+    build(:plan, %{
+      routing_errors: [build(:routing_error, %{code: code})]
+    })
+  end
+end