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

update cancelled trips group filter to only cancel a trip if all stus are skipped for bus/route_type 3 #359

Merged
merged 5 commits into from
Sep 25, 2024

Conversation

bfauble
Copy link
Contributor

@bfauble bfauble commented Sep 16, 2024

Summary of changes

Asana Ticket: 🐛 [Investigate] Subway passthrough trips in Concentrate have schedule_relationship CANCELED

  • store route type for trips along with route_id and direction_id
  • check route_type before applying :CANCELED status to trip when all stop time updates are :SKIPPED

… are marked as skipped for bus, route_type 3
@bfauble bfauble requested review from a team and Whoops and removed request for a team September 16, 2024 23:53
@Whoops
Copy link
Collaborator

Whoops commented Sep 17, 2024

Checking my understanding, passthrough trips are nonrevenue trips where a train will pass through a station but won't pick up passengers, right? And they are currently being represented by a trip with all skipped stops?

So the idea here is that bus does not have these passthrough trips, so if we have all skipped stops it's a block waiver and we should cancel them, but subway it could be a passthrough?

I'm wondering if this is a safe assumption (that this logic should only every apply to bus, not Subway). Would it be possible to check the revenue status of the trip instead, or can we not rely on it being marked nonrevenue?

@bfauble
Copy link
Contributor Author

bfauble commented Sep 17, 2024

So the idea here is that bus does not have these passthrough trips, so if we have all skipped stops it's a block waiver and we should cancel them, but subway it could be a passthrough?

This is my understanding of the intent of the original change to add the cancellation of the block based on all skipped stus which was inadvertently also catching rail trips from time to time.

@Whoops
Copy link
Collaborator

Whoops commented Sep 18, 2024

Not seeing any glaring issues in dev-green, I think this is good to go.

@Whoops
Copy link
Collaborator

Whoops commented Sep 18, 2024

I re-enabled CI (thanks Github for auto-disabling, SUPER helpful). I don't seem to be able to kick it into reruning for this though, I think you might have to re-push?

@Whoops
Copy link
Collaborator

Whoops commented Sep 18, 2024

Even better CI is blocked because they have decided we need to use a newer version of upload-artifact. Fix is in this PR. I'm stepping away for an hour or so, if you want feel free to merge that after approving to unblock this PR.

Copy link
Collaborator

@Whoops Whoops left a comment

Choose a reason for hiding this comment

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

I think the code in this version is easier to follow than the original, and it's looking good after running in dev blue for awhile 👍

@bfauble bfauble merged commit 981d822 into master Sep 25, 2024
16 checks passed
@bfauble bfauble deleted the bf-all-skipped-stus-bus-only branch September 25, 2024 14:31
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