OTP-1382 GMAP Push notifications for leg transition #275

br648 · 2024-12-03T11:56:08Z

Checklist

Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
Any modified or new methods or classes have helpful JavaDoc and code is thoroughly commented
The description lists all applicable issues this PR seeks to resolve
The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

Check for upcoming leg transitions (arrive, departure & mode change) and notify observers.

…hanges):

binh-dam-ibigroup

Could you clarify the intent behind origin/destination change? I think what we want to do is to notify observers that someone has departed or arrived on their trip.

src/main/java/org/opentripplanner/middleware/i18n/Message.java

src/main/java/org/opentripplanner/middleware/models/TripMonitorNotification.java

src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java

br648 · 2024-12-05T14:15:27Z

Could you clarify the intent behind origin/destination change? I think what we want to do is to notify observers that someone has departed or arrived on their trip.

Oh, really! I thought I was to check the OTP response for any changes to the traveler's trip and notify them of this. Could I have a couple of use cases just to clarify what is expected please?

binh-dam-ibigroup · 2024-12-05T14:24:16Z

Oh, really! I thought I was to check the OTP response for any changes to the traveler's trip and notify them of this. Could I have a couple of use cases just to clarify what is expected please?

It is simpler than you think it is. Basically, a notification when the traveler leaves the origin or arrives at destination, and when the traveler changes modes (arrives at a bus stop, gets off the bus, gets on/off the bike if there is a bike involved).

binh-dam-ibigroup · 2024-12-05T14:32:46Z

Basically, a notification when the traveler leaves the origin

Notifications are for observers who are not traveling with the traveler and their companion.

binh-dam-ibigroup

Could you make these changes please:

convert the departure/arrival change notification into leaving the origin/arriving at destination notifications, and send the notification when the traveler gets to the corresponding location.
execute mode change notification when approaching the end of a leg where the next leg has a different mode.

src/main/java/org/opentripplanner/middleware/triptracker/TravelerLocator.java

…tion types):

binh-dam-ibigroup

I think the logic is getting there. I don't think we need to identify/process a list of notification types (one should be fine). Notifications are for the observers and non-trip-initiators, and there are language tweaks.

src/main/java/org/opentripplanner/middleware/models/LegTransitionNotification.java

src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java

src/main/java/org/opentripplanner/middleware/models/LegTransitionNotification.java

src/main/java/org/opentripplanner/middleware/utils/ItineraryUtils.java

src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java

src/main/java/org/opentripplanner/middleware/triptracker/TravelerLocator.java

src/test/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTripTest.java

src/main/java/org/opentripplanner/middleware/triptracker/TravelerLocator.java

…wn class):

…ss):

binh-dam-ibigroup · 2024-12-13T15:00:03Z

src/main/java/org/opentripplanner/middleware/i18n/Message.java

