-
Notifications
You must be signed in to change notification settings - Fork 2
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
Trip sharing #271
Trip sharing #271
Conversation
This is to accommodate the dependent traveler profile. BREAKING CHANGE: Fields from RelatedUser in MonitoredTrip.primary are no longer supported.
…ared trips, add test.
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.
LGTM!
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.
Just one comment related to the relationship between related users and monitored trips. Also, I was expecting a new end point, but I guess that wasn't needed in the end?
@@ -176,7 +176,7 @@ public class MonitoredTrip extends Model { | |||
*/ | |||
public boolean notifyAtLeadingInterval = true; | |||
|
|||
public RelatedUser primary; | |||
public MobilityProfileLite primary; | |||
public RelatedUser companion; | |||
public List<RelatedUser> observers = new ArrayList<>(); |
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.
Probably out of scope, but should any updates to related users be reflected in monitored trips? For instance, if a related user is removed?
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.
@binh-dam-ibigroup just this to potentially address if you think it is in scope/important?
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.
@binh-dam-ibigroup just this to potentially address if you think it is in scope/important?
@binh-dam-ibigroup I just need your thoughts on this and then I can approve if it is out of scope.
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.
@br648 If you are ok with this, we can do delete propagation in another PR.
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.
@binh-dam-ibigroup I think it is out of scope. Will approve.
src/main/java/org/opentripplanner/middleware/controllers/api/MonitoredTripController.java
Show resolved
Hide resolved
Indeed! |
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.
Approving but delete propagation needs to be covered in another PR.
Checklist
dev
before they can be merged tomaster
)Description
Provides a new endpoint to query a given user's monitored trips, including trips on which a user is a companion and or an observer. The field type for
primary
user is changed toMobilityProfileLite
as a result from introducing the endpoint in #266 for querying the dependent profile.Out of scope: email notifications to users added as companions/observers/primary travelers will be addressed in a separate PR.