Skip to content

Commit

Permalink
refactor(TripSurveyController): Handle invalid survey URL parameters.
Browse files Browse the repository at this point in the history
  • Loading branch information
@binh-dam-ibigroup
binh-dam-ibigroup committed Dec 12, 2024
1 parent 81bb4c4 commit 31c77b7
Show file tree
Hide file tree
Showing 3 changed files with 93 additions and 23 deletions.
36 changes: 26 additions & 10 deletions src/main/java/org/opentripplanner/middleware/controllers/api/TripSurveyController.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import io.github.manusant.ss.SparkSwagger;
import io.github.manusant.ss.rest.Endpoint;
import org.eclipse.jetty.http.HttpStatus;
import org.opentripplanner.middleware.models.MonitoredTrip;
import org.opentripplanner.middleware.models.OtpUser;
import org.opentripplanner.middleware.models.TripSurveyNotification;
import org.opentripplanner.middleware.persistence.Persistence;
Expand All @@ -12,9 +14,11 @@

import java.time.Instant;
import java.util.Date;
import java.util.Optional;

import static io.github.manusant.ss.descriptor.EndpointDescriptor.endpointPath;
import static io.github.manusant.ss.descriptor.MethodDescriptor.path;
import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt;
import static org.opentripplanner.middleware.utils.NotificationUtils.TRIP_SURVEY_ID;
import static org.opentripplanner.middleware.utils.NotificationUtils.TRIP_SURVEY_SUBDOMAIN;

Expand Down Expand Up @@ -49,22 +53,34 @@ public void bind(final SparkSwagger restApi) {
/**
* Check that the requested survey is valid (user, trip, and notifications point to existing data).
*/
private static OtpUser checkParameters(String userId, String tripId, String notificationId, Response res) {
// TODO
return Persistence.otpUsers.getById(userId);
private static OtpUser checkParameters(String userId, String tripId, String notificationId, Request request) {
OtpUser user = Persistence.otpUsers.getById(userId);
if (user == null) {
returnInvalidUrlParametersError(request);
} else {
Optional<TripSurveyNotification> notificationOpt = user.findNotification(notificationId);
if (notificationOpt.isEmpty()) returnInvalidUrlParametersError(request);

MonitoredTrip trip = Persistence.monitoredTrips.getById(tripId);
if (trip == null) returnInvalidUrlParametersError(request);
}

return user;
}

private static void returnInvalidUrlParametersError(Request request) {
logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, "Invalid URL parameters");
}

/**
* Mark notification as opened.
*/
private static void updateNotificationState(OtpUser user, String notificationId) {
for (TripSurveyNotification notification : user.tripSurveyNotifications) {
if (notificationId.equals(notification.id)) {
notification.timeOpened = Date.from(Instant.now());
break;
}
Optional<TripSurveyNotification> notificationOpt = user.findNotification(notificationId);
if (notificationOpt.isPresent()) {
notificationOpt.get().timeOpened = Date.from(Instant.now());
Persistence.otpUsers.replace(user.id, user);
}
Persistence.otpUsers.replace(user.id, user);
}

public static String makeTripSurveyUrl(String subdomain, String surveyId, String userId, String tripId, String notificationId) {
Expand All @@ -84,7 +100,7 @@ private static boolean processCall(Request req, Response res) {
String tripId = req.queryParams("trip_id");
String notificationId = req.queryParams("notification_id");

OtpUser user = checkParameters(userId, tripId, notificationId, res);
OtpUser user = checkParameters(userId, tripId, notificationId, req);

if (user != null) {
String surveyUrl = makeTripSurveyUrl(
Expand Down
7 changes: 7 additions & 0 deletions src/main/java/org/opentripplanner/middleware/models/OtpUser.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.fasterxml.jackson.annotation.JsonGetter;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonSetter;
import org.apache.logging.log4j.util.Strings;
import org.opentripplanner.middleware.auth.Auth0Users;
import org.opentripplanner.middleware.auth.RequestingUser;
import org.opentripplanner.middleware.persistence.Persistence;
Expand Down Expand Up @@ -210,4 +211,10 @@ public Optional<TripSurveyNotification> findLastTripSurveyNotificationSent() {
if (tripSurveyNotifications == null) return Optional.empty();
return tripSurveyNotifications.stream().max(Comparator.comparingLong(n -> n.timeSent.getTime()));
}

/** Obtains a notification with the given id, if available. */
public Optional<TripSurveyNotification> findNotification(String id) {
if (tripSurveyNotifications == null || Strings.isBlank(id)) return Optional.empty();
return tripSurveyNotifications.stream().filter(n -> id.equals(n.id)).findFirst();
}
}
73 changes: 60 additions & 13 deletions src/test/java/org/opentripplanner/middleware/controllers/api/TripSurveyControllerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,11 @@
import org.eclipse.jetty.http.HttpStatus;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.opentripplanner.middleware.models.MonitoredTrip;
import org.opentripplanner.middleware.models.OtpUser;
import org.opentripplanner.middleware.models.TrackedJourney;
Expand All @@ -19,6 +23,7 @@
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.stream.Stream;

import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
Expand All @@ -42,12 +47,6 @@ public static void setUp() throws Exception {
trackedJourney = new TrackedJourney();
trackedJourney.id = UUID.randomUUID().toString();
Persistence.trackedJourneys.create(trackedJourney);

otpUser.tripSurveyNotifications = List.of(
new TripSurveyNotification("other-notification", Date.from(Instant.now()), "other-journey"),
new TripSurveyNotification(NOTIFICATION_ID, Date.from(Instant.now()), trackedJourney.id)
);
Persistence.otpUsers.replace(otpUser.id, otpUser);
}

private static MonitoredTrip createMonitoredTrip() {
Expand All @@ -69,6 +68,15 @@ public static void tearDown() throws Exception {
if (trackedJourney != null) trackedJourney.delete();
}

@BeforeEach
void setUpTest() {
otpUser.tripSurveyNotifications = List.of(
new TripSurveyNotification("other-notification", Date.from(Instant.now()), "other-journey"),
new TripSurveyNotification(NOTIFICATION_ID, Date.from(Instant.now()), trackedJourney.id)
);
Persistence.otpUsers.replace(otpUser.id, otpUser);
}

@Test
void canMakeTripSurveyUrl() {
assertEquals(
Expand Down Expand Up @@ -106,14 +114,53 @@ void canOpenSurveyAndUpdateNotificationStatus() {
OtpUser updatedUser = Persistence.otpUsers.getById(otpUser.id);
assertNotNull(updatedUser);
assertEquals(otpUser.tripSurveyNotifications.size(), updatedUser.tripSurveyNotifications.size());
updatedUser.tripSurveyNotifications.forEach(n -> {
if (NOTIFICATION_ID.equals(n.id)) {
assertNotNull(n.timeOpened);
assertTrue(n.timeOpened.toInstant().isAfter(requestInstant));
assertTrue(n.timeOpened.toInstant().isBefore(requestCompleteInstant));

int updatedNotificationCount = 0;
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));
updatedNotificationCount++;
} else {
assertNull(n.timeOpened);
assertNull(notification.timeOpened);
}
});
}
assertEquals(1, updatedNotificationCount);
}

@ParameterizedTest
@MethodSource("createShouldRejectInvalidParamsCases")
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&notification_id=%s",
userId,
tripId,
notificationId
),
"",
Map.of(),
HttpMethod.GET
);

assertEquals(
HttpStatus.BAD_REQUEST_400,
response.status,
"Invalid URL params should result in HTTP Status 400."
);
}

private static Stream<Arguments> createShouldRejectInvalidParamsCases() {
return Stream.of(
Arguments.of("invalid-user-id", monitoredTrip.id, NOTIFICATION_ID),
Arguments.of(null, monitoredTrip.id, NOTIFICATION_ID),
Arguments.of(otpUser.id, "invalid-trip-id", NOTIFICATION_ID),
Arguments.of(otpUser.id, null, NOTIFICATION_ID),
Arguments.of(otpUser.id, monitoredTrip.id, "invalid-notification-id"),
Arguments.of(otpUser.id, monitoredTrip.id, null)
);
}
}

0 comments on commit 31c77b7

Please sign in to comment.