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

Filter flex trips by agency and route #6421

Open
wants to merge 26 commits into
base: dev-2.x
Choose a base branch
from

Conversation

leonardehrenfried
Copy link
Member

Summary

It introduces filtering of flex trips by agency and route.

To achieve this, the regular filter request is mapped to a flex-specific version of it and then the filter logic is implemented that way.

I'm not 100% sure I got the layers of abstraction correct. I would appreciate a design or pair programming session about this.

Issue

Closes #5463

Unit tests

Lots added

Documentation

Javadoc

@leonardehrenfried leonardehrenfried added Improvement A functional improvement Sandbox labels Feb 3, 2025
@leonardehrenfried leonardehrenfried requested a review from a team as a code owner February 3, 2025 14:23
Copy link

codecov bot commented Feb 3, 2025

Codecov Report

Attention: Patch coverage is 73.38710% with 33 lines in your changes missing coverage. Please review.

Project coverage is 70.19%. Comparing base (d048f93) to head (0ba6f08).
Report is 2 commits behind head on dev-2.x.

Files with missing lines Patch % Lines
...entripplanner/transit/api/request/TripRequest.java 34.61% 9 Missing and 8 partials ⚠️
...pentripplanner/transit/api/model/FilterValues.java 16.66% 2 Missing and 3 partials ⚠️
...anner/apis/transmodel/TransmodelGraphQLSchema.java 0.00% 4 Missing ⚠️
.../opentripplanner/ext/flex/filter/FilterMapper.java 94.44% 1 Missing and 1 partial ⚠️
.../java/org/opentripplanner/ext/flex/FlexRouter.java 80.00% 1 Missing ⚠️
...opentripplanner/ext/flex/template/ClosestTrip.java 50.00% 0 Missing and 1 partial ⚠️
...ansit/api/model/FilterValuesEmptyIsEverything.java 0.00% 0 Missing and 1 partial ⚠️
.../transit/api/model/FilterValuesEmptyIsNothing.java 75.00% 0 Missing and 1 partial ⚠️
...ner/transit/model/filter/expr/NegationMatcher.java 83.33% 1 Missing ⚠️
Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #6421      +/-   ##
=============================================
+ Coverage      70.18%   70.19%   +0.01%     
- Complexity     18308    18348      +40     
=============================================
  Files           2082     2085       +3     
  Lines          77188    77287      +99     
  Branches        7828     7844      +16     
=============================================
+ Hits           54175    54255      +80     
- Misses         20247    20253       +6     
- Partials        2766     2779      +13     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@optionsome optionsome requested a review from eibakke February 4, 2025 10:22
@leonardehrenfried leonardehrenfried force-pushed the flex-filtering branch 5 times, most recently from 63fdbe2 to 03f20e6 Compare February 28, 2025 09:27
@leonardehrenfried
Copy link
Member Author

@eibakke This is now ready for review.

public TripRequestBuilder withAgencies(FilterValues<FeedScopedId> agencies) {
this.agencies = agencies;
public TripRequestBuilder withIncludedAgencies(Collection<FeedScopedId> agencies) {
this.includedAgencies = FilterValues.ofEmptyIsNothing("includedAgencies", agencies);
Copy link
Member Author

Choose a reason for hiding this comment

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

@eibakke This is a different behaviour to what happens in the TransmodelAPI. Let's discuss this in the meeting.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah I see now. We'll have to fix the Transmodel API to handle empty and unset arguments better. Right now it is not working as intended.

@optionsome optionsome requested a review from t2gran March 6, 2025 13:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Improvement A functional improvement Sandbox
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Flex routing ignores bannedAgencies and bannedRoutes
2 participants