Add Approval Counts to pulls list

[zeripath]

Add simple approvals counts to pulls lists

As #9274 appears to have slightly stalled this PR adds simple approvals counts to the pull lists.

[codecov-io]

Codecov Report

Merging #10238 into master will increase coverage by 0.02%.
The diff coverage is 34.78%.

@@            Coverage Diff             @@
##           master   #10238      +/-   ##
==========================================
+ Coverage   43.66%   43.69%   +0.02%     
==========================================
  Files         587      587              
  Lines       82183    82252      +69     
==========================================
+ Hits        35886    35940      +54     
- Misses      41843    41861      +18     
+ Partials     4454     4451       -3

Impacted Files	Coverage Δ
models/pull.go	42.85% <0%> (-0.49%)	⬇️
routers/repo/issue.go	37.91% <31.57%> (-0.09%)	⬇️
routers/user/home.go	54.18% <31.57%> (-0.72%)	⬇️
models/issue_list.go	65.77% <48%> (-1.15%)	⬇️
services/pull/check.go	50% <0%> (-3.05%)	⬇️
modules/git/command.go	86.95% <0%> (-2.61%)	⬇️
models/gpg_key.go	53.95% <0%> (+0.52%)	⬆️
modules/git/repo.go	47.7% <0%> (+0.91%)	⬆️
modules/log/event.go	65.64% <0%> (+1.02%)	⬆️
services/pull/pull.go	35.88% <0%> (+1.17%)	⬆️
... and 7 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 3502730...6440ac3. Read the comment docs.

[jaqra]

Should we add this to milestone_issues.tmpl ?

[zeripath]

OK I think I'm gonna stop where I am. Realistically for simplicity we should have a column on the pulls which is set to whether they're ready to merge or not - but this is better than the current situation.

@jaqra I'm not sure about the milestones page, conflicting files are not listed on there at present so I didn't copy this.

[lunny]

The problem is still there when a non-official reviewer approved at first but then rejected. @zeripath

[guillep2k]

@lunny as per #9055 only one "official" review per user is counted; approvals and rejections are mutually exclusive.

gitea/models/review.go

Lines 290 to 299 in 9f1f2e6

	if reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject {
	// Only reviewers latest review of type approve and reject shall count as "official", so existing reviews needs to be cleared
	if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, doer.ID); err != nil {
	return nil, nil, err
	}
	official, err = isOfficialReviewer(sess, issue, doer)
	if err != nil {
	return nil, nil, err
	}
	}

[lunny]

@guillep2k But that only for official reviewer. For non-official reviewer, it always keeps false.

[guillep2k]

For non-official reviewer, it always keeps false.

Now I see what you mean. Yes, the counts will be off like this. Maybe if only official counts are displayed?

[lunny]

@guillep2k Only official counts is reasonable.

[jamesorlakin]

@jaqra I'm not sure about the milestones page, conflicting files are not listed on there at present so I didn't copy this.

I confess the reason it's not there is because I forgot about that page! 😀

[guillep2k]

@zeripath because of limitations in the way unofficial reviews are accounted, this PR will be displaying wrong information about them at its current state. If you could limit this PR as to only care about official reviews, then it would be proper; unofficial reviews could be fixed another time.

Details

There are "official" reviewers and there are "official" reviews. Unfortunately the word "official" is used differently in each case. An "official" review is the latest review from a whitelisted user. Any other review is "unofficial", no matter who posted it.

Currently, the following happens when a review is added:

• If the user is not whitelisted, the review is marked "unofficial".
• If the user is whitelisted, any previous reviews from that user are changed to "unofficial". The new review is inserted as "official".

So, the "official" pool of reviews has exactly one approval or rejection from each reviewing whitelisted user. The "unofficial" pool contains all the other reviews, even outdated, mixing up indiscriminatedly approvals and rejections from all users, including whitelisted.

[lunny]

Thanks @guillep2k to clarify. That's really what I mean. :(

[zeripath]

We should be replacing the approval/rejection for non-official reviewers at the time of approval. I'll hide the non-official counts.

[zeripath]

I predict that within 1 week of this being merged without display of non-official reviewers we will get bug-reports stating that their reviews are not being shown.

[zeripath]

OK so it looks like the bulk of templates/repo/issue/milestones_issues.tmpl should not exist as it is essentially a copy of templates/repo/issue/list.tmpl. Further there's an issue with this list in that it doesn't easily differentiate between issues and PRs.

[zeripath]

OK I've updated the milestones page - however this needs better thought as it mixes Issues and PRs in one list too. - That can be done as another PR I think

[lunny]

Just found #10624

[zeripath]

Make lg-tm work