-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat: validate new contact method action #48320
feat: validate new contact method action #48320
Conversation
src/libs/Navigation/AppNavigator/ModalStackNavigators/index.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Profile/Contacts/ValidateCodeForm/BaseValidateCodeForm.tsx
Outdated
Show resolved
Hide resolved
src/pages/settings/Profile/Contacts/ValidateCodeForm/BaseValidateCodeForm.tsx
Outdated
Show resolved
Hide resolved
Thanks for assigning. I'll review this shortly |
Bug 1: User can spam Screen.Recording.2024-08-30.at.21.33.22.mov |
Same as |
Bug 2: If you added a contact method before but not enter the magic code then exist the page, later when you press Screen.Recording.2024-08-30.at.21.38.43.mov |
I've never received any magic code even when I requested/ resent few times. Did you guys receive magic code input after |
Bug 4: Click on close icon does not clear the error message. Screen.Recording.2024-08-30.at.21.52.12.movWe probably also want to clear the error when clicking |
Bug 5: If you interrupt the api called on Screen.Recording.2024-08-30.at.21.55.14.mov |
Oh shoot :/ I got blocked because I requested so many times 😭 . I'll wait for a while then continue with my testing 😈 |
I use a different email when that happens 😄 |
Bug 6: New contact method page shows errors even there's no error. Steps :
Screen.Recording.2024-08-30.at.22.28.39.mov |
@hungvu193 could you recheck ^ bugs, now? thanks! |
Sure |
I still can reproduce the RBR bug by trying to request magic code few time at Screen.Recording.2024-08-30.at.23.02.08.mov |
Navigation.navigate(ROUTES.SETTINGS_CONTACT_METHODS.route); | ||
}, [loginData, loginData?.pendingFields, loginList]); | ||
|
||
useEffect(() => () => User.clearUnvalidatedNewContactMethodAction(), []); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Once user existed the page, we should also clear the error (RBR) as well.
Let's also add User.clearUnvalidatedNewContactMethodAction()
as the return function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which function are you referring?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry. I mean the cleanup function of this useEffect.
@hungvu193 this is what i see trying to reproduce bug 5 Screen.Recording.2024-08-31.at.10.12.14.in.the.morning.mov |
Oh weird, after loging out and login again, I can't reproduce anymore :) |
Reviewer Checklist
Screenshots/VideosAndroid: mWeb ChromeiOS: Native// native error iOS: mWeb SafariScreen.Recording.2024-08-31.at.15.08.18.movMacOS: Chrome / SafariScreen.Recording.2024-08-31.at.14.32.03.mov |
On mSafari, after entering the magic code, it still stay on Screen.Recording.2024-08-31.at.14.57.38.mov |
@hungvu193 could you logout & sign in again then retest? Screen.Recording.2024-08-31.at.11.03.42.in.the.morning.mov |
I actually logged out before testing, but let me try again. |
Screen.Recording.2024-08-31.at.15.06.29.mov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All yours @mountiny
🎯 @hungvu193, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #48374. |
Thanks for the hustle, I will try to test with the backend changes later today |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@getusha Thank you!
I think we will need to add some clean ups but for the sake of the urgency I think this is good to go and I would like to merge it before the deploy so we have it in staging tomorrow to unblock the backend changes
@getusha Could you look into the fixing the navigation when the user removes the optimistic login? The page goes to not found page because the contact method is in the URL. I think we need to redirect to the contact method list page
Screen.Recording.2024-09-02.at.22.00.17.mp4
/** | ||
* Clears a contact method optimistically. this is used when the contact method fails to be added to the backend | ||
*/ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.28-0 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 9.0.28-3 🚀
|
useEffect(() => { | ||
if (!pendingContactAction?.validateCodeSent) { | ||
return; | ||
} | ||
|
||
Navigation.navigate(ROUTES.SETINGS_CONTACT_METHOD_VALIDATE_ACTION); | ||
}, [pendingContactAction]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@getusha What is the purpose for this logic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@parasharrajat We started requiring verification before adding a contact method. the new flow is temporarily save new contact > verify action > add the new contact method.
saveNewContactMethodAndRequestValidationCode
just saves the contact method temporarily and requests a magic code, so this logic just navigates youValidateContactActionPage
after the magic code is sent
Line 342 in e1d726e
validateCodeSent: true, |
Is there a design doc for this PR? @hungvu193 @getusha |
@parasharrajat there is no design doc but you can check this thread and this issue for more detail |
No design doc for this, its an urgent change we require now to validate the primary login before taking certain actions, little more details in here #48541 |
Details
Fixed Issues
$ https://github.com/Expensify/Expensify/issues/423805
PROPOSAL:
Tests
Offline tests
QA Steps
NOTE: The magic code verification does not work yet so no matter what magic code number you put in, it will succeed
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label and/or tagged@Expensify/design
so the design team can review the changes.ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Android: Native
Screen.Recording.2024-08-31.at.11.17.09.in.the.morning.mov
Android: mWeb Chrome
Screen.Recording.2024-08-30.at.7.48.30.in.the.evening.mov
iOS: Native
Screen.Recording.2024-08-30.at.7.55.19.in.the.evening.mov
iOS: mWeb Safari
Screen.Recording.2024-08-30.at.5.30.34.in.the.afternoon.mov
MacOS: Chrome / Safari
Screen.Recording.2024-08-30.at.5.17.13.in.the.afternoon.mov
MacOS: Desktop
Screen.Recording.2024-08-30.at.5.31.36.in.the.afternoon.mov