Move official reviews to db instead of counting approvals each time

Author: davidsvantesson

models/branches.go

@@ -111,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 {
@@ -121,46 +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
 	}
 
-	repo, err := GetRepositoryByID(protectBranch.RepoID)
-	if err != nil {
-		log.Error("GetRepositoryByID: %v", err)
-		return 0
-	}
-
-	approvals := int64(0)
-	userIDs := make([]int64, 0)
-	for _, review := range reviews {
-		if review.Type != ReviewTypeApprove {
-			continue
-		}
-		if !protectBranch.EnableApprovalsWhitelist {
-			if user, err := GetUserByID(review.ID); err != nil {
-				log.Error("GetUserByID: %v", err)
-			} else if writeAccess, err := HasAccessUnit(user, repo, UnitTypeCode, AccessModeWrite); err != nil {
-				log.Error("HasAccessUnit: %v", err)
-			} else if writeAccess {
-				approvals++
-			}
-			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
@@ -171,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
 	}

models/migrations/v110.go

@@ -5,6 +5,9 @@
 package migrations
 
 import (
+	"code.gitea.io/gitea/models"
+
+	"xorm.io/core"
 	"xorm.io/xorm"
 )
 
@@ -17,16 +20,74 @@ func addBranchProtectionCanPushAndEnableWhitelist(x *xorm.Engine) error {
 		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 := x.Sync2(new(ProtectedBranch)); err != nil {
+	if err := sess.Sync2(new(ProtectedBranch)); err != nil {
 		return err
 	}
 
-	if _, err := x.Exec("UPDATE `protected_branch` SET `can_push` = `enable_whitelist`"); err != nil {
+	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
 	}
-	_, err := x.Exec("UPDATE `protected_branch` SET `enable_approvals_whitelist` = ? WHERE `required_approvals` > ?", true, 0)
-	return err
+
+	// Find latest review of each user in each pull request, and set official field if appropriate
+	reviews := []*models.Review{}
+	if x.Dialect().DBType() == core.MSSQL {
+		if err := x.SQL(`SELECT *, max(review.updated_unix) as review_updated_unix FROM review WHERE (review.type = ? OR review.type = ?)
+GROUP BY review.id, review.issue_id, review.reviewer_id, review.type, ) as review
+ORDER BY review_updated_unix DESC`,
+			models.ReviewTypeApprove, models.ReviewTypeReject).
+			Find(&reviews); err != nil {
+			return err
+		}
+	} else {
+		if err := x.Select("review.*, max(review.updated_unix) as review_updated_unix").
+			Table("review").
+			Join("INNER", "`user`", "review.reviewer_id = `user`.id").
+			Where("(review.type = ? OR review.type = ?)",
+				models.ReviewTypeApprove, models.ReviewTypeReject).
+			GroupBy("review.issue_id, review.reviewer_id, review.type").
+			OrderBy("review_updated_unix DESC").
+			Find(&reviews); err != nil {
+			return err
+		}
+	}
+
+	// We need to group our results by user id _and_ review type, otherwise the query fails when using postgresql.
+	usersInArray := make(map[int64]map[int64]bool)
+	for _, review := range reviews {
+		if usersInArray[review.IssueID] == nil {
+			usersInArray[review.IssueID] = make(map[int64]bool)
+		}
+		if !usersInArray[review.IssueID][review.ReviewerID] {
+			if err := review.LoadAttributes(); err != nil {
+				return err
+			}
+			official, err := models.IsOfficialReviewer(review.Issue, review.Reviewer)
+			if err != nil {
+				return err
+			}
+			review.Official = official
+
+			if _, err := sess.ID(review.ID).Cols("official").Update(review); err != nil {
+				return err
+			}
+			usersInArray[review.IssueID][review.ReviewerID] = true
+		}
+	}
+
+	return sess.Commit()
 }

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

models/pull.go

@@ -160,16 +160,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
 }
 

models/review.go

@@ -53,6 +53,8 @@ type Review struct {
 	Issue      *Issue `xorm:"-"`
 	IssueID    int64  `xorm:"index"`
 	Content    string
+	// 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 +124,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
@@ -182,14 +167,40 @@ type CreateReviewOptions struct {
 	Reviewer *User
 }
 
+// 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) {
+	official, err := isOfficialReviewer(e, opts.Issue, opts.Reviewer)
+	if err != nil {
+		return nil, err
+	}
+
 	review := &Review{
 		Type:       opts.Type,
 		Issue:      opts.Issue,
 		IssueID:    opts.Issue.ID,
 		Reviewer:   opts.Reviewer,
 		ReviewerID: opts.Reviewer.ID,
 		Content:    opts.Content,
+		Official:   official,
 	}
 	if _, err := e.Insert(review); err != nil {
 		return nil, err
@@ -250,6 +261,11 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
 		return nil, nil, err
 	}
 
+	// Only reviewers latest review 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
+	}
+
 	review, err := getCurrentReview(sess, doer, issue)
 	if err != nil {
 		if !IsErrReviewNotExist(err) {
@@ -278,10 +294,18 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
 			return nil, nil, ContentEmptyErr{}
 		}
 
+		// Official status of review updated at every submit in case settings changed
+		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,22 +331,23 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content strin
 type PullReviewersWithType struct {
 	User              `xorm:"extends"`
 	Type              ReviewType
+	Official          bool
 	ReviewUpdatedUnix timeutil.TimeStamp `xorm:"review_updated_unix"`
 }
 
 // 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
+		err = x.SQL(`SELECT [user].*, review.type, review.official, 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").
+		err = x.Select("`user`.*, review.type, review.official, 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 = ?)",