Add support for trip booking rules as per GTFS-Flex v2.1 by vpavlushkov · Pull Request #4 · CGI-Move/OpenTripPlanner

vpavlushkov · 2020-11-27T09:05:36Z

Description

These code changes implement the steps listed in Asana: Add support for trip booking rules as per GTFS-Flex 2.1 as required to add support for booking rules as defined in GTFS-Flex v2.1:

onebusaway gtfs modules -> implemented in Support for GTFS-Flex v2.1 booking rules onebusaway-gtfs-modules#1
OTP internal model (with new BookingRule class)
OTP GTFS -> internal model mapper (add the rule in the trip timetables and FlexTrip times)
GraphQL schema + resolvers

Comments on routing response schema

The routing response schema as per https://dev.opentripplanner.org.s3.amazonaws.com/apidoc/1.5.0/json_Leg.html has only the DRT message fields, but booking rules from GTFS-Flex v2.1 provide phone numbers and URLs too. The following mapping has been made:

flexDrtPickupMessage reused for pickup message configured through booking rule (with fallback to message of that rule)
flexDrtDropOffMessage reused for drop-off message configured through booking rule (with fallback to message of that rule)
flexPhoneNumber NEW used for phone number configured through pickup/drop-off booking rule
flexInfoUrl NEW used for info URL configured through pickup/drop-off booking rule
flexBookingUrl NEW used for booking URL configured through pickup/drop-off booking rule

To be completed by pull request submitter:

issue: Link to or create an issue that describes the relevant feature or bug. Add GitHub keywords to this PR's description (e.g., closes #45).
roadmap: Check the roadmap for this feature or bug. If it is not already on the roadmap, PLC will discuss as part of the review process.
tests: Have you added relevant test coverage? Are all the tests passing on the continuous integration service (Travis CI)?
formatting: Have you followed the suggested code style?
documentation: If you are adding a new configuration option, have you added an explanation to the configuration documentation tables and sections?
changelog: add a bullet point to the changelog file with description and link to the linked issue

To be completed by @opentripplanner/plc:

reviews and approvals by 2 members, ideally from different organizations
after merging: update the relevant card on the roadmap

hannesj

Looks good so far. The next steps is to copy the BookingRules as fields to the following classes.

https://github.com/kyyticom/OpenTripPlanner/blob/master/src/ext/java/org/opentripplanner/ext/flex/trip/UnscheduledTrip.java#L168
https://github.com/kyyticom/OpenTripPlanner/blob/master/src/ext/java/org/opentripplanner/ext/flex/trip/ScheduledDeviatedTrip.java#L167
https://github.com/kyyticom/OpenTripPlanner/blob/master/src/ext/java/org/opentripplanner/ext/flex/trip/ContinuousPickupDropOffTrip.java#L248

After this, a separate utility should be created to check what is the earliest departure time per the booking rules. This can be then used in earliestDepartureTime and latestArrivalTime.

You can look at the example from OTP1.x in https://github.com/opentripplanner/OpenTripPlanner/blob/dev-1.x/src/main/java/org/opentripplanner/routing/trippattern/TripTimes.java#L322-L355

src/main/java/org/opentripplanner/gtfs/mapping/BookingRuleMapper.java

…top rules

src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java

vpavlushkov · 2020-12-02T14:14:33Z

src/main/java/org/opentripplanner/routing/trippattern/TripTimes.java

+                // Up to prior day(s) booking
+                int priorNoticeStartDay = pickupBookingRule.getPriorNoticeStartDay();
+                int priorNoticeStartTime = pickupBookingRule.getPriorNoticeStartTime();
+                return sd.time((int) Math.round((priorNoticeStartDay * 24.0 + priorNoticeStartTime) * 60.0));


This is something I am not sure if it produces a number of seconds from now (currTime) till service date in priorNoticeStartDay days + priorNoticeStartTime hours. 🤔 There are too many factors in play: service calendar, timezone, current time and the time mappers.

I guess the only way forward here is to have unit tests that cover different types of booking rules 🤔

The priorNoticeStartDay should probably operate on days, converting to ServiceDate and back, rather that 24 hour increments, to account for switching to an from DST.

…r upon initialization

…uting

vpavlushkov · 2020-12-21T14:52:26Z

In cc1ee2a, booking rule properties are added to routing mapping. Note that the time calculations are not done there.

Location of the GraphQL configs still remains a mystery. 🤔

vpavlushkov · 2020-12-22T09:24:45Z

Sketch of further development provided by @hannesj:

The schema is configured in schema.graphqls file, it is only about the data stricture of the output.
After you have changed it, there is a README.
You need to generate the new Java types, and implement them in here. It doesn’t compile after you’ve added the types, as the interfaces aren’t implemented.

hannesj · 2020-12-22T08:58:15Z

src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java

-        stopTimeMapper = new StopTimeMapper(stopMapper, locationMapper, locationGroupMapper, tripMapper);
+
+        agencyAndIdMapper = new AgencyAndIdMapper();
+        bookingRuleMapper = new BookingRuleMapper();


Initialisation can be moved together with declaration, as there are no parameters from the constructor used.

hannesj · 2020-12-22T08:59:19Z

src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java


    private final TripMapper tripMapper;

+    private final AgencyAndIdMapper agencyAndIdMapper;


