Opened 10 years ago

Closed 10 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 10 years ago by phobos

Owner: changed from erinn to phobos
Status: newaccepted

comment:2 Changed 10 years ago by phobos

Resolution: implemented
Status: acceptedclosed

done.

comment:3 Changed 10 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 10 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 10 years ago by weasel

Owner: changed from phobos to Sebastian
Status: reopenedneeds_review

like that?

comment:6 Changed 10 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 10 years ago by weasel

Cc: weasel added

comment:8 Changed 10 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 10 years ago by Sebastian

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

comment:10 Changed 10 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 10 years ago by weasel

Status: acceptedneeds_review

how about now?

comment:12 Changed 10 years ago by Sebastian

Owner: changed from weasel to Sebastian
Status: needs_reviewassigned

comment:13 Changed 10 years ago by Sebastian

Status: assignedneeds_review

comment:14 Changed 10 years ago by Sebastian

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

comment:15 Changed 10 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.