refactor(NotificationUtils): Erase pendingPhoneNumber before checking…

… new phone validity.
ibi-group · Oct 15, 2020 · 5621a13 · 5621a13
1 parent 0bb7520
commit 5621a13
Show file tree

Hide file tree

Showing 3 changed files with 28 additions and 10 deletions.
diff --git a/src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java b/src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java
@@ -102,6 +102,11 @@ public VerificationResult sendVerificationText(Request req, Response res) {
             logMessageAndHalt(req, HttpStatus.BAD_REQUEST_400, "A phone number must be provided.");
         }
 
+        // Set OtpUser.pendingPhoneNumber* to null before checking numbers.
+        otpUser.pendingPhoneNumber = null;
+        otpUser.pendingPhoneNumberFormatted = null;
+        Persistence.otpUsers.replace(otpUser.id, otpUser);
+
         PhoneNumber validNumber = NotificationUtils.ensureDomesticPhoneNumber(req, phoneNumber);
 
         // Update OtpUser.pendingPhoneNumber before submitting SMS request.
@@ -149,7 +154,9 @@ public VerificationResult verifyPhoneWithCode(Request req, Response res) {
             // If the check is successful, update the OtpUser's phoneNumber and erase pendingPhoneNumber.
             // TODO: Figure out adding a test for this update to the OtpUser record.
             otpUser.phoneNumber = otpUser.pendingPhoneNumber;
+            otpUser.phoneNumberFormatted = otpUser.pendingPhoneNumberFormatted;
             otpUser.pendingPhoneNumber = null;
+            otpUser.pendingPhoneNumberFormatted = null;
             Persistence.otpUsers.replace(otpUser.id, otpUser);
         }
 

diff --git a/src/main/java/org/opentripplanner/middleware/utils/NotificationUtils.java b/src/main/java/org/opentripplanner/middleware/utils/NotificationUtils.java
@@ -21,7 +21,6 @@
 import org.slf4j.LoggerFactory;
 
 import java.io.IOException;
-import java.util.List;
 
 import static org.opentripplanner.middleware.utils.ConfigUtils.getConfigPropertyAsText;
 import static org.opentripplanner.middleware.utils.JsonUtils.logMessageAndHalt;
@@ -187,9 +186,9 @@ public static com.twilio.rest.lookups.v1.PhoneNumber ensureDomesticPhoneNumber(s
         try {
             Twilio.init(TWILIO_ACCOUNT_SID, TWILIO_AUTH_TOKEN);
             phoneNumber = com.twilio.rest.lookups.v1.PhoneNumber.fetcher(
-                new PhoneNumber(phoneNumberString))
+                // HACK: Decoding in the twilio API will turn spaces into '+' signs that will fail otherwise valid numbers.
+                new PhoneNumber(phoneNumberString.replace(" ", "")))
                 .setCountryCode(COUNTRY_CODE)
-                .setType(List.of("carrier"))
                 .fetch();
         } catch (ApiException apiException) {
             // Handle 404 response - corresponds to invalid number.
@@ -216,12 +215,6 @@ public static com.twilio.rest.lookups.v1.PhoneNumber ensureDomesticPhoneNumber(s
         }
 
         if (phoneNumber != null) {
-            System.out.println(phoneNumber.getPhoneNumber());
-            System.out.println(phoneNumber.getCarrier().get("type"));
-            System.out.println(phoneNumber.getCarrier().get("name"));
-            System.out.println(phoneNumber.getCountryCode());
-            System.out.println(phoneNumber.getNationalFormat());
-
             // Reject numbers that are international with respect to COUNTRY_CODE.
             // TODO: Also reject numbers whose .getCarrier().get("type") is not "mobile"?
             if (!phoneNumber.getCountryCode().equals(COUNTRY_CODE)) {

diff --git a/src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java b/src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java
@@ -15,11 +15,13 @@
 import org.opentripplanner.middleware.utils.NotificationUtils;
 
 import java.io.IOException;
+import java.io.UnsupportedEncodingException;
 import java.net.http.HttpResponse;
 import java.util.List;
 import java.util.UUID;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
 import static org.junit.jupiter.api.Assumptions.assumeTrue;
 import static org.opentripplanner.middleware.TestUtils.mockAuthenticatedRequest;
 
@@ -64,7 +66,7 @@ public static void tearDown() {
      * and leaves OtpUser.phoneNumber intact.
      */
     @Test
-    public void smsRequestShouldSetPendingPhoneNumberOnly() {
+    public void smsRequestShouldSetPendingPhoneNumberOnly() throws UnsupportedEncodingException {
         final String PHONE_NUMBER_TO_VERIFY = "+15555550321";
         final String PHONE_NUMBER_TO_VERIFY_FORMATTED = "(555) 555-0321";
 
@@ -105,6 +107,8 @@ public void smsRequestShouldSetPendingPhoneNumberOnly() {
     public void invalidOrForeignNumbersShouldProduceBadRequest(String number) {
         assumeTrue(NotificationUtils.COUNTRY_CODE.equals("US"));
 
+        // 1. Request verification SMS.
+        // The invalid number should fail the call.
         HttpResponse<String> response = mockAuthenticatedRequest(
             String.format("api/secure/user/%s/verify_sms/%s",
                 otpUser.id,
@@ -114,6 +118,20 @@ public void invalidOrForeignNumbersShouldProduceBadRequest(String number) {
             HttpUtils.REQUEST_METHOD.GET
         );
         assertEquals(HttpStatus.BAD_REQUEST_400, response.statusCode());
+
+        // 2. Fetch the newly-created user.
+        // pendingPhoneNumber* fields should be null.
+        HttpResponse<String> otpUserWithBadPhoneRequest = mockAuthenticatedRequest(
+            String.format("api/secure/user/%s", otpUser.id),
+            otpUser,
+            HttpUtils.REQUEST_METHOD.GET
+        );
+        assertEquals(HttpStatus.OK_200, otpUserWithBadPhoneRequest.statusCode());
+
+        OtpUser otpUserWithBadPhone = JsonUtils.getPOJOFromJSON(otpUserWithBadPhoneRequest.body(), OtpUser.class);
+        assertEquals(INITIAL_PHONE_NUMBER, otpUserWithBadPhone.phoneNumber);
+        assertNull(otpUserWithBadPhone.pendingPhoneNumber);
+        assertNull(otpUserWithBadPhone.pendingPhoneNumberFormatted);
     }
 
     private static List<String> createRejectedNumbers() {