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.

Sign up for GitHub

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

Jump to bottom

Show review labels in the pull request lists #9274

Closed
wants to merge 24 commits into from
Closed

Show review labels in the pull request lists #9274

Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
d9f1bbf
Show reviews labels - Review required, Approved and Changes requested…
oscarcosta Dec 7, 2019
4ff5406
Removing non translated labels
oscarcosta Dec 7, 2019
46b9e29
Checking rejected reviews before enough approvals.
oscarcosta Dec 7, 2019
8236a04
Showing number of reviews required, approvals and changes requests
oscarcosta Dec 7, 2019
6686bd7
Removing translations as they are addressed through Crowdin.
oscarcosta Dec 7, 2019
3761e06
Setting Issue's reference in PullRequest when loading PullRequests.
oscarcosta Dec 7, 2019
582d7ef
Fix runtime error: invalid memory address or nil pointer dereference.
oscarcosta Dec 7, 2019
d723929
Simplifying conditional expressions.
oscarcosta Dec 8, 2019
9a6f4a8
Revert "Simplifying conditional expressions."
oscarcosta Dec 8, 2019
f670132
Merge branch 'master' into review_labels_in_pull_lists
oscarcosta Feb 3, 2020
3c16eea
Merge branch 'master' into review_labels_in_pull_lists
oscarcosta Feb 3, 2020
3f93cfe
Calling pr.LoadIssue() to load issue info from database.
oscarcosta Feb 3, 2020
d3657fd
suggestions
6543 Feb 3, 2020
deca4a8
Merge pull request #1 from 6543/pull_9274
oscarcosta Feb 3, 2020
4f792ec
Getting labels and counters of reviews from two methods, thus removin…
oscarcosta Feb 3, 2020
fbd4c09
Moved methods GetRejectedReviewsCount, HasEnoughApprovals and GetGran…
oscarcosta Feb 3, 2020
64b47d5
Fix comment
oscarcosta Feb 3, 2020
b3ef97e
Merge branch 'master' of github.com:go-gitea/gitea into review_labels…
oscarcosta Feb 12, 2020
01bd93c
Returning status for reviews
oscarcosta Feb 12, 2020
f84ce89
Merge branch 'master' of github.com:go-gitea/gitea into review_labels…
oscarcosta Feb 23, 2020
eea70d5
Merge branch 'master' of github.com:go-gitea/gitea into review_labels…
oscarcosta Feb 23, 2020
6577c95
Refactor: extracting code that load issues in PulRequestList to a method
oscarcosta Feb 23, 2020
d97dc25
Batch loading protected branch objects in pull_list.go
oscarcosta Feb 23, 2020
c63d0a9
Loading pull request attributes
oscarcosta Feb 24, 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
25 changes: 0 additions & 25 deletions models/branches.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
78 changes: 78 additions & 0 deletions models/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,84 @@ func (pr *PullRequest) loadProtectedBranch(e Engine) (err error) {
return
}

func (pr *PullRequest) requiredApprovals() int64 {
if pr.ProtectedBranch == nil {
if err := pr.loadProtectedBranch(x); err != nil {
log.Error("Error loading ProtectedBranch", err)
}
}
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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If someone sent a rejected review and after that send another approval review. We should only count 1 approval but 0 rejected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is in the official column which is accounted for, only last review of a reviewer can be official. (The reviewer shall also be in the review whitelist)

Copy link
Member

@lunny lunny Feb 13, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

official means this review could be counted as approvals but it don't mean it's the last review of the official reviewer. See https://github.com/go-gitea/gitea/blob/master/models/review.go#L57-L58

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 {
Expand Down
2 changes: 1 addition & 1 deletion models/pull_sign.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
3 changes: 3 additions & 0 deletions options/locale/locale_en-US.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1094,6 +1094,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 <span class="ui white small label">%d</span>
pulls.review_approved = Approved <span class="ui white small label">%d</span>
pulls.review_rejected = Changes requested <span class="ui white small label">%d</span>

milestones.new = New Milestone
milestones.open_tab = %d Open
Expand Down
4 changes: 2 additions & 2 deletions routers/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion services/pull/merge.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -550,7 +550,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",
}
Expand Down
13 changes: 12 additions & 1 deletion templates/repo/issue/list.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,18 @@
{{else}}
{{$.i18n.Tr .GetLastEventLabelFake $timeStr (.Poster.GetDisplayName | Escape) | Safe}}
{{end}}

{{if .IsPull }}{{if .PullRequest.RequiresApproval }}
lafriks marked this conversation as resolved.
Show resolved Hide resolved
{{ $reviewStatus := .PullRequest.GetReviewStatus }}
<a class="milestone" href="{{$.Link}}/{{.Index}}">
{{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}}
</a>
{{end}}{{end}}
{{if .Milestone}}
<a class="milestone" href="{{$.Link}}?q={{$.Keyword}}&type={{$.ViewType}}&state={{$.State}}&labels={{$.SelectLabels}}&milestone={{.Milestone.ID}}&assignee={{$.AssigneeID}}">
{{svg "octicon-milestone" 16}} {{.Milestone.Name}}
Expand Down
12 changes: 12 additions & 0 deletions templates/repo/issue/milestone_issues.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,18 @@
{{else}}
{{$.i18n.Tr .GetLastEventLabelFake $timeStr (.Poster.GetDisplayName|Escape) | Safe}}
{{end}}
{{if .IsPull }}{{if .PullRequest.RequiresApproval }}
lafriks marked this conversation as resolved.
Show resolved Hide resolved
{{ $reviewStatus := .PullRequest.GetReviewStatus }}
<a class="milestone" href="{{$.RepoLink}}/pulls/{{.Index}}">
{{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}}
</a>
{{end}}{{end}}
{{if .Ref}}
<a class="ref" href="{{$.RepoLink}}/src/branch/{{.Ref}}">
{{svg "octicon-git-branch" 16}} {{.Ref}}
Expand Down
12 changes: 12 additions & 0 deletions templates/user/dashboard/issues.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,18 @@
{{else}}
{{$.i18n.Tr .GetLastEventLabelFake $timeStr (.Poster.GetDisplayName|Escape) | Safe}}
{{end}}
{{if .IsPull }}{{if .PullRequest.RequiresApproval }}
lafriks marked this conversation as resolved.
Show resolved Hide resolved
{{ $reviewStatus := .PullRequest.GetReviewStatus }}
<a class="milestone" href="{{AppSubUrl}}/{{.Repo.Owner.Name}}/{{.Repo.Name}}/pulls/{{.Index}}">
{{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}}
</a>
{{end}}{{end}}
{{if .Milestone}}
<a class="milestone" href="{{AppSubUrl}}/{{.Repo.Owner.Name}}/{{.Repo.Name}}{{$.Link}}?q={{$.Keyword}}&type={{$.ViewType}}&state={{$.State}}&labels={{$.SelectLabels}}&milestone={{.Milestone.ID}}&assignee={{$.AssigneeID}}">
{{svg "octicon-milestone" 16}} {{.Milestone.Name}}
Expand Down