Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add team support for review request #12039

Merged
merged 39 commits into from
Oct 12, 2020
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
39 commits
Select commit Hold shift + click to select a range
cfa6db7
Add team support for review request
a1012112796 Jun 20, 2020
0501111
Merge branch 'master' into team_review_request
a1012112796 Jun 24, 2020
3196c8e
fix lint
a1012112796 Jun 24, 2020
8e37106
Merge branch 'master' into team_review_request
a1012112796 Sep 13, 2020
3c02d48
Apply review suggestion
a1012112796 Sep 13, 2020
df750f5
Merge branch 'master' into team_review_request
a1012112796 Sep 16, 2020
fbef6d7
Apply suggestion from review @lafriks
a1012112796 Sep 17, 2020
66a0f47
fix lint
a1012112796 Sep 17, 2020
3685fc6
Merge branch 'master' into team_review_request
a1012112796 Sep 28, 2020
f36a5ec
Update models/migrations/v153.go
a1012112796 Sep 28, 2020
26eb8c4
Apply suggestion from code review @lafriks
a1012112796 Sep 29, 2020
a6b94f6
Merge branch 'master' into team_review_request
a1012112796 Sep 29, 2020
515d09d
fix lint
a1012112796 Sep 29, 2020
3f8fee5
fix test
a1012112796 Sep 29, 2020
39f0b94
Update models/repo.go
a1012112796 Oct 5, 2020
94ee9d5
Apply suggestion from code review
a1012112796 Oct 5, 2020
ae7b7ad
Merge branch 'master' into team_review_request
a1012112796 Oct 5, 2020
a4e0ffc
Merge branch 'master' into team_review_request
a1012112796 Oct 6, 2020
7d75af0
splite get reviewers and add some test
a1012112796 Oct 6, 2020
f81ea8d
Merge branch 'master' into team_review_request
a1012112796 Oct 7, 2020
49ddcc7
fix code style
a1012112796 Oct 7, 2020
1037845
Apply suggestions from code review
a1012112796 Oct 9, 2020
c05ba9f
Merge branch 'master' into team_review_request
a1012112796 Oct 9, 2020
c25e769
Apply suggestions from code review
a1012112796 Oct 11, 2020
c83cc04
Merge branch 'master' into team_review_request
a1012112796 Oct 11, 2020
ef394de
Remove getPrivateReviewers and getPublicReviewer
zeripath Oct 11, 2020
877c38b
Simplifications
zeripath Oct 11, 2020
170b57f
Simplify and improve logging
zeripath Oct 11, 2020
75a1ae6
Log the underlying panic in runMigrateTask (#13096)
zeripath Oct 11, 2020
3035a37
Update routers/repo/issue.go
a1012112796 Oct 12, 2020
6b8e0be
Merge pull request #2 from zeripath/team_review_request
a1012112796 Oct 12, 2020
b2f1db1
Merge branch 'master' into team_review_request
a1012112796 Oct 12, 2020
386cb75
fix lint and return 403 for reviewer not exist
a1012112796 Oct 12, 2020
b13cad0
fix bugs
a1012112796 Oct 12, 2020
c0a29f7
Merge branch 'master' into team_review_request
a1012112796 Oct 12, 2020
0b909f9
Update models/review.go
a1012112796 Oct 12, 2020
857f5de
Merge branch 'master' into team_review_request
zeripath Oct 12, 2020
e5d3c88
Add octicon-people to the team reviews
zeripath Oct 12, 2020
9da446e
Merge branch 'master' into team_review_request
lafriks Oct 12, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions models/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ type Comment struct {
AssigneeID int64
RemovedAssignee bool
Assignee *User `xorm:"-"`
TeamAssignee *Team `xorm:"-"`
ResolveDoerID int64
ResolveDoer *User `xorm:"-"`
OldTitle string
Expand Down Expand Up @@ -465,6 +466,25 @@ func (c *Comment) LoadAssigneeUser() error {
}
c.Assignee = NewGhostUser()
}
} else {
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.TeamAssignee, err = GetTeamByID(-c.AssigneeID)
if err != nil && !IsErrTeamNotExist(err) {
return err
}
}
}
return nil
}
Expand Down
18 changes: 15 additions & 3 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -685,9 +685,9 @@ 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) {
func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []*User, teams []*Team, err error) {
lafriks marked this conversation as resolved.
Show resolved Hide resolved
if err = repo.getOwner(e); err != nil {
return nil, err
return nil, nil, err
}

if repo.IsPrivate ||
Expand All @@ -696,6 +696,18 @@ func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []
} else {
users, err = repo.getReviewersPublic(x, doerID, posterID)
}

if repo.Owner.IsOrganization() {
teams, err = GetTeamsWithAccessToRepo(repo.OwnerID, repo.ID, AccessModeRead)
if err != nil {
return nil, nil, err
}

for _, team := range teams {
team.ID = -team.ID
lafriks marked this conversation as resolved.
Show resolved Hide resolved
}
}

