refactor(Addressed PR feedback): Handle related user update on PUT

README.md

@@ -301,8 +301,7 @@ The special E2E client settings should be defined in `env.yml`:
 | TRIP_INSTRUCTION_IMMEDIATE_RADIUS | integer | Optional | 2 | The radius in meters under which an immediate instruction is given. |
 | TRIP_INSTRUCTION_UPCOMING_RADIUS | integer | Optional | 10 | The radius in meters under which an upcoming instruction is given. |
 | TWILIO_ACCOUNT_SID | string | Optional | your-account-sid | Twilio settings available at: https://twilio.com/user/account |
-| TRUSTED_COMPANION_CONFIRMATION_PAGE_URL | string | Optional | https://otp-server.example.com/trusted/confirmation | URL to the trusted companion confirmation page. |
-| TRUSTED_COMPANION_ERROR_PAGE_URL | string | Optional | https://otp-server.example.com/trusted/error | URL to the trusted companion error page. |
+| TRUSTED_COMPANION_CONFIRMATION_PAGE_URL | string | Optional | https://otp-server.example.com/trusted/confirmation | URL to the trusted companion confirmation page. This page should support handling an error URL parameter. |
 | TWILIO_AUTH_TOKEN | string | Optional | your-auth-token | Twilio settings available at: https://twilio.com/user/account |
 | US_RIDE_GWINNETT_BUS_OPERATOR_NOTIFIER_API_URL | string | Optional | http://host.example.com | US RideGwinnett bus notifier API. |
 | US_RIDE_GWINNETT_BUS_OPERATOR_NOTIFIER_API_KEY | string | Optional | your-api-key | API key for the US RideGwinnett bus notifier API. |

src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java

@@ -25,6 +25,7 @@
 
 import static io.github.manusant.ss.descriptor.MethodDescriptor.path;
 import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.ACCEPT_KEY;
+import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.ensureRelatedUserIntegrity;
 import static org.opentripplanner.middleware.tripmonitor.TrustedCompanion.manageAcceptDependentEmail;
 import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt;
 
