Add team support for review request (#12039)

Add team support for review request

Block #11355

Signed-off-by: a1012112796 <1012112796@qq.com>
Signed-off-by: Andrew Thornton <art27@cantab.net>

Co-authored-by: Lauris BH <lauris@nix.lv>
Co-authored-by: Andrew Thornton <art27@cantab.net>
This commit is contained in:
赵智超 2020-10-13 03:55:13 +08:00 committed by GitHub
parent b546eda7a8
commit 8be3e439c2
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
17 changed files with 956 additions and 293 deletions

View file

@ -1994,6 +1994,26 @@ func (err ErrReviewNotExist) Error() string {
return fmt.Sprintf("review does not exist [id: %d]", err.ID) return fmt.Sprintf("review does not exist [id: %d]", err.ID)
} }
// ErrNotValidReviewRequest an not allowed review request modify
type ErrNotValidReviewRequest struct {
Reason string
UserID int64
RepoID int64
}
// IsErrNotValidReviewRequest checks if an error is a ErrNotValidReviewRequest.
func IsErrNotValidReviewRequest(err error) bool {
_, ok := err.(ErrReviewNotExist)
return ok
}
func (err ErrNotValidReviewRequest) Error() string {
return fmt.Sprintf("%s [user_id: %d, repo_id: %d]",
err.Reason,
err.UserID,
err.RepoID)
}
// ________ _____ __ .__ // ________ _____ __ .__
// \_____ \ / _ \ __ ___/ |_| |__ // \_____ \ / _ \ __ ___/ |_| |__
// / | \ / /_\ \| | \ __\ | \ // / | \ / /_\ \| | \ __\ | \

View file

@ -44,6 +44,7 @@
reviewer_id: 2 reviewer_id: 2
issue_id: 3 issue_id: 3
content: "New review 3" content: "New review 3"
original_author_id: 0
updated_unix: 946684811 updated_unix: 946684811
created_unix: 946684811 created_unix: 946684811
- -
@ -52,6 +53,7 @@
reviewer_id: 3 reviewer_id: 3
issue_id: 3 issue_id: 3
content: "New review 4" content: "New review 4"
original_author_id: 0
updated_unix: 946684812 updated_unix: 946684812
created_unix: 946684812 created_unix: 946684812
- -
@ -59,6 +61,7 @@
type: 1 type: 1
reviewer_id: 4 reviewer_id: 4
issue_id: 3 issue_id: 3
original_author_id: 0
content: "New review 5" content: "New review 5"
commit_id: 8091a55037cd59e47293aca02981b5a67076b364 commit_id: 8091a55037cd59e47293aca02981b5a67076b364
stale: true stale: true
@ -72,6 +75,7 @@
content: "New review 3 rejected" content: "New review 3 rejected"
updated_unix: 946684814 updated_unix: 946684814
created_unix: 946684814 created_unix: 946684814
original_author_id: 0
- -
id: 10 id: 10

View file

@ -137,6 +137,8 @@ type Comment struct {
AssigneeID int64 AssigneeID int64
RemovedAssignee bool RemovedAssignee bool
Assignee *User `xorm:"-"` Assignee *User `xorm:"-"`
AssigneeTeamID int64 `xorm:"NOT NULL DEFAULT 0"`
AssigneeTeam *Team `xorm:"-"`
ResolveDoerID int64 ResolveDoerID int64
ResolveDoer *User `xorm:"-"` ResolveDoer *User `xorm:"-"`
OldTitle string OldTitle string
@ -487,11 +489,11 @@ func (c *Comment) UpdateAttachments(uuids []string) error {
return sess.Commit() return sess.Commit()
} }
// LoadAssigneeUser if comment.Type is CommentTypeAssignees, then load assignees // LoadAssigneeUserAndTeam if comment.Type is CommentTypeAssignees, then load assignees
func (c *Comment) LoadAssigneeUser() error { func (c *Comment) LoadAssigneeUserAndTeam() error {
var err error var err error
if c.AssigneeID > 0 { if c.AssigneeID > 0 && c.Assignee == nil {
c.Assignee, err = getUserByID(x, c.AssigneeID) c.Assignee, err = getUserByID(x, c.AssigneeID)
if err != nil { if err != nil {
if !IsErrUserNotExist(err) { if !IsErrUserNotExist(err) {
@ -499,6 +501,25 @@ func (c *Comment) LoadAssigneeUser() error {
} }
c.Assignee = NewGhostUser() c.Assignee = NewGhostUser()
} }
} else if c.AssigneeTeamID > 0 && c.AssigneeTeam == nil {
if err = c.LoadIssue(); err != nil {
return err
}
if err = c.Issue.LoadRepo(); err != nil {
return err
}
if err = c.Issue.Repo.GetOwner(); err != nil {
return err
}
if c.Issue.Repo.Owner.IsOrganization() {
c.AssigneeTeam, err = GetTeamByID(c.AssigneeTeamID)
if err != nil && !IsErrTeamNotExist(err) {
return err
}
}
} }
return nil return nil
} }
@ -685,6 +706,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err
ProjectID: opts.ProjectID, ProjectID: opts.ProjectID,
RemovedAssignee: opts.RemovedAssignee, RemovedAssignee: opts.RemovedAssignee,
AssigneeID: opts.AssigneeID, AssigneeID: opts.AssigneeID,
AssigneeTeamID: opts.AssigneeTeamID,
CommitID: opts.CommitID, CommitID: opts.CommitID,
CommitSHA: opts.CommitSHA, CommitSHA: opts.CommitSHA,
Line: opts.LineNum, Line: opts.LineNum,
@ -849,6 +871,7 @@ type CreateCommentOptions struct {
OldProjectID int64 OldProjectID int64
ProjectID int64 ProjectID int64
AssigneeID int64 AssigneeID int64
AssigneeTeamID int64
RemovedAssignee bool RemovedAssignee bool
OldTitle string OldTitle string
NewTitle string NewTitle string

View file

@ -240,6 +240,8 @@ var migrations = []Migration{
NewMigration("set default password algorithm to Argon2", setDefaultPasswordToArgon2), NewMigration("set default password algorithm to Argon2", setDefaultPasswordToArgon2),
// v152 -> v153 // v152 -> v153
NewMigration("add TrustModel field to Repository", addTrustModelToRepository), NewMigration("add TrustModel field to Repository", addTrustModelToRepository),
// v153 > v154
NewMigration("add Team review request support", addTeamReviewRequestSupport),
} }
// GetCurrentDBVersion returns the current db version // GetCurrentDBVersion returns the current db version

25
models/migrations/v153.go Normal file
View file

@ -0,0 +1,25 @@
// Copyright 2020 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.
package migrations
import (
"xorm.io/xorm"
)
func addTeamReviewRequestSupport(x *xorm.Engine) error {
type Review struct {
ReviewerTeamID int64 `xorm:"NOT NULL DEFAULT 0"`
}
type Comment struct {
AssigneeTeamID int64 `xorm:"NOT NULL DEFAULT 0"`
}
if err := x.Sync2(new(Review)); err != nil {
return err
}
return x.Sync2(new(Comment))
}

View file

@ -694,10 +694,19 @@ func (repo *Repository) GetAssignees() (_ []*User, err error) {
return repo.getAssignees(x) return repo.getAssignees(x)
} }
func (repo *Repository) getReviewersPrivate(e Engine, doerID, posterID int64) (users []*User, err error) { func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User, error) {
users = make([]*User, 0, 20) // Get the owner of the repository - this often already pre-cached and if so saves complexity for the following queries
if err := repo.getOwner(e); err != nil {
return nil, err
}
if err = e. var users []*User
if repo.IsPrivate ||
(repo.Owner.IsOrganization() && repo.Owner.Visibility == api.VisibleTypePrivate) {
// 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 NOT IN ( ?, ?)) ORDER BY name",
repo.ID, AccessModeRead, repo.ID, AccessModeRead,
doerID, posterID). doerID, posterID).
@ -706,20 +715,16 @@ func (repo *Repository) getReviewersPrivate(e Engine, doerID, posterID int64) (u
} }
return users, nil return users, nil
} }
func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_ []*User, err error) { // This is a "public" repository:
// Any user that has write access or who is a watcher can be requested to review
users := make([]*User, 0) if err := e.
SQL("SELECT * FROM `user` WHERE id IN ( "+
const SQLCmd = "SELECT * FROM `user` WHERE id IN ( " + "SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?) "+
"SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?) " + "UNION "+
"UNION " + "SELECT user_id FROM `watch` WHERE repo_id = ? AND user_id NOT IN ( ?, ?) AND mode IN (?, ?) "+
"SELECT user_id FROM `watch` WHERE repo_id = ? AND user_id NOT IN ( ?, ?) AND mode IN (?, ?) " + ") ORDER BY name",
") ORDER BY name"
if err = e.
SQL(SQLCmd,
repo.ID, AccessModeRead, doerID, posterID, repo.ID, AccessModeRead, doerID, posterID,
repo.ID, doerID, posterID, RepoWatchModeNormal, RepoWatchModeAuto). repo.ID, doerID, posterID, RepoWatchModeNormal, RepoWatchModeAuto).
Find(&users); err != nil { Find(&users); err != nil {
@ -729,27 +734,30 @@ func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_
return users, nil return users, nil
} }
func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []*User, err error) { // GetReviewers get all users can be requested to review:
if err = repo.getOwner(e); err != nil { // * for private repositories this returns all users that have read access or higher to the repository.
// * for public repositories this returns all users that have write access or higher to the repository,
// and all repo watchers.
// TODO: may be we should hava a busy choice for users to block review request to them.
func (repo *Repository) GetReviewers(doerID, posterID int64) ([]*User, error) {
return repo.getReviewers(x, doerID, posterID)
}
// GetReviewerTeams get all teams can be requested to review
func (repo *Repository) GetReviewerTeams() ([]*Team, error) {
if err := repo.GetOwner(); err != nil {
return nil, err
}
if !repo.Owner.IsOrganization() {
return nil, nil
}
teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
if err != nil {
return nil, err return nil, err
} }
if repo.IsPrivate || return teams, err
(repo.Owner.IsOrganization() && repo.Owner.Visibility == api.VisibleTypePrivate) {
users, err = repo.getReviewersPrivate(x, doerID, posterID)
} else {
users, err = repo.getReviewersPublic(x, doerID, posterID)
}
return
}
// GetReviewers get all users can be requested to review
// for private rpo , that return all users that have read access or higher to the repository.
// but for public rpo, that return all users that have write access or higher to the repository,
// and all repo watchers.
// TODO: may be we should hava a busy choice for users to block review request to them.
func (repo *Repository) GetReviewers(doerID, posterID int64) (_ []*User, err error) {
return repo.getReviewers(x, doerID, posterID)
} }
// GetMilestoneByID returns the milestone belongs to repository by given ID. // GetMilestoneByID returns the milestone belongs to repository by given ID.

View file

@ -193,3 +193,34 @@ func TestDoctorUserStarNum(t *testing.T) {
assert.NoError(t, DoctorUserStarNum()) assert.NoError(t, DoctorUserStarNum())
} }
func TestRepoGetReviewers(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
// test public repo
repo1 := AssertExistsAndLoadBean(t, &Repository{ID: 1}).(*Repository)
reviewers, err := repo1.GetReviewers(2, 2)
assert.NoError(t, err)
assert.Equal(t, 4, len(reviewers))
// test private repo
repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
reviewers, err = repo2.GetReviewers(2, 2)
assert.NoError(t, err)
assert.Equal(t, 0, len(reviewers))
}
func TestRepoGetReviewerTeams(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
repo2 := AssertExistsAndLoadBean(t, &Repository{ID: 2}).(*Repository)
teams, err := repo2.GetReviewerTeams()
assert.NoError(t, err)
assert.Empty(t, teams)
repo3 := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
teams, err = repo3.GetReviewerTeams()
assert.NoError(t, err)
assert.Equal(t, 2, len(teams))
}

View file

@ -8,6 +8,7 @@ import (
"fmt" "fmt"
"strings" "strings"
"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/timeutil" "code.gitea.io/gitea/modules/timeutil"
"xorm.io/builder" "xorm.io/builder"
@ -54,6 +55,8 @@ type Review struct {
Type ReviewType Type ReviewType
Reviewer *User `xorm:"-"` Reviewer *User `xorm:"-"`
ReviewerID int64 `xorm:"index"` ReviewerID int64 `xorm:"index"`
ReviewerTeamID int64 `xorm:"NOT NULL DEFAULT 0"`
ReviewerTeam *Team `xorm:"-"`
OriginalAuthor string OriginalAuthor string
OriginalAuthorID int64 OriginalAuthorID int64
Issue *Issue `xorm:"-"` Issue *Issue `xorm:"-"`
@ -98,18 +101,32 @@ func (r *Review) loadIssue(e Engine) (err error) {
} }
func (r *Review) loadReviewer(e Engine) (err error) { func (r *Review) loadReviewer(e Engine) (err error) {
if r.Reviewer != nil || r.ReviewerID == 0 { if r.ReviewerID == 0 || r.Reviewer != nil {
return nil return
} }
r.Reviewer, err = getUserByID(e, r.ReviewerID) r.Reviewer, err = getUserByID(e, r.ReviewerID)
return return
} }
func (r *Review) loadReviewerTeam(e Engine) (err error) {
if r.ReviewerTeamID == 0 || r.ReviewerTeam != nil {
return
}
r.ReviewerTeam, err = getTeamByID(e, r.ReviewerTeamID)
return
}
// LoadReviewer loads reviewer // LoadReviewer loads reviewer
func (r *Review) LoadReviewer() error { func (r *Review) LoadReviewer() error {
return r.loadReviewer(x) return r.loadReviewer(x)
} }
// LoadReviewerTeam loads reviewer team
func (r *Review) LoadReviewerTeam() error {
return r.loadReviewerTeam(x)
}
func (r *Review) loadAttributes(e Engine) (err error) { func (r *Review) loadAttributes(e Engine) (err error) {
if err = r.loadIssue(e); err != nil { if err = r.loadIssue(e); err != nil {
return return
@ -120,6 +137,9 @@ func (r *Review) loadAttributes(e Engine) (err error) {
if err = r.loadReviewer(e); err != nil { if err = r.loadReviewer(e); err != nil {
return return
} }
if err = r.loadReviewerTeam(e); err != nil {
return
}
return return
} }
@ -193,17 +213,18 @@ type CreateReviewOptions struct {
Type ReviewType Type ReviewType
Issue *Issue Issue *Issue
Reviewer *User Reviewer *User
ReviewerTeam *Team
Official bool Official bool
CommitID string CommitID string
Stale bool Stale bool
} }
// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals) // IsOfficialReviewer check if at least one of the provided reviewers can make official reviews in issue (counts towards required approvals)
func IsOfficialReviewer(issue *Issue, reviewer *User) (bool, error) { func IsOfficialReviewer(issue *Issue, reviewers ...*User) (bool, error) {
return isOfficialReviewer(x, issue, reviewer) return isOfficialReviewer(x, issue, reviewers...)
} }
func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) { func isOfficialReviewer(e Engine, issue *Issue, reviewers ...*User) (bool, error) {
pr, err := getPullRequestByIssueID(e, issue.ID) pr, err := getPullRequestByIssueID(e, issue.ID)
if err != nil { if err != nil {
return false, err return false, err
@ -215,7 +236,38 @@ func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
return false, nil return false, nil
} }
return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer) for _, reviewer := range reviewers {
official, err := pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
if official || err != nil {
return official, err
}
}
return false, nil
}
// IsOfficialReviewerTeam check if reviewer in this team can make official reviews in issue (counts towards required approvals)
func IsOfficialReviewerTeam(issue *Issue, team *Team) (bool, error) {
return isOfficialReviewerTeam(x, issue, team)
}
func isOfficialReviewerTeam(e Engine, issue *Issue, team *Team) (bool, error) {
pr, err := getPullRequestByIssueID(e, issue.ID)
if err != nil {
return false, err
}
if err = pr.loadProtectedBranch(e); err != nil {
return false, err
}
if pr.ProtectedBranch == nil {
return false, nil
}
if !pr.ProtectedBranch.EnableApprovalsWhitelist {
return team.Authorize >= AccessModeWrite, nil
}
return base.Int64sContains(pr.ProtectedBranch.ApprovalsWhitelistTeamIDs, team.ID), nil
} }
func createReview(e Engine, opts CreateReviewOptions) (*Review, error) { func createReview(e Engine, opts CreateReviewOptions) (*Review, error) {
@ -224,12 +276,20 @@ func createReview(e Engine, opts CreateReviewOptions) (*Review, error) {
Issue: opts.Issue, Issue: opts.Issue,
IssueID: opts.Issue.ID, IssueID: opts.Issue.ID,
Reviewer: opts.Reviewer, Reviewer: opts.Reviewer,
ReviewerID: opts.Reviewer.ID, ReviewerTeam: opts.ReviewerTeam,
Content: opts.Content, Content: opts.Content,
Official: opts.Official, Official: opts.Official,
CommitID: opts.CommitID, CommitID: opts.CommitID,
Stale: opts.Stale, Stale: opts.Stale,
} }
if opts.Reviewer != nil {
review.ReviewerID = opts.Reviewer.ID
} else {
if review.Type != ReviewTypeRequest {
review.Type = ReviewTypeRequest
}
review.ReviewerTeamID = opts.ReviewerTeam.ID
}
if _, err := e.Insert(review); err != nil { if _, err := e.Insert(review); err != nil {
return nil, err return nil, err
} }
@ -311,14 +371,13 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil { if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
return nil, nil, err return nil, nil, err
} }
official, err = isOfficialReviewer(sess, issue, doer) if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
if err != nil {
return nil, nil, err return nil, nil, err
} }
} }
// No current review. Create a new one! // No current review. Create a new one!
review, err = createReview(sess, CreateReviewOptions{ if review, err = createReview(sess, CreateReviewOptions{
Type: reviewType, Type: reviewType,
Issue: issue, Issue: issue,
Reviewer: doer, Reviewer: doer,
@ -326,8 +385,7 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
Official: official, Official: official,
CommitID: commitID, CommitID: commitID,
Stale: stale, Stale: stale,
}) }); err != nil {
if err != nil {
return nil, nil, err return nil, nil, err
} }
} else { } else {
@ -343,8 +401,7 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil { if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
return nil, nil, err return nil, nil, err
} }
official, err = isOfficialReviewer(sess, issue, doer) if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
if err != nil {
return nil, nil, err return nil, nil, err
} }
} }
@ -373,13 +430,34 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
return nil, nil, err return nil, nil, err
} }
// try to remove team review request if need
if issue.Repo.Owner.IsOrganization() && (reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject) {
teamReviewRequests := make([]*Review, 0, 10)
if err := sess.SQL("SELECT * FROM review WHERE reviewer_team_id > 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
return nil, nil, err
}
for _, teamReviewRequest := range teamReviewRequests {
ok, err := isTeamMember(sess, issue.Repo.OwnerID, teamReviewRequest.ReviewerTeamID, doer.ID)
if err != nil {
return nil, nil, err
} else if !ok {
continue
}
if _, err := sess.Delete(teamReviewRequest); err != nil {
return nil, nil, err
}
}
}
comm.Review = review comm.Review = review
return review, comm, sess.Commit() return review, comm, sess.Commit()
} }
// GetReviewersByIssueID gets the latest review of each reviewer for a pull request // GetReviewersByIssueID gets the latest review of each reviewer for a pull request
func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) { func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
reviewsUnfiltered := []*Review{} reviews := make([]*Review, 0, 10)
sess := x.NewSession() sess := x.NewSession()
defer sess.Close() defer sess.Close()
@ -388,40 +466,67 @@ func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
} }
// Get latest review of each reviwer, sorted in order they were made // Get latest review of each reviwer, sorted in order they were made
if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND type in (?, ?, ?) GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC", if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest). issueID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
Find(&reviewsUnfiltered); err != nil { Find(&reviews); err != nil {
return nil, err return nil, err
} }
// Load reviewer and skip if user is deleted teamReviewRequests := make([]*Review, 0, 5)
for _, review := range reviewsUnfiltered { if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id <> 0 AND original_author_id = 0 GROUP BY issue_id, reviewer_team_id) ORDER BY review.updated_unix ASC",
if err = review.loadReviewer(sess); err != nil { issueID).
if !IsErrUserNotExist(err) { Find(&teamReviewRequests); err != nil {
return nil, err return nil, err
} }
} else {
reviews = append(reviews, review) if len(teamReviewRequests) > 0 {
} reviews = append(reviews, teamReviewRequests...)
} }
return reviews, nil return reviews, nil
} }
// GetReviewerByIssueIDAndUserID get the latest review of reviewer for a pull request // GetReviewByIssueIDAndUserID get the latest review of reviewer for a pull request
func GetReviewerByIssueIDAndUserID(issueID, userID int64) (review *Review, err error) { func GetReviewByIssueIDAndUserID(issueID, userID int64) (*Review, error) {
return getReviewerByIssueIDAndUserID(x, issueID, userID) return getReviewByIssueIDAndUserID(x, issueID, userID)
} }
func getReviewerByIssueIDAndUserID(e Engine, issueID, userID int64) (review *Review, err error) { func getReviewByIssueIDAndUserID(e Engine, issueID, userID int64) (*Review, error) {
review := new(Review)
has, err := e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND original_author_id = 0 AND type in (?, ?, ?))",
issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
Get(review)
if err != nil {
return nil, err
}
if !has {
return nil, ErrReviewNotExist{}
}
return review, nil
}
// GetTeamReviewerByIssueIDAndTeamID get the latest review requst of reviewer team for a pull request
func GetTeamReviewerByIssueIDAndTeamID(issueID, teamID int64) (review *Review, err error) {
return getTeamReviewerByIssueIDAndTeamID(x, issueID, teamID)
}
func getTeamReviewerByIssueIDAndTeamID(e Engine, issueID, teamID int64) (review *Review, err error) {
review = new(Review) review = new(Review)
if _, err := e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND type in (?, ?, ?))", has := false
issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest). if has, err = e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = ?)",
issueID, teamID).
Get(review); err != nil { Get(review); err != nil {
return nil, err return nil, err
} }
if !has {
return nil, ErrReviewNotExist{0}
}
return return
} }
@ -482,10 +587,16 @@ func InsertReviews(reviews []*Review) error {
} }
// AddReviewRequest add a review request from one reviewer // AddReviewRequest add a review request from one reviewer
func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Comment, err error) { func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID) sess := x.NewSession()
if err != nil { defer sess.Close()
return if err := sess.Begin(); err != nil {
return nil, err
}
review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
if err != nil && !IsErrReviewNotExist(err) {
return nil, err
} }
// skip it when reviewer hase been request to review // skip it when reviewer hase been request to review
@ -493,46 +604,26 @@ func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Commen
return nil, nil return nil, nil
} }
sess := x.NewSession() official, err := isOfficialReviewer(sess, issue, reviewer, doer)
defer sess.Close()
if err := sess.Begin(); err != nil {
return nil, err
}
var official bool
official, err = isOfficialReviewer(sess, issue, reviewer)
if err != nil { if err != nil {
return nil, err return nil, err
} } else if official {
if !official {
official, err = isOfficialReviewer(sess, issue, doer)
if err != nil {
return nil, err
}
}
if official {
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, reviewer.ID); err != nil { if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, reviewer.ID); err != nil {
return nil, err return nil, err
} }
} }
_, err = createReview(sess, CreateReviewOptions{ if _, err = createReview(sess, CreateReviewOptions{
Type: ReviewTypeRequest, Type: ReviewTypeRequest,
Issue: issue, Issue: issue,
Reviewer: reviewer, Reviewer: reviewer,
Official: official, Official: official,
Stale: false, Stale: false,
}) }); err != nil {
return nil, err
if err != nil {
return
} }
comment, err = createComment(sess, &CreateCommentOptions{ comment, err := createComment(sess, &CreateCommentOptions{
Type: CommentTypeReviewRequest, Type: CommentTypeReviewRequest,
Doer: doer, Doer: doer,
Repo: issue.Repo, Repo: issue.Repo,
@ -540,7 +631,6 @@ func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Commen
RemovedAssignee: false, // Use RemovedAssignee as !isRequest RemovedAssignee: false, // Use RemovedAssignee as !isRequest
AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID
}) })
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -549,40 +639,34 @@ func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Commen
} }
//RemoveReviewRequest remove a review request from one reviewer //RemoveReviewRequest remove a review request from one reviewer
func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Comment, err error) { func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID)
if err != nil {
return
}
if review.Type != ReviewTypeRequest {
return nil, nil
}
sess := x.NewSession() sess := x.NewSession()
defer sess.Close() defer sess.Close()
if err := sess.Begin(); err != nil { if err := sess.Begin(); err != nil {
return nil, err return nil, err
} }
_, err = sess.Delete(review) review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
if err != nil { if err != nil && !IsErrReviewNotExist(err) {
return nil, err return nil, err
} }
var official bool if review == nil || review.Type != ReviewTypeRequest {
official, err = isOfficialReviewer(sess, issue, reviewer) return nil, nil
if err != nil {
return
} }
if official { if _, err = sess.Delete(review); err != nil {
// recalculate which is the latest official review from that user return nil, err
var review *Review }
review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID) official, err := isOfficialReviewer(sess, issue, reviewer)
if err != nil { if err != nil {
return nil, err return nil, err
} else if official {
// recalculate the latest official review for reviewer
review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
if err != nil && !IsErrReviewNotExist(err) {
return nil, err
} }
if review != nil { if review != nil {
@ -592,11 +676,7 @@ func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com
} }
} }
if err != nil { comment, err := createComment(sess, &CreateCommentOptions{
return nil, err
}
comment, err = createComment(sess, &CreateCommentOptions{
Type: CommentTypeReviewRequest, Type: CommentTypeReviewRequest,
Doer: doer, Doer: doer,
Repo: issue.Repo, Repo: issue.Repo,
@ -604,7 +684,6 @@ func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com
RemovedAssignee: true, // Use RemovedAssignee as !isRequest RemovedAssignee: true, // Use RemovedAssignee as !isRequest
AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID
}) })
if err != nil { if err != nil {
return nil, err return nil, err
} }
@ -612,6 +691,123 @@ func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com
return comment, sess.Commit() return comment, sess.Commit()
} }
// AddTeamReviewRequest add a review request from one team
func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, error) {
sess := x.NewSession()
defer sess.Close()
if err := sess.Begin(); err != nil {
return nil, err
}
review, err := getTeamReviewerByIssueIDAndTeamID(sess, issue.ID, reviewer.ID)
if err != nil && !IsErrReviewNotExist(err) {
return nil, err
}
// This team already has been requested to review - therefore skip this.
if review != nil {
return nil, nil
}
official, err := isOfficialReviewerTeam(sess, issue, reviewer)
if err != nil {
return nil, fmt.Errorf("isOfficialReviewerTeam(): %v", err)
} else if !official {
if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
return nil, fmt.Errorf("isOfficialReviewer(): %v", err)
}
}
if _, err = createReview(sess, CreateReviewOptions{
Type: ReviewTypeRequest,
Issue: issue,
ReviewerTeam: reviewer,
Official: official,
Stale: false,
}); err != nil {
return nil, err
}
if official {
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_team_id=?", false, issue.ID, reviewer.ID); err != nil {
return nil, err
}
}
comment, err := createComment(sess, &CreateCommentOptions{
Type: CommentTypeReviewRequest,
Doer: doer,
Repo: issue.Repo,
Issue: issue,
RemovedAssignee: false, // Use RemovedAssignee as !isRequest
AssigneeTeamID: reviewer.ID, // Use AssigneeTeamID as reviewer team ID
})
if err != nil {
return nil, fmt.Errorf("createComment(): %v", err)
}
return comment, sess.Commit()
}
//RemoveTeamReviewRequest remove a review request from one team
func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, error) {
sess := x.NewSession()
defer sess.Close()
if err := sess.Begin(); err != nil {
return nil, err
}
review, err := getTeamReviewerByIssueIDAndTeamID(sess, issue.ID, reviewer.ID)
if err != nil && !IsErrReviewNotExist(err) {
return nil, err
}
if review == nil {
return nil, nil
}
if _, err = sess.Delete(review); err != nil {
return nil, err
}
official, err := isOfficialReviewerTeam(sess, issue, reviewer)
if err != nil {
return nil, fmt.Errorf("isOfficialReviewerTeam(): %v", err)
}
if official {
// recalculate which is the latest official review from that team
review, err := getReviewByIssueIDAndUserID(sess, issue.ID, -reviewer.ID)
if err != nil && !IsErrReviewNotExist(err) {
return nil, err
}
if review != nil {
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE id=?", true, review.ID); err != nil {
return nil, err
}
}
}
if doer == nil {
return nil, sess.Commit()
}
comment, err := createComment(sess, &CreateCommentOptions{
Type: CommentTypeReviewRequest,
Doer: doer,
Repo: issue.Repo,
Issue: issue,
RemovedAssignee: true, // Use RemovedAssignee as !isRequest
AssigneeTeamID: reviewer.ID, // Use AssigneeTeamID as reviewer team ID
})
if err != nil {
return nil, fmt.Errorf("createComment(): %v", err)
}
return comment, sess.Commit()
}
// MarkConversation Add or remove Conversation mark for a code comment // MarkConversation Add or remove Conversation mark for a code comment
func MarkConversation(comment *Comment, doer *User, isResolve bool) (err error) { func MarkConversation(comment *Comment, doer *User, isResolve bool) (err error) {
if comment.Type != CommentTypeCode { if comment.Type != CommentTypeCode {

View file

@ -130,6 +130,9 @@ func TestGetReviewersByIssueID(t *testing.T) {
}) })
allReviews, err := GetReviewersByIssueID(issue.ID) allReviews, err := GetReviewersByIssueID(issue.ID)
for _, reviewer := range allReviews {
assert.NoError(t, reviewer.LoadReviewer())
}
assert.NoError(t, err) assert.NoError(t, err)
if assert.Len(t, allReviews, 3) { if assert.Len(t, allReviews, 3) {
for i, review := range allReviews { for i, review := range allReviews {

View file

@ -435,14 +435,188 @@ func retrieveProjects(ctx *context.Context, repo *models.Repository) {
} }
} }
// repoReviewerSelection items to bee shown
type repoReviewerSelection struct {
IsTeam bool
Team *models.Team
User *models.User
Review *models.Review
CanChange bool
Checked bool
ItemID int64
}
// RetrieveRepoReviewers find all reviewers of a repository // RetrieveRepoReviewers find all reviewers of a repository
func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issuePosterID int64) { func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue *models.Issue, canChooseReviewer bool) {
var err error ctx.Data["CanChooseReviewer"] = canChooseReviewer
ctx.Data["Reviewers"], err = repo.GetReviewers(ctx.User.ID, issuePosterID)
reviews, err := models.GetReviewersByIssueID(issue.ID)
if err != nil {
ctx.ServerError("GetReviewersByIssueID", err)
return
}
if len(reviews) == 0 && !canChooseReviewer {
return
}
var (
pullReviews []*repoReviewerSelection
reviewersResult []*repoReviewerSelection
teamReviewersResult []*repoReviewerSelection
teamReviewers []*models.Team
reviewers []*models.User
)
if canChooseReviewer {
posterID := issue.PosterID
if issue.OriginalAuthorID > 0 {
posterID = 0
}
reviewers, err = repo.GetReviewers(ctx.User.ID, posterID)
if err != nil { if err != nil {
ctx.ServerError("GetReviewers", err) ctx.ServerError("GetReviewers", err)
return return
} }
teamReviewers, err = repo.GetReviewerTeams()
if err != nil {
ctx.ServerError("GetReviewerTeams", err)
return
}
if len(reviewers) > 0 {
reviewersResult = make([]*repoReviewerSelection, 0, len(reviewers))
}
if len(teamReviewers) > 0 {
teamReviewersResult = make([]*repoReviewerSelection, 0, len(teamReviewers))
}
}
pullReviews = make([]*repoReviewerSelection, 0, len(reviews))
for _, review := range reviews {
tmp := &repoReviewerSelection{
Checked: review.Type == models.ReviewTypeRequest,
Review: review,
ItemID: review.ReviewerID,
}
if review.ReviewerTeamID > 0 {
tmp.IsTeam = true
tmp.ItemID = -review.ReviewerTeamID
}
if ctx.Repo.IsAdmin() {
// Admin can dismiss or re-request any review requests
tmp.CanChange = true
} else if ctx.User != nil && ctx.User.ID == review.ReviewerID && review.Type == models.ReviewTypeRequest {
// A user can refuse review requests
tmp.CanChange = true
} else if (canChooseReviewer || (ctx.User != nil && ctx.User.ID == issue.PosterID)) && review.Type != models.ReviewTypeRequest &&
ctx.User.ID != review.ReviewerID {
// The poster of the PR, a manager, or official reviewers can re-request review from other reviewers
tmp.CanChange = true
}
pullReviews = append(pullReviews, tmp)
if canChooseReviewer {
if tmp.IsTeam {
teamReviewersResult = append(teamReviewersResult, tmp)
} else {
reviewersResult = append(reviewersResult, tmp)
}
}
}
if len(pullReviews) > 0 {
// Drop all non-existing users and teams from the reviews
currentPullReviewers := make([]*repoReviewerSelection, 0, len(pullReviews))
for _, item := range pullReviews {
if item.Review.ReviewerID > 0 {
if err = item.Review.LoadReviewer(); err != nil {
if models.IsErrUserNotExist(err) {
continue
}
ctx.ServerError("LoadReviewer", err)
return
}
item.User = item.Review.Reviewer
} else if item.Review.ReviewerTeamID > 0 {
if err = item.Review.LoadReviewerTeam(); err != nil {
if models.IsErrTeamNotExist(err) {
continue
}
ctx.ServerError("LoadReviewerTeam", err)
return
}
item.Team = item.Review.ReviewerTeam
} else {
continue
}
currentPullReviewers = append(currentPullReviewers, item)
}
ctx.Data["PullReviewers"] = currentPullReviewers
}
if canChooseReviewer && reviewersResult != nil {
preadded := len(reviewersResult)
for _, reviewer := range reviewers {
found := false
reviewAddLoop:
for _, tmp := range reviewersResult[:preadded] {
if tmp.ItemID == reviewer.ID {
tmp.User = reviewer
found = true
break reviewAddLoop
}
}
if found {
continue
}
reviewersResult = append(reviewersResult, &repoReviewerSelection{
IsTeam: false,
CanChange: true,
User: reviewer,
ItemID: reviewer.ID,
})
}
ctx.Data["Reviewers"] = reviewersResult
}
if canChooseReviewer && teamReviewersResult != nil {
preadded := len(teamReviewersResult)
for _, team := range teamReviewers {
found := false
teamReviewAddLoop:
for _, tmp := range teamReviewersResult[:preadded] {
if tmp.ItemID == -team.ID {
tmp.Team = team
found = true
break teamReviewAddLoop
}
}
if found {
continue
}
teamReviewersResult = append(teamReviewersResult, &repoReviewerSelection{
IsTeam: true,
CanChange: true,
Team: team,
ItemID: -team.ID,
})
}
ctx.Data["TeamReviewers"] = teamReviewersResult
}
} }
// RetrieveRepoMetas find all the meta information of a repository // RetrieveRepoMetas find all the meta information of a repository
@ -981,13 +1155,7 @@ func ViewIssue(ctx *context.Context) {
} }
} }
if canChooseReviewer { RetrieveRepoReviewers(ctx, repo, issue, canChooseReviewer)
RetrieveRepoReviewers(ctx, repo, issue.PosterID)
ctx.Data["CanChooseReviewer"] = true
} else {
ctx.Data["CanChooseReviewer"] = false
}
if ctx.Written() { if ctx.Written() {
return return
} }
@ -1131,8 +1299,8 @@ func ViewIssue(ctx *context.Context) {
} }
} else if comment.Type == models.CommentTypeAssignees || comment.Type == models.CommentTypeReviewRequest { } else if comment.Type == models.CommentTypeAssignees || comment.Type == models.CommentTypeReviewRequest {
if err = comment.LoadAssigneeUser(); err != nil { if err = comment.LoadAssigneeUserAndTeam(); err != nil {
ctx.ServerError("LoadAssigneeUser", err) ctx.ServerError("LoadAssigneeUserAndTeam", err)
return return
} }
} else if comment.Type == models.CommentTypeRemoveDependency || comment.Type == models.CommentTypeAddDependency { } else if comment.Type == models.CommentTypeRemoveDependency || comment.Type == models.CommentTypeAddDependency {
@ -1279,12 +1447,6 @@ func ViewIssue(ctx *context.Context) {
pull.HeadRepo != nil && pull.HeadRepo != nil &&
git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) &&
(!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"]) (!pull.HasMerged || ctx.Data["HeadBranchCommitID"] == ctx.Data["PullHeadCommitID"])
ctx.Data["PullReviewers"], err = models.GetReviewersByIssueID(issue.ID)
if err != nil {
ctx.ServerError("GetReviewersByIssueID", err)
return
}
} }
// Get Dependencies // Get Dependencies
@ -1526,12 +1688,20 @@ func UpdateIssueAssignee(ctx *context.Context) {
}) })
} }
func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models.Issue) error { func isValidReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models.Issue) error {
if reviewer.IsOrganization() { if reviewer.IsOrganization() {
return fmt.Errorf("Organization can't be added as reviewer [user_id: %d, repo_id: %d]", reviewer.ID, issue.PullRequest.BaseRepo.ID) return models.ErrNotValidReviewRequest{
Reason: "Organization can't be added as reviewer",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
} }
if doer.IsOrganization() { if doer.IsOrganization() {
return fmt.Errorf("Organization can't be doer to add reviewer [user_id: %d, repo_id: %d]", doer.ID, issue.PullRequest.BaseRepo.ID) return models.ErrNotValidReviewRequest{
Reason: "Organization can't be doer to add reviewer",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
} }
permReviewer, err := models.GetUserRepoPermission(issue.Repo, reviewer) permReviewer, err := models.GetUserRepoPermission(issue.Repo, reviewer)
@ -1544,8 +1714,8 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
return err return err
} }
lastreview, err := models.GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID) lastreview, err := models.GetReviewByIssueIDAndUserID(issue.ID, reviewer.ID)
if err != nil { if err != nil && !models.IsErrReviewNotExist(err) {
return err return err
} }
@ -1553,10 +1723,14 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
if isAdd { if isAdd {
pemResult = permReviewer.CanAccessAny(models.AccessModeRead, models.UnitTypePullRequests) pemResult = permReviewer.CanAccessAny(models.AccessModeRead, models.UnitTypePullRequests)
if !pemResult { if !pemResult {
return fmt.Errorf("Reviewer can't read [user_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name) return models.ErrNotValidReviewRequest{
Reason: "Reviewer can't read",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
} }
if doer.ID == issue.PosterID && lastreview != nil && lastreview.Type != models.ReviewTypeRequest { if doer.ID == issue.PosterID && issue.OriginalAuthorID == 0 && lastreview != nil && lastreview.Type != models.ReviewTypeRequest {
return nil return nil
} }
@ -1567,33 +1741,103 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models
return err return err
} }
if !pemResult { if !pemResult {
return fmt.Errorf("Doer can't choose reviewer [user_id: %d, repo_name: %s, issue_id: %d]", doer.ID, issue.Repo.Name, issue.ID) return models.ErrNotValidReviewRequest{
Reason: "Doer can't choose reviewer",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
} }
} }
if doer.ID == reviewer.ID { if doer.ID == reviewer.ID {
return fmt.Errorf("doer can't be reviewer [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name) return models.ErrNotValidReviewRequest{
Reason: "doer can't be reviewer",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
} }
if reviewer.ID == issue.PosterID { if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 {
return fmt.Errorf("poster of pr can't be reviewer [user_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name) return models.ErrNotValidReviewRequest{
Reason: "poster of pr can't be reviewer",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
} }
} else { } else {
if lastreview.Type == models.ReviewTypeRequest && lastreview.ReviewerID == doer.ID { if lastreview != nil && lastreview.Type == models.ReviewTypeRequest && lastreview.ReviewerID == doer.ID {
return nil return nil
} }
pemResult = permDoer.IsAdmin() pemResult = permDoer.IsAdmin()
if !pemResult { if !pemResult {
return fmt.Errorf("Doer is not admin [user_id: %d, repo_name: %s]", doer.ID, issue.Repo.Name) return models.ErrNotValidReviewRequest{
Reason: "Doer is not admin",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
} }
} }
return nil return nil
} }
// updatePullReviewRequest change pull's request reviewers func isValidTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bool, issue *models.Issue) error {
func updatePullReviewRequest(ctx *context.Context) { if doer.IsOrganization() {
return models.ErrNotValidReviewRequest{
Reason: "Organization can't be doer to add reviewer",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
}
permission, err := models.GetUserRepoPermission(issue.Repo, doer)
if err != nil {
log.Error("Unable to GetUserRepoPermission for %-v in %-v#%d", doer, issue.Repo, issue.Index)
return err
}
if isAdd {
if issue.Repo.IsPrivate {
hasTeam := models.HasTeamRepo(reviewer.OrgID, reviewer.ID, issue.RepoID)
if !hasTeam {
return models.ErrNotValidReviewRequest{
Reason: "Reviewing team can't read repo",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
}
}
doerCanWrite := permission.CanAccessAny(models.AccessModeWrite, models.UnitTypePullRequests)
if !doerCanWrite {
official, err := models.IsOfficialReviewer(issue, doer)
if err != nil {
log.Error("Unable to Check if IsOfficialReviewer for %-v in %-v#%d", doer, issue.Repo, issue.Index)
return err
}
if !official {
return models.ErrNotValidReviewRequest{
Reason: "Doer can't choose reviewer",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
}
}
} else if !permission.IsAdmin() {
return models.ErrNotValidReviewRequest{
Reason: "Only admin users can remove team requests. Doer is not admin",
UserID: doer.ID,
RepoID: issue.Repo.ID,
}
}
return nil
}
// UpdatePullReviewRequest add or remove review request
func UpdatePullReviewRequest(ctx *context.Context) {
issues := getActionIssues(ctx) issues := getActionIssues(ctx)
if ctx.Written() { if ctx.Written() {
return return
@ -1609,17 +1853,99 @@ func updatePullReviewRequest(ctx *context.Context) {
} }
for _, issue := range issues { for _, issue := range issues {
if issue.IsPull { if err := issue.LoadRepo(); err != nil {
ctx.ServerError("issue.LoadRepo", err)
return
}
if !issue.IsPull {
log.Warn(
"UpdatePullReviewRequest: refusing to add review request for non-PR issue %-v#%d",
issue.Repo, issue.Index,
)
ctx.Status(403)
return
}
if reviewID < 0 {
// negative reviewIDs represent team requests
if err := issue.Repo.GetOwner(); err != nil {
ctx.ServerError("issue.Repo.GetOwner", err)
return
}
if !issue.Repo.Owner.IsOrganization() {
log.Warn(
"UpdatePullReviewRequest: refusing to add team review request for %s#%d owned by non organization UID[%d]",
issue.Repo.FullName(), issue.Index, issue.Repo.ID,
)
ctx.Status(403)
return
}
team, err := models.GetTeamByID(-reviewID)
if err != nil {
ctx.ServerError("models.GetTeamByID", err)
return
}
if team.OrgID != issue.Repo.OwnerID {
log.Warn(
"UpdatePullReviewRequest: refusing to add team review request for UID[%d] team %s to %s#%d owned by UID[%d]",
team.OrgID, team.Name, issue.Repo.FullName(), issue.Index, issue.Repo.ID)
ctx.Status(403)
return
}
err = isValidTeamReviewRequest(team, ctx.User, action == "attach", issue)
if err != nil {
if models.IsErrNotValidReviewRequest(err) {
log.Warn(
"UpdatePullReviewRequest: refusing to add invalid team review request for UID[%d] team %s to %s#%d owned by UID[%d]: Error: %v",
team.OrgID, team.Name, issue.Repo.FullName(), issue.Index, issue.Repo.ID,
err,
)
ctx.Status(403)
return
}
ctx.ServerError("isValidTeamReviewRequest", err)
return
}
err = issue_service.TeamReviewRequest(issue, ctx.User, team, action == "attach")
if err != nil {
ctx.ServerError("TeamReviewRequest", err)
return
}
continue
}
reviewer, err := models.GetUserByID(reviewID) reviewer, err := models.GetUserByID(reviewID)
if err != nil { if err != nil {
if models.IsErrUserNotExist(err) {
log.Warn(
"UpdatePullReviewRequest: requested reviewer [%d] for %-v to %-v#%d is not exist: Error: %v",
reviewID, issue.Repo, issue.Index,
err,
)
ctx.Status(403)
return
}
ctx.ServerError("GetUserByID", err) ctx.ServerError("GetUserByID", err)
return return
} }
err = isLegalReviewRequest(reviewer, ctx.User, action == "attach", issue) err = isValidReviewRequest(reviewer, ctx.User, action == "attach", issue)
if err != nil { if err != nil {
ctx.ServerError("isLegalRequestReview", err) if models.IsErrNotValidReviewRequest(err) {
log.Warn(
"UpdatePullReviewRequest: refusing to add invalid review request for %-v to %-v#%d: Error: %v",
reviewer, issue.Repo, issue.Index,
err,
)
ctx.Status(403)
return
}
ctx.ServerError("isValidReviewRequest", err)
return return
} }
@ -1628,10 +1954,6 @@ func updatePullReviewRequest(ctx *context.Context) {
ctx.ServerError("ReviewRequest", err) ctx.ServerError("ReviewRequest", err)
return return
} }
} else {
ctx.Status(403)
return
}
} }
ctx.JSON(200, map[string]interface{}{ ctx.JSON(200, map[string]interface{}{
@ -1639,11 +1961,6 @@ func updatePullReviewRequest(ctx *context.Context) {
}) })
} }
// UpdatePullReviewRequest add or remove review request
func UpdatePullReviewRequest(ctx *context.Context) {
updatePullReviewRequest(ctx)
}
// UpdateIssueStatus change issue's status // UpdateIssueStatus change issue's status
func UpdateIssueStatus(ctx *context.Context) { func UpdateIssueStatus(ctx *context.Context) {
issues := getActionIssues(ctx) issues := getActionIssues(ctx)

View file

@ -52,7 +52,7 @@ func ToggleAssignee(issue *models.Issue, doer *models.User, assigneeID int64) (r
return return
} }
// ReviewRequest add or remove a review for this PR, and make comment for it. // ReviewRequest add or remove a review request from a user for this PR, and make comment for it.
func ReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.User, isAdd bool) (err error) { func ReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.User, isAdd bool) (err error) {
var comment *models.Comment var comment *models.Comment
if isAdd { if isAdd {
@ -71,3 +71,40 @@ func ReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.User
return nil return nil
} }
// TeamReviewRequest add or remove a review request from a team for this PR, and make comment for it.
func TeamReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.Team, isAdd bool) (err error) {
var comment *models.Comment
if isAdd {
comment, err = models.AddTeamReviewRequest(issue, reviewer, doer)
} else {
comment, err = models.RemoveTeamReviewRequest(issue, reviewer, doer)
}
if err != nil {
return
}
if comment == nil || !isAdd {
return
}
// notify all user in this team
if err = comment.LoadIssue(); err != nil {
return
}
if err = reviewer.GetMembers(&models.SearchMembersOptions{}); err != nil {
return
}
for _, member := range reviewer.Members {
if member.ID == comment.Issue.PosterID {
continue
}
comment.AssigneeID = member.ID
notification.NotifyPullReviewRequest(doer, issue, member, isAdd, comment)
}
return nil
}

View file

@ -68,14 +68,13 @@ func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models
return nil, err return nil, err
} }
review, err = models.CreateReview(models.CreateReviewOptions{ if review, err = models.CreateReview(models.CreateReviewOptions{
Type: models.ReviewTypePending, Type: models.ReviewTypePending,
Reviewer: doer, Reviewer: doer,
Issue: issue, Issue: issue,
Official: false, Official: false,
CommitID: latestCommitID, CommitID: latestCommitID,
}) }); err != nil {
if err != nil {
return nil, err return nil, err
} }
} }

View file

@ -595,6 +595,7 @@
</a> </a>
<span class="text grey"> <span class="text grey">
<a class="author" href="{{.Poster.HomeLink}}">{{.Poster.GetDisplayName}}</a> <a class="author" href="{{.Poster.HomeLink}}">{{.Poster.GetDisplayName}}</a>
{{if (gt .AssigneeID 0)}}
{{if .RemovedAssignee}} {{if .RemovedAssignee}}
{{if eq .PosterID .AssigneeID}} {{if eq .PosterID .AssigneeID}}
{{$.i18n.Tr "repo.issues.review.remove_review_request_self" $createdStr | Safe}} {{$.i18n.Tr "repo.issues.review.remove_review_request_self" $createdStr | Safe}}
@ -604,6 +605,13 @@
{{else}} {{else}}
{{$.i18n.Tr "repo.issues.review.add_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}} {{$.i18n.Tr "repo.issues.review.add_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}}
{{end}} {{end}}
{{else}}
{{if .RemovedAssignee}}
{{$.i18n.Tr "repo.issues.review.remove_review_request" (.AssigneeTeam.Name|Escape) $createdStr | Safe}}
{{else}}
{{$.i18n.Tr "repo.issues.review.add_review_request" (.AssigneeTeam.Name|Escape) $createdStr | Safe}}
{{end}}
{{end}}
</span> </span>
</div> </div>
{{else if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}} {{else if and (eq .Type 29) (or (gt .CommitsNum 0) .IsForcePush)}}

