Skip to content
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

Last Trip of the Day phase 1 #746

Merged
merged 23 commits into from
May 8, 2024
Merged

Last Trip of the Day phase 1 #746

merged 23 commits into from
May 8, 2024

Conversation

PaulJKim
Copy link
Collaborator

Summary of changes

Asana Ticket: Read in the "last train" information from transit data feed and display "end of service" information

This PR adds new state for tracking recent departures within the last hour as well as any trips that were marked as a last trip within the last hour.

It also adds logic for showing and playing visual content and audio indicating that service has ended for the night. The logic varies based on whether the source is for a single direction or two directions (platform vs mezzanine, for the most part). The logic is outlined in the Asana task.

Copy link

Coverage of commit e6a01d6

Summary coverage rate:
  lines......: 71.6% (1907 of 2662 lines)
  functions..: 74.2% (531 of 716 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio/service_ended.ex                         |33.3%     24|50.0%     6|    -      0
  lib/content/message/last_trip/no_service.ex                |75.0%      4|50.0%     4|    -      0
  lib/content/message/last_trip/platform_closed.ex           |50.0%      2|50.0%     4|    -      0
  lib/content/message/last_trip/service_ended.ex             |50.0%      2|50.0%     4|    -      0
  lib/content/message/last_trip/station_closed.ex            |50.0%      2|50.0%     4|    -      0
  lib/engine/last_trip.ex                                    |58.1%     62|71.4%    14|    -      0
  lib/realtime_signs.ex                                      | 100%     13| 100%     3|    -      0
  lib/signs/realtime.ex                                      |90.4%     73|92.3%    13|    -      0
  lib/signs/utilities/audio.ex                               |87.6%    121| 100%    15|    -      0
  lib/signs/utilities/messages.ex                            |79.8%     89| 100%     9|    -      0

Download coverage report

Copy link

Coverage of commit 9984b40

Summary coverage rate:
  lines......: 71.6% (1906 of 2662 lines)
  functions..: 74.2% (531 of 716 functions)
  branches...: no data found

Files changed coverage rate:
                                                             |Lines       |Functions  |Branches    
  Filename                                                   |Rate     Num|Rate    Num|Rate     Num
  =================================================================================================
  lib/content/audio/service_ended.ex                         |33.3%     24|50.0%     6|    -      0
  lib/content/message/last_trip/no_service.ex                |75.0%      4|50.0%     4|    -      0
  lib/content/message/last_trip/platform_closed.ex           |50.0%      2|50.0%     4|    -      0
  lib/content/message/last_trip/service_ended.ex             |50.0%      2|50.0%     4|    -      0
  lib/content/message/last_trip/station_closed.ex            |50.0%      2|50.0%     4|    -      0
  lib/engine/last_trip.ex                                    |58.1%     62|71.4%    14|    -      0
  lib/realtime_signs.ex                                      | 100%     13| 100%     3|    -      0
  lib/signs/realtime.ex                                      |90.4%     73|92.3%    13|    -      0
  lib/signs/utilities/audio.ex                               |87.6%    121| 100%    15|    -      0
  lib/signs/utilities/messages.ex                            |79.8%     89| 100%     9|    -      0

Download coverage report

lib/engine/last_trip.ex Outdated Show resolved Hide resolved
lib/engine/last_trip.ex Outdated Show resolved Hide resolved
lib/engine/last_trip.ex Outdated Show resolved Hide resolved
lib/engine/last_trip.ex Outdated Show resolved Hide resolved
lib/engine/last_trip.ex Outdated Show resolved Hide resolved
lib/engine/last_trip.ex Outdated Show resolved Hide resolved
@@ -196,6 +211,25 @@ defmodule Signs.Realtime do
{:noreply, state}
end

defp has_service_ended_for_source?(sign, source, current_time) do
SourceConfig.sign_stop_ids(source)
|> Enum.all?(&has_last_trip_departed_stop?(&1, sign, current_time))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is all? the right logic here? For the signs with multiple stops, I'd assume that trains would stop at one of them, not pass through them all.

Also, how are multiple routes (e.g. GL branches) handled? Do they each get their own last train?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah that's a good call out, it would be just one stop id and not all. And good question about the multiple branches too, I'll ask the transit data team about that one.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So about routes, I'm not sure if it's something we need/want to handle right now as GL last trips are not going to be supported until last trips are a feature in Glides. It does sound like once it's implemented though, that it will be a last trip for every route, which means that we would probably need to key recent departures on stop_id + route, instead of just stop_id. It's not much more effort to include the route right now though so we can also just go ahead and do it to prepare for the future.

The routes discussion brings up an interesting case though. Since the red line has branching, there will be two last trips in both directions, but there aren't GTFS route_ids for the two red line branches so somehow the code will need to know to wait for two last trips at RL trunk stops, and just one for RL branch stops..

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, interesting. I guess we'll need a special case there, since I don't think there's semantic info we could leverage at those stops.

lib/signs/utilities/messages.ex Outdated Show resolved Hide resolved
lib/signs/utilities/messages.ex Outdated Show resolved Hide resolved

has_top_service_ended? ->
if get_message_length(bottom_message) <= 18 do
{bottom_message,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works fine for typical mezzanine modes, but it doesn't handle cases where the lines no longer correspond 1-1 with the source configs, like combined headways, combined alerts, or JFK special paging. I think we'll need to take a similar approach to the early AM logic, and unpack those cases so we can handle them more holistically.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think at this point in the logic, the only case where the lines don't correspond 1-1 anymore is the JFK Mezzanine case where we have a NB platform prediction alongside SB headways. We could account for that specific case and unpack them into a regular prediction struct and paging headway struct respectively. I think at that point, the rest of the logic here would work. What are your thoughts on that or do you see a bug in that logic that I might've missed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One case I ran into was if a mezzanine sign comes up with no predictions, it can display a two-line combined headway message, which can then get sliced in half if one direction is LTOD. I think a similar thing can happen with alerts, though I didn't check that definitively.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah right the headways is one that doesn't work. I wonder if the alerts case is something we can just pass along though considering that if in any case a MZ sign is showing a two-lined alert message, I believe that means service is definitely not running at that station and saying "No service use shuttle bus" or just simply "No service" essentially means the same thing as something like "Station closed service has ended for the night". Might be something to run by a PM/Designer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spoke with Dan and we decided that for two-line combined alerts, we should show LTOTD message once service has fully ended. Otherwise, just show the alert.

Copy link
Collaborator

@panentheos panentheos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good. All the comments below are small things, and the only one that affects visible behavior is the one about not suppressing predictions in the platform case.

I spot-checked a few cases locally, but did not do extensive local testing.

lib/signs/realtime.ex Outdated Show resolved Hide resolved
lib/engine/last_trip.ex Outdated Show resolved Hide resolved
lib/signs/realtime.ex Outdated Show resolved Hide resolved
lib/signs/realtime.ex Outdated Show resolved Hide resolved
lib/engine/last_trip.ex Outdated Show resolved Hide resolved
lib/predictions/last_trip.ex Outdated Show resolved Hide resolved
lib/signs/utilities/last_trip.ex Outdated Show resolved Hide resolved
lib/signs/utilities/last_trip.ex Outdated Show resolved Hide resolved
lib/engine/last_trip.ex Outdated Show resolved Hide resolved
lib/content/audio/service_ended.ex Outdated Show resolved Hide resolved
@PaulJKim PaulJKim merged commit 27bd645 into main May 8, 2024
3 checks passed
@PaulJKim PaulJKim deleted the pk/ltotd-phase-1 branch May 8, 2024 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants