Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Restore Link Sign Up viewmodel and tests #9377
Restore Link Sign Up viewmodel and tests #9377
Changes from all commits
fee10f5
9e3be14
d5eb7e7
b6dca29
628af3d
f1eaa42
11d0d8a
88e4c02
c69c28b
efebf86
16ee79b
ceb4a54
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
(for a follow up PR) should the analytics be handled as part of link account manager instead of here? I'd expect the analytics to be the same wherever linkAccountManager.signUp is called from
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 this be done before we even get to this screen? It seems like this would always navigate away from the sign up screen, so I wonder if we should do it while loading or something instead
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.
This will be checked on the LinkVM. The user will not get to this screen if we find an account. The first lookup here will return null (most likely) since we already checked beforehand.
This VM also listens to email changes. If the lookup is successful after an email change, we would navigate to the next screen as well. This is not likely and I'm certain it won't be the case since we don't have persistent log in anymore. I decided to keep in case we change our mind to add persistence later on. The old link implementation had persistence and did a lookup anytime the email changed.
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.
Gotcha, it sounds like this check is unnecessary right now then, is that right? If we add persistence back, we should add this code snippet back. But I think we should avoid adding this unless it's needed.
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 this logger indicate somehow that the error is coming from within the signup VM?