diff --git a/models/error.go b/models/error.go
index 364924c996..be94d78891 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 35d3dee2e6..3db0b47353 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 726ed7472b..270a10e240 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 dda8c00941..c63e02f314 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 0000000000..1e5ae9f7da
--- /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 a743f65737..f505412e03 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
-}
+ var users []*User
-func (repo *Repository) getReviewersPublic(e Engine, doerID, posterID int64) (_ []*User, err error) {
+ 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
+ }
- users := make([]*User, 0)
+ return users, nil
+ }
- 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"
-
- 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
+ }
+
+ teams, err := GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
+ if err != nil {
return nil, err
}
- 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)
- }
- 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 045f94670b..a366772d5c 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 5f27e2b7fd..2c38176ef4 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,22 @@ 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 reviewer can make official reviews in issue (counts towards required approvals)
-func IsOfficialReviewer(issue *Issue, reviewer *User) (bool, error) {
- return isOfficialReviewer(x, issue, reviewer)
+// 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, reviewer *User) (bool, error) {
+func isOfficialReviewer(e Engine, issue *Issue, reviewers ...*User) (bool, error) {
pr, err := getPullRequestByIssueID(e, issue.ID)
if err != nil {
return false, err
@@ -215,20 +236,59 @@ func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
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) {
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) {
+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)
- 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 (?, ?, ?))",
- issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
+ 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,10 +587,16 @@ 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
+func AddReviewRequest(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
}
// 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
}
- sess := x.NewSession()
- defer sess.Close()
- if err := sess.Begin(); err != nil {
- return nil, err
- }
-
- var official bool
- official, err = isOfficialReviewer(sess, issue, reviewer)
-
+ official, err := isOfficialReviewer(sess, issue, reviewer, doer)
if err != nil {
return nil, err
- }
-
- if !official {
- official, err = isOfficialReviewer(sess, issue, doer)
-
- if err != nil {
- return nil, err
- }
- }
-
- if official {
+ } 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,33 @@ 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
- }
-
- if review.Type != ReviewTypeRequest {
- return nil, nil
- }
-
+func RemoveReviewRequest(issue *Issue, reviewer, 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 := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
+ if err != nil && !IsErrReviewNotExist(err) {
return nil, err
}
- var official bool
- official, err = isOfficialReviewer(sess, issue, reviewer)
- if err != nil {
- return
+ if review == nil || review.Type != ReviewTypeRequest {
+ return nil, nil
}
- if official {
- // recalculate which is the latest official review from that user
- var review *Review
+ if _, err = sess.Delete(review); err != nil {
+ return nil, err
+ }
- review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
- if err != nil {
+ 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
}
@@ -592,11 +676,7 @@ func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com
}
}
- if err != nil {
- return nil, err
- }
-
- comment, err = createComment(sess, &CreateCommentOptions{
+ comment, err := createComment(sess, &CreateCommentOptions{
Type: CommentTypeReviewRequest,
Doer: doer,
Repo: issue.Repo,
@@ -604,7 +684,6 @@ func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com
RemovedAssignee: true, // Use RemovedAssignee as !isRequest
AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID
})
-
if err != nil {
return nil, err
}
@@ -612,6 +691,123 @@ func RemoveReviewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com
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
func MarkConversation(comment *Comment, doer *User, isResolve bool) (err error) {
if comment.Type != CommentTypeCode {
diff --git a/models/review_test.go b/models/review_test.go
index 7103962ce9..702e216824 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 f44e88fc4b..ef10651aa9 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,29 +1853,107 @@ func updatePullReviewRequest(ctx *context.Context) {
}
for _, issue := range issues {
- if issue.IsPull {
+ if err := issue.LoadRepo(); err != nil {
+ ctx.ServerError("issue.LoadRepo", err)
+ return
+ }
- reviewer, err := models.GetUserByID(reviewID)
- if err != nil {
- ctx.ServerError("GetUserByID", err)
- return
- }
-
- err = isLegalReviewRequest(reviewer, ctx.User, action == "attach", issue)
- if err != nil {
- ctx.ServerError("isLegalRequestReview", err)
- return
- }
-
- err = issue_service.ReviewRequest(issue, ctx.User, reviewer, action == "attach")
- if err != nil {
- ctx.ServerError("ReviewRequest", err)
- return
- }
- } else {
+ 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)
+ 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
+ }
}
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
func UpdateIssueStatus(ctx *context.Context) {
issues := getActionIssues(ctx)
diff --git a/services/issue/assignee.go b/services/issue/assignee.go
index d63c7bf032..f48e55e53c 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 09ab3ff567..99afdd73c2 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 3b70a10715..f9ae9ba071 100644
--- a/templates/repo/issue/view_content/comments.tmpl
+++ b/templates/repo/issue/view_content/comments.tmpl
@@ -595,14 +595,22 @@
</a>
<span class="text grey">
<a class="author" href="{{.Poster.HomeLink}}">{{.Poster.GetDisplayName}}</a>
- {{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}}
</span>
</div>
diff --git a/templates/repo/issue/view_content/pull.tmpl b/templates/repo/issue/view_content/pull.tmpl
index 9111ab7e86..b21d53419f 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 }}
<div class="comment box">
<div class="content">
<div class="ui segment">
<h4>{{$.i18n.Tr "repo.issues.review.reviewers"}}</h4>
{{range .PullReviewers}}
- {{ $createdStr:= TimeSinceUnix .UpdatedUnix $.Lang }}
+ {{ $createdStr:= TimeSinceUnix .Review.UpdatedUnix $.Lang }}
<div class="ui divider"></div>
<div class="review-item">
<div class="review-item-left">
- <a class="ui avatar image" href="{{.Reviewer.HomeLink}}">
- <img src="{{.Reviewer.RelAvatarLink}}">
- </a>
- <span class="text grey"><a href="{{.Reviewer.HomeLink}}">{{.Reviewer.Name}}</a>
- {{if eq .Type 1}}
+ {{if .User}}
+ <a class="ui avatar image" href="{{.User.HomeLink}}">
+ <img src="{{.User.RelAvatarLink}}">
+ </a>
+ {{end}}
+ <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}}
- {{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 @@
</span>
</div>
<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"}}">
<i class="octicon icon fa-hourglass-end"></i>
</span>
{{end}}
- <span class="type-icon text {{if eq .Type 1}}green
- {{- else if eq .Type 2}}grey
- {{- else if eq .Type 3}}red
- {{- else if eq .Type 4}}yellow
+ <span class="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}}">
- {{$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 }}
- <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">
+ {{if .CanChange }}
+ <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">
{{svg "octicon-sync"}}
</a>
{{end}}
- {{svg (printf "octicon-%s" .Type.Icon)}}
+ {{svg (printf "octicon-%s" .Review.Type.Icon)}}
</span>
</div>
</div>
diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl
index e47bcbe35c..a1dbc7ef7f 100644
--- a/templates/repo/issue/view_content/sidebar.tmpl
+++ b/templates/repo/issue/view_content/sidebar.tmpl
@@ -5,7 +5,7 @@
{{if .Issue.IsPull }}
<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">
<strong>{{.i18n.Tr "repo.issues.review.reviewers"}}</strong>
{{if and .CanChooseReviewer (not .Repository.IsArchived)}}
@@ -20,36 +20,30 @@
<input type="text" placeholder="{{.i18n.Tr "repo.issues.filter_reviewers"}}">
</div>
{{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}}
+ <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}}>
+ <span class="octicon-check {{if not .Checked}}invisible{{end}}">{{svg "octicon-check"}}</span>
+ <span class="text">
+ <img class="ui avatar image" src="{{.User.RelAvatarLink}}"> {{.User.GetDisplayName}}
+ </span>
+ </a>
{{end}}
{{end}}
-
- {{ if $notReviewed}}
- {{$canChoose = true}}
+ {{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}}
-
- <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">
- <img class="ui avatar image" src="{{.RelAvatarLink}}"> {{.GetDisplayName}}
- </span>
- </a>
{{end}}
</div>
</div>
@@ -59,30 +53,23 @@
<div class="selected">
{{range .PullReviewers}}
<div class="item" style="margin-bottom: 10px;">
- <a href="{{.Reviewer.HomeLink}}"><img class="ui avatar image" src="{{.Reviewer.RelAvatarLink}}"> {{.Reviewer.GetDisplayName}}</a>
- <span class="ui right type-icon text {{if eq .Type 1}}green
- {{- else if eq .Type 2}}grey
- {{- else if eq .Type 3}}red
- {{- else if eq .Type 4}}yellow
+ {{if .User}}
+ <a href="{{.User.HomeLink}}"><img class="ui avatar image" src="{{.User.RelAvatarLink}}"> {{.User.GetDisplayName}}</a>
+ {{else if .Team}}
+ <span class="text">{{svg "octicon-people" 16 "teamavatar"}}{{$.Issue.Repo.OwnerName}}/{{.Team.Name}}</span>
+ {{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 ">
- {{$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}}
- <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">
+ {{if .CanChange}}
+ <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">
{{svg "octicon-sync"}}
</a>
{{end}}
- {{svg (printf "octicon-%s" .Type.Icon)}}
+ {{svg (printf "octicon-%s" .Review.Type.Icon)}}
</span>
</div>
{{end}}
diff --git a/web_src/js/index.js b/web_src/js/index.js
index e1fd24f37c..9fafe62d3e 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 a6e3be68e7..ca1a87b557 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;
}