From 7bcbdd07072d375eb9f24a64a047879ae2aa7aed Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Wed, 13 Oct 2021 02:11:35 +0800 Subject: [PATCH] Add user status filter to admin user management page (#16770) It makes Admin's life easier to filter users by various status. * introduce window.config.PageData to pass template data to javascript module and small refactor move legacy window.ActivityTopAuthors to window.config.PageData.ActivityTopAuthors make HTML structure more IDE-friendly in footer.tmpl and head.tmpl remove incorrect in head.tmpl use log.Error instead of log.Critical in admin user search * use LEFT JOIN instead of SubQuery when admin filters users by 2fa. revert non-en locale. * use OptionalBool instead of status map * refactor SearchUserOptions.toConds to SearchUserOptions.toSearchQueryBase * add unit test for user search * only allow admin to use filters to search users --- .eslintrc | 1 - models/fixtures/user.yml | 1 + models/user.go | 53 ++++++++++++++++++++++++----- models/user_test.go | 12 +++++++ modules/context/context.go | 12 ++++--- modules/templates/helper.go | 5 +-- modules/util/util.go | 12 ++++++- modules/util/util_test.go | 13 +++++++ options/locale/locale_en-US.ini | 12 +++++++ routers/web/admin/users.go | 23 ++++++++++++- routers/web/explore/user.go | 21 +++++++----- templates/admin/base/search.tmpl | 2 +- templates/admin/user/list.tmpl | 54 ++++++++++++++++++++++++++++-- templates/base/footer.tmpl | 5 +-- templates/base/head.tmpl | 9 +++-- web_src/js/features/admin-users.js | 32 ++++++++++++++++++ web_src/js/index.js | 2 ++ 17 files changed, 233 insertions(+), 36 deletions(-) create mode 100644 web_src/js/features/admin-users.js diff --git a/.eslintrc b/.eslintrc index 98f843495..bab34478c 100644 --- a/.eslintrc +++ b/.eslintrc @@ -3,7 +3,6 @@ reportUnusedDisableDirectives: true ignorePatterns: - /web_src/js/vendor - - /templates/base/head.tmpl - /templates/repo/activity.tmpl - /templates/repo/view_file.tmpl diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index 850ee4041..c49fe1b65 100644 --- a/models/fixtures/user.yml +++ b/models/fixtures/user.yml @@ -524,6 +524,7 @@ avatar_email: user30@example.com num_repos: 2 is_active: true + prohibit_login: true - id: 31 diff --git a/models/user.go b/models/user.go index 934b834e9..3ce23ef2e 100644 --- a/models/user.go +++ b/models/user.go @@ -35,7 +35,9 @@ import ( "golang.org/x/crypto/bcrypt" "golang.org/x/crypto/pbkdf2" "golang.org/x/crypto/scrypt" + "xorm.io/builder" + "xorm.io/xorm" ) // UserType defines the user type @@ -1600,11 +1602,16 @@ type SearchUserOptions struct { OrderBy SearchOrderBy Visible []structs.VisibleType Actor *User // The user doing the search - IsActive util.OptionalBool - SearchByEmail bool // Search by email as well as username/full name + SearchByEmail bool // Search by email as well as username/full name + + IsActive util.OptionalBool + IsAdmin util.OptionalBool + IsRestricted util.OptionalBool + IsTwoFactorEnabled util.OptionalBool + IsProhibitLogin util.OptionalBool } -func (opts *SearchUserOptions) toConds() builder.Cond { +func (opts *SearchUserOptions) toSearchQueryBase() (sess *xorm.Session) { var cond builder.Cond = builder.Eq{"type": opts.Type} if len(opts.Keyword) > 0 { lowerKeyword := strings.ToLower(opts.Keyword) @@ -1658,14 +1665,39 @@ func (opts *SearchUserOptions) toConds() builder.Cond { cond = cond.And(builder.Eq{"is_active": opts.IsActive.IsTrue()}) } - return cond + if !opts.IsAdmin.IsNone() { + cond = cond.And(builder.Eq{"is_admin": opts.IsAdmin.IsTrue()}) + } + + if !opts.IsRestricted.IsNone() { + cond = cond.And(builder.Eq{"is_restricted": opts.IsRestricted.IsTrue()}) + } + + if !opts.IsProhibitLogin.IsNone() { + cond = cond.And(builder.Eq{"prohibit_login": opts.IsProhibitLogin.IsTrue()}) + } + + sess = db.NewSession(db.DefaultContext) + if !opts.IsTwoFactorEnabled.IsNone() { + // 2fa filter uses LEFT JOIN to check whether a user has a 2fa record + // TODO: bad performance here, maybe there will be a column "is_2fa_enabled" in the future + if opts.IsTwoFactorEnabled.IsTrue() { + cond = cond.And(builder.Expr("two_factor.uid IS NOT NULL")) + } else { + cond = cond.And(builder.Expr("two_factor.uid IS NULL")) + } + sess = sess.Join("LEFT OUTER", "two_factor", "two_factor.uid = `user`.id") + } + sess = sess.Where(cond) + return sess } // SearchUsers takes options i.e. keyword and part of user name to search, // it returns results in given range and number of total results. func SearchUsers(opts *SearchUserOptions) (users []*User, _ int64, _ error) { - cond := opts.toConds() - count, err := db.GetEngine(db.DefaultContext).Where(cond).Count(new(User)) + sessCount := opts.toSearchQueryBase() + defer sessCount.Close() + count, err := sessCount.Count(new(User)) if err != nil { return nil, 0, fmt.Errorf("Count: %v", err) } @@ -1674,13 +1706,16 @@ func SearchUsers(opts *SearchUserOptions) (users []*User, _ int64, _ error) { opts.OrderBy = SearchOrderByAlphabetically } - sess := db.GetEngine(db.DefaultContext).Where(cond).OrderBy(opts.OrderBy.String()) + sessQuery := opts.toSearchQueryBase().OrderBy(opts.OrderBy.String()) + defer sessQuery.Close() if opts.Page != 0 { - sess = db.SetSessionPagination(sess, opts) + sessQuery = db.SetSessionPagination(sessQuery, opts) } + // the sql may contain JOIN, so we must only select User related columns + sessQuery = sessQuery.Select("`user`.*") users = make([]*User, 0, opts.PageSize) - return users, count, sess.Find(&users) + return users, count, sessQuery.Find(&users) } // GetStarredRepos returns the repos starred by a particular user diff --git a/models/user_test.go b/models/user_test.go index bf796a8c6..2dcca2034 100644 --- a/models/user_test.go +++ b/models/user_test.go @@ -161,6 +161,18 @@ func TestSearchUsers(t *testing.T) { // order by name asc default testUserSuccess(&SearchUserOptions{Keyword: "user1", ListOptions: db.ListOptions{Page: 1}, IsActive: util.OptionalBoolTrue}, []int64{1, 10, 11, 12, 13, 14, 15, 16, 18}) + + testUserSuccess(&SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsAdmin: util.OptionalBoolTrue}, + []int64{1}) + + testUserSuccess(&SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsRestricted: util.OptionalBoolTrue}, + []int64{29, 30}) + + testUserSuccess(&SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsProhibitLogin: util.OptionalBoolTrue}, + []int64{30}) + + testUserSuccess(&SearchUserOptions{ListOptions: db.ListOptions{Page: 1}, IsTwoFactorEnabled: util.OptionalBoolTrue}, + []int64{24}) } func TestDeleteUser(t *testing.T) { diff --git a/modules/context/context.go b/modules/context/context.go index bed476032..2076ef82a 100644 --- a/modules/context/context.go +++ b/modules/context/context.go @@ -48,10 +48,11 @@ type Render interface { // Context represents context of a request. type Context struct { - Resp ResponseWriter - Req *http.Request - Data map[string]interface{} - Render Render + Resp ResponseWriter + Req *http.Request + Data map[string]interface{} // data used by MVC templates + PageData map[string]interface{} // data used by JavaScript modules in one page + Render Render translation.Locale Cache cache.Cache csrf CSRF @@ -646,6 +647,9 @@ func Contexter() func(next http.Handler) http.Handler { "Link": link, }, } + // PageData is passed by reference, and it will be rendered to `window.config.PageData` in `head.tmpl` for JavaScript modules + ctx.PageData = map[string]interface{}{} + ctx.Data["PageData"] = ctx.PageData ctx.Req = WithContext(req, &ctx) ctx.csrf = Csrfer(csrfOpts, &ctx) diff --git a/modules/templates/helper.go b/modules/templates/helper.go index b935eb6cc..791958635 100644 --- a/modules/templates/helper.go +++ b/modules/templates/helper.go @@ -351,12 +351,13 @@ func NewFuncMap() []template.FuncMap { } } else { // if sort arg is in url test if it correlates with column header sort arguments + // the direction of the arrow should indicate the "current sort order", up means ASC(normal), down means DESC(rev) if urlSort == normSort { // the table is sorted with this header normal - return SVG("octicon-triangle-down", 16) + return SVG("octicon-triangle-up", 16) } else if urlSort == revSort { // the table is sorted with this header reverse - return SVG("octicon-triangle-up", 16) + return SVG("octicon-triangle-down", 16) } } // the table is NOT sorted with this header diff --git a/modules/util/util.go b/modules/util/util.go index d26e6f13e..cbc6eb4f8 100644 --- a/modules/util/util.go +++ b/modules/util/util.go @@ -9,6 +9,7 @@ import ( "crypto/rand" "errors" "math/big" + "strconv" "strings" ) @@ -17,7 +18,7 @@ type OptionalBool byte const ( // OptionalBoolNone a "null" boolean value - OptionalBoolNone = iota + OptionalBoolNone OptionalBool = iota // OptionalBoolTrue a "true" boolean value OptionalBoolTrue // OptionalBoolFalse a "false" boolean value @@ -47,6 +48,15 @@ func OptionalBoolOf(b bool) OptionalBool { return OptionalBoolFalse } +// OptionalBoolParse get the corresponding OptionalBool of a string using strconv.ParseBool +func OptionalBoolParse(s string) OptionalBool { + b, e := strconv.ParseBool(s) + if e != nil { + return OptionalBoolNone + } + return OptionalBoolOf(b) +} + // Max max of two ints func Max(a, b int) int { if a < b { diff --git a/modules/util/util_test.go b/modules/util/util_test.go index f82671787..39cf07c85 100644 --- a/modules/util/util_test.go +++ b/modules/util/util_test.go @@ -156,3 +156,16 @@ func Test_RandomString(t *testing.T) { assert.NotEqual(t, str3, str4) } + +func Test_OptionalBool(t *testing.T) { + assert.Equal(t, OptionalBoolNone, OptionalBoolParse("")) + assert.Equal(t, OptionalBoolNone, OptionalBoolParse("x")) + + assert.Equal(t, OptionalBoolFalse, OptionalBoolParse("0")) + assert.Equal(t, OptionalBoolFalse, OptionalBoolParse("f")) + assert.Equal(t, OptionalBoolFalse, OptionalBoolParse("False")) + + assert.Equal(t, OptionalBoolTrue, OptionalBoolParse("1")) + assert.Equal(t, OptionalBoolTrue, OptionalBoolParse("t")) + assert.Equal(t, OptionalBoolTrue, OptionalBoolParse("True")) +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 2d522acb9..1324303fc 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -2371,6 +2371,18 @@ users.still_own_repo = This user still owns one or more repositories. Delete or users.still_has_org = This user is a member of an organization. Remove the user from any organizations first. users.deletion_success = The user account has been deleted. users.reset_2fa = Reset 2FA +users.list_status_filter.menu_text = Filter +users.list_status_filter.reset = Reset +users.list_status_filter.is_active = Active +users.list_status_filter.not_active = Inactive +users.list_status_filter.is_admin = Admin +users.list_status_filter.not_admin = Not Admin +users.list_status_filter.is_restricted = Restricted +users.list_status_filter.not_restricted = Not Restricted +users.list_status_filter.is_prohibit_login = Prohibit Login +users.list_status_filter.not_prohibit_login = Allow Login +users.list_status_filter.is_2fa_enabled = 2FA Enabled +users.list_status_filter.not_2fa_enabled = 2FA Disabled emails.email_manage_panel = User Email Management emails.primary = Primary diff --git a/routers/web/admin/users.go b/routers/web/admin/users.go index ea666ab4d..0041f3d07 100644 --- a/routers/web/admin/users.go +++ b/routers/web/admin/users.go @@ -19,6 +19,7 @@ import ( "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/password" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/util" "code.gitea.io/gitea/modules/web" "code.gitea.io/gitea/routers/web/explore" router_user_setting "code.gitea.io/gitea/routers/web/user/setting" @@ -38,13 +39,33 @@ func Users(ctx *context.Context) { ctx.Data["PageIsAdmin"] = true ctx.Data["PageIsAdminUsers"] = true + statusFilterKeys := []string{"is_active", "is_admin", "is_restricted", "is_2fa_enabled", "is_prohibit_login"} + statusFilterMap := map[string]string{} + for _, filterKey := range statusFilterKeys { + statusFilterMap[filterKey] = ctx.FormString("status_filter[" + filterKey + "]") + } + + sortType := ctx.FormString("sort") + if sortType == "" { + sortType = explore.UserSearchDefaultSortType + } + ctx.PageData["adminUserListSearchForm"] = map[string]interface{}{ + "StatusFilterMap": statusFilterMap, + "SortType": sortType, + } + explore.RenderUserSearch(ctx, &models.SearchUserOptions{ Actor: ctx.User, Type: models.UserTypeIndividual, ListOptions: db.ListOptions{ PageSize: setting.UI.Admin.UserPagingNum, }, - SearchByEmail: true, + SearchByEmail: true, + IsActive: util.OptionalBoolParse(statusFilterMap["is_active"]), + IsAdmin: util.OptionalBoolParse(statusFilterMap["is_admin"]), + IsRestricted: util.OptionalBoolParse(statusFilterMap["is_restricted"]), + IsTwoFactorEnabled: util.OptionalBoolParse(statusFilterMap["is_2fa_enabled"]), + IsProhibitLogin: util.OptionalBoolParse(statusFilterMap["is_prohibit_login"]), }, tplUsers) } diff --git a/routers/web/explore/user.go b/routers/web/explore/user.go index 4ddb90132..1fe45ed58 100644 --- a/routers/web/explore/user.go +++ b/routers/web/explore/user.go @@ -22,6 +22,9 @@ const ( tplExploreUsers base.TplName = "explore/users" ) +// UserSearchDefaultSortType is the default sort type for user search +const UserSearchDefaultSortType = "alphabetically" + var ( nullByte = []byte{0x00} ) @@ -44,23 +47,23 @@ func RenderUserSearch(ctx *context.Context, opts *models.SearchUserOptions, tplN orderBy models.SearchOrderBy ) + // we can not set orderBy to `models.SearchOrderByXxx`, because there may be a JOIN in the statement, different tables may have the same name columns ctx.Data["SortType"] = ctx.FormString("sort") switch ctx.FormString("sort") { case "newest": - orderBy = models.SearchOrderByIDReverse + orderBy = "`user`.id DESC" case "oldest": - orderBy = models.SearchOrderByID + orderBy = "`user`.id ASC" case "recentupdate": - orderBy = models.SearchOrderByRecentUpdated + orderBy = "`user`.updated_unix DESC" case "leastupdate": - orderBy = models.SearchOrderByLeastUpdated + orderBy = "`user`.updated_unix ASC" case "reversealphabetically": - orderBy = models.SearchOrderByAlphabeticallyReverse - case "alphabetically": - orderBy = models.SearchOrderByAlphabetically + orderBy = "`user`.name DESC" + case UserSearchDefaultSortType: // "alphabetically" default: - ctx.Data["SortType"] = "alphabetically" - orderBy = models.SearchOrderByAlphabetically + orderBy = "`user`.name ASC" + ctx.Data["SortType"] = UserSearchDefaultSortType } opts.Keyword = ctx.FormTrim("q") diff --git a/templates/admin/base/search.tmpl b/templates/admin/base/search.tmpl index e4e7e2d46..98fd3f4a0 100644 --- a/templates/admin/base/search.tmpl +++ b/templates/admin/base/search.tmpl @@ -15,7 +15,7 @@ -
+
diff --git a/templates/admin/user/list.tmpl b/templates/admin/user/list.tmpl index 661d38cb0..ceab7a9b1 100644 --- a/templates/admin/user/list.tmpl +++ b/templates/admin/user/list.tmpl @@ -10,7 +10,55 @@
- {{template "admin/base/search" .}} + + + +
+ +
+ {{.i18n.Tr "admin.users.list_status_filter.menu_text"}} {{svg "octicon-triangle-down" 14 "dropdown icon"}} +
+ {{.i18n.Tr "admin.users.list_status_filter.reset"}} +
+ + +
+ + +
+ + +
+ + +
+ + +
+
+ + +
+ + {{.i18n.Tr "repo.issues.filter_sort"}} {{svg "octicon-triangle-down" 14 "dropdown icon"}} + +
+ + + + + + +
+
+
+ + +
+ + +
+
@@ -28,9 +76,9 @@ - diff --git a/templates/base/footer.tmpl b/templates/base/footer.tmpl index 25e163b19..ead5630da 100644 --- a/templates/base/footer.tmpl +++ b/templates/base/footer.tmpl @@ -1,8 +1,9 @@ -{{/* +{{if false}} + {{/* to make html structure "likely" complete to prevent IDE warnings */}}
-*/}} +{{end}} {{template "custom/body_inner_post" .}} diff --git a/templates/base/head.tmpl b/templates/base/head.tmpl index 15f2826ab..817bdae28 100644 --- a/templates/base/head.tmpl +++ b/templates/base/head.tmpl @@ -26,6 +26,7 @@ {{end}}
{{.i18n.Tr "admin.users.2fa"}} {{.i18n.Tr "admin.users.repos"}} {{.i18n.Tr "admin.users.created"}} + {{.i18n.Tr "admin.users.last_login"}} - {{SortArrow "recentupdate" "leastupdate" $.SortType false}} + {{SortArrow "leastupdate" "recentupdate" $.SortType false}} {{.i18n.Tr "admin.users.edit"}}