Opened 9 years ago

Closed 9 years ago

Last modified 4 years ago

#1736 closed enhancement (implemented)

Trac should have a "needs code-review" status

Reported by: nickm Owned by: Sebastian
Priority: Medium Milestone:
Component: Internal Services/Service - trac Version:
Severity: Keywords:
Cc: weasel Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

We've discussed using trac to remember which pending Tor branches need code review. This is mostly necessary for Tor itself, where our current documented process (mention the branch in a git-branches file) is at odds with the most effective process (bug nickm on IRC until he reviews/merges your branch).

This is apparently possible with versions of Trac since 0.11 : see http://trac.edgewall.org/wiki/TracWorkflow for details how. Doing this is complicated enough that we should try it out on the testing trac installation before we do it in the main one.

Child Tickets

Change History (16)

comment:1 Changed 9 years ago by phobos

Owner: changed from erinn to phobos
Status: newaccepted

comment:2 Changed 9 years ago by phobos

Resolution: implemented
Status: acceptedclosed

done.

comment:3 Changed 9 years ago by Sebastian

Isn't this different from what we wanted initially? I thought the review thing was something that isn't just a different type

comment:4 Changed 9 years ago by nickm

Resolution: implemented
Status: closedreopened

Sebastian is right. You added a new type. What we need is a new status, as documented on the page I linked to.

comment:5 Changed 9 years ago by weasel

Owner: changed from phobos to Sebastian
Status: reopenedneeds_review

like that?

comment:6 Changed 9 years ago by nickm

Almost. Is there any way we can make it so that the thing you select says something like "mark as 'needs review' by X" ? And is there any way that stuff can be 'needs review' without being 'needs review' by a particular person?

comment:7 Changed 9 years ago by weasel

Cc: weasel added

comment:8 Changed 9 years ago by weasel

Owner: changed from Sebastian to weasel
Status: needs_reviewaccepted

currently the (re-)assign review sets state to needs_review and changes the owner to the reviewer.

We could have something that just sets the state without modifying owner, but how would we distinguish between 'it's the old owner' and 'this is the reviewer'?

We would need to introduce a second new state.

Where we have 'needs_reviewer' (owner field meaningless) and the other state is 'reviewing' (by the owner). How about that?

comment:9 Changed 9 years ago by Sebastian

I think just the former is good enough? The owner field could typically be the patch author

comment:10 Changed 9 years ago by nickm

Sounds like a good idea to me.

Can we maybe use the phrase "code review" in the user-visible text, so that users don't decide we're talking about some kind of "bug review" or something.

comment:11 Changed 9 years ago by weasel

Status: acceptedneeds_review

how about now?

comment:12 Changed 9 years ago by Sebastian

Owner: changed from weasel to Sebastian
Status: needs_reviewassigned

comment:13 Changed 9 years ago by Sebastian

Status: assignedneeds_review

comment:14 Changed 9 years ago by Sebastian

Ok, looks much better. All these owner changes are annoying ;)

comment:15 Changed 9 years ago by nickm

Resolution: implemented
Status: needs_reviewclosed

Looks good to me now. I'll be bold and close it for now; please reopen if we missed anything.

comment:16 Changed 4 years ago by qbi

Component: TracService - trac

Move all tickets from trac to "Service - trac" component.

Note: See TracTickets for help on using tickets.