-
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
Handle magic links for fresh sessions #19819
Conversation
@hayata-suenaga @abdulrahuman5196 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] |
Reviewer Checklist
Screenshots/VideosWebUntitled.39.mp4Mobile Web - Chromeaz_recorder_20230602_005517_edited.mp4Mobile Web - SafariScreenRecording.movDesktopUntitled.40.mp4iOSUntitled.38.mp4Androidaz_recorder_20230602_004432.mp4 |
@jjcoffee For me in native android, its still going to password screen if I directly use the magic link without entering the email id in mobile app. |
@abdulrahuman5196 I am aware of that, that's why the testing steps are different for mobile - this is a web-only change (as proposed). I don't think there's a strong case for opening a magic link on mobile with a signed-out user on the app? Especially since there is no existing support for displaying the "just sign in here" link on native mobile apps (the interstitial screen doesn't display at all), so it doesn't seem to be supported behaviour. It is possible to adjust the native behaviour to apply the same fix, but it would not display an interstitial (with the "just sign in here" link), just immediately log in. What do you think? |
@abdulrahuman5196 Just to clarify in case you want to test, if we want to adjust the native behaviour, we'd need to update the check here: To this: We need to add the credentials to the Let me know what you think! |
@jjcoffee #19819 (comment) |
@abdulrahuman5196 Yes it will fix the native behaviour you described. I wasn't sure if you want to apply the fix since the original proposal was only for web behaviour. Shall I just push the fix do you think? |
@abdulrahuman5196 I've just pushed the fix for native behaviour, hope that's okay! |
Thanks @jjcoffee, had a quick check and it seems to work. Could you update your author's checklist native platform videos on the same. I will continue review in one hour. |
@jjcoffee Can you update the test in author checklist to reflect the mobile native fix as well? |
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.
Changes look's good and works well. Added Reviewers checklist as well.
All yours
@hayata-suenaga
|
@abdulrahuman5196 Wasn't sure if it was still needed, but I've retested and updated the native videos now. Sorry for the delay! |
@jjcoffee Meanwhile could you update the comments as alone |
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.
LGTM as long as @abdulrahuman5196's suggestions are addressed 👍
@jjcoffee Could you address the comment suggestions? We can close this out then? |
@hayata-suenaga Comments addressed! |
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.
@hayata-suenaga I think you should be good to merge
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 Cherry-picked to staging by https://github.com/hayata-suenaga in version: 1.3.24-5 🚀
@Expensify/applauseleads please QA this PR and check it off on the deploy checklist if it passes. |
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.24-5 🚀
|
🚀 Deployed to production by https://github.com/roryabraham in version: 1.3.24-5 🚀
|
Details
On web we have the logic to sign in on a fresh session/different device with a "just sign in here" link, but with fresh sessions we don't have the user's permissions to the passwordless beta available, so opening magic links will always run the legacy validation (for the password-based flow) which breaks logging in for passwordless users.
This PR modifies this behaviour so that on web, the legacy validation only runs if the user credentials have been loaded (which indicates the betas will also be available).
Fixed Issues
$ #19214
PROPOSAL: #19214 (comment)
Tests
On web and mobile web only (which tests the actual change):
For native and desktop apps, you can also verify that the normal sign in flow still works:
Offline tests
N/A
QA Steps
Same as above.
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.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/Videos
Web
19214-magic-link-fresh-session-desktop-chrome.mp4
Mobile Web - Chrome
19214-magic-link-fresh-session-android-chrome.mp4
Mobile Web - Safari
19214-magic-link-fresh-session-ios-safari.mp4
Desktop
19214-magic-link-fresh-session-macos-desktop.mp4
iOS
19214-magic-link-fresh-session-ios-native.mp4
Android
19214-magic-link-fresh-session-android-native.mp4