[ ] DON'T SHIP IT button

[GoogleCodeExporter]

I'm using ReviewBoard at VMware.

Today I saw a review where someone else had x'd [x] Ship it!
-- but I saw a serious flaw that made me want to actively
retract the other person's approval.

I want a [ ] DON'T SHIP IT!  button.  Something that I can
use to mark an already approved review as really requiring
further fixing -- or at least discussion.

>Bela<

Original issue reported on code.google.com by bela.lub...@gmail.com on 15 May 2008 at 6:33

[GoogleCodeExporter]

Original comment by trowb...@gmail.com on 18 May 2008 at 1:16

• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Maybe "Sink-it" should be used?  That way, we can report that a review was done 
and
the quality isn't good enough to be shipped.  It seems that Review Board 
assumes that
all code presented for review is shippable.  Without a "SINK IT" report, it's 
not
possible to restrain a release.  As I see it, this should probably be an urgent
enhancement.

Original comment by kevin.be...@beatport.com on 3 Jul 2008 at 8:59

[GoogleCodeExporter]

What we've been doing at VMware is to just write in the review that the change 
isn't
good enough and either shouldn't go in or needs to be rewritten.

Ship It is a boolean. If checked, it means the code's ready. If not checked, 
the code
isn't ready to go in. I'm still not convinced that we need a tri-state, but I'll
think about it some more.

Original comment by chip...@gmail.com on 3 Jul 2008 at 9:26

[GoogleCodeExporter]

Bugzilla has a multi-state value for "flags" - the value can be unset, ?, + or 
-. 
Bugzilla uses ? to indicate request you a grant, + means the flag was granted, 
and -
means it was denied.  So, review? requests a review grant, review+ means review
granted (implying the code passed review), and review- means review denied 
(implying
the code did not pass review).

Being able to specifically say - review denied or "SINK IT" speaks far more 
clearly
to review requestors than simply not checking the "Ship It" checkbox.  I think 
it
should become either a radio or a drop-down selection that defaults to "Select 
One"
or "Not Fully Reviewed."

Original comment by kevin.be...@beatport.com on 3 Jul 2008 at 9:32

[GoogleCodeExporter]

I think it would be enough to undo the "ship it" action.

Original comment by mkoeb...@gmail.com on 20 Aug 2008 at 6:28

[GoogleCodeExporter]

Re Comment 5:

It wouldn't here.  It's helpful to know when something specifically needs work, 
that
a review was done but the code did not pass muster, helping others to see that
further review is not warranted until the "SINK IT" is cleared, or at least be 
sure
to read the "SINK IT" first, then determine if the remarks are valid and proceed
accordingly.  I do agree, however, that revoking a SHIP IT or SINK IT should be
possible in either case, but that privilege should be limited so that say the
reviewer and a limited set of individuals can override a ship/sink it decision.

Kevin

Original comment by kevin.be...@beatport.com on 20 Aug 2008 at 7:07

[GoogleCodeExporter]

Re Comment 6:

I failed to mention, when a replacement attachment is published, I feel that the
configuration should support clearing any/all SHIP/SINK IT flags, but that's 
probably
a different RFE.

Original comment by kevin.be...@beatport.com on 20 Aug 2008 at 7:10

[GoogleCodeExporter]

I do want this sort of thing in for 1.0. There's become enough demand for it. 
How we
do it is somewhat tricky due to the database schema, though, so we'll need to 
figure
things out first.

Original comment by chip...@gmail.com on 20 Aug 2008 at 8:40

• Added labels: Milestone-Release1.0

[GoogleCodeExporter]

Part of the problem here is that "ship-it" is a flag on a particular comment, 
and is
not tied to a particular user.  So there's no concept of "retracting a ship-it",
since the same user can post multiple ship-its (from multiple comments), and 
there's
no way to indicate that a new comment is a retraction of a ship-it from an old 
comment.

What should probably be done is to break off the ship-it flag from comments, 
and make
it a separate column of "user approvals" (or disapprovals, as the case may be), 
where
a user can set an approval, retract it later, or even set a disapproval to 
counteract
some other user's approval.

Original comment by shankar....@gmail.com on 21 Aug 2008 at 6:01

[GoogleCodeExporter]

Original comment by trowb...@gmail.com on 28 Sep 2008 at 9:07

• Changed state: Confirmed

[GoogleCodeExporter]

As much as I want this ASAP, we need to push out a 1.0 release and this is 
going to
require some additional work and discussion. It's going to be a top priority 
for 1.5.
I'm trying to tie it into some policy stuff I'm toying around with, where 
companies
can sort of customize what they want a bit.

