This should be a setting in the product, not an add-on. As Sukumar above notes, it's an extra hassle/bunch of process etc to get an add-on installed in any actual corporation, and it's a trivial change.

Of course, listening to your customers/users isn't super high on your capabilities list. At least as not as high as writing haikus is.

Thanks so much, Zafer! Let me know if there's anything I can do on our instance to help debug

Do you know if the plugin is intended to work on v6.4.0?

Hi Zafer - thanks so much for publishing the free add-on. That functionality is exactly what we're looking for.

We had previously used this plugin when we were on v5.8.0: **https://marketplace.atlassian.com/apps/1218855/needs-work-merge-check-for-bitbucket/version-history

After upgrading to 6.4.0, however, we needed the same functionality and the above plugin does not support v6+.

So we tried out yours, but I'm seeing errors whenever I enable it on a repository.

I can't currently figure out how to upload screenshots, but I'm seeing "Repository hook com.kodgemisi.plugins.bitbucket.prevent-when-needs-work:needs-work-merge-hook failed" when I click the Merge button to see why it's disabled

In other cases, I see simply "java.lang.NullPointerException"

Why do I need to chase my admin to install an add-on for this very basic functionality.

All we're asking for is the option to prevent merges when someone marks a PR w/ Needs Work - via a merge check.  This is not a complicated ask and should not be a stretch!

Hi all,

we've created and published a free add-on to solve these problem. We also optionally remove approval if developer updates PR.

You can find add-on on the link below:

https://marketplace.atlassian.com/apps/1219882/prevent-merge-when-needs-work?hosting=server&tab=overview

Best Regards,

Zafer Cakmak

Hi,

Our Control Freak add-on can block PR's with "needs-work" reviews.  It's pretty flexible:  you can choose which branches to apply this policy against, and you can ignore drive-by "needs-work" reviews (from read-only users - especially handy for public repositories where anyone in your entire company could mark something "needs-work" even if they have nothing to do with the repository).

In addition, our PR-Booster add-on (paid) is able to retain "needs-work" reviews whenever a pull-request (PR) receives new pushes.

By combining these two add-ons you can setup a pretty solid policy.  Basically makes your pull-request process behave a lot closer to Gerrit, with "needs work" becoming the equivalent of a -2 review in Gerrit.

Here's some screenshots:

Control-Freak config screen

PR-Booster config screen

Regards,

Sylvie Davies
Founder and Software Engineer, bit-booster.com

Svante,

I recently discovered this suggestion when we ran into a similar workflow problem. Have you had a chance you publish the source code? I haven't seen it anywhere. 

John

Yes, I will publish the code shortly.

Thank you Svante! Are you going to publish the source code too?

Regards

Michal

Hey all,

While waiting for this feature to reach Bitbucket I have published a simple free plugin providing this functionality: https://marketplace.atlassian.com/plugins/com.pineant.bitbucket.needs-work-check/server/overview

It is compatible with Bitbucket 5.X+

Go ahead and give it a try! 

Cheers,

// Svante

@Svante, Do you have a sample code which implements this feature? I am trying to find out the state of the PR programatically. I am not sure how we can find out if the PR is marked as 'needs work'.

Sorry I just saw the answer in Svante's comment.

Is there a plugin/add-on available for this? or do we need to make our own add-on?

If you follow the recipe above you will be able to configure this per repo/project.

the Workzone plugin (awesome plugin btw) provides this feature on a global level.

I did the same thing, it was not hard to create, but I think something like this should not be a plugin, but native.

It is a breeze adding this yourself. After my last comment I searched in the Atlassian Developer resources and found this recipe: https://developer.atlassian.com/server/bitbucket/tutorials-and-examples/controlling-when-pull-requests-can-be-merged/ It took me less than 2 hrs to create a simple plugin (with quite moderate java knowledge) that adds the Merge Check blocker. This includes the Project/Repo scope and can be configured just like the native checks.

Try it out! It is worth it!

Thnx for the great docu in this area, Atlassian!!!

I think this is a very basic feature that is expected to be there. Github handles this very elegantly and correctly. If a PR is marked as "Needs work" then the right behavior would be to block the merge of that PR until that user marks it as approved. OR if the admin merges it (of course, with a warning or confirmation popup).

I would think that the Review status Needs work should be shown before Approved in the Approver list in the PR. That simple change would probably resolve most cases when there is a Needs work status among all the Approved

