101179 - (bz-custstat) Customised statuses and workflow

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P3)

Description

[Matthew Tuck [:CodeMachine]]

Split off from bug #37613.

We need to customise statuses.  This will get rid of an enumeration, but also
allow status renaming of statuses, setting of permissions on transitions, and
arbitrary graphs of statuses.

This is what I currently think:

statuses table:

name
description
{open, nonopen}
{confirmed, unconfirmed, maybeconfirmed}

statustransitions table:

+-------------+--------------+--------------------------------+
| state_now   | avail_states | privs_required                 |
+-------------+--------------+--------------------------------+
| (null)      | UNCONFIRMED  | (null)                         |
| (null)      | NEW          | (canconfirm|editbugs)          |
| UNCONFIRMED | NEW          | (canconfirm|editbugs|owner|qa) |          
| UNCONFIRMED | ASSIGNED     | (editbugs|owner|qa)            |
| UNCONFIRMED | ACCEPTED     | (editbugs|owner)               |
| UNCONFIRMED | RESOLVED     | (editbugs|owner|qa|reporter)   |
| NEW         | ASSIGNED     | (editbugs|owner|qa)            |
| NEW         | ACCEPTED     | (editbugs|owner)               |
| NEW         | RESOLVED     | (editbugs|owner|qa|reporter)   |
| ASSIGNED    | NEW          | (editbugs|owner|qa)            |
| ASSIGNED    | ACCEPTED     | (editbugs|owner)               |
| ASSIGNED    | RESOLVED     | (editbugs|owner|qa|reporter)   |
| ACCEPTED    | NEW          | (editbugs|owner|qa)            |
| ACCEPTED    | RESOLVED     | (editbugs|owner|qa|reporter)   |
| RESOLVED    | REOPENED     | (editbugs|owner|qa|reporter)   |
| RESOLVED    | VERIFIED     | (editbugs|qa|reporter)         |
| RESOLVED    | CLOSED       | (editbugs|owner|qa)            |
| REOPENED    | ASSIGNED     | (editbugs|owner|qa)            |
| REOPENED    | ACCEPTED     | (editbugs|owner)               |
| REOPENED    | RESOLVED     | (editbugs|owner|qa|reporter)   |
| VERIFIED    | REOPENED     | (editbugs|owner|qa|reporter)   |
| VERIFIED    | CLOSED       | (editbugs|owner|qa)            |
+-------------+--------------+--------------------------------+

I ripped this from the old bug, it is not entirely correct, a RESOLVED ->
UNCONFIRMED transition is not present for example.

We need to automatically generate the actions from here, including:

Whether the bug gets confirmed by the transition, whether the resolution needs
to be cleared or asked, etc.

We need to handle automatic transitions somehow.  These include popular vote
(either put one status on the status table for each unconfirmed open status or
have a special transition).