@@ -19,10 +19,10 @@ public enum Message {
    ACCEPT_DEPENDENT_EMAIL_SUBJECT,
    ACCEPT_DEPENDENT_EMAIL_MANAGE,
    ACCEPT_DEPENDENT_ERROR,
+    ARRIVED_AND_MODE_CHANGE_NOTIFICATION,


I'd say just keep the old name MODE_CHANGE_NOTIFICATION.

binh-dam-ibigroup

Could you address duplicate notifications still being sent, please?

src/main/java/org/opentripplanner/middleware/models/LegTransitionNotification.java

binh-dam-ibigroup · 2024-12-13T15:12:26Z

src/main/java/org/opentripplanner/middleware/models/LegTransitionNotification.java

    }

    /**
-     * Create locale specific notifications.
+     * If a traveler is on schedule and on either a walk or transit leg check for possible leg transition notification.


I think you meant to say "If a traveler is on the route (not deviated)"...

Insert comma before "check".

binh-dam-ibigroup · 2024-12-13T15:14:00Z

src/main/java/org/opentripplanner/middleware/models/LegTransitionNotification.java

-}
+
+    /**
+     * Depending on the traveler's proximity to the start/end of a leg return the appropriate notification type.


Insert comma before "return".

binh-dam-ibigroup · 2024-12-13T15:30:47Z

src/test/java/org/opentripplanner/middleware/models/LegTransitionNotificationTest.java

+
+    private static Stream<Arguments> createLegTransitionNotificationTestCases() throws Exception {
+        String travelerName = "Obi-Wan";
+        Locale locale = Locale.US;


Maybe remove locale as a test parameter (set that in the test itself).

binh-dam-ibigroup · 2024-12-13T16:39:35Z

src/main/java/org/opentripplanner/middleware/tripmonitor/jobs/CheckMonitoredTrip.java

@@ -559,6 +592,9 @@ private void sendNotifications() {
        // TODO: better handle below when one of the following fails
        if (successEmail || successPush || successSms) {
            notificationTimestampMillis = DateTimeUtils.currentTimeMillis();
+            // Prevent repeated notifications by saving successfully sent notifications.


This check is at the wrong spot, I see duplicate notifications being sent and recorded (see screenshot)

…updating monitored trip):

binh-dam-ibigroup · 2024-12-17T17:36:53Z

src/main/java/org/opentripplanner/middleware/models/LegTransitionNotification.java

+        if (isAtStartOfLeg(travelerPosition)) {
+            return DEPARTED_NOTIFICATION;


Nitpick: Change the condition so that departing notifications are sent at the beginning of the trip only.

I found one case where I set two coordinates, one close to the end of a leg, and the next one close to the beginning of the next leg, and I got two companion notifications, one for arriving at the end of the previous leg, and one for departing the next leg, which is redundant with the previous one and is thus unneeded noise. What do you think?

I think for consistency we leave it as is. The observer would get a notification that the traveler has arrived but not one to tell them they have departed. The next notification would be the traveler arriving at the end of the next leg. Correct?

That's what I observed.

binh-dam-ibigroup

Overall, the changes are working and code looks good. The nice-to-have tweak is not blocking.

br648 · 2024-12-18T07:57:28Z

src/main/resources/Message_fr.properties

@@ -4,7 +4,10 @@ ACCEPT_DEPENDENT_EMAIL_LINK_TEXT = Accepter la demande
 ACCEPT_DEPENDENT_EMAIL_SUBJECT = Demande d'accompagnateur
 ACCEPT_DEPENDENT_EMAIL_MANAGE = G�rez vos pr�f�rences
 ACCEPT_DEPENDENT_ERROR = La demande d'accompagnateur n'a pas �t� re�ue.
+ARRIVED_NOTIFICATION = TODO %s.


@binh-dam-ibigroup can you provide the french messages please!

Oui ! 36b26af

…b.com/ibi-group/otp-middleware into feature/OTP-1382-notify-major-changes

br648 added 5 commits December 3, 2024 11:54

refactor(Initial work to check for major trip changes):

7ba414d

refactor(Updated swagger docs and included the current leg on major c…

7c466fc

…hanges):

Merge branch 'dev' into feature/OTP-1382-notify-major-changes

21d9407

Merge branch 'dev' into feature/OTP-1382-notify-major-changes

ee451f6

refactor(Added unit tests and made changes to wording.):

f9126c4

br648 assigned binh-dam-ibigroup Dec 4, 2024

br648 changed the title ~~Check trip for major changes~~ OTP-1382 Check trip for major changes Dec 5, 2024

binh-dam-ibigroup reviewed Dec 5, 2024

View reviewed changes

binh-dam-ibigroup requested changes Dec 5, 2024

View reviewed changes

src/main/java/org/opentripplanner/middleware/triptracker/TravelerLocator.java Outdated Show resolved Hide resolved

binh-dam-ibigroup assigned br648 and unassigned binh-dam-ibigroup Dec 5, 2024

br648 added 5 commits December 6, 2024 08:07

Merge branch 'dev' into feature/OTP-1382-notify-major-changes

ad42160

refactor(Various updates to monitor leg transition):

75f44c8

refactor(Addressed unused params and methods. Updated Swagger docs.):

926f994

refactor(Fixed merge conflicts):

128fb32

refactor(Refactor to create and process leg tranistion based notifica…

91d6619

…tion types):

br648 changed the title ~~OTP-1382 Check trip for major changes~~ OTP-1382 GMAP Push notifications for leg transition Dec 10, 2024

br648 assigned binh-dam-ibigroup and unassigned br648 Dec 10, 2024

binh-dam-ibigroup requested changes Dec 11, 2024

View reviewed changes

binh-dam-ibigroup assigned br648 and unassigned binh-dam-ibigroup Dec 11, 2024

br648 added 5 commits December 13, 2024 10:18

refactor(Addressed PR feedback.):

c5582e1

refactor(Fixed merge conflicts and moved leg trans notify test into o…

2638f04

…wn class):

refactor(Refactored otp user to provide display name):

17363ac

refactor(Moved various methods into the LegTransitionNotification cla…

bc20b84

…ss):

refactor(Created builder to handle basic traveler position needs):

4e788c5

br648 assigned ibi-group-team and binh-dam-ibigroup and unassigned br648 and ibi-group-team Dec 13, 2024

binh-dam-ibigroup reviewed Dec 13, 2024

View reviewed changes

binh-dam-ibigroup requested changes Dec 13, 2024

View reviewed changes

binh-dam-ibigroup assigned br648 and unassigned binh-dam-ibigroup Dec 13, 2024

br648 added 3 commits December 16, 2024 17:44

refactor(Addressed PR feedback):

22ed611

refactor(Update to guard against missing target zoned date time when …

5159804

…updating monitored trip):

refactor(CheckMonitoredTrip): Update to preserve matching itinerary

c05e011

br648 assigned binh-dam-ibigroup and JymDyerIBI and unassigned br648 and binh-dam-ibigroup Dec 16, 2024

binh-dam-ibigroup reviewed Dec 17, 2024

View reviewed changes

binh-dam-ibigroup approved these changes Dec 17, 2024

View reviewed changes

br648 commented Dec 18, 2024

View reviewed changes

br648 and others added 5 commits December 18, 2024 08:01

refactor(Fixed merge conflicts):

8340c43

improvement(hidden getDisplayName from Mongo and JSON serialisation):

07ad8d8

chore(i18n): Add French text for new strings.

36b26af

Merge branch 'dev' into feature/OTP-1382-notify-major-changes

4366df5

Merge branch 'feature/OTP-1382-notify-major-changes' of https://githu…

b3ab1fd

…b.com/ibi-group/otp-middleware into feature/OTP-1382-notify-major-changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTP-1382 GMAP Push notifications for leg transition #275

OTP-1382 GMAP Push notifications for leg transition #275

br648 commented Dec 3, 2024 •

edited

Loading

binh-dam-ibigroup left a comment

br648 commented Dec 5, 2024

binh-dam-ibigroup commented Dec 5, 2024

binh-dam-ibigroup commented Dec 5, 2024

binh-dam-ibigroup left a comment

binh-dam-ibigroup left a comment

binh-dam-ibigroup Dec 13, 2024

binh-dam-ibigroup left a comment

binh-dam-ibigroup Dec 13, 2024

binh-dam-ibigroup Dec 13, 2024

binh-dam-ibigroup Dec 13, 2024

binh-dam-ibigroup Dec 13, 2024

binh-dam-ibigroup Dec 13, 2024

binh-dam-ibigroup Dec 17, 2024

br648 Dec 18, 2024

binh-dam-ibigroup Dec 18, 2024

binh-dam-ibigroup left a comment

br648 Dec 18, 2024

binh-dam-ibigroup Dec 18, 2024

		if (isAtStartOfLeg(travelerPosition)) {
		return DEPARTED_NOTIFICATION;

OTP-1382 GMAP Push notifications for leg transition #275

Are you sure you want to change the base?

OTP-1382 GMAP Push notifications for leg transition #275

Conversation

br648 commented Dec 3, 2024 • edited Loading

Checklist

Description

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

br648 commented Dec 5, 2024

binh-dam-ibigroup commented Dec 5, 2024

binh-dam-ibigroup commented Dec 5, 2024

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binh-dam-ibigroup left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

binh-dam-ibigroup left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

br648 commented Dec 3, 2024 •

edited

Loading