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

Improve the comment for 2FA filter in admin panel #18017

Merged
merged 3 commits into from Jan 13, 2022
Merged

Improve the comment for 2FA filter in admin panel #18017

merged 3 commits into from Jan 13, 2022

Conversation

Gusted
Copy link
Contributor

@Gusted Gusted commented Dec 18, 2021 •
edited

  • Better TODO message.

lunny
lunny reviewed Dec 18, 2021
View reviewed changes
models/user/user.go Outdated Show resolved Hide resolved
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 18, 2021
@wxiaoguang
Copy link
Contributor

That TODO comes from me (I added the filter)

If there is no performance problem, I think we can postpone the introducing of new fields.

It seems that 2FA system should to be refactored also.

@Gusted
Copy link
Contributor Author

Gusted commented Dec 19, 2021

That TODO comes from me (I added the filter)

Ah, IDE showed that last activity came from Lunny(so I just assumed)

If there is no performance problem, I think we can postpone the introducing of new fields.

It seems that 2FA system should to be refactored also.

Last time I checked using JOIN's aren't quite good for the performance lookups.

@wxiaoguang
Copy link
Contributor

wxiaoguang commented Dec 21, 2021 •
edited

Last time I checked using JOIN's aren't quite good for the performance lookups.

Yep, although its performance not good, as long as it doesn't harm now so it's not too bad 😂. Such SQL is seldom executed.

Since we are refactoring u2f/webauthn now, I have a feeling that 2FA mechanism should be improved after these refactoring.

@lunny
Copy link
Member

lunny commented Dec 21, 2021

More and more columns in user table is not a good idea imo.

@singuliere
Copy link
Contributor

I agree that adding a new field is not worth the gain. The TODO comment should be updated to not suggest adding the field so that someone else does not try to implement what turns out to be something that is not desirable.

@lunny
Copy link
Member

lunny commented Jan 13, 2022

Since user setting table has been added, I think this PR could do some adjustment to use that table but not create a new column.

@Gusted
Copy link
Contributor Author

Gusted commented Jan 13, 2022

I've for now changed the TODO

@Gusted Gusted changed the title Add is_2fa_enabled field to user Change TODO to add it to user's settings Jan 13, 2022
@Gusted Gusted changed the title Change TODO to add it to user's settings Change 2fa TODO to add it to user's settings Jan 13, 2022
wxiaoguang
wxiaoguang reviewed Jan 13, 2022
View reviewed changes
models/user/search.go Outdated Show resolved Hide resolved
Gusted and others added 2 commits January 13, 2022 19:52
@Gusted @wxiaoguang
Update models/user/search.go
2cca4f4 
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@wxiaoguang
Update search.go
4d231f1
@wxiaoguang wxiaoguang changed the title Change 2fa TODO to add it to user's settings Improve the comment for 2FA filter in admin panel Jan 13, 2022
@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 13, 2022
@wxiaoguang wxiaoguang added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 13, 2022
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 13, 2022
@6543 6543 merged commit d413a1f into go-gitea:main Jan 13, 2022
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jan 14, 2022
@zjjhot
Merge branch 'main' of https://github.com/go-gitea/gitea
8abecc5 
* 'main' of https://github.com/go-gitea/gitea:
  [skip ci] Updated translations via Crowdin
  Improve the comment for 2FA filter in admin panel (go-gitea#18017)
  fix regression from go-gitea#16075 (go-gitea#18260)
  Prevent underline hover on cards (go-gitea#18259)
  Fix release link broken (go-gitea#18252)
  migrations: a deadline at January 1st, 1970 is valid (go-gitea#18237)
Chianina pushed a commit to Chianina/gitea that referenced this pull request Mar 28, 2022
@Gusted @wxiaoguang
Improve the comment for 2FA filter in admin panel (go-gitea#18017)
67eab0b 
Replace TODO with explanation

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny lunny left review comments

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@6543 6543 6543 approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Gusted @wxiaoguang @lunny @singuliere @6543 @GiteaBot