feat(TripDetails): Remove predictions for already-visited stops

[thecristen]

Scope

Asana Ticket: [SF/UD] Trip Details: Hide predictions for stops that the vehicle has already visited

Implementation

A refactor I did to make this easier -- The vehicle used in UpcomingDepartures is already matched to the predicted schedule's stop and stop sequence -- if it also is on the predicted schedule's trip (as opposed to a prior trip or some such), we can pass it into TripDetails directly and do the rest.
The actual task -- in the last commit, I adjust the logic to remove any prior trip stops based on the vehicle location (basically option B in the Asana ticket).

Screenshots

How to test

Since this was an intermittently-seen problem I'm not sure how to reliably reproduce, but I remember seeing it on the 9 bus semi-often...

---

• To see the specific tasks where the Asana app for GitHub is being used, see below:
  • https://app.asana.com/0/0/1212926668576293

[joshlarson]

Since this was an intermittently-seen problem I'm not sure how to reliably reproduce, but I remember seeing it on the 9 bus semi-often...

This bug reliably shows up when a trip is running more than a minute early, because if the prediction disappears, then its scheduled trip_stop may still be in the future. (That's not the only way this can happen, but it is a way it can happen)

Sooo.... you can throw 👇 into a LiveBook

route_id = "Orange" # Or whatever route_id you want
predictions = Predictions.Repo.all(route: route_id)
schedules = Schedules.Repo.by_route_ids([route_id])

predicted_schedules =
  PredictedSchedule.group(predictions, schedules)
  |> Enum.filter(&(&1.schedule != nil && &1.prediction != nil))
  |> Enum.filter(&(&1.schedule.arrival_time != nil && &1.prediction.arrival_time != nil))
  |> Enum.filter(&DateTime.before?(&1.prediction.arrival_time, &1.schedule.arrival_time))
  |> Enum.sort_by(& &1.prediction.arrival_time)
  |> Enum.map(
    &{PredictedSchedule.direction_id(&1), PredictedSchedule.stop(&1).name,
     PredictedSchedule.trip(&1).id, &1.prediction.arrival_time,
     DateTime.diff(&1.schedule.arrival_time, &1.prediction.arrival_time)}
  )

Look for results ☝️ that have larger DateTime.diff's, because the further ahead a trip is, the longer a stale schedule will stick around. You'll be able to see the bug if you look one or two stops downstream of the stop in the prediction - e.g. if the prediction is for Orange Line Southbound @ Downtown Crossing, then you could look at Chinatown or Tufts Medical Center (or further downstream if you want).

Anywho... here are some screenshots that show that this PR does in fact solve this issue:

[joshlarson]

Left a few style comments, and one about missing test coverage, but I wouldn't consider any of them blocking.

[joshlarson]

Question: Will this ever happen? I think it can in test, because the factory typically creates a vehicle that doesn't align with any of the predictions, but when UpcomingDepartures.upcoming_departures/1 is contemplating vehicles, it already doesn't pass in the vehicle if it's associated with a different trip.

TripDetails.trip_details(%{
  predicted_schedules: predicted_schedules_by_trip_id |> Map.get(trip_id, []),
  trip_vehicle: if(vehicle && vehicle.trip_id == trip_id, do: vehicle)
})

---

I don't actually feel strongly about "fixing" this, especially if doing so would be obnoxious wrt the tests... I'm mostly just curious.

[joshlarson]

Issue: I think there's missing test coverage here - replacing this whole if statement with Enum.drop(upcoming_stops, 1) doesn't cause any tests to fail.

test "does not drop the 'current' stop if the vehicle is :in_transit or :incoming" do
  trip = Factories.Schedules.Trip.build(:trip)

  stops = Faker.Util.sample_uniq(20, fn -> Factories.Stops.Stop.build(:stop) end)

  predicted_schedules =
    stops
    |> Enum.with_index()
    |> Enum.map(fn {stop, index} ->
      %PredictedSchedule{
        prediction:
          Factories.Predictions.Prediction.build(:prediction,
            trip: trip,
            stop: stop,
            stop_sequence: index
          ),
        schedule:
          Factories.Schedules.Schedule.build(:schedule,
            trip: trip,
            stop: stop,
            stop_sequence: index
          )
      }
    end)

  current_index = Faker.random_between(5, 15)
  {_earlier_stops, [current_stop | future_stops]} = stops |> Enum.split(current_index)

  vehicle =
    Factories.Vehicles.Vehicle.build(:vehicle,
      status: Faker.Util.pick([:incoming, :in_transit]),
      stop_id: current_stop.id,
      stop_sequence: current_index,
      trip_id: trip.id
    )

  trip_details =
    TripDetails.trip_details(%{
      predicted_schedules: predicted_schedules,
      trip_vehicle: vehicle
    })

  assert trip_details.stops |> Enum.map(& &1.stop_id) == [
           current_stop.id | Enum.map(future_stops, & &1.id)
         ]

  assert trip_details.stops |> Enum.map(& &1.stop_name) == [
           current_stop.name | Enum.map(future_stops, & &1.name)
         ]
end

☝️ would do the trick, I think

[joshlarson]

Style Nit / Question: Why trip_vehicle rather than just vehicle?

[joshlarson]

Suggestion / Style Nit: Admittedly, this is largely aesthetics on my part, because I'm not the biggest fan of index arithmetic, but could this be accomplished using Enum.split_while/2? Something like 👇 ?

if Enum.any?(trip_stops, &vehicle_at_stop?(&1, vehicle_info)) do
  {_past_stops, upcoming_stops} =
    trip_stops
    |> Enum.split_while(&(!vehicle_at_stop?(&1, vehicle_info)))

  # Omit current trip_stop if the vehicle is stopped
  if vehicle_info.status == :stopped do
    Enum.drop(upcoming_stops, 1)
  else
    upcoming_stops
  end
else
  # finishing another trip -- show all the stops
  trip_stops
end