Ship It, and Don't Ship It
Mike Conley
So I've been mulling over Issue 489
(http://code.google.com/p/reviewboard/issues/detail?id=489) - a feature
request for a "Don't Ship It" option when doing a review.
I posted one proposal for a solution here:
http://code.google.com/p/reviewboard/issues/detail?id=489#c29
I've got another one, and I guess I wanted to open this discussion up to
folks who don't necessarily follow the bug tracker.
Currently, Ship Its are coupled with reviews. Once a review with a Ship
It has been published, the Ship It is out, and rather immutable. If,
for example, I changed my mind (I gave a Ship It, and then noticed a
major problem in the code, for example), I cannot retract my Ship It.
I'm also unable to say anything but "Ship It". I cannot say "Don't Ship
It".
So, the idea is that perhaps we can decouple Ship It / Don't Ship It
"votes" from reviews themselves. For each review request revision, each
reviewer gets a single vote. That vote can be either Ship It, Don't
Ship It, or a non-vote. There will be an interface for setting your
vote within the "publish review" dialog that currently has the "Ship It"
checkbox, but there'll also be an interface for changing that vote on
the review request itself. Once a review request has a new revision,
votes for the old revision are locked, and the voting is reset for the
new revision.
Here's a super quick-and-dirty screenshot I threw together to show you
what I mean:
Are there any problems with this approach that I'm not seeing? Is this
"voting" business a touchy social issue?
All feedback welcome! Thanks,
-Mike
Tucker
buttons. I'm completely biased in my opinion because I'm anti
Facebook and using thumbs has the same immature feel to me. My
personal preference would be something like "Approved" and "Not
Approved." Regardless of my issues though, I think scoring reviews is
a good idea and has potential uses outside of just the individual
review (like recognizing patterns with submitters).
--
--tucker
Christian Hammond
The current Ship It behaviour is probably the first thing added to review, back when we first wrote "the reviewboard." It's long overdue for a change.
I think the conceptual separation is fine, but I do like associating with a review what the decision is. It helps to show a history of when the ship/don't ship decisions were made, and why.
If we went with a separate voting model, which was independent of reviews, I think we'd lose a few crucial bits of info. It would be too easy for someone to say "no" without giving a reason why, and there's no good way of seeing when such a vote was made.
So, I'd personally still prefer having this be an option on reviews. It should probably, however, cancel itself when it makes sense, such as a new diff upload.
The difficulty in that is knowing when to cancel. Not everything being reviewed is a diff these days. One option is to have a checkbox when submitting a new set of changes, allowing the Ship It-type statuses to be cleared, and enabling it by default, but I don't love that idea.
Certainly, if it does remain tied to a review, it should be possible/easy to undo or change the vote. I should be able to cancel a Ship It, or say "Oops, that was meant to be a Ship It. *click*" or "I misunderstood, so I'm changing from Don't Ship It to Ship It" or whatever.
So that's a bit different from what you want. Note that my opinion her doesn't mean "This is set in stone" :). I think it's good that we start exploring different models for this, and hopefully we can figure this out for 1.7.
Christian
Gilles Moris
I agree. There is currently no way to express a clear veto on a code change.
Regards.
Gilles.
Gilles Moris
Hi Christian,
I totally agree that a Ship It! have to be linked to a review. In my mind a Do
not Ship it! would be as well. For the moment, we do not have this option.
Without a Ship it!, there is no distinction between a neutral comment about a
piece of code that "Could be better" but will be OK for the time being;
possibly something implemented in 3 lines when it could be done in 1, but
hey, it does the job. And an "Absolutely no way" review because the change is
really doing something wrong.
Possibly in your mind, if there is no "Ship it!", this means Don't Ship it!
But several times, I have seen people not taking care about reviews, because
they already had a Ship it! previously. Here a Don't Ship it! would help to
veto the change until fixed.
As for the clearing of the Ship it! vote, my Submit hook looks only at the
reviews attached to the latest diff. In a way, this is implicitly clearing
the Ship it! vote at each revision. So I am with you for this.
Note that this is more or less due to the lack of review policies slated for
2.0 as I understand - possibly through plugins?. The Don't Ship it! would be
an interesting intermediate solution.
Regards.
Gilles.
Christian Hammond
Hi Christian,
I totally agree that a Ship It! have to be linked to a review. In my mind a Do
not Ship it! would be as well. For the moment, we do not have this option.
Without a Ship it!, there is no distinction between a neutral comment about a
piece of code that "Could be better" but will be OK for the time being;
possibly something implemented in 3 lines when it could be done in 1, but
hey, it does the job. And an "Absolutely no way" review because the change is
really doing something wrong.
Possibly in your mind, if there is no "Ship it!", this means Don't Ship it!
But several times, I have seen people not taking care about reviews, because
they already had a Ship it! previously. Here a Don't Ship it! would help to
veto the change until fixed.
As for the clearing of the Ship it! vote, my Submit hook looks only at the
reviews attached to the latest diff. In a way, this is implicitly clearing
the Ship it! vote at each revision. So I am with you for this.
Note that this is more or less due to the lack of review policies slated for
2.0 as I understand - possibly through plugins?. The Don't Ship it! would be
an interesting intermediate solution.
Regards.
Gilles.
We're on the same page here :) I do agree with a Don't Ship It state, alongside Ship it. I don't think not having "Ship It" implies "Don't Ship It." It can still mean that, depending on the content, but of course you won't quite get that knowledge programatically.
Adding a new "Don't Ship It" or "Not Ready" state is completely doable. Resetting the state on new diffs would be easy enough. We'd need to decide when exactly to reset on other things. File attachments or screenshots, for example. Perhaps we also reset if anything commented on changed.
If we do decide we want a new model for Ship It in 1.7 beyond a new state and resets, we'll probably want to nail that down soon. We'll also need to figure out how that maps to existing Ship Its.
As for review policies, I'm evaluating some things along those lines.
Christian
--
Christian Hammond - chi...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com
Colin Caughie
I'm glad this is being discussed, this is one area where there is a lot of room for improvement. One thing we would like to see, in additions to the suggestions made so far, is a way of indicating the current position of each reviewer, rather than just a global Ship It flag.
Our policy is that changesets can't be pushed until all reviewers have said Ship It, and currently it's not obvious when that has happened. (The Ship It count helps a little, but not much, since a) a single reviewer can add a Ship It multiple times, and b) the dashboard doesn't show how many reviewers there are).
Here are a couple of ideas as to how this might look:
Dashboard: In the Ship It column, the icon should be green only if all reviewers have said Ship It and none have said Don't Ship It. It might be Red if anyone has said Don't Ship It, Yellow if some but not all have said Ship It, and grey if nobody has said anything.
Review Page: In the summary section it would be nice to have a table with all the reviewers in one column, and an indication of their current position (Ship It, Don't Ship It, undecided/not reviewed) in the other.
Regards,
Colin
scanned for viruses and spam by
indigovision
If you consider this email spam, please forward to
sp...@indigovision.com
Latest News at: http://www.indigovision.com/index.php/en/news.html
David Stanek
Adding a new "Don't Ship It" or "Not Ready" state is completely doable. Resetting the state on new diffs would be easy enough. We'd need to decide when exactly to reset on other things. File attachments or screenshots, for example. Perhaps we also reset if anything commented on changed.
David
blog: http://www.traceback.org
twitter: http://twitter.com/dstanek
Christian Hammond
On Sun, Sep 25, 2011 at 6:20 PM, Christian Hammond <chi...@chipx86.com> wrote:Adding a new "Don't Ship It" or "Not Ready" state is completely doable. Resetting the state on new diffs would be easy enough. We'd need to decide when exactly to reset on other things. File attachments or screenshots, for example. Perhaps we also reset if anything commented on changed.
Can the history of the "Ship It" and "Don't Ship It" reviews be saved when the counts are cleared?
Yes, I think whatever we do, keeping that history is crucial.
Christian
