1. Bitbucket
  2. Public Issue Tracker
  3. master
  4. Issues

Issues

Issue #7667 duplicate

Further commits on a pull request should remove any prior approvals (BB-6784)

ocharles
created an issue

In short, it is very hard to manage a large amount of pull requests with multiple reviewers. For example, User A might approve a pull request, while user B might have concerns. User C updates their pull request with a new commit. At this point User A should have to review again, but at the moment their approval is retained.

Comments (43)

  1. Jon

    I'd be in favor of making this an opt-in (or at least opt back to the current way if auto-removal is the default behavior) setting at the repo/account level. I think it's probably the right thing to do in 99% of the cases, but some peoples' workflows may be built around the current functionality.

    Also, it would be nice to retain the old approvals so you can see them in the activity tab (oh, Bob approved it, then it got changed, etc.) and just have the final/effective status of the PR be 'unapproved' for that reviewer. I think it would be frustrating to wonder 'did I ever approve it at any point or am I imagining things?' if they just disappear.

  2. Jon

    Also, it'd be nice to see unapprovals as an 'event' in the activity chronology, but that's likely a separate feature (although it's probably in the neighborhood of my previous comment).

  3. Jon

    Does anyone else's workflow involve lots of "oh, I better IM the other reviewer(s) to let them know that the PR they previously approved has moved so they know they need to go back and click the unapprove button until they have a chance to review the commits that happened after their approval"? I find we're constantly doing that (and not always consistently or in a timely manner).

  4. Dmitry Belyaev

    There should be no separate state "pull request is accepted", it must be calculated as "are all the commits approved?" The button "approve pull request" should approve all the commits under the pull request.

  5. Sébastien Gautrin

    Anand Kumria and thus not implemented ;)

    From what I gather, there's really nothing in common (code wise) between stash and bitbucket (hence why no mercurial support in stash), so the fact Atlassian added it to stash does probably not reduce much the workload for adding it to bitbucket (though it might reduces the functional conception part if they choose to have it behave the same way).

    Jon while having the overall approval of a PR be linked to approval of each commit seems to be a nice thing, it might not be enough all the times: you can indeed actually remove commits from PRs (strip it from origin repo then update the PR), which would break that workflow, as someone who would have approved the PR before removal of the last commit would still be considered having approved the PR; he would indeed have approved all commits in the PR, but the PR when approved was different than after the strip.

    Commit approval within a PR (i.e. approving the commit as being part of the PR, not approving the commit separately as is possible today) could be a nice thing to have, but user should still have to manually re-approve.

  6. Dmitry Belyaev

    This is really annoying. Yesterday a team member submitted a pull request, it was approved and then he updated it and merged as if it the amendments were also approved. We're in a dire need of this feature.

  7. Brian Contario

    For those who do not know, there is an Atlassian plugin for installable versions of Bitbucket, but not the cloud version.

    See https://jira.atlassian.com/browse/BSERV-2918 This functionality was added to the Atlassian Labs Auto-Unapprove plugin on marketplace and is not included in Stash's core offering. Please install that plugin if you'd like to automatically remove the approval of any reviewers on a push to the source branch.

    This has been available since 2013/03 !!!! Why are cloud users being punished???

  8. Log in to comment