Also, automatic sleep/wakeups like ASLEEP (bug #8527) and REMIND (bug #65402)
could hopefully be in this scheme.  We need to know when it can be put to sleep
manually, when it should occur automatically, when it should wake up
automatically and when it should be able to be woken up manually.

We could do various sanity checks on transitions:

- Prevent automatic transitions from going to open to non-open and vice versa,
and similar sanity checks.
- Prevent going to unconfirmed except from other unconfirmed.

Comment 1

[Matthew Tuck [:CodeMachine]]

Slight errata:  it is permissible to have a transition from a maybeconfirmed
status to unconfirmed status, but such a transition is only followable for
unconfirmed bugs.

Comment 2

[Matthew Tuck [:CodeMachine]]

And for those not following me, unconfirmed bugs are those with everconfirmed =
0, not status = UNCONFIRMED.  That won't change.

Comment 3

[Matthew Tuck [:CodeMachine]]

Actions could look something like this for a resolved unconfirmed bug:

( ) Move to UNCONFIRMED (reopen)
( ) Move to REOPENED (reopen, confirm)
( ) Move to VERIFIED

Comment 4

[Matthew Tuck [:CodeMachine]]

Suggest each status have a column called confirmchange that lists the status to
go to on change of everconfirmed.

This must be 0 for maybeconfirmed statuses, must be the id of an unconfirmed or
maybeconfirmed status for a confirmed status, and must be the id of a confirmed
or maybeconfirmed status for an unconfirmed status.

It is useful to have the capability to unconfirm, both for a manual action, as
well as possibly rechecking confirmation on product move (see bug #92594).

Comment 5

[Matthew Tuck [:CodeMachine]]

Bug #99251 is a use for confirmchange, as is popular vote confirmation.

Comment 6

[Matthew Tuck [:CodeMachine]]

Mine mine all mine!

Comment 7

[Matthew Tuck [:CodeMachine]]

Another thing we need to consider is that each status has a status it will move
to upon reassignment.  Usually this will be the same, but might be different
sometimes, eg reassignment of an ACCEPTED or INPROGRESS bug should go back to
NEW.

That means there are the following special actions: vote confirmation,
reassignment, product move.  Any more?

Comment 8

[Paul Baker]

The use of character fields instead of integer id fields frightens me. The use
of plurals for table names also frightens me but not quite as much. Having said
that I better give my own suggestion right :-) I would recommend tables along
these lines.

status table:

status_id   int
name        char
description varchar

+-----------+-------------+--------------------------------------+
| status_id | name        | description                          |
+-----------+-------------+--------------------------------------+
| 1         | UNCONFIRMED | This bug has recently been added ... |
| 2         | NEW         | This bug has recently been added ... |
| 3         | ASSIGNED    | This bug is not yet resolved, but... |
| ...       | ...         | ...                                  |
+-----------+-------------+--------------------------------------+

statustransition table:

status_id      int ==> status.status_id
avail_status   int ==> status.status_id
privs_required ???

+-------------+--------------+--------------------------------+
| status_id   | avail_status | privs_required                 |
+-------------+--------------+--------------------------------+
| (null)      | 1            | (null)                         |
| (null)      | 2            | (canconfirm|editbugs)          |
| 1           | 2            | (canconfirm|editbugs|owner|qa) |          
| 1           | 3            | (editbugs|owner|qa)            |
| 1           | 4            | (editbugs|owner)               |
| 1           | 5            | (editbugs|owner|qa|reporter)   |
| 2           | 3            | (editbugs|owner|qa)            |
| 2           | 4            | (editbugs|owner)               |
| 2           | 5            | (editbugs|owner|qa|reporter)   |
| ...         | ...          | ...                            |
+-------------+--------------+--------------------------------+

Note the use of id numbers instead of the text name of the status. This way a
status can be renamed without there needing to be code that angerly goes throug
the whole database replacing every instance of RESOLVED to TAKENCAREOF. 
I also left off the two enum columns from the "status" table. I did it because I
don't quite understand what they are used for, so that is my bad. That and
Enum's are bad and only exist in MySQL. No point in creating new features that
are MySQL dependant while there is work going on to make this work with other
DBs at the exact same time.

It would also be good if the privilege system used id numbers instead of text
(in priv_required) but that's probably going to have to be another bug.

Just my two cents.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

I agree with Paul in general, with the exception that I think the "status_id"
column name can be called "id" (this is also consistent with the names of other
id columns in the Bugzilla database schema).

The other thing you will want to do so that you can do joins against privilege
records is to separate transition records from privilege records:

Transitions:

+----+-------------+-----------+
| id | from_status | to_status |
+----+-------------+-----------+
| 1  | (null)      | 1         |
| 2  | (null)      | 2         |
| 3  | 1           | 2         |          
| 4  | 1           | 3         |
| 5  | 1           | 4         |
| 6  | 1           | 5         |
| 7  | 2           | 3         |
| 8  | 2           | 4         |
| 9  | 2           | 5         |
| 10 | ...         | ...       |
+----+-------------+-----------+

Privileges Required:

+---------------+------------+
| transition_id | privilege  |
+---------------+------------+
| 2             | canconfirm |
| 2             | editbugs   |
| 3             | canconfirm |
| 3             | editbugs   |
| 3             | owner      |          
| 3             | qa         |
| ...           | ...        |
+---------------+------------+

Comment 10

[Paul Baker]

> I think the "status_id" column name can be called "id"

Your are right about it being consistant with the current database so we may
want to do that (although that doesn't make it right). Ideally all id columns
would have not just been "id". The reason for that is so you don't have "id"
column collisions when doing joins. At my job a good portion of my time is
helping spec out well designed and normalized database scheme's. One of the
things we avoid like the plague is columns named only "id". Just a
thought...might have to save that one for BZ3 :-)

> The other thing you will want to do so that you can do joins against privilege
> records is to separate transition records from privilege records:

Good call on that, I was trying to think of better way to do that as well, looks
like you did it for me :-)