@@ -70,6 +71,7 @@ OtpUser preCreateHook(OtpUser user, Request req) {
     @Override
     OtpUser preUpdateHook(OtpUser user, OtpUser preExistingUser, Request req) {
         preliminaryTasks(user);
+        ensureRelatedUserIntegrity(user, preExistingUser);
         return super.preUpdateHook(user, preExistingUser, req);
     }
 

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

@@ -9,20 +9,20 @@ public enum RelatedUserStatus {
     public String email;
     public RelatedUserStatus status = RelatedUserStatus.PENDING;
     public String acceptKey;
-    public String nickName;
+    public String nickname;
 
     public RelatedUser() {
         // Required for JSON deserialization.
     }
 
-    public RelatedUser(String email, RelatedUserStatus status, String nickName) {
+    public RelatedUser(String email, RelatedUserStatus status, String nickname) {
         this.email = email;
         this.status = status;
-        this.nickName = nickName;
+        this.nickname = nickname;
     }
 
-    public RelatedUser(String email, RelatedUserStatus status, String nickName, String acceptKey) {
-        this (email, status, nickName);
+    public RelatedUser(String email, RelatedUserStatus status, String nickname, String acceptKey) {
+        this (email, status, nickname);
         this.acceptKey = acceptKey;
     }
 }

src/main/java/org/opentripplanner/middleware/tripmonitor/TrustedCompanion.java

@@ -1,7 +1,6 @@
 package org.opentripplanner.middleware.tripmonitor;
 
 import com.mongodb.client.model.Filters;
-import org.eclipse.jetty.http.HttpStatus;
 import org.opentripplanner.middleware.OtpMiddlewareMain;
 import org.opentripplanner.middleware.i18n.Message;
 import org.opentripplanner.middleware.models.OtpUser;
@@ -15,15 +14,16 @@
 import spark.Response;
 
 import java.util.HashMap;
+import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
 import java.util.UUID;
+import java.util.stream.Collectors;
 
 import static com.mongodb.client.model.Filters.eq;
 import static org.opentripplanner.middleware.tripmonitor.jobs.CheckMonitoredTrip.SETTINGS_PATH;
 import static org.opentripplanner.middleware.utils.I18nUtils.label;
-import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt;
 
 public class TrustedCompanion {
 
@@ -35,8 +35,8 @@ private TrustedCompanion() {
     private static final String AWS_API_STAGE = ConfigUtils.getConfigPropertyAsText("AWS_API_STAGE");
     private static final String OTP_UI_URL = ConfigUtils.getConfigPropertyAsText("OTP_UI_URL");
     private static final String TRUSTED_COMPANION_CONFIRMATION_PAGE_URL = ConfigUtils.getConfigPropertyAsText("TRUSTED_COMPANION_CONFIRMATION_PAGE_URL");
-    private static final String TRUSTED_COMPANION_ERROR_PAGE_URL = ConfigUtils.getConfigPropertyAsText("TRUSTED_COMPANION_ERROR_PAGE_URL");
     public static final String ACCEPT_KEY = "acceptKey";
+    public static final String EMAIL_FIELD_NAME = "email";
 
     /** Note: This path is excluded from security checks, see {@link OtpMiddlewareMain#initializeHttpEndpoints()}. */
     public static final String ACCEPT_DEPENDENT_PATH = "api/secure/user/acceptdependent";
@@ -46,31 +46,33 @@ private TrustedCompanion() {
      * successful redirect the user to the confirmation page, else redirect to the error page with related error.
      */
     public static OtpUser acceptDependent(Request request, Response response) {
-        String acceptKey = getAcceptKeyFromRequest(request);
-        OtpUser dependentUser = getUserFromAcceptKey(request, acceptKey);
-        OtpUser relatedUser = getRelatedUserFromEmail(dependentUser, acceptKey);
-
-        if (dependentUser != null && relatedUser != null) {
-            Optional<RelatedUser> relatedUserToUpdate = dependentUser.relatedUsers
-                .stream()
-                .filter(related -> related.email.equals(relatedUser.email))
-                .findFirst();
-            relatedUserToUpdate.ifPresent(value -> value.status = RelatedUser.RelatedUserStatus.CONFIRMED);
-
-            // Maintain a list of dependents.
-            relatedUser.dependents.add(dependentUser.id);
-            Persistence.otpUsers.replace(relatedUser.id, relatedUser);
-            // Update list of related users.
-            Persistence.otpUsers.replace(dependentUser.id, dependentUser);
-
-            // Redirect to confirmation page and provide dependent user information.
-            response.redirect(TRUSTED_COMPANION_CONFIRMATION_PAGE_URL);
-            return dependentUser;
+        try {
+            String acceptKey = getAcceptKeyFromRequest(request);
+            OtpUser dependentUser = getUserFromAcceptKey(acceptKey);
+            OtpUser relatedUser = getRelatedUserFromEmail(dependentUser, acceptKey);
+
+            if (relatedUser != null) {
+                Optional<RelatedUser> relatedUserToUpdate = dependentUser.relatedUsers
+                    .stream()
+                    .filter(related -> related.email.equals(relatedUser.email))
+                    .findFirst();
+                relatedUserToUpdate.ifPresent(value -> value.status = RelatedUser.RelatedUserStatus.CONFIRMED);
+
+                // Maintain a list of dependents.
+                relatedUser.dependents.add(dependentUser.id);
+                Persistence.otpUsers.replace(relatedUser.id, relatedUser);
+                // Update list of related users.
+                Persistence.otpUsers.replace(dependentUser.id, dependentUser);
+
+                // Redirect to confirmation page and provide dependent user information.
+                response.redirect(TRUSTED_COMPANION_CONFIRMATION_PAGE_URL);
+                return dependentUser;
+            }
+        } catch (IllegalArgumentException e) {
+            response.redirect(
+                String.format("%s?error=%s", TRUSTED_COMPANION_CONFIRMATION_PAGE_URL, Message.ACCEPT_DEPENDENT_ERROR.get(null))
+            );
         }
-
-        response.redirect(
-            String.format("%s?error=%s", TRUSTED_COMPANION_ERROR_PAGE_URL, Message.ACCEPT_DEPENDENT_ERROR.get(null))
-        );
         return null;
     }
 
@@ -85,32 +87,31 @@ private static OtpUser getRelatedUserFromEmail(OtpUser dependentUser, String acc
             .stream()
             .filter(user -> user.acceptKey.equalsIgnoreCase(acceptKey))
             .findFirst();
-        return relatedUser.map(user -> Persistence.otpUsers.getOneFiltered(eq("email", user.email))).orElse(null);
+        return relatedUser.map(user -> Persistence.otpUsers.getOneFiltered(eq(EMAIL_FIELD_NAME, user.email))).orElse(null);
     }
 
     /**
      * Extract the accept key from the request parameters.
      */
-    private static String getAcceptKeyFromRequest(Request request) {
-        String acceptKey = HttpUtils.getQueryParamFromRequest(request, ACCEPT_KEY, false);
-        if (acceptKey.isEmpty()) {
-            logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, "Accept key not provided.");
-            return null;
+    private static String getAcceptKeyFromRequest(Request request) throws IllegalArgumentException {
+        // Note: optional is true so a missing accept key will be handled here.
+        String acceptKey = HttpUtils.getQueryParamFromRequest(request, ACCEPT_KEY, true);
+        if (acceptKey == null || acceptKey.isEmpty()) {
+            throw new IllegalArgumentException("Accept key not provided.");
         }
         return acceptKey;
     }
 
     /**
      * Retrieve the dependent user matching the accept key.
      */
-    private static OtpUser getUserFromAcceptKey(Request request, String acceptKey) {
+    private static OtpUser getUserFromAcceptKey(String acceptKey) throws IllegalArgumentException {
         if (acceptKey == null) {
             return null;
         }
         OtpUser user = getUserForAcceptKey(acceptKey);
         if (user == null) {
-            logMessageAndHalt(request, HttpStatus.BAD_REQUEST_400, "Otp user unknown.");
-            return null;
+            throw new IllegalArgumentException("Otp user unknown.");
         }
         return user;
     }
@@ -135,13 +136,13 @@ public static void manageAcceptDependentEmail(OtpUser dependentUser, boolean isT
             .filter(relatedUser -> relatedUser.acceptKey == null)
             .forEach(relatedUser -> {
                 String acceptKey = UUID.randomUUID().toString();
-                OtpUser userToReceiveEmail = Persistence.otpUsers.getOneFiltered(eq("email", relatedUser.email));
+                OtpUser userToReceiveEmail = Persistence.otpUsers.getOneFiltered(eq(EMAIL_FIELD_NAME, relatedUser.email));
                 if (userToReceiveEmail != null && (isTest || sendAcceptDependentEmail(dependentUser, userToReceiveEmail, acceptKey))) {
                     relatedUser.acceptKey = acceptKey;
                 }
             });
 
-        // Preserve email sent status.
+        // Preserve email sent status (by storing the accept key).
         Persistence.otpUsers.replace(dependentUser.id, dependentUser);
     }
 
@@ -162,7 +163,7 @@ private static boolean sendAcceptDependentEmail(OtpUser dependentUser, OtpUser r
                 "acceptDependentLinkLabelAndUrl", label(acceptDependentLinkLabel, acceptDependentUrl, locale),
                 "acceptDependentUrl", acceptDependentUrl,
                 "emailFooter", Message.ACCEPT_DEPENDENT_EMAIL_FOOTER.get(locale),
-                "emailGreeting", String.format("%s %s", addressee, Message.ACCEPT_DEPENDENT_EMAIL_GREETING.get(locale)),
+                "emailGreeting", String.format(Message.ACCEPT_DEPENDENT_EMAIL_GREETING.get(locale), addressee),
                 "manageLinkUrl", String.format("%s%s", OTP_UI_URL, SETTINGS_PATH),
                 "manageLinkText", Message.ACCEPT_DEPENDENT_EMAIL_MANAGE.get(locale)
             )
@@ -192,4 +193,21 @@ public static String getAcceptDependentEndPoint(String acceptKey) {
     private static OtpUser getUserForAcceptKey(String acceptKey) {
         return Persistence.otpUsers.getOneFiltered(Filters.elemMatch("relatedUsers", Filters.eq(ACCEPT_KEY, acceptKey)));
     }
+
+    /**
+     * If a dependent removes a related user, remove the dependent from the related user.
+     */
+    public static void ensureRelatedUserIntegrity(OtpUser updatedUser, OtpUser preExistingUser) {
+        List<RelatedUser> difference = preExistingUser.relatedUsers
+            .stream()
+            .filter(relatedUser -> !updatedUser.relatedUsers.contains(relatedUser))
+            .collect(Collectors.toList());
+        for (RelatedUser relatedUser : difference) {
+            OtpUser user = Persistence.otpUsers.getOneFiltered(eq(EMAIL_FIELD_NAME, relatedUser.email));
+            if (user != null) {
+                user.dependents.remove(updatedUser.id);
+                Persistence.otpUsers.replace(user.id, user);
+            }
+        }
+    }
 }

src/main/resources/Message.properties

@@ -1,5 +1,5 @@
 ACCEPT_DEPENDENT_EMAIL_FOOTER = You are receiving this email because you have been selected to be a trusted companion.
-ACCEPT_DEPENDENT_EMAIL_GREETING = would like you to be a trusted companion.
+ACCEPT_DEPENDENT_EMAIL_GREETING = %s would like you to be a trusted companion.
 ACCEPT_DEPENDENT_EMAIL_LINK_TEXT = Accept trusted companion
 ACCEPT_DEPENDENT_EMAIL_SUBJECT = Trusted companion request
 ACCEPT_DEPENDENT_EMAIL_MANAGE = Manage settings

src/main/resources/Message_fr.properties

@@ -1,5 +1,5 @@
 ACCEPT_DEPENDENT_EMAIL_FOOTER = TODO
-ACCEPT_DEPENDENT_EMAIL_GREETING = TODO
+ACCEPT_DEPENDENT_EMAIL_GREETING = %s TODO
 ACCEPT_DEPENDENT_EMAIL_LINK_TEXT = TODO
 ACCEPT_DEPENDENT_EMAIL_SUBJECT = TODO
 ACCEPT_DEPENDENT_EMAIL_MANAGE = TODO

src/main/resources/env.schema.json

@@ -327,12 +327,7 @@
     "TRUSTED_COMPANION_CONFIRMATION_PAGE_URL": {
       "type": "string",
       "examples": ["https://otp-server.example.com/trusted/confirmation"],
-      "description": "URL to the trusted companion confirmation page."
-    },
-    "TRUSTED_COMPANION_ERROR_PAGE_URL": {
-      "type": "string",
-      "examples": ["https://otp-server.example.com/trusted/error"],
-      "description": "URL to the trusted companion error page."
+      "description": "URL to the trusted companion confirmation page. This page should support handling an error URL parameter."
     },
     "TWILIO_AUTH_TOKEN": {
       "type": "string",

src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java

@@ -20,7 +20,6 @@
 import org.opentripplanner.middleware.utils.JsonUtils;
 
 import java.util.Date;
-import java.util.HashMap;
 import java.util.List;
 import java.util.UUID;
 import java.util.stream.Stream;
@@ -30,7 +29,6 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 
-import static org.opentripplanner.middleware.testutils.ApiTestUtils.createAndAssignAuth0User;
 import static org.opentripplanner.middleware.testutils.ApiTestUtils.getMockHeaders;
 import static org.opentripplanner.middleware.testutils.ApiTestUtils.makeGetRequest;
 import static org.opentripplanner.middleware.testutils.ApiTestUtils.makeRequest;
@@ -48,8 +46,9 @@ public class OtpUserControllerTest extends OtpMiddlewareTestEnvironment {
     private static OtpUser dependentUserTwo;
     private static OtpUser relatedUserThree;
     private static OtpUser dependentUserThree;
-    private static HashMap<String, String> relatedUserHeaders;
-    private static final String nickName = "my-trusted-companion";
+    private static OtpUser relatedUserFour;
+    private static OtpUser dependentUserFour;
+    private static final String nickname = "my-trusted-companion";
 
     @BeforeAll
     public static void setUp() throws Exception {
@@ -72,7 +71,8 @@ public static void setUp() throws Exception {
         dependentUserTwo = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("dependent-two"));
         relatedUserThree = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("related-user-three"));
         dependentUserThree = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("dependent-three"));
-        relatedUserHeaders = createAndAssignAuth0User(relatedUserOne);
+        relatedUserFour = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("related-user-four"));
+        dependentUserFour = PersistenceTestUtils.createUser(ApiTestUtils.generateEmailAddress("dependent-four"));
     }
 
     @AfterAll
@@ -83,9 +83,11 @@ public static void tearDown() {
             relatedUserOne,
             relatedUserTwo,
             relatedUserThree,
+            relatedUserFour,
             dependentUserOne,
             dependentUserTwo,
-            dependentUserThree
+            dependentUserThree,
+            dependentUserFour
         );
 
         // Restore original isAuthDisabled state.
@@ -180,7 +182,7 @@ void canAcceptDependentRequest() {
         dependentUserOne.relatedUsers.add(new RelatedUser(
             relatedUserOne.email,
             RelatedUser.RelatedUserStatus.PENDING,
-            nickName,
+            nickname,
             acceptKey
         ));
         Persistence.otpUsers.replace(dependentUserOne.id, dependentUserOne);
@@ -200,13 +202,13 @@ void canAcceptDependentRequest() {
     }
 
     @Test
-    void canInvalidateDependent() {
+    void canInvalidateDependentOnDelete() {
         relatedUserTwo.dependents.add(dependentUserTwo.id);
         Persistence.otpUsers.replace(relatedUserTwo.id, relatedUserTwo);
         dependentUserTwo.relatedUsers.add(new RelatedUser(
             relatedUserTwo.email,
             RelatedUser.RelatedUserStatus.CONFIRMED,
-            nickName
+            nickname
         ));
         Persistence.otpUsers.replace(dependentUserTwo.id, dependentUserTwo);
         relatedUserTwo.delete(false);
@@ -216,17 +218,59 @@ void canInvalidateDependent() {
     }
 
     @Test
-    void canRemoveRelatedUser() {
+    void canRemoveRelatedUserOnDelete() {
         relatedUserThree.dependents.add(dependentUserThree.id);
         Persistence.otpUsers.replace(relatedUserThree.id, relatedUserThree);
         dependentUserThree.relatedUsers.add(new RelatedUser(
             relatedUserThree.email,
             RelatedUser.RelatedUserStatus.CONFIRMED,
-            nickName
+            nickname
         ));
         Persistence.otpUsers.replace(dependentUserThree.id, dependentUserThree);
         dependentUserThree.delete(false);
         relatedUserThree = Persistence.otpUsers.getById(relatedUserThree.id);
         assertFalse(relatedUserThree.dependents.contains(dependentUserThree.id));
     }
+
+    /**
+     * Confirm that a user can be removed from a related users list, and importantly, the related user no longer lists
+     * the removed dependent.
+     */
+    @Test
+    void canRemoveUserFromRelatedUsersList() throws Exception {
+        setAuthDisabled(true);
+        relatedUserFour.dependents.add(dependentUserFour.id);
+        Persistence.otpUsers.replace(relatedUserFour.id, relatedUserFour);
+        dependentUserFour.relatedUsers.add(new RelatedUser(
+            relatedUserFour.email,
+            RelatedUser.RelatedUserStatus.CONFIRMED,
+            nickname
+        ));
+        Persistence.otpUsers.replace(dependentUserFour.id, dependentUserFour);
+
+        // Remove the first related user.
+        dependentUserFour.relatedUsers.clear();
+
+        // Add a new related user that should not be considered for integrity update.
+        dependentUserFour.relatedUsers.add(new RelatedUser(
+            relatedUserThree.email,
+            RelatedUser.RelatedUserStatus.PENDING,
+            nickname
+        ));
+
+        makeRequest(
+            String.format("api/secure/user/%s", dependentUserFour.id),
+            JsonUtils.toJson(dependentUserFour),
+            getMockHeaders(dependentUserFour),
+            HttpMethod.PUT
+        );
+
+        dependentUserFour = Persistence.otpUsers.getById(dependentUserFour.id);
+        assertFalse(dependentUserFour.relatedUsers.stream().anyMatch(u -> u.email.equalsIgnoreCase(relatedUserFour.email)));
+
+        relatedUserFour = Persistence.otpUsers.getById(relatedUserFour.id);
+        assertFalse(relatedUserFour.dependents.contains(dependentUserFour.id));
+
+        setAuthDisabled(false);
+    }
 }