Ticket #2548 (closed defect: fixed)

Opened 7 years ago

Last modified 7 years ago

Useless ticket state changes

Reported by: price Owned by: robyn
Priority: normal Milestone:
Component: TCT Keywords:
Cc: gpdf@…, ktl@…, mjuric@…, rplante@…, rhl@…, rowen@…, smm@…, robyn Blocked By:
Blocking: Project: LSST
Version Number:
How to repeat:

not applicable

Description

At least some developers feel compelled to make multiple ticket state changes in rapid succession, e.g.:

  • new --> assigned
  • assigned --> inTicketWork
  • inTicketWork --> inStandardsReview

This results in multiple update e-mails for what is, in essence, only a single state change.

Please permit the following state transitions:

  • new --> inTicketWork
  • new --> inStandardsReview
  • assigned --> inStandardsReview

Once these are permitted, the Trac configuration will have to be updated.

Attachments

Workflow rev2012.pdf (29.2 KB) - added by robyn 7 years ago.
Current trac ticket workflow including dashed lines for proposed flows
Workflow rev2013.pdf (24.9 KB) - added by robyn 7 years ago.
Proposed workflow which removes target 'assigned' and includes proposed flows.
WorkflowRev2012V2.pdf (26.4 KB) - added by robyn 7 years ago.
Updated to include new transitions and owndership options
WorkflowRev2013V2.pdf (24.0 KB) - added by robyn 7 years ago.
Update to add new transitions and ownership options
newini.png (91.9 KB) - added by robyn 7 years ago.
Proposed workflow

Change History

comment:1 Changed 7 years ago by mjuric

I vote 'yes' for bringing this up at the TCT at the earliest time convenient.

Changed 7 years ago by robyn

Current trac ticket workflow including dashed lines for proposed flows

Changed 7 years ago by robyn

Proposed workflow which removes target 'assigned' and includes proposed flows.

comment:2 Changed 7 years ago by robyn

I've added two graphics shoiwing the current workflow and a variant:

  • 'workflow rev 2012' is the current ticket workflow with the 3 new flows added in as dashed lines;
  • 'workflow rev2013' is a proposed workflow which removes the 'assigned' target and incorporates the 3 new flows as dashed lines.

The ability to define the appropriate state transitions incorporating relevant global assignments has yet to be examined. Once I determine that the proposed flows are legitimate, I'll call a TCT meeting.

comment:3 follow-up: ↓ 5 Changed 7 years ago by mjuric

I believe Paul was proposing just to add "shortcuts" to inTicketWork and inStandardsReview states; your proposal from the second attachment goes farther than that (removes the 'assigned' state).

Was that intentional? What is the goal?

comment:4 follow-up: ↓ 7 Changed 7 years ago by ktl

(Grr, wrote a long thesis and then had it wiped out due to inadvertent click. Trying again...)

I believe part of the intent of the "assigned" and "inTicketWork" states was to make it possible to discourage tickets from being left in "new" with no (proper) owner that can be held accountable. (Of course, this enforcement task, and other similar tasks supported by the current workflow, has rarely been performed.)

Allowing a "new" to "inStandardsReview" transition seems to make "assigned" and "inTicketWork" completely optional. If they are used at all, I suspect it will be in a manner too inconsistent to make reporting on them useful. I therefore suggest one of these options:

  1. Remove "assigned" and "inTicketWork". Allow "new" to "inStandardsReview" transition.
  2. Remove "inTicketWork" only. Allow "assigned" to "inStandardsReview", but do not allow "new" to "inStandardsReview".

Robyn seems to diagrammed the first half of option 2, keeping "inTicketWork" instead of "assigned", but allowing the "new" to "inStandardsReview" transition.

It feels a little strange to have "inTicketWork" used for "review complete, merge needed". On the other hand, creating a new ticket state for this also seems excessive. Perhaps just reassignment within "inStandardsReview" would be sufficient for this.

With regard to reassignment: Why can't a "needInfo" ticket be reassigned? What does it mean for a "closed" ticket to be reassigned?

If we are rethinking the workflow, I think the "Review" field should also be rethought. Given the states and transitions we have, I think that "notReviewed" and "selfReviewed" are the only really useful values. (Perhaps, if "inStandardsReview" reassignment is adopted, "reviewed" will also be useful.) As long as tickets currently with "needsReview" are moved into the appropriate state, I think this field can be simplified or even removed.

