Show review labels in the pull request lists #9274

oscarcosta · 2019-12-07T01:57:43Z

Show review labels (Review required, Approved and Changes requested) in the pull request lists. Related to issue #8257

… - in the pull request lists. Related to issue go-gitea#8257

codecov-io · 2019-12-07T02:23:52Z

Codecov Report

Merging #9274 into master will increase coverage by 0.16%.
The diff coverage is 40.47%.

@@            Coverage Diff             @@
##           master    #9274      +/-   ##
==========================================
+ Coverage   43.38%   43.54%   +0.16%     
==========================================
  Files         575      583       +8     
  Lines       79683    80296     +613     
==========================================
+ Hits        34567    34967     +400     
- Misses      40832    40991     +159     
- Partials     4284     4338      +54

Impacted Files	Coverage Δ
routers/org/home.go	`65.68% <ø> (-0.34%)`	⬇️
routers/routes/routes.go	`86.57% <ø> (+0.04%)`	⬆️
routers/org/setting.go	`0% <ø> (ø)`	⬆️
models/helper_environment.go	`90% <ø> (ø)`	⬆️
models/migrations/migrations.go	`4.16% <ø> (ø)`	⬆️
routers/api/v1/org/member.go	`16.01% <0%> (ø)`	⬆️
routers/init.go	`63.04% <0%> (-2.13%)`	⬇️
models/migrations/v127.go	`0% <0%> (ø)`
modules/graceful/manager_unix.go	`40.83% <0%> (ø)`	⬆️
cmd/serv.go	`2.84% <0%> (-0.05%)`	⬇️
... and 61 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 64b47d5...01bd93c. Read the comment docs.

bagasme

Related strings for some locales are same as on en-US locale.

lunny · 2019-12-07T05:07:30Z

@oscarcosta If you can show approved count, that's better! I found github didn't show that.

models/pull.go

options/locale/locale_cs-CZ.ini

oscarcosta · 2019-12-07T08:14:53Z

Review labels with counters:

guillep2k · 2019-12-07T14:54:48Z

models/pull.go

+		if pr.ProtectedBranch.GetRejectedReviewsCount(pr) > 0 {
+			return i18n.Tr(lang, "repo.pulls.review_rejected", rejections)
+		}
+		approvals := pr.ProtectedBranch.GetGrantedApprovalsCount(pr)


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.

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.

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

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.

One should approve or request change.

davidsvantesson

In general I think this PR looks good!
One issue is that we haven't implemented blocking PR on changes requested yet (#9130), so the list will say "Changes requested" although it will not block merging. I think it would be easy to implement that after this PR though.
I think we need to decide how it should work:

If changes are requested it will always block merging (no option for it). For me this would be OK and makes sense, though it would be a breaking change.
Have a branch protection option whether requested changes shall block merging. In that case I don't know if it should say "Changes Requested" in the PR list, don't make sense if it isn't blocking?

options/locale/locale_es-ES.ini

routers/user/home.go

templates/repo/issue/list.tmpl

guillep2k · 2019-12-07T23:58:56Z

models/pull.go

+// 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)


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

I've tried to display these counters after the label. Does it seems right?

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.

templates/repo/issue/milestone_issues.tmpl

templates/user/dashboard/issues.tmpl

models/issue.go

This reverts commit d723929

models/branches.go

models/issue_list.go

6543 · 2020-02-02T16:10:30Z

@oscarcosta can you resolve change-requests and conflicts pleace

oscarcosta · 2020-02-02T16:50:11Z

@6543 sure, I can work on that today.

# Conflicts: # models/branches.go # models/pull.go # options/locale/locale_en-US.ini

models/pull.go

…g the i18n from the model.

…tedApprovalsCount from ProtectedBranch to PullRequest.

models/pull.go

lunny · 2020-02-03T07:30:05Z

Two nit things will make this better.

like what @lafrks said return a status. And you could also check if ConflictedFiles length to return conflicted status.
batch load protected branch or other objects on pull_list.go for a performance consideration.

…_in_pull_lists

oscarcosta · 2020-02-12T09:17:47Z

Hi @lunny,
Wouldn't the conflict status be slightly different from the review status? Perhaps a different label in the pull request list.

