Hi Jennifer,

The "Ship It" checkbox is part of a review, not a review request, and like
all fields in a review, the reviewer has complete control over the contents.
When a reviewer marks something as "Ship It," that is his opinion. It's not
necessarily approval for the code to be committed.

What you're really wanting, I think, is some way to officially have someone
approve code once it has been reviewed. This is where the sociology problem
comes into play. This is handled differently in every organization. In some,
lead developers have to approve it, even if other people give their opinion
that it's ready to go in. In others, you need some number/percentage of Ship
Its. In others, it's more loosely governed and the developer can commit when
he gets Ship Its from people who know the code well enough, people who
aren't wearing any particular "hats."

This is a complex thing for Review Board to just handle, and the Ship It
mechanism is not flexible enough for it. It's a hint, and it's up to
organizations to make use of it how they choose.

David mentioned Policy support. I'll go into that briefly and let you know
my thoughts. The idea is that, down the road, we'll have a way for
administrators to create rules specifying who can do certain things, such as
read code in parts of certain repositories, looked at review requests in
certain groups only, make groups invite-only, and indicate what constitutes
approval for code checkin.

Note that in the last case there, the "Ship It" is not approval, or at least
not the version today. Maybe we'll move to a thumbs up/down model of some
sort to disambiguate "Ship It."

So, at that point, it'd be possible to query a review request and ask if the
code is approved for going in. The review request won't be able to be marked
as submitted until this approval is flagged. It would be up to organizations
to specify the rules. Probably, we'd have some presets that can be chosen,
and an expression editor for more advanced functionality.

That's a ways off, though. Shorter-term, we're trying to get the 1.5 release
out the door, and then the plan is to work on a 1.6 with some smaller
features people have been asking for. We'd then be working on 2.0, which
will have extension support (allowing for organizations to make their own
additions to Review Board) and, hopefully, some form of policy.

Since you're already writing a custom script for controlling whether code
can go in, it might be worth at this point introducing your own policy
support. Take into account the Ship Its and compare the reviewers based on
those reviewers who are allowed to approve code. Sure, it won't be ideal,
but it's probably the best you'll really be able to do short-term.

Another possible option, which David and I would have to talk about, is to
add a permission for allowing a user to mark code as Ship It. The problem
that will need to be addressed is that of default permissions and
retroactively setting permissions. While you likely want to only enable this
for specific users, most places are content with allowing every user to
specify Ship it. There's no mechanism today in either Review Board or Django
for specifying defaults for permissions for users, so this would have to be
written as well. Ends up becoming a slightly larger project

Christian

-- 
Christian Hammond - chip...@chipx86.com
Review Board - http://www.reviewboard.org
VMware, Inc. - http://www.vmware.com


On Fri, Feb 5, 2010 at 11:42 AM, Jennifer <quyr...@yahoo.com> wrote:

>
> Thank you for your answer.  I would like to respectfully disagree.
>
> What is the difference between the "ship it" checkbox and any other
> field in the review?  There are specific permissions from changing the
> header, changing the diff, the text, etc, I think there is even a
> permission to say if people were allowed to post reviews or not.  Why
> is the "ship it" button different?  If this is a sociology problem,
> then why have permissions at all?  Everything should be open, and
> people told what they are allowed to edit and not edit.  We all know
> this does not work in the real world.  Even with advance training and
> whatever, someone could check the box accidentally.  Since there is no
> way a way to uncheck this box we want to limit its' exposure.
>
> We are working on cvs commit hook scripts, so checkin's cannot be made
> unless the referenced review has be marked ship it, so yes actually, a
> check-box *will* stop people from checking in code. :)
>
> On Feb 3, 5:18 pm, David Trowbridge <trowb...@gmail.com> wrote:
> > In general we've avoided adding this sort of permissions and policy,
> > since it adds a lot of UI complexity for what is essentially a
> > sociology problem. We have a bug open requesting some sort of general
> > policy set-up (while we try to figure out what that means), but I'd
> > recommend that this is better enforced through training and
> > management. If people are submitting code without the proper
> > authorizations, then a check-box isn't going to stop them.
>
> --
> Want to help the Review Board project? Donate today at
> http://www.reviewboard.org/donate/
> Happy user? Let us know at http://www.reviewboard.org/users/
> -~----------~----~----~----~------~----~------~--~---
> To unsubscribe from this group, send email to
> reviewboard+unsubscr...@googlegroups.com<reviewboard%2bunsubscr...@googlegroups.com>
> For more options, visit this group at
> http://groups.google.com/group/reviewboard?hl=en
>

-- 
Want to help the Review Board project? Donate today at 
http://www.reviewboard.org/donate/
Happy user? Let us know at http://www.reviewboard.org/users/
-~----------~----~----~----~------~----~------~--~---
To unsubscribe from this group, send email to 
reviewboard+unsubscr...@googlegroups.com
For more options, visit this group at 
http://groups.google.com/group/reviewboard?hl=en

Reply via email to