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 approvals on the PR listing page #8762

Closed
wants to merge 3 commits into from
Closed

Show approvals on the PR listing page #8762

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a25f336
Show approvals on the PR list page
Oct 31, 2019
43b3e7d
Remove extra property on ProtectedBranch for PR approvals
Nov 1, 2019
ade2ffe
Merge branch 'master' into PRListApprovals
jamesorlakin Nov 1, 2019
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
23 changes: 12 additions & 11 deletions models/branches.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,17 +31,18 @@ type ProtectedBranch struct {
BranchName string `xorm:"UNIQUE(s)"`
CanPush bool `xorm:"NOT NULL DEFAULT false"`
EnableWhitelist bool
WhitelistUserIDs []int64 `xorm:"JSON TEXT"`
WhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"`
WhitelistDeployKeys bool `xorm:"NOT NULL DEFAULT false"`
MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"`
StatusCheckContexts []string `xorm:"JSON TEXT"`
ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"`
WhitelistUserIDs []int64 `xorm:"JSON TEXT"`
WhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
EnableMergeWhitelist bool `xorm:"NOT NULL DEFAULT false"`
WhitelistDeployKeys bool `xorm:"NOT NULL DEFAULT false"`
MergeWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
MergeWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
EnableStatusCheck bool `xorm:"NOT NULL DEFAULT false"`
StatusCheckContexts []string `xorm:"JSON TEXT"`
ApprovalsWhitelistUserIDs []int64 `xorm:"JSON TEXT"`
ApprovalsWhitelistTeamIDs []int64 `xorm:"JSON TEXT"`
RequiredApprovals int64 `xorm:"NOT NULL DEFAULT 0"`
GrantedApprovalsCount int64
Copy link
Member

Choose a reason for hiding this comment

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

This should not be added column so database so should be marked so:

Suggested change
GrantedApprovalsCount int64
GrantedApprovalsCount int64 `xorm:"-"`

Copy link
Member

Choose a reason for hiding this comment

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

granted aprovals Should be stored for each PR and not for each branch?!

Copy link
Member

Choose a reason for hiding this comment

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

And the information Iis stored already somewhere ... have to check where ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah, so any properties I add are mapped to the database by default?

Copy link
Member

Choose a reason for hiding this comment

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

you can get the information via models.GetUniqueApprovalsByPullRequestID(prID int64) ([]*Review, error)

Copy link
Member

Choose a reason for hiding this comment

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

summary:

  • no changes in models/branches.go
  • call models.GetUniqueApprovalsByPullRequestID(prID int64) in routers/repo/issue.go and store in ctx.Data["GrantedApprovalsCount"]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

call models.GetUniqueApprovalsByPullRequestID(prID int64) in routers/repo/issue.go and store in ctx.Data["GrantedApprovalsCount"]

How would I reference that per PR, seeing as I have an array of them? Issues get popped under ctx.Data["Issues"] as a whole so I created a property to be able to access it from each one within the template. I presume something dynamic like ctx.Data["Issues"][i].GrantedApprovals = count isn't possible - I come from a JS background 😉.

The reason I left it as a property of ProtectedBranch was simply because GetGrantedApprovalsCount lived there already, and I just assumed I would keep them together.

Copy link
Member

Choose a reason for hiding this comment

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

no

Copy link
Member

Choose a reason for hiding this comment

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

I can look into it later ...

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'm attempting to change it to a solution similar to ctx.Data["CommitStatus"] and removing the extra property on ProtectedBranch.

Looking at it, GetUniqueApprovalsByPullRequestID will return all approved reviews for a given PR, not just those on the approval whitelist. This might be equally valid - what would the user reasonably expect to see? Personally I think only reviews "which matter" should count to quickly determine if a PR is ready to be merged.

CreatedUnix timeutil.TimeStamp `xorm:"created"`
UpdatedUnix timeutil.TimeStamp `xorm:"updated"`
}
Expand Down
9 changes: 8 additions & 1 deletion routers/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -232,8 +232,15 @@ func issues(ctx *context.Context, milestoneID int64, isPullOption util.OptionalB
ctx.ServerError("LoadPullRequest", err)
return
}
pull := issues[i].PullRequest

