diff options
Diffstat (limited to 'models')
| -rw-r--r-- | models/branches.go | 82 | ||||
| -rw-r--r-- | models/fixtures/review.yml | 20 | ||||
| -rw-r--r-- | models/migrations/migrations.go | 2 | ||||
| -rw-r--r-- | models/migrations/v111.go | 87 | ||||
| -rw-r--r-- | models/org_team.go | 6 | ||||
| -rw-r--r-- | models/pull.go | 8 | ||||
| -rw-r--r-- | models/review.go | 130 | ||||
| -rw-r--r-- | models/review_test.go | 39 |
8 files changed, 267 insertions, 107 deletions
diff --git a/models/branches.go b/models/branches.go index 46670b18a0..cf4b4fe393 100644 --- a/models/branches.go +++ b/models/branches.go @@ -39,6 +39,7 @@ type ProtectedBranch struct { MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"` StatusCheckContexts []string `xorm:"JSON TEXT"` + EnableApprovalsWhitelist bool `xorm:"NOT NULL DEFAULT false"` ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"` ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"` RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` @@ -53,10 +54,25 @@ func (protectBranch *ProtectedBranch) IsProtected() bool { // CanUserPush returns if some user could push to this protected branch func (protectBranch *ProtectedBranch) CanUserPush(userID int64) bool { - if !protectBranch.EnableWhitelist { + if !protectBranch.CanPush { return false } + if !protectBranch.EnableWhitelist { + if user, err := GetUserByID(userID); err != nil { + log.Error("GetUserByID: %v", err) + return false + } else if repo, err := GetRepositoryByID(protectBranch.RepoID); err != nil { + log.Error("GetRepositoryByID: %v", err) + return false + } else if writeAccess, err := HasAccessUnit(user, repo, UnitTypeCode, AccessModeWrite); err != nil { + log.Error("HasAccessUnit: %v", err) + return false + } else { + return writeAccess + } + } + if base.Int64sContains(protectBranch.WhitelistUserIDs, userID) { return true } @@ -95,6 +111,38 @@ func (protectBranch *ProtectedBranch) CanUserMerge(userID int64) bool { return in } +// IsUserOfficialReviewer check if user is official reviewer for the branch (counts towards required approvals) +func (protectBranch *ProtectedBranch) IsUserOfficialReviewer(user *User) (bool, error) { + return protectBranch.isUserOfficialReviewer(x, user) +} + +func (protectBranch *ProtectedBranch) isUserOfficialReviewer(e Engine, user *User) (bool, error) { + repo, err := getRepositoryByID(e, protectBranch.RepoID) + if err != nil { + return false, err + } + + if !protectBranch.EnableApprovalsWhitelist { + // Anyone with write access is considered official reviewer + writeAccess, err := hasAccessUnit(e, user, repo, UnitTypeCode, AccessModeWrite) + if err != nil { + return false, err + } + return writeAccess, nil + } + + if base.Int64sContains(protectBranch.ApprovalsWhitelistUserIDs, user.ID) { + return true, nil + } + + inTeam, err := isUserInTeams(e, user.ID, protectBranch.ApprovalsWhitelistTeamIDs) + if err != nil { + return false, err + } + + return inTeam, nil +} + // HasEnoughApprovals returns true if pr has enough granted approvals. func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool { if protectBranch.RequiredApprovals == 0 { @@ -105,30 +153,16 @@ func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool { // GetGrantedApprovalsCount returns the number of granted approvals for pr. A granted approval must be authored by a user in an approval whitelist. func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest) int64 { - reviews, err := GetReviewersByPullID(pr.IssueID) + approvals, err := x.Where("issue_id = ?", pr.Issue.ID). + And("type = ?", ReviewTypeApprove). + And("official = ?", true). + Count(new(Review)) if err != nil { - log.Error("GetReviewersByPullID: %v", err) + log.Error("GetGrantedApprovalsCount: %v", err) return 0 } - approvals := int64(0) - userIDs := make([]int64, 0) - for _, review := range reviews { - if review.Type != ReviewTypeApprove { - continue - } - if base.Int64sContains(protectBranch.ApprovalsWhitelistUserIDs, review.ID) { - approvals++ - continue - } - userIDs = append(userIDs, review.ID) - } - approvalTeamCount, err := UsersInTeamsCount(userIDs, protectBranch.ApprovalsWhitelistTeamIDs) - if err != nil { - log.Error("UsersInTeamsCount: %v", err) - return 0 - } - return approvalTeamCount + approvals + return approvals } // GetProtectedBranchByRepoID getting protected branch by repo ID @@ -139,8 +173,12 @@ func GetProtectedBranchByRepoID(repoID int64) ([]*ProtectedBranch, error) { // GetProtectedBranchBy getting protected branch by ID/Name func GetProtectedBranchBy(repoID int64, branchName string) (*ProtectedBranch, error) { + return getProtectedBranchBy(x, repoID, branchName) +} + +func getProtectedBranchBy(e Engine, repoID int64, branchName string) (*ProtectedBranch, error) { rel := &ProtectedBranch{RepoID: repoID, BranchName: branchName} - has, err := x.Get(rel) + has, err := e.Get(rel) if err != nil { return nil, err } diff --git a/models/fixtures/review.yml b/models/fixtures/review.yml index a0122dea99..87621012ff 100644 --- a/models/fixtures/review.yml +++ b/models/fixtures/review.yml @@ -44,32 +44,32 @@ reviewer_id: 2 issue_id: 3 content: "New review 3" - updated_unix: 946684810 - created_unix: 946684810 + updated_unix: 946684811 + created_unix: 946684811 - id: 7 type: 3 reviewer_id: 3 issue_id: 3 content: "New review 4" - updated_unix: 946684810 - created_unix: 946684810 + updated_unix: 946684812 + created_unix: 946684812 - id: 8 type: 1 reviewer_id: 4 issue_id: 3 content: "New review 5" - updated_unix: 946684810 - created_unix: 946684810 + updated_unix: 946684813 + created_unix: 946684813 - id: 9 type: 3 reviewer_id: 2 issue_id: 3 content: "New review 3 rejected" - updated_unix: 946684810 - created_unix: 946684810 + updated_unix: 946684814 + created_unix: 946684814 - id: 10 @@ -77,5 +77,5 @@ reviewer_id: 100 issue_id: 3 content: "a deleted user's review" - updated_unix: 946684810 - created_unix: 946684810
\ No newline at end of file + updated_unix: 946684815 + created_unix: 946684815
\ No newline at end of file diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index 2de32f8e78..8aa39904f3 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -276,6 +276,8 @@ var migrations = []Migration{ NewMigration("add can_create_org_repo to team", addCanCreateOrgRepoColumnForTeam), // v110 -> v111 NewMigration("change review content type to text", changeReviewContentToText), + // v111 -> v112 + NewMigration("update branch protection for can push and whitelist enable", addBranchProtectionCanPushAndEnableWhitelist), } // Migrate database to current version diff --git a/models/migrations/v111.go b/models/migrations/v111.go new file mode 100644 index 0000000000..cf57e35dff --- /dev/null +++ b/models/migrations/v111.go @@ -0,0 +1,87 @@ +// Copyright 2019 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 ( + "code.gitea.io/gitea/models" + + "xorm.io/xorm" +) + +func addBranchProtectionCanPushAndEnableWhitelist(x *xorm.Engine) error { + + type ProtectedBranch struct { + CanPush bool `xorm:"NOT NULL DEFAULT false"` + EnableApprovalsWhitelist bool `xorm:"NOT NULL DEFAULT false"` + RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"` + } + + type Review struct { + ID int64 `xorm:"pk autoincr"` + Official bool `xorm:"NOT NULL DEFAULT false"` + } + + sess := x.NewSession() + defer sess.Close() + + if err := sess.Sync2(new(ProtectedBranch)); err != nil { + return err + } + + if err := sess.Sync2(new(Review)); err != nil { + return err + } + + if _, err := sess.Exec("UPDATE `protected_branch` SET `can_push` = `enable_whitelist`"); err != nil { + return err + } + if _, err := sess.Exec("UPDATE `protected_branch` SET `enable_approvals_whitelist` = ? WHERE `required_approvals` > ?", true, 0); err != nil { + return err + } + + var pageSize int64 = 20 + qresult, err := sess.QueryInterface("SELECT max(id) as max_id FROM issue") + if err != nil { + return err + } + var totalIssues int64 + totalIssues, ok := qresult[0]["max_id"].(int64) + if !ok { + // If there are no issues at all we ignore it + return nil + } + totalPages := totalIssues / pageSize + + // Find latest review of each user in each pull request, and set official field if appropriate + reviews := []*models.Review{} + var page int64 + for page = 0; page <= totalPages; page++ { + if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id > ? AND issue_id <= ? AND type in (?, ?) GROUP BY issue_id, reviewer_id)", + page*pageSize, (page+1)*pageSize, models.ReviewTypeApprove, models.ReviewTypeReject). + Find(&reviews); err != nil { + return err + } + + for _, review := range reviews { + if err := review.LoadAttributes(); err != nil { + // Error might occur if user or issue doesn't exist, ignore it. + continue + } + official, err := models.IsOfficialReviewer(review.Issue, review.Reviewer) + if err != nil { + // Branch might not be proteced or other error, ignore it. + continue + } + review.Official = official + + if _, err := sess.ID(review.ID).Cols("official").Update(review); err != nil { + return err + } + } + + } + + return sess.Commit() +} diff --git a/models/org_team.go b/models/org_team.go index 2dadf3820c..63c6e11636 100644 --- a/models/org_team.go +++ b/models/org_team.go @@ -914,7 +914,11 @@ func RemoveTeamMember(team *Team, userID int64) error { // IsUserInTeams returns if a user in some teams func IsUserInTeams(userID int64, teamIDs []int64) (bool, error) { - return x.Where("uid=?", userID).In("team_id", teamIDs).Exist(new(TeamUser)) + return isUserInTeams(x, userID, teamIDs) +} + +func isUserInTeams(e Engine, userID int64, teamIDs []int64) (bool, error) { + return e.Where("uid=?", userID).In("team_id", teamIDs).Exist(new(TeamUser)) } // UsersInTeamsCount counts the number of users which are in userIDs and teamIDs diff --git a/models/pull.go b/models/pull.go index 5b18c9ed10..27824c546a 100644 --- a/models/pull.go +++ b/models/pull.go @@ -159,16 +159,20 @@ func (pr *PullRequest) loadIssue(e Engine) (err error) { // LoadProtectedBranch loads the protected branch of the base branch func (pr *PullRequest) LoadProtectedBranch() (err error) { + return pr.loadProtectedBranch(x) +} + +func (pr *PullRequest) loadProtectedBranch(e Engine) (err error) { if pr.BaseRepo == nil { if pr.BaseRepoID == 0 { return nil } - pr.BaseRepo, err = GetRepositoryByID(pr.BaseRepoID) + pr.BaseRepo, err = getRepositoryByID(e, pr.BaseRepoID) if err != nil { return } } - pr.ProtectedBranch, err = GetProtectedBranchBy(pr.BaseRepo.ID, pr.BaseBranch) + pr.ProtectedBranch, err = getProtectedBranchBy(e, pr.BaseRepo.ID, pr.BaseBranch) return } diff --git a/models/review.go b/models/review.go index 62781bc0f6..04d363073a 100644 --- a/models/review.go +++ b/models/review.go @@ -10,7 +10,6 @@ import ( "code.gitea.io/gitea/modules/timeutil" "xorm.io/builder" - "xorm.io/core" ) // ReviewType defines the sort of feedback a review gives @@ -53,6 +52,8 @@ type Review struct { Issue *Issue `xorm:"-"` IssueID int64 `xorm:"index"` Content string `xorm:"TEXT"` + // Official is a review made by an assigned approver (counts towards approval) + Official bool `xorm:"NOT NULL DEFAULT false"` CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` @@ -122,23 +123,6 @@ func GetReviewByID(id int64) (*Review, error) { return getReviewByID(x, id) } -func getUniqueApprovalsByPullRequestID(e Engine, prID int64) (reviews []*Review, err error) { - reviews = make([]*Review, 0) - if err := e. - Where("issue_id = ? AND type = ?", prID, ReviewTypeApprove). - OrderBy("updated_unix"). - GroupBy("reviewer_id"). - Find(&reviews); err != nil { - return nil, err - } - return -} - -// GetUniqueApprovalsByPullRequestID returns all reviews submitted for a specific pull request -func GetUniqueApprovalsByPullRequestID(prID int64) ([]*Review, error) { - return getUniqueApprovalsByPullRequestID(x, prID) -} - // FindReviewOptions represent possible filters to find reviews type FindReviewOptions struct { Type ReviewType @@ -180,6 +164,27 @@ type CreateReviewOptions struct { Type ReviewType Issue *Issue Reviewer *User + Official 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) +} + +func isOfficialReviewer(e Engine, issue *Issue, reviewer *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 + } + + return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer) } func createReview(e Engine, opts CreateReviewOptions) (*Review, error) { @@ -190,6 +195,7 @@ func createReview(e Engine, opts CreateReviewOptions) (*Review, error) { Reviewer: opts.Reviewer, ReviewerID: opts.Reviewer.ID, Content: opts.Content, + Official: opts.Official, } if _, err := e.Insert(review); err != nil { return nil, err @@ -255,6 +261,8 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin return nil, nil, err } + var official = false + review, err := getCurrentReview(sess, doer, issue) if err != nil { if !IsErrReviewNotExist(err) { @@ -265,12 +273,24 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin return nil, nil, ContentEmptyErr{} } + if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject { + // Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared + 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 { + return nil, nil, err + } + } + // No current review. Create a new one! review, err = createReview(sess, CreateReviewOptions{ Type: reviewType, Issue: issue, Reviewer: doer, Content: content, + Official: official, }) if err != nil { return nil, nil, err @@ -283,10 +303,23 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin return nil, nil, ContentEmptyErr{} } + if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject { + // Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared + 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 { + return nil, nil, err + } + } + + review.Official = official review.Issue = issue review.Content = content review.Type = reviewType - if _, err := sess.ID(review.ID).Cols("content, type").Update(review); err != nil { + + if _, err := sess.ID(review.ID).Cols("content, type, official").Update(review); err != nil { return nil, nil, err } } @@ -307,46 +340,33 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin return review, comm, sess.Commit() } -// PullReviewersWithType represents the type used to display a review overview -type PullReviewersWithType struct { - User `xorm:"extends"` - Type ReviewType - ReviewUpdatedUnix timeutil.TimeStamp `xorm:"review_updated_unix"` -} +// GetReviewersByIssueID gets the latest review of each reviewer for a pull request +func GetReviewersByIssueID(issueID int64) (reviews []*Review, err error) { + reviewsUnfiltered := []*Review{} -// GetReviewersByPullID gets all reviewers for a pull request with the statuses -func GetReviewersByPullID(pullID int64) (issueReviewers []*PullReviewersWithType, err error) { - irs := []*PullReviewersWithType{} - if x.Dialect().DBType() == core.MSSQL { - err = x.SQL(`SELECT [user].*, review.type, review.review_updated_unix FROM -(SELECT review.id, review.type, review.reviewer_id, max(review.updated_unix) as review_updated_unix -FROM review WHERE review.issue_id=? AND (review.type = ? OR review.type = ?) -GROUP BY review.id, review.type, review.reviewer_id) as review -INNER JOIN [user] ON review.reviewer_id = [user].id ORDER BY review_updated_unix DESC`, - pullID, ReviewTypeApprove, ReviewTypeReject). - Find(&irs) - } else { - err = x.Select("`user`.*, review.type, max(review.updated_unix) as review_updated_unix"). - Table("review"). - Join("INNER", "`user`", "review.reviewer_id = `user`.id"). - Where("review.issue_id = ? AND (review.type = ? OR review.type = ?)", - pullID, ReviewTypeApprove, ReviewTypeReject). - GroupBy("`user`.id, review.type"). - OrderBy("review_updated_unix DESC"). - Find(&irs) + sess := x.NewSession() + defer sess.Close() + if err := sess.Begin(); err != nil { + return nil, err } - // We need to group our results by user id _and_ review type, otherwise the query fails when using postgresql. - // But becaus we're doing this, we need to manually filter out multiple reviews of different types by the - // same person because we only want to show the newest review grouped by user. Thats why we're using a map here. - issueReviewers = []*PullReviewersWithType{} - usersInArray := make(map[int64]bool) - for _, ir := range irs { - if !usersInArray[ir.ID] { - issueReviewers = append(issueReviewers, ir) - usersInArray[ir.ID] = true + // 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", + issueID, ReviewTypeApprove, ReviewTypeReject). + Find(&reviewsUnfiltered); 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) } } - return + return reviews, nil } diff --git a/models/review_test.go b/models/review_test.go index 3e7563b434..a94a65f754 100644 --- a/models/review_test.go +++ b/models/review_test.go @@ -98,7 +98,7 @@ func TestCreateReview(t *testing.T) { AssertExistsAndLoadBean(t, &Review{Content: "New Review"}) } -func TestGetReviewersByPullID(t *testing.T) { +func TestGetReviewersByIssueID(t *testing.T) { assert.NoError(t, PrepareTestDatabase()) issue := AssertExistsAndLoadBean(t, &Issue{ID: 3}).(*Issue) @@ -106,24 +106,29 @@ func TestGetReviewersByPullID(t *testing.T) { user3 := AssertExistsAndLoadBean(t, &User{ID: 3}).(*User) user4 := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User) - expectedReviews := []*PullReviewersWithType{} - expectedReviews = append(expectedReviews, &PullReviewersWithType{ - User: *user2, - Type: ReviewTypeReject, - ReviewUpdatedUnix: 946684810, - }, - &PullReviewersWithType{ - User: *user3, - Type: ReviewTypeReject, - ReviewUpdatedUnix: 946684810, + expectedReviews := []*Review{} + expectedReviews = append(expectedReviews, + &Review{ + Reviewer: user3, + Type: ReviewTypeReject, + UpdatedUnix: 946684812, }, - &PullReviewersWithType{ - User: *user4, - Type: ReviewTypeApprove, - ReviewUpdatedUnix: 946684810, + &Review{ + Reviewer: user4, + Type: ReviewTypeApprove, + UpdatedUnix: 946684813, + }, + &Review{ + Reviewer: user2, + Type: ReviewTypeReject, + UpdatedUnix: 946684814, }) - allReviews, err := GetReviewersByPullID(issue.ID) + allReviews, err := GetReviewersByIssueID(issue.ID) assert.NoError(t, err) - assert.Equal(t, expectedReviews, allReviews) + for i, review := range allReviews { + assert.Equal(t, expectedReviews[i].Reviewer, review.Reviewer) + assert.Equal(t, expectedReviews[i].Type, review.Type) + assert.Equal(t, expectedReviews[i].UpdatedUnix, review.UpdatedUnix) + } } |