-
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
Fix validation code not sending on initial page load. #48047
Fix validation code not sending on initial page load. #48047
Conversation
@allroundexperts Please 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] |
@wildan-m there are conflicts @allroundexperts what is your eta for the review? |
…x/43359-validation-code-not-send-onPress-solution
The conflict is resolved |
…x/43359-validation-code-not-send-onPress-solution
Reviewer Checklist
Screenshots/VideosAndroid: NativeScreen.Recording.2024-09-09.at.2.06.41.AM.movAndroid: mWeb ChromeScreen.Recording.2024-09-09.at.2.03.49.AM.moviOS: NativeScreen.Recording.2024-09-09.at.2.02.09.AM.moviOS: mWeb SafariScreen.Recording.2024-09-09.at.1.59.53.AM.movMacOS: Chrome / SafariScreen.Recording.2024-09-09.at.1.53.13.AM.movMacOS: DesktopScreen.Recording.2024-09-09.at.1.55.56.AM.mov |
@wildan-m If I repeat the steps, I can get the validation messages again: Screen.Recording.2024-08-29.at.5.07.19.AM.mov |
…x/43359-validation-code-not-send-onPress-solution
@allroundexperts I added test steps 4-6 that seem reasonable, but this code intentionally resets the validateCodeSent state on each mount
Depending on the expected outcome, we can either eliminate the extra test steps or delete this code. |
@mountiny What is the expected behavior when revisiting the validation page? |
@allroundexperts @mountiny bump for confirmation. Should we re-send on each page load? |
I think @mountiny would be able to answer that. |
Hmmm, I am not sure; I don't think it is best to call it every time the page is navigated. Could we check if the Then require the user to click the request button |
@mountiny Thanks for your confirmation. in that case we can remove this useEffect. @allroundexperts the PR has been updated |
…x/43359-validation-code-not-send-onPress-solution
@wildan-m this flow was not slightly changed so we require the magic code also for the primary's login Can you retest this flow with latest main merged? |
…x/43359-validation-code-not-send-onPress-solution
@mountiny sure, I found flicker in re-sent message and noticeable in iOS, but it has been fixed. Currently conducting more tests and will update the video soon |
@mountiny @allroundexperts The video with latest main has been updated. #48047 (comment) |
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.
Why is this change needed @wildan-m? This doesn't seem to be related to your changes.
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.
@allroundexperts did you mean adding validateCodeSentIsPressedRef
? without that, the message Link has been re-sent
would be shown on the initial load.
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.
I mean all of the changes in these files. It looks to me they are not needed. How were they working previously?
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.
Before: The requestContactMethodValidateCode
function is not triggered upon the initial page load, resulting in the validateCodeSent
variable not being updated. It is only updated when the magicCodeNotReceived
button is clicked, allowing validateCodeSent
to solely indicate whether a magic code has been sent. Additionally, validateCodeSent
is reset to false each time the component is mounted.
After: The function requestContactMethodValidateCode
is invoked on each page call if validateCodeSent
is false. When this function is called, validateCodeSent
is changed to true. On the initial page load of BaseValidateCodeForm.tsx
, the message Link has been re-sent
will be displayed. We cannot rely solely on validateCodeSent
because its value is modified before the page call.
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.
…x/43359-validation-code-not-send-onPress-solution
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.
Looks good!
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.
@wildan-m @allroundexperts sorry for this taking a bit longer, the changes look good to me, we started to use the validation page for adding secondary login. Can you validate that adding secondary login works on this PR as well please?
@mountiny In my video, the secondary login test at the end seemed to work as expected. |
@wildan-m nice, I will also check the the reveal virtual card details flow works |
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪 |
@wildan-m can you check the wallet page? unfortunately its hard for you to test but I am not getting the validation code on this branch to reveal the card details. That is the Reveal details on the card page |
@mountiny there are three
We are currently focused on the second Should we include the recently added third item in this PR, even though it's a bit difficult to test? If so, how can we simplify the testing process? |
Just saw this. I think we should- both @mountiny and I can test in the adhoc builds. |
@grgia I think we should do this in a follow up, this is already fixing existing bugs. I will ask in slack |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Started a thread here |
🚀 Deployed to staging by https://github.com/mountiny in version: 9.0.36-0 🚀
|
🚀 Deployed to production by https://github.com/grgia in version: 9.0.36-2 🚀
|
Details
Fix initial code validation in contact method and 2FA screen for PR.
Fixed Issues
$ #43359
PROPOSAL: #43359 (comment)
Tests
Offline tests
QA Steps
Same as Test
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
Kapture.2024-09-05.at.11.33.49.mp4
Android: mWeb Chrome
Kapture.2024-09-05.at.11.36.52.mp4
iOS: Native
Kapture.2024-09-05.at.11.21.45.mp4
iOS: mWeb Safari
Kapture.2024-09-05.at.11.24.59.mp4
MacOS: Chrome / Safari
Kapture.2024-09-05.at.11.27.11.mp4
MacOS: Desktop
Kapture.2024-09-05.at.11.39.06.mp4