Hide unactive on explore users and some refactors (#2741)

* hide unactive on explore users and some refactors

* fix test for removed Organizations

* fix test for removed Organizations

* fix imports

* fix logic bug

* refactor the toConds

* Rename TestOrganizations to TestSearchUsers and add tests for users

* fix other tests

* fix other tests

* fix watchers tests

* fix comments and remove unused code

models/fixtures/issue_watch.yml

@@ -1,6 +1,6 @@
 -
   id: 1
-  user_id: 1
+  user_id: 9
   issue_id: 1
   is_watching: true
   created_unix: 946684800

models/fixtures/user.yml

@@ -13,6 +13,7 @@
   avatar: avatar1
   avatar_email: user1@example.com
   num_repos: 0
+  is_active: true
 
 -
   id: 2
@@ -62,6 +63,7 @@
   avatar_email: user4@example.com
   num_repos: 0
   num_following: 1
+  is_active: true
 
 -
   id: 5

models/fixtures/watch.yml

@@ -10,5 +10,5 @@
 
 -
   id: 3
-  user_id: 10
+  user_id: 9
   repo_id: 1

models/issue_watch_test.go

@@ -25,7 +25,7 @@ func TestCreateOrUpdateIssueWatch(t *testing.T) {
 func TestGetIssueWatch(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 
-	_, exists, err := GetIssueWatch(1, 1)
+	_, exists, err := GetIssueWatch(9, 1)
 	assert.Equal(t, true, exists)
 	assert.NoError(t, err)
 

models/notification_test.go

@@ -16,10 +16,13 @@ func TestCreateOrUpdateIssueNotifications(t *testing.T) {
 
 	assert.NoError(t, CreateOrUpdateIssueNotifications(issue, 2))
 
-	// Two watchers are inactive, thus only notification for user 10 is created
-	notf := AssertExistsAndLoadBean(t, &Notification{UserID: 10, IssueID: issue.ID}).(*Notification)
+	// User 9 is inactive, thus notifications for user 1 and 4 are created
+	notf := AssertExistsAndLoadBean(t, &Notification{UserID: 1, IssueID: issue.ID}).(*Notification)
 	assert.Equal(t, NotificationStatusUnread, notf.Status)
 	CheckConsistencyFor(t, &Issue{ID: issue.ID})
+
+	notf = AssertExistsAndLoadBean(t, &Notification{UserID: 4, IssueID: issue.ID}).(*Notification)
+	assert.Equal(t, NotificationStatusUnread, notf.Status)
 }
 
 func TestNotificationsForUser(t *testing.T) {

models/org.go

@@ -201,23 +201,6 @@ func CountOrganizations() int64 {
 	return count
 }
 
-// Organizations returns number of organizations in given page.
-func Organizations(opts *SearchUserOptions) ([]*User, error) {
-	orgs := make([]*User, 0, opts.PageSize)
-
-	if len(opts.OrderBy) == 0 {
-		opts.OrderBy = "name ASC"
-	}
-
-	sess := x.
-		Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
-		Where("type=1")
-
-	return orgs, sess.
-		OrderBy(opts.OrderBy).
-		Find(&orgs)
-}
-
 // DeleteOrganization completely and permanently deletes everything of organization.
 func DeleteOrganization(org *User) (err error) {
 	sess := x.NewSession()

models/org_test.go

@@ -237,27 +237,6 @@ func TestCountOrganizations(t *testing.T) {
 	assert.Equal(t, expected, CountOrganizations())
 }
 
-func TestOrganizations(t *testing.T) {
-	assert.NoError(t, PrepareTestDatabase())
-	testSuccess := func(opts *SearchUserOptions, expectedOrgIDs []int64) {
-		orgs, err := Organizations(opts)
-		assert.NoError(t, err)
-		if assert.Len(t, orgs, len(expectedOrgIDs)) {
-			for i, expectedOrgID := range expectedOrgIDs {
-				assert.EqualValues(t, expectedOrgID, orgs[i].ID)
-			}
-		}
-	}
-	testSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 1, PageSize: 2},
-		[]int64{3, 6})
-
-	testSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 2, PageSize: 2},
-		[]int64{7, 17})
-
-	testSuccess(&SearchUserOptions{Page: 3, PageSize: 2},
-		[]int64{})
-}
-
 func TestDeleteOrganization(t *testing.T) {
 	assert.NoError(t, PrepareTestDatabase())
 	org := AssertExistsAndLoadBean(t, &User{ID: 6}).(*User)

models/repo_watch_test.go

@@ -40,8 +40,8 @@ func TestGetWatchers(t *testing.T) {
 	repo := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
 	watches, err := GetWatchers(repo.ID)
 	assert.NoError(t, err)
-	// Two watchers are inactive, thus minus 2
-	assert.Len(t, watches, repo.NumWatches-2)
+	// One watchers are inactive, thus minus 1
+	assert.Len(t, watches, repo.NumWatches-1)
 	for _, watch := range watches {
 		assert.EqualValues(t, repo.ID, watch.RepoID)
 	}
@@ -62,7 +62,7 @@ func TestRepository_GetWatchers(t *testing.T) {
 		AssertExistsAndLoadBean(t, &Watch{UserID: watcher.ID, RepoID: repo.ID})
 	}
 
-	repo = AssertExistsAndLoadBean(t, &Repository{ID: 10}).(*Repository)
+	repo = AssertExistsAndLoadBean(t, &Repository{ID: 9}).(*Repository)
 	watchers, err = repo.GetWatchers(1)
 	assert.NoError(t, err)
 	assert.Len(t, watchers, 0)
@@ -78,7 +78,7 @@ func TestNotifyWatchers(t *testing.T) {
 	}
 	assert.NoError(t, NotifyWatchers(action))
 
-	// Two watchers are inactive, thus action is only created for user 8, 10
+	// One watchers are inactive, thus action is only created for user 8, 1, 4
 	AssertExistsAndLoadBean(t, &Action{
 		ActUserID: action.ActUserID,
 		UserID:    8,
@@ -87,7 +87,13 @@ func TestNotifyWatchers(t *testing.T) {
 	})
 	AssertExistsAndLoadBean(t, &Action{
 		ActUserID: action.ActUserID,
-		UserID:    10,
+		UserID:    1,
+		RepoID:    action.RepoID,
+		OpType:    action.OpType,
+	})
+	AssertExistsAndLoadBean(t, &Action{
+		ActUserID: action.ActUserID,
+		UserID:    4,
 		RepoID:    action.RepoID,
 		OpType:    action.OpType,
 	})

models/user.go

@@ -36,6 +36,7 @@ import (
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // UserType defines the user type
@@ -729,22 +730,6 @@ func CountUsers() int64 {
 	return countUsers(x)
 }
 
-// Users returns number of users in given page.
-func Users(opts *SearchUserOptions) ([]*User, error) {
-	if len(opts.OrderBy) == 0 {
-		opts.OrderBy = "name ASC"
-	}
-
-	users := make([]*User, 0, opts.PageSize)
-	sess := x.
-		Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
-		Where("type=0")
-
-	return users, sess.
-		OrderBy(opts.OrderBy).
-		Find(&users)
-}
-
 // get user by verify code
 func getVerifyUser(code string) (user *User) {
 	if len(code) <= base.TimeLimitCodeLength {
@@ -1284,45 +1269,50 @@ type SearchUserOptions struct {
 	OrderBy  string
 	Page     int
 	PageSize int // Can be smaller than or equal to setting.UI.ExplorePagingNum
+	IsActive util.OptionalBool
 }
 
-// SearchUserByName takes keyword and part of user name to search,
-// it returns results in given range and number of total results.
-func SearchUserByName(opts *SearchUserOptions) (users []*User, _ int64, _ error) {
-	if len(opts.Keyword) == 0 {
-		return users, 0, nil
+func (opts *SearchUserOptions) toConds() builder.Cond {
+	var cond builder.Cond = builder.Eq{"type": opts.Type}
+	if len(opts.Keyword) > 0 {
+		lowerKeyword := strings.ToLower(opts.Keyword)
+		cond = cond.And(builder.Or(
+			builder.Like{"lower_name", lowerKeyword},
+			builder.Like{"LOWER(full_name)", lowerKeyword},
+		))
 	}
-	opts.Keyword = strings.ToLower(opts.Keyword)
 
-	if opts.PageSize <= 0 || opts.PageSize > setting.UI.ExplorePagingNum {
-		opts.PageSize = setting.UI.ExplorePagingNum
-	}
-	if opts.Page <= 0 {
-		opts.Page = 1
+	if !opts.IsActive.IsNone() {
+		cond = cond.And(builder.Eq{"is_active": opts.IsActive.IsTrue()})
 	}
 
-	users = make([]*User, 0, opts.PageSize)
-
-	// Append conditions
-	cond := builder.And(
-		builder.Eq{"type": opts.Type},
-		builder.Or(
-			builder.Like{"lower_name", opts.Keyword},
-			builder.Like{"LOWER(full_name)", opts.Keyword},
-		),
-	)
+	return cond
+}
 
+// 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 := x.Where(cond).Count(new(User))
 	if err != nil {
 		return nil, 0, fmt.Errorf("Count: %v", err)
 	}
 
-	sess := x.Where(cond).
-		Limit(opts.PageSize, (opts.Page-1)*opts.PageSize)
-	if len(opts.OrderBy) > 0 {
-		sess.OrderBy(opts.OrderBy)
+	if opts.PageSize <= 0 || opts.PageSize > setting.UI.ExplorePagingNum {
+		opts.PageSize = setting.UI.ExplorePagingNum
+	}
+	if opts.Page <= 0 {
+		opts.Page = 1
 	}
-	return users, count, sess.Find(&users)
+	if len(opts.OrderBy) == 0 {
+		opts.OrderBy = "name ASC"
+	}
+
+	users = make([]*User, 0, opts.PageSize)
+	return users, count, x.Where(cond).
+		Limit(opts.PageSize, (opts.Page-1)*opts.PageSize).
+		OrderBy(opts.OrderBy).
+		Find(&users)
 }
 
 // GetStarredRepos returns the repos starred by a particular user

models/user_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -38,6 +39,56 @@ func TestCanCreateOrganization(t *testing.T) {
 	assert.False(t, user.CanCreateOrganization())
 }
 
+func TestSearchUsers(t *testing.T) {
+	assert.NoError(t, PrepareTestDatabase())
+	testSuccess := func(opts *SearchUserOptions, expectedUserOrOrgIDs []int64) {
+		users, _, err := SearchUsers(opts)
+		assert.NoError(t, err)
+		if assert.Len(t, users, len(expectedUserOrOrgIDs)) {
+			for i, expectedID := range expectedUserOrOrgIDs {
+				assert.EqualValues(t, expectedID, users[i].ID)
+			}
+		}
+	}
+
+	// test orgs
+	testOrgSuccess := func(opts *SearchUserOptions, expectedOrgIDs []int64) {
+		opts.Type = UserTypeOrganization
+		testSuccess(opts, expectedOrgIDs)
+	}
+
+	testOrgSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 1, PageSize: 2},
+		[]int64{3, 6})
+
+	testOrgSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 2, PageSize: 2},
+		[]int64{7, 17})
+
+	testOrgSuccess(&SearchUserOptions{Page: 3, PageSize: 2},
+		[]int64{})
+
+	// test users
+	testUserSuccess := func(opts *SearchUserOptions, expectedUserIDs []int64) {
+		opts.Type = UserTypeIndividual
+		testSuccess(opts, expectedUserIDs)
+	}
+
+	testUserSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 1},
+		[]int64{1, 2, 4, 5, 8, 9, 10, 11, 12, 13, 14, 15, 16, 18})
+
+	testUserSuccess(&SearchUserOptions{Page: 1, IsActive: util.OptionalBoolFalse},
+		[]int64{9})
+
+	testUserSuccess(&SearchUserOptions{OrderBy: "id ASC", Page: 1, IsActive: util.OptionalBoolTrue},
+		[]int64{1, 2, 4, 5, 8, 10, 11, 12, 13, 14, 15, 16, 18})
+
+	testUserSuccess(&SearchUserOptions{Keyword: "user1", OrderBy: "id ASC", Page: 1, IsActive: util.OptionalBoolTrue},
+		[]int64{1, 10, 11, 12, 13, 14, 15, 16, 18})
+
+	// order by name asc default
+	testUserSuccess(&SearchUserOptions{Keyword: "user1", Page: 1, IsActive: util.OptionalBoolTrue},
+		[]int64{1, 10, 11, 12, 13, 14, 15, 16, 18})
+}
+
 func TestDeleteUser(t *testing.T) {
 	test := func(userID int64) {
 		assert.NoError(t, PrepareTestDatabase())

modules/util/util.go

@@ -16,6 +16,21 @@ const (
 	OptionalBoolFalse
 )
 
+// IsTrue return true if equal to OptionalBoolTrue
+func (o OptionalBool) IsTrue() bool {
+	return o == OptionalBoolTrue
+}
+
+// IsFalse return true if equal to OptionalBoolFalse
+func (o OptionalBool) IsFalse() bool {
+	return o == OptionalBoolFalse
+}
+
+// IsNone return true if equal to OptionalBoolNone
+func (o OptionalBool) IsNone() bool {
+	return o == OptionalBoolNone
+}
+
 // OptionalBoolOf get the corresponding OptionalBool of a bool
 func OptionalBoolOf(b bool) OptionalBool {
 	if b {

routers/admin/orgs.go

@@ -22,11 +22,8 @@ func Organizations(ctx *context.Context) {
 	ctx.Data["PageIsAdmin"] = true
 	ctx.Data["PageIsAdminOrganizations"] = true
 
-	routers.RenderUserSearch(ctx, &routers.UserSearchOptions{
+	routers.RenderUserSearch(ctx, &models.SearchUserOptions{
 		Type:     models.UserTypeOrganization,
-		Counter:  models.CountOrganizations,
-		Ranger:   models.Organizations,
 		PageSize: setting.UI.Admin.OrgPagingNum,
-		TplName:  tplOrgs,
-	})
+	}, tplOrgs)
 }

routers/admin/users.go

@@ -30,13 +30,10 @@ func Users(ctx *context.Context) {
 	ctx.Data["PageIsAdmin"] = true
 	ctx.Data["PageIsAdminUsers"] = true
 
-	routers.RenderUserSearch(ctx, &routers.UserSearchOptions{
+	routers.RenderUserSearch(ctx, &models.SearchUserOptions{
 		Type:     models.UserTypeIndividual,
-		Counter:  models.CountUsers,
-		Ranger:   models.Users,
 		PageSize: setting.UI.Admin.UserPagingNum,
-		TplName:  tplUsers,
-	})
+	}, tplUsers)
 }
 
 // NewUser render adding a new user page

routers/api/v1/user/user.go

@@ -35,7 +35,7 @@ func Search(ctx *context.APIContext) {
 		opts.PageSize = 10
 	}
 
-	users, _, err := models.SearchUserByName(opts)
+	users, _, err := models.SearchUsers(opts)
 	if err != nil {
 		ctx.JSON(500, map[string]interface{}{
 			"ok":    false,

routers/home.go

@@ -12,6 +12,7 @@ import (
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/routers/user"
 
 	"github.com/Unknwon/paginater"
@@ -147,20 +148,11 @@ func ExploreRepos(ctx *context.Context) {
 	})
 }
 
-// UserSearchOptions options when render search user page
-type UserSearchOptions struct {
-	Type     models.UserType
-	Counter  func() int64
-	Ranger   func(*models.SearchUserOptions) ([]*models.User, error)
-	PageSize int
-	TplName  base.TplName
-}
-
 // RenderUserSearch render user search page
-func RenderUserSearch(ctx *context.Context, opts *UserSearchOptions) {
-	page := ctx.QueryInt("page")
-	if page <= 1 {
-		page = 1
+func RenderUserSearch(ctx *context.Context, opts *models.SearchUserOptions, tplName base.TplName) {
+	opts.Page = ctx.QueryInt("page")
+	if opts.Page <= 1 {
+		opts.Page = 1
 	}
 
 	var (
@@ -189,40 +181,22 @@ func RenderUserSearch(ctx *context.Context, opts *UserSearchOptions) {
 		orderBy = "name ASC"
 	}
 
-	keyword := strings.Trim(ctx.Query("q"), " ")
-	if len(keyword) == 0 {
-		users, err = opts.Ranger(&models.SearchUserOptions{
-			OrderBy:  orderBy,
-			Page:     page,
-			PageSize: opts.PageSize,
-		})
+	opts.Keyword = strings.Trim(ctx.Query("q"), " ")
+	opts.OrderBy = orderBy
+	if len(opts.Keyword) == 0 || isKeywordValid(opts.Keyword) {
+		users, count, err = models.SearchUsers(opts)
 		if err != nil {
-			ctx.Handle(500, "opts.Ranger", err)
+			ctx.Handle(500, "SearchUsers", err)
 			return
 		}
-		count = opts.Counter()
-	} else {
-		if isKeywordValid(keyword) {
-			users, count, err = models.SearchUserByName(&models.SearchUserOptions{
-				Keyword:  keyword,
-				Type:     opts.Type,
-				OrderBy:  orderBy,
-				Page:     page,
-				PageSize: opts.PageSize,
-			})
-			if err != nil {
-				ctx.Handle(500, "SearchUserByName", err)
-				return
-			}
-		}
 	}
-	ctx.Data["Keyword"] = keyword
+	ctx.Data["Keyword"] = opts.Keyword
 	ctx.Data["Total"] = count
-	ctx.Data["Page"] = paginater.New(int(count), opts.PageSize, page, 5)
+	ctx.Data["Page"] = paginater.New(int(count), opts.PageSize, opts.Page, 5)
 	ctx.Data["Users"] = users
 	ctx.Data["ShowUserEmail"] = setting.UI.ShowUserEmail
 
-	ctx.HTML(200, opts.TplName)
+	ctx.HTML(200, tplName)
 }
 
 // ExploreUsers render explore users page
@@ -231,13 +205,11 @@ func ExploreUsers(ctx *context.Context) {
 	ctx.Data["PageIsExplore"] = true
 	ctx.Data["PageIsExploreUsers"] = true
 
-	RenderUserSearch(ctx, &UserSearchOptions{
+	RenderUserSearch(ctx, &models.SearchUserOptions{
 		Type:     models.UserTypeIndividual,
-		Counter:  models.CountUsers,
-		Ranger:   models.Users,
 		PageSize: setting.UI.ExplorePagingNum,
-		TplName:  tplExploreUsers,
-	})
+		IsActive: util.OptionalBoolTrue,
+	}, tplExploreUsers)
 }
 
 // ExploreOrganizations render explore organizations page
@@ -246,13 +218,10 @@ func ExploreOrganizations(ctx *context.Context) {
 	ctx.Data["PageIsExplore"] = true
 	ctx.Data["PageIsExploreOrganizations"] = true
 
-	RenderUserSearch(ctx, &UserSearchOptions{
+	RenderUserSearch(ctx, &models.SearchUserOptions{
 		Type:     models.UserTypeOrganization,
-		Counter:  models.CountOrganizations,
-		Ranger:   models.Organizations,
 		PageSize: setting.UI.ExplorePagingNum,
-		TplName:  tplExploreOrganizations,
-	})
+	}, tplExploreOrganizations)
 }
 
 // NotFound render 404 page