[Freeipa-devel] Reviewer in Trac

[Tomas Babej]

On 02/20/2014 01:22 PM, Petr Viktorin wrote:
> On 02/20/2014 01:14 PM, Martin Kosek wrote:
>> We had a discussion with other developers how better track who is
>> reviewing
>> which patch. Recently, we introduced the Reviewed-By tag in a commit
>> message,
>> but that is a post-review tag which is not useful for someone who
>> wants to know
>> which patches are already reviewed and which are not reviewed.
>>
>> We were testing Patch Work [1] in last months to contain this
>> information, but
>> I personally think that it is suboptimal - it introduces 2 tracking
>> tools that
>> needs to be maintained (Trac and Patch Work) and the Patch Work still
>> requires
>> lot of manual actions when maintaining it's state.
>>
>> I think it would be better to hold this information rather in a
>> single tracking
>> tool - Trac. There are 2 options:
>>
>> 1) "Patch on review" flag, similar to "Patch posted for review" flag
>> which
>> would hold 1 bit information if the patch is just lying there or has
>> somebody
>> assigned.
>>
>> 2) "Reviewed by" text field which would hold a login of a person who is
>> reviewing it. It would be filled either by a person starting the
>> review or by a
>> supervisor like me to forcefully assign a reviewer ;-)
>>
>> With that information in Trac, we could run using a single tracking
>> tool for
>> all patches that have a ticket (which is 95% of patches). It would be
>> then
>> fairly easy to see which patches are sent for review but are
>> reviewer-less.
>>
>> It would also have a benefit for Petr's sendpatches.py script which
>> could pull
>> the reviewer from a ticket and one would not have to use the "-r"
>> option to
>> hard code a reviewer.
>>
>> Any objections to using "Reviewed by" field?
>
> +1, this is the only thing I used Patchwork for, and keeping Patchwork
> up to date just for this took a lot of unnecessary mindless clicking.
>
> Just a nitpick: name it "Patch Reviewer"
> - there's more to a ticket than a patch
> - the review is not done yet when the field is filled out
>
I agree with all said above (and with Petr's comments as well). Honestly
I'm surprised this solution did not occur to us sooner.

Having the "Patch Reviewer" rather than "Patch on review" field is
beneficial in that:
 1.) you can query the tickets by the reviewer (e.g. look up the patches
you volunteered to review)
 2.) you don't have to look up in ticket comments to see who changed the
"Patch on review" flag

-- 
Tomas Babej
Associate Software Engeneer | Red Hat | Identity Management
RHCE | Brno Site | IRC: tbabej | freeipa.org