Comment 11

[Myk Melez [:myk] [@mykmelez]]

>Ideally all id columns
>would have not just been "id". The reason for that is so you don't have "id"
>column collisions when doing joins.

I agree in general, but in Bugzilla's case the vast majority of references to id
columns do not have problems with join collisions, so the convenience of using
the shortened name outweighs the convenience of more easily avoiding join
collisions.

One more question: why the problem with plural table names?  Plural seems more
logical than singular to me, since a table is a collection of objects.

Comment 12

[Paul Baker]

> why the problem with plural table names?

1. as databases are used more and more as storage for objects instead of just
raw data, it's more crucial that table names be the same as class names. You
would have a 'bug' class, not a 'bugs' class. If you did have a 'bugs' class,
the object would probably be a list of bug objects. Want a wide example of this?
Look to mysql itself. It's privilege system uses tables named 'user' not
'users', 'db' not 'dbs', 'host' not 'hosts'.

2. confusion on plural form of nouns. if you had a person class, should it be
stored in a table called 'persons' or 'people'. How about we just stick with
'person'.

3. 'bug' is less typing than 'bugs'. if we are using 'id' to save space from
having to type 'bug_id' why all the extra s's from plural table names?

4. table.column form. "SELECT bug.id ..." is nicer looking than "SELECT bugs.id
..." Reads better too.

That is just a few, I probably could go on longer but I don't want to sound too
much like an elitest :-P

Comment 13

[Matthew Tuck [:CodeMachine]]

I don't think anyone was ever really proposing using string keys, we are rapidly
exorcising that stuff from the code base, not adding to it.  It was more of a
notational convenience.

Comment 14

[Matthew Tuck [:CodeMachine]]

*** Bug 126211 has been marked as a duplicate of this bug. ***

Comment 15

[Matthew Tuck [:CodeMachine]]

Need to consider:

- whether to allow substates (ie like resolution) for open states
- whether to specify a transition can only occur when certains resolution are
present

Comment 16

[Joel Peshkin]

Also need to consider permitting the status transitions to be defined either
globally for the site or per-product.

Comment 17

[Jeff Hedlund]

*** Bug 213585 has been marked as a duplicate of this bug. ***

Comment 18

[Jason Pyeron]

I have a slightly different but congruent approach.

will drop some code patches off tomorrow. (my searching abilities suck Bug 
213585 )

Comment 19

[Matthew Tuck [:CodeMachine]]

I am going to get to writing this, honest.

I'll certainly look at anything you might have.

Comment 20

[Dave Miller [:justdave]]

*** Bug 223708 has been marked as a duplicate of this bug. ***

Comment 21

[Jason Pyeron]

Okay can any (in)complete patches be posted?

Comment 22

[Dave Miller [:justdave]]

Taking Jake's bugs...  his Army Reserve unit has been deployed.

Comment 23

[Dave Miller [:justdave]]

*** Bug 231564 has been marked as a duplicate of this bug. ***

Comment 24

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 25

[Dave Miller [:justdave]]

*** Bug 254054 has been marked as a duplicate of this bug. ***

Comment 26

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 27

[Matthew Tuck [:CodeMachine]]

There are no incomplete patches for this, and won't be until bug #94534 is done.

Comment 28

[Timothee Besset]

ya know. after several years waiting for this, I don't think I care anymore..

Comment 29

[Kevin Benton]

