From 6eeadb2082193f8e3dc5269dfc6da2daef7eb6a1 Mon Sep 17 00:00:00 2001 From: Lunny Xiao Date: Wed, 25 Oct 2017 01:36:19 +0800 Subject: [PATCH] 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 | 2 +- models/fixtures/user.yml | 2 + models/fixtures/watch.yml | 2 +- models/issue_watch_test.go | 2 +- models/notification_test.go | 7 ++- models/org.go | 17 -------- models/org_test.go | 21 --------- models/repo_watch_test.go | 16 ++++--- models/user.go | 76 ++++++++++++++------------------- models/user_test.go | 51 ++++++++++++++++++++++ modules/util/util.go | 15 +++++++ routers/admin/orgs.go | 7 +-- routers/admin/users.go | 7 +-- routers/api/v1/user/user.go | 2 +- routers/home.go | 67 ++++++++--------------------- 15 files changed, 143 insertions(+), 151 deletions(-) diff --git a/models/fixtures/issue_watch.yml b/models/fixtures/issue_watch.yml index 596662d20..75351eb17 100644 --- a/models/fixtures/issue_watch.yml +++ b/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 diff --git a/models/fixtures/user.yml b/models/fixtures/user.yml index 73ec1c859..1e0625598 100644 --- a/models/fixtures/user.yml +++ b/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 diff --git a/models/fixtures/watch.yml b/models/fixtures/watch.yml index 2ff888b7c..5cd3b55fc 100644 --- a/models/fixtures/watch.yml +++ b/models/fixtures/watch.yml @@ -10,5 +10,5 @@ - id: 3 - user_id: 10 + user_id: 9 repo_id: 1 diff --git a/models/issue_watch_test.go b/models/issue_watch_test.go index a4819b222..ce0d2045c 100644 --- a/models/issue_watch_test.go +++ b/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) diff --git a/models/notification_test.go b/models/notification_test.go index c1b7d5052..07d9ee764 100644 --- a/models/notification_test.go +++ b/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) { diff --git a/models/org.go b/models/org.go index 3c1657260..4a4fcb4ad 100644 --- a/models/org.go +++ b/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() diff --git a/models/org_test.go b/models/org_test.go index c7bdb8b5d..8f59af074 100644 --- a/models/org_test.go +++ b/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) diff --git a/models/repo_watch_test.go b/models/repo_watch_test.go index 1277b156c..852f09f1c 100644 --- a/models/repo_watch_test.go +++ b/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, }) diff --git a/models/user.go b/models/user.go index 835a775eb..337c39efe 100644 --- a/models/user.go +++ b/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,15 +1269,34 @@ 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}, + )) + } + + if !opts.IsActive.IsNone() { + cond = cond.And(builder.Eq{"is_active": opts.IsActive.IsTrue()}) + } + + 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) } - opts.Keyword = strings.ToLower(opts.Keyword) if opts.PageSize <= 0 || opts.PageSize > setting.UI.ExplorePagingNum { opts.PageSize = setting.UI.ExplorePagingNum @@ -1300,29 +1304,15 @@ func SearchUserByName(opts *SearchUserOptions) (users []*User, _ int64, _ error) if opts.Page <= 0 { opts.Page = 1 } + if len(opts.OrderBy) == 0 { + opts.OrderBy = "name ASC" + } 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}, - ), - ) - - 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) - } - return users, count, sess.Find(&users) + 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 diff --git a/models/user_test.go b/models/user_test.go index a16f97953..7ac9ebb0f 100644 --- a/models/user_test.go +++ b/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()) diff --git a/modules/util/util.go b/modules/util/util.go index 485996538..104c80f52 100644 --- a/modules/util/util.go +++ b/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 { diff --git a/routers/admin/orgs.go b/routers/admin/orgs.go index eeb9002bc..c556d3fcf 100644 --- a/routers/admin/orgs.go +++ 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) } diff --git a/routers/admin/users.go b/routers/admin/users.go index d48002914..c22accc20 100644 --- a/routers/admin/users.go +++ b/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 diff --git a/routers/api/v1/user/user.go b/routers/api/v1/user/user.go index 0ab6eaf44..5a59fd7ca 100644 --- a/routers/api/v1/user/user.go +++ b/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, diff --git a/routers/home.go b/routers/home.go index 768eb2207..d653d1e84 100644 --- a/routers/home.go +++ b/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