- removed comment
track should have a "reviewed" state for tickets whose attached patch has been reviewed
Issue #664
closed
A typical tickets live-cycle would be:
- open
- possibly accept or reassign
- review
- reviewed
- closed
with possible several iterations through the review and reviewed states.
Keyword:
Comments (18)
-
-
- removed comment
"failedreview" should be the same as "accepted".
-
reporter
- removed comment
Uh, trac does not enforce any order in these states. You can already change a state back to review when you attach a new and improved patch to a reviewed ticket that did not pass review. "Closed" just means :no further action required" doesn't it? I really only wanted to suggest having a state that lets owners quickly find any patches that they can commit. The states were just an example to see if this would be a reasonable sounding workflow. So in that sense "accepted" would suit my definition (well as long as it can mean "accepted or rejected").
-
- removed comment
I meant that you could not change it from "review" back to "accepted". It turns out that you can - you have to use "Reassign to <user>", after which the state will be "accepted". I don't know if a non-admin can do this, and I don't know how to do this if there was never any owning user. It is quite unintuitive.
-
- removed comment
I don't think we should introduce yet another state, which would complicate the workflow even more. What are the possible options when a ticket in 'review' state is reviewed:
a) The patch is accepted, and the reviewer decides to commit. He can then close the bug. b) The patch is accepted, and the reviewer decides to let the submitter of the bug commit the patch. This should happen in a relatively short timeframe (the submitter should be notified). If this is the case, there is no problem. If this is not the case and it takes a long time it would probably be good to keep it in 'review' so that people are reminded often that there is this outstanding, but solved issue. Maybe the reviewer just changes her mind and commits after some silence from the submitter. c) The patch is not accepted. In this case the bug should be reassigned to someone, most likely the submitter. The 'review' state would be deleted. d) The problem was "not valid" or a "duplicate" or some other special case which can already be taken care of.
-
- removed comment
I do not think the workflow is complicated. We have had many tickets in the past (and probably even now) which are in the state of "has been reviewed and can be committed" which are impossible to distinguish from the tickets which need review without opening each one. These are different states, and we should have ticket states for them. I don't see why adding an additional state is a bad thing.
-
reporter
- removed comment
Since we already have an "accepted" state (I had not realized we had when submitting the ticket) we could simply rename that one into "reviewed" to catch both accepted and rejected patches. Then we do not have to increase the number of states
-
- removed comment
I would like to either restart the discussion, or close this ticket with wontfix. Is anyone still thinking we need a 'reviewed' (as in reviewed and good) state?
-
reporter
- removed comment
Accepted works for me.
-
- removed comment
I think "accepted" means that someone accepted the issue, and someone works on solving it. Can we change the name to "solution accepted" or so?
-
- removed comment
When a patch for a ticket is just waiting to be commited - doesn't does mean someone is working on it? What is the use-case for a new 'solution accepted' state? You want to know which tickets have patches attached which have been reviewed, in order to commit them?
-
- removed comment
Often, committing a patch is a non-trivial operation. The person reviewing the patch may not be the one committing it. Or the person who wrote the patch does not have permissions to apply it.
When I look for patches to review, I don't want to be presented with patches that are only waiting to be committed.
Yes, I want to see which patches have been reviewed and accepted, and are only waiting to be committed. How many of these do exist at the moment? Is there a patch I forgot to commit, and that is now in danger of being forgotten?
-
- changed status to open
- removed comment
Ok, let us test this. Setting this issue to review. You should then be able to set it to a new state: review_ok.
-
- removed status
- removed comment
-
- changed status to resolved
- removed comment
-
reporter
- removed comment
I know this is after the fact, but would it be desirable/possible to rename the new state "reviewed_ok" (past tense)?
-
- removed comment
done
-
reporter
- edited description
- changed status to closed
- Log in to comment
- Assignee
- –
- Type
- Priority
- Status
- closed
- Component
- Server Infrastructure
- Milestone
- –
- Version
- –
- Votes
- 0
- Watchers
- 0
What do you think should happen if a patch has been reviewed and found to have problems? On the one hand, it could remain in "review", as at least there is a patch available, but on the other hand, it cannot be reviewed. Searching for patches which can be reviewed is not currently possible without finding also the ones with rejected patches. Should we enable TRAC to allow a transition from review back to accepted for this situation? If you are looking for tickets which have patches attached (as this might indicate tickets which can be advanced more easily, even if the patches first need to be fixed), then we could work out how to make a query for tickets with patches. If we can tolerate a small additional complication, we could make the ticket states reflect the actual process, and have "review", "failedreview" and "approved" (with a better name than "failedreview"). "reviewed" does not reflect the fact that tickets with patches in this state have passed the review. "approved" would have the semantics of "please commit".