I agree these could be useful features, but you have to be careful not
to delve into the 'policy' realm which will make everyone despise the

We also use CodeCollaborator where I work and no one wants to use it
for small reviews because it's a waste of time. It implements this
ping-pong model to the extreme. Every time you finish the review, you
mark it complete. If a single person makes another comment, all
completed reviews are now uncompleted and every other reviewer must go
back and mark the new comment as read, and sign off again.

It might ultimately be most useful to add customizable user fields to
reviews, instead of something like 'severity' which inevitably won't
meet someone's needs.


On Oct 10, 7:53 pm, Christian Hammond <chip...@chipx86.com> wrote:
> Hi,
> Thanks for your feedback. Comments inline.
> On Tue, Oct 6, 2009 at 8:17 AM, Biking4Fun <govo0...@gmail.com> wrote:
> > Only show requests that need your action - Tennis Ball model
> > --------------------------------------------------------------------------- 
> > -----
> > Currently, a review stays in everyone's view until the requester has
> > marked it submitted, etc.  This tends to clutter one's view and makes
> > it unclear what you need to act upon.
> > It would be better if ReviewBoard implemented a tennis / ping pong
> > model.
> > What I mean by that is the review should only be in someone's view
> > when they need to act on the review.   Once the reviewer has commented
> > and/or marked it "Ship It", that is like hitting the ball back to the
> > requester's court.  It should no longer appear in that particular
> > reviewer's view, because they have already acted upon it.  It should
> > now show up in the requester's view, because someone has commented and
> > perhaps found a problem.
> There's a lot of value in this model, but we'd have to be careful with how
> we implement it. It's not always ideal to only see review requests you
> haven't commented on. There are times when I've only been able to review
> part of a very large change, made that review public, and then went back to
> it later to review more of it. In cases like these, I'd certainly want to
> see the review request still.
> I think what we could do is add a form of filtering to the dashboard, so you
> could show or hide review requests you've already looked at that haven't
> been updated since you last saw it.
> I don't know that anything needs to be done in the requester's view. A
> person generally only has a few review requests out at any given point in
> time. The above filter could just apply to this view as well if it's really
> needed.
> Feel free to file a feature request on this, so we can track it for future
> releases.
> Request Priority
> > --------------------
> > Some code changes are for future releases, a lower priority.  Others
> > are for a critical fix, or trying to make a code freeze deadline- a
> > higher priority.  There currently isn't a way for a requester to
> > indicate that a particular review is important and needs to be looked
> > at quickly.
> We've discussed adding some sort of severity indicator to a review request.
> Maybe we will eventually, but we're not set on it yet for a couple of
> reasons.
> 1) Severity levels can (and will) be abused. Everyone thinks their review
> request is important and unless you're working with Git or something
> similar, you're often blocked until your change goes in. If we give the
> ability to indicate low, medium and high priorities, then most people will
> probably pick high, in order to get their change looked at. Review requests
> with lower priorities will probably just end up being ignored, as there will
> always be higher priority review requests. In the end, this won't do anyone
> any good.
> 2) Severity levels set by the person posting the review request makes sense
> in corporate environments but often don't in open source projects. If
> someone is submitting a patch to Review Board, for instance, I really don't
> want them to decide that it's critical to get it in. It makes a lot more
> sense for myself as the developer to decide the priority of it. But then,
> that doesn't really help us any, because if we consider it critical to get
> in now, we should just review it now.
> 3) Severity levels don't communicate *why* it's important. Is it because
> there's a beta coming up that needs that change? Is it blocking the person?
> Is it blocking somebody else? Is it some emergency security fix? Build
> breakage? I don't think a severity level communicates enough here.
> 4) There's already a mechanism for communicating that something is important
> and needs to be done for some deadline or milestone. Put it in the summary.
> At VMware, for example, people will sometimes prefix a summary with "[Beta 1
> fix]" and everyone working on the product will know that beta 1 is coming up
> and that that needs to take priority. No need for any special fields in the
> database, special UI, or anything.
> We may reconsider it sometime, but our design philosophy so far has been to
> keep things pretty straightforward and free of a lot of policy. Many things
> can be communicated quite well through text, and introducing special UI just
> means that users now have to learn one more little thing. It also often
> means that, while the feature is good for some people, it won't meet someone
> else's need/preference, and we'll be asked to expand on it, thus making it
> more complicated for everyone else. So far, we've worked to keep the number
> of features like this in the product pretty small.
> Hope that made sense.
> Christian
> --
> Christian Hammond - chip...@chipx86.com
> Review Board -http://www.review-board.org
> VMware, Inc. -http://www.vmware.com
Want to help the Review Board project? Donate today at 
Happy user? Let us know at http://www.review-board.org/users/
You received this message because you are subscribed to the Google
Groups "reviewboard" group.
To post to this group, send email to reviewboard@googlegroups.com
To unsubscribe from this group, send email to
For more options, visit this group at

Reply via email to