lunny

I think this is the last one thought from me. Otherwise LGTM.

lunny · 2020-02-13T06:58:32Z

models/pull.go

+}
+
+// GetReviewStatus returns a ReviewStatusCount of this pull request
+func (pr *PullRequest) GetReviewStatus() ReviewStatusCount {


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

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)

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

…_in_pull_lists

lunny · 2020-02-23T07:37:55Z

We just should use select status, count(status) from review where ... and is_offcial=true group by status. Because is_offcial means official reviewers' last review.

oscarcosta · 2020-02-24T05:18:04Z

Hi @lunny,
I tried to load the pull request attributes in batch, calling the methods in the loadAttributes in pull_list.go. However, the PullRequests are loaded as Issues with the isPull attribute setted (see files routers/user/home.go and routers/repo/issue.go), so I called the PullRequest.LoadAttributes in those places.

oscarcosta · 2020-02-24T05:21:15Z

We just should use select status, count(status) from review where ... and is_offcial=true group by status. Because is_offcial means official reviewers' last review.

About this query, would it replace both GetRejectedReviewsCount and GetGrantedApprovalsCount methods in GetReviewStatus?

stale · 2020-04-25T05:03:40Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

6543 · 2020-04-25T07:29:03Z

Whats the status of this?

oscarcosta · 2020-04-25T15:48:42Z

Whats the status of this?

Sorry, but I think I will not be able to finish it. If anyone wants to finish it, go ahead.

6543 · 2020-04-26T14:01:01Z

@oscarcosta I couldn't find the specific pull but its arlready implemented:

Show reviews labels - Review required, Approved and Changes requested…

d9f1bbf

… - in the pull request lists. Related to issue go-gitea#8257

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

bagasme reviewed Dec 7, 2019

View reviewed changes

lunny added the type/enhancement An improvement of existing functionality label Dec 7, 2019

lunny added this to the 1.12.0 milestone Dec 7, 2019

lafriks requested changes Dec 7, 2019

View reviewed changes

models/pull.go Outdated Show resolved Hide resolved

options/locale/locale_cs-CZ.ini Outdated Show resolved Hide resolved

oscarcosta added 3 commits December 6, 2019 22:59

Removing non translated labels

4ff5406

Checking rejected reviews before enough approvals.

46b9e29

Showing number of reviews required, approvals and changes requests

8236a04

oscarcosta requested a review from lafriks December 7, 2019 08:17

guillep2k reviewed Dec 7, 2019

View reviewed changes

davidsvantesson suggested changes Dec 7, 2019

View reviewed changes

options/locale/locale_es-ES.ini Outdated Show resolved Hide resolved

routers/user/home.go Show resolved Hide resolved

oscarcosta added 3 commits December 7, 2019 12:28

Removing translations as they are addressed through Crowdin.

6686bd7

Setting Issue's reference in PullRequest when loading PullRequests.

3761e06

Fix runtime error: invalid memory address or nil pointer dereference.

582d7ef

guillep2k mentioned this pull request Dec 7, 2019

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

Open

17 tasks

guillep2k reviewed Dec 8, 2019

View reviewed changes

oscarcosta added 2 commits December 8, 2019 11:17

Simplifying conditional expressions.

d723929

Revert "Simplifying conditional expressions."

9a6f4a8

This reverts commit d723929

jamesorlakin mentioned this pull request Feb 2, 2020

Show approvals on the PR listing page #8762

Closed

6543 reviewed Feb 2, 2020

View reviewed changes

models/branches.go Outdated Show resolved Hide resolved

6543 reviewed Feb 2, 2020

View reviewed changes

models/issue_list.go Outdated Show resolved Hide resolved

oscarcosta added 3 commits February 2, 2020 18:25

Merge branch 'master' into review_labels_in_pull_lists

f670132

# Conflicts: # models/branches.go # models/pull.go # options/locale/locale_en-US.ini

Merge branch 'master' into review_labels_in_pull_lists

3c16eea

Calling pr.LoadIssue() to load issue info from database.

3f93cfe

6543 reviewed Feb 3, 2020

View reviewed changes

