Issue #6565 open

Need a "reject" as well as "approve" (BB-7750)

Ben Buchanan
created an issue

Reviewers are complaining there's no "reject" button on PRs.

Need to support a flow where a reviewer can easily show they don't want a PR, noting they must be able to update that later.

Possibly, all review/reject marks need to be reset after a PR is updated, too.

Comments (17)

  1. ZyX_I

    This seems logical. I would note though that resetting mark is not the only option. I see the following behaviors that may be good regarding the issue:

    1. Approval/rejection marks are reset on update (already suggested).
    2. Approval mark blocks PR from being updated. It is not logical to make rejection mark block though, thus 1. or 3. should be used in this case.
    3. Attach approval/rejection mark to the current tip of the PR in place of the whole PR. If changesets are added approval/rejection mark remain, but people with permissions to merge would see that they are attached to an outdated changeset.
  2. Caleb Shoemaker

    I think that Pull Requests would be much more useful if the Decline action could be undone at all, which currently doesn't seem to be the case. I agree that Approval/Declines should be removed if additional commits are pushed to the branch under review and the PR is updated to include them.

    Currently, both the Approve (aka, "like") and Decline (aka, "banish") buttons are practically useless.

  3. Ben Buchanan reporter

    Looking back in ... I've seen this same idea/feedback come up with basically everyone who looks at PRs (including Atlassian teams ;)). Thought I could expand a little...

    Essentially people want the option as a reviewer to indicate they feel the PR is not suitable/ready for merge. This could be the addition of a "reject" or "not approved" button; or the mechanism could be treated as Upvote/Downvote. In some cases, a reviewer wishes to register their disapproval against the general opinion on the PR - so the PR will be merged, not rejected; but it gives that person the option to show they disagreed (sounds harsh but it does happen). However it is far more common that someone just wants to indicate they've come in, they've read, they've commented, they don't think it's ready yet but they have no way to indicate their feedback is finished.

    "Decline" is only appropriate for the PR's author or the project maintainer; and also it's only for a total rejection, not an indication that someone disagrees with other reviewers or simply isn't ready to approve a PR (eg. when they've given feedback and are waiting to see if the author updates the PR).

  4. Erik van Zijst staff

    As of yesterday there's a bit of a workaround to this. That is, reviewers can now raise tasks on a PR. Tasks can be checked off and provide a list of things reviewers want to see addressed.

    While it's not the same as explicitly disapproving, it does provide for a record for raising issues that have not been met or that the reviewer feels are required.

    tasks.png

  5. Ben Buchanan reporter

    Awesome! If there was an option to approve pending tasks, it'd be pretty much ideal.

    So the flow would be: 1) comment, raise a task 2) approve pending tasks raised, ie approval doesn't 3) author updates, ticks them off 4) when all ticked off, author's approval shows up

    That is the "wannapony" version, but this is still awesome.

  6. Mike Gruen

    I agree that adding a simple "hold" button would be nice. Reject seems akin to Decline. Hold implies there is still work or an open question and that the PR shouldn't be merged until the "hold" is removed.

  7. Log in to comment