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

Support Fares v2 FareMedium and update spec implementation #5227

Merged
merged 4 commits into from Aug 16, 2023

Conversation

EmmaSimon
Copy link
Contributor

@EmmaSimon EmmaSimon commented Jul 7, 2023

Summary

This updates OTP to support the changes made in OneBusAway/onebusaway-gtfs-modules#221, it isn't complete and only makes the necessary changes to get OTP to compile and build successfully using the onebusaway-gtfs-modules changes.

Issue

Related to OneBusAway/onebusaway-gtfs-modules#220, still need to create an issue in this repo for the changes to OTP

Unit tests

  • Tests were only updated to support the type changes, no further verification was done

@EmmaSimon EmmaSimon requested a review from a team as a code owner July 7, 2023 15:53
@leonardehrenfried leonardehrenfried changed the title WIP - Support latest Fares v2 spec Support latest Fares v2 spec Jul 10, 2023
@leonardehrenfried leonardehrenfried marked this pull request as draft July 10, 2023 07:40
@leonardehrenfried
Copy link
Member

Since I merged #5217 you will have to resolve the conflicts but I will leave a few comments nevertheless.

@t2gran t2gran added this to the 2.4 (next release) milestone Jul 18, 2023
@leonardehrenfried
Copy link
Member

OBA 1.4.4 has been released. I think you can pick this up again.

@EmmaSimon EmmaSimon marked this pull request as ready for review August 8, 2023 20:13
@EmmaSimon
Copy link
Contributor Author

@leonardehrenfried This just includes your own fares v2 relevant changes from ibi-group@1352ddf
I haven't tested this with the fares v2 sandbox toggle enabled, but it compiles and tests pass, and standard functionality seems to be working properly.
We're getting close to being ready to cutover, so it'll be helpful for us to merge this in soon.

@codecov
Copy link

codecov bot commented Aug 8, 2023

Codecov Report

Patch coverage: 81.81% and project coverage change: +0.01% 🎉

Comparison is base (24ce710) 65.92% compared to head (6ad19c7) 65.93%.
Report is 15 commits behind head on dev-2.x.

Additional details and impacted files
@@              Coverage Diff              @@
##             dev-2.x    #5227      +/-   ##
=============================================
+ Coverage      65.92%   65.93%   +0.01%     
- Complexity     14804    14811       +7     
=============================================
  Files           1770     1771       +1     
  Lines          68702    68727      +25     
  Branches        7283     7284       +1     
=============================================
+ Hits           45289    45314      +25     
+ Misses         20930    20927       -3     
- Partials        2483     2486       +3     
Files Changed Coverage Δ
.../opentripplanner/gtfs/graphbuilder/GtfsModule.java 69.47% <ø> (ø)
...org/opentripplanner/model/fare/ItineraryFares.java 86.95% <0.00%> (-3.96%) ⬇️
...opentripplanner/model/fare/FareProductBuilder.java 66.66% <66.66%> (ø)
...g/opentripplanner/ext/fares/model/FareLegRule.java 80.00% <80.00%> (-20.00%) ⬇️
...pentripplanner/gtfs/mapping/FareLegRuleMapper.java 89.74% <83.33%> (+9.74%) ⬆️
...ipplanner/gtfs/mapping/FareTransferRuleMapper.java 90.47% <85.71%> (-0.44%) ⬇️
...entripplanner/ext/fares/impl/GtfsFaresService.java 84.21% <100.00%> (ø)
...tripplanner/ext/fares/impl/GtfsFaresV2Service.java 96.74% <100.00%> (-0.03%) ⬇️
...ripplanner/ext/fares/model/FareLegRuleBuilder.java 100.00% <100.00%> (ø)
...ntripplanner/ext/fares/model/FareTransferRule.java 100.00% <100.00%> (ø)
... and 3 more

... and 4 files with indirect coverage changes

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

@leonardehrenfried
Copy link
Member

My Flex PR has not been merged at OBA so lets just get this through review and I will take care of the Flex stuff.

Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

I have a few requests.

This follows the same format as FareLegRuleBuilder, for the easier
creation of new FareProducts in tests.
Add some new test cases for FareLegRules, and update empty/null
FareProduct checks in a couple places.
Copy link
Member

@leonardehrenfried leonardehrenfried left a comment

Choose a reason for hiding this comment

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

This looks great - thanks!

BTW, if you want to assert that something throws an exception, you can use Assertions.assertThrows.

@leonardehrenfried leonardehrenfried changed the title Support latest Fares v2 spec Support Fares v2 FareMedium and update spec implementation Aug 10, 2023
@leonardehrenfried
Copy link
Member

Our regular meetings will start again next week where I will hopefully be able to find a second reviewer.

@leonardehrenfried
Copy link
Member

@vesameskanen I've taken the liberty to assign you to review. I hope this is ok.

@t2gran
Copy link
Member

t2gran commented Aug 15, 2023

I looked through the otp main code - looks ok. I will leave the official review to others.

@leonardehrenfried
Copy link
Member

@EmmaSimon I believe you don't have the permission to merge so I will do it.

@leonardehrenfried leonardehrenfried merged commit 7b2f980 into opentripplanner:dev-2.x Aug 16, 2023
5 checks passed
t2gran pushed a commit that referenced this pull request Aug 16, 2023
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.

None yet

4 participants