models/pull.go Outdated Show resolved Hide resolved

oscarcosta added 3 commits February 2, 2020 20:29

Getting labels and counters of reviews from two methods, thus removin…

4f792ec

…g the i18n from the model.

Moved methods GetRejectedReviewsCount, HasEnoughApprovals and GetGran…

fbd4c09

…tedApprovalsCount from ProtectedBranch to PullRequest.

Fix comment

64b47d5

lafriks approved these changes Feb 3, 2020

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 3, 2020

lafriks reviewed Feb 3, 2020

View reviewed changes

models/pull.go Outdated Show resolved Hide resolved

zeripath mentioned this pull request Feb 11, 2020

Add Approval Counts to pulls list #10238

Merged

oscarcosta added 2 commits February 11, 2020 23:36

Merge branch 'master' of github.com:go-gitea/gitea into review_labels…

b3ef97e

…_in_pull_lists

Returning status for reviews

01bd93c

lunny requested changes Feb 13, 2020

View reviewed changes

oscarcosta added 2 commits February 22, 2020 20:21

Merge branch 'master' of github.com:go-gitea/gitea into review_labels…

f84ce89

…_in_pull_lists

Merge branch 'master' of github.com:go-gitea/gitea into review_labels…

eea70d5

…_in_pull_lists

oscarcosta added 3 commits February 23, 2020 09:02

Refactor: extracting code that load issues in PulRequestList to a method

6577c95

Batch loading protected branch objects in pull_list.go

d97dc25

Loading pull request attributes

c63d0a9

stale bot added the issue/stale label Apr 25, 2020

stale bot removed the issue/stale label Apr 25, 2020

techknowlogick closed this Apr 26, 2020

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show review labels in the pull request lists #9274

Show review labels in the pull request lists #9274

oscarcosta commented Dec 7, 2019

codecov-io commented Dec 7, 2019 •

edited

Loading

bagasme left a comment

lunny commented Dec 7, 2019

oscarcosta commented Dec 7, 2019

guillep2k Dec 7, 2019

davidsvantesson Dec 7, 2019

guillep2k Dec 7, 2019

lunny Dec 8, 2019

lunny Dec 8, 2019

davidsvantesson left a comment •

edited

Loading

guillep2k Dec 7, 2019

oscarcosta Dec 8, 2019

guillep2k Dec 8, 2019

6543 commented Feb 2, 2020

oscarcosta commented Feb 2, 2020

lunny commented Feb 3, 2020 •

edited

Loading

oscarcosta commented Feb 12, 2020

lunny left a comment

lunny Feb 13, 2020

davidsvantesson Feb 13, 2020

lunny Feb 13, 2020 •

edited

Loading

lunny commented Feb 23, 2020

oscarcosta commented Feb 24, 2020

oscarcosta commented Feb 24, 2020

stale bot commented Apr 25, 2020

6543 commented Apr 25, 2020

oscarcosta commented Apr 25, 2020

6543 commented Apr 26, 2020

Show review labels in the pull request lists #9274

Show review labels in the pull request lists #9274

Conversation

oscarcosta commented Dec 7, 2019

codecov-io commented Dec 7, 2019 • edited Loading

Codecov Report

bagasme left a comment

Choose a reason for hiding this comment

lunny commented Dec 7, 2019

oscarcosta commented Dec 7, 2019

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

davidsvantesson left a comment • edited Loading

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 Feb 2, 2020

oscarcosta commented Feb 2, 2020

lunny commented Feb 3, 2020 • edited Loading

oscarcosta commented Feb 12, 2020

lunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny Feb 13, 2020 • edited Loading

Choose a reason for hiding this comment

lunny commented Feb 23, 2020

oscarcosta commented Feb 24, 2020

oscarcosta commented Feb 24, 2020

stale bot commented Apr 25, 2020

6543 commented Apr 25, 2020

oscarcosta commented Apr 25, 2020

6543 commented Apr 26, 2020

codecov-io commented Dec 7, 2019 •

edited

Loading

davidsvantesson left a comment •

edited

Loading

lunny commented Feb 3, 2020 •

edited

Loading

lunny Feb 13, 2020 •

edited

Loading