diff --git a/src/main/java/org/opentripplanner/middleware/controllers/api/TripSurveyController.java b/src/main/java/org/opentripplanner/middleware/controllers/api/TripSurveyController.java index 3fd4dd72..4e416e97 100644 --- a/src/main/java/org/opentripplanner/middleware/controllers/api/TripSurveyController.java +++ b/src/main/java/org/opentripplanner/middleware/controllers/api/TripSurveyController.java @@ -73,12 +73,12 @@ private static void returnInvalidUrlParametersError(Request request) { } /** - * Mark notification as opened. + * Mark notification as opened, but if an opened date is already populated, do nothing. */ - private static void updateNotificationState(OtpUser user, String notificationId) { - Optional notificationOpt = user.findNotification(notificationId); - if (notificationOpt.isPresent()) { - notificationOpt.get().timeOpened = Date.from(Instant.now()); + private static void updateNotificationStateIfNeeded(OtpUser user, String notificationId) { + TripSurveyNotification notification = user.findNotification(notificationId).orElse(null); + if (notification != null && notification.timeOpened == null) { + notification.timeOpened = Date.from(Instant.now()); Persistence.otpUsers.replace(user.id, user); } } @@ -112,7 +112,7 @@ private static boolean processCall(Request req, Response res) { ); // Update notification state - updateNotificationState(user, notificationId); + updateNotificationStateIfNeeded(user, notificationId); // Redirect res.redirect(surveyUrl); diff --git a/src/test/java/org/opentripplanner/middleware/controllers/api/TripSurveyControllerTest.java b/src/test/java/org/opentripplanner/middleware/controllers/api/TripSurveyControllerTest.java index 28199a23..31a963c5 100644 --- a/src/test/java/org/opentripplanner/middleware/controllers/api/TripSurveyControllerTest.java +++ b/src/test/java/org/opentripplanner/middleware/controllers/api/TripSurveyControllerTest.java @@ -17,6 +17,7 @@ import org.opentripplanner.middleware.testutils.ApiTestUtils; import org.opentripplanner.middleware.testutils.OtpMiddlewareTestEnvironment; import org.opentripplanner.middleware.testutils.PersistenceTestUtils; +import org.opentripplanner.middleware.utils.HttpResponseValues; import java.time.Instant; import java.util.Date; @@ -33,6 +34,7 @@ import static org.opentripplanner.middleware.testutils.ApiTestUtils.makeRequest; class TripSurveyControllerTest extends OtpMiddlewareTestEnvironment { + public static final String SURVEY_PATH_TEMPLATE = "api/trip-survey/open?user_id=%s&trip_id=%s¬ification_id=%s"; private static OtpUser otpUser; private static MonitoredTrip monitoredTrip; private static TrackedJourney trackedJourney; @@ -91,22 +93,10 @@ void canOpenSurveyAndUpdateNotificationStatus() { OtpUser existingUser = Persistence.otpUsers.getById(otpUser.id); assertNotNull(existingUser); - existingUser.tripSurveyNotifications.forEach(n -> { - assertNull(n.timeOpened); - }); + existingUser.tripSurveyNotifications.forEach(n -> assertNull(n.timeOpened)); Instant requestInstant = Instant.now(); - var response = makeRequest( - String.format( - "api/trip-survey/open?user_id=%s&trip_id=%s¬ification_id=%s", - otpUser.id, - monitoredTrip.id, - NOTIFICATION_ID - ), - "", - Map.of(), - HttpMethod.GET - ); + var response = invokeSurveyEndpoint(otpUser.id, monitoredTrip.id, NOTIFICATION_ID); Instant requestCompleteInstant = Instant.now(); assertEquals(HttpStatus.OK_200, response.status); @@ -116,17 +106,32 @@ void canOpenSurveyAndUpdateNotificationStatus() { assertEquals(otpUser.tripSurveyNotifications.size(), updatedUser.tripSurveyNotifications.size()); int updatedNotificationCount = 0; + Date notificationTimeOpened = null; for (TripSurveyNotification notification : updatedUser.tripSurveyNotifications) { if (NOTIFICATION_ID.equals(notification.id)) { - assertNotNull(notification.timeOpened); - assertTrue(notification.timeOpened.toInstant().isAfter(requestInstant)); - assertTrue(notification.timeOpened.toInstant().isBefore(requestCompleteInstant)); + notificationTimeOpened = notification.timeOpened; + assertNotNull(notificationTimeOpened); + assertTrue(notificationTimeOpened.toInstant().isAfter(requestInstant)); + assertTrue(notificationTimeOpened.toInstant().isBefore(requestCompleteInstant)); updatedNotificationCount++; } else { assertNull(notification.timeOpened); } } assertEquals(1, updatedNotificationCount); + assertNotNull(notificationTimeOpened); + + // A second request should not change the notification opened date. + var response2 = invokeSurveyEndpoint(otpUser.id, monitoredTrip.id, NOTIFICATION_ID); + assertEquals(HttpStatus.OK_200, response2.status); + + updatedUser = Persistence.otpUsers.getById(otpUser.id); + assertNotNull(updatedUser); + for (TripSurveyNotification notification : updatedUser.tripSurveyNotifications) { + if (NOTIFICATION_ID.equals(notification.id)) { + assertEquals(notificationTimeOpened, notification.timeOpened); + } + } } @ParameterizedTest @@ -134,17 +139,7 @@ void canOpenSurveyAndUpdateNotificationStatus() { void shouldRejectInvalidParams(String userId, String tripId, String notificationId) { assumeTrue(IS_END_TO_END); - var response = makeRequest( - String.format( - "api/trip-survey/open?user_id=%s&trip_id=%s¬ification_id=%s", - userId, - tripId, - notificationId - ), - "", - Map.of(), - HttpMethod.GET - ); + var response = invokeSurveyEndpoint(userId, tripId, notificationId); assertEquals( HttpStatus.BAD_REQUEST_400, @@ -163,4 +158,14 @@ private static Stream createShouldRejectInvalidParamsCases() { Arguments.of(otpUser.id, monitoredTrip.id, null) ); } + + /** Helper to invoke the survey endpoint */ + private static HttpResponseValues invokeSurveyEndpoint(String userId, String tripId, String notificationId) { + return makeRequest( + String.format(SURVEY_PATH_TEMPLATE, userId, tripId, notificationId), + "", + Map.of(), + HttpMethod.GET + ); + } }