Approvals are dismissed when new commits are pushed

[jpraet]

Description

With #25882, if

is set on the branch protection, approvals get dismissed when new commits are added.

I am not sure if it was the actual intent of this branch protection to actively "Dismiss" the review.

Previously, this option already caused stale approvals to be ignored towards the PR approval count, which seems sufficient?

gitea/models/issues/pull.go

Lines 837 to 839 in a50d9af

	if protectBranch.DismissStaleApprovals {
	sess = sess.And("stale = ?", false)
	}

Now, the review is actually dismissed, resulting in an entry in the PR history, and an (empty) email notification.

This dismissed review is also not shown in the "Reviewers" view of the PR, whereas stale reviews are.

In my opinion actually dismissing the review because new commits are added is a bit excessive?

Gitea Version

1.20.4

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

docker

Database

None

[jpraet]

The PR #9532 to mark PR reviews as stale at push and allow to dismiss stale approvals was merged in 2020.
The PR #12674 to add dismiss review feature was merged in 2021.

So I believe the term "dismiss" in the branch protection setting does not actually refer to the real "dismiss review" functionality because that didn't exist yet.

[lunny]

#25882 is to fix #25858 . So maybe the definition of Dismiss stale approvals is no very clear. The previous implementation will not update the dismissed column but stale column and it will be ignored when counting official approvals. But the UI is still not right.

[jpraet]

Yes, in my opinion it would have been better to update the UI description (e.g. "Ignore stale approvals") of this branch protection setting to match the implemented behavior. Instead of updating the implementation to match the description.

[jpraet]

Dismissing the stale approval reviews also has the following unwanted side effects:

• if a review was requested from the author of the stale approval, it does not revert to the "review requested" state
• it is not possible for the PR author to re-request a review from the author of the stale approval

[wderoey]

Agreeing with @jpraet ; simply ignoring a stale approval suffices. Dismissing a review completely when new commits are added is not a behavior you wish to make automatic. So i am pro updating the UI description to "Ignore stale approvals" and reverting the implemented behaviour to update the stale column and ignore the approval when counting official approvals (not updating the dismiss column).

[jpraet]

Would a PR that reverts to the old behavior and clarifies the UI description be accepted?

Or would a PR that adds a new "Ignore stale approvals" branch protection setting be preferred? That way, both options are supported.

[lunny]

I would like the second option.