Post deviated trip notification #260

binh-dam-ibigroup · 2024-10-09T17:26:15Z

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
[na] The description lists all applicable issues this PR seeks to resolve
[na] The description lists any configuration setting(s) that differ from the default settings
All tests and CI builds passing

Description

This PR sets up a job to send post-trip surveys for users who took trips the day before.
The same notification endpoint will be used for both trip monitoring notifications and trip survey notifications post travel.

Blockers:

~~Need a modified or new push notifications endpoint for trip surveys.~~

…or trip surveys.

…sers to notify.

…age.

br648

A few comments, but pretty solid so far.

br648 · 2024-10-10T12:52:57Z

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

+    /**
+     * Get users whose last trip survey notification was at least a week ago.
+     */
+    public static List<OtpUser> getUsersWithNotificationsOverAWeekAgo() {


Is or should there be a option to opt-out of receiving surveys?

No opt-out of post-travel surveys at the moment.

br648 · 2024-10-10T12:57:30Z

src/main/java/org/opentripplanner/middleware/OtpMiddlewareMain.java

@@ -84,6 +85,16 @@ public static void main(String[] args) throws IOException, InterruptedException
                1,
                TimeUnit.MINUTES
            );
+
+            // Schedule recurring job for post-trip surveys, once every few hours
+            // TODO: Determine whether this should go in some other process.


I think the trigger location is fine. Perhaps follow the approach of ConnectedDataManager.scheduleTripHistoryUploadJob(); and have the schduler in the class. It might also be benefical to have the ability to disable this via a config property.

@binh-dam-ibigroup thoughts on these two points?

We'd have to rethink where to run periodic tasks.

src/main/java/org/opentripplanner/middleware/OtpMiddlewareMain.java

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

src/test/java/org/opentripplanner/middleware/triptracker/TripSurveySenderJobTest.java

…hold to select trips.

… trip completion.

… delete FTL template.

br648

This looks good to me. Some minor points to consider, but approving.

br648 · 2024-10-30T12:49:47Z

src/main/java/org/opentripplanner/middleware/OtpMiddlewareMain.java

@@ -84,6 +85,16 @@ public static void main(String[] args) throws IOException, InterruptedException
                1,
                TimeUnit.MINUTES
            );
+
+            // Schedule recurring job for post-trip surveys, once every few hours
+            // TODO: Determine whether this should go in some other process.


@binh-dam-ibigroup thoughts on these two points?

br648 · 2024-10-30T12:56:35Z

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

@@ -91,4 +98,40 @@ public void updateNotificationMessage(String routeId, String body) {
            busNotificationMessages
        );
    }
