-
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
Create separate transition pages to sign the user in / out #8855
Conversation
Prevent multiple updates to the session and store the accountID as int
Prevent "Continue setup" navigating back immediately
Everyone is allowed to view this page so don't navigate away if the betas haven't loaded yet
Don't wait for betas while transitioning
I cherry picked the last commit from this one a while back. I never got a chance to explain it since I was OOO at the time. Sometimes, especially on dev, the Alternatively we could make CreateLogin idempotent and let it be retried, but I think this is simpler. Check out the logs
|
Just a heads up, we are overhauling the retry logic atm and won't ever be retrying |
Testing again.
InstructionsA. Set up my company for free
B. Select a free workspace
C. Clicking on the name of a free policy
D. Get Started Inbox Task
E. Pricing select free E.1 Creating a free policy
E.2 Navigating to an existing free policy
F. Continue setup Inbox task
|
Ok, I think it might be ready to go now. |
* | ||
* We subscribe to the session using withOnyx in the AuthScreens and | ||
* pass it in as a parameter. withOnyx guarantees that the value has been read | ||
* from Onyx because it will not render the AuthScreens until that point. |
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.
Excellent comments here.
// is rendered and the navigation state is null. We can't navigate at | ||
// that time, so we use a promise to delay transition navigation until | ||
// it is ready. We set the navigation ready here since we know that the | ||
// navigator is now rendered. |
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.
Perfect 👏
It would be good to create a follow up to explore the single navigator navigation setup we can - but I have a feeling that will be a tough refactor.
I had to fix some merge conflicts. @marcaaron @roryabraham @deetergp could you all please give a review? I would love to merge this today if I get 2 approving reviews. |
@marcaaron looks like this was merged without passing tests. Please add a note explaining why this was done and remove the Note: Not an emergency tests passed. - Marc |
😂 alright, thanks Marc! |
✋ 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 @marcaaron in version: 1.1.72-0 🚀
|
@neil-marcellini Since this PR requires transition between OldDot and NewDot, It is not possible for us to do the same steps on Desktop or Native apps. Is it ok to test this on Web and mWeb only or there any specific steps we should validate on Desktop or Native apps |
You only need to test on Web for now. |
🚀 Deployed to production by @yuwenmemon in version: 1.1.73-2 🚀
|
Details
This PR fixes all of the transition flows from OldDot to NewDot. The transition flow has been simplified by creating separate pages for when the user is signed out / in. There was a tricky bug where the transitioning user would be signed out multiple times, which was fixed by waiting for Onyx to finish clearing after signing out the previous user and before signing in the transitioning user.
Fixed Issues
$ #8295
$ #8676
$ https://github.com/Expensify/Expensify/issues/207655
Related Issues
https://github.com/Expensify/Expensify/issues/207423
https://github.com/Expensify/Expensify/issues/202627
Tests / Web QA
On dev only:
npm i
to install the latest Onyx changes, thennpm run web
@gmail.com
addressE. Pricing select free
PR Review Checklist
Contributor (PR Author) Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
)src/languages/*
filesSTYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
displayName
propertythis
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)PR Reviewer Checklist
### Fixed Issues
section aboveTests
sectionQA steps
sectiontoggleReport
and notonIconClick
).src/languages/*
filesSTYLE.md
) were followed/** comment above it */
displayName
propertythis
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)Screenshots
Web
Mobile Web
Desktop
iOS
Android