Consider feed id in HSL fare service #5284

nurmAV · 2023-08-10T10:31:07Z

Summary

Previously, the HSL fare service did not consider which feed fare rules and attributes are from. This led to tickets from one feed to be sometimes applicable for legs originating from other feeds with no fare attributes or rules. This PR adds the feed consideration into the HSL fare service.

Unit tests

A unit test was added in HSLFareServiceTest. Known erratic ticket suggestions were also manually checked

codecov · 2023-08-10T10:42:27Z

Codecov Report

Patch coverage: 93.44% and project coverage change: +0.08% 🎉

Comparison is base (7136d18) 66.37% compared to head (a9caa84) 66.46%.
Report is 41 commits behind head on dev-2.x.

Additional details and impacted files

@@              Coverage Diff              @@
##             dev-2.x    #5284      +/-   ##
=============================================
+ Coverage      66.37%   66.46%   +0.08%     
- Complexity     15184    15225      +41     
=============================================
  Files           1787     1786       -1     
  Lines          69263    69290      +27     
  Branches        7339     7318      -21     
=============================================
+ Hits           45974    46054      +80     
+ Misses         20804    20752      -52     
+ Partials        2485     2484       -1

Files Changed	Coverage Δ
...tripplanner/ext/fares/impl/HSLFareServiceImpl.java	`68.26% <81.81%> (+4.43%)`	⬆️
...tripplanner/ext/fares/impl/DefaultFareService.java	`91.57% <96.00%> (-1.04%)`	⬇️

... and 29 files with indirect coverage changes

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

src/ext/java/org/opentripplanner/ext/fares/impl/HSLFareServiceImpl.java

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

optionsome

There is still an issue that wrong fare is suggested in some cases. I tried searching for a trip from somewhere in Helsinki to Tampere and it suggested two HSL fares even though you would need a Tampere fare instead of the other HSL fare. You need HSL, tampere and digitraffic data to test it, maybe there is a simpler way to replicate the problem with unit tests.

…com/OpenTripPlanner into DT-5946-missing-ticket-disclaimer

optionsome

@leonardehrenfried mentioned in the dev meeting that he would prefer to have the updated calculateFares code in the DefaultFareServiceImpl instead of duplicating most of the code to the HSLFareServiceImpl. Probably then we should also add some tests to the default service.

optionsome · 2023-08-18T08:11:11Z

src/ext/java/org/opentripplanner/ext/fares/impl/HSLFareServiceImpl.java

+    var legsByFeed = itinerary
+      .getLegs()
+      .stream()
+      .filter(leg -> leg instanceof ScheduledTransitLeg)


I think we might need to consider FlexibleTransitLeg here as well.

This was handled by moving the multifeed logic into the DefaultFareService that already considers FlexibleTransitLegs

leonardehrenfried · 2023-08-18T09:02:00Z

@leonardehrenfried mentioned in the dev meeting that he would prefer to have the updated calculateFares code in the DefaultFareServiceImpl instead of duplicating most of the code to the HSLFareServiceImpl. Probably then we should also add some tests to the default service.

Indeed. It's going to be a maintenance nightmare if we copy and paste the code.

There is a small number of tests for the DefaultFareService but there are also a lot of hidden assumptions which we can document as we find them.

Luckily there are lots and lots of tests for city specific calculators and also some integration tests so all in all I'm pretty confident of not breaking anything by accident.

A long term goal for HSL could be to move to Fares V2 where the code isn't quite so bogged down with legacy issues.

leonardehrenfried · 2023-09-11T12:52:56Z

src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareService.java

