Bug fix allow users with access to count as official reviewers

[william-allspice]

This Pull Request fixes an issue where you can assign a user with only READ access as a reviewer, but their review does not count towards the minimum required approval count.

You can see an example of this here: https://demo.gitea.com/Witea1/PermissionsTest/pulls/1

Steps to reproduce:

1. Create a new organization
   1a Keep default settings

2. Create a new repository

3. Create a branch protection
   3a. Branch main
   3b. 1 required approval

4. Create a “READ-ONLY” team with only access to read
   4a. Set repository access to ‘All Repositories’

5. Add a non-admin user to the “READ-ONLY” team

6. In the repo you created for step 2, create a new pull request

7. Assign your user from the “READ-ONLY” team as a reviewer

8. Login as the user on the “READ-ONLY” team and approve the design review

9. Notice, you will still have “0 of 1” approvals granted and visible both from the “READ-ONLY” team member and from the admin user who created the design review.

[kdumontnu]

Gitea code-base has the concept of "official reviewer", but how it is counted is pretty inconsistent and confusing the UI.

Before:

• Any user with Pull Request "read" permissions can submit a review
• Only users with CODE "write" permissions are counted towards "official reviewers" and counted towards branch protection. However, nothing explains this to the user, so we are left with the "bug" explained above where we have on the same page one reviewer approved, but "0/1 approvals".

So, what is the purpose of "official reviewers"? Can we simply get rid of this and count all reviews toward approvals?
It looks like the origin goes back to this PR: #9055. I didn't see where it was decided that code "write" permissions is the correct access level, but perhaps there was some discussion of that.

The changes here reduce the requirement for CODE from "write" to "read". Can someone still leave a review with PullRequest "read" permission and no CODE permission? If so, I don't think this PR fully solves the problem.

Further, there seems to be an issue of communication. If a user adds branch-protection with users specifically in allow-list do they see the same thing here? It seems we need to communicate in the PR page why someone is/isn't counting towards reviews. I believe github will either give a "green" check or a "grey" check based on their permissions. Any other ideas how we can simplify & communicate this? I'm always a fan of tooltips.

[yp05327]

I think this is by design. Maybe we need docs to explain it.

[kdumontnu]

I think this is by design. Maybe we need docs to explain it.

What's the intended case for it?

[yp05327]

Approve means I think this PR is ok to be merged, this is opinion, and anyone can convey their opinions.
But the approval count will finally affect Merge, and Merge needs write permission, so if you have no write permission,
it should not be allowed to affect Merge. You can only READ the codes and left your ideas(create PR or comment).

If anyone's approvals should be count, then there's no meaning of branch protection rules.
I can create another account to approve it, e.g. for this PR, I create two normal account, and approve my PR, and all checks are ok, then does this mean this PR can be merged? Of cause, normally, mergers will check it, but logically it is strange.

[kdumontnu]

Approve means I think this PR is ok to be merged, this is opinion, and anyone can convey their opinions. But the approval count will finally affect Merge, and Merge needs write permission, so if you have no write permission, it should not be allowed to affect Merge. You can only READ the codes and left your ideas(create PR or comment).

If anyone's approvals should be count, then there's no meaning of branch protection rules. I can create another account to approve it, e.g. for this PR, I create two normal account, and approve my PR, and all checks are ok, then does this mean this PR can be merged? Of cause, normally, mergers will check it, but logically it is strange.

Thanks for considering and explaining.

It's strange logic to me that we allow someone to review (and get a green checkbox), but then not count them as a "approver". And certainly needs to be explained better in the application (note that the original proposal would change the wording to " 0 of 1 approvals granted from reviewers with write access", but that change was not implemented).

I propose one of the following two options:

• We remove the concept of "official reviewers" logic. Anyone with "read" pull request logic can approve a PR and be counted towards approvers. If a repo owner requires more strict permissions, they can use "Allow-List" function to require a team with higher privileges.
• (my preferred) Also get rid of "official reviewers". We make submitting reviews a PullRequest "write" property. Thus, anyone submitting a review must have "write" permissions for PRs (not code). All reviews then count toward approval count (unless whitelist is set).
  • There was a suggestion by @yp05327 to allow users with Pull Request read permissions to comment, but not approve/reject. I think that's a great idea.

Either way, I think we need to change the UI to indicate the following:

