Putting Defect Tracking/Reporting into Review Board

[Mike Conley]

RB Devs:

Hello, I'm Mike, one of the RB GSoC students for this summer.

ChipX86 and purple_cow have given me the green-light to try to put
simple (and extend-able) defect tracking/reporting into the RB core.

I've tossed up some ideas and some UI mockups on my blog, here:
http://mikeconley.ca/blog/2010/07/10/filing-defects-in-review-board/

What do you think? Am I on the right track?

If you have any questions, comments or suggestions, I'd love to hear
them!

Thanks,

-Mike

[Chris Trimble]

Cool project Mike!

A few thoughts from the trenches...

The word "defect" is pretty loaded.  It is generally a term for a bug and is first reported by a QA department, not a code review.  Then the workflows for dealing with prioritizing them and acting upon the vary wildly.

In light of that, using the word "defect" means the first question you'll get is going to be "how do I push these to Bugzilla/Trac/DevTrack/FogBugz/Jira?"   That question sucks, unless you plan to implement all of the functionality of those systems, which I imagine you aren't planning right now.

So... avoid the word "defect".. IMO.

In our RB process, a chunk of code that passes through a review successfully with known issues gets one of two things:

a) An immediate review/checkin afterwards of the fix.

b) A "TODO:" comment in the code.

The word I prefer for what I think you're trying to do here is "Blocker".  In our workflow, blockers _must_ be fixed or the code can't be checked in.  So there's no such thing as changing a blocker to a "TODO", like the "defect passed" functionality in your proposal.  A developer needs to argue their case in person for that kind of override.

We implemented the functionality I described just now and put it on Github.  You are more than welcome to pick it up and run with it, though based on an older revision of the code at this point.

http://github.com/ChrisTrimble/reviewboard/commits/story1948

 - Chris

[Mike Conley]

Chris:

> A few thoughts from the trenches...

Excellent - exactly what I wanted: some wisdom from the trenches. :D

> The word "defect" is pretty loaded. It is generally a term for a bug and is
> first reported by a QA department, not a code review. Then the workflows
> for dealing with prioritizing them and acting upon the vary wildly.

You're absolutely right. And I like Blocker. A lot. It's simple.
And it already fits in - at least when I've used RB, if I can't get a
review request through, I say that I'm blocked by it.

If there are no objections, I think I might go that route.

> In our workflow, blockers _must_ be fixed or the code can't be checked in.

In your workflow, when a developer successfully argues their case,
what happens to the Blocker? ie., what's the verb? Is it
"overridden"? "Removed"? "Released"? "Closed"?

> You are more than welcome to pick it up and run with it, though based on an
> older revision of the code at this point.

I'll take a peek. Thanks for the insight! Let me know if you think
of anything more.

-Mike

[Mike Conley]

Chris:

I've had some more thoughts about the naming. Initially, I thought
"Blocker" was the right fit, but I'm starting to have second thoughts.

Thinking about it, "blocker" sounds a little adversarial. Almost
defensive. A blocker is an impediment.

This might not sound important, but code review, whether we like it or
not, is a social activity. I believe the less confrontational, and
the more *collaborative*, the better.

So how about this - instead of a Defect, or a Blocker, how about an
Issue?

An Issue can be raised. An Issue can be Resolved. An Issue can be
Dropped.

Would that work? And anybody else care to chime in?

-Mike

[Chris Trimble]

On Sun, Jul 11, 2010 at 8:25 PM, Mike Conley <mike.d...@gmail.com> wrote:

So how about this - instead of a Defect, or a Blocker, how about an
Issue?

Sounds good to me.  Whatever term you think accurately indicates "we need to fix this before a checkin" but also doesn't read as "needs to be in our bug database."  

Or we could just make that term something one can set themselves in the config file.  I could set it up to call them "Thunderdomes" for example.  Two reviews enter, one checkin leaves.

 - Chris

[Chris Trimble]

On Sat, Jul 10, 2010 at 7:42 PM, Mike Conley <mike.d...@gmail.com> wrote:

In your workflow, when a developer successfully argues their case,
what happens to the Blocker?  ie., what's the verb?  Is it
"overridden"?  "Removed"?  "Released"?  "Closed"?

Not sure we have a defined term for it.  "Resolved" sounds good though.

  c 

[Christian Hammond]

On Sunday, July 11, 2010, Mike Conley <mike.d...@gmail.com> wrote:
> So how about this - instead of a Defect, or a Blocker, how about an
> Issue?
>
> An Issue can be raised.  An Issue can be Resolved.  An Issue can be
> Dropped.
>
> Would that work?  And anybody else care to chime