+      if (legWithoutRulesFound) {
+        fare.addFare(
+          fareType,
+          Money.ofFractionalAmount(fares.get(0).currency(), Float.POSITIVE_INFINITY)


This is quite dubious. You're adding an infinite fare when you have a leg without rules?

Yes, it looks a bit funny but that seems to be how the fare is set for similar single feed cases in the performSearch method. I could of course change that to directly give the -0.01 that seems to be the current way to indicate that some legs do not have fares. Or do you maybe have some other ideas on how this should be handled?

Sure, but we are not in the performSearch method. The positive infinity is directly returned to the client.

If we want -1 cent then we need to return it here.

Also, we need lots and lots of tests that document what exactly the expected return values are since it's very confusing.

Ok, I'll change that infinity to the -0.01. I added two test cases for the new multifeed logic. Is there some pre-existing functionality that I should add tests for in this PR?

There is lots of functionality that we should test but knowledge about what the expectations are has been lost over time. The best we can do is to write tests when we add new features or find bugs.

Ok, I added some tests cases for the new multifeed logic

leonardehrenfried · 2023-09-12T13:02:09Z

src/ext-test/java/org/opentripplanner/ext/fares/impl/DefaultFareServiceTest.java

+      .map(r -> r.fareId())
+      .toList();
+
+    assertEquals(


Can you please add assertions for the expected prices like is done here?

OpenTripPlanner/src/ext-test/java/org/opentripplanner/ext/fares/impl/DefaultFareServiceTest.java

Lines 75 to 100 in bf90bcf

@Test

void unknownLeg() {

var service = new DefaultFareService();

service.addFareRules(FareType.regular, List.of(AIRPORT_TO_CITY_CENTER_SET));

var itin = newItinerary(Place.forStop(AIRPORT_STOP), T11_00)

.bus(1, T11_00, T11_12, Place.forStop(CITY_CENTER_A_STOP))

.bus(3, T11_20, T11_33, Place.forStop(SUBURB_STOP))

.build();

var fare = service.calculateFares(itin);

assertNotNull(fare);

var price = fare.getFare(FareType.regular);

assertEquals(Money.usDollars(-0.01f), price);

var components = fare.getComponents(FareType.regular);

assertEquals(1, components.size());

var component = components.get(0);

assertEquals(AIRPORT_TO_CITY_CENTER_SET.getFareAttribute().getId(), component.fareId());

assertEquals(Money.usDollars(10), component.price());

var firstBusLeg = itin.firstTransitLeg().get();

assertEquals(List.of(firstBusLeg), component.legs());

}

Added those

leonardehrenfried

This is ok but rather than trying to fix the very flawed concept of the FareComponent I would encourage HSL to migrate to the new API that replaces FareComponent with FareProduct.

https://docs.opentripplanner.org/api/dev-2.x/graphql-gtfs/types/FareProductUse

It also assigns the fares directly to the legs which makes displaying them in the UI easier.

src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareService.java

src/ext/java/org/opentripplanner/ext/fares/impl/HSLFareServiceImpl.java

leonardehrenfried · 2023-09-14T09:32:54Z

Could you also please merge ~~master~~ dev-2.x so I can get an accurate view of the coverage change?

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

optionsome · 2023-09-14T09:38:47Z

Could you also please merge master so I can get an accurate view of the coverage change?

By master you mean dev-2.x right?

leonardehrenfried · 2023-09-14T09:42:40Z

Yes of course.

leonardehrenfried · 2023-09-14T12:49:39Z

src/ext/java/org/opentripplanner/ext/fares/impl/DefaultFareService.java

+      if (legWithoutRulesFound || legsWithoutMatchingRulesFound) {
+        fare.addFare(
+          fareType,
+          Money.ofFractionalAmount(fares.get(0).currency(), UNKNOWN_FARE_PRICE)


Actually, I'm reconsidering this. If no fare could be found, should we simply skip this? Does your UI expect there to be a -1 cent?

Yes, it specifically checks if the price is -1 cent and if it is, it warns the user of missing fares. Otherwise no warning is shown.

Alright then.

My point about moving to the more expressive API still stands.

Consider feed id in HSL fare service

1828c89

nurmAV requested a review from a team as a code owner August 10, 2023 10:31

leonardehrenfried reviewed Aug 10, 2023

View reviewed changes

src/ext/java/org/opentripplanner/ext/fares/impl/HSLFareServiceImpl.java Outdated Show resolved Hide resolved

Apply feedback from review

9c07374

Co-authored-by: Leonard Ehrenfried <mail@leonard.io>

leonardehrenfried added Sandbox Skip Changelog labels Aug 10, 2023

leonardehrenfried previously approved these changes Aug 10, 2023

View reviewed changes

optionsome requested changes Aug 10, 2023

View reviewed changes

t2gran added this to the 2.4 (next release) milestone Aug 15, 2023

viljaminurminen-cgi added 2 commits August 17, 2023 15:40

Consider each feed separately in HSL fare service

9cb7e8d

Merge branch 'DT-5946-missing-ticket-disclaimer' of github.com:HSLdev…

45a864a

…com/OpenTripPlanner into DT-5946-missing-ticket-disclaimer

nurmAV dismissed leonardehrenfried’s stale review via 45a864a August 17, 2023 12:42

leonardehrenfried requested review from leonardehrenfried and optionsome August 17, 2023 13:55

optionsome reviewed Aug 18, 2023

View reviewed changes

viljaminurminen-cgi added 2 commits September 7, 2023 08:09

Add multifeed handling to DefaultFareService

d303045

Remove calculateFares from HSL fare service

3cd90af

leonardehrenfried reviewed Sep 11, 2023

View reviewed changes

Add multifeed tests for DefaultFareService

bf15416

leonardehrenfried reviewed Sep 12, 2023

View reviewed changes

t2gran modified the milestones: 2.4, 2.5 (next release) Sep 13, 2023

viljaminurminen-cgi added 2 commits September 14, 2023 09:58

Consider unknown fares within feeds

c441d8b

Return null fares only if no fare type has fare

36d93cd

leonardehrenfried previously approved these changes Sep 14, 2023

View reviewed changes

optionsome reviewed Sep 14, 2023

View reviewed changes

Apply suggestions from code review

34bd978

Co-authored-by: Joel Lappalainen <lappalj8@gmail.com>

nurmAV dismissed leonardehrenfried’s stale review via 34bd978 September 14, 2023 09:34

Merge branch 'otp-dev-2.x' into DT-5946-missing-ticket-disclaimer

c488177

Fix formatting

a9caa84

optionsome approved these changes Sep 14, 2023

View reviewed changes

leonardehrenfried reviewed Sep 14, 2023

View reviewed changes

leonardehrenfried approved these changes Sep 14, 2023

View reviewed changes

optionsome merged commit 260c581 into opentripplanner:dev-2.x Sep 14, 2023

optionsome deleted the DT-5946-missing-ticket-disclaimer branch September 14, 2023 13:25

leonardehrenfried mentioned this pull request Oct 5, 2023

Fix IBI fare calculators #5397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider feed id in HSL fare service #5284

Consider feed id in HSL fare service #5284

nurmAV commented Aug 10, 2023

codecov bot commented Aug 10, 2023 •

edited

Loading

optionsome left a comment •

edited

Loading

optionsome left a comment

optionsome Aug 18, 2023

nurmAV Sep 12, 2023

leonardehrenfried commented Aug 18, 2023

leonardehrenfried Sep 11, 2023

nurmAV Sep 12, 2023

leonardehrenfried Sep 12, 2023

nurmAV Sep 12, 2023

leonardehrenfried Sep 12, 2023

nurmAV Sep 14, 2023

leonardehrenfried Sep 12, 2023

nurmAV Sep 14, 2023

leonardehrenfried left a comment

leonardehrenfried commented Sep 14, 2023 •

edited

Loading

optionsome commented Sep 14, 2023

leonardehrenfried commented Sep 14, 2023

leonardehrenfried Sep 14, 2023

nurmAV Sep 14, 2023 •

edited

Loading

leonardehrenfried Sep 14, 2023

	@Test
	void unknownLeg() {
	var service = new DefaultFareService();
	service.addFareRules(FareType.regular, List.of(AIRPORT_TO_CITY_CENTER_SET));

	var itin = newItinerary(Place.forStop(AIRPORT_STOP), T11_00)
	.bus(1, T11_00, T11_12, Place.forStop(CITY_CENTER_A_STOP))
	.bus(3, T11_20, T11_33, Place.forStop(SUBURB_STOP))
	.build();

	var fare = service.calculateFares(itin);
	assertNotNull(fare);

	var price = fare.getFare(FareType.regular);
	assertEquals(Money.usDollars(-0.01f), price);

	var components = fare.getComponents(FareType.regular);
	assertEquals(1, components.size());

	var component = components.get(0);
	assertEquals(AIRPORT_TO_CITY_CENTER_SET.getFareAttribute().getId(), component.fareId());
	assertEquals(Money.usDollars(10), component.price());

	var firstBusLeg = itin.firstTransitLeg().get();
	assertEquals(List.of(firstBusLeg), component.legs());
	}

Consider feed id in HSL fare service #5284

Consider feed id in HSL fare service #5284

Conversation

nurmAV commented Aug 10, 2023

Summary

Unit tests

codecov bot commented Aug 10, 2023 • edited Loading

Codecov Report

optionsome left a comment • edited Loading

Choose a reason for hiding this comment

optionsome left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leonardehrenfried left a comment

Choose a reason for hiding this comment

leonardehrenfried commented Sep 14, 2023 • edited Loading

optionsome commented Sep 14, 2023

leonardehrenfried commented Sep 14, 2023

Choose a reason for hiding this comment

nurmAV Sep 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Aug 10, 2023 •

edited

Loading

optionsome left a comment •

edited

Loading

leonardehrenfried commented Sep 14, 2023 •

edited

Loading

nurmAV Sep 14, 2023 •

edited

Loading