-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add support for trip booking rules as per GTFS-Flex v2.1 #4
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
Show resolved
Hide resolved
src/main/java/org/opentripplanner/gtfs/mapping/GTFSToOtpTransitServiceMapper.java
Show resolved
Hide resolved
// 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess the only way forward here is to have unit tests that cover different types of booking rules 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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. 🤔 |
Sketch of further development provided by @hannesj:
|
stopTimeMapper = new StopTimeMapper(stopMapper, locationMapper, locationGroupMapper, tripMapper); | ||
|
||
agencyAndIdMapper = new AgencyAndIdMapper(); | ||
bookingRuleMapper = new BookingRuleMapper(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initialisation can be moved together with declaration, as there are no parameters from the constructor used.
@@ -56,6 +56,10 @@ | |||
|
|||
private final TripMapper tripMapper; | |||
|
|||
private final AgencyAndIdMapper agencyAndIdMapper; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not used anywhere, delete?
/* 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should extend TransitEntity
return id; | ||
} | ||
|
||
public void setId(FeedScopedId id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As other TransitEnetites, this should be set in the constructor
|
||
public final class BookingRule { | ||
|
||
private FeedScopedId id; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be final
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe super(id)
in the parent class does the trick. 🤔 At least that is the way FareAttribute
class is set.
// 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)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* @param sd - service day | ||
* @return time | ||
*/ | ||
public long getLatestDropOffBookingTime (int stop, long currTime, ServiceDay sd) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@hannesj Requested changes are applied in 88c87a1 1cdaa9a also adds the fields to the GraphQL schema, but:
|
I'll take a look at this |
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:
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 ruleflexInfoUrl
NEW used for info URL configured through pickup/drop-off booking ruleflexBookingUrl
NEW used for booking URL configured through pickup/drop-off booking ruleTo be completed by pull request submitter:
closes #45
).To be completed by @opentripplanner/plc: