feat(OpenTripPlannerClient.Error): more detailed erroring #1
Conversation
thecristen
requested review from
KaylaBrady and
joshlarson
and removed request for
a team
|
|
||
| for %{code: code, description: description} <- routing_errors do | ||
| %__MODULE__{ | ||
| message: code_to_message(code, description, plan), |
There was a problem hiding this comment.
suggestion: I think it would be worth preserving this code as well to support localization on the client side.
There was a problem hiding this comment.
I agree with this. Also, that way if we wind up having to use the fallback error message, it's easier to debug why.
There was a problem hiding this comment.
Can you expand on this a little? I've too little experience with localization but I want to better understand what doing this actually entails!
There was a problem hiding this comment.
Sure thing! Since the mobile app needs to have manually added translations for every string that a user will see (or hear for voice over users), it is easier for us to create set of strings that need to be translated from a field with a limited set of values rather than a free-text field like message.
Some ios-specific details on what that looks like in practice: We currently maintain a localization file that contains all the strings with their translations. Here is an example where we turn enum values for an alert effect returned from the backend into translatable text strings (the Text view does some magic in XCode to automatically create placeholder entries in the localization file, which we then can manually add or import translations for).
In the future if we know that we definitely want to display the same error strings across web and mobile we could consider passing a locale code to the trip planning endpoint and push more localization server-side. It looks like OTP supports passing a locale code via the accept-language, which we may also want to explore.
| assert origin_message =~ "is not close enough to any transit stops" | ||
| assert origin_message =~ "is not close enough to any transit stops" | ||
| assert message =~ "Location is not close enough to any transit stops" |
There was a problem hiding this comment.
question (non-blocking): Should line 45 be destination_message here?
config/config.exs
Outdated
| @@ -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 else went wrong." | |||
There was a problem hiding this comment.
What about just "Something went wrong"? If I'm reading the rest of this PR right, this error would appear in a context like:
%OpenTripPlannerClient.Error{
message: "Something else went wrong."
}Which feels to me like it wants to be at the end of a long list of errors: "Your origin was too far away from anything, your destination was too warm, the bus driver on the optimal route doesn't like you, AND something else went wrong".
I'd go with either "Something else went wrong" or "Unknown error".
test/support/factory.ex
Outdated
| %Plan{ | ||
| date: Faker.DateTime.forward(2), | ||
| itineraries: __MODULE__.build_list(3, :itinerary), | ||
| routing_errors: Faker.Util.pick([__MODULE__.build_list(1, :routing_error), []]), |
There was a problem hiding this comment.
I would split out a plan_with_errors factory from a plan_factory. I would be surprised if I went build(:plan) and got a plan that had errors.
| end | ||
|
|
||
| defp validate_routing(%{data: %{plan: plan}}) do | ||
| Nestru.decode(plan, OpenTripPlannerClient.Plan) | ||
| defp with_errors(plan) do |
There was a problem hiding this comment.
Nit (non-blocking): I don't think I'm getting a full picture of what this function does based on the name with_errors. It almost feels like drop_nonfatal_errors or something along those lines would be a more communicative name?
| with_log(fn -> | ||
| validate_body(%{ | ||
| data: %{plan: %{routing_errors: [%{code: "PATH_NOT_FOUND"}]}} | ||
| }) | ||
| end) | ||
|
|
||
| assert error == [ |
There was a problem hiding this comment.
Nit (non-blocking): error in this test is a list of errors. Could you either call it errors or destructure it as [error] on line 105 where it's initialized?
Part of Trip Planner returns error messages.
First step... get OTP's error messages out and send them along to client users!
This also incorporates the custom error messages we want to display in the trip planner.
Surprise feature: A custom fallback error message can be assigned via