(In reply to comment #9)
> I agree with Paul in general, with the exception that I think the "status_id"
> column name can be called "id" (this is also consistent with the names of other
> id columns in the Bugzilla database schema).
> 
> The other thing you will want to do so that you can do joins against privilege
> records is to separate transition records from privilege records:
> 
> Transitions:
> 
> +----+-------------+-----------+
> | id | from_status | to_status |
> +----+-------------+-----------+
> | 1  | (null)      | 1         |
> | 2  | (null)      | 2         |
> | 3  | 1           | 2         |          
> | 4  | 1           | 3         |
> | 5  | 1           | 4         |
> | 6  | 1           | 5         |
> | 7  | 2           | 3         |
> | 8  | 2           | 4         |
> | 9  | 2           | 5         |
> | 10 | ...         | ...       |
> +----+-------------+-----------+
> 
> Privileges Required:
> 
> +---------------+------------+
> | transition_id | privilege  |
> +---------------+------------+
> | 2             | canconfirm |
> | 2             | editbugs   |
> | 3             | canconfirm |
> | 3             | editbugs   |
> | 3             | owner      |          
> | 3             | qa         |
> | ...           | ...        |
> +---------------+------------+
> 
> 

How about...

transition_privs:

+-------------+-----------+--------------------+
| from_status | to_status | privilege_required |
+-------------+-----------+--------------------+
| (null)      | 1         | (null)             |
| (null)      | 2         | canconfirm         |
| (null)      | 2         | editbugs           |
| 1           | 2         | canconfirm         |
| 1           | 2         | editbugs           |
| 1           | 2         | owner              |
| 1           | 2         | qa                 |
| ...         | ...       | ...                |
+-------------+-----------+--------------------+

Then, determining permissions would be as simple as

SELECT privilege_required FROM transition_privs WHERE from_status = ? AND
to_status = ? AND privilege_required IN ..."

Then, if $sth->rows > 0 - allow it, otherwise deny it.

Now you have a fully dynamic permission set.

Comment 30

[Frédéric Buclin]

let's "try" to get it for 2.24 (I'm a dreamer :)

Comment 31

[A. Karl Kornel]

Well, somebody who will not be named decided to make me interested in this bug, and since nobody seems to be working on it I might as well give it a try. 8-)

More soon...

Comment 32

[Frédéric Buclin]

*** Bug 317567 has been marked as a duplicate of this bug. ***

Comment 33

[Douglas Calvert]

(In reply to comment #31)
> Well, somebody who will not be named decided to make me interested in this bug,
> and since nobody seems to be working on it I might as well give it a try. 8-)
> 
> More soon...
> 
I apologize if this is an ignorant question...but:
How is this depending on bug 94534 and yet bug 94534 is unassigned? 

Comment 34

[A. Karl Kornel]

(In reply to comment #33)
> I apologize if this is an ignorant question...but:
> How is this depending on bug 94534 and yet bug 94534 is unassigned? 

Bug 94534 is actually assigned to somebody.  The status is still NEW either because the assignee has not officially accepted the bug, or because they are not actively working on it.  The latter case is probably true here, since bmo has a fake user that we assign bugs to when nobody wants to work on it.  I would simply guess that the assignee has not had time to work on it 8-)

On another note, this bug may not actually depend on bug 94534...

Comment 35

[Joel Peshkin]

One implementation suggestion that should help reduce the number of collisions with other patches landing and keep the patches small and reviewable.  If you anticipate this being an issue, I suggest you break this into a sequence....

1) Create functions that know all the right answers for questions like.... "what statuses can be next" as a function of current status, (and possibly product and user).  Those method should probably accept a hash as an argument { currentstatus => "NEW", user => $user } ....

2) Change all the places in the code to use those methods (this could be done in one patch or in a series of smaller ones)

3) Create the schema and admin cgis to make the statuses configurable

4) Update the common objects and admin cgis to permit customization on a sitewide and per-product basis.

Comment 36

[Frédéric Buclin]

(In reply to comment #33)
> How is this depending on bug 94534 and yet bug 94534 is unassigned? 

Unless I'm "nobody", this bug is assigned to someone.

karl, we need to discuss about this bug and bug 94534 on IRC to coordinate things. But I won't touch it while the trunk is frozen.

Comment 37

[Harri Vartiainen]

There's Workflow module in CPAN: http://search.cpan.org/dist/Workflow/

I wonder if it is useful for this task?

Comment 38

[Marc Schumann [:Wurblzap]]

Workflow-wise, I think that this is what we're up against:
http://manual.mantisbt.org/manual.page.descriptions.system.management.pages.manage.configuration.workflow.thresholds.php
http://manual.mantisbt.org/manual.page.descriptions.system.management.pages.manage.configuration.workflow.transitions.php

And regarding status set customization:
http://manual.mantisbt.org/manual.customizing.mantis.customizing.status.values.php

Comment 39

[jessn]

Just an idea that came to my mind. 

Did you consider the possibility of working with multiple workflow i.e. different products are using different workflows. Often different projects are using different workflows depending on i.e. the size of the projects amoung others.

What about the following?

1) By default a "master workflow" is chosen, which only can be modified by the admin. An inheritance of the master workflow can be made per product, which can be modified by the product_admin / admin or the product_admin / admin can make a brand new work flow from scratch.

2) A "workflow library", which contains different kind of workflows. The workflow library is made by the admin and is provided to help the product_admin. The product_admin can either use the workflows from the library as is or make a customized inheritance of them per product.

3) The admin can "promote" a customized workflow to be a template in the "workflow library" or even choose it to be the standard workflow used for any new products. (See 1)

Comment 40

[jessn]

Regarding my last comment, the idea is ditto for statuses.

Comment 41

[Max Kanat-Alexander]

*** Bug 333096 has been marked as a duplicate of this bug. ***

Comment 42

[Frédéric Buclin]

*** Bug 335787 has been marked as a duplicate of this bug. ***

Comment 43

[A. Karl Kornel]

OK, time for the action plan.  This is going to be a fun multi-step process, which will hopefully go as follows:

1) Create Bugzilla::Workflow object, with whatever methods are needed.  Each method in Bugzilla::Workflow will be created as it is needed, with files having their code modified one at a time or in small batches.  These new methods will only reproduce current functionality.  You won't get new workflows yet!

2) Introduce the DB structure to support 1 workflow.  Once the DB structure is in place, INSERT all of the records required to represent the default workflow.  Modify the code in Bugzilla::Workflow to reference the database.

3) Move all of the code dealing with spontaneous transitions into Bugzilla::Workflow (or a sub-module).  This is where the enough-votes-confirms-a-bug code is covered (which can happen from votes.cgi or editproducts.cgi).

4) Introduce an interface for showing/editing workflow.

5) Introduce the ability to add new workflows.  Introduce code to handle migrating products from one workflow to another.  Introduce code to handle moving bugs from a product in one workflow to a product in another workflow.

Comment 44

[Kevin Benton]

If you don't mind my addition, I think that custom workflows would work best if we had both a status and substatus.  The status table would contain the status id, name, and description.  The substatus table would contain an id, name, parentid, and description.  The statusperms table would contain statusid, substatusid and groupid (authorizing transitions to these new statuses).  A statustransition table would contain the following:

fromstatusid (mediumint)
fromsubstatusid (mediumint)
tostatusid (mediumint)
tosubstatusid (mediumint)
allow_on_create (boolean - overrides statusperms assuming the user is authorized to get to the new bug form and see the product)
autochange_owner_to enum('none', 'reporter', 'qacontact', 'defaultowner', 'luser')

To determine which statuses were available, a select would be made using the fromstatusid and fromsubstatusid to locate which statuses the current was allowed to transition to.  If the tosubstatus is NULL, any allowable substatus is acceptable.  When a bug is being created, the user should be offered statuses that have allow_on_create set to true.  The autochange_owner_to field sets the assigned_to field as documented (no change, reporter, qacontact, default owner for the product/component, or the logged-in user respectively).

This would allow admins to implement their own "Pending" status with an associated substatus (i.e. 'Customer Feedback'), as well as allowing us to keep the existing Resolution (renamed to substatus).  This would also allow for New/Acknowledged as well as New/NoAck.

Thoughts?

Comment 45

[Kevin Benton]

(In reply to comment #44)
> allow_on_create (boolean - overrides statusperms assuming the user is
> authorized to get to the new bug form and see the product)

Correction - does not override so that some users are not allowed to set "higher privilege" statuses.

Comment 46

[Gunnar Wagenknecht]

(In reply to comment #44)
> The status table would contain the status id, name, and description.  
> The substatus table would contain an id, name, parentid, and description.

You could simply store this in one table STATUS(id,name,parentid,description) and easily build a deep hierarchy of status.

ROOT (id 0)
 |-NEW
 |  |-ACK
 ..
 |-RESOLVED
 |  |-FIXED
 |  |  |-BRANCH1
 |  |  |-BRANCH2
 ...

This would be very extensible but might be challenging for the implementation. ;)

Comment 47

[Frédéric Buclin]

(In reply to comment #46)
> You could simply store this in one table STATUS(id,name,parentid,description)
> and easily build a deep hierarchy of status.

No, this doesn't work. A status can have several parents. Duplicating the description and name is a very bad table formatting.

Comment 48

[A. Karl Kornel]

(In reply to comment #44)
> <<<snip>>>

Based on what you said, it seems like resolutions would almost take care of this.  However, I accept that you probably have a situation that falls into the "almost" part of what I said.

Given that, I think what you are looking for could be implemented easily enough once this bug has landed.

Comment 49

[Gunnar Wagenknecht]

(In reply to comment #47)
> (In reply to comment #46)
> > You could simply store this in one table STATUS(id,name,parentid,description)
> > and easily build a deep hierarchy of status.
> 
> No, this doesn't work. A status can have several parents. Duplicating the
> description and name is a very bad table formatting.

Uh. I wasn't aware that a status can have several parents. I don't know the current model I just use Bugzilla so this may be a stupid question. Is it currently implemented that way?

I'm wondering why a status can have more than one parent. This would mean that you could have the following tree.

ROOT
ROOT-STATUS(A)
ROOT-STATUS(A)-SUB(1)
ROOT-STATUS(A)-SUB(2)
ROOT-STATUS(B)
ROOT-STATUS(B)-SUB(2)
...

So SUB(2) would have two parents: STATUS(A) and STATUS(B). But from an entity point of view this is wrong. ROOT-STATUS(B)-SUB(2) is a different entity then ROOT-STATUS(A)-SUB(2). It might have the same name but most likely it means different things and thus, would have a different description.

Comment 50

[Gunnar Wagenknecht]

(In reply to comment #49)
> So SUB(2) would have two parents: STATUS(A) and STATUS(B). But from an entity
> point of view this is wrong. ROOT-STATUS(B)-SUB(2) is a different entity then
> ROOT-STATUS(A)-SUB(2). It might have the same name but most likely it means
> different things and thus, would have a different description.

Anyway, instead of defining the parent relation it's also possible to define the child relations. But this would require a mapping table (SUBSTATUS[parentid,childid]) for a M to N relation.

Comment 51

[Frédéric Buclin]

*** Bug 356286 has been marked as a duplicate of this bug. ***

Comment 52

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 53

[Frédéric Buclin]

Attachment: Work in progress, v0.4

Works fine when changing one bug at once. I didn't check yet whether it still works correctly when changing several bugs at once.

Also, I still have to update templates so that they look at existing actions and display those valid for the current bug(s).

And of course, a UI allowing you to add/edit/delete actions.


Note to self: process_bug.cgi is a big hack. :)

Comment 54

[Frédéric Buclin]

Attachment: patch, v1

Here is a working patch which implement basic support for custom statuses and workflow. The UI to add new statuses and edit the workflow will come in a separate patch (you can do it directly from the mysql commandline for now).

There are several things to fix to have it fully implemented with all you want to have fun (but which I will do in separate bugs):

- Use get_status() instead of status_descs in all templates (this patch only uses it once, to show you how hard it is to use ;) );
- Remove the BUG_STATE_OPEN constant in favor of Bugzilla::Bug->get_open_states() which will look at open bugs in the DB directly (I added a 'is_open' attribute to the bug_status table);
- Implement the UI to choose the correct action in show_bug.cgi based on the status of the bug and the custom workflow (the backend is already fully implemented in this patch);
- Implement the UI to add new statuses (trivial, I wrote it already, but I will keep it for another patch);
- Implement the UI to customize the workflow (also trivial, but would make this patch too big to review);
- Fix coming regressions as such a feature will necessarily have some.

About triggers, this will be the very last part of the process. So stuff like "I want the Severity field to be 'minor' if the status is NOT_SO_IMPORTANT" will come at the very end.

Comment 55

[Max Kanat-Alexander]

Comment on attachment 258158 [details] [diff] [review]
patch, v1

Okay, wow. This patch is huge.

First off, thanks for writing it. :-)

I'd like to do a bit less in this initial bug than this patch is doing. All I really want to see is being able to add new open states, like NEW or ASSIGNED. Then we can add one extra complicated bit at a time. So maybe we can split this patch into several smaller patches, and I'd be happy to review those individually.

Comment 56

[Joe Ogulin]

This seems to be the best place to add this:

The lifecycle flow is far too minimal.  We do not consider a bug "resolved" until it's ready for the code to be committed to the source code management trunk.  What does not exist is a status that would equate to "Peer review," or "System Architecht review," or something similar.  Alternatively, a subordinate status field would help so that when a bug is in an ACCEPTED state, you can set the subordinate field to "IN PROGRESS," "NEEDS REVIEW," "REWORK," or "ACCEPTED." (Or pick whatever names you like, or make them customizable.)

There is also a need for pointing a custom drop-down field at a specific, already existing list like the user list.  Why should I have to retype all of the names in there and maintain two copies of the list?  Having this, it would be easy to set the peer review person (or people).

Comment 57

[Frédéric Buclin]

(In reply to comment #55)
> I'd like to do a bit less in this initial bug than this patch is doing. All I
> really want to see is being able to add new open states, like NEW or ASSIGNED.

We first have to fix the backend before letting admins add new statuses.

Comment 58

[Greg Hendricks]

I wasn't able to find this anywhere else and this may be something for a separate bug, but would it not make sense to have roles associated with custom statuses? For example, with NEEDINFO we have a role called Infoprovider - the person the information is being requested from. This allows users to search for bugs that they might not otherwise be directly associated with as the Assignee or QA contact for instance. 

I could also see the need to have different approvers for different state changes. True that flags provide some level of this now, but this would consolidate the workflow significantly. 

Of course, talk is cheap, and I have no idea how to go about implementing this, but it seems doable. Thoughts?

Comment 59

[Frédéric Buclin]

Attachment: patch, v2

This patch depends on bug 369987. This is the very last patch in the implementation of bug status workflow and customization. The implementation is now complete and fully functional. Have fun!

Comment 60

[Albert Ting]

Minor nit, but shouldn't the admin page have a "Workflow' section?

Comment 61

[Frédéric Buclin]

(In reply to comment #60)
> Minor nit, but shouldn't the admin page have a "Workflow' section?

Sure! It has one already, see bug 345100.

Comment 62

[Albert Ting]

(In reply to comment #61)
> (In reply to comment #60)
> > Minor nit, but shouldn't the admin page have a "Workflow' section?
> 
> Sure! It has one already, see bug 345100.
> 

Duh, thanks!

Comment 63

[Kevin Benton]

Re your patch (#2), LpSolit,

+    [% IF field.name == "bug_status" %]
+      <tr>
+        <th align="right"><label for="is_open">Open Status:</label></th>
+        <td>
+          <input type="checkbox" id="is_open" name="is_open" value="1" checked="checked">
+          Note: you can only set the open/close attribute now. You cannot edit it later!
+        </td>
+      </tr>
+    [% END %]

Seems to me that a r- may be appropriate unless someone is committed to pulling this code out later.  As I see it, if an administrator puts a status in incorrectly by accident, we shouldn't make them manhandle the SQL to fix it later.  If the admin accidentally commits a status change contrary to what the system needs, there ought to be a way to fix it as long as no bugs are currently in the status in question.

+    [% IF field.name == "bug_status" %]
+      <tr>
+        <th align="right"><label for="is_open">Open Status:</label></th>
+        <td>
+          <input type="checkbox" id="is_open" name="is_open" value="1"
+                 [% IF is_open %]checked="checked"[% END %] disabled>
+          Note: you cannot edit the open/close attribute of existing [% terms.bug %] statuses.
+        </td>
+      </tr>
+    [% END %]

Same here.  I wonder - shouldn't that be s/disabled/readonly/ ?  I could be wrong.

Of course, I'm not max or gerv, but other than these two comments, it looks like an r+ from my end.

Comment 64

[Frédéric Buclin]

(In reply to comment #63)
> Seems to me that a r- may be appropriate unless someone is committed to pulling
> this code out later.  As I see it, if an administrator puts a status in
> incorrectly by accident, we shouldn't make them manhandle the SQL to fix it
> later.  If the admin accidentally commits a status change contrary to what the
> system needs, there ought to be a way to fix it as long as no bugs are
> currently in the status in question.

No. If no bugs use this status, then all you have to do is to delete this bug status and re-create it again with the correct open/close attribute. When you create a new custom field with the incorrect type, you also cannot change it anymore. This avoids a lot of trouble.

Comment 65

[Gervase Markham [:gerv]]

Comment on attachment 266251 [details] [diff] [review]
patch, v2

> # Alternatively, a list of non-editable values can be specified.
> # In this case, only the sortkey can be altered.
> my %static;
>+$static{'bug_status'} = ['UNCONFIRMED'];

So for the moment, we are making UNCONFIRMED a special status - but it's the only one that's special?

Given that we have code that treats it as special, that's fair enough, although we should work towards eliminating that.
 
> [% PROCESS global/footer.html.tmpl %]
>Index: template/en/default/admin/fieldvalues/create.html.tmpl
>===================================================================
>+    [% IF field.name == "bug_status" %]
>+      <tr>
>+        <th align="right"><label for="is_open">Open Status:</label></th>

While a choice of two things is often a checkbox, I think in this case it's not a "Yes/No", and therefore a two-option radiobutton is more appropriate:

Status Type:  (o) Open
              ( ) Closed (requires a Resolution)

>+        <td>
>+          <input type="checkbox" id="is_open" name="is_open" value="1" checked="checked">
>+          Note: you can only set the open/close attribute now. You cannot edit it later!

I suggest:

"Note: the open/close attribute can only be set now, when you create the status. It cannot be edited later."

BTW, I think this is a perfectly reasonable restriction.

>Index: template/en/default/admin/fieldvalues/edit.html.tmpl
>===================================================================
>+    [% IF field.name == "bug_status" %]
>+      <tr>
>+        <th align="right"><label for="is_open">Open Status:</label></th>
>+        <td>
>+          <input type="checkbox" id="is_open" name="is_open" value="1"
>+                 [% IF is_open %]checked="checked"[% END %] disabled>
>+          Note: you cannot edit the open/close attribute of existing [% terms.bug %] statuses.

Instead of using a disabled check box, and explanatory text, just use the following:

Status Type: Open
or
Status Type: Closed

If you agree with my UI suggestions, r=gerv, otherwise we can discuss it.

Gerv

Comment 66

[Frédéric Buclin]

(In reply to comment #65)
> So for the moment, we are making UNCONFIRMED a special status - but it's the
> only one that's special?

Yes, due to the everconfirmed attribute of bugs. We couldn't find an agreement on IRC if we should keep this attribute or not, so I prefer to keep it here for now. But I would be happy to kill it later. :)


> a "Yes/No", and therefore a two-option radiobutton is more appropriate:
> 
> Status Type:  (o) Open
>               ( ) Closed (requires a Resolution)

Good idea!


> I suggest:
> 
> "Note: the open/close attribute can only be set now, when you create the
> status. It cannot be edited later."

OK.

> Instead of using a disabled check box, and explanatory text, just use the
> following:
> 
> Status Type: Open
> or
> Status Type: Closed

Heh, even better. So you suggest to not display the explanatory text at all?


> If you agree with my UI suggestions, r=gerv, otherwise we can discuss it.

OK, I will update my patch. Thanks for the review.

Comment 67

[Frédéric Buclin]

Attachment: patch, v2.1

Addressing gerv's comments.

Comment 68

[Frédéric Buclin]

Here we go \o/

Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.21; previous revision: 1.20
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.132; previous revision: 1.131
done
Checking in template/en/default/admin/fieldvalues/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/fieldvalues/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/admin/fieldvalues/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/fieldvalues/created.html.tmpl,v  <--  created.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/admin/fieldvalues/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/fieldvalues/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.214; previous revision: 1.213
done

Comment 70

[Frédéric Buclin]

Attachment: doc patch, v1

Comment 71

[Colin Ogilvie [:cso]]

Comment on attachment 301762 [details] [diff] [review]
doc patch, v1

r=me with the following fixes:

>+    <para>
>+      The bug status workflow is no longer hardcoded but can be freely customized
>+      from the web interface. Only one bug status cannot be renamed nor deleted,
>+      UNCONFIRMED, but even the workflow involving it is free. The configuration

I would remove the 'even'.

>+      page displays all existing bug statuses twice, with a logic "From... to...".

The 'logic "From... to..." bit doesn't make sense to me. Might be best to explain that From is along the top and To is down the left (or switch as appropriate).

>+      If the checkbox is checked, then the transition between the two bug statuses
>+      is legal, else it's forbidden independently of your privileges. The bug status
>+      used for the "duplicate_or_move_bug_status" parameter must be part of the
>+      workflow because that's the bug status which will be used when duplicating or
>+      moving a bug, and so it must be available from each bug status.

I'd go for 'as that is' instead of 'because that's' and lose the 'and' in the last bit.

Comment 72

[Frédéric Buclin]

I fixed your comments on checkin:

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.85; previous revision: 1.84
done

Comment 73

[Max Kanat-Alexander]

Added to the Bugzilla 3.2 release notes in bug 432331.

Comment 74

[Frédéric Buclin]

Attachment: selenium test, v1

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.0/
modified t/test_custom_fields.t
Committed revision 176.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/3.6/
modified t/test_custom_fields.t
Committed revision 147.

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/3.4/
modified t/test_custom_fields.t
Committed revision 119.