comment:5 in reply to: ↑ 3 Changed 7 years ago by robyn

Replying to mjuric:

I believe Paul was proposing just to add "shortcuts" to inTicketWork and inStandardsReview states; your proposal from the second attachment goes farther than that (removes the 'assigned' state).

Was that intentional? What is the goal?

The removal was intentional. With 'new' gaining the ability to move directly to 'inTicketWork' and 'inStandardsReview' as requested, there doesn't seem to be any reason to transition through 'assigned' at all.

If the initial person handed the Ticket is not correct, the new 'new' flow allows that person to reassign the ownership without changing from the 'new' state.

comment:6 Changed 7 years ago by rhl

I agree with Paul, although I was willing to let this sleeping dog lie until it's replaced by JIRA. Still, we'll learn how to configure JIRA by settling this.

I think that we have one too many state; I'd keep "assigned" and get rid of "inTicketWork". I'd also permit all state transitions, but encourage people to assign a ticket as soon as they notice its existence. Sometimes it makes sense to open a ticket, fix it, and request a review all in one go and I don't see any reason to forbid this --- if you do, people just step through the states and send meaningless email, and it doesn't make them think about what they're doing.

comment:7 in reply to: ↑ 4 Changed 7 years ago by robyn

Replying to ktl:

(Grr, wrote a long thesis and then had it wiped out due to inadvertent click. Trying again...)

I believe part of the intent of the "assigned" and "inTicketWork" states was to make it possible to discourage tickets from being left in "new" with no (proper) owner that can be held accountable. (Of course, this enforcement task, and other similar tasks supported by the current workflow, has rarely been performed.)

I consider it the responsibility of the person receiving a Ticket for which they are not the correct recipient to forward it on to either the System Architect for another redirection or to someone they know is a better match. This handoff was described in the old document describing the Ticket workflow - but I admit I haven't looked at it recently. That document will be revised should the workflow changed.

Allowing a "new" to "inStandardsReview" transition seems to make "assigned" and "inTicketWork" completely optional. If they are used at all, I suspect it will be in a manner too inconsistent to make reporting on them useful. I therefore suggest one of these options:

  1. Remove "assigned" and "inTicketWork". Allow "new" to "inStandardsReview" transition.
  2. Remove "inTicketWork" only. Allow "assigned" to "inStandardsReview", but do not allow "new" to "inStandardsReview".

Robyn seems to diagrammed the first half of option 2, keeping "inTicketWork" instead of "assigned", but allowing the "new" to "inStandardsReview" transition.

I'm now considering 'ne' to have assumed the role of the old 'assigned'. The developer should generally spend some amount of time implementing & testing new or revised code - hence the inTicketWork phase.

'NeedInfo?'/'returnInfo' was to capture time when the Ticket is held until some necessary piece of info/data becomes available. A Ticket's 'block' feature only works when another Ticket's completion is blocking progress. The 'needInfo' state could be abandoned if the developers want to assume the responsibility for poking the secondary developer holding the desired info. Additionally, the 'needInfo' state could reasonably flow to either 'closed' or 'deferred' if the secondary info will never become available. I prefer, though, that the primary developer be notified via the 'infoProvided' flow accompanied by the secondary developer's regrets and then let the primary developer 'close' or 'defer' the Ticket..

It feels a little strange to have "inTicketWork" used for "review complete, merge needed". On the other hand, creating a new ticket state for this also seems excessive. Perhaps just reassignment within "inStandardsReview" would be sufficient for this.

Could be, but I consider inTicketWork to include all work done by the developer. Here is where the 'review' state maintained over the life of the Ticket indicates whether the Ticket is in 'design' review,'standards' review', whatever. The current 'review' state is not very rich at the moment and needs to be updated.

With regard to reassignment: Why can't a "needInfo" ticket be reassigned? What does it mean for a "closed" ticket to be reassigned?

A 'needInfo' state could easily have a 'reassign' flow applied to it.

Regarding the 'closed' -> 'reassign': there have been times when the original Ticket creator wants to grab hold of the Ticket ownership once the issue is resolved. This was requested long ago.

