From 8be3e439c2b3a90fcb639b732008486b85314b8d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E8=B5=B5=E6=99=BA=E8=B6=85?= <1012112796@qq.com> Date: Tue, 13 Oct 2020 03:55:13 +0800 Subject: [PATCH] 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 Co-authored-by: Lauris BH Co-authored-by: Andrew Thornton --- models/error.go | 20 + models/fixtures/review.yml | 4 + models/issue_comment.go | 29 +- models/migrations/migrations.go | 2 + models/migrations/v153.go | 25 ++ models/repo.go | 80 ++-- models/repo_test.go | 31 ++ models/review.go | 392 ++++++++++++----- models/review_test.go | 3 + routers/repo/issue.go | 413 ++++++++++++++++-- services/issue/assignee.go | 39 +- services/pull/review.go | 5 +- .../repo/issue/view_content/comments.tmpl | 18 +- templates/repo/issue/view_content/pull.tmpl | 54 ++- .../repo/issue/view_content/sidebar.tmpl | 81 ++-- web_src/js/index.js | 5 +- web_src/less/_repository.less | 6 + 17 files changed, 935 insertions(+), 272 deletions(-) create mode 100644 models/migrations/v153.go diff --git a/models/error.go b/models/error.go index 364924c99..be94d7889 100644 --- a/models/error.go +++ b/models/error.go @@ -1994,6 +1994,26 @@ func (err ErrReviewNotExist) Error() string { 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) +} + // ________ _____ __ .__ // \_____ \ / _ \ __ ___/ |_| |__ // / | \ / /_\ \| | \ __\ | \ diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index 35d3dee2e..3db0b4735 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -44,6 +44,7 @@ reviewer_id: 2 issue_id: 3 content: "New review 3" + original_author_id: 0 updated_unix: 946684811 created_unix: 946684811 - @@ -52,6 +53,7 @@ reviewer_id: 3 issue_id: 3 content: "New review 4" + original_author_id: 0 updated_unix: 946684812 created_unix: 946684812 - @@ -59,6 +61,7 @@ type: 1 reviewer_id: 4 issue_id: 3 + original_author_id: 0 content: "New review 5" commit_id: 8091a55037cd59e47293aca02981b5a67076b364 stale: true @@ -72,6 +75,7 @@ content: "New review 3 rejected" updated_unix: 946684814 created_unix: 946684814 + original_author_id: 0 - id: 10 diff --git a/models/issue_comment.go b/models/issue_comment.go index 726ed7472..270a10e24 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -137,6 +137,8 @@ type Comment struct { AssigneeID int64 RemovedAssignee bool Assignee *User `xorm:"-"` + AssigneeTeamID int64 `xorm:"NOT NULL DEFAULT 0"` + AssigneeTeam *Team `xorm:"-"` ResolveDoerID int64 ResolveDoer *User `xorm:"-"` OldTitle string @@ -487,11 +489,11 @@ func (c *Comment) UpdateAttachments(uuids []string) error { return sess.Commit() } -// LoadAssigneeUser if comment.Type is CommentTypeAssignees, then load assignees -func (c *Comment) LoadAssigneeUser() error { +// LoadAssigneeUserAndTeam if comment.Type is CommentTypeAssignees, then load assignees +func (c *Comment) LoadAssigneeUserAndTeam() error { var err error - if c.AssigneeID > 0 { + if c.AssigneeID > 0 && c.Assignee == nil { c.Assignee, err = getUserByID(x, c.AssigneeID) if err != nil { if !IsErrUserNotExist(err) { @@ -499,6 +501,25 @@ func (c *Comment) LoadAssigneeUser() error { } 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 } @@ -685,6 +706,7 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err ProjectID: opts.ProjectID, RemovedAssignee: opts.RemovedAssignee, AssigneeID: opts.AssigneeID, + AssigneeTeamID: opts.AssigneeTeamID, CommitID: opts.CommitID, CommitSHA: opts.CommitSHA, Line: opts.LineNum, @@ -849,6 +871,7 @@ type CreateCommentOptions struct { OldProjectID int64 ProjectID int64 AssigneeID int64 + AssigneeTeamID int64 RemovedAssignee bool OldTitle string NewTitle string diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index dda8c0094..c63e02f31 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -240,6 +240,8 @@ var migrations = []Migration{ NewMigration("set default password algorithm to Argon2", setDefaultPasswordToArgon2), // v152 -> v153 NewMigration("add TrustModel field to Repository", addTrustModelToRepository), + // v153 > v154 + NewMigration("add Team review request support", addTeamReviewRequestSupport), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v153.go b/models/migrations/v153.go new file mode 100644 index 000000000..1e5ae9f7d --- /dev/null +++ b/models/migrations/v153.go @@ -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)) +} diff --git a/models/repo.go b/models/repo.go index a743f6573..f505412e0 100644 --- a/models/repo.go +++ b/models/repo.go @@ -694,32 +694,37 @@ func (repo *Repository) GetAssignees() (_ []*User, err error) { return repo.getAssignees(x) } -func (repo *Repository) getReviewersPrivate(e Engine, doerID, posterID int64) (users []*User, err error) { - users = make([]*User, 0, 20) - - 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", - repo.ID, AccessModeRead, - doerID, posterID). - Find(&users); err != nil { +func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) ([]*User, error) { + // 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 } - return users, nil -} - -func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_ []*User, err error) { + var users []*User - users := make([]*User, 0) + 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", + repo.ID, AccessModeRead, + doerID, posterID). + Find(&users); err != nil { + return nil, err + } - const SQLCmd = "SELECT * FROM `user` WHERE id IN ( " + - "SELECT user_id FROM `access` WHERE repo_id = ? AND mode >= ? AND user_id NOT IN ( ?, ?) " + - "UNION " + - "SELECT user_id FROM `watch` WHERE repo_id = ? AND user_id NOT IN ( ?, ?) AND mode IN (?, ?) " + - ") ORDER BY name" + return users, nil + } - if err = e. - SQL(SQLCmd, + // This is a "public" repository: + // Any user that has write access or who is a watcher can be requested to review + 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 ( ?, ?) "+ + "UNION "+ + "SELECT user_id FROM `watch` WHERE repo_id = ? AND user_id NOT IN ( ?, ?) AND mode IN (?, ?) "+ + ") ORDER BY name", repo.ID, AccessModeRead, doerID, posterID, repo.ID, doerID, posterID, RepoWatchModeNormal, RepoWatchModeAuto). Find(&users); err != nil { @@ -729,27 +734,30 @@ func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_ return users, nil } -func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []*User, err error) { - if err = repo.getOwner(e); err != nil { +// GetReviewers get all users can be requested to review: +// * 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 + } - if repo.IsPrivate || - (repo.Owner.IsOrganization() && repo.Owner.Visibility == api.VisibleTypePrivate) { - users, err = repo.getReviewersPrivate(x, doerID, posterID) - } else { - users, err = repo.getReviewersPublic(x, doerID, posterID) + teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead) + if err != nil { + return nil, err } - 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) + return teams, err } // GetMilestoneByID returns the milestone belongs to repository by given ID. diff --git a/models/repo_test.go b/models/repo_test.go index 045f94670..a366772d5 100644 --- a/models/repo_test.go +++ b/models/repo_test.go @@ -193,3 +193,34 @@ func TestDoctorUserStarNum(t *testing.T) { 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)) +} diff --git a/models/review.go b/models/review.go index 5f27e2b7f..2c38176ef 100644 --- a/models/review.go +++ b/models/review.go @@ -8,6 +8,7 @@ import ( "fmt" "strings" + "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/timeutil" "xorm.io/builder" @@ -54,6 +55,8 @@ type Review struct { Type ReviewType Reviewer *User `xorm:"-"` ReviewerID int64 `xorm:"index"` + ReviewerTeamID int64 `xorm:"NOT NULL DEFAULT 0"` + ReviewerTeam *Team `xorm:"-"` OriginalAuthor string OriginalAuthorID int64 Issue *Issue `xorm:"-"` @@ -98,18 +101,32 @@ func (r *Review) loadIssue(e Engine) (err error) { } func (r *Review) loadReviewer(e Engine) (err error) { - if r.Reviewer != nil || r.ReviewerID == 0 { - return nil + if r.ReviewerID == 0 || r.Reviewer != nil { + return } r.Reviewer, err = getUserByID(e, r.ReviewerID) 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 func (r *Review) LoadReviewer() error { 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) { if err = r.loadIssue(e); err != nil { return @@ -120,6 +137,9 @@ func (r *Review) loadAttributes(e Engine) (err error) { if err = r.loadReviewer(e); err != nil { return } + if err = r.loadReviewerTeam(e); err != nil { + return + } return } @@ -189,21 +209,49 @@ func FindReviews(opts FindReviewOptions) ([]*Review, error) { // CreateReviewOptions represent the options to create a review. Type, Issue and Reviewer are required. type CreateReviewOptions struct { - Content string - Type ReviewType - Issue *Issue - Reviewer *User - Official bool - CommitID string - Stale bool + Content string + Type ReviewType + Issue *Issue + Reviewer *User + ReviewerTeam *Team + Official bool + CommitID string + Stale bool +} + +// IsOfficialReviewer check if at least one of the provided reviewers can make official reviews in issue (counts towards required approvals) +func IsOfficialReviewer(issue *Issue, reviewers ...*User) (bool, error) { + return isOfficialReviewer(x, issue, reviewers...) +} + +func isOfficialReviewer(e Engine, issue *Issue, reviewers ...*User) (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 + } + + for _, reviewer := range reviewers { + official, err := pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer) + if official || err != nil { + return official, err + } + } + + return false, nil } -// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals) -func IsOfficialReviewer(issue *Issue, reviewer *User) (bool, error) { - return isOfficialReviewer(x, issue, reviewer) +// 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 isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) { +func isOfficialReviewerTeam(e Engine, issue *Issue, team *Team) (bool, error) { pr, err := getPullRequestByIssueID(e, issue.ID) if err != nil { return false, err @@ -215,20 +263,32 @@ func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) { return false, nil } - return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer) + 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) { review := &Review{ - Type: opts.Type, - Issue: opts.Issue, - IssueID: opts.Issue.ID, - Reviewer: opts.Reviewer, - ReviewerID: opts.Reviewer.ID, - Content: opts.Content, - Official: opts.Official, - CommitID: opts.CommitID, - Stale: opts.Stale, + Type: opts.Type, + Issue: opts.Issue, + IssueID: opts.Issue.ID, + Reviewer: opts.Reviewer, + ReviewerTeam: opts.ReviewerTeam, + Content: opts.Content, + Official: opts.Official, + CommitID: opts.CommitID, + 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 { 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 { return nil, nil, err } - official, err = isOfficialReviewer(sess, issue, doer) - if err != nil { + if official, err = isOfficialReviewer(sess, issue, doer); err != nil { return nil, nil, err } } // No current review. Create a new one! - review, err = createReview(sess, CreateReviewOptions{ + if review, err = createReview(sess, CreateReviewOptions{ Type: reviewType, Issue: issue, Reviewer: doer, @@ -326,8 +385,7 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm Official: official, CommitID: commitID, Stale: stale, - }) - if err != nil { + }); err != nil { return nil, nil, err } } 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 { return nil, nil, err } - official, err = isOfficialReviewer(sess, issue, doer) - if err != nil { + if official, err = isOfficialReviewer(sess, issue, doer); err != nil { return nil, nil, err } } @@ -373,13 +430,34 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm 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 return review, comm, sess.Commit() } // GetReviewersByIssueID gets the latest review of each reviewer for a pull request -func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) { - reviewsUnfiltered := []*Review{} +func GetReviewersByIssueID(issueID int64) ([]*Review, error) { + reviews := make([]*Review, 0, 10) sess := x.NewSession() 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 - 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). - Find(&reviewsUnfiltered); err != nil { + Find(&reviews); err != nil { return nil, err } - // Load reviewer and skip if user is deleted - for _, review := range reviewsUnfiltered { - if err = review.loadReviewer(sess); err != nil { - if !IsErrUserNotExist(err) { - return nil, err - } - } else { - reviews = append(reviews, review) - } + teamReviewRequests := make([]*Review, 0, 5) + 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", + issueID). + Find(&teamReviewRequests); err != nil { + return nil, err + } + + if len(teamReviewRequests) > 0 { + reviews = append(reviews, teamReviewRequests...) } return reviews, nil } -// GetReviewerByIssueIDAndUserID get the latest review of reviewer for a pull request -func GetReviewerByIssueIDAndUserID(issueID, userID int64) (review *Review, err error) { - return getReviewerByIssueIDAndUserID(x, issueID, userID) +// GetReviewByIssueIDAndUserID get the latest review of reviewer for a pull request +func GetReviewByIssueIDAndUserID(issueID, userID int64) (*Review, error) { + return getReviewByIssueIDAndUserID(x, issueID, userID) } -func getReviewerByIssueIDAndUserID(e Engine, issueID, userID int64) (review *Review, err error) { - review = new(Review) +func getReviewByIssueIDAndUserID(e Engine, issueID, userID int64) (*Review, error) { + 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, 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) + + has := false + 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 { return nil, err } + if !has { + return nil, ErrReviewNotExist{0} + } + return } @@ -482,57 +587,43 @@ func InsertReviews(reviews []*Review) error { } // AddReviewRequest add a review request from one reviewer -func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Comment, err error) { - review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID) - if err != nil { - return - } - - // skip it when reviewer hase been request to review - if review != nil && review.Type == ReviewTypeRequest { - return nil, nil - } - +func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) { sess := x.NewSession() defer sess.Close() if err := sess.Begin(); err != nil { return nil, err } - var official bool - official, err = isOfficialReviewer(sess, issue, reviewer) - - if err != nil { + review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID) + if err != nil && !IsErrReviewNotExist(err) { return nil, err } - if !official { - official, err = isOfficialReviewer(sess, issue, doer) - - if err != nil { - return nil, err - } + // skip it when reviewer hase been request to review + if review != nil && review.Type == ReviewTypeRequest { + return nil, nil } - if official { + official, err := isOfficialReviewer(sess, issue, reviewer, doer) + if err != nil { + return nil, err + } else if official { if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, reviewer.ID); err != nil { return nil, err } } - _, err = createReview(sess, CreateReviewOptions{ + if _, err = createReview(sess, CreateReviewOptions{ Type: ReviewTypeRequest, Issue: issue, Reviewer: reviewer, Official: official, Stale: false, - }) - - if err != nil { - return + }); err != nil { + return nil, err } - comment, err = createComment(sess, &CreateCommentOptions{ + comment, err := createComment(sess, &CreateCommentOptions{ Type: CommentTypeReviewRequest, Doer: doer, Repo: issue.Repo, @@ -540,7 +631,6 @@ func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Commen RemovedAssignee: false, // Use RemovedAssignee as !isRequest AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID }) - if err != nil { return nil, err } @@ -549,39 +639,146 @@ func AddReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Commen } //RemoveReviewRequest remove a review request from one reviewer -func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Comment, err error) { - review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID) - if err != nil { - return +func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) { + sess := x.NewSession() + defer sess.Close() + 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 } - if review.Type != ReviewTypeRequest { + if review == nil || review.Type != ReviewTypeRequest { return nil, nil } + if _, err = sess.Delete(review); err != nil { + return nil, err + } + + official, err := isOfficialReviewer(sess, issue, reviewer) + if err != nil { + 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 _, err := sess.Exec("UPDATE `review` SET official=? WHERE id=?", true, review.ID); err != nil { + return nil, err + } + } + } + + comment, err := createComment(sess, &CreateCommentOptions{ + Type: CommentTypeReviewRequest, + Doer: doer, + Repo: issue.Repo, + Issue: issue, + RemovedAssignee: true, // Use RemovedAssignee as !isRequest + AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID + }) + if err != nil { + return nil, err + } + + 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 } - _, err = sess.Delete(review) - if err != nil { + review, err := getTeamReviewerByIssueIDAndTeamID(sess, issue.ID, reviewer.ID) + if err != nil && !IsErrReviewNotExist(err) { return nil, err } - var official bool - official, err = isOfficialReviewer(sess, issue, reviewer) + // 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 + 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 { - // recalculate which is the latest official review from that user - var review *Review + 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 + } + } - review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID) - if err != nil { + 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 } @@ -592,21 +789,20 @@ func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com } } - if err != nil { - return nil, err + if doer == nil { + return nil, sess.Commit() } - comment, err = createComment(sess, &CreateCommentOptions{ + comment, err := createComment(sess, &CreateCommentOptions{ Type: CommentTypeReviewRequest, Doer: doer, Repo: issue.Repo, Issue: issue, RemovedAssignee: true, // Use RemovedAssignee as !isRequest - AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID + AssigneeTeamID: reviewer.ID, // Use AssigneeTeamID as reviewer team ID }) - if err != nil { - return nil, err + return nil, fmt.Errorf("createComment(): %v", err) } return comment, sess.Commit() diff --git a/models/review_test.go b/models/review_test.go index 7103962ce..702e21682 100644 --- a/models/review_test.go +++ b/models/review_test.go @@ -130,6 +130,9 @@ func TestGetReviewersByIssueID(t *testing.T) { }) allReviews, err := GetReviewersByIssueID(issue.ID) + for _, reviewer := range allReviews { + assert.NoError(t, reviewer.LoadReviewer()) + } assert.NoError(t, err) if assert.Len(t, allReviews, 3) { for i, review := range allReviews { diff --git a/routers/repo/issue.go b/routers/repo/issue.go index f44e88fc4..ef10651aa 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -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 -func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issuePosterID int64) { - var err error - ctx.Data["Reviewers"], err = repo.GetReviewers(ctx.User.ID, issuePosterID) +func RetrieveRepoReviewers(ctx *context.Context, repo *models.Repository, issue *models.Issue, canChooseReviewer bool) { + ctx.Data["CanChooseReviewer"] = canChooseReviewer + + reviews, err := models.GetReviewersByIssueID(issue.ID) if err != nil { - ctx.ServerError("GetReviewers", err) + 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 { + ctx.ServerError("GetReviewers", err) + 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 @@ -981,13 +1155,7 @@ func ViewIssue(ctx *context.Context) { } } - if canChooseReviewer { - RetrieveRepoReviewers(ctx, repo, issue.PosterID) - ctx.Data["CanChooseReviewer"] = true - } else { - ctx.Data["CanChooseReviewer"] = false - } - + RetrieveRepoReviewers(ctx, repo, issue, canChooseReviewer) if ctx.Written() { return } @@ -1131,8 +1299,8 @@ func ViewIssue(ctx *context.Context) { } } else if comment.Type == models.CommentTypeAssignees || comment.Type == models.CommentTypeReviewRequest { - if err = comment.LoadAssigneeUser(); err != nil { - ctx.ServerError("LoadAssigneeUser", err) + if err = comment.LoadAssigneeUserAndTeam(); err != nil { + ctx.ServerError("LoadAssigneeUserAndTeam", err) return } } else if comment.Type == models.CommentTypeRemoveDependency || comment.Type == models.CommentTypeAddDependency { @@ -1279,12 +1447,6 @@ func ViewIssue(ctx *context.Context) { pull.HeadRepo != nil && git.IsBranchExist(pull.HeadRepo.RepoPath(), pull.HeadBranch) && (!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 @@ -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() { - 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() { - 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) @@ -1544,8 +1714,8 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models return err } - lastreview, err := models.GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID) - if err != nil { + lastreview, err := models.GetReviewByIssueIDAndUserID(issue.ID, reviewer.ID) + if err != nil && !models.IsErrReviewNotExist(err) { return err } @@ -1553,10 +1723,14 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models if isAdd { pemResult = permReviewer.CanAccessAny(models.AccessModeRead, models.UnitTypePullRequests) 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 } @@ -1567,33 +1741,103 @@ func isLegalReviewRequest(reviewer, doer *models.User, isAdd bool, issue *models return err } 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 { - 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 { - return fmt.Errorf("poster of pr can't be reviewer [user_id: %d, repo_name: %s]", reviewer.ID, issue.Repo.Name) + if reviewer.ID == issue.PosterID && issue.OriginalAuthorID == 0 { + return models.ErrNotValidReviewRequest{ + Reason: "poster of pr can't be reviewer", + UserID: doer.ID, + RepoID: issue.Repo.ID, + } } } else { - if lastreview.Type == models.ReviewTypeRequest && lastreview.ReviewerID == doer.ID { + if lastreview != nil && lastreview.Type == models.ReviewTypeRequest && lastreview.ReviewerID == doer.ID { return nil } pemResult = permDoer.IsAdmin() 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 } -// updatePullReviewRequest change pull's request reviewers -func updatePullReviewRequest(ctx *context.Context) { +func isValidTeamReviewRequest(reviewer *models.Team, doer *models.User, isAdd bool, issue *models.Issue) error { + 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) if ctx.Written() { return @@ -1609,27 +1853,105 @@ func updatePullReviewRequest(ctx *context.Context) { } 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 + } - reviewer, err := models.GetUserByID(reviewID) + team, err := models.GetTeamByID(-reviewID) if err != nil { - ctx.ServerError("GetUserByID", err) + ctx.ServerError("models.GetTeamByID", err) return } - err = isLegalReviewRequest(reviewer, ctx.User, action == "attach", issue) + 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 { - ctx.ServerError("isLegalRequestReview", err) + 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.ReviewRequest(issue, ctx.User, reviewer, action == "attach") + err = issue_service.TeamReviewRequest(issue, ctx.User, team, action == "attach") if err != nil { - ctx.ServerError("ReviewRequest", err) + ctx.ServerError("TeamReviewRequest", err) return } - } else { - ctx.Status(403) + continue + } + + reviewer, err := models.GetUserByID(reviewID) + 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) + return + } + + err = isValidReviewRequest(reviewer, ctx.User, action == "attach", issue) + if err != nil { + 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 + } + + err = issue_service.ReviewRequest(issue, ctx.User, reviewer, action == "attach") + if err != nil { + ctx.ServerError("ReviewRequest", err) return } } @@ -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 func UpdateIssueStatus(ctx *context.Context) { issues := getActionIssues(ctx) diff --git a/services/issue/assignee.go b/services/issue/assignee.go index d63c7bf03..f48e55e53 100644 --- a/services/issue/assignee.go +++ b/services/issue/assignee.go @@ -52,7 +52,7 @@ func ToggleAssignee(issue *models.Issue, doer *models.User, assigneeID int64) (r 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) { var comment *models.Comment if isAdd { @@ -71,3 +71,40 @@ func ReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.User 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 +} diff --git a/services/pull/review.go b/services/pull/review.go index 09ab3ff56..99afdd73c 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -68,14 +68,13 @@ func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models return nil, err } - review, err = models.CreateReview(models.CreateReviewOptions{ + if review, err = models.CreateReview(models.CreateReviewOptions{ Type: models.ReviewTypePending, Reviewer: doer, Issue: issue, Official: false, CommitID: latestCommitID, - }) - if err != nil { + }); err != nil { return nil, err } } diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 3b70a1071..f9ae9ba07 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -595,14 +595,22 @@ {{.Poster.GetDisplayName}} - {{if .RemovedAssignee}} - {{if eq .PosterID .AssigneeID}} - {{$.i18n.Tr "repo.issues.review.remove_review_request_self" $createdStr | Safe}} + {{if (gt .AssigneeID 0)}} + {{if .RemovedAssignee}} + {{if eq .PosterID .AssigneeID}} + {{$.i18n.Tr "repo.issues.review.remove_review_request_self" $createdStr | Safe}} + {{else}} + {{$.i18n.Tr "repo.issues.review.remove_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}} + {{end}} {{else}} - {{$.i18n.Tr "repo.issues.review.remove_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}} + {{$.i18n.Tr "repo.issues.review.add_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}} {{end}} {{else}} - {{$.i18n.Tr "repo.issues.review.add_review_request" (.Assignee.GetDisplayName|Escape) $createdStr | Safe}} + {{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}} diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl index 9111ab7e8..b21d53419 100644 --- a/templates/repo/issue/view_content/pull.tmpl +++ b/templates/repo/issue/view_content/pull.tmpl @@ -1,24 +1,31 @@ -{{if gt (len .PullReviewers) 0}} +{{if .PullReviewers }}