Original comment by chip...@gmail.com on 31 Dec 2008 at 8:26

• Added labels: Component-Reviews, Milestone-Release1.5, Priority-High
• Removed labels: Milestone-Release1.0, Priority-Medium

[GoogleCodeExporter]

(Late to the party..)

Would it make sense to have a ReviewFlag concept (similar to CommentFlag from
django.contrib.comments), where a user could flag another reviewers "SHIP IT" 
as "NOT
YET"/"DISAGREE"/etc. And this can be tied together with
reviewboard.reviews.models.Comment to provide a rationale on the action taken?

Original comment by rusha...@gmail.com on 19 Feb 2009 at 2:57

[GoogleCodeExporter]

Issue 963 has been merged into this issue.

Original comment by trowb...@gmail.com on 24 Mar 2009 at 9:05

[GoogleCodeExporter]

I'm happy to see the comment blocking issue 963 being merged here.  IMO, an 
ideal 
solution lets the reviewer block at the diff/comment level, and his/her 
overall "ship-it/sink-it" flag can't go to "ship-it" if they have unsatisfied 
"sink-
it" blocks.  The overall "ship-it" status for the review (across all reviewers) 
should be dependent on some rule, such as: "75% of participating reviewers 
designate "ship-it" and NO unresolved "sink-it" blocks".

Original comment by thornsof...@gmail.com on 17 Apr 2009 at 12:40

[GoogleCodeExporter]

Original comment by trowb...@gmail.com on 28 Aug 2009 at 8:59

• Added labels: Milestone-Release1.1
• Removed labels: Milestone-Release1.5

[GoogleCodeExporter]

