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

.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: user30@example.com
   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()
+	sessCount := opts.toSearchQueryBase()
-	count, err := db.GetEngine(db.DefaultContext).Where(cond).Count(new(User))
+	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
+	Resp     ResponseWriter
-	Req    *http.Request
+	Req      *http.Request
-	Data   map[string]interface{}
+	Data     map[string]interface{} // data used by MVC templates
-	Render Render
+	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
+		orderBy = "`user`.name DESC"
-	case "alphabetically":
+	case UserSearchDefaultSortType: // "alphabetically"
-		orderBy = models.SearchOrderByAlphabetically
 	default:
-		ctx.Data["SortType"] = "alphabetically"
+		orderBy = "`user`.name ASC"
-		orderBy = models.SearchOrderByAlphabetically
+		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>

templates/admin/user/list.tmpl

@@ -10,7 +10,55 @@
 			</div>
 		</h4>
 		<div class="ui attached segment">
-			{{template "admin/base/search" .}}
+			<form class="ui form ignore-dirty" id="user-list-search-form">
+
+				<!-- Right Menu -->
+				<div class="ui right floated secondary filter menu">
+					<!-- Status Filter Menu Item -->
+					<div class="ui dropdown type jump item">
+						<span class="text">{{.i18n.Tr "admin.users.list_status_filter.menu_text"}} {{svg "octicon-triangle-down" 14 "dropdown icon"}}</span>
+						<div class="menu">
+							<a class="item j-reset-status-filter">{{.i18n.Tr "admin.users.list_status_filter.reset"}}</a>
+							<div class="ui divider"></div>
+							<label class="item"><input type="radio" name="status_filter[is_admin]" value="1"> {{.i18n.Tr "admin.users.list_status_filter.is_admin"}}</label>
+							<label class="item"><input type="radio" name="status_filter[is_admin]" value="0"> {{.i18n.Tr "admin.users.list_status_filter.not_admin"}}</label>
+							<div class="ui divider"></div>
+							<label class="item"><input type="radio" name="status_filter[is_active]" value="1"> {{.i18n.Tr "admin.users.list_status_filter.is_active"}}</label>
+							<label class="item"><input type="radio" name="status_filter[is_active]" value="0"> {{.i18n.Tr "admin.users.list_status_filter.not_active"}}</label>
+							<div class="ui divider"></div>
+							<label class="item"><input type="radio" name="status_filter[is_restricted]" value="0"> {{.i18n.Tr "admin.users.list_status_filter.not_restricted"}}</label>
+							<label class="item"><input type="radio" name="status_filter[is_restricted]" value="1"> {{.i18n.Tr "admin.users.list_status_filter.is_restricted"}}</label>
+							<div class="ui divider"></div>
+							<label class="item"><input type="radio" name="status_filter[is_prohibit_login]" value="0"> {{.i18n.Tr "admin.users.list_status_filter.not_prohibit_login"}}</label>
+							<label class="item"><input type="radio" name="status_filter[is_prohibit_login]" value="1"> {{.i18n.Tr "admin.users.list_status_filter.is_prohibit_login"}}</label>
+							<div class="ui divider"></div>
+							<label class="item"><input type="radio" name="status_filter[is_2fa_enabled]" value="1"> {{.i18n.Tr "admin.users.list_status_filter.is_2fa_enabled"}}</label>
+							<label class="item"><input type="radio" name="status_filter[is_2fa_enabled]" value="0"> {{.i18n.Tr "admin.users.list_status_filter.not_2fa_enabled"}}</label>
+						</div>
+					</div>
+
+					<!-- Sort Menu Item -->
+					<div class="ui dropdown type jump item">
+						<span class="text">
+							{{.i18n.Tr "repo.issues.filter_sort"}} {{svg "octicon-triangle-down" 14 "dropdown icon"}}
+						</span>
+						<div class="menu">
+							<button class="item" name="sort" value="oldest">{{.i18n.Tr "repo.issues.filter_sort.oldest"}}</button>
+							<button class="item" name="sort" value="newest">{{.i18n.Tr "repo.issues.filter_sort.latest"}}</button>
+							<button class="item" name="sort" value="alphabetically">{{.i18n.Tr "repo.issues.label.filter_sort.alphabetically"}}</button>
+							<button class="item" name="sort" value="reversealphabetically">{{.i18n.Tr "repo.issues.label.filter_sort.reverse_alphabetically"}}</button>
+							<button class="item" name="sort" value="recentupdate">{{.i18n.Tr "repo.issues.filter_sort.recentupdate"}}</button>
+							<button class="item" name="sort" value="leastupdate">{{.i18n.Tr "repo.issues.filter_sort.leastupdate"}}</button>
+						</div>
+					</div>
+				</div>
+
+				<!-- Search Text -->
+				<div class="ui fluid action input" style="max-width: 70%;">
+					<input name="q" value="{{.Keyword}}" placeholder="{{.i18n.Tr "explore.search"}}..." autofocus>
+					<button class="ui blue button">{{.i18n.Tr "explore.search"}}</button>
+				</div>
+			</form>
 		</div>
 		<div class="ui attached table segment">
 			<table class="ui very basic striped table">