+
+    /** The sum of the deviations for all tracking locations that have it. */
+    public double computeTotalDeviation() {


This method has no usages. Remove?

br648 · 2024-10-30T12:59:00Z

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

@@ -152,6 +152,8 @@ private static EndTrackingResponse completeJourney(TripTrackingData tripData, bo
        trackedJourney.end(isForciblyEnded);
        Persistence.trackedJourneys.updateField(trackedJourney.id, TrackedJourney.END_TIME_FIELD_NAME, trackedJourney.endTime);
        Persistence.trackedJourneys.updateField(trackedJourney.id, TrackedJourney.END_CONDITION_FIELD_NAME, trackedJourney.endCondition);
+        trackedJourney.longestConsecutiveDeviatedPoints = trackedJourney.computeLargestConsecutiveDeviations();
+        Persistence.trackedJourneys.updateField(trackedJourney.id, TrackedJourney.LONGEST_CONSECUTIVE_DEVIATED_POINTS_FIELD_NAME, trackedJourney.longestConsecutiveDeviatedPoints);


Nit. Wrap this line.

br648 · 2024-10-30T13:13:08Z

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

+        Locale locale = I18nUtils.getOtpUserLocale(otpUser);
+        String tripTime = DateTimeUtils.formatShortDate(trip.itinerary.startTime, locale);
+        String body = String.format(
+            org.opentripplanner.middleware.i18n.Message.TRIP_SURVEY_NOTIFICATION.get(locale),


Nit. Consider a static import.

br648 · 2024-10-30T13:23:17Z

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

+                location.tripStatus = d == null
+                    ? null
+                    : d == 1
+                    ? TripStatus.DEVIATED
+                    : TripStatus.ON_SCHEDULE;


src/test/java/org/opentripplanner/middleware/triptracker/TripSurveySenderJobTest.java

JymDyerIBI

👍 This looks good.

…ious notifications=.

binh-dam-ibigroup · 2024-11-06T02:17:41Z

@br648 and @JymDyerIBI I added a notificationId to the push message, in addition to updates to the survey ids/subdomains.

JymDyerIBI

👍 Changes look good.

binh-dam-ibigroup added 12 commits October 4, 2024 10:34

feat(TrackedJourney): Add method to compute total deviation.

9fc6822

feat(TrackedJourney): Persist total deviation upon completing journey.

6507d5a

.feat(OtpUser): Add field for last survey notif sent.

44c456e

refactor(ApiController): Extract const for mongo id field.

9e19b1e

refactor(MonitoredTrip): Extract const for trip id field.

462639b

feat(TripSurveySenderJob): Add basic logic for trip survey job.

1f31842

refactor(TripSurveySenderJob): Convert methods to static.

2e2473f

test(TripSurveySenderJob): Reuse journeys.

2432090

feat(TripSurveySenderJob): Send push notification using current svc f…

c00ac92

…or trip surveys.

feat(OtpMiddlewareMain): Schedule trip survey job.

ca2b8aa

docs(swagger): Update snapshot.

20fdb33

fix(TripSurveySenderJob): Include missing last trip survey field in u…

ab5a82f

…sers to notify.

binh-dam-ibigroup added the BLOCKED Blocked (waiting on another PR to be merged) label Oct 9, 2024

refactor(TripSurveySenderJob): Remove day of week in survey push mess…

828afdc

…age.

binh-dam-ibigroup requested a review from br648 October 10, 2024 12:36

binh-dam-ibigroup assigned br648 Oct 10, 2024

br648 reviewed Oct 10, 2024

View reviewed changes

binh-dam-ibigroup added 4 commits October 10, 2024 14:25

docs(OtpMiddlewareMain): Fix trip survey comment typo.

3ed298b

refactor(TrackedJourney): Introduce consecutive deviation metric.

fa6b838

refactor(TripSurveySenderJob): Add a minimum 1-minute deviation thres…

72ecf80

…hold to select trips.

Merge branch 'dev' into post-deviated-trip-notification

e46abca

br648 assigned binh-dam-ibigroup and unassigned br648 Oct 15, 2024

binh-dam-ibigroup added 7 commits October 21, 2024 16:04

Merge branch 'dev' into post-deviated-trip-notification

25d19f9

refactor(TrackedJourney): Remove cumulative deviation field.

be37335

Merge branch 'dev' into post-deviated-trip-notification

653b62c

fix(TripSurveySenderJob): Send survey notifications within 30 mins of…

eb59c11

… trip completion.

docs: Introduce new TRIP_SURVEY_ID config parameter.

cd2b05a

refactor(Message): Extract i18n message for trip survey notification,…

b27a4dd

… delete FTL template.

refactor(TripSurveySenderJob): Streamline notification for trip surveys.

4cd55b0

binh-dam-ibigroup marked this pull request as ready for review October 29, 2024 20:58

refactor(TripSurveySenderJob): Tweak comments.

edb9392

binh-dam-ibigroup requested a review from br648 October 29, 2024 21:14

binh-dam-ibigroup assigned br648 and unassigned binh-dam-ibigroup Oct 29, 2024

binh-dam-ibigroup removed the BLOCKED Blocked (waiting on another PR to be merged) label Oct 29, 2024

br648 approved these changes Oct 30, 2024

View reviewed changes

br648 assigned binh-dam-ibigroup and unassigned br648 Oct 30, 2024

binh-dam-ibigroup added 3 commits October 30, 2024 13:19

refactor(TrackedJourney): Remove unused method.

4755ba8

style(ManageTripTracking):Wrap line

0a8a81b

style(NotificationUtils): Use static import

2065188

binh-dam-ibigroup requested a review from JymDyerIBI October 31, 2024 18:57

binh-dam-ibigroup assigned JymDyerIBI and unassigned binh-dam-ibigroup Oct 31, 2024

binh-dam-ibigroup added 2 commits November 1, 2024 16:23

docs: Introduce TRIP_SURVEY_SUBDOMAIN config param

de1d9f4

feat(NotificationUtils): Include survey subdomain to push notification

5bfe653

JymDyerIBI approved these changes Nov 4, 2024

View reviewed changes

binh-dam-ibigroup added 4 commits November 5, 2024 16:18

refactor(OtpUser): Use list of sent notifications for traceability.

e8256da

refactor(TripSurveySenderJob): Support instances where users had prev…

e096f84

…ious notifications=.

refactor(TripSurveySenderJob): Improve Mongo filters.

402383f

refactor(TripSurveySenderJob): Include notification id.

986d5ce

binh-dam-ibigroup added 4 commits November 6, 2024 08:06

Merge branch 'dev' into post-deviated-trip-notification

8dba681

refactor(OtpUser): Fix imports

9f2ab38

docs(Swagger): Update snapshots

bb99963

Merge branch 'dev' into post-deviated-trip-notification

6128725

JymDyerIBI approved these changes Nov 6, 2024

View reviewed changes

binh-dam-ibigroup merged commit 120d920 into dev Nov 13, 2024
2 checks passed

binh-dam-ibigroup deleted the post-deviated-trip-notification branch November 13, 2024 16:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Post deviated trip notification #260

Post deviated trip notification #260

binh-dam-ibigroup commented Oct 9, 2024 •

edited

Loading

br648 left a comment

br648 Oct 10, 2024

binh-dam-ibigroup Oct 25, 2024

br648 Oct 10, 2024

br648 Oct 30, 2024

binh-dam-ibigroup Oct 31, 2024

br648 left a comment

br648 Oct 30, 2024

br648 Oct 30, 2024

br648 Oct 30, 2024

br648 Oct 30, 2024

br648 Oct 30, 2024

JymDyerIBI left a comment

binh-dam-ibigroup commented Nov 6, 2024

JymDyerIBI left a comment

Post deviated trip notification #260

Post deviated trip notification #260

Conversation

binh-dam-ibigroup commented Oct 9, 2024 • edited Loading

Checklist

Description

br648 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

br648 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

JymDyerIBI left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup commented Nov 6, 2024

JymDyerIBI left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup commented Oct 9, 2024 •

edited

Loading