• If a review does not count towards approvals, they should get a grey check instead of green
• A tooltip for the checkboxes should indicate that
• "This pull request does not have enough approvals yet. X of Y approvals granted." Should become "This pull request does not have enough required approvals yet. X of Y approvals granted (from whitelisted users)." (paren should only be include if whitelist is set).
• We should change "whitelist" to "allowlist" to be consistent with the rest of the application (this is out of scope to the original issue, but while we're here..)

I believe we cannot expect all users to go read the documentation for typical use cases and anything that can be explained in the app at the time of need should be.

What do you think @yp05327

[william-allspice]

@kdumontnu The changes you outlined to make the UI more intuitive are clear and make sense. I'll get working on those enhancements.

[yp05327]

I partly agree with your suggestions.

It's strange logic to me that we allow someone to review (and get a green checkbox), but then not count them as a "approver".

And

If a review does not count towards approvals, they should get a grey check instead of green

And

A tooltip for the checkboxes should indicate that

I agree. And maybe the colors can be changed for different roles?

I propose one of two better options:

If we don't implement the second one, for the first one only, then maybe it is acceptable.
But if we implement both pf them, it is strange.
For now, review has many cases:

• a comment /a set of comments
• approve with a comment / a set of comments / no comments
• require changes with a comment / a set of comments / no comments

If users with read permission can not review, then how about left a simple comment (not a review) ?

"This pull request does not have enough approvals yet. X of Y approvals granted." Should become "This pull request does not have enough required approvals yet. X of Y approvals granted (from whitelisted users)." (paren should only be include if whitelist is set).

I agree. These contents should be changes, but not only in this place, but also in the branch rules settings page.

We should change "whitelist" to "allowlist" to be consistent with the rest of the application

Any more details about this change? Is it only rename, or we need to change the logic?

Summary:
I agree with adjusting the descriptions and different roles with different check color.
But I would not suggest users without write permission will be counted for merge.
And this is not a block, if there are two approves, this PR can be merged. So feel free to commit your codes.

Maybe we need @go-gitea/technical-oversight-committee 's advices.

[wolfogre]

I believe there's also something like "official reviewer" on GitHub (no idea how GitHub defines that), and GitHub has a better UI with that. (It's not advice, but a reference in case someone is not aware of it)

See:

The PR has been merged with only one approval? (I can see the number of approvals may be because my browser has installed the GitHub UI enhancement plugin, but this is not important. I guess I can also get the number of approvals which is one here through the API.)

No, there were two. But one of them has a gray icon, and GitHub didn't count it.

The PR got lgtm/done from the bot, which is reading the MAINTAINERS file.

[wolfogre]

Anyway, I'll block this PR until other TOC agree to break the current design.

[kdumontnu]

@yp05327 @wolfogre Thanks, I think we agree for the most part. To be clear: I only suggest one of those two options, not both. Both involve getting rid of "official reviewer".

I think the concept of "Official Reviewer" is really confusing when you also consider all of the repo units. Part of the reason I was so confused is that there are Pull Request "Read", Pull Request "Write", but Official Review requires repo "Write" in addition (otherwise you will be silently ignored) 🤯

If all that comes out of this is improving the in-app UI for explaining this then that's a good start - I definitely won't block improvements. However, I feel strongly about abolishing "official reviewers" to simplify permissions (and there are other ways to achieve higher repo protections).

Regarding GitHub - I think many things GitHub does are very confusing 😆. I had no idea about GitHub's official reviewers until now.

[william-allspice]

PR with UI changes opened here: #31924

[lunny]

Why do we need to store the official reviewer in the database someone may not be an official reviewer anymore. He maybe leave one team to another team. But we need to keep the original situation when he approved that PR.

[kdumontnu]

Why do we need to store the official reviewer in the database someone may not be an official reviewer anymore. He maybe leave one team to another team. But we need to keep the original situation when he approved that PR.

Good point. That functionality can remain the same. The change here is just regarding permissions.

[lunny]

Why a person can read the code could approve the code change? At least, it's impossible for open-source projects like Gitea. Also for those internal repositories shared with all persons in a company, it's also not a good practice.

[kdumontnu]

Why a person can read the code could approve the code change? At least, it's impossible for open-source projects like Gitea. Also for those internal repositories shared with all persons in a company, it's also not a good practice.

Can you please read my analysis here? #31886 (comment)

I have explained these points and suggestions moving forward.

[kdumontnu]

Why a person can read the code could approve the code change? At least, it's impossible for open-source projects like Gitea. Also for those internal repositories shared with all persons in a company, it's also not a good practice.

Can you please read my analysis here? #31886 (comment)

I have explained these points and suggestions moving forward.

@lunny since I suspect you will not ready my analysis, I will summarize here:

• Unlike GitHub, we have separate permissions for pull requests
• It is too confusing and unnecessary to combine these permissions (eg. "official reviewer" = "repo write" + "pull request read"
• I suggest we make reviewing a pull request "write" permission (and repo read, which is required to leave a review)
  • What is the point of reviewing if it doesn't count as "official"?
  • One suggestion is that "pull request read" permission is still allowed to comment, but not review
• An alternative is that we remove repo write requirement and users can use allow-list to restrict "official reviewers" if needed

Either of the two solutions should cover the case you mentioned while simplifying permissions.

[lunny]

Pull Request Read is a permission to allow users to visit pull request and create pull request. So Code Write is the only permission for the official reviewer.

[kdumontnu]

pull request read is a permission to allow users to visit pull request and create pull request. So the official reviewer just ask code writer or allowlist of protected branch.

I'm not sure what you're saying here.

[yp05327]

Maybe add an option in repo settings to allow it is the best solution.
It will not broken the current design, and can also meet your needs.

[kdumontnu]

Maybe add an option in repo settings to allow it is the best solution. It will not broken the current design, and can also meet your needs.

It's a good idea and an understandable conclusion, but I would rather not make a change here than add another setting. There are too many settings already & our goal here is to remove one permutation, not add another.

[william-allspice]

Closing this Pull Request.

The project decided that only users and teams with write access should have the ability to count towards the required PR reviews on the protected branches.

We were able to improve the user experience by adding frontend enhancements to allow users to distinguish between approvals that are official and those that are not official. That work was merged in PR 31924.