RFC: Review with Flags (Version 3)

[Kevin Fenzi]

On Mon, 05 Feb 2007 18:41:23 -0500
wtogami at redhat.com (Warren Togami) wrote:

> Raw notes... your comments would be very much appreciated.

Just up front, I think one important thing we should do in any review
procedure is to push as much of the processing and dealing with
bugzilla off on the reviewer, and make it easy for the submitter to
just deal with getting their package up to meeting guidelines and
getting approved. 

It's much more likely the reviewer will know the process, where the
submitter won't, so we need to make it easy for them to see what to do. 

> Key Changes from Previous Procedures
> ====================================
> 1) Assigned to pointer indicates that there is a reviewer.  Assigned 
> pointer to nobody means there is no reviewer yet.
> 2) BLANK means review not requested.  ? means review requested.  This 
> makes it easier to query, because querying for BLANK is a bit
> inefficient. (Good idea, bad idea?)

If it requires the submitter to set, I think it's a bad idea. 

> 3) NEEDINFO is used to avoid changing the Assigned pointer when fixes 
> are needed.
> 4) Assigned pointer DOES NOT CHANGE during and after the review.
> 
> Fedora Review Flag States
> =========================
> fedora-review BLANK (Not under review at all!)
> fedora-review? (I want a review)
> fedora-review- (rejected, needs work, set NEEDINFO to person who
> needs to fix it)
> fedora-review+ (APPROVED)

How about instead: 

fedora-review BLANK (I want a review) (so it doesn't need to be set)
fedora-review ? (in process of review, questions for submitter or
reviewer pending)
fedora-review - (submitter stopped responding, withdrew package,
decided to let someone else submit, etc)
fedora-review + (APPROVED)

I think we should try and avoid the "rejected" word in the flow. I
suspect people will see that as "this package is no good, we don't want
it".

> 
> Assigned Pointer
> ================
> (Note, Assigned pointer is different from the ASSIGNED state.)
> - Assigned pointer to nobody at fedoraproject.org if no reviewer yet.
> - Assigned pointer to reviewer, during and after review.
> - Set Assigned pointer back to nobody if reviewer gave up.

Sounds good. 

> Bugzilla States
> ===============
> NEW ASSIGNED REOPENED (rather meaningless distinction?)
> NEEDINFO (to owner or somebody who needs to act to fix)
> MODIFIED (seems to be fixed, please test it)

What does this state do?

> CLOSED (ticket is done)
> 
> Process
> =======
> 1. Review Request is filed.
> 2. Set fedora-review? flag to signify that the ticket is ready for
> review. 

I'm sure this will get missed by people who don't know the process that
well (submitters). 

> 3. Reviewer takes bug, Assigned pointer set to self.
> 4. If APPROVED, fedora-review+.
> 5. If rejected, fedora-review- and NEEDINFO owner to fix it.
> 6. If fedora-review- and owner fixes something, set back to ? to
> request review again.

I think it might make more sense to just move from 

BLANK (new request)
to
? (in process) To my mind, ? makes it more obvious that there is a
question, and things are pending, but - makes it sound like it was
unacceptable at all. 
to
+ (APPROVED)

> 7. After fedora-review+, initiate the fedora-cvs request procedure.
> 8. After fedora-cvs, checkin, build, and set to CLOSED.

Could the admin that processes the fedora-cvs request also close the
bug? That would take another step away from a submitter who might not
know the process well. 

> Drawbacks
> =========
> Since the mass review tickets were not filed by the package owners,
> it is less than obvious.  Most reviews however are filed by owners,
> so this is less of a problem beyond this mass review.

Agreed. 

> Other Possibilities
> ===================
> fedora-review? could also be used on any other Fedora bug when a 
> horrible mess is found in an existing package, and attention for a 
> re-review would be desired to fix it.  (Good idea, bad idea?)

Might work... but it would add a bunch of comments to a bug that might
be un-related to other review cleanups. Perhaps a 're-review' request
could be filed for the package. 

> Possible Process Optimizations
> ==============================
> (These might possibly help... although they might require hacks in 
> Bugzilla's code, so we only request these optimizations only if they 
> would make a huge positive difference.  Your thoughts?)
> 1. Changing fedora-review in any way should auto-CC yourself
> automatically. 2. Changing fedora-review to + should auto-set
> Assigned pointer to self.

Those might be nice if easy to do.. 

> Warren Togami

kevin
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/fedora-maintainers/attachments/20070205/086c4ac7/attachment.sig>