@@ -28,9 +76,9 @@
 						<th>{{.i18n.Tr "admin.users.2fa"}}</th>
 						<th>{{.i18n.Tr "admin.users.repos"}}</th>
 						<th>{{.i18n.Tr "admin.users.created"}}</th>
-						<th data-sortt-asc="recentupdate" data-sortt-desc="leastupdate">
+						<th data-sortt-asc="leastupdate" data-sortt-desc="recentupdate">
 							{{.i18n.Tr "admin.users.last_login"}}
-							{{SortArrow "recentupdate" "leastupdate" $.SortType false}}
+							{{SortArrow "leastupdate" "recentupdate" $.SortType false}}
 						</th>
 						<th>{{.i18n.Tr "admin.users.edit"}}</th>
 					</tr>

templates/base/footer.tmpl

@@ -1,8 +1,9 @@
-{{/*
+{{if false}}
+	{{/* to make html structure "likely" complete to prevent IDE warnings */}}
 <html>
 <body>
 	<div>
-*/}}
+{{end}}
 
 	{{template "custom/body_inner_post" .}}
 

templates/base/head.tmpl

@@ -26,6 +26,7 @@
 	<meta name="go-source" content="{{.GoGetImport}} _ {{.GoDocDirectory}} {{.GoDocFile}}">
 {{end}}
 	<script>
+		<!-- /* eslint-disable */ -->
 		window.config = {
 			AppVer: '{{AppVer}}',
 			AppSubUrl: '{{AppSubUrl}}',
@@ -33,6 +34,7 @@
 			CustomEmojis: {{CustomEmojis}},
 			UseServiceWorker: {{UseServiceWorker}},
 			csrf: '{{.CsrfToken}}',
+			PageData: {{ .PageData }},
 			HighlightJS: {{if .RequireHighlightJS}}true{{else}}false{{end}},
 			SimpleMDE: {{if .RequireSimpleMDE}}true{{else}}false{{end}},
 			Tribute: {{if .RequireTribute}}true{{else}}false{{end}},
@@ -75,7 +77,6 @@
 			.ui.secondary.menu .dropdown.item > .menu { margin-top: 0; }
 		</style>
 	</noscript>
-	<style class="list-search-style"></style>
 {{if .PageIsUserProfile}}
 	<meta property="og:title" content="{{.Owner.Name}}" />
 	<meta property="og:type" content="profile" />
@@ -134,8 +135,10 @@
 				{{template "base/head_navbar" .}}
 			</div><!-- end bar -->
 		{{end}}
-{{/*
+
+{{if false}}
+	{{/* to make html structure "likely" complete to prevent IDE warnings */}}
 	</div>
 </body>
 </html>
-*/}}
+{{end}}

web_src/js/features/admin-users.js

@@ -0,0 +1,32 @@
+export function initAdminUserListSearchForm() {
+  const searchForm = window.config.PageData.adminUserListSearchForm;
+  if (!searchForm) return;
+
+  const $form = $('#user-list-search-form');
+  if (!$form.length) return;
+
+  $form.find(`button[name=sort][value=${searchForm.SortType}]`).addClass('active');
+
+  if (searchForm.StatusFilterMap) {
+    for (const [k, v] of Object.entries(searchForm.StatusFilterMap)) {
+      if (!v) continue;
+      $form.find(`input[name="status_filter[${k}]"][value=${v}]`).prop('checked', true);
+    }
+  }
+
+  $form.find(`input[type=radio]`).click(() => {
+    $form.submit();
+    return false;
+  });
+
+  $form.find('.j-reset-status-filter').click(() => {
+    $form.find(`input[type=radio]`).each((_, e) => {
+      const $e = $(e);
+      if ($e.attr('name').startsWith('status_filter[')) {
+        $e.prop('checked', false);
+      }
+    });
+    $form.submit();
+    return false;
+  });
+}

web_src/js/index.js

@@ -17,6 +17,7 @@ import initMigration from './features/migration.js';
 import initProject from './features/projects.js';
 import initServiceWorker from './features/serviceworker.js';
 import initTableSort from './features/tablesort.js';
+import {initAdminUserListSearchForm} from './features/admin-users.js';
 import {createCodeEditor, createMonaco} from './features/codeeditor.js';
 import {initMarkupAnchors} from './markup/anchors.js';
 import {initNotificationsTable, initNotificationCount} from './features/notification.js';
@@ -2875,6 +2876,7 @@ $(document).ready(async () => {
   initReleaseEditor();
   initRelease();
   initIssueContentHistory();
+  initAdminUserListSearchForm();
 
   const routes = {
     'div.user.settings': initUserSettings,