View file

@ -1,24 +1,31 @@
{{if gt (len .PullReviewers) 0}} {{if .PullReviewers }}
<div class="comment box"> <div class="comment box">
<div class="content"> <div class="content">
<div class="ui segment"> <div class="ui segment">
<h4>{{$.i18n.Tr "repo.issues.review.reviewers"}}</h4> <h4>{{$.i18n.Tr "repo.issues.review.reviewers"}}</h4>
{{range .PullReviewers}} {{range .PullReviewers}}
{{ $createdStr:= TimeSinceUnix .UpdatedUnix $.Lang }} {{ $createdStr:= TimeSinceUnix .Review.UpdatedUnix $.Lang }}
<div class="ui divider"></div> <div class="ui divider"></div>
<div class="review-item"> <div class="review-item">
<div class="review-item-left"> <div class="review-item-left">
<a class="ui avatar image" href="{{.Reviewer.HomeLink}}"> {{if .User}}
<img src="{{.Reviewer.RelAvatarLink}}"> <a class="ui avatar image" href="{{.User.HomeLink}}">
<img src="{{.User.RelAvatarLink}}">
</a> </a>
<span class="text grey"><a href="{{.Reviewer.HomeLink}}">{{.Reviewer.Name}}</a> {{end}}
{{if eq .Type 1}} <span class="text grey">
{{if .User}}
<a href="{{.User.HomeLink}}">{{.User.Name}}</a>
{{else if .Team}}
<span class="ui text">{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}</span>
{{end}}
{{if eq .Review.Type 1}}
{{$.i18n.Tr "repo.issues.review.approve" $createdStr | Safe}} {{$.i18n.Tr "repo.issues.review.approve" $createdStr | Safe}}
{{else if eq .Type 2}} {{else if eq .Review.Type 2}}
{{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}}
{{else if eq .Type 3}} {{else if eq .Review.Type 3}}
{{$.i18n.Tr "repo.issues.review.reject" $createdStr | Safe}} {{$.i18n.Tr "repo.issues.review.reject" $createdStr | Safe}}
{{else if eq .Type 4}} {{else if eq .Review.Type 4}}
{{$.i18n.Tr "repo.issues.review.wait" $createdStr | Safe}} {{$.i18n.Tr "repo.issues.review.wait" $createdStr | Safe}}
{{else}} {{else}}
{{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}}
@ -26,34 +33,23 @@
</span> </span>
</div> </div>
<div class="review-item-right"> <div class="review-item-right">
{{if .Stale}} {{if .Review.Stale}}
<span class="ui poping up type-icon text grey" data-content="{{$.i18n.Tr "repo.issues.is_stale"}}"> <span class="ui poping up type-icon text grey" data-content="{{$.i18n.Tr "repo.issues.is_stale"}}">
<i class="octicon icon fa-hourglass-end"></i> <i class="octicon icon fa-hourglass-end"></i>
</span> </span>
{{end}} {{end}}
<span class="type-icon text {{if eq .Type 1}}green <span class="type-icon text {{if eq .Review.Type 1}}green
{{- else if eq .Type 2}}grey {{- else if eq .Review.Type 2}}grey
{{- else if eq .Type 3}}red {{- else if eq .Review.Type 3}}red
{{- else if eq .Type 4}}yellow {{- else if eq .Review.Type 4}}yellow
{{else}}grey{{end}}"> {{else}}grey{{end}}">
{{$canChoose := false}} {{if .CanChange }}
{{if eq .Type 4}} <a href="#" class="ui poping up icon re-request-review {{if .Checked}}checked{{end}}" data-issue-id="{{$.Issue.ID}}" data-content="{{if .Checked}} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}" data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
{{if or (eq .ReviewerID $.SignedUserID) $.Permission.IsAdmin}}
{{$canChoose = true}}
{{end}}
{{else}}
{{if and (or $.IsIssuePoster $.CanChooseReviewer) (not (eq $.SignedUserID .ReviewerID))}}
{{$canChoose = true}}
{{end}}
{{end}}
{{if $canChoose }}
<a href="#" class="ui poping up icon re-request-review" data-is-checked="{{if eq .Type 4}}true{{else}}false{{end}}" data-issue-id="{{$.Issue.ID}}" data-content="{{ if eq .Type 4 }} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}" data-id="{{.ReviewerID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
{{svg "octicon-sync"}} {{svg "octicon-sync"}}
</a> </a>
{{end}} {{end}}
{{svg (printf "octicon-%s" .Type.Icon)}} {{svg (printf "octicon-%s" .Review.Type.Icon)}}
</span> </span>
</div> </div>
</div> </div>