{{$.i18n.Tr "repo.issues.review.reviewers"}}

{{range .PullReviewers}} - {{ $createdStr:= TimeSinceUnix .UpdatedUnix $.Lang }} + {{ $createdStr:= TimeSinceUnix .Review.UpdatedUnix $.Lang }}
- - - - {{.Reviewer.Name}} - {{if eq .Type 1}} + {{if .User}} + + + + {{end}} + + {{if .User}} + {{.User.Name}} + {{else if .Team}} + {{$.Issue.Repo.OwnerName}}/{{.Team.Name}} + {{end}} + {{if eq .Review.Type 1}} {{$.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}} - {{else if eq .Type 3}} + {{else if eq .Review.Type 3}} {{$.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}} {{else}} {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} @@ -26,34 +33,23 @@
- {{if .Stale}} + {{if .Review.Stale}} {{end}} - - {{$canChoose := false}} - {{if eq .Type 4}} - {{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 }} - + {{if .CanChange }} + {{svg "octicon-sync"}} {{end}} - {{svg (printf "octicon-%s" .Type.Icon)}} + {{svg (printf "octicon-%s" .Review.Type.Icon)}}
diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index e47bcbe35..a1dbc7ef7 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -5,7 +5,7 @@ {{if .Issue.IsPull }} -
+
{{.i18n.Tr "repo.issues.review.reviewers"}} {{if and .CanChooseReviewer (not .Repository.IsArchived)}} @@ -20,36 +20,30 @@
{{end}} - {{range .Reviewers}} - {{$ReviewerID := .ID}} - {{$checked := false}} - {{$canChoose := false}} - {{$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}} + {{if .Reviewers}} + {{range .Reviewers}} + {{if .User}} + + {{svg "octicon-check"}} + + {{.User.GetDisplayName}} + + {{end}} {{end}} - - {{ if $notReviewed}} - {{$canChoose = true}} + {{end}} + {{if .TeamReviewers}} +
+ {{range .TeamReviewers}} + {{if .Team}} + + {{svg "octicon-check" 16}} + + {{svg "octicon-people" 16 "ml-4 mr-2"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}} + + + {{end}} {{end}} - - - {{svg "octicon-check"}} - - {{.GetDisplayName}} - - {{end}}
@@ -59,30 +53,23 @@
{{range .PullReviewers}}
-  {{.Reviewer.GetDisplayName}} -  {{.User.GetDisplayName}} + {{else if .Team}} + {{svg "octicon-people" 16 "teamavatar"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}} + {{end}} + - {{$canChoose := false}} - {{if eq .Type 4}} - {{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}} - + {{if .CanChange}} + {{svg "octicon-sync"}} {{end}} - {{svg (printf "octicon-%s" .Type.Icon)}} + {{svg (printf "octicon-%s" .Review.Type.Icon)}}
{{end}} diff --git a/web_src/js/index.js b/web_src/js/index.js index e1fd24f37..9fafe62d3 100644 --- a/web_src/js/index.js +++ b/web_src/js/index.js @@ -663,15 +663,16 @@ function initIssueComments() { const url = $(this).data('update-url'); const issueId = $(this).data('issue-id'); const id = $(this).data('id'); - const isChecked = $(this).data('is-checked'); + const isChecked = $(this).hasClass('checked'); event.preventDefault(); updateIssuesMeta( url, - isChecked === 'true' ? 'attach' : 'detach', + isChecked ? 'detach' : 'attach', issueId, id, ).then(reload); + return false; }); $(document).on('click', (event) => { diff --git a/web_src/less/_repository.less b/web_src/less/_repository.less index a6e3be68e..ca1a87b55 100644 --- a/web_src/less/_repository.less +++ b/web_src/less/_repository.less @@ -101,6 +101,12 @@ line-height: 2em; } + &.assignees .teamavatar { + margin-top: .125rem; + margin-left: 6.75px; + margin-right: 8.75px; + } + .hide { display: none !important; }