-
Notifications
You must be signed in to change notification settings - Fork 3.5k
Also check for shortLivedToken when signing in with shortLivedAuthToken #14045
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
Conversation
|
@aimane-chnaif @stitesExpensify 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] |
NikkiWines
left a 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.
Aside from the lint error this looks good. @alex-mechler since we're doing an oldApp release fairly soon, how do you feel about updating this in Mobile-Expensify so that we can eventually only have to check on value, not both?
|
Updated to fix the lint error.
Is the new mobile release going to force people to update? If it does not, people still might be using the old version of the app, in which case we will still have to support both |
|
Fair... eventually yes they will be forced to update because we will move to passwordless authentication across the board. |
|
Fair enough, we can update it there, since we are releasing anyways. Might have to keep an eye on adoption numbers before we pull the plug on checking both, as people still signed in to the app won't have to update. It wouldn't hurt to update there, I'll get a PR up |
|
The fix is clear to me and tests well, since I am already familiar with this while reviewing #13456
Nevertheless, this PR should be merged to support current version of OldDot mobile app, right? |
aimane-chnaif
left a 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.
Should shortLivedToken be added in propTypes as well?
App/src/pages/LogInWithShortLivedAuthTokenPage.js
Lines 11 to 17 in c166a44
| params: PropTypes.shape({ | |
| /** Short lived authToken to sign in a user */ | |
| shortLivedAuthToken: PropTypes.string, | |
| /** The email of the transitioning user */ | |
| email: PropTypes.string, | |
| }), |
Correct, this needs to be merged so that the oldApp transitions are fixed for now, as we're not quite sure yet when the next update for oldApp will be |
|
Yup, we still do want this to support old versions of the mobile app. Added proptypes |
NikkiWines
left a 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.
![]()
Reviewer Checklist
Screenshots/VideosWeb2023-01-09_14-40-38.mp4Mobile Web - Chrome2023-01-09_15-13-54.mp4Mobile Web - Safari2023-01-09_15-14-31.mp4Desktopn/a iOSn/a Androidn/a |
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
|
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
|
🚀 Deployed to staging by @stitesExpensify in version: 1.2.52-0 🚀
|
|
🚀 Deployed to production by @Julesssss in version: 1.2.52-4 🚀
|
cc @NikkiWines
Fixed Issues
$ #13894
PROPOSAL: N/a
Tests
I couldn't directly test this flow locally, as I cannot build the old app locally. I tested that the flow still works by updating the parameter name in the url of an old app transition, and the following steps to confirm that existing transitions still work.
Desktop web
MWeb
Set up my company for freeOffline tests
N/a, can only transition online as it logs you in
QA Steps
Use Staging Serverswitch is onPR 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.Screenshots/Videos
Web
2023-01-05_16-16-40.mp4
Mobile Web - Chrome
2023-01-05_16-27-20.mp4
Mobile Web - Safari
2023-01-05_16-13-51.mp4
Desktop
N/A We do not transition into the desktop
iOS
N/A We do not transition into the mobile apps
Android
N/A We do not transition into the mobile apps