The thing that un-approves the PR on new pushes can probably stay as it is. In my opinion the new push has changed everything and the PR needs to be re-reviewed, including by the user that set the Needs work status in the first place.

But a more elegant soultion would be if the Needs work status is not cleared by the push, it needs to be manually changed to Approved after the push, preferably by the user that set the status in the the first place.

We are using Workzone for extended PR control and this plugin needs to be updated too for this use-case

its probably better to add a category to branch permissions "Allow merge regardless of Needs Work status", and let the admin specify who those are. We in our company like to give this to a special "Integrator" / "Release Manager" role.

We are also confused by the syntax of "Needs work" when you can't enforce the need.  I do think there would need to be a way for an admin to override PRs marked as Needs Work because someone might mark it as Needs Work and block the PR.  If an admin can merge regardless that would be best case scenario.  If a hard block is not possible, at the very least sort the list of reviewers so that the ones that marked it as "Needs Work" are displayed without clicking on the drop-down.  Adding a warning before the merge is also better than what we have now.

How is this not a feature? What if someone sees a fatal flaw and needs to block but the other two missed. Gerrit has loads more functionality than this. For example trusted contributors can add a +2 not just an approved and you can have voted amounts rather than just approved or not. Gerrit is demolishing Bitbucket in terms of reviewing functionality!

Tell me your story?

If the latter is required (hard block), why? Is there .... (a) strong team desire for strict workflows, fear of things slipping through, significant delays to development pipeline when it actually slips through, dysfunction with people ignoring team conventions

Yes.

@rogerbarnes to give more input in response to your questions:

• Happens rarely, because we know about this limitation and communicate offline to get around it. But we'd prefer not to work around this limitation.
• We have had situations where we've needed to get around the PR controls (e.g. when our build server died) but what we've done in the past is ask a repo admin to disable those controls temporarily in order to allow the merge. This has the advantage that a (probably) relatively senior person is involved in the conversation and they can help ensure that the merge really is necessary.
• A hard block might be required to prevent those kind of users who just keep clicking 'ok' until they see a success message. There are plenty of them out there; they're not malicious, but just popup-message-weary and have a habit of ignoring those kind of messages and just clicking through.

I do think it comes back to the principle of least surprise and making sure the 'needs work' button does what people expect. However, if a big "this needs work, are you really sure you want to merge" banner appeared when you hit merge, then this would still be a big step forwards and would very much reduce the need for a hard block. However the 'needs work' flag would probably need to persist despite new changes being pushed.

Thanks cosmotic, that's useful context. To be clear, it's not that we don't see the benefit, it's that the relative benefit needs to be traded off against other priority improvements we might work on, and the side-effects of any change to the overall base. While the product can be somewhat opinionated in places, our intention isn't to force workflows, indeed the extensibility of Bitbucket means it would be possible to write an add-on to produce the desired behaviour - a merge check could veto merges where the PR has outstanding "needs work" statuses. As an aside, a push to the source branch resets such statuses, so there are ways around any protection one might put in place (unless you also use the add-on that resets approvals).

Anyway, this is back on our radars for consideration, but to set expectations, lots of things are.

I just looked into our repo, there are 5 total PRs that were merged despite a 'needs work' status, about 1.5 per month. The Bitbucket UI confusingly prioritizes approvals in the sort of reviewers on the list of PRs. It also frustratingly folds most reviewers into a dropdown. Because of this, only 1 of the 5 even shows without clicking every single drop-down or doing some fancy jQuery work. I also discovered the aria-hidden attribute is improperly set on 1457 elements:

jQuery('.badge.badge-hidden[aria-hidden=false]')

I know of one time, just today, that it happened. Fortunately I caught it and was able to resolve the issue. The bigger problem are the instances I don't catch, which there is no way to know about. All this time I thought I was safe by 'locking' the PR with a 'needs work'.

I hear your concerns about negative side effects. I see those as easily solved with permissions. This 'needs work' block should be optional as it is and project leads can turn on the lock if they see fit. Once on, project leads should be able to bypass the lock, although there should definitely be an in-your-face message that the lock is being bypassed and that someone still thinks it needs work.

Our team uses PRs as a check or a gate before code merges into the main code branches. We enforce this using Bitbucket blocks on direct commits to develop, stable, and master. We are on an extremely tight timeline and even then, I don't see this being of any negative impact on our team.

I would prefer to make the workflow rules as a team instead of being forced into them because Atlassian doesn't see the benefit. As it stands, I just realized a protection I thought our team had in place was not protecting us at all due to Atlassian's poor UX. If there's a status "Needs work" then that should block merge; if you don't believe so, please consult a dictionary on the word 'need'. If you guys think so strongly that it shouldn't block merge that you refuse to add this lock, then you should change that status to "I think this could be better" or something else that does not imply the merge block.

Obviously the machinery is there if others are implementing this feature as plugins.

cosmotic, it seems the 2 add-ons I used to refer to are no longer maintained, which is clearly not ideal. I'll re-open this for consideration.

Some thing I'm curious about:

• How often does important "needs work" feedback slip through the cracks? What happens when it does?
• There are often negative side-effects to team velocity if things are locked down too tightly, and sometimes valid reasons to need to merge ASAP despite reviewer status. Would a more visible merge-time warning as opposed to blocking the merge satisfy your use cases, or would a hard block be the only suitable approach? 
• If the latter is required (hard block), why? Is there an external compliance control, strong team desire for strict workflows, fear of things slipping through, significant delays to development pipeline when it actually slips through, dysfunction with people ignoring team conventions, and/or some other reason?

We wrote our own plugin for this.

There is minimal benefit to a 'needs work' flag if it can just be ignored. I just searched for a plugin to resolve this for my team and I could not find one. This just bit my team: two JR developers approved a PR and the lead marked it as 'needs work'; the author merged anyway, likely because they didn't see the 'needs work' flag. The "need" in "needs work" doesn't hold much weight if it doesn't block the merge.

At the very least, please link to the plugins that implement this feature. 

Someone mentioned there were some addons that could allow this behaviour, so it would be useful to add some links to those addons in this ticket.

Just in case it's useful, my use case goes like this:

• We often invite several people to reviews (maybe 3 or 4) to get input from several sources.
• Not all of those reviewers will feel comfortable approving the review as a whole, but we still want them to be involved - so turning on "requires all reviewers to approve" doesn't work for us.
• However if one of them marks it as "needs work", then we wouldn't expect the feature to be merged.

We find the default behaviour a bit surprising - if someone has marked the PR as 'needs work' then they have a good reason, so it would be very unlikely that you'd want to merge the PR without talking to them and adressing their concerns. Being able to prevent the merge prevents you making a mistake and enforces the process.

Merge conflicts are highlighted very visibly - I think at a minimum the 'needs work' flag needs to be just as visible.

A merge-time "warning" is definitely useful or at least making the “needs works“ status more prominent, maybe similar highlighted as a merge conflict.

We bounced the idea around the team one last time and agreed that this is not something that would be broadly valuable to add to the core product. As discussed in previous comments there are add-ons that do enforce a block type of behaviour if this is the desire.

I do think there is an unresolved use case here though, and I'd be happy to reconsider and reopen this if it finds support - the idea is that a merge-time "warning" as opposed to a merge veto could be useful to remind users that there is unresolved state on the pull request that they might consider before proceeding. Thoughts?

mfriedrich, we do like to support differing philosophies where we can, and our products provide extensibility for that purpose. Indeed, "a push also clears all approvals" is not bundled functionality. Perhaps that add-on could be extended to provide additional protection in a similar vein.

I'll leave this in triage for now and mull it over some more with the team. To help spur that conversation, would you mind elaborating on the underlying need? Is there an external or internal compliance need, a development-team driven desire (or indeed active pain), or perhaps something else? 

@Norman: But a push also clears all approvals. There is no risk in providing such a block, especially if combined with "Requires x approvers".

It provides additional protection from accidentally merging in case you have many users approved and one as "needs work". Please keep in mind that the PR-overview sometimes hides the one user with "needs works" behind a .../+ icon.

@Roger our philosophy is different from yours, supporting different philosophies (by selectable options) should be the goal for Atlassian.

chrisalbrecht, any thoughts on Norman's previous comment? In short, our philosophy is to require approvals rather than have the ephemeral "needs work" act as a blocker.

If that's truly a desired behaviour, there are add-ons available that add a separate "block" intent.

Hi chrisalbrecht,

We've considered adding a merge check for "Needs Work"; a problem arises when the PR author does their next push, which clears the "Needs Work" state for reviewers, at which point the merge check would no longer be effective. Would the ability to enforce unanimous approvals (https://jira.atlassian.com/browse/BSERV-3927) be appropriate in your use case?

Regards,

Norman Ma [Atlassian]