Issue could work, and I believe some other code review software uses
this term too. However, now we reach your original problem. "Issue"
will sound to users like something that integrates with their issue
tracker.

Christian

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

[Mike Conley]

Ok, more cracks at it:

Problem
Problem Solved
Not a Problem

Concern
Concern Addressed
Concern Dropped

Point Raised
Point Taken
Point Dropped

Catch
Caught
Dropped

Favourites?

-Mike

On Jul 12, 12:31 am, Christian Hammond <chip...@chipx86.com> wrote:

> On Sunday, July 11, 2010, Mike Conley <mike.d.con...@gmail.com> wrote:
> > So how about this - instead of a Defect, or a Blocker, how about an
> > Issue?
>
> > An Issue can be raised.  An Issue can be Resolved.  An Issue can be
> > Dropped.
>
> > Would that work?  And anybody else care to chime
>
> Issue could work, and I believe some other code review software uses
> this term too. However, now we reach your original problem. "Issue"
> will sound to users like something that integrates with their issue
> tracker.
>
> Christian
>
> --
> --

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

[Daniel Laird]

I had a look at the mock up - looks good.

I would like to provide some feedback, on the whole concept.
From your mock-up and description it seems like you are trying to mark
a difference between something that should be fixed before commit and
a general comment on that bit of code.

Whilst a tick box that marks the comment is great (and simple which is
always a bonus) - my concern is that its too simple.

I wrote a code review tool for my last company that was built on top
of Telelogic Synergy.
When developers were reviewing code they wanted to make a distinction
between comments and issues/bugs/problems etc.
However we found that a simple distinction of comment / bug was too simplistic.

What we did what have a configurable list of issues which also
contained a comment / fix next time

In your mock up you could replace the tick box with a drop down box
that defaults to comment / general observation.
But if the user clicks on the drop down box a configurable list
appears. For us the list contained things like
Logic, Standards, Performance, Defensive etc.

This way the table of issues can be more informative.
Also much as statistics and historical data gathering are not the
first love of developers - it is good to have the information
available and to be able to spot a trend - maybe people regularly are
ignoring coding standards etc.

I would love to see something like this - I think it is still simple
and uses little more real estate on the UI. But is allows each user
of ReviewBoard to further tailor the tool for their own environment.

Hope this is helpful and maybe some useful feedback on what I would like to see.

Cheers for the great work
Dan

[Stephen Gallagher]

On 7/12/2010 12:55 AM, Mike Conley wrote:
> Ok, more cracks at it:
>
> Problem
> Problem Solved
> Not a Problem
>
> Concern
> Concern Addressed
> Concern Dropped
>
> Point Raised
> Point Taken
> Point Dropped
>
> Catch
> Caught
> Dropped
>
> Favourites?
>

In our world, we've always used the simple terms "Ack" and "Nack" to
express the high-level results of a code reviews. (A Nack would then be
followed by an example of why it failed to pass muster).

I'd suggest the following workflow:
1) An engineer submits a code review
2) Reviewer reads the code and sets either the "Ship It" (aka Ack) flag,
or sets the "Nack" flag and provides code review comments
3) Assuming Nack flag, the original engineer then makes changes and
submits a new version of the patch.
* This action (replacing the original patch with a newer one) should
CLEAR the ShipIt or Nack flags if either are set.
* This will allow easy filtering for patches. There will essentially
be "Needs Review" (no flags set), "Needs Revision" (the "Nack" flag is
set), "Awaiting Submission" (the "ShipIt" flag is set) or "Submitted"
(The "Submitted" flag is set).

Of course, this also implies that the dashboard should be able to filter
based on these flags.

Also, my recommendation for terminology would be "Needs Revision". It's
fairly non-confrontational, but it gets the point across.\

[David Trowbridge]

My personal favorite is "Issue", but no matter what we call it, we're
going to get the "how do I get these issues into my bug tracker"
question, to which our answer is that "these aren't product bugs,
they're requested code changes that people want to track." As long as
we emphasize that it's a new workflow element instead of something
that they already have, I think we'll be fine.

-David

[Mike Conley]

Ok, I'm fine with that. Unless there are further objections, Issue it
is.

Hey David and Stephen - thank you both for the feedback and
suggestions!

> In your mock up you could replace the tick box with a drop down box
> that defaults to comment / general observation.
> But if the user clicks on the drop down box a configurable list
> appears. For us the list contained things like
> Logic, Standards, Performance, Defensive etc.

