[Freeipa-devel] Reviewer in Trac

[Lukas Slebodnik]

On (20/02/14 15:09), Martin Kosek wrote:
>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.
>
I could not resist.

This effort with the trac looks like reinventing wheel (gerrit or similar tool)

LS