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.

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

Add Approval Counts to pulls list #10238

Merged
merged 8 commits into from
Mar 6, 2020

Conversation

zeripath
Copy link
Contributor

Add simple approvals counts to pulls lists

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

@zeripath zeripath added the topic/ui Change the appearance of the Gitea UI label Feb 11, 2020
@zeripath zeripath added this to the 1.12.0 milestone Feb 11, 2020
@zeripath zeripath added the pr/wip This PR is not ready for review label Feb 11, 2020
@codecov-io
Copy link

codecov-io commented Feb 11, 2020

Codecov Report

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

Impacted file tree graph

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

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 11, 2020
@jaqra
Copy link
Contributor

jaqra commented Feb 13, 2020

Should we add this to milestone_issues.tmpl ?

Add simple approvals counts to pulls lists
@zeripath zeripath force-pushed the add-review-labels-to-pull-lists branch from 579edf1 to cf2e82e Compare February 17, 2020 13:54
@zeripath zeripath changed the title WIP: Add Approval Counts to pulls list Add Approval Counts to pulls list Feb 17, 2020
@zeripath zeripath removed the pr/wip This PR is not ready for review label Feb 17, 2020
@zeripath
Copy link
Contributor Author

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
Copy link
Member

lunny commented Feb 18, 2020

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

@guillep2k
Copy link
Member

@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
Copy link
Member

lunny commented Feb 19, 2020

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

@guillep2k
Copy link
Member

guillep2k commented Feb 19, 2020

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
Copy link
Member

lunny commented Feb 19, 2020

@guillep2k Only official counts is reasonable.

@jamesorlakin
Copy link
Contributor

@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
Copy link
Member

@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
Copy link
Member

lunny commented Feb 29, 2020

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

@zeripath
Copy link
Contributor Author

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

@zeripath
Copy link
Contributor Author

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.

@GiteaBot 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 29, 2020
@zeripath
Copy link
Contributor Author

zeripath commented Mar 1, 2020

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
Copy link
Contributor Author

zeripath commented Mar 2, 2020

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
Copy link
Member

lunny commented Mar 6, 2020

Just found #10624

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Mar 6, 2020
@zeripath
Copy link
Contributor Author

zeripath commented Mar 6, 2020

Make lg-tm work

@zeripath zeripath merged commit 80db442 into go-gitea:master Mar 6, 2020
@zeripath zeripath deleted the add-review-labels-to-pull-lists branch March 6, 2020 03:44
a1012112796 added a commit to a1012112796/gitea that referenced this pull request Apr 5, 2020
…ists also

change icon for review requests to eye
lafriks added a commit that referenced this pull request Apr 6, 2020
…10756)

* add request review feature in pull request

add a way to notify specific reviewers to review like github , by add  or delet a  special type
review . The acton is  is similar to Assign ,  so many code reuse the function and items of
Assignee, but the meaning and result is different.

The Permission style is is similar to github, that only writer can add a review request from Reviewers,
but the poster can recall and remove a review request after a reviwer has revied even if he don't have
Write Premission. only manager , the poster and reviewer of a request review can remove it.

The reviewers can be requested to review contain all readers for private repo , for public, contain
all writers and watchers.

The offical Review Request will block merge if Reject can block it.

an other change: add ui otify for Assignees.

Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>
Co-authored-by: Lauris BH <lauris@nix.lv>

Signed-off-by: a1012112796 <1012112796@qq.com>

* new change

* add placeholder string

* do some changes follow #10238 to add review requests num on lists also
change icon for review requests to eye

Co-authored-by: Lauris BH <lauris@nix.lv>
ydelafollye pushed a commit to ydelafollye/gitea that referenced this pull request Jul 31, 2020
…o-gitea#10756)

* add request review feature in pull request

add a way to notify specific reviewers to review like github , by add  or delet a  special type
review . The acton is  is similar to Assign ,  so many code reuse the function and items of
Assignee, but the meaning and result is different.

The Permission style is is similar to github, that only writer can add a review request from Reviewers,
but the poster can recall and remove a review request after a reviwer has revied even if he don't have
Write Premission. only manager , the poster and reviewer of a request review can remove it.

The reviewers can be requested to review contain all readers for private repo , for public, contain
all writers and watchers.

The offical Review Request will block merge if Reject can block it.

an other change: add ui otify for Assignees.

Co-authored-by: guillep2k <18600385+guillep2k@users.noreply.github.com>
Co-authored-by: Lauris BH <lauris@nix.lv>

Signed-off-by: a1012112796 <1012112796@qq.com>

* new change

* add placeholder string

* do some changes follow go-gitea#10238 to add review requests num on lists also
change icon for review requests to eye

Co-authored-by: Lauris BH <lauris@nix.lv>
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants