-
Notifications
You must be signed in to change notification settings - Fork 31
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
feat: Login through username in the new app #181
feat: Login through username in the new app #181
Conversation
- Add username support for the authentication fixes: LEARNER-9782
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.
only some minor nits
auth/src/main/java/org/openedx/auth/presentation/restore/RestorePasswordFragment.kt
Outdated
Show resolved
Hide resolved
auth/src/test/java/org/openedx/auth/presentation/signin/SignInViewModelTest.kt
Outdated
Show resolved
Hide resolved
Pattern.compile("^[A-Z0-9._%+-]+@[A-Z0-9.-]+\\.[A-Z]{2,6}$", Pattern.CASE_INSENSITIVE) | ||
val matcher = validEmailAddressRegex.matcher(email) | ||
return matcher.find() | ||
fun isEmailOrUserNameValid(email: String): Boolean { |
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.
we are still only verifying the email address, do we need to verify the username?
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.
we only need to check that user name shouldn't be empty. iOS did the same.
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.
it would be great if you could just check space
in the field.
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.
already replaced with isBlank
, so no need to check white spaces. Also can trim the white space in case of non-blank. thoughts?
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.
only nits
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.
Just a minor nit
0b709c4
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.
LGTM
- Add username support for the authentication fixes: LEARNER-9782
Description
username
support for the authentication.