#493: Trac: add status 'please review'
------------------------+---------------------
Reporter: jblayloc | Owner:
Type: defect | Status: new
Priority: major | Milestone:
Component: *general* | Version:
Resolution: | Keywords: INSPIRE
------------------------+---------------------
Changes (by simko):
* status: infoneeded_new => new
Comment:
(Sorry for getting to this one only now.)
Yes, peer review is a good thing, and it is happening right now in a
kind of less institutionalised or less visible manner, so it may be
good to put it more in evidence via Trac ticket status, especially for
geographically distributed teams such as ours.
@jlavik: Indeed, `in_merge` consists of a code review step and a
testing step and a system integration step, so it is encompassing
several QA activities. Therefore having separate `in_review` and
`in_merge` can be confusing in this respect, since the former is
almost always implicit in the latter. Maybe we can word things
differently somehow, e.g. to have //please review only// versus
//please review and merge// or something like that.
@jblayloc: I think it would lead to a contradiction to be both the
developer responsible for a ticket and to be able to reassign it to
somebody else for a review, since the reassigned ticket would
disappear from the tickets one owns, so to speak. We can either have
an `in_review` status but the developer would stay the ticket owner
and the reviewer would be perhaps just a tag. Or we can reassign the
ticket to the reviewer but then the ticket would disappear from
developer's ticket list unless we again tag it appropriately. I don't
think we can have several owners in Trac yet. So, in order to
reconcile these two needs, we shall probably have to play with the
keyword tags, or else invent plenty of statuses like
`in_review_by_erika`. Neither seems fully satisfactory.
I'd probably keep the ticket with the developer, plug `in_review` step
after the `in_work` step, and the reviewers would come and leave
comments in the free style in the ticket, and we'd keep the reviewer
name tagged like `reviewer:simko` in the ticket comment or something
like that. Moreover, in order not to make unnecessary ping-pong with
ticket updates and heavier ticket management, I'd still probably
promote the usage of other informal channels such as voice/chatroom to
ask people to review something. So John Doe would create a branch
that attacks a certain ticket, make a //please review// request that
would put the ticket status into `in_review`, and use chatroom to ask
Erika Mustermann to review the branch, and Erika would leave her
comments in the ticket, and let John Doe push the ticket to final
integration, or perhaps push it herself. Something like that.
@jblayloc: Also, you mention "if you're asked to review something, do
it". I would add "... or redirect to appropriate person". It would
not be economical to ask someone who is dealing with BibSched back-end
programming to review BibEdit jQuery code, to take an example. This
is why we have informal "module maintainers" who can best review code
contributed to a given module or on a given technical topic. In
practice, the peer review is naturally self-organising around
concerned people anyway, so it works out of the box like this already,
but it may be perhaps good to voice it explicitly here for the benefit
of larger Invenio developer community.
@jblayloc: Finally, you mention to have //please review// available
from `in_work`, `assigned` or `new` statuses. We can review only
something that was already (at least partially) implemented, hence it
would be applicable only for `in_work` status. Our current Trac
workflow uses a modification of the enterprise workflow:
[[http://trac.edgewall.org/raw-attachment/wiki/WorkFlow/Examples
/enterprise-workflow.medium.png]]
I'd plug the peer review step probably in the in_QA area in parallel
to, or before, the hitherto merge/integration step, and use a workflow
as suggested above in the John Doe and Erika Mustermann example.
(Unless we also investigate some other Trac add-ons or unofficial
hacks. For example, there are various peer review and code review
plugins for Trac, though I think these are typically suited for SVN
shared-push repository style of collaboration; moreover, line-based
code review interfaces may be quite heavier than informal voice/email
reviewing we have been doing so far.)
--
Ticket URL: <http://invenio-software.org/ticket/493#comment:6>
Invenio <http://invenio-software.org>
