Automatic dismissal of reviews via push is misrepresented in UI

[aspiers]

If the new feature as described in #783 (comment) is enabled to automatically dismiss code reviews on protected branches when new code is pushed to that branch, whenever there is a push, the UI will say something like

aspiers dismissed nicolasbock’s stale review via 0aa1b69 5 minutes ago

However this is misleading because it makes it look like I went into the PR myself and pressed the Dismiss review button, when in fact all I did is a git push -f. On one occasion I even came close to getting annoyed with a colleague because I mistakenly assumed they had manually dismissed my review without any explanation, when in fact they had just pushed a new version to respond to my feedback!

This should be fixed.

[aspiers]

@cirosantilli Please could the code review label be added to this?

[SebastianKristof]

For me this happened after I clicked the "Update branch" button on Github to merge master into my branch. Very uncomfortable to look like I went in and dismissed a review of a senior colleague.

[ruimcf]

This is especially annoying when a colleague leaves an approved review, maybe leaving a few minor comments that have small impact, or I just want to do a rebase to squash some commits. The new push will remove the approved review and I need to ask my colleague to leave a review again.

Can this dismiss be configurable?

[tiago]

@ruimcf As someone who rebases on a regular basis, I share your annoyance.
Yes, it can be disabled: Settings ▶ Branches ▶ Branch protection rules ▶ Edit ▶ Uncheck "Dismiss stale pull request approvals when new commits are pushed".

See #783 (comment)

[ruimcf]

@tiago always helpful as usual
good year to you 🎉 💩 🦄

[sanyukt2505]

Does this option also removes all comments that other team members left on the PR ? The description of this feature says that the approval will be removed. There is no mention of what happens to the comments. Is it like PR will get reset/ like a fresh start, with every push. ?

Is there a way to only remove the approval from the PR, retaining all the past comments?

[tn3rb]

@sanyukt2505

If you have that option checked, then any pushes will invalidate any existing code review approvals.

Here's how it works

• PR submitted by user
• PR assigned to peer for code review
• peer approves code review
• user pushes new commit

this is where things change depending on whether that option is checked or not

IF the Dismiss stale pull request approvals when new commits are pushed option IS checked:

• the new commits invalide the existing code review approval, therefore the PR is no longer approved
• since the review approval is required to merge, the PR can no longer be merged until a new code review approval is obtained

IF the Dismiss stale pull request approvals when new commits are pushed option is NOT checked:

• the new commits DO NOT invalide the existing code review approval, therefore the PR is STILL approved
• the PR can be merged

There is no mention of what happens to the comments.

That's because nothing happens to the comments, or anything else! Not sure how you even interpreted it that way.
Is that a typo maybe? Did you mean to write "commits"??? Even so, it doesn't say anything about removing commits.
Those all stay as they are. The only thing that happens if you make any additional commits, is the PR will go from having one or more approved reviews to having none.

Is it like PR will get reset/ like a fresh start, with every push. ?

no, only the approval gets reset (removed)

Is there a way to only remove the approval from the PR, retaining all the past comments?

yup, just check off the Dismiss stale pull request approvals when new commits are pushed option

[blackliner]

Is there a way to keep the approvals on non-substantial commits? For example a rebase, triggered manually or automatically (on a merge to master from another PR)?
Bitbucket can differentiate:

• https://jira.atlassian.com/browse/BSERV-7673?error=login_required&error_description=Login+required&state=3cbdfc9f-b8b4-4370-8e15-5c7dae0ee00c
• https://marketplace.atlassian.com/apps/1211449/auto-unapprove-for-bitbucket-server?hosting=server&tab=overview

This is the description from Atlassian:

This is now addressed in versions 2.2.0 and 3.0.0 of Bitbucket Server Auto Unapprove Plugin.
Clean merges, rebases, squashes, and amends no longer cause approvals to drop. 
Dirty merges (e.g., conflicting resolving merges or evil merges) continue to cause unapprovals to drop.

[joehorsnell]

Is there a way to keep the approvals on non-substantial commits? For example a rebase, triggered manually or automatically (on a merge to master from another PR)?

Yes, exactly this @blackliner - this would be extremely useful. The policy-bot app seems to have some support for ignoring "update merges", but I've not tried it out so can't comment on how reliable it is. Also, not ideal as you have to run that app yourself, so a built-in GitHub feature would be highly preferable.

[jd]

You could use the the dismiss_reviews action from Mergify to be e.g. not triggered on certain pull requests and only dismiss reviews on some PR.

That means you could use e.g. a label no-dismiss on certain PR so to be sure that the approval is kept. You can also do this for only certain authors from a team.

[chrisribe]

I would love having the option of only dismissing the pull request if the changes where in the code and NOT in the code comments.

Happens a lot where you update minor code comments (punctuation etc) and you need to re-ask for code approvals.
Something like minor edit/commit (comments only)

[ChrisUnityArto]

I agree that this is an issue but it's not the functionality that's a problem. In my company, we would like pushes to require new reviews, however it is the wording of these messages that is the issue. When you respond to a PR code review and implement changes and push, you are not 'dismissing' anything, and the review is not 'stale'. You are taking on board valid comments and responding positively.

The OP here expresses this well, though it seems a number of comments above are about tangential features, such as the ability to not 'dismiss' reviews for certain PRs or for merge commits. These are indeed nice features, but they don't address the original issue which seems to be a simple request for more friendly wording in this messaging.

One suggestion:

aspiers responded to nicolasbock’s review by pushing this commit 5 minutes ago

[tommcdo]

I just noticed this myself and found this issue, thanks @ChrisUnityArto for reining it back in to the topic of focus.

To add another use case: Sometimes I just remembered on my own to add something, and I push a new commit after someone already approved. So it's not really responding to the review, but it does render the review stale. I think it would be suitable to use a wording that's a bit more general, without making the committer sound dismissive:

aspiers pushed 0aa1b69 5 minutes ago. nicolasbock's review has been marked stale.

[aspiers]

I agree that this is an issue but it's not the functionality that's a problem. In my company, we would like pushes to require new reviews, however it is the wording of these messages that is the issue. When you respond to a PR code review and implement changes and push, you are not 'dismissing' anything, and the review is not 'stale'. You are taking on board valid comments and responding positively.

The OP here expresses this well, though it seems a number of comments above are about tangential features, such as the ability to not 'dismiss' reviews for certain PRs or for merge commits. These are indeed nice features, but they don't address the original issue which seems to be a simple request for more friendly wording in this messaging.

Precisely.

To add another use case: Sometimes I just remembered on my own to add something, and I push a new commit after someone already approved. So it's not really responding to the review, but it does render the review stale.

Yes, I was about to say just the same. It cannot be assumed that the push is always a response to the review.

I think it would be suitable to use a wording that's a bit more general, without making the committer sound dismissive:

aspiers pushed 0aa1b69 5 minutes ago. nicolasbock's review has been marked stale.

That sounds perfect to me.

[will-molloy]

I noticed the "Update branch" button (when using the other branch protection setting Require branches to be up to date before merging) no longer dismisses reviews, when was this changed?