View file

@ -5,7 +5,7 @@
{{if .Issue.IsPull }} {{if .Issue.IsPull }}
<input id="reviewer_id" name="reviewer_id" type="hidden" value="{{.reviewer_id}}"> <input id="reviewer_id" name="reviewer_id" type="hidden" value="{{.reviewer_id}}">
<div class="ui {{if or (not .CanChooseReviewer) .Repository.IsArchived}}disabled{{end}} floating jump select-reviewers-modify dropdown"> <div class="ui {{if or (not .Reviewers) (not .CanChooseReviewer) .Repository.IsArchived}}disabled{{end}} floating jump select-reviewers-modify dropdown">
<span class="text"> <span class="text">
<strong>{{.i18n.Tr "repo.issues.review.reviewers"}}</strong> <strong>{{.i18n.Tr "repo.issues.review.reviewers"}}</strong>
{{if and .CanChooseReviewer (not .Repository.IsArchived)}} {{if and .CanChooseReviewer (not .Repository.IsArchived)}}
@ -20,37 +20,31 @@
<input type="text" placeholder="{{.i18n.Tr "repo.issues.filter_reviewers"}}"> <input type="text" placeholder="{{.i18n.Tr "repo.issues.filter_reviewers"}}">
</div> </div>
{{end}} {{end}}
{{if .Reviewers}}
{{range .Reviewers}} {{range .Reviewers}}
{{$ReviewerID := .ID}} {{if .User}}
{{$checked := false}} <a class="{{if not .CanChange}}ui poping up{{end}} item {{if .Checked}} checked {{end}} {{if not .CanChange}}ban-change{{end}}" href="#" data-id="{{.ItemID}}" data-id-selector="#review_request_{{.ItemID}}" {{if not .CanChange}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
{{$canChoose := false}} <span class="octicon-check {{if not .Checked}}invisible{{end}}">{{svg "octicon-check"}}</span>
{{$notReviewed := true}}
{{range $.PullReviewers}}
{{if eq .ReviewerID $ReviewerID }}
{{$notReviewed = false }}
{{if eq .Type 4 }}
{{$checked = true}}
{{if or (eq $ReviewerID $.SignedUserID) $.Permission.IsAdmin}}
{{$canChoose = true}}
{{end}}
{{else}}
{{$canChoose = true}}
{{end}}
{{end}}
{{end}}
{{ if $notReviewed}}
{{$canChoose = true}}
{{end}}
<a class="{{if not $canChoose}}ui poping up{{end}} item {{if $checked}} checked {{end}} {{if not $canChoose}}ban-change{{end}}" href="#" data-id="{{.ID}}" data-id-selector="#review_request_{{.ID}}" {{if not $canChoose}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
<span class="octicon-check {{if not $checked}}invisible{{end}}">{{svg "octicon-check"}}</span>
<span class="text"> <span class="text">
<img class="ui avatar image" src="{{.RelAvatarLink}}"> {{.GetDisplayName}} <img class="ui avatar image" src="{{.User.RelAvatarLink}}"> {{.User.GetDisplayName}}
</span> </span>
</a> </a>
{{end}} {{end}}
{{end}}
{{end}}
{{if .TeamReviewers}}
<div class="ui divider"></div>
{{range .TeamReviewers}}
{{if .Team}}
<a class="{{if not .CanChange}}ui poping up{{end}} item {{if .Checked}} checked {{end}} {{if not .CanChange}}ban-change{{end}}" href="#" data-id="{{.ItemID}}" data-id-selector="#review_request_team_{{.Team.ID}}" {{if not .CanChange}} data-content="{{$.i18n.Tr "repo.issues.remove_request_review_block"}}"{{end}}>
<span class="octicon-check {{if not .Checked}}invisible{{end}}">{{svg "octicon-check" 16}}</span>
<span class="text">
{{svg "octicon-people" 16 "ml-4 mr-2"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}
</span>
</a>
{{end}}
{{end}}
{{end}}
</div> </div>
</div> </div>
@ -59,30 +53,23 @@
<div class="selected"> <div class="selected">
{{range .PullReviewers}} {{range .PullReviewers}}
<div class="item" style="margin-bottom: 10px;"> <div class="item" style="margin-bottom: 10px;">
<a href="{{.Reviewer.HomeLink}}"><img class="ui avatar image" src="{{.Reviewer.RelAvatarLink}}">&nbsp;{{.Reviewer.GetDisplayName}}</a> {{if .User}}
<span class="ui right type-icon text {{if eq .Type 1}}green <a href="{{.User.HomeLink}}"><img class="ui avatar image" src="{{.User.RelAvatarLink}}">&nbsp;{{.User.GetDisplayName}}</a>
{{- else if eq .Type 2}}grey {{else if .Team}}
{{- else if eq .Type 3}}red <span class="text">{{svg "octicon-people" 16 "teamavatar"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}</span>
{{- else if eq .Type 4}}yellow {{end}}
<span class="ui right type-icon text {{if eq .Review.Type 1}}green
{{- else if eq .Review.Type 2}}grey
{{- else if eq .Review.Type 3}}red
{{- else if eq .Review.Type 4}}yellow
{{- else}}grey{{end}} right "> {{- else}}grey{{end}} right ">
{{$canChoose := false}} {{if .CanChange}}
{{if eq .Type 4}} <a href="#" class="ui poping up icon re-request-review {{if .Checked}}checked{{end}}" data-content="{{if .Checked}} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}" data-issue-id="{{$.Issue.ID}}" data-id="{{.ItemID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
{{if or (eq .ReviewerID $.SignedUserID) $.Permission.IsAdmin}}
{{$canChoose = true}}
{{end}}
{{else}}
{{if and (or $.IsIssuePoster $.CanChooseReviewer) (not (eq $.SignedUserID .ReviewerID))}}
{{$canChoose = true}}
{{end}}
{{end}}
{{if $canChoose}}
<a href="#" class="ui poping up icon re-request-review" data-is-checked="{{if eq .Type 4}}true{{else}}false{{end}}" data-content="{{ if eq .Type 4 }} {{$.i18n.Tr "repo.issues.remove_request_review"}} {{else}} {{$.i18n.Tr "repo.issues.re_request_review"}} {{end}}" data-issue-id="{{$.Issue.ID}}" data-id="{{.ReviewerID}}" data-update-url="{{$.RepoLink}}/issues/request_review">
{{svg "octicon-sync"}} {{svg "octicon-sync"}}
</a> </a>
{{end}} {{end}}
{{svg (printf "octicon-%s" .Type.Icon)}} {{svg (printf "octicon-%s" .Review.Type.Icon)}}
</span> </span>
</div> </div>
{{end}} {{end}}

View file

@ -663,15 +663,16 @@ function initIssueComments() {
const url = $(this).data('update-url'); const url = $(this).data('update-url');
const issueId = $(this).data('issue-id'); const issueId = $(this).data('issue-id');
const id = $(this).data('id'); const id = $(this).data('id');
const isChecked = $(this).data('is-checked'); const isChecked = $(this).hasClass('checked');
event.preventDefault(); event.preventDefault();
updateIssuesMeta( updateIssuesMeta(
url, url,
isChecked === 'true' ? 'attach' : 'detach', isChecked ? 'detach' : 'attach',
issueId, issueId,
id, id,
).then(reload); ).then(reload);
return false;
}); });
$(document).on('click', (event) => { $(document).on('click', (event) => {

View file

@ -101,6 +101,12 @@
line-height: 2em; line-height: 2em;
} }
&.assignees .teamavatar {
margin-top: .125rem;
margin-left: 6.75px;
margin-right: 8.75px;
}
.hide { .hide {
display: none !important; display: none !important;
} }