If we are rethinking the workflow, I think the "Review" field should also be rethought. Given the states and transitions we have, I think that "notReviewed" and "selfReviewed" are the only really useful values. (Perhaps, if "inStandardsReview" reassignment is adopted, "reviewed" will also be useful.) As long as tickets currently with "needsReview" are moved into the appropriate state, I think this field can be simplified or even removed.

I agree this needs to be reconsidered with respect to the workflow we want to occur.

It's also necessary to remember that Tickets are created for more than just 'fix a bug', 'implement a feature'. Check out: https://dev.lsstcorp.org/trac/wiki/LsstDCTicketWorkflow for other reasons envisioned: request 3rd party software installation, make a Release. Also read RHL's 5/12 suggested update to the Ticket workflow; it's referenced in the second line of the provided URL.

comment:8 Changed 7 years ago by ktl

Oh, and I definitely need a reassign within "inStandardsReview" in order to pass the ticket to the reviewer. Right now, this becomes "inTicketWork" prematurely, before the review is even started, for both tickets that are "inStandardsReview" and also those that are "new" plus "needsReview".

comment:9 Changed 7 years ago by ktl

Another consideration is what kinds of reports we want to generate. Some of these suggestions could make the following reports impossible to generate reliably, which may or may not be an issue:

  • Which tickets are actively being worked on?
  • How many tickets are waiting on the System Architect for reviewers?
  • Which tickets are waiting to be merged after review?

comment:10 Changed 7 years ago by mjuric

I'd prefer to decouple the larger issue of workflow redesign from the more immediate issue of Paul's request.

I think Paul's request is about acknowledging the facts on the ground that:

  1. When someone sees a new ticket, they may begin working on it right away (new -> inTicketWork transition), and
  2. That the changes are sometimes tiny enough that they can be promptly implemented and sent to review (new -> inStandardsReview, or assigned -> inStandardsReview).

These are not meant to encourage skipping steps, but avoid situations where getting the ticket through the workflow is more work than whatever the ticket is actually about (e.g., a one-line fix, or (in sysadmin context), changing a user's password).

Therefore, a proposal: let's decide on the specific transition that Paul has requested, and continue the discussion on the new workflow independent of that request.

comment:11 Changed 7 years ago by ktl

I just think we know enough at this point about what people need and want to do that we should stop kicking this can down the road (it has been a couple of years, I think) and just do the right thing.

I also want to be able to succinctly tell new developers how to manage tickets -- or, even better, not have to tell them at all because the workflow is so obvious. If there are lots of "understood" exceptions, experienced developers will have no problems, but new ones will be lost in a maze of twisty transitions, all different.

comment:12 Changed 7 years ago by mjuric

Well, right now all the devs we have are the experienced ones (I wish that were not the case :)).

I'm worried that in search of a comprehensive workflow reform, we'll keep deferring potential quick fixes that will make devs happy right away. It takes ~5 minutes to implement Paul's request, assuming TCT agrees it's sensible.

comment:13 Changed 7 years ago by ktl

Recent new developers include: douglas, pgee, yusra, kmsmith, kolsen.

I hope that we can agree on a reasonable simplification of the workflow in less than 15 minutes. But I tend to underestimate.

comment:14 Changed 7 years ago by mjuric

I'd like to have an "escape hatch" -- I propose if we can't agree in those 15 minutes, we adopt Paul's fix. I doubt it'll make things (that much) worse.

comment:15 Changed 7 years ago by rowen

I like Paul's suggestion of being able to go straight to needsReview, but I think RHL's idea is even better: allow all state transitions. Restrictions don't seem to provide us any benefit and only generate intrusive extra state transitions. I agree with Robyn and others that assigned and inTicketWork are redundant and one will no longer be used once we allow going more directly to needsReview.

The current system also seems to have no clean way of closing a ticket after review (the only allowed transition that seems relevant is "close without review") so I'd just call that close.

comment:16 follow-up: ↓ 17 Changed 7 years ago by rhl

Looking at the transitions, if we get rid of Assigned, the normal process is:

New -> InTicketWork -> InStdsReview -> InTicketWork -> Closed

There are two other possible states: Deferred and NeedsInfo whence any state except Closed can go.

That means that usually the ticket is in a state that tells us where we stand, which is KT's desideratum.

