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*

diff --git a/models/repo/user_repo.go b/models/repo/user_repo.go
index 1c5412fe7da18774bfaed6e4dcde29f4e351bb3b..c305603e02070aad6cac6045aa1022f2bbeb3ff5 100644 (file)
--- 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:

diff --git a/models/repo/user_repo_test.go b/models/repo/user_repo_test.go
index 591dcea5b540f3a949de9adabc053af1d9e696cd..d2bf6dc9121c974a0ca00d9799552c39c9fa0027 100644 (file)
--- 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,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, 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
+       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.NotContains(t, []int64{users[0].ID, users[1].ID, users[2].ID}, 15)
+       }
 }
 
 func TestRepoGetReviewers(t *testing.T) {
@@ -38,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 bc2720d51e057422b109e58c1368c726aa86632d..f33827e58bd94f942de7bef0416b61b46e201ea6 100644 (file)
--- 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) {