Error modal when signin from expired magic link #15505
Conversation
…switching from PublicScreens-AuthScreen
|
@cristipaval As of now |
For the flows where the user has initiated the sign in process on another device/browser client, it won't work as expected for now. But this is a known issue that we discussed internally about. Linking here a Slack discussion for reference if someone will come back to this. I think you don't have permissions to access that discussion. |
|
Alright then, thanks! |
There was a problem hiding this comment.
LGTM, Tests well ready for you @roryabraham and @youssef-lr
|
Reviewing and testing now. |
|
|
||
| render() { | ||
| const codeRequestedMessage = lodashGet(this.props, 'account.message', null); | ||
| const accountErrors = lodashGet(this.props, 'account.errors', {}); |
There was a problem hiding this comment.
NAB but I think we can simplify this part as getLatestErrorMessage already checks for keys and if it's empty it will return an empty string which should evaluate to false. no?
render() {
const codeRequestedMessage = lodashGet(this.props, 'account.message', null);
const codeRequestedErrors = ErrorUtils.getLatestErrorMessage(this.props.account);
return (
...
)
| ...withLocalizePropTypes, | ||
| }; | ||
|
|
||
| class AbracadabraModal extends PureComponent { |
There was a problem hiding this comment.
NAB but this should be a functional component w/ memo, which is typically what we do for components that don't have state or use lifecycle methods.
There was a problem hiding this comment.
This file is almost exactly the same as TfaRequiredModal, and has only minimal differences with the other two components in this directory. I think it would be valuable to DRY these up by creating a ValidateCodeModalLayout component, and use it in the other components you've created here.
There was a problem hiding this comment.
hey @roryabraham ! Thanks for the review! I do agree that we can dry up the code here. I spent a lot of time on this PR due to that challenge that I was facing (componentDidMount being called twice). I created components for each state just to simplify the code in VaidateLoginPage. But now it looks like I ended up having duplicate code. Now that I solved the aforementioned challenge, I totally do agree I can reuse the same component for multiple states. I created this follow-up issue for your suggested refactor.
|
✋ 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/cristipaval in version: 1.2.89-0 🚀
|
|
🚀 Deployed to production by https://github.com/luacmartins in version: 1.2.89-0 🚀
|
|
This PR introduced a regression: #17091 |
|
I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check. We've also updated the item in the checklist with this PR to avoid this issue in the future. |
| : <FullScreenLoadingIndicator /> | ||
| <> | ||
| {this.getAutoAuthState() === CONST.AUTO_AUTH_STATE.FAILED && <ExpiredValidateCodeModal />} | ||
| {this.getAutoAuthState() === CONST.AUTO_AUTH_STATE.JUST_SIGNED_IN && (!isTfaRequired || isSignedIn) && <AbracadabraModal />} |
There was a problem hiding this comment.
This caused this bug: #41772. More details are here: #41772 (comment). TL;DR We should also use credentials.login in this condition to show AbracadabraModal only for a session that initiated sign-in.
|
Looks like we missed a case where autoAuthState does not reset for the second magic link. #47883 |
Details
!!! Web only on macos and mobile web Safari on iPhone and Chrome on Android
Fixed Issues
$ #15331
Tests
Sign in process initiated in another tab, on the same browser:
Resend codeoption and then click on the new magic linkSign in process initiated on another device
Permissions.jsto always returntrueincanUsePasswordlessLoginsfunction, hereOffline tests
N/A
QA Steps
Same steps from Tests, but only the section with the sign in initiated in the same browser.
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)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
Web
Sign in initiated on the same browserweb.mov
Sign in process initiated on another device
web.mov
Mobile Web - Chrome
Sign in initiated on the same browserandroid.mov
Sign in process initiated on another device
android.mov
Mobile Web - Safari
Sign in initiated on the same browserios.mov
Sign in process initiated on another device
ios.mov
Desktop
N/A
iOS
N/A
Android
N/A