feat(stop_events): add ApiWeb.StopEventView#979
Conversation
runkelcorey
added
enhancement
| field: :vehicle_id | ||
| ) | ||
|
|
||
| attributes([ |
There was a problem hiding this comment.
I think it's redundant to declare :vehicle_id, :trip_id, etc here; the has_one calls above implicitly declare these fields as part of the relationships to other types. I'd also suggest putting attributes before all the has_ones, just for stylistic consistency with other view definitions. Compare to stop_view.ex and vehicle_view.ex.
| def departed(%{departed: %DateTime{} = dt}, _conn), do: DateTime.to_iso8601(dt) | ||
|
|
||
| @doc """ | ||
| Preloads schedule relationships for stop events when requested via ?include=schedule to prevent N+1 queries. |
There was a problem hiding this comment.
Do we want to preload other relationships too? I believe preload(super, ...) would do that.
| assert get_in(rendered, ["data", Access.at(0), "relationships", "schedule", "data", "id"]) == | ||
| "schedule-trip1-stop1-1" | ||
|
|
||
| assert get_in(rendered, ["data", Access.at(1), "relationships", "schedule", "data", "id"]) == |
There was a problem hiding this comment.
Should you also assert that rendered["included"] contains the actual contents of the schedules? (Here and in the next test case.)
| refute Map.has_key?(relationships, "schedule") | ||
| end | ||
|
|
||
| test "preloads schedules for multiple stop_events", %{conn: conn} do |
There was a problem hiding this comment.
I see that here you're testing for ?include=schedule in the request, but do you also want to support (for example) ?include=trip? If so there should be a test case for at least one of the relationships.
3 participants
Part 2 of disaggregating #973
Summary of changes
Model.StopEventtoApiWeb.StopEventViewState.Schedule.schedule_forandState.Schedule.schedule_for_manyto accept aModel.StopEventA future PR will add the
locationfor StopEvents along with the StopEventController. I think that's also the right place to test for included trip/stop/route/vehicle lookups that don't return results.How were these changes validated?
ApiWeb.StopEventViewWhat questions should reviewers consider?
include?State.ScheduleTestto test a slightly wider type? I think the current tests are sufficient b/c the functionality doesn't really change