[Einstein Toolkit] #664: track should have a "reviewed" state for tickets whose attached patch has been reviewed

List overview All Threads
Download

newer

older

[Einstein Toolkit] #836: Improve...

[Einstein Toolkit] #837: valgrind...

Einstein Toolkit

7 Nov 2011 7 Nov '11

4 p.m.

#664: track should have a "reviewed" state for tickets whose attached patch has been reviewed ----------------------------------+----------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: EinsteinToolkit trac | Version: Keywords: | ----------------------------------+----------------------------------------- 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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Show replies by date

Einstein Toolkit

7 Nov 7 Nov

5:10 p.m.

#664: track should have a "reviewed" state for tickets whose attached patch has been reviewed -----------------------------------+---------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: new Priority: minor | Milestone: Component: EinsteinToolkit trac | Version: Resolution: | Keywords: -----------------------------------+----------------------------------------

Comment (by hinder):

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".

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:1 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

5:38 p.m.

Comment (by eschnett):

"failedreview" should be the same as "accepted".

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:2 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

5:48 p.m.

Comment (by rhaas):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:3 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

8 Nov 8 Nov

1:19 a.m.

Comment (by hinder):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:4 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

9:29 p.m.

Comment (by knarf):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:5 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

9 Nov 9 Nov

8:31 a.m.

Comment (by hinder):

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:6 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

9:09 a.m.

Comment (by rhaas):

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

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:7 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

17 Apr 17 Apr

8:52 p.m.

Comment (by knarf):

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:8 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

10:13 p.m.

Comment (by rhaas):

Accepted works for me.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:9 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

18 Apr 18 Apr

7:51 a.m.

Comment (by eschnett):

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:10 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

9:47 a.m.

Comment (by knarf):

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:11 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

12:57 p.m.

Comment (by eschnett):

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?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:12 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

1:16 p.m.

#664: track should have a "reviewed" state for tickets whose attached patch has been reviewed -----------------------------------+---------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: review Priority: minor | Milestone: Component: EinsteinToolkit trac | Version: Resolution: | Keywords: -----------------------------------+---------------------------------------- Changes (by knarf):

* status: new => review

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.

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:13 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

1:17 p.m.

#664: track should have a "reviewed" state for tickets whose attached patch has been reviewed -----------------------------------+---------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: review_ok Priority: minor | Milestone: Component: EinsteinToolkit trac | Version: Resolution: | Keywords: -----------------------------------+---------------------------------------- Changes (by eschnett):

* status: review => review_ok

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:14 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

23 Apr 23 Apr

11:18 a.m.

#664: track should have a "reviewed" state for tickets whose attached patch has been reviewed -----------------------------------+---------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: EinsteinToolkit trac | Version: Resolution: fixed | Keywords: -----------------------------------+---------------------------------------- Changes (by knarf):

* status: review_ok => closed * resolution: => fixed

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:15 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

30 Apr 30 Apr

11:16 a.m.

#664: track should have a "reviewed" state for tickets whose attached patch has been reviewed -----------------------------------+---------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: EinsteinToolkit trac | Version: Resolution: fixed | Keywords: -----------------------------------+----------------------------------------

Comment (by rhaas):

I know this is after the fact, but would it be desirable/possible to rename the new state "reviewed_ok" (past tense)?

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:16 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

Einstein Toolkit

12:23 p.m.

#664: track should have a "reviewed" state for tickets whose attached patch has been reviewed -----------------------------------+---------------------------------------- Reporter: rhaas | Owner: Type: enhancement | Status: closed Priority: minor | Milestone: Component: EinsteinToolkit trac | Version: Resolution: fixed | Keywords: -----------------------------------+----------------------------------------

Comment (by knarf):

done

-- Ticket URL: https://trac.einsteintoolkit.org/ticket/664#comment:17 Einstein Toolkit http://einsteintoolkit.org The Einstein Toolkit

5275

Age (days ago)

5450

Last active (days ago)

trac@lists.einsteintoolkit.org

17 comments

1 participants

tags (0)

participants (1)

Einstein Toolkit