Skip to content
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.

Sign up for GitHub

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

Jump to bottom

LoginWithShortLivedTokenPage needs improvement #8229

Closed
marcaaron opened this issue Mar 18, 2022 · 6 comments
Closed

LoginWithShortLivedTokenPage needs improvement #8229

marcaaron opened this issue Mar 18, 2022 · 6 comments
Assignees
@neil-marcellini
Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2

Comments

@marcaaron
Copy link
Contributor

Problem

The transition page logic feels like it is a bit brittle and difficult to understand. There are not obvious side effects like performing some actions in componentDidUpdate() which will only happen if we connect to the betas because they load and update via AuthScreens. Logic like that is pretty hard to follow and therefore easy to break.

Why is this important?

This flow is critical to allowing users to move smoothly from OldDot to NewDot.

Solution

Think about how to improve the code (refactor), cover, and re-test the various scenarios. Maybe write automated tests for this flow if at all possible (not sure if it is).
@marcaaron marcaaron added Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff labels Mar 18, 2022
@marcaaron marcaaron self-assigned this Mar 18, 2022
@marcaaron marcaaron changed the title LoginWithShortLivedAuthToken needs improvement LoginWithShortLivedTokenPage needs improvement Mar 18, 2022
@MelvinBot MelvinBot added the Monthly KSv2 label Mar 21, 2022
@melvin-bot melvin-bot bot added the Overdue label Apr 21, 2022
@marcaaron marcaaron added Reviewing Has a PR in review and removed Reviewing Has a PR in review labels Apr 25, 2022
@melvin-bot melvin-bot bot removed the Overdue label Apr 25, 2022
@marcaaron marcaaron removed their assignment Apr 25, 2022
@marcaaron
Copy link
Contributor Author

I know @neil-marcellini is looking into fixes for various issues.
I'm not sure if we're doing a total refactor of the code at this point though.

@neil-marcellini
Copy link
Contributor

I was able to get all of the flows to work on dev. I have this PR up for review now, Prevent "Continue setup" navigating back immediately and these two PRs which will follow to complete the fix on the feature branch.

[HOLD 8498] Only create 1 free workspace when transitioning
[HOLD 8611] Wait for Onyx to clear before signing in the transitioning user

@michaelhaxhiu
Copy link
Contributor

@neil-marcellini are you intending to fix this issue? If so, want to self assign?

@neil-marcellini
Copy link
Contributor

Oh yeah, thanks @michaelhaxhiu. I'm waiting on 1 more approval for this PR and then it will be fixed #8855.

@neil-marcellini neil-marcellini self-assigned this Jun 2, 2022
@neil-marcellini
Copy link
Contributor

#8855 was merged, so I'm closing this.

@marcaaron
Copy link
Contributor Author

Awesome! Thanks for all your hard work and patience with this one @neil-marcellini

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@neil-marcellini neil-marcellini

Labels
Engineering Improvement Item broken or needs improvement. Internal Requires API changes or must be handled by Expensify staff Monthly KSv2
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@MelvinBot @michaelhaxhiu @neil-marcellini @marcaaron