Change in persisting phone number. #83

binh-dam-ibigroup · 2020-10-08T15:44:32Z

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
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 fixes #79.

Per team discussion, the /api/secure/user/<id>/verify_sms GET endpoint (for requesting an SMS) now takes a phone number as path parameter and persists that number as the user's phone number. It also sets isPhoneNumberVerified = false ~~and notificationChannel = "sms" (let me know if we should not set the notification channel)~~. A test has been added to check these assignments.

The /api/secure/user/<id>/verify_sms POST endpoint now sets isPhoneNumberVerified = true ~~and notificationChannel = "sms"~~ upon receiving an "approved" phone verification status.

…e sending SMS.

…idation check.

…ment.

landonreed · 2020-10-08T20:21:44Z

src/test/java/org/opentripplanner/middleware/ApiUserFlowTest.java

+     * Tests for the chain of API calls to verify a user's phone number.
+     */
+    @Test
+    public void canSimulatePhoneVerificationFlow() {


I don't think this test really belongs here. This test class was intended to model how a third party API user interacts with otp-middleware. And an API user (eventually) won't be able to create an OtpUser that can register a phone number for SMS notifications.

I moved and changed the test in 44864ac.

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

landonreed

Looks good. Just a couple of comments (location of test and automatically setting notification channel).

…o not set the notification chan

codecov-io · 2020-10-08T21:18:01Z

Codecov Report

Merging #83 into dev will increase coverage by 5.76%.
The diff coverage is 51.85%.

@@             Coverage Diff              @@
##                dev      #83      +/-   ##
============================================
+ Coverage     38.74%   44.50%   +5.76%     
- Complexity      264      381     +117     
============================================
  Files            81       83       +2     
  Lines          2261     2775     +514     
  Branches        258      344      +86     
============================================
+ Hits            876     1235     +359     
- Misses         1301     1419     +118     
- Partials         84      121      +37

Impacted Files	Coverage Δ	Complexity Δ
...org/opentripplanner/middleware/models/OtpUser.java	`45.45% <ø> (+13.63%)`	`6.00 <0.00> (+2.00)`
.../middleware/controllers/api/OtpUserController.java	`45.45% <51.85%> (+5.45%)`	`6.00 <2.00> (+3.00)`
...eware/controllers/api/MonitoredTripController.java	`13.79% <0.00%> (-14.78%)`	`2.00% <0.00%> (ø%)`
...rg/opentripplanner/middleware/utils/HttpUtils.java	`50.00% <0.00%> (-7.15%)`	`20.00% <0.00%> (+10.00%)`	⬇️
...tripplanner/middleware/otp/response/Itinerary.java	`63.15% <0.00%> (-5.60%)`	`7.00% <0.00%> (+2.00%)`	⬇️
...dleware/controllers/api/TripHistoryController.java	`43.02% <0.00%> (-5.47%)`	`5.00% <0.00%> (+2.00%)`	⬇️
...nner/middleware/controllers/api/LogController.java	`40.84% <0.00%> (-5.46%)`	`2.00% <0.00%> (ø%)`
.../middleware/controllers/response/ResponseList.java	`68.42% <0.00%> (ø)`	`2.00% <0.00%> (?%)`
...ipplanner/middleware/otp/response/OtpResponse.java	`100.00% <0.00%> (ø)`	`2.00% <0.00%> (?%)`
...a/org/opentripplanner/middleware/models/Model.java	`40.90% <0.00%> (+0.90%)`	`4.00% <0.00%> (+2.00%)`
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6971187...6088b31. Read the comment docs.

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

landonreed · 2020-10-08T21:29:39Z

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

                    .withDescription("Request an SMS verification to be sent to an OtpUser's phone number.")
-                    .withPathParam().withName(ID_PARAM).withDescription("The id of the OtpUser.").and()
+                    .withPathParam().withName(ID_PARAM).withRequired(true).withDescription("The id of the OtpUser.").and()
+                    .withPathParam().withName(PHONE_PARAM).withRequired(true).withDescription("The phone number to validate, in E.164 format.").and()


While precise, this isn't super helpful for most people. It would be good to add an example to this description (e.g., +15555551234).

This item is unresolved.

Fixed. in e1e2cd2

Sorry, I didn't mean to remove that it should be E.164 (which is nice because it is precise), just that we should add the example (for folks who don't immediately know what that means). To further clarify: we should be picky about the format used for this phone number field. If it doesn't match e.164, we should reject the request.

