Opened 3 years ago

Closed 6 months ago

#493 closed defect (fixed)

Trac: add status 'please review'

Reported by: jblayloc Owned by:
Priority: major Milestone:
Component: *general* Version:
Keywords: INSPIRE Cc:

Description

The INSPIRE folks would like to more explicitly encourage peer code review.

To support this, we'd like Trac to have an additional status, "Please Review", available from "in work", "assigned" or "new", and parallel to "in merge".

The idea is that most of the time, you'll finish something and mark it as "Please Review", reassigning it to someone else if you know who you would like to read your work. Or not reassigning it if you don't know who should review it, but want *someone* to. You remain responsible for your ticket, though. To keep "Please Review" without assignments from piling up, you should go through after a while and just make them "in merge" instead.

Also sometimes small patches, emergency items, or things you get reviewed by just asking the person who sits across from you can go straight from "in work" to "in merge."

Also naturally we don't want to force other Invenio users to use the "Please Review" status if it doesn't fit into their work organization scheme.

Combined with the above we should have a checklist of what to look for and a template for communicating branch status efficiently, and also a cultural shift which is two-fold:
1) if you're asked to review something, do it.
2) on an ongoing basis, if you've got some spare cycles, look for something marked "please review" and assigned to "nobody" so that you can help out.

I've discussed this with a bunch of the folks over here and everyone seems to agree it's a good idea.

Change History (8)

comment:1 Changed 3 years ago by jblayloc

  • Status changed from new to infoneeded_new

Tibor, is this difficult to do? I'm hoping it can just be done with some configuration. I'm also hoping that Trac is hosted on a server that a variety of people have root on, so this can be assigned appropriately.

If it's complicated and you have to do it, then I'll fail the ticket and we can work out a sociological hack using Keywords. I'd rather not do that, but I'd also rather not wait and I don't think it would make *that* big a difference.

comment:2 Changed 3 years ago by jlavik

  • Status changed from infoneeded_new to new

I also support the idea of introducing extra measures to further encourage code reviews.

In this case, perhaps it could simply require an addition to the normal work-flow, indirectly represented in Trac. I mean, that anything with status 'in_merge' is deemed ready to be looked at, by someone. If this someone happens to be John Integrator, then he will remark and perhaps even integrate it, otherwise others can voice their opinion as well. To avoid situations where someone is reviewing something that gets merged at the same time, the reviewer/merger can let everyone know by submitting a comment before reviewing.

This may sound a lot like what you are saying, and it is, but most of the time people are interested in getting their stuff into master as soon as possible, so I suspect many would skip the 'review_me' step in favor of the 'in_merge' for this reason. These steps are also quite similar in practice.

comment:3 Changed 3 years ago by jlavik

  • Status changed from new to infoneeded_new

Whoops, did not mean to change status earlier.

comment:4 Changed 3 years ago by jblayloc

  • Status changed from infoneeded_new to new

I'm not sure I understand exactly what you're saying Jan. So, you're suggesting that we just assume that "in merge" actually means "in review", and so the responsibility is on people receiving ticket notifications to proactively review things that enter status "in merge"? Or am I misunderstanding?

comment:5 Changed 3 years ago by jlavik

  • Status changed from new to infoneeded_new

Well, yeah, in the sense that you label this piece of code open for public eyes, but also allowing it to be integrated if integrator deems it ready. Perhaps some wording changes are needed for the Trac status, as 'in_merge' may then not cover it all, perhaps something like 'merge_review' would work.

The idea is that rather then (or in addition to) introducing a new step that solely consists of reviewing, we could extend the meaning of 'in_merge' to also include reviewing, as it is almost what happens now, just not explicitly to anyone else then integrators.

However, as you mention a separate review step would also be useful in its own, if you know that this code certainly is not up for merging just yet, but I want x to look at it. Which seems to be what you meant all along.

To put it more clearly; I propose both things. i.e. having a separate 'review' status, and also implicitly include open reviewal in the 'in_merge' step. Does this make sense?

Version 0, edited 3 years ago by jlavik (next)

comment:6 Changed 3 years ago by simko

  • Status changed from infoneeded_new to new

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

comment:7 Changed 3 years ago by jblayloc

To summarize my understanding of your suggestions, Tibor, I think that you think it may be a good idea to:

  • have status in_review plugged into the workflow after in_merge
  • use informal channels as much as possible to get people to review your stuff
  • or barring that (or possibly in addition to that, or possibly just for documentary purposes) use a tag in the style of review:jlavik
  • people should probably get e.g., an RSS feed of tickets tagged review:myusername and cope with them as appropriate (either reviewing or redirecting to someone more appropriate)

And, as a matter of protocol, I'm guessing that the reviewer should be the person to take things from in_review to in_merge?

I agree with all of these things. I think that in a geographically distributed group like ours, doing everything we can to expose the various communication flows in relatively discoverable, public fashion has real value. I think that this would help.

The only question I still have is what is the pragmatic difference (in terms of time-to-merge, for example) between things which a reviewer puts in_merge and things which the author puts in_merge?

comment:8 Changed 6 months ago by jlavik

  • Resolution set to fixed
  • Status changed from new to closed

This (or similar) workflow have been put in place by Tibor fairly recently. Closing this as fixed.

Note: See TracTickets for help on using tickets.