Show approvals on the PR listing page #8762

jamesorlakin · 2019-10-31T09:12:04Z

As suggested in #8659, this PR displays the whitelisted approvals in Gitea's PR listing page if a given branch is protected and requires more than 0 approvals to be merged. I've chosen the eye Octicon but perhaps a person one would be better?

I've tested this works locally in the browser but as I'm on a Windows environment Docker doesn't work properly for the full test suite 😢 . This is also my first time using Go, so all suggestions (nitpicking) are most welcome!

codecov-io · 2019-10-31T09:31:34Z

Codecov Report

Merging #8762 into master will decrease coverage by <.01%.
The diff coverage is 46.15%.

@@            Coverage Diff             @@
##           master    #8762      +/-   ##
==========================================
- Coverage   41.39%   41.38%   -0.01%     
==========================================
  Files         539      539              
  Lines       69501    69514      +13     
==========================================
- Hits        28770    28769       -1     
- Misses      37032    37051      +19     
+ Partials     3699     3694       -5

Impacted Files	Coverage Δ
routers/repo/issue.go	`34.48% <46.15%> (+0.11%)`	⬆️
modules/indexer/indexer.go	`44.73% <0%> (-10.53%)`	⬇️
models/unit.go	`62.16% <0%> (-5.41%)`	⬇️
modules/task/migrate.go	`24.35% <0%> (-3.85%)`	⬇️
models/error.go	`32.08% <0%> (-1.25%)`	⬇️
modules/log/event.go	`64.61% <0%> (-1.03%)`	⬇️
models/pull.go	`54.33% <0%> (-0.59%)`	⬇️
models/repo.go	`48.62% <0%> (+0.05%)`	⬆️
modules/migrations/gitea.go	`11.77% <0%> (+1.49%)`	⬆️
modules/migrations/migrate.go	`22.9% <0%> (+1.67%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ebcc381...ade2ffe. Read the comment docs.

lafriks · 2019-10-31T10:07:47Z

models/branches.go

+	ApprovalsWhitelistUserIDs []int64  `xorm:"JSON TEXT"`
+	ApprovalsWhitelistTeamIDs []int64  `xorm:"JSON TEXT"`
+	RequiredApprovals         int64    `xorm:"NOT NULL DEFAULT 0"`
+	GrantedApprovalsCount     int64


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

Suggested change

GrantedApprovalsCount int64

GrantedApprovalsCount int64 `xorm:"-"`

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

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

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

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

summary:

no changes in models/branches.go

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

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.

I can look into it later ...

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.

lafriks · 2019-10-31T10:12:45Z

templates/repo/issue/list.tmpl

@@ -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}}


this can be reduced to one if statement:

Suggested change

{{if .IsPull}}

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

Aah that's much simpler, will do

Unfortunately this didn't work for branches without protection enabled

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

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

6543

Beside this ... nice idear

6543 · 2019-10-31T13:08:00Z

models/branches.go

+	ApprovalsWhitelistUserIDs []int64  `xorm:"JSON TEXT"`
+	ApprovalsWhitelistTeamIDs []int64  `xorm:"JSON TEXT"`
+	RequiredApprovals         int64    `xorm:"NOT NULL DEFAULT 0"`
+	GrantedApprovalsCount     int64


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

6543 · 2019-10-31T13:09:36Z

models/branches.go

+	ApprovalsWhitelistUserIDs []int64  `xorm:"JSON TEXT"`
+	ApprovalsWhitelistTeamIDs []int64  `xorm:"JSON TEXT"`
+	RequiredApprovals         int64    `xorm:"NOT NULL DEFAULT 0"`
+	GrantedApprovalsCount     int64


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

6543 · 2019-10-31T13:12:15Z

templates/repo/issue/list.tmpl

+					{{if .IsPull}}
+						{{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>


Suggested change

 {{.PullRequest.ProtectedBranch.GrantedApprovalsCount}} / {{.PullRequest.ProtectedBranch.RequiredApprovals}}

 {{.PullRequest.GrantedApprovalsCount}} / {{.PullRequest.ProtectedBranch.RequiredApprovals}}

Continue things .. from abouth

6543 · 2019-10-31T14:08:42Z

@jamesorlakin look at #8762 (comment) ;)

lunny · 2019-10-31T15:57:51Z

I think you guys should consider the page load performance. Protected branches could be load once. Or I should send another PR to do that after this one.

kolaente · 2019-10-31T19:16:14Z

routers/repo/issue.go


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


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( }

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

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

jamesorlakin · 2019-11-01T12:12:35Z

Hi all,

Thanks for the comments. I've reworked it to now use a method similar to the last commit status - a map by PR ID for both required approvals and approved accepted ones.

6543 mentioned using GetUniqueApprovalsByPullRequestID. This would surely count all reviews on a PR, not just ones whitelisted needed for the PR to be merged? This raised a good point in my mind though - should the user see all approvals, or just ones relevant to the branch protection?

lunny, you're right, I didn't think about performance! 3 PRs rendered quick enough on my laptop... A to-do (assuming this approach looks okay) would be to cache protected branches during the loop - say a page had 25 PRs all for the same target branch, we would only need to load the ProtectedBranch once leaving just a call to GetGrantedApprovalsCount for each one.

guillep2k · 2019-11-01T13:42:09Z

The number of protected branches in a given repository should not be too large (3? 6? 10?). In my experience, it's more performant to load all of them at once rather than reaching for them one by one. Databases do that themselves: for example, for tables under a certain number of rows, indexes are never used for querying (only for updating); the database considers that reading one disk block is the "expensive" part; sorting/filtering is quickly done in memory. Context/process switching from Gitea to and from the database is costly as well.

guillep2k · 2019-11-01T13:55:27Z

For the number of approvals, I'd use a single database query, which is much faster than retrieving the information each time:

Collect all the issues/prs to show in the page using models.Issues() as normal, so pagination and other options are taken into account.
In a slice (array), collect all the ids for rows where .IsPull is true and .IsClosed is false.
If the number of ids collected is not zero, make one single query to the database counting approvals for those ids (using group by and count); load that into a map. For that, a method must be created in models/issue.go, where all database accesses should happen. That method should use sess.GroupBy("issue.id").Select("issue.id AS id, COUNT(*) AS count"); see CountIssuesByRepo() for an example.
Then that map can be used to display the info in the page.
Alternatively, the approval count can be moved into models.Issues() as well.

Hope this helps.

6543 · 2019-11-02T21:13:40Z

templates/repo/issue/list.tmpl

@@ -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}}


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

jamesorlakin · 2019-11-03T21:38:43Z

Thanks for the suggestions @guillep2k! Unfortunately I'm not sure I'm capable at dropping to such complex SQL (joining and comparing whitelisted approvers including teams, etc). That said, loading all protected branches at once initially could be better optimised than the individual looping - I'll try and do that.

ids for rows where .IsPull is true and .IsClosed is false.

I would be under the assumption closed PRs should show the same as open ones?

Moving forward, I'm also considering showing total approvals (not whitelisted) for PRs that aren't on a protected branch. This would just be a simple number next to the eye. If possible I'll want to grab all approvals for the PRs at once, and then do the whitelist filtering using a util function (similar to how GetGrantedApprovalsCount works, but without the query) - this may not go to plan...

I've since backported this to our production 1.9 instance and performance seems to be fine - though it's not a great representative measurement!

6543 · 2019-11-06T23:18:45Z

@jamesorlakin are you still working on this?

jamesorlakin · 2019-11-07T12:28:29Z

@6543 yep I'm keen to finish it, but I likely won't have much time until the weekend. Feel free to tweak if you want to in the meantime 😃 .

davidsvantesson · 2019-12-04T22:23:53Z

routers/repo/issue.go

+			if pull.ProtectedBranch != nil && pull.ProtectedBranch.RequiredApprovals != 0 {
+				requiredApprovals[pull.ID] = pull.ProtectedBranch.RequiredApprovals
+				approvals[pull.ID] = pull.ProtectedBranch.GetGrantedApprovalsCount(pull)
+			}


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

6543 · 2019-12-17T16:01:58Z

@jamesorlakin what is the stats?

6543 · 2020-01-16T01:39:52Z

@jamesorlakin ping

jamesorlakin · 2020-02-02T15:18:37Z

Hi all,

Apologies for the general radio silence - everything has been a tad busy of recent and it all generally got forgotten about 😥. In the meantime I've had a look around and #9274 seems to be a generally more polished implementation if you would like to go ahead with that. I don't mind continuing on if @oscarcosta doesn't want to.

Show approvals on the PR list page

a25f336

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 31, 2019

lafriks requested changes Oct 31, 2019

View reviewed changes

lafriks added the type/enhancement An improvement of existing functionality label Oct 31, 2019

lafriks added this to the 1.11.0 milestone Oct 31, 2019

6543 requested changes Oct 31, 2019

View reviewed changes

guillep2k mentioned this pull request Oct 31, 2019

Functionality that is useful for Gitea's administrative workflow #8659

Open

17 tasks

kolaente suggested changes Oct 31, 2019

View reviewed changes

Remove extra property on ProtectedBranch for PR approvals

43b3e7d

Merge branch 'master' into PRListApprovals

ade2ffe

6543 requested changes Nov 2, 2019

View reviewed changes

davidsvantesson reviewed Dec 4, 2019

View reviewed changes

lunny modified the milestones: 1.11.0, 1.12.0 Dec 28, 2019

jamesorlakin closed this Feb 2, 2020

lafriks removed this from the 1.12.0 milestone Feb 2, 2020

go-gitea locked and limited conversation to collaborators Nov 24, 2020

	GrantedApprovalsCount int64
	GrantedApprovalsCount int64 `xorm:"-"`

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

	<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>


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

Show approvals on the PR listing page #8762

Show approvals on the PR listing page #8762

Conversation

jamesorlakin commented Oct 31, 2019 • edited Loading

codecov-io commented Oct 31, 2019 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6543 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

6543 commented Oct 31, 2019

lunny commented Oct 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesorlakin commented Nov 1, 2019

guillep2k commented Nov 1, 2019

guillep2k commented Nov 1, 2019

Choose a reason for hiding this comment

jamesorlakin commented Nov 3, 2019

6543 commented Nov 6, 2019 • edited Loading

jamesorlakin commented Nov 7, 2019

Choose a reason for hiding this comment

6543 commented Dec 17, 2019

6543 commented Jan 16, 2020

jamesorlakin commented Feb 2, 2020 • edited Loading

jamesorlakin commented Oct 31, 2019 •

edited

Loading

codecov-io commented Oct 31, 2019 •

edited

Loading

6543 commented Nov 6, 2019 •

edited

Loading

jamesorlakin commented Feb 2, 2020 •

edited

Loading