-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: optionally use OpenTripPlanner for nearby transit #41
Changes from 3 commits
399192e
78771b1
943f0f3
4b4614c
c1bf4ea
3ba001e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
defmodule OpenTripPlannerClient do | ||
@moduledoc """ | ||
Fetches data from the OpenTripPlanner API. | ||
|
||
## Configuration | ||
|
||
```elixir | ||
config :mobile_app_backend, | ||
otp_url: "http://localhost:8080" | ||
``` | ||
""" | ||
|
||
require Logger | ||
|
||
alias OpenTripPlannerClient.Nearby | ||
|
||
@doc """ | ||
Fetches stops within the given number of meters of the given position. | ||
""" | ||
@spec nearby(float(), float(), integer(), Keyword.t()) :: | ||
{:ok, [MBTAV3API.Stop.t()]} | {:error, term()} | ||
def nearby(latitude, longitude, radius, opts \\ []) do | ||
root_url = | ||
Keyword.get(opts, :root_url, Application.fetch_env!(:mobile_app_backend, :otp_url)) | ||
|
||
request = | ||
Nearby.request(latitude, longitude, radius) | ||
|> Req.update(base_url: root_url, url: "/otp/routers/default/index/graphql") | ||
|
||
case send_request(request) do | ||
{:ok, body} -> Nearby.parse(body) | ||
{:error, error} -> {:error, error} | ||
end | ||
end | ||
|
||
@spec send_request(Req.Request.t()) :: {:ok, term()} | {:error, term()} | ||
defp send_request(request) do | ||
with {:ok, response} <- log_response(request), | ||
%{status: 200, body: body} <- response do | ||
{:ok, body} | ||
else | ||
%{status: _} = response -> | ||
{:error, response} | ||
|
||
error -> | ||
error | ||
end | ||
end | ||
|
||
@spec log_response(Req.Request.t()) :: {:ok, Req.Response.t()} | {:error, term()} | ||
defp log_response(request) do | ||
{duration, response} = | ||
:timer.tc( | ||
MobileAppBackend.HTTP, | ||
:request, | ||
[request] | ||
) | ||
|
||
_ = | ||
Logger.info(fn -> | ||
"#{__MODULE__}.otp_response query=#{inspect(request.options[:graphql])} #{status_text(response)} duration=#{duration / :timer.seconds(1)}" | ||
end) | ||
|
||
response | ||
end | ||
|
||
defp status_text({:ok, %{status: code}}) do | ||
"status=#{code}" | ||
end | ||
|
||
defp status_text({:error, error}) do | ||
"status=error error=#{inspect(error)}" | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
defmodule OpenTripPlannerClient.Nearby do | ||
@spec request(float(), float(), integer()) :: Req.Request.t() | ||
def request(latitude, longitude, radius) do | ||
Req.new(method: :post) | ||
|> AbsintheClient.attach( | ||
graphql: {graphql_query(), %{latitude: latitude, longitude: longitude, radius: radius}} | ||
) | ||
end | ||
|
||
@spec parse(map()) :: {:ok, [MBTAV3API.Stop.t()]} | {:error, term()} | ||
def parse(data) do | ||
case data do | ||
%{"data" => %{"nearest" => %{"edges" => edges}}} -> | ||
for edge <- edges, reduce: {:ok, []} do | ||
{:ok, stops} -> | ||
with %{"node" => %{"place" => stop}} <- edge, | ||
{:ok, stop} <- parse_stop(stop) do | ||
{:ok, [stop | stops]} | ||
end | ||
end | ||
|> case do | ||
{:ok, stops} -> {:ok, Enum.reverse(stops)} | ||
end | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: I find this function a bit hard to read with the nesting, but it may also be because I am less familiar with the for comprehension than reducing via enum. Could this perhaps be broken into smaller steps, or use more piped steps? The single pipe on line 21 into a case with only one condition is a bit confusing as well - could pattern matching be used here if there is only one case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The idea here was to implement a skeleton on top of which we can build error handling without having to rearchitect all this code, but with no actual error handling it's just harder to read for no reason. Conveniently, in my visualization work I encountered the problem of Massport shuttle stops being neither errors nor success, so it's worth fleshing this out somewhat to ensure those stops are correctly filtered out. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't know much about the massport shuttle and the expectations for whether or not it would show up in nearby transit - filtering seems reasonable for now, but can you ask product/design if you haven't done so already about how to handle them? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
|
||
_ -> | ||
{:error, :bad_format} | ||
end | ||
end | ||
|
||
defp graphql_query do | ||
""" | ||
query NearbyQuery($latitude: Float!, $longitude: Float!, $radius: Int!) { | ||
nearest(lat: $latitude, lon: $longitude, maxDistance: $radius, filterByPlaceTypes: [STOP]) { | ||
edges { | ||
node { | ||
place { | ||
... on Stop { | ||
...stopDetails | ||
parentStation { | ||
...stopDetails | ||
} | ||
} | ||
} | ||
distance | ||
} | ||
} | ||
} | ||
} | ||
|
||
fragment stopDetails on Stop { | ||
gtfsId | ||
lat | ||
lon | ||
name | ||
} | ||
""" | ||
end | ||
|
||
@spec parse_stop(map()) :: {:ok, MBTAV3API.Stop.t()} | {:error, term()} | ||
defp parse_stop(place) do | ||
case place do | ||
%{"lat" => latitude, "lon" => longitude, "gtfsId" => "mbta-ma-us:" <> id, "name" => name} -> | ||
{:ok, | ||
%MBTAV3API.Stop{ | ||
id: id, | ||
latitude: latitude, | ||
longitude: longitude, | ||
name: name, | ||
parent_station: | ||
with {:ok, parent_station} when not is_nil(parent_station) <- | ||
Map.fetch(place, "parentStation"), | ||
{:ok, parent_station} <- parse_stop(parent_station) do | ||
parent_station | ||
else | ||
_ -> nil | ||
end | ||
}} | ||
end | ||
end | ||
end |
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
{"data":{"nearest":{"edges":[{"node":{"distance":354,"place":{"gtfsId":"mbta-ma-us:67120","lat":42.28101,"lon":-71.177035,"name":"Millennium Park","parentStation":null}}},{"node":{"distance":618,"place":{"gtfsId":"mbta-ma-us:129","lat":42.278959,"lon":-71.175667,"name":"Rivermoor St @ Charles Park Rd","parentStation":null}}},{"node":{"distance":627,"place":{"gtfsId":"mbta-ma-us:137","lat":42.278907,"lon":-71.175492,"name":"Charles Park Rd @ Rivermoor St","parentStation":null}}}]}}} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion: could you please add a case where there is a stop that has a parentStation? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this is a nice approach while we're in the testing phase! Can you please add a ticket to the backlog so we can revisit what to do here once we've learned more? Do we want to remove one entirely? Do we want to keep both options and have configuration on the backend so that we can make a switch between the two with a deploy if needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://app.asana.com/0/1205425564113216/1206433651837131/f