Misleading reason, why merging a PR is blocked

[ckittl]

• Gitea version (or commit ref): 1.12.1
• Can you reproduce the bug at https://try.gitea.io:
  • Yes (https://try.gitea.io/ckittl/PRMergeBlockedMessage/pulls/1)
  • No

Sorry, cannot answer the following information, as I'm not hosting the Gitea instance myself. AFAIK, it doesn't play a role either.

• Git version:
• Operating system:
• Database
  • PostgreSQL
  • MySQL
  • MSSQL
  • SQLite
• Log gist:

Description

With #9592, blocking merging a pull request was enabled, if one reviewer requested changes on the code. However, with ef89e75 (regarding #10756) the checks got extended to also cover the case if one explicitly requested reviewer hasn't given any comments yet. I feel a bit confused about this in two ways.

Think of the following test case (cf. also the test repo):

• Globally I require at least 1 review
• In a specific PR I ask two people to review
• One hasn't answered, yet. The other has approved without any change request

The information, why merging is blocked is misleading

As far as I was able to get from your code, the reason, why a pull request merge is blocked, is determined here:

gitea/routers/repo/issue.go

Lines 1435 to 1445 in 1bb5c09

	if pull.ProtectedBranch != nil {
	cnt := pull.ProtectedBranch.GetGrantedApprovalsCount(pull)
	ctx.Data["IsBlockedByApprovals"] = !pull.ProtectedBranch.HasEnoughApprovals(pull)
	ctx.Data["IsBlockedByRejection"] = pull.ProtectedBranch.MergeBlockedByRejectedReview(pull)
	ctx.Data["IsBlockedByOutdatedBranch"] = pull.ProtectedBranch.MergeBlockedByOutdatedBranch(pull)
	ctx.Data["GrantedApprovals"] = cnt
	ctx.Data["RequireSigned"] = pull.ProtectedBranch.RequireSignedCommits
	ctx.Data["ChangedProtectedFiles"] = pull.ChangedProtectedFiles
	ctx.Data["IsBlockedByChangedProtectedFiles"] = len(pull.ChangedProtectedFiles) != 0
	ctx.Data["ChangedProtectedFilesNum"] = len(pull.ChangedProtectedFiles)
	}

However, obviously the condition is true also, if nobody requested changes, but still one of the explicitly requested reviewers didn't answer yet. This leads to the misleading prompt, that requested changes not have been addressed, yet.

My suggestion would be to spin of Branch#MergeBlockedByExplcitReviewRequest from Branch#MergeBlockedByRejectedReview and have both conditions separated.

The explicit review request override my global config

As with GitHub, I would expect, that everything is fine to be merged, as long as one reviewer has replied with approval. However, current implementation blocks the merge with no need. Despite me not finding it handy to require all requested reviews, at least it is not clear, that this is the case. Maybe a renaming of the label for review requests from "Reviewers" to "Mandatory Reviewers" would point out clearly, what it's about (cf. Screenshot).

Screenshots

[davidsvantesson]

It would be better to only add a warning "Not all requested reviewers have reviewed.", but still allow merge. As it was anyhow the writer of the PR that made the review requests.

[6543]

I think this is an expected behaviour ...

and repoAdmins + requested reviwer can dismiss request to review if needed.

[6543]

@ckittl you you propose to allow a review-request-creator to dismis those requests he had maid?

[ckittl]

@ckittl you you propose to allow a review-request-creator to dismis those requests he had maid?

Thank you for your consideration, @6543! I think, revoking a review request is still possible, yet. I don't really bother about the pull request being blocked. If this is intended behaviour, I'm fine with it. It is just that I didn't expect this behaviour, as it is different for famous github. I would more wish for a clarification, by e.g. renaming the headline shown in the screenshot I made.

[wxiaoguang]

Outdated, the code has changed a lot