A lot of discussion and design needs to take place regarding this and the 
proposals
for comment types (which likely will not be happening as part of the core Review
Board product, but that's another discussion). I don't feel comfortable making 
this
part of the 1.1 plans. Moving back to 1.5.

Original comment by chip...@gmail.com on 18 Oct 2009 at 8:28

• Added labels: Milestone-Release1.5
• Removed labels: Milestone-Release1.1

[GoogleCodeExporter]

I don't like the sink it idea. If the code has not passed review then the 
reviewer
should never select Ship It. And if the code is just too horrible then it 
should be
marked so in the comments and never shipped.

To my a review request that has not been selected as Ship It is the same as 
being
selected as Sink It. It makes no sense to add this feature.

My 2 cents.

Original comment by pete...@gmail.com on 17 Nov 2009 at 8:14

[GoogleCodeExporter]

Comment #17 misses my point.  Perhaps in Peter's project it is normal for a 
change 
to be strictly reviewed by a single reviewer.  In my project, a review is 
broadcast 
to one or more groups as well as a few focal reviewer individuals.  A single 
change 
is likely to get reviewed by several people -- whose opinions may differ.

Of course if reviewer A thinks the code is not worthy, he should not checkmark 
"Ship 
it".  My request is for when reviewer A said it was good, then reviewer B came 
along 
and found problems.

I want B to be able to negate A's "ship it" in a manner which is visible to 
review-
board itself -- not only to a person who is reading every word of every comment.

It my environment it is also quite normal for people to post reviews that read 
like 
"I've reviewed your use of the XYZ API, which seems good.  However since I 
don't 
know anything about the ABC API which you are also using, you need to get this 
reviewed by someone from that team."  Such a review is very unlikely to be 
tagged 
"Ship it" since the reviewer cannot speak to the entire change.  The box is 
left 
unchecked indicating not that the review fails, but that it is incomplete.

On the other hand, a partial review could very easily result in a "Don't ship 
it": 
"your use of XYZ API is wrong, you need to call free_blarg() on each item you 
originally allocated with create_blarg(); you're leaking them like crazy".

Original comment by bela.lub...@gmail.com on 17 Nov 2009 at 9:39

[GoogleCodeExporter]

If I wanted to be able to give a detailed state along with a comment, those 
states 
would be something like:

  ( ) I reject this entire change, it's a bad idea in the first place
      [no, I don't think we should add a battleship game to the kernel!]

  ( ) I reject this entire change, good idea but wrong design, try again
      [yes this driver needs some manageability; no, don't create a new /proc 
hierarchy]

  ( ) I reject this change due to specific design or code problems (see comment)
      [you missed several calls to free_blarg()]

  ( ) I approve of part of this change but am not qualified/permitted/don't have 
time to approve the whole thing
      [I approve this for the XYZ team, wait for review from ABC team]

  ( ) I approve the whole change
      [Ship it!]

  ( ) Just a random comment with no semantic meaning to reviewboard
      [I'm glad you're fixing this!  Can't comment on the tech, not familiar with 
that module]

Original comment by bela.lub...@gmail.com on 17 Nov 2009 at 9:47

[GoogleCodeExporter]

I agree with #19. Having a Ship it or Sink it will turn a code review into a 
vote. If
more reviewers choose Ship it does it win? I foresee weird issues with having a 
black
and white selection. What if one reviewer Ships and one Sinks? Then what?

#19's comment makes much more sense to me. Anyway I think we all agree that the 
Ship
It mechanism could some changes. :)

Original comment by pete...@gmail.com on 20 Nov 2009 at 2:40

[GoogleCodeExporter]

Original comment by chip...@gmail.com on 20 Dec 2009 at 11:37

• Added labels: Milestone-Release1.6

[GoogleCodeExporter]

[deleted comment]

[GoogleCodeExporter]

Basis of problem is in differences between OpenSource and commerical reviewing 
model. RB support OpenSource review model from nature. When I need update some 
informations in bug track I made this like this:
* When RB is submitted I update issue - review ok
* When RB comment is published I check Ship It and:
  * When checked I don't do anything
  * When unchecked I update issue - review rejected

Of course Ship It is stricly OpenSource function which doesn't have sens in 
commercial model but I think my idea is fine. But this should be noticed under 
webhooks implementation to pass enough informations to webhook in this 
situations.

Original comment by Jan.Koprowski on 8 Jun 2010 at 2:54

[GoogleCodeExporter]

I do not understand very well your point. How is reviewing different between a 
"commercial model" versus an "open source model"?

Original comment by bernard3...@gmail.com on 8 Jun 2010 at 3:02

[GoogleCodeExporter]

This is complex issue. Reasons of this difference come from variety of reviews 
occur in Open Source.
In open source review system people send various requests. One will be submit, 
other will be discard. There is also something like "community" which can rate 
(ship it) idea.
In commercial You get client. Only client advance ideas, fixes, features. 
Before implementing each of them was discussed with architects, payed (by 
client), approved by project leader and they MUST be implemented. When 
developer send code to review there is almost no option to discard or delete 
change. Submiting is the ultimate purpose - always. There is no need to rate 
this code they are acceptable or not. There is no opportunity to discard review 
because they only need improvement. Commercial review have very quasistate 
workflow Submit/Reject nothing more. No discard, no ship it, no sink it, no 
dammit ranking cause this code only need improvement - nothing more. They can't 
be discarded because client want it and payed this code.

Original comment by Jan.Koprowski on 8 Jun 2010 at 5:16

[GoogleCodeExporter]

Jan, this is not an Open Source vs. Commercial thing here. I think you're 
misunderstanding the history of Review Board and how most people use it. Review 
Board itself is open source, yes, but was designed to work at companies, and 
the majority of companies using it do make use of Ship It. Many have asked for 
a Don't Ship It or some customizable names that can mean something in-between.

The usage at your company is clearly very different from most companies I've 
talked to. In most companies using Review Board, there's no "client" involved. 
Rather, there's a group of developers, and they work on code (features, bug 
fixes, architecture improvements, whatever). Sometimes that's on code that 
their team owns, sometimes it's code other teams own. When they put it up for 
review, they're asking for a new set of eyes on the code, especially when it's 
code that they don't own. Other developers mark Ship It when they are 
comfortable with the code going into the product.

That's not much different than how it's used in open source, too. People submit 
code for review. The core developers and active contributors look over it and 
mark Ship It when they're comfortable with it.

The difference between the two cases is who actually submits the code, but 
that's not something that Review Board needs to care about. 

In companies, code gets discarded/deleted all the time. Not all code is good 
and destined for the product. Sometimes a better idea comes around, or the 
feature is no longer needed, or it's substantially different, or it needs to be 
broken up into smaller changes, or senior engineers disapprove of the change, 
or the intern working on the code misunderstood requirements, or something. 
Saying that *all* code is destined to be submitted, and never discarded, simply 
doesn't reflect the reality at most companies.

Your company appears to work quite differently, and isn't really the model that 
we're aiming for with Review Board. It sounds more like a contractor-based 
setup, where you don't have the say in what does and does not go in (at least 
on a high level). We can try to do things to improve the workflow there, but it 
is more of a special case, not the common case.

Original comment by chip...@gmail.com on 8 Jun 2010 at 6:17

[GoogleCodeExporter]

I work only in one company in my life and this my only experience. I'm 
surprised what You are talking about but also I'm skewed by my company. Without 
comparsion this is hard to distinct. Sorry for this misunderstanding.

Original comment by Jan.Koprowski on 8 Jun 2010 at 6:56

[GoogleCodeExporter]

Pushing out to 1.7 (tentatively).

Original comment by chip...@gmail.com on 28 Feb 2011 at 3:27

• Added labels: Milestone-Release1.7

[GoogleCodeExporter]

How about instead of "Ship It", "Don't Ship It", we use +1 / 0 / -1?

Examples:

I've just finished reviewing some code, and I feel it's not exactly ready for 
the prime time.  I put in my final comments regarding the review request, and 
then give the review request a "-1".  Back in the Dashboard (or while viewing 
the review request), I can see that the current revision for this review 
request has a field in the table showing: "0 / -1" meaning that there are no 
+1's, and a single -1.

Another reviewer has a different opinion, however, and after their review, they 
give a +1.  So now, in the Dashboard I see that the latest revision for this 
review request displays a "score" of "+1 / -1".

A third reviewer comes in, and agrees with me, so now we see:

"+1 / -2".

The reviewee modifies their review request.  A new revision is uploaded, so the 
score is now reset at 0 / 0.

Note that a reviewer can also mark a review with "0", in the event that they 
have 

I know that there was some concern that this would turn into a vote.  I'm 
pretty sure the Ship It is already a "vote" though.  There's just no way to 
vote the other way.

Thoughts?

Original comment by mike.d.c...@gmail.com on 23 Sep 2011 at 3:35

[GoogleCodeExporter]

My preference would be to add a "Hold" button.  This would be a generic 
indicator that could be used for any review to flag at a high level that 
something is amiss.  Comments should accompany the indication detailing why a 
hold was being requested such as "I want to review but need more time", "Issue 
below on line xxx needs discussion", or "I think another solution is needed".  
The semantics of how this flag is used could be defined by the organization.  
There will also need to be a process to release the Hold.

Original comment by whwalk...@gmail.com on 9 Nov 2011 at 10:43

[GoogleCodeExporter]

[Hold] is good.   Per-reviewer flag, can be cleared by anyone (leaving a paper 
trail).  Any further semantic process needs to be handled by humans once the 
review has gotten into this sort of exception state.

Original comment by bela.lub...@gmail.com on 10 Nov 2011 at 2:52

[GoogleCodeExporter]

Related discussion in Issue 920 and Issue 2106

Original comment by yuzi...@gmail.com on 5 Jan 2012 at 5:54

[GoogleCodeExporter]

All the discussion above highlights a very nice enhancement, but I just thought 
to put down what I've been doing as a possible work around.

In order for code being valid for "shipping" it needs x no of "Ship It!"'s and 
no "open issues".

For example:
Dev A comes along and says "Ship It!", but then Dev B finds a problem and opens 
and issue. Because there are still issues open, the "Ship It!" is not valid and 
simply comments on the fact that Dev A found it ok to go.

This prevents code from being shipped despite someone having clicked "Ship It!".

A delete 'Ship It!' would still be nice for accidental clicks, and it would be 
cool to see the no of open issues on the dashboard.

Original comment by narf1...@googlemail.com on 31 Jul 2012 at 10:43

[GoogleCodeExporter]

Here's what I've been thinking:

A 3-way toggle between: "Ship it!", "Undecided", and "Hold it!" per reviewer. 
(See attachment)
A reviewer's default state is "Undecided".

Maybe each review will also have a counter to indicate the number of "ship 
it"'s and "hold it"'s the review request has received, and possibly list the 
reviewers who have given it the ship it's and hold it's.

Whoever is pushing the code will also be warned before pushing if the review 
request has any "hold it"'s, but this will not restrict them from pushing.

This will allow people to undo any Ship it/Hold it actions done intentionally 
or accidentally, or change their stance after a request has been updated or 
after further review.

Original comment by meda...@gmail.com on 10 Aug 2012 at 11:09

Attachments:

• 3waytoggle.png

[GoogleCodeExporter]

Original comment by trowb...@gmail.com on 24 Nov 2012 at 6:33

• Added labels: Milestone-Release1.8
• Removed labels: Milestone-Release1.7

[GoogleCodeExporter]

The whole 'karma' idea is fairly common... see e.g. gerrit, Fedora Bodhi, etc. 
Although I'm not sure that's actually necessary for RB, since we already have 
issues. If the presence of unresolved issues precluded a review showing as 
'ready' (on dashboards particularly, I would say), I think that would generally 
suffice.

