Allow users to self-request a PR review (#19030)

The review request feature was added in #10756,
where the doer got explicitly excluded from available reviewers. I don't see a
functionality or security related reason to forbid this case.

As shown by GitHubs implementation, it may be useful to self-request a review,
to be reminded oneselves about reviewing, while communicating to team mates that a
review is missing.

Co-authored-by: delvh <dev.lh@web.de>

Author: noerw

models/repo.go

@@ -225,13 +225,21 @@ func getReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post
 		// This a private repository:
 		// Anyone who can read the repository is a requestable reviewer
 		if err := e.
-			SQL("SELECT * FROM `user` WHERE id in (SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?)) ORDER BY name",
+			SQL("SELECT * FROM `user` WHERE id in ("+
+				"SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id != ?"+ // private org repos
+				") ORDER BY name",
 				repo.ID, perm.AccessModeRead,
-				doerID, posterID).
+				posterID).
 			Find(&users); err != nil {
 			return nil, err
 		}
 
+		if repo.Owner.Type == user_model.UserTypeIndividual && repo.Owner.ID != posterID {
+			// as private *user* repos don't generate an entry in the `access` table,
+			// the owner of a private repo needs to be explicitly added.
+			users = append(users, repo.Owner)
+		}
+
 		return users, nil
 	}
 
@@ -244,11 +252,11 @@ func getReviewers(ctx context.Context, repo *repo_model.Repository, doerID, post
 			"SELECT user_id FROM `watch` WHERE repo_id = ? AND mode IN (?, ?) "+
 			"UNION "+
 			"SELECT uid AS user_id FROM `org_user` WHERE org_id = ? "+
-			") AND id NOT IN (?, ?) ORDER BY name",
+			") AND id != ? ORDER BY name",
 			repo.ID, perm.AccessModeRead,
 			repo.ID, repo_model.WatchModeNormal, repo_model.WatchModeAuto,
 			repo.OwnerID,
-			doerID, posterID).
+			posterID).
 		Find(&users); err != nil {
 		return nil, err
 	}

models/repo_test.go

@@ -120,11 +120,34 @@ func TestRepoGetReviewers(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Len(t, reviewers, 4)
 
-	// test private repo
+	// should include doer if doer is not PR poster.
+	reviewers, err = GetReviewers(repo1, 11, 2)
+	assert.NoError(t, err)
+	assert.Len(t, reviewers, 4)
+
+	// should not include PR poster, if PR poster would be otherwise eligible
+	reviewers, err = GetReviewers(repo1, 11, 4)
+	assert.NoError(t, err)
+	assert.Len(t, reviewers, 3)
+
+	// test private user repo
 	repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}).(*repo_model.Repository)
-	reviewers, err = GetReviewers(repo2, 2, 2)
+
+	reviewers, err = GetReviewers(repo2, 2, 4)
+	assert.NoError(t, err)
+	assert.Len(t, reviewers, 1)
+	assert.EqualValues(t, reviewers[0].ID, 2)
+
+	// test private org repo
+	repo3 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 3}).(*repo_model.Repository)
+
+	reviewers, err = GetReviewers(repo3, 2, 1)
+	assert.NoError(t, err)
+	assert.Len(t, reviewers, 2)
+
+	reviewers, err = GetReviewers(repo3, 2, 2)
 	assert.NoError(t, err)
-	assert.Empty(t, reviewers)
+	assert.Len(t, reviewers, 1)
 }
 
 func TestRepoGetReviewerTeams(t *testing.T) {

services/issue/assignee.go

@@ -140,14 +140,6 @@ func IsValidReviewRequest(reviewer, doer *user_model.User, isAdd bool, issue *mo
 			}
 		}
 
-		if doer.ID == reviewer.ID {
-			return models.ErrNotValidReviewRequest{
-				Reason: "doer can't be reviewer",
-				UserID: doer.ID,
-				RepoID: issue.Repo.ID,
-			}
-		}
-
 		if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 {
 			return models.ErrNotValidReviewRequest{
 				Reason: "poster of pr can't be reviewer",