diff --git a/models/branches.go b/models/branches.go index 75f5c0a3a7be8..540b6614ffc66 100644 --- a/models/branches.go +++ b/models/branches.go @@ -148,31 +148,6 @@ func (protectBranch *ProtectedBranch) isUserOfficialReviewer(e Engine, user *Use return inTeam, nil } -// HasEnoughApprovals returns true if pr has enough granted approvals. -func (protectBranch *ProtectedBranch) HasEnoughApprovals(pr *PullRequest) bool { - if protectBranch.RequiredApprovals == 0 { - return true - } - return protectBranch.GetGrantedApprovalsCount(pr) >= protectBranch.RequiredApprovals -} - -// 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 { - sess := x.Where("issue_id = ?", pr.IssueID). - And("type = ?", ReviewTypeApprove). - And("official = ?", true) - if protectBranch.DismissStaleApprovals { - sess = sess.And("stale = ?", false) - } - approvals, err := sess.Count(new(Review)) - if err != nil { - log.Error("GetGrantedApprovalsCount: %v", err) - return 0 - } - - return approvals -} - // MergeBlockedByRejectedReview returns true if merge is blocked by rejected reviews func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullRequest) bool { if !protectBranch.BlockOnRejectedReviews { diff --git a/models/pull.go b/models/pull.go index 46c50986b92b1..657306b7edf58 100644 --- a/models/pull.go +++ b/models/pull.go @@ -89,6 +89,10 @@ func (pr *PullRequest) loadAttributes(e Engine) (err error) { } } + if err = pr.loadProtectedBranch(e); err != nil { + return fmt.Errorf("pr.loadAttributes: loadProtectedBranch: %v", err) + } + return nil } @@ -171,6 +175,79 @@ func (pr *PullRequest) loadProtectedBranch(e Engine) (err error) { return } +func (pr *PullRequest) requiredApprovals() int64 { + if pr.ProtectedBranch != nil { + return pr.ProtectedBranch.RequiredApprovals + } + return 0 +} + +// RequiresApproval returns true if this pull request requires approval, otherwise returns false +func (pr *PullRequest) RequiresApproval() bool { + return pr.requiredApprovals() > 0 +} + +// ReviewStatusCount holds the Status and Count of a Review +type ReviewStatusCount struct { + Status string + Count int64 +} + +// GetReviewStatus returns a ReviewStatusCount of this pull request +func (pr *PullRequest) GetReviewStatus() ReviewStatusCount { + if pr.RequiresApproval() { + rejections := pr.GetRejectedReviewsCount() + if rejections > 0 { + return ReviewStatusCount{Status: "rejected", Count: rejections} + } + approvals := pr.GetGrantedApprovalsCount() + if approvals >= pr.ProtectedBranch.RequiredApprovals { + return ReviewStatusCount{Status: "approved", Count: approvals} + } + return ReviewStatusCount{Status: "required", Count: pr.ProtectedBranch.RequiredApprovals - approvals} + } + return ReviewStatusCount{Status: "approved", Count: 0} // by default +} + +// GetRejectedReviewsCount returns the number of rejected reviews for pr. +func (pr *PullRequest) GetRejectedReviewsCount() int64 { + rejects, err := x.Where("issue_id = ?", pr.IssueID). + And("type = ?", ReviewTypeReject). + And("official = ?", true). + Count(new(Review)) + if err != nil { + log.Error("GetRejectedReviewsCount: %v", err) + return 0 + } + + return rejects +} + +// HasEnoughApprovals returns true if pr has enough granted approvals. +func (pr *PullRequest) HasEnoughApprovals() bool { + if pr.ProtectedBranch.RequiredApprovals == 0 { + return true + } + return pr.GetGrantedApprovalsCount() >= pr.ProtectedBranch.RequiredApprovals +} + +// GetGrantedApprovalsCount returns the number of granted approvals for pr. A granted approval must be authored by a user in an approval whitelist. +func (pr *PullRequest) GetGrantedApprovalsCount() int64 { + sess := x.Where("issue_id = ?", pr.IssueID). + And("type = ?", ReviewTypeApprove). + And("official = ?", true) + if pr.ProtectedBranch.DismissStaleApprovals { + sess = sess.And("stale = ?", false) + } + approvals, err := sess.Count(new(Review)) + if err != nil { + log.Error("GetGrantedApprovalsCount: %v", err) + return 0 + } + + return approvals +} + // GetDefaultMergeMessage returns default message used when merging pull request func (pr *PullRequest) GetDefaultMergeMessage() string { if pr.HeadRepo == nil { diff --git a/models/pull_list.go b/models/pull_list.go index 989de46891e22..44e2f0d3988e4 100644 --- a/models/pull_list.go +++ b/models/pull_list.go @@ -113,7 +113,30 @@ func (prs PullRequestList) loadAttributes(e Engine) error { return nil } - // Load issues. + if err := prs.loadIssues(e); err != nil { + return fmt.Errorf("prs.loadAttributes: loadIssues: %v", err) + } + + if err := prs.loadProtectedBranches(e); err != nil { + return fmt.Errorf("prs.loadAttributes: loadProtectedBranches: %v", err) + } + + return nil +} + +func (prs PullRequestList) getIssueIDs() []int64 { + issueIDs := make([]int64, 0, len(prs)) + for i := range prs { + issueIDs = append(issueIDs, prs[i].IssueID) + } + return issueIDs +} + +func (prs PullRequestList) loadIssues(e Engine) (err error) { + if len(prs) == 0 { + return nil + } + issueIDs := prs.getIssueIDs() issues := make([]*Issue, 0, len(issueIDs)) if err := e. @@ -133,12 +156,54 @@ func (prs PullRequestList) loadAttributes(e Engine) error { return nil } -func (prs PullRequestList) getIssueIDs() []int64 { - issueIDs := make([]int64, 0, len(prs)) - for i := range prs { - issueIDs = append(issueIDs, prs[i].IssueID) +type protectedBranchKey struct { + RepoID int64 + BranchName string +} + +func (prs PullRequestList) getProtectedBranchKeys() []protectedBranchKey { + prKeys := make(map[protectedBranchKey]struct{}, len(prs)) + for _, pr := range prs { + if pr.BaseRepoID <= 0 { + continue + } + key := protectedBranchKey{pr.BaseRepoID, pr.BaseBranch} + if _, ok := prKeys[key]; !ok { + prKeys[key] = struct{}{} + } } - return issueIDs + return valuesProtectedBranchKeys(prKeys) +} + +func valuesProtectedBranchKeys(m map[protectedBranchKey]struct{}) []protectedBranchKey { + values := make([]protectedBranchKey, 0, len(m)) + for k := range m { + values = append(values, k) + } + return values +} + +func (prs PullRequestList) loadProtectedBranches(e Engine) (err error) { + if len(prs) == 0 { + return nil + } + + prKeys := prs.getProtectedBranchKeys() + prMaps := make(map[protectedBranchKey]*ProtectedBranch, len(prKeys)) + sess := e.Where("repo_id = ? and branch_name = ?", prKeys[0].RepoID, prKeys[0].BranchName) + for _, prk := range prKeys[1:] { + sess.Or("repo_id = ? and branch_name = ?", prk.RepoID, prk.BranchName) + } + err = sess.Find(&prMaps) + if err != nil { + return err + } + + for _, pr := range prs { + pr.ProtectedBranch = prMaps[protectedBranchKey{pr.BaseRepoID, pr.BaseBranch}] + } + + return nil } // LoadAttributes load all the prs attributes diff --git a/models/pull_sign.go b/models/pull_sign.go index 5b26b4bdc9e80..3c36b37f6cb32 100644 --- a/models/pull_sign.go +++ b/models/pull_sign.go @@ -57,7 +57,7 @@ func (pr *PullRequest) SignMerge(u *User, tmpBasePath, baseCommit, headCommit st if protectedBranch == nil { return false, "", &ErrWontSign{approved} } - if protectedBranch.GetGrantedApprovalsCount(pr) < 1 { + if pr.GetGrantedApprovalsCount() < 1 { return false, "", &ErrWontSign{approved} } case baseSigned: diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 7fe7bf697dc55..983dcbadc378f 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -1100,6 +1100,9 @@ pulls.update_branch = Update branch pulls.update_branch_success = Branch update was successful pulls.update_not_allowed = You are not allowed to update branch pulls.outdated_with_base_branch = This branch is out-of-date with the base branch +pulls.review_required = Review required %d +pulls.review_approved = Approved %d +pulls.review_rejected = Changes requested %d milestones.new = New Milestone milestones.open_tab = %d Open diff --git a/routers/repo/issue.go b/routers/repo/issue.go index fdade2795d164..1d1313ca23c8a 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -229,8 +229,8 @@ func issues(ctx *context.Context, milestoneID int64, isPullOption util.OptionalB } if issues[i].IsPull { - if err := issues[i].LoadPullRequest(); err != nil { - ctx.ServerError("LoadPullRequest", err) + if err := issues[i].PullRequest.LoadAttributes(); err != nil { + ctx.ServerError("LoadAttributes", err) return } @@ -971,8 +971,8 @@ func ViewIssue(ctx *context.Context) { return } if pull.ProtectedBranch != nil { - cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull) - ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull) + cnt := pull.GetGrantedApprovalsCount() + ctx.Data["IsBlockedByApprovals"] = !pull.HasEnoughApprovals() ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull) ctx.Data["GrantedApprovals"] = cnt ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits diff --git a/routers/user/home.go b/routers/user/home.go index 6b71c51de32a5..3cf5a35e921da 100644 --- a/routers/user/home.go +++ b/routers/user/home.go @@ -535,6 +535,11 @@ func Issues(ctx *context.Context) { if isPullList { commitStatus[issue.PullRequest.ID], _ = issue.PullRequest.GetLastCommitStatus() + + if err := issue.PullRequest.LoadAttributes(); err != nil { + ctx.ServerError("LoadAttributes", err) + return + } } } diff --git a/services/pull/merge.go b/services/pull/merge.go index 4d02d7193dea7..c7ce50399581d 100644 --- a/services/pull/merge.go +++ b/services/pull/merge.go @@ -559,7 +559,7 @@ func CheckPRReadyToMerge(pr *models.PullRequest) (err error) { } } - if enoughApprovals := pr.ProtectedBranch.HasEnoughApprovals(pr); !enoughApprovals { + if enoughApprovals := pr.HasEnoughApprovals(); !enoughApprovals { return models.ErrNotAllowedToMerge{ Reason: "Does not have enough approvals", } diff --git a/templates/repo/issue/list.tmpl b/templates/repo/issue/list.tmpl index 0b618daaa9d65..dd562f3852504 100644 --- a/templates/repo/issue/list.tmpl +++ b/templates/repo/issue/list.tmpl @@ -239,7 +239,18 @@ {{else}} {{$.i18n.Tr .GetLastEventLabelFake $timeStr (.Poster.GetDisplayName | Escape) | Safe}} {{end}} - + {{if .IsPull }}{{if .PullRequest.RequiresApproval }} + {{ $reviewStatus := .PullRequest.GetReviewStatus }} + + {{if eq $reviewStatus.Status "rejected"}} + {{$.i18n.Tr "repo.pulls.review_rejected" $reviewStatus.Count | Safe}} + {{else if eq $reviewStatus.Status "required"}} + {{$.i18n.Tr "repo.pulls.review_required" $reviewStatus.Count | Safe}} + {{else}} + {{$.i18n.Tr "repo.pulls.review_approved" $reviewStatus.Count | Safe}} + {{end}} + + {{end}}{{end}} {{if .Milestone}} {{svg "octicon-milestone" 16}} {{.Milestone.Name}} diff --git a/templates/repo/issue/milestone_issues.tmpl b/templates/repo/issue/milestone_issues.tmpl index 49712b1d09fdb..6be8b20de7fea 100644 --- a/templates/repo/issue/milestone_issues.tmpl +++ b/templates/repo/issue/milestone_issues.tmpl @@ -206,6 +206,18 @@ {{else}} {{$.i18n.Tr .GetLastEventLabelFake $timeStr (.Poster.GetDisplayName|Escape) | Safe}} {{end}} + {{if .IsPull }}{{if .PullRequest.RequiresApproval }} + {{ $reviewStatus := .PullRequest.GetReviewStatus }} + + {{if eq $reviewStatus.Status "rejected"}} + {{$.i18n.Tr "repo.pulls.review_rejected" $reviewStatus.Count | Safe}} + {{else if eq $reviewStatus.Status "required"}} + {{$.i18n.Tr "repo.pulls.review_required" $reviewStatus.Count | Safe}} + {{else}} + {{$.i18n.Tr "repo.pulls.review_approved" $reviewStatus.Count | Safe}} + {{end}} + + {{end}}{{end}} {{if .Ref}} {{svg "octicon-git-branch" 16}} {{.Ref}} diff --git a/templates/user/dashboard/issues.tmpl b/templates/user/dashboard/issues.tmpl index dfb94560e5641..fe4f42d3e048f 100644 --- a/templates/user/dashboard/issues.tmpl +++ b/templates/user/dashboard/issues.tmpl @@ -124,6 +124,18 @@ {{else}} {{$.i18n.Tr .GetLastEventLabelFake $timeStr (.Poster.GetDisplayName|Escape) | Safe}} {{end}} + {{if .IsPull }}{{if .PullRequest.RequiresApproval }} + {{ $reviewStatus := .PullRequest.GetReviewStatus }} + + {{if eq $reviewStatus.Status "rejected"}} + {{$.i18n.Tr "repo.pulls.review_rejected" $reviewStatus.Count | Safe}} + {{else if eq $reviewStatus.Status "required"}} + {{$.i18n.Tr "repo.pulls.review_required" $reviewStatus.Count | Safe}} + {{else}} + {{$.i18n.Tr "repo.pulls.review_approved" $reviewStatus.Count | Safe}} + {{end}} + + {{end}}{{end}} {{if .Milestone}} {{svg "octicon-milestone" 16}} {{.Milestone.Name}}