Bring back new SignInPage#65178
Conversation
|
|
|
Diff will get better once we merge #64786 |
Julesssss
left a comment
There was a problem hiding this comment.
Holding on wider HybridApp signin changes
|
Hey @mateuuszzzzz could you merge main please as we have merged the sign in changes now. |
SignInPageSignInPage
src/Expensify.tsx
Outdated
| const isSplashVisible = splashScreenState === CONST.BOOT_SPLASH_STATE.VISIBLE; | ||
| const isHybridAppReady = splashScreenState === CONST.BOOT_SPLASH_STATE.READY_TO_BE_HIDDEN && isAuthenticated; | ||
| const shouldHideSplash = shouldInit && (CONFIG.IS_HYBRID_APP ? isHybridAppReady : isSplashVisible); | ||
| const shouldInit = isNavigationReady && hasAttemptedToOpenPublicRoom && !!preferredLocale && (CONFIG.IS_HYBRID_APP ? !hybridApp?.loggedOutFromOldDot : true); |
There was a problem hiding this comment.
I'd check if that condition is correct. After our changes, we say that the app shouldn't be init if we logged out from OldDot, right?
src/Expensify.tsx
Outdated
| const shouldHideSplash = | ||
| shouldInit && | ||
| (CONFIG.IS_HYBRID_APP | ||
| ? splashScreenState === CONST.BOOT_SPLASH_STATE.READY_TO_BE_HIDDEN && (isAuthenticated || !!hybridApp?.useNewDotSignInPage) | ||
| : splashScreenState === CONST.BOOT_SPLASH_STATE.VISIBLE); |
There was a problem hiding this comment.
Let's split this logic as it's split on main currently
src/libs/actions/Session/index.ts
Outdated
| authPromiseResolver = null; | ||
| } | ||
| if (CONFIG.IS_HYBRID_APP && session.authToken && session.authToken !== INVALID_TOKEN) { | ||
| if (CONFIG.IS_HYBRID_APP && session.authToken && session.authToken !== INVALID_TOKEN && isSetUpReady) { |
There was a problem hiding this comment.
Let's check if this additional value is still necessary
There was a problem hiding this comment.
it is :/ Without it we would send old auth token to OD when we sign out from it
src/pages/signin/SignInPage.tsx
Outdated
| useEffect(() => { | ||
| if (!CONFIG.IS_HYBRID_APP || !hybridApp?.loggedOutFromOldDot) { | ||
| return; | ||
| } | ||
| HybridAppModule.clearOldDotAfterSignOut(); | ||
| }, [hybridApp?.loggedOutFromOldDot]); |
There was a problem hiding this comment.
Do we really have to do this in useEffect? 🤔
If we decide to change loggedOutFromOldDot in some other place, this logic will fire :/
How about putting this in Session/index.ts in setupNewDotAfterTransitionFromOldDot?
cc @mateuuszzzzz for thoughts
There was a problem hiding this comment.
I added comment expalining why we do it here
# Conflicts: # src/components/BookTravelButton.tsx # src/pages/signin/SAMLSignInPage/index.native.tsx
…eload-on-new-sign-in-page # Conflicts: # modules/hybrid-app/android/src/main/java/com/expensify/reactnativehybridapp/ReactNativeHybridApp.kt # modules/hybrid-app/ios/ReactNativeHybridApp.mm # modules/hybrid-app/src/NativeReactNativeHybridApp.ts # modules/hybrid-app/src/index.native.ts # modules/hybrid-app/src/index.ts # modules/hybrid-app/src/types.ts # src/App.tsx
|
🚧 @mountiny has triggered a test Expensify/App build. You can view the workflow run here. |
|
🧪🧪 Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! 🧪🧪
|
|
Bugs summary:
|
|
Thanks Marcin for fixing the second issue. So yeah the Google SSO on AdHoc was expected, I broke that while fixing Google SSO on prod and this shouldn't block us. |
|
The ESLint warning will also be left as-is. This rule will be applied with later, for now we need to complete the auth page switch. |
|
@Julesssss looks like this was merged without a test passing. Please add a note explaining why this was done and remove the |
See here. |
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 9.1.89-1 🚀
|
|
InternalQA
FYI @war-in for tomorrow |
|
🚀 Deployed to production by https://github.com/jasperhuangg in version: 9.1.89-21 🚀
|
Explanation of Change
Fixed Issues
$ #49844
MOBILE-EXPENSIFY: https://github.com/Expensify/Mobile-Expensify/pull/13607#pullrequestreview-2972319319
Tests
Please thoroughly test these cases, these changes have far reaching changes to authentication.
Base information to every flow:
Redirection tests apply to HybridApp, for web you should remain on the current URL
The flow is based on NVP
tryNewDotvalue, we have two possible scenarios to testtryNewDotis properly setdismissed==false)Switch to Expensify Classicbuttondismissed==true)tryNewDotisundefined. This state can be achieved by having old classic account or manually settingawait NVP.set("tryNewDot", undefined);in Expenisfy classic on web.Try New Expensifybutton is no visibleMain flow:
Main flow new Account:
SAML Magic code
Use magic codeSAML single sign on
single sign-onDomain-based login flow test
Google sign-in Apple sign-in:
2FA:
Travel, OD <-> ND switch etc
Verify that:
tryNewDotcan transition between OD and ND.In addition, please run through the new test cases that were previously created for this feature.
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectioncanBeMissingparam foruseOnyxtoggleReportand notonIconClick)src/languages/*files and using the translation methodSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)Designlabel and/or tagged@Expensify/designso the design team can review the changes.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
Android: Native
android.mov
Android: mWeb Chrome
iOS: Native
ScreenRecording_07-02-2025.12-12-07_1.MP4
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop