Ticket #2659 (closed defect: fixed)

Opened 7 years ago

Last modified 6 years ago

Remove 'review' field from tickets

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

not applicable

Description

The 'Review' field in tickets is redundant with the states provided by 'Status'. I propose we remove the 'Review' radio button.

Change History

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

The redundancy is not total. It is not possible to determine "selfReviewed" or "notReviewed" from the "Status" alone, although the union of those two could perhaps be inferred from a "new/assigned/inTicketWork -> closed" transition.

I'd be fine with eliminating "notReady", "needsReview", and "needsTest"; having tickets start in "notReviewed"; and, if possible, have the "reviewed" action set the "reviewed" value.

comment:2 Changed 7 years ago by rowen

I would prefer to eliminate this field entirely because I don't believe we learn enough from it to justify the complexity. But K-T's suggestion is better than leaving it as it is.

comment:3 in reply to: ↑ 1 Changed 7 years ago by mjuric

Replying to ktl:

The redundancy is not total. It is not possible to determine "selfReviewed" or "notReviewed" from the "Status" alone, although the union of those two could perhaps be inferred from a "new/assigned/inTicketWork -> closed" transition.

I'd argue that:

  • A ticket that has been merged w/o passing through review is implicitly selfReviewed.
  • A ticket that hasn't yet passed through a review is implicitly notReviewed.

I think the state transitions do tell the whole story.

comment:4 Changed 7 years ago by ktl

State transitions may tell a story, but states do not. I don't think it is possible to produce a report based on state transitions, unfortunately.

comment:5 Changed 7 years ago by mjuric

Unless you're taking care of setting 'review' correctly for each ticket that passes by you, I doubt those reports are accurate. E.g., more often than not I forget to set the review field when switching states, and I doubt I'm the only one.

PS: If we were *really* motivated, we could back out the transitions directly from the database. They do get recorded.

comment:6 Changed 7 years ago by rowen

My main concern is that it is easy to overlook the reviewed field, so I doubt it will always be set correctly (if at all), in which case conclusions based on it are not trustworthy. State transitions are much more robust. From a user's perspective, the simpler we can make the ticketing system, the more likely it will be used correctly and willingly.

If we cannot derive metrics from state transitions, that is a pity. (Is it not practical to derive transitions from a history of states? Does JIRA track state transitions more usefully?)

(Duplicating what Mario said, but I've been trying to post this comment for several minutes now).

comment:7 Changed 7 years ago by ktl

While I do set "review" for tickets that I review, and I've seen RHL use "selfReviewed", I agree that people often get it wrong or ignore it. I surrender.

comment:8 Changed 6 years ago by robyn

  • Owner changed from robyn to igoodenow
  • Status changed from new to assigned

TCT members: KT, Mario, Serge, Russell, and Robyn, discussed this issue on 7 Mar 2013.

KT, who was the only TCT member who was hesitant to remove the field in the pre-meeting ticket discussions, decided that removing the review field would also remove one source of confusion regarding how a ticket is annotated as needing review.

All TCT members present voted to remove the 'review' field from the Ticket Workflow process.

Action item: Robyn will turn this Ticket over to Iain who will effect the removal of the 'review' field.

comment:9 Changed 6 years ago by robyn

Remove the 'reviewstatus' flag by removing the following lines from the trac.ini file:

reviewstatus = radio
reviewstatus.label = Review
reviewstatus.options = notReady|needsReview|needsTest|reviewed|notReviewed|selfReviewed
reviewstatus.order = 2
reviewstatus.value = 0

There is a second trac modification request discussed in #2840 which should be done at the same time. Let me know when you've installed both of these on the trac clone so that I can test them.

comment:10 Changed 6 years ago by igoodenow

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