commitStatus[issues[i].PullRequest.ID], _ = issues[i].PullRequest.GetLastCommitStatus()
if err := pull.LoadProtectedBranch(); err == nil {
Copy link
Member

Choose a reason for hiding this comment

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

You're not handling the error here. Please do something like

if err := pull.LoadProtectedBranch(); err != nil {
    return
}

if pull.ProtectedBranch != nil && pull.ProtectedBranch.RequiredApprovals != 0 {
    ProtectedBranch.GrantedApprovalsCount = pull.ProtectedBranch.GetGrantedApprovalsCount(
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah ok, is there a subtle difference in the behaviour of errors and nil?

Copy link
Member

Choose a reason for hiding this comment

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

If the function returns nil for an error, there was no error. You don't check for the case where the error was not nil which means you're never actually doing something with the error.

In Go, there is this pattern of keeping a "happy path" of code, that's why you'll see a lot of if err != nil code used to handle errors. Take a look at this article for more information: https://medium.com/@matryer/line-of-sight-in-code-186dd7cdea88

if pull.ProtectedBranch != nil && pull.ProtectedBranch.RequiredApprovals != 0 {
pull.ProtectedBranch.GrantedApprovalsCount = pull.ProtectedBranch.GetGrantedApprovalsCount(pull)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Set value in slice also when branch is not protected? (with 0 value)


commitStatus[pull.ID], _ = pull.GetLastCommitStatus()
}
}

Expand Down
8 changes: 8 additions & 0 deletions templates/repo/issue/list.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,14 @@
<a class="ui label has-emoji" href="{{$.Link}}?q={{$.Keyword}}&type={{$.ViewType}}&state={{$.State}}&labels={{.ID}}&milestone={{$.MilestoneID}}&assignee={{$.AssigneeID}}" style="color: {{.ForegroundColor}}; background-color: {{.Color}}" title="{{.Description}}">{{.Name}}</a>
{{end}}

{{if .IsPull}}
Copy link
Member

Choose a reason for hiding this comment

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

this can be reduced to one if statement:

Suggested change
{{if .IsPull}}
{{if and .IsPull .PullRequest.ProtectedBranch .PullRequest.ProtectedBranch.RequiredApprovals}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Aah that's much simpler, will do

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately this didn't work for branches without protection enabled

Copy link
Member

Choose a reason for hiding this comment

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

{{if and .IsPull (if ne (index $.RequiredApprovals .PullRequest.ID) 0)}} ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I suppose that requires that $.RequiredApprovals contain all indexes? Should be possible by setting them 0 in issue.go.

{{if .PullRequest.ProtectedBranch}}
{{if .PullRequest.ProtectedBranch.RequiredApprovals}}
<span class="comment ui right"><i class="octicon octicon-eye"></i> {{.PullRequest.ProtectedBranch.GrantedApprovalsCount}} / {{.PullRequest.ProtectedBranch.RequiredApprovals}}</span>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<span class="comment ui right"><i class="octicon octicon-eye"></i> {{.PullRequest.ProtectedBranch.GrantedApprovalsCount}} / {{.PullRequest.ProtectedBranch.RequiredApprovals}}</span>
<span class="comment ui right"><i class="octicon octicon-eye"></i> {{.PullRequest.GrantedApprovalsCount}} / {{.PullRequest.ProtectedBranch.RequiredApprovals}}</span>

Continue things .. from abouth

{{end}}
{{end}}
{{end}}

{{if .NumComments}}
<span class="comment ui right"><i class="octicon octicon-comment"></i> {{.NumComments}}</span>
{{end}}
Expand Down