[CP Staging] add validate code modal#48628
Conversation
|
Thanks. Gonna pull this branch and test a few things locally. |
|
@marcaaron One of you needs to copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
|
@getusha I also replaced |
mountiny
left a comment
There was a problem hiding this comment.
Looking to test this also on the adhoc build
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
|
@marcaaron @hungvu193 I have tested this, and it works 🎉 not add recording because, duh, but the validation seems to pass for any validation code now. Is that expected for now? @marcaaron |
|
It's the same for Adding new contact method 🤔 we will always navigate back to contact method list after entering magic code, but not sure we should also do it here. |
|
Yep, to clarify, that's expected. We will switch it on in https://github.com/Expensify/Web-Secure/pull/2740 and be able to QA this properly before shipping the change. Then we can force users to upgrade to the next version around the same time as deploying Web-Secure. Since @getusha hasn't reviewed yet I'll go ahead and do the checklist on this one. |
|
I think we still need copy to be confirmed as well? Mentioned it here. I'll add a label to get some help with that. |
|
And also the copy changes requested here. |
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
Co-authored-by: Vit Horacek <36083550+mountiny@users.noreply.github.com>
…93/App into feat/validate-code-modal
|
Sure. I've just addressed all the comments. |
|
Requested a translation on slack: |
|
Let's CP this one? |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
add validate code modal (cherry picked from commit aaa60b9) (CP triggered by mountiny)
| validatePendingAction={pendingContactAction?.pendingFields?.actionVerified} | ||
| validateError={validateLoginError} | ||
| handleSubmitForm={addNewContactMethod} | ||
| clearError={() => User.clearContactMethodErrors(contactMethod, 'validateLogin')} |
There was a problem hiding this comment.
Hi, @hungvu193, If I register with a phone number, when I add a new contact method, this clearContactMethodErrors will add a new record into the loginList. Is this behavior intentional? (because the value of account.primaryLogin is the phone number at that point.)

There was a problem hiding this comment.
Oh wait. I think I misunderstood the question. I'll need my laptop to take a look 🥲
There was a problem hiding this comment.
Eh, What I'm confused about is: the new contact I actually wanted to add is 2471314+21@gmail.com, but it seems that clearContactMethodErrors unexpectedly added a record with +13393011884 to the loginList. 😂
There was a problem hiding this comment.
Yeap that's pretty weird. I think it shouldn't add a new contact method into loginList but let me grab my laptop and see what I find.
There was a problem hiding this comment.
I'm trying to signup with phone number but I can't receive the magic code :( . Can you check the verbose to see where does that contact method come from? (Probably Pusher event).
There was a problem hiding this comment.
test.mp4
App/src/pages/settings/Profile/Contacts/NewContactMethodPage.tsx
Lines 150 to 152 in b7c0055
validateLoginError is empty object, so when BaseValidateCodeForm is mounted, clearError (i.e., clearContactMethodErrors) is automatically called. And because the value of contactMethod (i.e., account.primaryLogin) is +13393011884, clearContactMethodErrors adds that phone number to the loginList instead of using the existing +13393011884@expensify.sms key.
BTW, I don't have a phone number either, just generated some randomly with this url, and no magic code is required when signing up with a new phone number. 😂
There was a problem hiding this comment.
I see. So I think we should check _isEmpty(validateError) instead of using !validateError. We will fix it in this PR
There was a problem hiding this comment.
I'm afraid that's not enough, clearError is also called when the modal is closed.
Lines 285 to 288 in 8d645bf
Perhaps we need to ensure that the
contactMethod string contains the SMS domain.account.primaryLogin is just a phone number +13393011884, while the correct key in loginList is +13393011884@expensify.sms.We have a function
addSMSDomainIfPhoneNumber that might help, which can add the SMS domain to the phone number. :)
There was a problem hiding this comment.
Thanks for your advice 💯 . I'll keep that in mind.
|
This issue was missed while implementing the feature. Issue: Copilot - Clicking outside magic code RHP does not dismiss the RHP entirely Steps to reprodue:
|
| onBackButtonPress={hide} | ||
| /> | ||
|
|
||
| <View style={[themeStyles.ph5, themeStyles.mt3, themeStyles.mb7]}> |
There was a problem hiding this comment.
themeStyles.mb7 was applied to the ValidateCodeForm container which has 28px of margin, this caused inconsistency in padding between the validate code and issue expensify cards flow, which caused #53583
| /** | ||
| * Check that all the form fields are valid, then trigger the submit callback | ||
| */ | ||
| const validateAndSubmitForm = useCallback(() => { |
There was a problem hiding this comment.
Coming from #54009, we should show the error when validateAndSubmitForm is triggered.
|
|
||
| if (validateError) { | ||
| clearError(); | ||
| User.clearValidateCodeActionError('actionVerified'); |
There was a problem hiding this comment.
We should have also cleared this when user has actionVerified error as improvement. #55490
| onBackButtonPress={hide} | ||
| /> | ||
|
|
||
| <View style={[themeStyles.ph5, themeStyles.mt3, themeStyles.mb7]}> |
There was a problem hiding this comment.
The page should have been wrapped in , This cause a regression here #56818
| if (!hasMagicCodeBeenSent) { | ||
| return; | ||
| } | ||
| inputValidateCodeRef.current?.clear(); |
There was a problem hiding this comment.
{BZ CHECKLIST} inputValidateCodeRef.current?.clear() was set too early, during the RHP animation. which caused this minor transition flickering issue #73030
We recently need a reusable component to verify the magic code, this PR introduces
ValidateCodeActionModalwhich is a modal that includes a magic code input that can be reused between flows.Details
Fixed Issues
$ #48541
PROPOSAL: N/A
Tests
For Card reveal details:
Prerequisite: Your account has an Expensify physical card setup
Reveal detailsbutton.For adding new contact method:
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop