search

isaacs / github

Just a place to track issues and feature requests that I have for github
2.2k stars 129 forks source link

Automatic dismissal of reviews via push is misrepresented in UI #1157

Open aspiers opened 6 years ago

aspiers commented 6 years ago

If the new feature as described in https://github.com/isaacs/github/issues/783#issuecomment-320211882 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 commented 6 years ago

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

SebastianKristof commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

@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 https://github.com/isaacs/github/issues/783#issuecomment-320211882

ruimcf commented 5 years ago

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

sanyukt2505 commented 4 years ago

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?

Screen Shot 2019-11-21 at 7 58 55 AM

tn3rb commented 4 years ago

@sanyukt2505

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

Here's how it works

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:

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

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 commented 3 years ago

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:

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 2 years ago

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?