Part of my task is to leave the Issue tracking/reporting open for
extension so that you *can* have those dropdowns if you need them.

I'm working on a project with Review Board where we'll probably want
the same type of dropdowns (specifically - issue type and severity).
Once my work getting the bare-bones Issue business in, I'll build that
extension, and then if you'd like, you can modify that extension to
suit your purposes.

Because in the end, if we were to toss those dropdowns in, someone
would undoubtably complain that they don't *exactly* fit their
workflow, etc...and we'd be sunk. So I'm adopting a strategy of uber-
simple + extensibility. Everybody (hopefully) wins! :D

Thanks everybody for all of your feedback, and for letting me think
out loud. :D

-Mike

[Daniel Swid]

Regarding choice of vocabulary, I don't think you'll be able to satisfy everyone. If anyone used Trac, the admin interface makes it easy to change the vocabulary for priorities, resolutions, severities, and ticket types. I also liked Stephan's workflow.

I think this is a great addition to the core by the way. 

Daniel 

[Jan Koprowski]

Hi,

This idea is blind alley in my opinion. We have very good and many
stable, grate defect tracking tools and we love it. If I undestand You
try implement very basic defect tracking functionality on the top of
Review Board. This remind me Smarty (PHP Template system) - if we have
already many template system on the top of PHP lets implement PHP in
template system :| Smarty in my opinion is a blind alley and worst
architecture idea for template system and If I understand Your idea
clearly You do something similar.
Instead integration with external tracking tools will be good idea.

Greetings from Poland!
--
Jan Koprowski

On 14 Lip, 01:46, Daniel Swid <da.s...@gmail.com> wrote:
> Regarding choice of vocabulary, I don't think you'll be able to satisfy
> everyone. If anyone used Trac, the admin interface makes it easy to change
> the vocabulary for priorities, resolutions, severities, and ticket types. I
> also liked Stephan's workflow.
>
> I think this is a great addition to the core by the way.
>
> Daniel
>

[David Trowbridge]

As I said, this isn't about defect tracking for the *product*, which I
agree that there are many great tools for. This is about providing a
way to aggregate and track defects in the change -- it's often easy to
miss fixing one thing when you're trying to respond to dozens of
requested changes, and improving the workflow for this is long
overdue. Most other code review tools have such a workflow.

-David

[Jan Koprowski]

How integration between defect tracking tools and Your Review Board
functionality looks like?

--
><> Jan Koprowski

[Mike Conley]

Hey Jan!

Hello to Poland. I visited your country last summer, and loved it.
Wroclaw is gorgeous this time of year. :D

So, just to reinforce what David is saying here, the defect/bug
tracking you mentioned, and the "issue tracking" feature I'm
suggesting are actually two separate things.

Defect/bug tracking is for problems that have already entered the code
base. A developer or a user has discovered a flaw in the software,
which has been tracked down to a bug in the code, and so we track that
bug until we can "squash" it.

The "issue tracking" feature I'm suggesting is *strictly* for review
requests - so, stuff that has either not entered the code base yet
(pre-commit code review), or has been committed (post-commit code
review) but hasn't been given the full green-light yet. The issue
tracking lets us keep track of the problems with the proposed
changes. Some easy examples: "Trim the whitespace"..."alphabetize
your imports here"... "invert the logic of this if/else clause", etc.

Does that make it any clearer?

> How integration between defect tracking tools and Your Review Board
> functionality looks like?

As far as I can tell, Review Board currently allows you to include the
defect/bug #'s from your bug tracker to a review request, in the event
that your review request addresses a particular defect/bug. Review
Board lets you easily link to your tracker, but I think the
interaction doesn't go much further.

HTH,

-Mike

On Jul 19, 3:38 am, Jan Koprowski <jan.koprow...@gmail.com> wrote:
> How integration between defect tracking tools and Your Review Board
> functionality looks like?
>
>
>
> On Mon, Jul 19, 2010 at 8:15 AM, David Trowbridge <trowb...@gmail.com> wrote:
> > As I said, this isn't about defect tracking for the *product*, which I
> > agree that there are many great tools for. This is about providing a
> > way to aggregate and track defects in the change -- it's often easy to
> > miss fixing one thing when you're trying to respond to dozens of
> > requested changes, and improving the workflow for this is long
> > overdue. Most other code review tools have such a workflow.
>
> > -David
>

[Jan Koprowski]

Thanks for clarification :)

Yes. I Love Poland Summer too and :) I just back from Bory Tucholskie
:] I get wash in lake on the morning and drive to work straight from
the forest :) This is beautiful!

--
><> Jan Koprowski