It looks like you have added a configurable regex for making this format flexible. I disagree with this change. We should just use a hard-coded regex and enforce a single format in requests.

evansiroky · 2020-10-08T21:30:06Z

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

-        Verification verification = NotificationUtils.sendVerificationText(otpUser.phoneNumber);
+
+        // Update OtpUser before submitting SMS request.
+        otpUser.phoneNumber = phoneNumber;


This approach overwrites the existing phone number before it is verified. If we do things this way, it will be impossible to send off alerts to the phone until it is verified. And if the user decides they actually don't want to verify after all and keep their old number, we will have overwritten it and they would have to go back in and reconfirm their existing phone number.

The temporary phone number should be stored in a temporary field until it is verified.

See the Revert Number button in opentripplanner/otp-react-redux#224 for part of this concern.

I disagree with the approach of having a revert option due to some of the concerns shared above. Additionally, it is more code to maintain than keeping an existing phone number intact until verification and requiring the user to reverify an original number if they want to change it back.

evansiroky · 2020-10-08T21:31:48Z

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

+            // If the check is successful, update the OtpUser
+            // (set isPhoneNumberVerified and notificationChannel).
+            // TODO: Figure out adding a test for this update to the OtpUser record.
+            otpUser.isPhoneNumberVerified = true;


Only at this point should the otpUser.phoneNumber be updated.

@landonreed and @evansiroky please agree on the above comment. 5621a13 does per above, d887661 does not.

Using isPhoneNumberVerified is how we originally designed things and in my opinion it works just fine. If there needs to be further discussion or an alternative approach, that should be handled outside of this PR.

I still disagree for reasons mentioned in company chat, but don't care enough to block progress.

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

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

landonreed

Thanks for the changes, @binh-dam-ibigroup. I think the only thing left is to enforce E.164 format in requests. I'm not sure if something I said caused you to deviate from how you had originally designed things. If so, I apologize -- I think your instincts were correct!

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

evansiroky

Contingent on #83 (comment) and @landonreed's approval following E.164 regex hardcoding check.

Still disagree about overwritten a confirmed number with an unconfirmed one, but don't care enough to block progress.

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

landonreed · 2020-10-21T13:30:30Z

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

+    @ParameterizedTest
+    @MethodSource("createBadPhoneNumbers")
+    public void invalidNumbersShouldProduceBadRequest(Map.Entry<String, Integer> testCase) {
+        assumeTrue(testsShouldRun());


I feel like there is a lot of churn happening on this auth disabled stuff, but I'm pretty sure in some PR we started to use the pattern of not checking DISABLE_AUTH=false but rather setting it with Auth0Connection#setAuthDisabled and reverting the value to getDefaultAuthDisabled in tearDown. Please follow that pattern here. Also, does e2e actually need to be enabled for these tests to work?

Good point. e2e is not required.

Updated in 8ec525a.

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

landonreed

Just a couple of comments that could be addressed, but otherwise looks great.

…ut valid phone number.

…equesting SMS.

binh-dam-ibigroup added 4 commits October 7, 2020 17:21

fix(OtpUserController): Add phone number as path param, save it befor…

0aa7231

…e sending SMS.

fix(OtpUserController): Set isPhoneNumberVerified after receiving val…

2d8dd5d

…idation check.

refactor(ApiUserFlowRequest): Remove unused var, add phone number com…

e0d7d80

…ment.

docs(OtpUserController): Add more method comments.

c3661d9

binh-dam-ibigroup requested review from landonreed, br648 and evansiroky October 8, 2020 15:44

binh-dam-ibigroup assigned landonreed, evansiroky and br648 Oct 8, 2020

chore(swagger): Update version-controlled swagger.

e59f10e

landonreed reviewed Oct 8, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 8, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java Outdated Show resolved Hide resolved

binh-dam-ibigroup mentioned this pull request Oct 8, 2020

Phone Verification opentripplanner/otp-react-redux#224

Merged

landonreed suggested changes Oct 8, 2020

View reviewed changes

landonreed assigned binh-dam-ibigroup and unassigned landonreed Oct 8, 2020

refactor(OtpUserControllerTest): Move phone API test to this class. D…

44864ac

…o not set the notification chan

binh-dam-ibigroup requested a review from landonreed October 8, 2020 21:19

binh-dam-ibigroup assigned landonreed and unassigned binh-dam-ibigroup Oct 8, 2020

landonreed reviewed Oct 8, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 8, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 8, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 8, 2020

View reviewed changes

evansiroky reviewed Oct 8, 2020

View reviewed changes

refactor(OtpUserController): Remove temp comment.

8888d01

evansiroky reviewed Oct 8, 2020

View reviewed changes

landonreed reviewed Oct 15, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/models/OtpUser.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 15, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/models/OtpUser.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 15, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/utils/NotificationUtils.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 15, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/utils/NotificationUtils.java Outdated Show resolved Hide resolved

landonreed suggested changes Oct 15, 2020

View reviewed changes

landonreed assigned binh-dam-ibigroup and unassigned landonreed Oct 15, 2020

evansiroky reviewed Oct 15, 2020

View reviewed changes

src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java Outdated Show resolved Hide resolved

evansiroky approved these changes Oct 15, 2020

View reviewed changes

evansiroky removed their assignment Oct 15, 2020

binh-dam-ibigroup added 2 commits October 16, 2020 10:32

refactor(OtpUserController): Accept E.164 phone numbers only.

a9386a6

docs(OtpUser/Controller): Tweak comments

74075ba

binh-dam-ibigroup requested a review from landonreed October 16, 2020 14:46

binh-dam-ibigroup assigned landonreed and unassigned binh-dam-ibigroup Oct 16, 2020

binh-dam-ibigroup added 2 commits October 16, 2020 11:55

fix(OtpUserController): Persist pending number after SMS sent.

b2198fe

refactor(OtpUserControllerTest): Remove unused imports

08de8f8

landonreed reviewed Oct 21, 2020

View reviewed changes

src/main/java/org/opentripplanner/middleware/controllers/api/OtpUserController.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 21, 2020

View reviewed changes

src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java Outdated Show resolved Hide resolved

landonreed reviewed Oct 21, 2020

View reviewed changes

src/test/java/org/opentripplanner/middleware/controllers/api/OtpUserControllerTest.java Outdated Show resolved Hide resolved

landonreed approved these changes Oct 21, 2020

View reviewed changes

landonreed assigned binh-dam-ibigroup and unassigned landonreed Oct 21, 2020

binh-dam-ibigroup added 4 commits October 21, 2020 12:02

refactor(OtpUserController/Test): Address PR comments

8ec525a

refactor(OtpUserController): Tweak message when submitting code witho…

6052d6f

…ut valid phone number.

fix(OtpUserController): Add missing ! in number check when sending code.

6088b31

refactor(OtpUserController): Make SMS the notification channel when r…

8b11107

…equesting SMS.

binh-dam-ibigroup merged commit ee8c6d7 into dev Oct 28, 2020

binh-dam-ibigroup deleted the change-phone-persisting branch October 28, 2020 21:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change in persisting phone number. #83

Change in persisting phone number. #83

binh-dam-ibigroup commented Oct 8, 2020 •

edited

Loading

landonreed Oct 8, 2020

binh-dam-ibigroup Oct 8, 2020

landonreed left a comment

codecov-io commented Oct 8, 2020 •

edited

Loading

landonreed Oct 8, 2020 •

edited

Loading

landonreed Oct 14, 2020

binh-dam-ibigroup Oct 15, 2020

landonreed Oct 15, 2020 •

edited

Loading

evansiroky Oct 8, 2020

binh-dam-ibigroup Oct 8, 2020

evansiroky Oct 8, 2020

evansiroky Oct 8, 2020

binh-dam-ibigroup Oct 15, 2020

landonreed Oct 15, 2020

evansiroky Oct 15, 2020

landonreed left a comment

evansiroky left a comment

landonreed Oct 21, 2020 •

edited

Loading

binh-dam-ibigroup Oct 21, 2020

binh-dam-ibigroup Oct 21, 2020

landonreed left a comment

Change in persisting phone number. #83

Change in persisting phone number. #83

Conversation

binh-dam-ibigroup commented Oct 8, 2020 • edited Loading

Checklist

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed left a comment

Choose a reason for hiding this comment

codecov-io commented Oct 8, 2020 • edited Loading

Codecov Report

landonreed Oct 8, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed Oct 15, 2020 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed left a comment

Choose a reason for hiding this comment

evansiroky left a comment

Choose a reason for hiding this comment

landonreed Oct 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

landonreed left a comment

Choose a reason for hiding this comment

binh-dam-ibigroup commented Oct 8, 2020 •

edited

Loading

codecov-io commented Oct 8, 2020 •

edited

Loading

landonreed Oct 8, 2020 •

edited

Loading

landonreed Oct 15, 2020 •

edited

Loading

landonreed Oct 21, 2020 •

edited

Loading