Of course, it would also help if 
http://code.google.com/p/reviewboard/issues/detail?id=2723 and/or 
http://code.google.com/p/reviewboard/issues/detail?id=2817 were also 
implemented.

(More generally, creation of an issue or uploading of a new revision should 
'disable' any previous 'Ship It' so that they are not counted on the dashboard. 
This, plus 2723, would allow reviewers to override open issues, but 
merge-masters would know when this is happening and be able to verify that such 
an action is appropriate.)

Original comment by matthew....@kitware.com on 8 Jan 2013 at 6:51

[GoogleCodeExporter]

For what it's worth, this situation would be handled fine if there were a "# of 
Open Issues" column on the dashboard.

When someone else does a "Ship It!" and I disagree, I simply post a review with 
comments/issues.

The Review Requester will be paying close enough attention to the review to 
know this information, but members of Review Groups would benefit from seeing 
"Oh hey, there's a ship it on this _and_ open issues, so there's some possible 
disagreement.  Maybe I should weigh in."

We've made it clear in our ("commercial", btw) organization that some random 
person's "Ship It!" is not sufficient for you to check in your code.  You have 
to get the "Ship It!" from the _right people_, and you need to give sufficient 
time for other people to chime in (even if you get a "Ship It!" immediately).

There are a lot of different use-cases of people using ReviewBoard, but adding 
another approval state "Hold It/Sink It" would only add confusion ("I have a 
couple of comments that need to be addressed, do I need to Hold It? Only when 
someone else has Ship It?").  And, the presence of open issues already means 
it's not ready (If they're not relevant, mark them dropped - otherwise, you 
need to fix them).

I would expect to get negative feedback from the people using the tool here if 
"Sink It/Hold It" were to appear (but, we could deal with it).

My $0.02

Original comment by ke...@keithmoyer.com on 10 Dec 2013 at 5:28

[GoogleCodeExporter]

Good to see this is finally accepted.

For our organization it would be important this 'hold it' 'sink it' toggle to 
be available through the api.

Our workflow for illustration purposes:

Our svn repository has contains lists of authorized reviewers for certain areas 
of the code base (developers who know at least a bit about that section of 
code). To be able to commit to these areas we have a svn precommit hook to 
check if the commit is authorized by any of these reviewers. due to the size of 
the project there are often more then one on many parts of the code.
Currently we check reviewboard if there is a 'ship-it' by a appropriate person 
if it is the commit is allowed. (else its refused).

The reason we need the hold it is because from time to time one of the 
reviewers reviews it and fails to find a bug present in the review and check 
the 'ship it'. if another reviewer finds a bug in the review before its 
committed (or wants to contest the entire commit somehow) there is currently no 
way (except email / messages) to prevent the commit.

in our case a commit would always be blocked as long as there is at least one 
hold-it present. (it would basically used as a veto system)

Original comment by pvgodd...@gmail.com on 19 Dec 2013 at 10:45

[GoogleCodeExporter]

Original comment by trowb...@gmail.com on 20 Jan 2014 at 8:43

• Removed labels: Milestone-Release1.8

[GoogleCodeExporter]

Issue 3219 has been merged into this issue.

Original comment by trowb...@gmail.com on 6 Feb 2014 at 9:46

[GoogleCodeExporter]

Issue 3219 has been merged into this issue.

Original comment by chip...@gmail.com on 6 Feb 2014 at 10:03

[GoogleCodeExporter]

Are there any known plugins for this feature available / is this this feature 
still under consideration?

Original comment by pvgodd...@gmail.com on 1 Apr 2015 at 9:53

[GoogleCodeExporter]

@pvgodd: looks like this issue got forgotten. 

Last time it was touched by a project member, it was bound to the 1.8 milestone 
(after it was repeatedly postponed through every big version milestonse 
earlier). 1.8 never came, version number went straight to 2.0, which didn't 
contain the new feature. It is also missing from the release notes of 2.5 Beta 
1.

BTW, there's another feature request targeting the ship it function, but in a 
less complicated manner, see issue 3462.

Original comment by csipak.a...@ardinsys.eu on 6 May 2015 at 10:01

[GoogleCodeExporter]

It's not forgotten, just that this is a relatively tricky thing (literally 
everyone has an opinion about how ship-its should be improved/changed and 
whatever we do has to be very carefully thought out) and we have fairly limited 
resources.

Original comment by trowb...@gmail.com on 6 May 2015 at 11:45