fix(orca-fares): remove fare attributes with no rules #6089
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## dev-2.x #6089 +/- ##
=============================================
+ Coverage 69.82% 69.84% +0.02%
- Complexity 17419 17462 +43
=============================================
Files 1974 1974
Lines 74545 74606 +61
Branches 7633 7640 +7
=============================================
+ Hits 52048 52107 +59
- Misses 19847 19850 +3
+ Partials 2650 2649 -1 ☔ View full report in Codecov by Sentry. |
| @@ -40,6 +40,15 @@ public Set<RouteOriginDestination> getRouteOriginDestinations() { | |||
| return routeOriginDestinations; | |||
| } | |||
|
|
|||
| public boolean hasRules() { | |||
There was a problem hiding this comment.
This one needs Javadoc and a test.
There was a problem hiding this comment.
Good point, I added tests for this and matches.
There was a problem hiding this comment.
That's a very nice test but where is the Javadoc?
There was a problem hiding this comment.
Oops
leonardehrenfried
added
Sandbox
IBI
daniel-heppner-ibigroup
force-pushed
the
orca-remove-ruleless-attributes
branch
2 times, most recently
from
f96de66 to
7cfb806
Compare
daniel-heppner-ibigroup
force-pushed
the
orca-remove-ruleless-attributes
branch
from
7cfb806 to
feedefe
Compare
Comment on lines
138
to
139
| new HashSet<>(), | ||
| new HashSet<>(), |
There was a problem hiding this comment.
Suggested change
| new HashSet<>(), | |
| new HashSet<>(), | |
| Set.of(), | |
| Set.of() |
I find this visually a bit more pleasing. Not a big issue though.
daniel-heppner-ibigroup
force-pushed
the
orca-remove-ruleless-attributes
branch
from
023a39b to
364cf4f
Compare
daniel-heppner-ibigroup
force-pushed
the
orca-remove-ruleless-attributes
branch
from
66dbc18 to
54223b1
Compare
leonardehrenfried
approved these changes
Sep 27, 2024
|
Only sandbox code. Merging with a single review. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
ORCA agencies have occasionally left in fare attributes with no associated rules. OTP will match these against any leg, and if the fare is lower than the correct fare, it will take priority. There does not seem to be a well defined behavior for this edge case in the GTFS spec, but to avoid changing the behavior of OTP in general, I have added a one liner to remove these fare attributes lacking rules when using the ORCA fare module.
Unit tests
I'm not sure how to test this or if it's necessary, since this change happened in the ORCA fare factory which is untested.