[Freeipa-devel] Reviewer in Trac

Martin Kosek mkosek at redhat.com
Thu Feb 20 14:09:15 UTC 2014


On 02/20/2014 02:54 PM, Petr Spacek wrote:
> On 20.2.2014 14:47, Martin Kosek wrote:
>> On 02/20/2014 02:31 PM, Jan Cholasta wrote:
>>> On 20.2.2014 13:14, 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.
>>>
>>> Is it possible to add new ticket states in Trac? I'm thinking extending it so
>>> that instead of "new -> assigned -> closed:fixed" we have "new -> assigned:work
>>> in progress -> assigned:patch available -> assigned:patch under review ->
>>> closed:fixed", or something like that.
>>
>> It is possible to change the workflow, yes - this is something I was also
>> considering.
>>
>> It can be done with this plugin:
>>
>> https://trac-hacks.org/wiki/AdvancedTicketWorkflowPlugin
>>
>> Unfortunately, the plugin that's in Fedorahosted Trac does not work properly,
>> it gave me some Internal Errors. I filed a ticket for that:
>> https://fedorahosted.org/fedora-infrastructure/ticket/4237
>>
>> When it is fixed, we can try to think about adjusting the workflow. Maybe we
>> can indeed add new states "submitted" and "onreview" to the workflow. But even
>> then I think we could use the "Patch Review by" field so that we know who is
>> reviewing, if anybody.
> 
> Thinking a bit more the e-mail notifications ...
> 
> We can reassign ticket to reviewer if we have state "onreview". IMHO ticket
> owner always get e-mails, right?
> 
> Nice side-effect is that report "{4} Assigned, Active Tickets by Owner" will
> give you exact list of open tickets (state != new && state != closed) and you
> will see who is in charge for each ticket right in the report.
> 
> Ticket owner is not set in stone and everything is still in Trac history (and
> Git, of course :-).

Maybe. But that would mean that when you NACK a patch, you would need to move
the ticket back to previous state to reset the owner. As I know how much you
like the bureaucracy, are you sure you want this change? :)

When the ticket is closed I personally like that owner is set to the
implementer. As that way I quickly see who I need to yell when this ticket
breaks something.

IMO ideal state would be to have a workflow:
"Accept as reviewer" - adds your name to reviewer field, moves to onreview
"Assign reviewer: ________" - adds some login to reviewer field, moves to onreview

We would just need to find out how to do this with some Workflow plugin when it
is ready.

Martin




More information about the Freeipa-devel mailing list