From 51b8d964c85b1fef52ffe7baeed0fb4a333ceeef Mon Sep 17 00:00:00 2001
From: 6543 <6543@obermui.de>
Date: Tue, 30 Apr 2024 17:36:28 +0200
Subject: [PATCH] Get repo assignees and reviewers should ignore deactivated
users (#30770) (#30782)
Backport #30770
If an user is deactivated, it should not be in the list of users who are
suggested to be assigned or review-requested.
old assignees or reviewers are not affected.
---
*Sponsored by Kithara Software GmbH*
(cherry picked from commit f2d8ccc5bb2df25557cc0d4d23f2cdd029358274)
Conflicts:
models/repo/user_repo_test.go
because there is one less fixture user compared to Gitea
---
models/repo/user_repo.go | 8 ++++++--
models/repo/user_repo_test.go | 23 ++++++++++++++++-------
tests/integration/api_repo_test.go | 4 +++-
3 files changed, 25 insertions(+), 10 deletions(-)
diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go
index d3fbd961bd..6790ee1da9 100644
--- a/models/repo/user_repo.go
+++ b/models/repo/user_repo.go
@@ -95,7 +95,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
}
}
@@ -117,7 +120,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:
diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go
index ad794beb9b..0433ff83d8 100644
--- a/models/repo/user_repo_test.go
+++ b/models/repo/user_repo_test.go
@@ -26,10 +26,17 @@ 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, 3)
- assert.Equal(t, users[0].ID, int64(15))
- assert.Equal(t, users[1].ID, int64(18))
- assert.Equal(t, users[2].ID, int64(16))
+ if assert.Len(t, users, 3) {
+ assert.ElementsMatch(t, []int64{15, 16, 18}, []int64{users[0].ID, users[1].ID, users[2].ID})
+ }
+
+ // do not return deactivated users
+ 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, 2) {
+ assert.NotContains(t, []int64{users[0].ID, users[1].ID}, 15)
+ }
}
func TestRepoGetReviewers(t *testing.T) {
@@ -41,17 +48,19 @@ 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, 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})
diff --git a/tests/integration/api_repo_test.go b/tests/integration/api_repo_test.go
index 2fb89cfa6e..96f38edc86 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) {