feat(ApiWeb): add /transfers endpoint - #1074
Conversation
| relationship(:from_trip) | ||
| relationship(:to_trip) |
There was a problem hiding this comment.
Thank you for adding transfers to the API 🙌
question(non-blocking): With this PR establishing the relationship from transfer => trips, do you have a sense of if it would be possible to later add a relationship from trip => transfer?
Definitely out of scope for this PR, but I wonder if it is feasible to support a query like /trips/trip_id?include=transfers
There was a problem hiding this comment.
I admittedly don't know how implementing includes works, but I think with the rest of these pieces in place, it'd be a relatively light lift!
There was a problem hiding this comment.
I ended up figuring out how to implement it in feat(TripView): include transfers
There was a problem hiding this comment.
Standby while I work on making it ready for prime time - I'm missing a few attributes
|
|
||
| def index_data(conn, params) do | ||
| case Params.filter_params(params, @filters, conn) do | ||
| {:ok, filters} when map_size(filters) > 0 -> |
There was a problem hiding this comment.
issue: so I worry a bit that by allowing only a filter by type, and allowing the type filter to take a comma-separated list of types, it's still trivially easy to write a query that would return all of the transfers. I'm not sure what to do about it, though, because we've presumably got to allow some filter more general than trip IDs for your use cases on dotcom.
There was a problem hiding this comment.
Maybe we could deploy to a dev environment and see what response times are actually like when filtering by multiple types to get a sense of how much of an issue it actually is.
There was a problem hiding this comment.
Let's send this to a dev environment! Maybe you can pick which environment/when? Don't want to interfere with anything else that might be going on.
There was a problem hiding this comment.
It looks like dev-blue auto-deploys the latest master every night, so you probably want dev-green if you don't want to have to redeploy every day. It doesn't look like anyone else is using dev-green right now.
There was a problem hiding this comment.
Re: the volume concern, it looks like our current GTFS (with the just-recently-released Fall rating included) is 13930 entries, so that's the kind of scale we're talking about.
| end | ||
|
|
||
| def transfers(%{id: trip_id}, _conn) do | ||
| case Transfer.by_from_trip_id(trip_id) do |
There was a problem hiding this comment.
question: so to confirm, including transfers for a given trip will give you transfers from that trip, and not to it? If so I think I would argue for calling the include argument something else if possible, like from_trip_transfers.
There was a problem hiding this comment.
Yeah, I just realized that might've been flawed now that I'm trying to use it... 😩 . Will re-do
3 participants
Summary of changes
Asana Ticket: Follow up on "🛜 🐞 Daily Schedules sometimes doesn't show school trips " prerequisite
👋🏼 To facilitate displaying in-seat transfers to riders, I embarked on an effort to finally learn V3 API development in order to expose the
transfers.txtGTFS data. Please let me know if there are pieces I overlooked or misplaced.At a high level, this PR:
transfers.txt:from_trip_id) and by type (i.e.:transfer_type). Not my prettiest code, most of it is copied from theFacilitiesControllerbut it works ✨from_trip,to_trip,from_stop, andto_stopmodeled as relationships... open to feedback on that one. I had an earlier draft which showed these as*_idattributes instead, just not sure what the best choice was there./statusendpoint is updated to show last updated timestamp for transfersI also tried to flesh out the documentation as thoroughly as I could think of, happy to field feedback on that too!