-
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: Entering the incorrect magic code spins the 'sign-in' button continuously offline #18960
Conversation
@puneetlath @Santhosh-Sellavel 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] |
I couldn't record offline mode with ios simulator, it required native device. |
@hungvu193 Please resolve conflicts |
@Santhosh-Sellavel I've just resolved conflict! |
src/components/MagicCodeInput.js
Outdated
if (!isReadyToSubmit()) { | ||
return; | ||
} | ||
inputRefs.current[editIndex].blur(); | ||
setFocusedIndex(undefined); | ||
props.onFulfill(props.value); |
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.
The whole block is redundant I mentioned it here already. Check lines 133 to 138 can we this into a function. isReadyToSubmit
is not need can be moved in the new method. Ex: validateAndSubmit
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.
@Santhosh-Sellavel Agreed. I've just resolved it now
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.
That's fast 🚀
src/components/MagicCodeInput.js
Outdated
@@ -255,6 +277,10 @@ function MagicCodeInput(props) { | |||
setEditIndex(newFocusedIndex); | |||
inputRefs.current[newFocusedIndex].focus(); | |||
} else if (keyValue === 'Enter') { | |||
// we shouldn also prevent user from submit here when it's offline. |
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.
// we shouldn also prevent user from submit here when it's offline. | |
//We should prevent users from submitting when it's offline. |
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.
+1
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.
My bad, just updated, Thank you @puneetlath
Reviewer Checklist
Screenshots/VideosWeb & DesktopScreen.Recording.2023-05-19.at.1.15.20.AM.movMobile Web - ChromeScreen_Recording_20230519_012230_Chrome.mp4AndroidScreen_Recording_20230519_013749_New.Expensify.mp4 |
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.
@puneetlath Looks good & tests well, just waiting for this NAB
cc: @hungvu193
src/components/MagicCodeInput.js
Outdated
@@ -255,6 +277,10 @@ function MagicCodeInput(props) { | |||
setEditIndex(newFocusedIndex); | |||
inputRefs.current[newFocusedIndex].focus(); | |||
} else if (keyValue === 'Enter') { | |||
// we shouldn also prevent user from submit here when it's offline. |
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.
+1
✋ 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/puneetlath in version: 1.3.17-0 🚀
|
🚀 Deployed to production by https://github.com/chiragsalian in version: 1.3.17-5 🚀
|
useOnNetworkReconnect(validateAndSubmit); | ||
|
||
useEffect(() => { | ||
validateAndSubmit(); |
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.
Hi, just dropping a note that this PR might have missed a case, causing the bug - "Incorrect magic code" error if click "Go back" in 2FA input page
Explaination of the problem - #19712 (comment)
Details
Fix: Entering the incorrect magic code spins the 'sign-in' button continuously offline
Fixed Issues
$ #18311
PROPOSAL: #18311 (comment)
Tests
Offline tests
Bug is related to offline mode itself. No extra steps needed.
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)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
Web
chrome.mov
Mobile Web - Chrome
Screen.Recording.2023-05-15.at.23.39.04.mov
Mobile Web - Safari
RPReplay_Final1684168578.mov
Desktop
Screen.Recording.2023-05-15.at.23.31.01.mov
iOS
Android
Screen.Recording.2023-05-16.at.09.57.35.mov