This is not used anywhere, delete?

hannesj · 2020-12-22T08:59:46Z

src/main/java/org/opentripplanner/model/BookingRule.java

+/* This file is based on code copied from project OneBusAway, see the LICENSE file for further information. */
+package org.opentripplanner.model;
+
+public final class BookingRule {


This should extend TransitEntity

hannesj · 2020-12-22T09:00:12Z

src/main/java/org/opentripplanner/model/BookingRule.java

+        return id;
+    }
+
+    public void setId(FeedScopedId id) {


As other TransitEnetites, this should be set in the constructor

hannesj · 2020-12-22T09:00:24Z

src/main/java/org/opentripplanner/model/BookingRule.java

+
+public final class BookingRule {
+
+    private FeedScopedId id;


This should be final

I believe super(id) in the parent class does the trick. 🤔 At least that is the way FareAttribute class is set.

hannesj · 2020-12-22T09:36:43Z

src/main/java/org/opentripplanner/routing/trippattern/TripTimes.java

+                // Up to prior day(s) booking
+                int priorNoticeStartDay = pickupBookingRule.getPriorNoticeStartDay();
+                int priorNoticeStartTime = pickupBookingRule.getPriorNoticeStartTime();
+                return sd.time((int) Math.round((priorNoticeStartDay * 24.0 + priorNoticeStartTime) * 60.0));


The priorNoticeStartDay should probably operate on days, converting to ServiceDate and back, rather that 24 hour increments, to account for switching to an from DST.

hannesj · 2020-12-22T09:41:09Z

src/main/java/org/opentripplanner/routing/trippattern/TripTimes.java

+     * @param sd - service day
+     * @return time
+     */
+    public long getLatestDropOffBookingTime (int stop, long currTime, ServiceDay sd) {


Does this make sense. The booking rules, are always calculated from the boarding time, so the above method would bee needed to check that a trip was valid.

vpavlushkov · 2020-12-22T13:36:34Z

@hannesj Requested changes are applied in 88c87a1

1cdaa9a also adds the fields to the GraphQL schema, but:

The four String fields are added to Leg schema as Strings
I see no reason of adding new Java types as the fields in question are just Strings and we only query them as a part of routing query; i.e. they needed only to be inside legs. Am I missing something? 🤔
Generating GraphQL resources per README fails for me, but I cannot figure out the reason yet

* ~OBA lib started to fail building in CircleCI but works perfectly fine in my local~ -> **UPDATE** Issue with OBA lib building was a temporal glitch that got auto-resolved after retry in CircleCI

hannesj · 2021-02-04T22:06:43Z

I'll take a look at this

chore(otp-model): add new StopTime fields and BookingRule model

442be4f

vpavlushkov added the enhancement New feature or request label Nov 27, 2020

vpavlushkov self-assigned this Nov 27, 2020

vpavlushkov changed the title ~~chore(otp-model): add new StopTime fields and BookingRule model~~ Add support for trip booking rules as per GTFS-Flex v2.1 Nov 27, 2020

Victor Pavlushkov added 5 commits November 27, 2020 16:49

chore(gtfs-mapping): add initial version of BookingRuleMapper

2ab71c9

fix(gtfs-mapping): add missing bracket

7f37872

fix(gtfs-mapping): add missing imports

b515ec1

chore(gtfs-mapping): add extra mappers to signatures

c373380

fix(gtfs-mapping): remove extra comma

8e14c31

hannesj suggested changes Nov 29, 2020

View reviewed changes

src/main/java/org/opentripplanner/gtfs/mapping/BookingRuleMapper.java Outdated Show resolved Hide resolved

Victor Pavlushkov added 3 commits November 30, 2020 16:20

chore(gtfs-mapping): try fixing GTFSToOtp mapper

0abbe38

chore(ext-flex): add booking rule fields into trips

53afadf

chore(routing): add get methods for earliest and latest booking per s…

9b6ffc6

…top rules

vpavlushkov commented Dec 2, 2020

View reviewed changes

src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java Show resolved Hide resolved

vpavlushkov commented Dec 2, 2020

View reviewed changes

Victor Pavlushkov added 3 commits December 14, 2020 13:58

Merge branch 'master' into flex2-booking-rules

4dc6290

chore(mapping): update BookingRuleMapper not to call AgencyAndIdMappe…

a57682f

…r upon initialization

chore(routing): add messages, phone and URLs from booking rules to ro…

cc1ee2a

…uting

hannesj suggested changes Dec 22, 2020

View reviewed changes

Victor Pavlushkov added 3 commits December 22, 2020 14:23

chore(graphql): update schema with new leg fields

1cdaa9a

fix(graphql): remove unused import slipped in by a typo

4e288cd

chore(gtfs): apply changes requested 22.12.2020

88c87a1


		private final TripMapper tripMapper;

		private final AgencyAndIdMapper agencyAndIdMapper;

Conversation

vpavlushkov commented Nov 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Comments on routing response schema

Uh oh!

hannesj left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpavlushkov commented Dec 21, 2020

Uh oh!

vpavlushkov commented Dec 22, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vpavlushkov commented Dec 22, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hannesj commented Feb 4, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

vpavlushkov commented Nov 27, 2020 •

edited

Loading

vpavlushkov commented Dec 22, 2020 •

edited

Loading