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 <style class="list-search-style"></style> 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

Author: wxiaoguang

.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
 

models/fixtures/user.yml

@@ -524,6 +524,7 @@
   avatar_email: [email protected]
   num_repos: 2
   is_active: true
+  prohibit_login: true
 
 -
   id: 31

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

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) {

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)

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

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 {

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"))
+}

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

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)
 }
 

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")

templates/admin/base/search.tmpl

@@ -15,7 +15,7 @@
 		</div>
 	</div>
 </div>
-<form class="ui form ignore-dirty"  style="max-width: 90%">
+<form class="ui form ignore-dirty"  style="max-width: 90%;">
 	<div class="ui fluid action input">
 		<input name="q" value="{{.Keyword}}" placeholder="{{.i18n.Tr "explore.search"}}..." autofocus>
 		<button class="ui blue button">{{.i18n.Tr "explore.search"}}</button>