return
}

Expand All @@ -704,7 +716,7 @@ func (repo *Repository) getReviewers(e Engine, doerID, posterID int64) (users []
// 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) {
func (repo *Repository) GetReviewers(doerID, posterID int64) (_ []*User, _ []*Team, err error) {
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
return repo.getReviewers(x, doerID, posterID)
}

Expand Down
195 changes: 192 additions & 3 deletions models/review.go
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"
"strings"

"code.gitea.io/gitea/modules/base"
"code.gitea.io/gitea/modules/timeutil"

"xorm.io/builder"
Expand Down Expand Up @@ -53,6 +54,7 @@ type Review struct {
ID int64 `xorm:"pk autoincr"`
Type ReviewType
Reviewer *User `xorm:"-"`
ReviewerTeam *Team `xorm:"-"`
ReviewerID int64 `xorm:"index"`
OriginalAuthor string
OriginalAuthorID int64
Expand Down Expand Up @@ -101,6 +103,11 @@ func (r *Review) loadReviewer(e Engine) (err error) {
if r.Reviewer != nil || r.ReviewerID == 0 {
return nil
}

if r.ReviewerID < 0 {
r.ReviewerTeam, err = getTeamByID(e, -r.ReviewerID)
return
}
r.Reviewer, err = getUserByID(e, r.ReviewerID)
return
}
Expand Down Expand Up @@ -218,6 +225,30 @@ func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
return pr.ProtectedBranch.isUserOfficialReviewer(e, 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 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,
Expand Down Expand Up @@ -373,6 +404,29 @@ 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_id < 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
return nil, nil, err
}

if len(teamReviewRequests) > 0 {
for _, teamReviewRequest := range teamReviewRequests {
ok := false
if ok, err = isTeamMember(sess, issue.Repo.OwnerID, -teamReviewRequest.ReviewerID, doer.ID); err != nil {
return nil, nil, err
}

if ok {
if _, err := sess.Delete(teamReviewRequest); err != nil {
return nil, nil, err
}
}
}
}
}

comm.Review = review
return review, comm, sess.Commit()
}
Expand All @@ -397,7 +451,7 @@ func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) {
// Load reviewer and skip if user is deleted
for _, review := range reviewsUnfiltered {
if err = review.loadReviewer(sess); err != nil {
if !IsErrUserNotExist(err) {
if !IsErrUserNotExist(err) && !IsErrTeamNotExist(err) {
return nil, err
}
} else {
Expand Down Expand Up @@ -482,7 +536,7 @@ 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) {
func AddReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err error) {
review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID)
if err != nil {
return
Expand Down Expand Up @@ -549,7 +603,7 @@ 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) {
func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (comment *Comment, err error) {
review, err := GetReviewerByIssueIDAndUserID(issue.ID, reviewer.ID)
if err != nil {
return
Expand Down Expand Up @@ -612,6 +666,141 @@ 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 *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
}

sess := x.NewSession()
defer sess.Close()
if err := sess.Begin(); err != nil {
return nil, err
}

var official bool
official, err = isOfficialReviewerTeam(sess, issue, reviewer)

a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}

if !official {
official, err = isOfficialReviewer(sess, issue, doer)

a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}
}

if official {
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, -reviewer.ID); err != nil {
return nil, err
}
}

_, err = createReview(sess, CreateReviewOptions{
Type: ReviewTypeRequest,
Issue: issue,
Reviewer: &User{ID: -reviewer.ID},
Official: official,
Stale: false,
})

if err != nil {
return
}

comment, err = createComment(sess, &CreateCommentOptions{
Type: CommentTypeReviewRequest,
Doer: doer,
Repo: issue.Repo,
Issue: issue,
RemovedAssignee: false, // Use RemovedAssignee as !isRequest
AssigneeID: -reviewer.ID, // Use AssigneeID as reviewer ID
})

a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return nil, err
}

return comment, sess.Commit()
}

//RemoveTeamReviewRequest remove a review request from one team
func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (comment *Comment, err error) {
review, err := GetReviewerByIssueIDAndUserID(issue.ID, -reviewer.ID)
if err != nil {
return
}

if review.Type != ReviewTypeRequest {
return nil, nil
}

sess := x.NewSession()
defer sess.Close()
if err := sess.Begin(); err != nil {
return nil, err
}

_, err = sess.Delete(review)
if err != nil {
return nil, err
}

var official bool
official, err = isOfficialReviewerTeam(sess, issue, reviewer)
if err != nil {
return
}

if official {
// recalculate which is the latest official review from that team
var review *Review

review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, -reviewer.ID)
if err != nil {
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 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
AssigneeID: -reviewer.ID, // Use AssigneeID as reviewer ID
})

if err != nil {
return nil, 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 {
Expand Down
Loading