From eceab9e26f671318d3b50e622736a514f384aee6 Mon Sep 17 00:00:00 2001 From: Norwin Date: Tue, 8 Mar 2022 22:48:47 +0100 Subject: [PATCH] Allow users to self-request a PR review (#19030) The review request feature was added in https://github.com/go-gitea/gitea/pull/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 --- models/repo.go | 16 ++++++++++++---- models/repo_test.go | 31 +++++++++++++++++++++++++++---- services/issue/assignee.go | 8 -------- 3 files changed, 39 insertions(+), 16 deletions(-) diff --git a/models/repo.go b/models/repo.go index 38527c74dc..53199bcca3 100644 --- a/models/repo.go +++ b/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 } diff --git a/models/repo_test.go b/models/repo_test.go index ea250be976..a93d84b81b 100644 --- a/models/repo_test.go +++ b/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 - repo2 := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 2}).(*repo_model.Repository) - reviewers, err = GetReviewers(repo2, 2, 2) + // should include doer if doer is not PR poster. + reviewers, err = GetReviewers(repo1, 11, 2) assert.NoError(t, err) - assert.Empty(t, reviewers) + 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, 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.Len(t, reviewers, 1) } func TestRepoGetReviewerTeams(t *testing.T) { diff --git a/services/issue/assignee.go b/services/issue/assignee.go index 4fdf0029c8..f09c51293b 100644 --- a/services/issue/assignee.go +++ b/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",