Additionally:

  • New can go straight to InStdsReview
  • any state should be able to go to New (so someone else gets it).
  • any state can go to Closed, and Closed can go to any state except Deferred or NeedsInfo

We can simplify these "additional" rules by saying, "All transitions are possible", and I recommend this approach

comment:17 in reply to: ↑ 16 Changed 7 years ago by mjuric

Replying to rhl:

Looking at the transitions, if we get rid of Assigned, the normal process is:

New -> InTicketWork -> InStdsReview -> InTicketWork -> Closed

There are two other possible states: Deferred and NeedsInfo whence any state except Closed can go.

What is the state that signals "yes, this is a problem (or a valid feature request), I acknowledge it's on my plate, but haven't started working on it yet"?

That's what I thought 'Assigned' was (and I'd expect that to be the state a large fraction of open tickets will be in).

comment:18 Changed 7 years ago by rhl

I have no problem adding Assigned just after New provided I can skip it.

comment:19 Changed 7 years ago by price

And if I assign when making a ticket, it should be opened in state 'assigned'.

comment:20 Changed 7 years ago by rowen

Regarding Mario's comment: To my mind, the distinction between "assigned" and "inTicketWork", although it sounds good in theory, turns out to be an awkward mix of project management tools with developer tools. I also doubt we are getting enough value out of it to justify the extra state and extra state transition notifications. But perhaps the distinction is useful for large tickets.

