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 14 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
14 changes: 14 additions & 0 deletions models/branches.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,20 @@ func (protectBranch *ProtectedBranch) GetGrantedApprovalsCount(pr *PullRequest)
return approvals
}

// GetRejectedReviewsCount returns the number of rejected reviews for pr.
func (protectBranch *ProtectedBranch) GetRejectedReviewsCount(pr *PullRequest) int64 {
oscarcosta marked this conversation as resolved.
Show resolved Hide resolved
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
}

// MergeBlockedByRejectedReview returns true if merge is blocked by rejected reviews
func (protectBranch *ProtectedBranch) MergeBlockedByRejectedReview(pr *PullRequest) bool {
if !protectBranch.BlockOnRejectedReviews {
Expand Down
35 changes: 35 additions & 0 deletions models/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import (
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/timeutil"

"github.com/unknwon/i18n"
)

// PullRequestType defines pull request type
Expand Down Expand Up @@ -171,6 +173,39 @@ 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
}

// GetReviewLabel returns the formatted review label with counter for the review of this pull request
func (pr *PullRequest) GetReviewLabel(lang string) string {
if pr.RequiresApproval() {
rejections := pr.ProtectedBranch.GetRejectedReviewsCount(pr)
Copy link
Member

Choose a reason for hiding this comment

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

IMHO we should display # of changes requested and approvals. That would solve the problem of whether the change requests are blocking or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tried to display these counters after the label. Does it seems right?
Screenshot from 2019-12-08 11-22-35

Copy link
Member

Choose a reason for hiding this comment

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

It's incomplete information. The PR can have 17 approvals and 1 change request. Should it show "1 changes requested" or "17 people approved this"? I think both.

if pr.ProtectedBranch.GetRejectedReviewsCount(pr) > 0 {
oscarcosta marked this conversation as resolved.
Show resolved Hide resolved
return i18n.Tr(lang, "repo.pulls.review_rejected", rejections)
oscarcosta marked this conversation as resolved.
Show resolved Hide resolved
}
approvals := pr.ProtectedBranch.GetGrantedApprovalsCount(pr)
Copy link
Member

Choose a reason for hiding this comment

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

I think that a PR that has been repeatedly approved and rejected from one reviewer should count only one approval or one rejection, depending on which happened last:

  • Reviewer 1: approves, requests changes = 0 approvals, 1 change requests
  • Reviewer 2: approves = 1 approval, 0 change requests
  • Reviewer 3: requests changes, requests changes, requests changes, approves = 1 approvals, 0 change requests
  • Reviewer 4: requests changes = 0 approvals, 1 change requests

Grand total: 2 approvals, 2 change requests

To achieve that, IMHO all counts (approvals, rejections) should be processed in the same database query. Check this comment (and other reviews of mine in that PR) for examples on how to achieve that.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that is already achieved. Both GetGrantedApprovalsCount and GetRejectedReviewsCount only counts reviews that are official, and official reviews are only the latest review (approval or rejection) of users that are in the whitelist.

Copy link
Member

Choose a reason for hiding this comment

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

Duh, I forgot about the official property! I had a rough week... I need some rest. 😄

Copy link
Member

Choose a reason for hiding this comment

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

We could use different color tag for approved counter and requested counter. Even the approval requirement. 1/2 means this PR require 2 approvals but has 1 approved.

Copy link
Member

Choose a reason for hiding this comment

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

One should approve or request change.

if approvals >= pr.ProtectedBranch.RequiredApprovals {
return i18n.Tr(lang, "repo.pulls.review_approved", approvals)
}
return i18n.Tr(lang, "repo.pulls.review_required", pr.ProtectedBranch.RequiredApprovals-approvals)
}
return i18n.Tr(lang, "repo.pulls.review_approved", 0) // by default
}

// GetDefaultMergeMessage returns default message used when merging pull request
func (pr *PullRequest) GetDefaultMergeMessage() string {
if pr.HeadRepo == nil {
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 @@ -1089,6 +1089,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: 3 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,9 @@
{{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
<a class="milestone" href="{{$.Link}}/{{.Index}}">{{.PullRequest.GetReviewLabel $.i18n.Lang | Safe}}</a>
{{end}}{{end}}
{{if .Milestone}}
<a class="milestone" href="{{$.Link}}?q={{$.Keyword}}&type={{$.ViewType}}&state={{$.State}}&labels={{$.SelectLabels}}&milestone={{.Milestone.ID}}&assignee={{$.AssigneeID}}">
<span class="octicon octicon-milestone"></span> {{.Milestone.Name}}
Expand Down
3 changes: 3 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,9 @@
{{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
<a class="milestone" href="{{$.RepoLink}}/pulls/{{.Index}}">{{.PullRequest.GetReviewLabel $.i18n.Lang | Safe}}</a>
{{end}}{{end}}
{{if .Ref}}
<a class="ref" href="{{$.RepoLink}}/src/branch/{{.Ref}}">
<span class="octicon octicon-git-branch"></span> {{.Ref}}
Expand Down
3 changes: 3 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,9 @@
{{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
<a class="milestone" href="{{AppSubUrl}}/{{.Repo.Owner.Name}}/{{.Repo.Name}}/pulls/{{.Index}}">{{.PullRequest.GetReviewLabel $.i18n.Lang | Safe}}</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}}">
<span class="octicon octicon-milestone"></span> {{.Milestone.Name}}
Expand Down