Re: Managing many review requests - Queued Workflow
I agree with all the sentiments in this thread. Tiptoeing into policy areas is a slippery slope, it has its devoted fans and equally devoted detractors. But I think this can be implemented as something much less onerous than a thick policy. It can simply be a filter option that does not show items that you have commented on or marked Ship It. Those that care can select the option. Or perhaps it can be a different view of Incoming Requests. In using Review Board this year, I have found that it is a great idea to manage a problem every shop has. The one issue I have experienced is inbox clutter- reviews that have long since been baked and done, but still hanging there. Or reviews that I skipped over for various reasons, but then get lost in the pile. Maybe a new view- Reviews You Need to Act On or something like that? --~--~-~--~~~---~--~~ 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 -~--~~~~--~~--~--~---
Re: Managing many review requests - Queued Workflow
Couldn't agree more. We've tried to stay away from any sort of thick policy for a while. I feel one of Review Board's main strengths is that very little really gets in your way. There's been requests for comment types to indicate if a comment is a defect, what type (security issue, coding style, etc.), and how severe it is. David and I have talked about this a bit and, while CodeCollaborator and Crucible both support this and some users have asked for it, we feel it goes down the route of too much policy. I think what we may do here is stick with the original plan of waiting for the 2.0 release (which will have extensions) and writing extensions for this sort of functionality. Those companies that need extensive customizations can write or use an appropriate extension. Keeps the Review Board code simple and the experience straightforward for the majority of people without preventing companies who really need this type of support from having it. Christian -- Christian Hammond - chip...@chipx86.com Review Board - http://www.review-board.org VMware, Inc. - http://www.vmware.com On Sun, Oct 11, 2009 at 8:18 PM, Dan Savilonis wrote: > > 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 > tool. > > 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. > > Dan > > On Oct 10, 7:53 pm, Christian Hammond wrote: > > Hi, > > > > Thanks for your feedback. Comments inline. > > > > > > > > > > > > On Tue, Oct 6, 2009 at 8:17 AM, Biking4Fun 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 i
Re: Managing many review requests - Queued Workflow
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 tool. 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. Dan On Oct 10, 7:53 pm, Christian Hammond wrote: > Hi, > > Thanks for your feedback. Comments inline. > > > > > > On Tue, Oct 6, 2009 at 8:17 AM, Biking4Fun 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 anyt
Re: Managing many review requests - Queued Workflow
Hi, Thanks for your feedback. Comments inline. On Tue, Oct 6, 2009 at 8:17 AM, Biking4Fun 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 --~--~-~--~~~---~--~~ You received this message because you are subscribed to the Google Groups "reviewboard" group. To post to this gro
Managing many review requests - Queued Workflow
Reviewboard is a great for publicizing proposed changes to a complex codebase. However, when there are many team members and a lot of review requests flying around, it is rather difficult to manage everything. (Or maybe I don't know how to use ReviewBoard correctly?) It would be nice if ReviewBoard had the following to help manage heavy traffic: - only show requests that need your action - Tennis Ball model - request priority 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. 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. --~--~-~--~~~---~--~~ 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 reviewboard+unsubscr...@googlegroups.com For more options, visit this group at http://groups.google.com/group/reviewboard?hl=en -~--~~~~--~~--~--~---