If we do ditch one I favor ditching "inTicketWork" and keeping "assigned" as being simpler to understand and less oriented towards project management. (Since I'd rather keep task reporting in the project management tools).

Paul brings up an interesting point: do we even need "new"?

comment:21 Changed 7 years ago by rhl

I don't think that there's any question we need New; it means "no one has looked at this yet, and maybe no one has even noticed it's existance". Paul asked the question, if the ticket is explicitly assigned to someone when opened, do we still need New?

I think that the answer's, "Yes". You can assign a ticket to rhl, but you can't guarantee that he's noticed. A search for New tickets should reveal very few, and only recently filed ones at that. It doesn't, but that's because a Human needs to take charge of making us follow the rules and keep on top of the bug tracker.

comment:22 Changed 7 years ago by robyn

Don't get caught up in the names of the states. We apply meaning to the names and the transitions. Except for state 'new' - all new tickets start in that state by trac definition. The ticket creator determines what developer initially owns the Ticket.

A new Ticket owner is immediately notified when a Ticket has been bestowed upon him. If he doesn't want it, he should toss it to someone else otherwise it remains his responsibility whether he asked for it or not. So, I'd argue that we don't need a separate 'accept' state.

If a Ticket owner is foolish enough to delay reassignment, then it'll be reported as his until he foists it elsewhere. Perhaps the old emails-of-shame should be resurrected to remind developers of their Ticket collection. This might alleviate the concerns that a Ticket holder might not even know he's been named as Ticket owner.

Changed 7 years ago by robyn

Updated to include new transitions and owndership options

Changed 7 years ago by robyn

Update to add new transitions and ownership options

comment:23 Changed 7 years ago by robyn

During a transition from one state to another, the ownership of the Ticket can be simultaneously modified. The graph doesn't really make that clear.

At the TCT meeting, we'll work from:

  • Workflow rev2012.pdf? Paul's original recommendation
  • WorkflowRev2012V2.pdf? Original plus a few more transitions (but not from all to all)
  • WorkflowRev2013V2.pdf? Drop 'Assigned' and add a few more transitions

comment:24 Changed 7 years ago by rhl

Robyn's https://dev.lsstcorp.org/trac/attachment/ticket/2548/WorkflowRev2012V2.pdf allows 24 out of 36 transitions:

New Work Info Review Defer Closed
New X X X X X
Work X X X X X
Info X X X
Review X X X X
Defer X X X X
Closed X X X

I think some of the 12 are needed:

  • New -> Info (I don't understand the ticket)
  • Work -> New (Assigned to the wrong person)
  • Info -> New (Ah! Now I know who gets it)
  • Info -> Closed (Ah! Not a real problem after all)
  • Closed -> Work (no, it's not fixed after all)

That leaves only 7 out of 36 forbidden transitions.

comment:25 Changed 7 years ago by ktl

I think Robert actually means WorkflowRev2013V2.pdf, without "Assigned". I agree with his proposed additions. I wonder whether "Review" -> "Defer" and vice versa will be used much, but I'm OK with leaving them in. I also wonder if it's worth keeping "Info" for reasons similar to Russell's objections to "inTicketWork" in favor of just a generic "assigned", but I'm OK with that staying if we can't agree in 15 minutes.

comment:26 Changed 7 years ago by robyn

Putting words in his mouth, I don't think that Robert is concerned with how often a transition is used, only if it might ever be used.

The 'new' usual new feature/Bug fix path would be: new (oh, I have a Ticket)-> work (I'm really doing something on it now) -> review (Time to review Prime's work) -> work (Review went OK and time to merge; or didn't go OK and time to address issues) -> close (Finally done with this ticket)

Any other transitions are less usual. I.e. most Ticket don't need additional information from another in order to complete the Ticket. But when they do, the information provider transitions the Ticket back to work and ownership is returned to the information requestor.

comment:27 Changed 7 years ago by robyn

  • Cc gpdf@…, ktl@…, mjuric@…, rplante@…, rhl@…, rowen@…, smm@… added; rhl, mjuric, ktl removed

comment:28 Changed 7 years ago by robyn

TCT meeting resolution:

Robyn will update the trac Ticket workflow based on Trac Ticket Workflow 2012 V2 and more. Not only will Paul's suggested 3 transitions be installed, per Robert's suggestion, all potential transitions will also be installed. This provides maximum flexibility for the Developer at every junction in a Ticket's lifetime and beyond.

This Ticket 2548 will also be used to effect the change.

comment:29 Changed 7 years ago by ktl

As part of this work, I'd like a ticket workflow explanation document (Trac page) to be generated (updated?), including all states and all transitions and when they are expected to be used.

comment:30 follow-up: ↓ 31 Changed 7 years ago by rhl

I can write that document, if Robyn would like.  I'd do a transition table as above with links to the explanations.

comment:31 in reply to: ↑ 30 Changed 7 years ago by robyn

I was going to edit the document: https://dev.lsstcorp.org/trac/wiki/LsstDCTicketWorkflow to include the new transitions. I am using it to determine the missing transitions.

However, you may have a format which is more approachable for Developers so feel free to take this on. Just let me know.

comment:32 Changed 7 years ago by rhl

Take a look at https://dev.lsstcorp.org/trac/wiki/foo for a description of the newly-blessed workflow.

This page is intended to capture essentially all the information in https://dev.lsstcorp.org/trac/wiki/LsstDCTicketWorkflow in a more accessible format.

Last edited 7 years ago by rhl (previous) (diff)

comment:33 Changed 7 years ago by robyn

The ticket-workflow block for the new trac.ini fbased on Robert's specification follows. A clone of the current trac system is being created so the new workflow can be tested in isolation prior to production installation.

The new workflow definition is:

[ticket-workflow]
leave = * -> *
leave.default = 10
leave.operations = leave_status
#--------------------------------------------------------------------------
renew = assigned,inTicketWork,needInfo -> new
renew.operations = set_owner
renew.permissions = TICKET_MODIFY
renew.name = re-new 

renew_deferred = deferred -> new
renew_deferred.operations = set_owner
renew_deferred.permissions = TICKET_MODIFY
renew_deferred.name = resume
#
reopen_new = closed -> new
reopen_new.operations = del_resolution,set_owner
reopen_new.permissions = TICKET_MODIFY
reopen_new.name = reopen 
#
reassign_new = new -> new
reassign_new.operations = set_owner
reassign_new.permissions = TICKET_MODIFY
reassign_new.name = reassign
#--------------------------------------------------------------------------
assign = new,inTicketWork,needInfo -> assigned
assign.operations = set_owner
assign.permissions = TICKET_MODIFY
assign.name = assign
#
assign_deferred = deferred -> assigned
assign_deferred.operations = set_owner
assign_deferred.permissions = TICKET_MODIFY
assign_deferred.name = resume
#
reopen = closed -> assigned
reopen.operations = del_resolution,set_owner
reopen.permissions = TICKET_MODIFY
reopen.name = reopen 
#--------------------------------------------------------------------------
accept = new, assigned -> inTicketWork
accept.operations = set_owner_to_self
accept.permissions = TICKET_MODIFY
accept.name = accept
#
infoprovided = needInfo -> inTicketWork
infoprovided.operations = set_owner
infoprovided.permissions = TICKET_MODIFY
infoprovided.name = haveInfo 
#
requestchanges = inStandardsReview -> inTicketWork
requestchanges.operations = set_owner
requestchanges.permissions = TICKET_MODIFY
requestchanges.name = needChanges 
#
rework = deferred -> inTicketWork
rework.operations = set_owner
rework.permissions = TICKET_MODIFY
rework.name = resume 
#
reopen_inTicketWork = closed -> inTicketWork
reopen_inTicketWork.operations = del_resolution,set_owner
reopen_inTicketWork.permissions = TICKET_MODIFY
reopen_inTicketWork.name = reopen 
#
reassign_inticket = inTicketWork -> inTicketWork
reassign_inticket.operations = set_owner
reassign_inticket.permissions = TICKET_MODIFY
reassign_inticket.name = reassign
#--------------------------------------------------------------------------
needsinfo = new,assigned,inTicketWork -> needInfo
needsinfo.operations = set_owner
needsinfo.permissions = TICKET_MODIFY
needsinfo.name = getInfo
#
reassign_info = needInfo -> needInfo
reassign_info.operations = set_owner
reassign_info.permissions = TICKET_MODIFY
reassign_info.name = reassign
#--------------------------------------------------------------------------
request_review = new,assigned,inTicketWork -> inStandardsReview
request_review.operations = set_owner
request_review.permissions = TICKET_MODIFY
request_review.name = review 
#
resume_review = closed -> inStandardsReview
resume_review.operations = del_resolution, set_owner
resume_review.permissions = TICKET_MODIFY
resume_review.name = reopen 
#
reassign_review = inStandardsReview -> inStandardsReview
reassign_review.operations = set_owner
reassign_review.permissions = TICKET_MODIFY
reassign_review.name = reassign
#--------------------------------------------------------------------------
resolve = new,assigned,inTicketWork,needInfo,deferred -> closed
resolve.operations = set_resolution
resolve.permissions = TICKET_MODIFY
resolve.name = resolve
#
resolve_closed = closed -> closed
resolve_closed.operations = set_owner
resolve_closed.permissions = TICKET_MODIFY
resolve_closed.name = reassign
#--------------------------------------------------------------------------
defer = new,assigned,inTicketWork,needInfo-> deferred
#
reassign_deferred = deferred -> deferred
reassign_deferred.operations = set_owner
reassign_deferred.permissions = TICKET_MODIFY
reassign_deferred.name = reassign
#--------------------------------------------------------------------------
Last edited 7 years ago by robyn (previous) (diff)

comment:34 follow-up: ↓ 35 Changed 7 years ago by ktl

  • Cc robyn added

Can you automatically set the "Review" field to "needsReview" on a transition to "inStandardsReview"?

comment:35 in reply to: ↑ 34 Changed 7 years ago by robyn

Short answer: No, not for the existing custom field.

In order to use the custom-field 'review', a new workflow plug-in needs to be developed to access the separate DB maintained uniquely for custom fields.

However, I did find a ready-made plug-in which provides a selector which then redirects the transition based on the selection. It's seemingly written with DM's review selector in mind.

Example (from the enterprise-review-workflow.ini):

review = in_review -> *
review.name = review as
review.operations = code_review
review.code_review =
approve -> in_QA,
approve as noted -> post_review,
request changes -> in_work

The source is found in trac's: trunk/sample-plugins/workflow/CodeReview.py

Changed 7 years ago by robyn

Proposed workflow

comment:36 Changed 7 years ago by robyn

The new workflow has been tested on the trac clone. Once the documentation on the new workflow is available in a better name than foo, the workflow will be installed into Production use.

comment:37 Changed 7 years ago by mjuric

Renamed it to https://dev.lsstcorp.org/trac/wiki/DM/Policy/TicketWorkflow . I propose you go ahead and implement the change on the main site (and send a note to lsst-data that some instability due to this may occur over the next few hrs).

comment:38 Changed 7 years ago by robyn

  • Project set to LSST

The new workflow has been added to the Production trac system.

comment:39 Changed 7 years ago by robyn

  • Status changed from new to closed
  • Resolution set to fixed
Note: See TracTickets for help on using tickets.