diff --git a/.credo.exs b/.credo.exs index 2f5b72d..bc2f924 100644 --- a/.credo.exs +++ b/.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`. # diff --git a/config/config.exs b/config/config.exs index 8e1a3b5..b01e808 100644 --- a/config/config.exs +++ b/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", diff --git a/lib/open_trip_planner_client/error.ex b/lib/open_trip_planner_client/error.ex new file mode 100644 index 0000000..4976e40 --- /dev/null +++ b/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 diff --git a/lib/open_trip_planner_client/parser.ex b/lib/open_trip_planner_client/parser.ex index d82b691..2792be0 100644 --- a/lib/open_trip_planner_client/parser.ex +++ b/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 diff --git a/lib/open_trip_planner_client/plan.ex b/lib/open_trip_planner_client/plan.ex index f78064b..c02f12f 100644 --- a/lib/open_trip_planner_client/plan.ex +++ b/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 diff --git a/lib/open_trip_planner_client/util.ex b/lib/open_trip_planner_client/util.ex index ed69c89..3c52291 100644 --- a/lib/open_trip_planner_client/util.ex +++ b/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, diff --git a/mix.exs b/mix.exs index e166818..0005e6b 100644 --- a/mix.exs +++ b/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/ ] ], diff --git a/test/open_trip_planner_client/error_test.exs b/test/open_trip_planner_client/error_test.exs new file mode 100644 index 0000000..01bd803 --- /dev/null +++ b/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 diff --git a/test/open_trip_planner_client/parser_test.exs b/test/open_trip_planner_client/parser_test.exs index 5bfe15f..632eb36 100644 --- a/test/open_trip_planner_client/parser_test.exs +++ b/test/open_trip_planner_client/parser_test.exs @@ -2,10 +2,11 @@ defmodule OpenTripPlannerClient.ParserTest do use ExUnit.Case, async: true import ExUnit.CaptureLog import OpenTripPlannerClient.Parser + import OpenTripPlannerClient.Test.Support.Factory describe "validate_body/1" do test "handles GraphQL request error" do - assert {{:error, :graphql_request_error}, log} = + assert {{:error, errors}, log} = with_log(fn -> validate_body(%{ errors: [ @@ -38,47 +39,89 @@ defmodule OpenTripPlannerClient.ParserTest do }) end) + assert errors == [ + %OpenTripPlannerClient.Error{ + details: %{ + extensions: %{classification: "ValidationError"}, + locations: [%{line: 3, column: 16}] + }, + message: + "Validation error (UndefinedVariable@[plan]) : Undefined variable 'from'", + type: :graphql_error + }, + %OpenTripPlannerClient.Error{ + details: %{ + extensions: %{classification: "ValidationError"}, + locations: [%{line: 1, column: 16}] + }, + message: "Validation error (UnusedVariable) : Unused variable 'fromPlace'", + type: :graphql_error + } + ] + assert log =~ "Validation error" end test "handles GraphQL field error" do - assert {{:error, :graphql_field_error}, log} = - with_log(fn -> - validate_body(%{ - data: %{plan: nil}, - errors: [ - %{ - message: - "Exception while fetching data (/plan) : The value is not in range[0.0, 1.7976931348623157E308]: -5.0", - locations: [ - %{ - line: 2, - column: 3 - } - ], - path: [ - "plan" - ], - extensions: %{ - classification: "DataFetchingException" - } - } - ] - }) - end) + {{:error, [error]}, log} = + with_log(fn -> + validate_body(%{ + data: %{plan: nil}, + errors: [ + %{ + message: + "Exception while fetching data (/plan) : The value is not in range[0.0, 1.7976931348623157E308]: -5.0", + locations: [ + %{ + line: 2, + column: 3 + } + ], + path: [ + "plan" + ], + extensions: %{ + classification: "DataFetchingException" + } + } + ] + }) + end) + + assert error == %OpenTripPlannerClient.Error{ + details: %{ + path: ["plan"], + extensions: %{classification: "DataFetchingException"}, + locations: [%{line: 2, column: 3}] + }, + message: + "Exception while fetching data (/plan) : The value is not in range[0.0, 1.7976931348623157E308]: -5.0", + type: :graphql_error + } assert log =~ "Exception while fetching data" end - test "handles routing errors" do - assert {{:error, "PATH_NOT_FOUND"}, log} = + test "handles and logs routing errors" do + code = "PATH_NOT_FOUND" + routing_error = build(:routing_error, code: code) + + assert {{:error, errors}, log} = with_log(fn -> validate_body(%{ - data: %{plan: %{routing_errors: [%{code: "PATH_NOT_FOUND"}]}} + data: %{plan: %{routing_errors: [routing_error]}} }) end) - assert log =~ "PATH_NOT_FOUND" + assert [ + %OpenTripPlannerClient.Error{ + details: ^routing_error, + message: "Something went wrong.", + type: :routing_error + } + ] = errors + + assert log =~ code end test "does not treat 'WALKING_BETTER_THAN_TRANSIT' as a fatal error" do diff --git a/test/support/factory.ex b/test/support/factory.ex index 2e9c3a5..525373b 100644 --- a/test/support/factory.ex +++ b/test/support/factory.ex @@ -9,7 +9,7 @@ if Code.ensure_loaded?(ExMachina) and Code.ensure_loaded?(Faker) do import Faker.Random.Elixir, only: [random_uniform: 0] - alias OpenTripPlannerClient.PlanParams + alias OpenTripPlannerClient.{Plan, PlanParams} alias OpenTripPlannerClient.Schema.{ Agency, @@ -24,6 +24,35 @@ if Code.ensure_loaded?(ExMachina) and Code.ensure_loaded?(Faker) do Trip } + def plan_factory do + %Plan{ + date: Faker.DateTime.forward(2), + itineraries: __MODULE__.build_list(3, :itinerary), + routing_errors: [], + search_window_used: 3600 + } + end + + def plan_with_errors_factory do + build(:plan, routing_errors: __MODULE__.build_list(2, :routing_error)) + end + + def routing_error_factory do + %{ + code: + Faker.Util.pick([ + "NO_TRANSIT_CONNECTION", + "NO_TRANSIT_CONNECTION_IN_SEARCH_WINDOW", + "OUTSIDE_SERVICE_PERIOD", + "OUTSIDE_BOUNDS", + "LOCATION_NOT_FOUND", + "NO_STOPS_IN_RANGE", + "WALKING_BETTER_THAN_TRANSIT" + ]), + description: Faker.Lorem.sentence(3) + } + end + def agency_factory do %Agency{ name: Faker.Util.pick(["MBTA", "Massport", "Logan Express"])