Get repo assignees and reviewers should ignore deactivated users (#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*

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:

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})

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) {