diff options
Diffstat (limited to 'models')
| -rw-r--r-- | models/error.go | 20 | ||||
| -rw-r--r-- | models/fixtures/review.yml | 4 | ||||
| -rw-r--r-- | models/issue_comment.go | 29 | ||||
| -rw-r--r-- | models/migrations/migrations.go | 2 | ||||
| -rw-r--r-- | models/migrations/v153.go | 25 | ||||
| -rw-r--r-- | models/repo.go | 80 | ||||
| -rw-r--r-- | models/repo_test.go | 31 | ||||
| -rw-r--r-- | models/review.go | 392 | ||||
| -rw-r--r-- | models/review_test.go | 3 |
9 files changed, 449 insertions, 137 deletions
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 -} - -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 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,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 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 { |