RFC: Review with Flags (Version 3)

[Hans de Goede]

Warren Togami wrote:

First of all let me say that I'm very happy that this process is still 
being worked on / streamlined!

> Raw notes... your comments would be very much appreciated.
> 
> 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?)
> 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)
> 

Why all the ping-ponging between ? and - during the review and all the 
ping-ponging between ASSIGNED and NEEDINFO. In FE we've never done this 
ping-ponging and we've never felt a need to introduce this IMHO this are 
just unnecesarry mouse clicks. NEEDINFO and fedora-review- may be a good 
idea if either the person requesting the review or the reviewer are slow 
to respond. But in my experience there is a bit of quick discussion 
between the two and the whole review process is finished with a day or 
two (for normal packages) I really feel that all this changing of flags 
and status is just unnecesarry mouse clicks.

> 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.
> 

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)
> CLOSED (ticket is done)
> 
> Process
> =======
> 1. Review Request is filed.
> 2. Set fedora-review? flag to signify that the ticket is ready for review.
Shouldn't the review request form also set this flag for us, I see no 
gain in having todo this manually.

> 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.
What is rejected? See above, often there are a couple of small must FIX 
/ should fix items, which usually get fixed really soon.
> 6. If fedora-review- and owner fixes something, set back to ? to request 
> review again.
Ping-pong-ping-pong, /me no like
> 7. After fedora-review+, initiate the fedora-cvs request procedure.
> 8. After fedora-cvs, checkin, build, and set to CLOSED.
> 

> 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.
> 
to much ping-pong

> 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?)
> 
In general I like being able to say this is a mess, this needs a 
re-review, but we need some rules for this, like whom may initiate this?

> 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.
Not necessary with the new assigned rules, the review requester is the 
reporter and thus get mails, the reviewer is the assignee and thus gets 
mail.

> 2. Changing fedora-review to + should auto-set Assigned pointer to self.
> 
? You write above "Assigned pointer to reviewer, during and after 
review." notice the and after, thus this also isn't needed.

Regards,

Hans