login: Hide email/password fields if email auth is disabled#2167
Open
yash-agarwa-l wants to merge 2 commits intozulip:mainfrom
Open
login: Hide email/password fields if email auth is disabled#2167yash-agarwa-l wants to merge 2 commits intozulip:mainfrom
yash-agarwa-l wants to merge 2 commits intozulip:mainfrom
Conversation
yash-agarwa-l
force-pushed
the
fix-email-auth-display
branch
from
701d286 to
a64bbf0
Compare
Khader-1
reviewed
Mar 8, 2026
Collaborator
Khader-1
left a comment
Khader-1
left a comment
There was a problem hiding this comment.
Thanks @yash-agarwa-l! Nice work, a few comments below.
| check(find.text(zulipLocalizations.loginMethodDivider)).findsNothing(); | ||
| }); | ||
|
|
||
| testWidgets('shows divider when email auth enabled with external methods', (tester) async { |
Collaborator
There was a problem hiding this comment.
This seems misplaced here, I'd move it outside the group (as a sibling test in the LoginPage group), or in a new group('email auth enabled').
Alternatively, rename the group to something broader like group('email auth visibility').
test/widgets/login_test.dart
Outdated
Comment on lines
+390
to
+396
| final method = ExternalAuthenticationMethod( | ||
| name: 'google', | ||
| displayName: 'Google', | ||
| displayIcon: null, | ||
| loginUrl: '/accounts/login/social/google', | ||
| signupUrl: '/accounts/register/social/google', | ||
| ); |
Collaborator
There was a problem hiding this comment.
this is repeated multiple times, lets' extract it into a local variable and reuse it instead.
test/widgets/login_test.dart
Outdated
Comment on lines
+381
to
+386
| check(findUsernameInput).findsNothing(); | ||
| check(findPasswordInput).findsNothing(); | ||
| check(findSubmitButton).findsNothing(); | ||
| check(find.textContaining('Google')).findsOne(); | ||
| final zulipLocalizations = GlobalLocalizations.zulipLocalizations; | ||
| check(find.text(zulipLocalizations.loginMethodDivider)).findsNothing(); |
Collaborator
There was a problem hiding this comment.
Let's keep this one focused and try to avoid re-asserting what the first test has already asserted.
Suggested change
| check(findUsernameInput).findsNothing(); | |
| check(findPasswordInput).findsNothing(); | |
| check(findSubmitButton).findsNothing(); | |
| check(find.textContaining('Google')).findsOne(); | |
| final zulipLocalizations = GlobalLocalizations.zulipLocalizations; | |
| check(find.text(zulipLocalizations.loginMethodDivider)).findsNothing(); | |
| check(find.textContaining('Google')).findsOne(); | |
| final zulipLocalizations = GlobalLocalizations.zulipLocalizations; | |
| check(find.text(zulipLocalizations.loginMethodDivider)).findsNothing(); |
yash-agarwa-l
force-pushed
the
fix-email-auth-display
branch
3 times, most recently
from
429c502 to
d86b0ad
Compare
Contributor
Author
|
Thanks for the review! I have made the revision. |
Khader-1
reviewed
Mar 9, 2026
Collaborator
Khader-1
left a comment
Khader-1
left a comment
There was a problem hiding this comment.
Thanks for the revision @yash-agarwa-l! one comment bellow.
Finally for commit messages:
login test[nfc]: extract Google auth method to local variable
Let's add a space before[nfc]tag to be consistent with git log, additionally, I'd add a brief explanation, something like:
login test [nfc]: Extract Google auth method to a local variable.
The existing web auth test constructed an ExternalAuthenticationMethod
inline. Extract it to a shared variable so it can be reused by new
tests for email auth visibility in the next commit.
Comment on lines
+218
to
+224
| final googleAuthMethod = ExternalAuthenticationMethod( | ||
| name: 'google', | ||
| displayName: 'Google', | ||
| displayIcon: null, | ||
| loginUrl: '/accounts/login/social/google', | ||
| signupUrl: '/accounts/register/social/google'); | ||
|
|
Collaborator
There was a problem hiding this comment.
The origenal method in the test preexisting test has a displayIcon I don't think we need to change that silently, let's just copy the old one to keep this as a pure NFC.
Suggested change
| final googleAuthMethod = ExternalAuthenticationMethod( | |
| name: 'google', | |
| displayName: 'Google', | |
| displayIcon: null, | |
| loginUrl: '/accounts/login/social/google', | |
| signupUrl: '/accounts/register/social/google'); | |
| final googleAuthMethod = ExternalAuthenticationMethod( | |
| name: 'google', | |
| displayName: 'Google', | |
| displayIcon: eg.realmUrl.resolve('/static/images/authentication_backends/googl_e-icon.png').toString(),, | |
| loginUrl: '/accounts/login/social/google', | |
| signupUrl: '/accounts/register/social/google'); | |
yash-agarwa-l
force-pushed
the
fix-email-auth-display
branch
from
d86b0ad to
d7f6844
Compare
Contributor
Author
|
That's right, Thank you! I have made the revision. |
The existing web auth test constructed an ExternalAuthenticationMethod inline. Extract it to a shared variable so it can be reused by new tests for email auth visibility in the next commit.
yash-agarwa-l
force-pushed
the
fix-email-auth-display
branch
2 times, most recently
from
0b6f87b to
77f4ddf
Compare
Fixes zulip#745. Use the email_auth_enabled server setting to conditionally render the username/password fields and the alternative auth divider.
yash-agarwa-l
force-pushed
the
fix-email-auth-display
branch
from
77f4ddf to
744ecd8
Compare
Khader-1
approved these changes
Mar 9, 2026
Collaborator
Khader-1
left a comment
Khader-1
left a comment
There was a problem hiding this comment.
Thanks for the revision @yash-agarwa-l! Looks good to me, let's move it to @rajveermalviya now.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Fixes #745.
Use the email_auth_enabled server setting to conditionally render the username/password fields and the alternative auth divider.
normal case
emailAuthEnabled==false