Closed andcarls closed 7 months ago
New commits should address above reviews. Curious on your thoughts on the button styles for requesting and verifying/submitting OTP. I chose a secondary, dark gray button for requesting OTP, and a primary, blue button (same as old save button) for submitting.
Thanks! @andcarls. I need to check the button design with @HemanginiShah and @jonespm, too
I believe in this app variables can be passed up through the spa_globals variable which is set on the backend. I'd be fine if you set it like this or even hardcoded it and created a new issue to improve the config in the future if you couldn't get it working.
On Fri, Apr 12, 2024, 6:03 PM Andrew Carlson @.***> wrote:
@.**** commented on this pull request.
In src/assets/src/components/preferences.tsx https://github.com/tl-its-umich-edu/remote-office-hours-queue/pull/474#discussion_r1563265467 :
@@ -69,25 +78,90 @@ function PreferencesEditor(props: PreferencesEditorProps) { label="As a host, I want to be notified via SMS when someone joins my empty queue." /> );
- const validateAndSubmit = (e: React.SyntheticEvent) => {
- const oneTimePasswordTimer = () => {
- let timer = process.env.REACT_APP_OTP_TIMER ? parseInt(process.env.REACT_APP_OTP_TIMER) : 30;
Hmm, it appears that the error thrown is that "process" doesn't exist. I did some digging, and found that it is a bit more of a complex process to pass environment variables to React using webpack, requiring modifying the webpack scripts (in package.json) and the likely use of external modules (see Dotenv https://www.npmjs.com/package/dotenv). I tried implementing some of the solutions I found online, but keep running into errors. Especially, for some odd reason, that webpack can't find the Dotenv module even after installation.
A simple solution would be to modify webpack.config.js to include:
plugins: [ ..., new webpack.DefinePlugin({ 'process.env': { REACT_APP_OTP_TIMER: JSON.stringify(30), } }) ]
Only changes a few lines and confirmed to work, but a bit less useful than a .env file.
I have never had to forward env variables to the frontend through webpack, and am running into weird errors locally. Any suggestions or does anyone have any insights on how to do this?
— Reply to this email directly, view it on GitHub https://github.com/tl-its-umich-edu/remote-office-hours-queue/pull/474#discussion_r1563265467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAGWN6B2WI5ASJ6CLFNCMLY5BKZFAVCNFSM6AAAAABFZ5WPC6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTSOJYGUYDMMRYHE . You are receiving this because you were mentioned.Message ID: <tl-its-umich-edu/remote-office-hours-queue/pull/474/review/1998506289@ github.com>
I agree. If you cannot get the spa_globals approach to work, then you can switch back to the hard coded value for this pull request. Thanks!
Got it to work with the spa_globals approach. Added it to the Page props that are passed to all pages for consistency & maintainability. Let me know if you would rather it be passed down explicitly, but it may be a bit messier and harder to understand in future.
The spa_globals approach works. We will keep the approach for now.
Digit-by-digit input added & updated PR screenshots. Autofocuses on first input box, moves focus forward/backward between boxes on input/deletion, and enter key submits OTP. Let me know your thoughts.
@andcarls: great work!
I just have one more comment: When user clicks on the edit link to enter new phone number, and click on the button to generate new OTP. The page refreshes but with the last used OTP numbers. Please reset the OTP number (to empty value) after (1) success verification; (2) failed verification; or (3) every time when user starts with a new phone number.
Done: commit is above your comment; github glitched out in syncing
Nice job! I like the screenshots for opt-in for text message and one time verification.
@zqian thanks! Not sure if this message is intended for me, but I don't have write access to the repo, so I can't merge.
@andcarls No Problem! I will do the merge!
Hello Andrew, I don't see any validation link when entered the phone. Please see the attached image from qa.remoteo Kindly help me, I would be happy to schedule a zoom call.
Hi Hema,
From what I can see on the QA site, none of the recent code changes have been deployed to the QA deployment yet, including mine and #479. I'm unsure about the process for deploying to QA, maybe @zqian or @jonespm know/have access?
OK Thanks, I will talk to them during our meeting today at 2:30 pm.
*With Warm Regards,* https://www.name-coach.com/hemangini-shah
Hemangini Shah https://www.name-coach.com/hemangini-shah, PMP , BI Analyst Senior, ITS
She/Her/Hers
On Wed, Apr 24, 2024 at 2:24 PM Andrew Carlson @.***> wrote:
Hi Hema,
From what I can see on the QA site, none of the recent code changes have been deployed to the QA deployment yet, including mine and #479 https://github.com/tl-its-umich-edu/remote-office-hours-queue/pull/479. I'm unsure about the process for deploying to QA, maybe @zqian https://github.com/zqian or @jonespm https://github.com/jonespm know/have access?
— Reply to this email directly, view it on GitHub https://github.com/tl-its-umich-edu/remote-office-hours-queue/pull/474#issuecomment-2075566401, or unsubscribe https://github.com/notifications/unsubscribe-auth/BFEK2QEBXCADBEONHL3JZXLY672EVAVCNFSM6AAAAABFZ5WPC6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDANZVGU3DMNBQGE . You are receiving this because you commented.Message ID: @.*** com>
Yes this app isn't auto deployed. We built and deployed it and found a few issues with testing that we're going to create new issues for.
FYI - Ask Chatgpt to provide test cases for the telephone number validation. Response below: When creating test cases for telephone number validation, it's essential to cover a variety of scenarios to ensure the validation function works correctly in different situations. Here are some common test cases:
123-456-7890
(123) 456-7890
123.456.7890
+1-123-456-7890
1234567890
1-123-456-7890
123-456-789
123-456-78901
123-ABC-7890
123-456-789@
123) 456-7890
456-7890
123.456-7890
123-456-7890
-123-456-7890
+
123456
+1+123-456-7890
123--456-7890
+1234567890
+
sign should be valid or invalid.By covering these test cases, you can ensure robust telephone number validation that can handle a wide range of inputs.
PR Summary
Resolves feature request in #391. Previous PR #406 utilized Twilio Lookup, but did not verify phone numbers or implement UI. PR includes functionality for users to enter a phone number, receive a text-based OTP, and verify/update their phone number. Options to resend code (with 30s buffer to prevent abuse), go back available.
Notes
/api/users/<userid>/otp/
for sending and verifying OTP/updating phone # through above viewScreenshots