Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SMS phone verification using user's locale #191

Merged
merged 4 commits into from
Nov 15, 2023
Merged

Conversation

binh-dam-ibigroup
Copy link
Collaborator

@binh-dam-ibigroup binh-dam-ibigroup commented Nov 8, 2023

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
  • [na] 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 makes use of the user's preferredLocale for sending the phone verification SMS via Twilio.

To test:

  1. Log in in OTP-RR
  2. Required: Change the UI language using the language drop-down selector. (If you must keep the current one, change the language to something else, then after step 6, change it back to what it was.)
  3. Go to "My account > Settings"
  4. If you already have a phone number set and can access only one phone number, manually delete phoneNumber and isPhoneNumberVerified from your OtpUser record in Mongo.
  5. Enter a new phone number under the SMS notification option and click "Send Verification"
  6. Wait for the verification SMS, upon receiving, note the language of the text in the SMS.

@JymDyerIBI
Copy link
Contributor

JymDyerIBI commented Nov 9, 2023

I ran into some trouble testing. After removing the fields in Mongo I got this:

An error was encountered: "Error GET-ing user: Unknown error sending verification text"

In the logs this comes down to these errors:

ERROR 2023-11-08T16:37:32.639 [qtp1909587988-50] o.o.m.utils.NotificationUtils(NotificationUtils.java:181)  Could not send SMS verification
java.lang.NullPointerException: Cannot invoke "String.hashCode()" because "<local1>" is null
	at org.opentripplanner.middleware.utils.NotificationUtils.getTwilioLocale(NotificationUtils.java:153)
	at org.opentripplanner.middleware.utils.NotificationUtils.sendVerificationText(NotificationUtils.java:175)
	at org.opentripplanner.middleware.controllers.api.OtpUserController.sendVerificationText(OtpUserController.java:105)

My locale within Java is null and the switch/default fails on null in my JRE (openjdk-20). I dug into this and apparently there are plans to allow null in a switch statement in Java 21, but it's not there yet. I made a code change to get past this, and it worked, please let me know whether it's acceptable.

@JymDyerIBI
Copy link
Contributor

JymDyerIBI commented Nov 9, 2023

I should add that I changed the settings on the front end (🌎 -> Français) and the website changes immediately, but the first time I did this it did not update preferredLocale in Mongo right away. Which may be why it was null my first time through, and if I remove that field in Mongo it is null again.

After that, it updates fairly quickly back and forth between the front end and Mongo each time I change the language from the 🌎 button.

@binh-dam-ibigroup
Copy link
Collaborator Author

There are plans to allow null in a switch statement in Java 21, but it's not there yet. I made a code change to get past this, and it worked, please let me know whether it's acceptable.

@JymDyerIBI Good catch, thanks for the fix!

Copy link
Contributor

@JymDyerIBI JymDyerIBI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Looks good.

Copy link
Contributor

@AdrianaCeric AdrianaCeric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, although anything but french and spanish falls back to english for me on ST-QA. Not sure if this is what other reviewers notice?
image

@binh-dam-ibigroup
Copy link
Collaborator Author

@amy-corson-ibigroup Would you mind seeing if you encounter the issues that @AdrianaCeric encountered on her local setup?

@JymDyerIBI
Copy link
Contributor

@AdrianaCeric - I did not have that happen to me, I set it to French and after a brief lag the notifications were French and stayed French. I was running my own otp-middleware instance with its own mongodb, though.

I wonder whether there's an issue with st and st-qa both calling the same push notifications middleware?

@binh-dam-ibigroup
Copy link
Collaborator Author

I wonder whether there's an issue with st and st-qa both calling the same push notifications middleware?

@JymDyerIBI This PR is about the Twilio SMS service and does not deal with the push notifications middleware.

@JymDyerIBI
Copy link
Contributor

JymDyerIBI commented Nov 15, 2023

Oops. Twilio's got separate instances and I see I was using the right one in my config when I tested.

Copy link
Contributor

@amy-corson-ibigroup amy-corson-ibigroup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested French, Spanish, Tagalog, Korean, and Vietnamese with no issues!

@binh-dam-ibigroup
Copy link
Collaborator Author

I tested French, Spanish, Tagalog, Korean, and Vietnamese with no issues!

Thank you @amy-corson-ibigroup for checking!

@binh-dam-ibigroup binh-dam-ibigroup merged commit 32f80b6 into dev Nov 15, 2023
2 checks passed
@binh-dam-ibigroup binh-dam-ibigroup deleted the sms-verif-locale branch November 15, 2023 21:59
@AdrianaCeric
Copy link
Contributor

Hm there may have been something weird going on on my end haha

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants