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.

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.

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

Actions could look something like this for a resolved unconfirmed bug: ( ) Move to UNCONFIRMED (reopen) ( ) Move to REOPENED (reopen, confirm) ( ) Move to VERIFIED

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

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

Mine mine all mine!

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?

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.

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

> 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 :-)

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

> 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

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.

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

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

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

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

I have a slightly different but congruent approach. will drop some code patches off tomorrow. (my searching abilities suck Bug 213585 )

I am going to get to writing this, honest. I'll certainly look at anything you might have.

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

Okay can any (in)complete patches be posted?

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

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

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)

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

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.

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

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

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

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

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

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

(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?

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

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.

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

There's Workflow module in CPAN: http://search.cpan.org/dist/Workflow/ I wonder if it is useful for this task?

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

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)

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

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

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

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.

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?

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

(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. ;)

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

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

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

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

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

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

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

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

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

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

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?

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!

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

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

(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!

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.

(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 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

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

Addressing gerv's comments.

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

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

Added to the Bugzilla 3.2 release notes in bug 432331.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.0/ modified t/test_custom_fields.t Committed revision 176. Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/3.6/ modified t/test_custom_fields.t Committed revision 147. Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/3.4/ modified t/test_custom_fields.t Committed revision 119.