From f328a1d79454dde7c94bf716defde63388a639ce Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 29 Apr 2024 23:40:54 +0200 Subject: [PATCH 1/7] add test against deactivated user for GetRepoAssignees --- models/repo/user_repo_test.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index 591dcea5b540f..4dd37a50b9366 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -9,6 +9,7 @@ import ( "code.gitea.io/gitea/models/db" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/models/unittest" + user_model "code.gitea.io/gitea/models/user" "github.com/stretchr/testify/assert" ) @@ -25,8 +26,19 @@ func TestRepoAssignees(t *testing.T) { repo21 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 21}) users, err = repo_model.GetRepoAssignees(db.DefaultContext, repo21) assert.NoError(t, err) - assert.Len(t, users, 4) - assert.ElementsMatch(t, []int64{10, 15, 16, 18}, []int64{users[0].ID, users[1].ID, users[2].ID, users[3].ID}) + if assert.Len(t, users, 4) { + assert.ElementsMatch(t, []int64{10, 15, 16, 18}, []int64{users[0].ID, users[1].ID, users[2].ID, users[3].ID}) + } + + // do not return deactivated users + user15 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 15}) + user_model.UpdateUserCols(db.DefaultContext, user15, "is_active") + users, err = repo_model.GetRepoAssignees(db.DefaultContext, repo21) + assert.NoError(t, err) + if assert.Len(t, users, 3) { + assert.ElementsMatch(t, []int64{10, 15, 16, 18}, []int64{users[0].ID, users[1].ID, users[2].ID, users[3].ID}) + } + assert.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15) } func TestRepoGetReviewers(t *testing.T) { From d71562e6695dc12400678f101c8d1b55c456f00e Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Tue, 30 Apr 2024 01:34:32 +0200 Subject: [PATCH 2/7] add test against deactivated user for GetReviewers --- models/repo/user_repo_test.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index 4dd37a50b9366..b2d2b66762b90 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -50,7 +50,9 @@ func TestRepoGetReviewers(t *testing.T) { ctx := db.DefaultContext reviewers, err := repo_model.GetReviewers(ctx, repo1, 2, 2) assert.NoError(t, err) - assert.Len(t, reviewers, 4) + if assert.Len(t, reviewers, 4) { + assert.ElementsMatch(t, []int64{1, 4, 9, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID, reviewers[3].ID}) + } // should include doer if doer is not PR poster. reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 2) @@ -80,4 +82,13 @@ func TestRepoGetReviewers(t *testing.T) { reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 2) assert.NoError(t, err) assert.Len(t, reviewers, 1) + + // do not return deactivated users + user11 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 11}) + user_model.UpdateUserCols(db.DefaultContext, user11, "is_active") + reviewers, err = repo_model.GetReviewers(ctx, repo1, 2, 2) + assert.NoError(t, err) + if assert.Len(t, reviewers, 3) { + assert.NotContains(t, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}, 11) + } } From cca8c14f2c4dc101c0e4e5fb0a06e500dc573474 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Mon, 29 Apr 2024 17:21:53 +0200 Subject: [PATCH 3/7] User Suggestion for Assignees and Reviewers must be activated --- models/repo/user_repo.go | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go index 1c5412fe7da18..c305603e02070 100644 --- a/models/repo/user_repo.go +++ b/models/repo/user_repo.go @@ -130,7 +130,10 @@ func GetRepoAssignees(ctx context.Context, repo *Repository) (_ []*user_model.Us // and just waste 1 unit is cheaper than re-allocate memory once. users := make([]*user_model.User, 0, len(uniqueUserIDs)+1) if len(userIDs) > 0 { - if err = e.In("id", uniqueUserIDs.Values()).OrderBy(user_model.GetOrderByName()).Find(&users); err != nil { + if err = e.In("id", uniqueUserIDs.Values()). + Where(builder.Eq{"`user`.is_active": true}). + OrderBy(user_model.GetOrderByName()). + Find(&users); err != nil { return nil, err } } @@ -152,7 +155,8 @@ func GetReviewers(ctx context.Context, repo *Repository, doerID, posterID int64) return nil, err } - cond := builder.And(builder.Neq{"`user`.id": posterID}) + cond := builder.And(builder.Neq{"`user`.id": posterID}). + And(builder.Eq{"`user`.is_active": true}) if repo.IsPrivate || repo.Owner.Visibility == api.VisibleTypePrivate { // This a private repository: From 3df91cd62d24799f9d233965d8c2a086354b63af Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Tue, 30 Apr 2024 01:40:52 +0200 Subject: [PATCH 4/7] user9 in fixtures was already inactive --- models/repo/user_repo_test.go | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index b2d2b66762b90..28a0da759d48c 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -50,19 +50,19 @@ func TestRepoGetReviewers(t *testing.T) { ctx := db.DefaultContext reviewers, err := repo_model.GetReviewers(ctx, repo1, 2, 2) assert.NoError(t, err) - if assert.Len(t, reviewers, 4) { - assert.ElementsMatch(t, []int64{1, 4, 9, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID, reviewers[3].ID}) + if assert.Len(t, reviewers, 3) { + assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) } // should include doer if doer is not PR poster. reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 2) assert.NoError(t, err) - assert.Len(t, reviewers, 4) + assert.Len(t, reviewers, 3) // should not include PR poster, if PR poster would be otherwise eligible reviewers, err = repo_model.GetReviewers(ctx, repo1, 11, 4) assert.NoError(t, err) - assert.Len(t, reviewers, 3) + assert.Len(t, reviewers, 2) // test private user repo repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}) @@ -81,14 +81,4 @@ func TestRepoGetReviewers(t *testing.T) { reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 2) assert.NoError(t, err) - assert.Len(t, reviewers, 1) - - // do not return deactivated users - user11 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 11}) - user_model.UpdateUserCols(db.DefaultContext, user11, "is_active") - reviewers, err = repo_model.GetReviewers(ctx, repo1, 2, 2) - assert.NoError(t, err) - if assert.Len(t, reviewers, 3) { - assert.NotContains(t, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}, 11) - } } From 16f797ba101c87eb69db1b58767c6cbde9589831 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Tue, 30 Apr 2024 01:46:21 +0200 Subject: [PATCH 5/7] correct tests --- models/repo/user_repo_test.go | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index 28a0da759d48c..d309a770768c3 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -31,14 +31,12 @@ func TestRepoAssignees(t *testing.T) { } // do not return deactivated users - user15 := unittest.AssertExistsAndLoadBean(t, &user_model.User{ID: 15}) - user_model.UpdateUserCols(db.DefaultContext, user15, "is_active") + assert.NoError(t, user_model.UpdateUserCols(db.DefaultContext, &user_model.User{ID: 15, IsActive: false}, "is_active")) users, err = repo_model.GetRepoAssignees(db.DefaultContext, repo21) assert.NoError(t, err) if assert.Len(t, users, 3) { - assert.ElementsMatch(t, []int64{10, 15, 16, 18}, []int64{users[0].ID, users[1].ID, users[2].ID, users[3].ID}) + assert.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15) } - assert.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15) } func TestRepoGetReviewers(t *testing.T) { From 6fef0b74abff582b54a848eae38c535df7fb87e6 Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Tue, 30 Apr 2024 02:06:08 +0200 Subject: [PATCH 6/7] re-add unwanted rm --- models/repo/user_repo_test.go | 1 + 1 file changed, 1 insertion(+) diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go index d309a770768c3..d2bf6dc9121c9 100644 --- a/models/repo/user_repo_test.go +++ b/models/repo/user_repo_test.go @@ -79,4 +79,5 @@ func TestRepoGetReviewers(t *testing.T) { reviewers, err = repo_model.GetReviewers(ctx, repo3, 2, 2) assert.NoError(t, err) + assert.Len(t, reviewers, 1) } From 91011c473e5e27a41f47983cc30568c8d06fb0ac Mon Sep 17 00:00:00 2001 From: "m.huber" Date: Tue, 30 Apr 2024 03:54:17 +0200 Subject: [PATCH 7/7] adjust TestAPIRepoGetReviewers too --- tests/integration/api_repo_test.go | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go index bc2720d51e057..f33827e58bd94 100644 --- a/tests/integration/api_repo_test.go +++ b/tests/integration/api_repo_test.go @@ -684,7 +684,9 @@ func TestAPIRepoGetReviewers(t *testing.T) { resp := MakeRequest(t, req, http.StatusOK) var reviewers []*api.User DecodeJSON(t, resp, &reviewers) - assert.Len(t, reviewers, 4) + if assert.Len(t, reviewers, 3) { + assert.ElementsMatch(t, []int64{1, 4, 11}, []int64{reviewers[0].ID, reviewers[1].ID, reviewers[2].ID}) + } } func TestAPIRepoGetAssignees(t *testing.T) {