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.

Sign up for GitHub

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

Jump to bottom

Update to cancelled_trip group filter #289

Merged
merged 2 commits into from
Jul 11, 2023
Merged

Update to cancelled_trip group filter #289

merged 2 commits into from
Jul 11, 2023

Conversation

ErinLMoore
Copy link
Contributor

Summary of changes

Cancelled trip group filter now filters out trips where all stop updates have SKIPPED status

Asana Ticket: Add trip-level schedule_relationship for canceled bus trips

This change surfaces a trip-level schedule relationship of CANCELED for bus trips where all of the associated stop time updates have been given the SKIPPED status.

@ErinLMoore ErinLMoore self-assigned this Jun 30, 2023
@github-actions
Copy link

Coverage of commit eec4079

Summary coverage rate:
  lines......: 95.1% (1195 of 1256 lines)
  functions..: 84.2% (383 of 455 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/group_filter/cancelled_trip.ex                    |95.0%     20| 100%     3|    -      0

Download coverage report

arkadyan
arkadyan approved these changes Jul 3, 2023
View reviewed changes
Copy link

@arkadyan arkadyan left a comment

Choose a reason for hiding this comment

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

Added a small suggestion you can take or leave, but otherwise 👍 just on the code side. I think it would be good to get a ✅ from Beatrix, too, since she obviously knows this codebase much better than I do.

lib/concentrate/group_filter/cancelled_trip.ex Outdated
@@ -21,6 +21,9 @@ defmodule Concentrate.GroupFilter.CancelledTrip do
is_nil(time) ->
group

Enum.all?(stop_time_updates, &(StopTimeUpdate.schedule_relationship(&1) == :SKIPPED)) ->
Copy link

Choose a reason for hiding this comment

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

suggestion (non-blocking): To point out an additional option, you could also make a function called skipped? or something like that on StopTimeUpdate that does the == :SKIPPED check. Maybe more style preference than anything, but it would encapsulate knowledge about the schedule_relationship property and the :SKIPPED value.

paulswartz
paulswartz reviewed Jul 3, 2023
View reviewed changes
Copy link
Member

@paulswartz paulswartz left a comment

Choose a reason for hiding this comment

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

could you deploy this to one of the dev environments so we can check out how it works?

lib/concentrate/group_filter/cancelled_trip.ex Outdated
@@ -21,6 +21,9 @@ defmodule Concentrate.GroupFilter.CancelledTrip do
is_nil(time) ->
group

Enum.all?(stop_time_updates, &(StopTimeUpdate.schedule_relationship(&1) == :SKIPPED)) ->
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: can we move this to the end? Since it needs to iterate over all the stop time updates, we might be able to avoid the work if the trip or route is already cancelled.

@github-actions
Copy link

github-actions bot commented Jul 5, 2023

Coverage of commit e26d322

Summary coverage rate:
  lines......: 94.8% (1192 of 1257 lines)
  functions..: 84.0% (383 of 456 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/group_filter/cancelled_trip.ex                    |95.0%     20| 100%     3|    -      0
  lib/concentrate/stop_time_update.ex                               |90.9%     22|71.0%    31|    -      0

Download coverage report

@ErinLMoore ErinLMoore temporarily deployed to dev July 5, 2023 14:11 — with GitHub Actions Inactive
@ErinLMoore
Copy link
Contributor Author

could you deploy this to one of the dev environments so we can check out how it works?

I just deployed it on dev

bklebe
bklebe reviewed Jul 5, 2023
View reviewed changes
lib/concentrate/group_filter/cancelled_trip.ex Outdated
Comment on lines 30 to 31
Enum.all?(stop_time_updates, &StopTimeUpdate.skipped?(&1)) ->
cancel_group(group)
Copy link
Member

Choose a reason for hiding this comment

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

question: do you know if we should also cancel the group if the STUs are all skipped but the first one's time is nil like the case on line 21?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm actually not sure which rule should take precedence, and there doesn't seem to be a test around the case on line 21. I'll ask on the ticket and see if someone can clarify.

@github-actions
Copy link

github-actions bot commented Jul 7, 2023

Coverage of commit e9f37c9

Summary coverage rate:
  lines......: 95.1% (1196 of 1257 lines)
  functions..: 84.2% (384 of 456 functions)
  branches...: no data found

Files changed coverage rate:
                                                                    |Lines       |Functions  |Branches    
  Filename                                                          |Rate     Num|Rate    Num|Rate     Num
  ========================================================================================================
  lib/concentrate/group_filter/cancelled_trip.ex                    |95.0%     20| 100%     3|    -      0
  lib/concentrate/stop_time_update.ex                               |90.9%     22|71.0%    31|    -      0

Download coverage report

@ErinLMoore ErinLMoore temporarily deployed to dev July 7, 2023 12:44 — with GitHub Actions Inactive
@ErinLMoore ErinLMoore merged commit 95dd635 into master Jul 11, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paulswartz paulswartz paulswartz left review comments

@arkadyan arkadyan arkadyan approved these changes

@bklebe bklebe bklebe approved these changes

Assignees

@ErinLMoore ErinLMoore

Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@ErinLMoore @arkadyan @paulswartz @bklebe