287326 - (bz_select) Ability to add custom single-select fields to a bug

Status: RESOLVED FIXED

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

Description

[Max Kanat-Alexander]

After plain-text fields, a single-select field is probably the easiest type of
custom field to implement. We already will have the code in place to deal with
this type of field by the time we finish bug 287322, all this bug should require
is putting an interface on it.

Comment 1

[Max Kanat-Alexander]

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

Comment 2

[Jochen Wiedmann]

Attachment: Patch introducing an abstract view of enumerations

The attached patch introduces a class Bugzilla::Enum. This class can both be
used to access the builtin enumerations (op_sys, bug_severity, ..., see bug
287311) and the custom enumerations, which are required for this bug.

Further work on both bugs is possible, if the attached patch is landed.

Comment 3

[Max Kanat-Alexander]

Jochen: You just requested review *from* yourself. Perhaps you meant to ask it
to a reviewer? You can see the details on the process at:

http://www.bugzilla.org/docs/contributor.html

Comment 4

[Max Kanat-Alexander]

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

Comment 5

[Max Kanat-Alexander]

Comment on attachment 193864 [details] [diff] [review]
Patch introducing an abstract view of enumerations

OK, I have a few general comments without going into depth.

First off, this needs to be Bugzila::Field::Enum, and it needs to be a subclass of a general Bugzilla::Field class.

The table probably wants to be named custom_enum_value, just for clarity.

Also, we want to modify the fielddefs table to be able to hold the field type, which is done by another patch, in the "plain text fields" bug.

Also, we don't usually declare more than one package in a single file, so the various packages should probably be split up.

"ThrowCodeError" should be used instead of "die."

You don't ever want to use SELECT *.

The Developer's Guide is undergoing an update so that it actually aligns with our real coding standards, so I apologize if any of this wasn't clear before. You can see the current Work-In-Progress on the new DevGuide at:

http://landfill.bugzilla.org/bugzilla.org/docs/developer.html

Comment 6

[Jochen Wiedmann]

Max, I do understand and appreciate most of your comments. However, I strongly disagree in one point:

The Bugzilla::Enum class should *not* be a Bugzilla::Field::Enum class. The fact, that an enumeration is being used should be clearly distinguished from the ability to manage the enumeration. In other words, an implementation of Bugzilla::Field::Enum should *use* Bugzilla::Enum. Let me quote the developers guide: "Code should be as simple as possible." Mixing these classes makes them more difficult.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> The Bugzilla::Enum class should *not* be a Bugzilla::Field::Enum class. The
> fact, that an enumeration is being used should be clearly distinguished from
> the ability to manage the enumeration.

  OK, I see your point. I think that since Bugzilla::Field is not yet an object, there's just a slight unclarity in its purpose. Bugzilla::Field actually represents a Bug Field, not a Rendered Field.

  I do understand that perhaps some day we'd want an Enum that's not a Field, although I can't think of any time we'd actually want that. Usually we use constants for that, since it's simpler.

  So, that is to say, every time we have an Enum in Bugzilla, it will be a Field.

  In any case, either way that it goes, the standard fields (OP_SYS, etc.) should defintely be in the Bugzilla::Field namespace, and anything having to do with their association with a bug should be in the Bugzilla::Field namespace.

Comment 8

[Max Kanat-Alexander]

Also, just to let you know, there's no way that I can make a final decision on this bug until many previous, blocking bugs are resolved. Also, I need the plain-text custom fields bug to go in first. It's just an issue of making sure that we get these features added in the right order.

Comment 9

[Jochen Wiedmann]

Max, I do get your point, but to me that means that I can continue my work not before whatever future version. I think it's unlikely, that I'll be ready to work on that stuff at the time. Consequently, I do give this bug back to the default assignee.

Comment 10

[Frédéric Buclin]

I think what we have to do is:
1. Move op_sys, rep_platform & co tables into the new field_legal_values table, and fix editvalues.cgi and Field::get_legal_field_values() accordingly. This should be done in a separate bug blocking this one.
2. Let editvalues.cgi select all fields which are of type FIELD_TYPE_SINGLE_SELECT or FIELD_TYPE_MULTIPLE_SELECT and allow the admin to edit these field values. This means that op_sys, etc... would have one of these types.
3. Add the ability in editfields.cgi to select the FIELD_TYPE_SINGLE_SELECT type. In order to edit legal values for fields of this type, editfields.cgi would have a link to editvalues.cgi, in the same way that editproducts.cgi has links pointing to editcomponents.cgi, editversions.cgi and editmilestones.cgi to edit theses values.
4. Unless already done during the first step, routines in editvalues.cgi should be moved into Field.pm.

Comment 11

[Frédéric Buclin]

Oh, and of course:
1': fix post_bug.cgi and process_bug.cgi to validate values in a centralized way.

Comment 12

[Max Kanat-Alexander]

Yeah, that all sounds fine, except the table should be called field_legal_value, not field_legal_valueS. (New tables should always be singular. The fact that any tables are plural is merely a historical accident.)

For 2, you don't have to let it edit MULTIPLE_SELECT fields until we have those.

Comment 13

[Bill Barry]

For some future bug, would it be possible that these fields are per-product like milestone, version and component? Perhaps if the custom field has no possible values for the given product they wouldn't appear. Eventually it might be good to make hardware and OS into these fields (they simply don't make sense in many cases; examples: safari, freebsd, kde, any MFC application).

For an example that we could put into the documentation of how this works we could walk through creating some sort of documentation workflow with these inputs (and values):
documentation-status
  ---
  Requested
  In process
  Waiting
  Resolved
  Verified
  Closed

documentation-Resolution
  ---
  Unnecessary
  WorksForMe
  WontFix
  Blockerwillfix
  Fixed

Comment 14

[Max Kanat-Alexander]

(In reply to comment #13)
> For some future bug, would it be possible that these fields are per-product
> like milestone, version and component? 

  Yeah. See bug 106592.

Comment 15

[Max Kanat-Alexander]

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

Comment 16

[Frédéric Buclin]

Attachment: part 1, v1

Max, here is the first part of the patch as discussed on IRC. I will let you implement $dbh->bz_add_field_table() and fix Search.pm.

When your part will be written, the last step will be to fix editvalues.cgi to accept custom fields of type FIELD_TYPE_SINGLE_SELECT.

Comment 17

[Frédéric Buclin]

Comment on attachment 235581 [details] [diff] [review]
part 1, v1

ok, I forgot to include comment 11. Max, feel free to have a look at the existing patch meanwhile.

Comment 18

[Max Kanat-Alexander]

(In reply to comment #11)
> Oh, and of course:
> 1': fix post_bug.cgi and process_bug.cgi to validate values in a centralized

  You might want to wait until bug 349741 lands, since that will fix post_bug, for the most part. Or at least, it will make the fix even easier.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 235581 [details] [diff] [review]
part 1, v1

>-          <option value="FIELD_TYPE_FREETEXT">Free Text</option>
>+          <option value="[% constants.FIELD_TYPE_FREETEXT FILTER html %]">Free Text</option>
>+          <option value="[% constants.FIELD_TYPE_SINGLE_SELECT FILTER html %]">Single Select</option>

  Admins are more familiar with the term "Drop Down" than "Single Select."


  Everything else looks fine. I was thinking it would be nice to put the <select> fields up where "Priority" currently is, on show_bug, since I think "Priority" should eventually become a custom field.

Comment 20

[Frédéric Buclin]

(In reply to comment #19)
>   Everything else looks fine. I was thinking it would be nice to put the
> <select> fields up where "Priority" currently is, on show_bug, since I think
> "Priority" should eventually become a custom field.

Yeah. We could probably process bug/field.html.tmpl twice, with the first call requesting select fields only, and the second one text fields only.

We have to admit that we have not much control over the UI (e.g. if an installation has 10 select fields).

Comment 21

[Max Kanat-Alexander]

(In reply to comment #20)
> We have to admit that we have not much control over the UI (e.g. if an
> installation has 10 select fields).

  Yeah. I think we can fix that in the future by making all the fields positionable, and defining certain areas on the page as certain "columns." But that's for the far future.

Comment 22

[Frédéric Buclin]

Max, can you help with the DB/Schema part?

Comment 23

[Max Kanat-Alexander]

Attachment: part 2, v1

Okay, here's the patch with Search.pm and Schema.pm modifications.

I also modified Field::match to not return obsolete fields by default, because I was getting tired of always having to put "obsolete => 0" whenever I called get_fields.

Comment 24

[Frédéric Buclin]

Attachment: complete patch, v2

Here we go! Creating, updating and searching for bugs are working fine. I had to fix a small error in Search.pm (you forgot to add the custom select fields to @legal_fields). I also had to fix several errors in my initial patch.

Do some tests on your own, mine are all successful. Yay! :)

Comment 25

[Frédéric Buclin]

You will notice that I left obsolete => 0 alone. I'm not convinced by selecting only non-obsolete fields by default, so if we change our mind later, we won't break everything. :) And if we really want to select non-obsolete fields by default, we can easily fix that in a separate patch.

Comment 26

[Max Kanat-Alexander]

Comment on attachment 236987 [details] [diff] [review]
complete patch, v2

>Index: Bugzilla/Field.pm
> [snip]
>+=item C<obsolete> - Boolean. True to only return obsolete fields.
>+False to not return obsolete fields. C<undef> to return both. Defaults 
>+to false if not specified.

  Nit: Should fix this POD, now.

>+sub REQUIRED_CREATE_FIELDS {
>+    my @required_custom_fields =
>+        Bugzilla->get_fields({custom => 1, obsolete => 0, enter_bug => 1});

  No, this is incorrect. enter_bug fields are not mandatory. They won't be mandatory until we allow them to be.

>+sub _check_custom_select_fields {

  Let's just call this _check_select_field, since we can re-use it for the other fields also.

>Index: Bugzilla/Object.pm
>@@ -136,7 +136,7 @@
>     my $validators = $self->VALIDATORS;
>     if (exists $validators->{$field}) {
>         my $validator = $validators->{$field};
>-        $value = $self->$validator($value);
>+        $value = $self->$validator($value, $field);

  That's clever, I like that. :-)

>+The third argument is optional and will be the name of the field being
>+validated. This may be required by validators which validate several
>+distinct fields.

  Don't mention that it's optional, because then I feel like sometimes it won't be passed. Really, it will always be passed.

  Make sure you put your name in the Contributors for this file.

>Index: editfields.cgi
>
>+# I couldn't find a better place to put these type names.
>+# But that's the only place where we need them.
>+$vars->{'type_names'} = {FIELD_TYPE_UNKNOWN, 'Unknown Type',
>+                         FIELD_TYPE_FREETEXT, 'Free Text',
>+                         FIELD_TYPE_SINGLE_SELECT, 'Drop Down'};

  These aren't localizable. This is the primary r- reason. They should either stay in the editfields template, or they can be in field-descs.html.tmpl.

> # List all existing custom fields if no action is given.
> if (!$action) {
>-    $vars->{'custom_fields'} = [Bugzilla->get_fields({'custom' => 1})];
>+    $vars->{'custom_fields'} = [Bugzilla->get_fields({'custom' => 1,
>+                                                       obsolete => undef})];

  You don't need to undef the obsolete anymore, that was my change. Also, it looks like you could just get these inside the template.

>+    # We hardcode valid values for $type. This doesn't matter.
>+    my $typ = $type;
>+    (detaint_natural($type) && ($type < 3))

  Nit: Type also shouldn't be less than 0.

>@@ -90,7 +102,8 @@
> 
>     Bugzilla::Field::create_or_update($vars);
> 
>-    $vars->{'custom_fields'} = [Bugzilla->get_fields({'custom' => 1})];
>+    $vars->{'custom_fields'} = [Bugzilla->get_fields({ 'custom' => 1,
>+                                                       obsolete => undef })];

  Nit: You could just get these in the template. Same for various other places.

>+# Add custom select fields.
>+my @custom_fields = Bugzilla->get_fields({custom => 1, obsolete => undef,

  You don't need to undef the obsolete. I was only doing that because of how I defaulted obsolete to 0.

  You should also modify this file to set the "---" value as static for all the custom fields, since that's the in-db default for all of them.

>Index: post_bug.cgi
> [snip]
>+# Include custom fields required on bug creation.

  Nit: These are fields allowed, not required.


  Everything else looks fine, although I didn't test it yet.

Comment 27

[Frédéric Buclin]

(In reply to comment #26)
> >+=item C<obsolete> - Boolean. True to only return obsolete fields.
> >+False to not return obsolete fields. C<undef> to return both. Defaults 
> >+to false if not specified.
> 
>   Nit: Should fix this POD, now.

Why? This is currently correct. You added code in Field::match() yourself which returns non-obsolete fields by default. So if we don't fix the code, we don't have to fix POD.


>   No, this is incorrect. enter_bug fields are not mandatory. They won't be
> mandatory until we allow them to be.

So how do you do if you want to make some custom fields mandatory? But OK, I will change that.


>   Don't mention that it's optional, because then I feel like sometimes it won't
> be passed. Really, it will always be passed.

Well, most of the time, methods won't care about this second param. But OK, I can remove "optional" from the sentence.


>   Make sure you put your name in the Contributors for this file.

For such a minor change? My criteria to add my name or not are different. ;)


>   These aren't localizable. This is the primary r- reason. They should either
> stay in the editfields template, or they can be in field-descs.html.tmpl.

Yeah, I thought about that too. I hesitated. :)


>   You don't need to undef the obsolete anymore, that was my change. Also, it
> looks like you could just get these inside the template.

I still have to as I kept your changes in Field::match(). It returns non-obsolete field by default.


>   You should also modify this file to set the "---" value as static for all the
> custom fields, since that's the in-db default for all of them.

No, I don't want to force "---" to be a valid value. The code handles this perfectly (bugs fields are updated accordingly). That's not a behavior I'm going to change.

Comment 28

[Max Kanat-Alexander]

(In reply to comment #27)
> Why? This is currently correct. You added code in Field::match() yourself which
> returns non-obsolete fields by default. So if we don't fix the code, we don't
> have to fix POD.

  Ohhh. I thought you reverted that code, from your comments. Nevermind.

> So how do you do if you want to make some custom fields mandatory? But OK, I
> will change that.

  Yeah. It's something we either have a bug for or need to file a bug for.

> >   Make sure you put your name in the Contributors for this file.
> 
> For such a minor change? My criteria to add my name or not are different. ;)

  It's actually a legal issue. Technically, any file you modify, you should add your name to.

> >   You should also modify this file to set the "---" value as static for all the
> > custom fields, since that's the in-db default for all of them.
> 
> No, I don't want to force "---" to be a valid value.

  Then we should modify the fields to allow NULLs, and remove the DEFAULT from SQL_DEFINITIONS in Bugzilla::Field. Also, there will have to be some way to set them to NULL, on enter_bug. Or set the DEFAULT to "''" and make sure they always have an empty value.

  Or if people can modify their default value, then the code should also be able to handle that.

Comment 29

[Frédéric Buclin]

(In reply to comment #28)
> > No, I don't want to force "---" to be a valid value.
> 
>   Then we should modify the fields to allow NULLs, and remove the DEFAULT from
> SQL_DEFINITIONS in Bugzilla::Field. Also, there will have to be some way to set
> them to NULL, on enter_bug. Or set the DEFAULT to "''" and make sure they
> always have an empty value.

Yeah, I thought about that too while updating the patch. Probably an empty string is better than NULL, because I'm not sure editvalues.cgi is able to manage NULL values. About "---", I think it's fine to keep it as the default value. Admins can very easily go to editvalues.cgi and rename it to "" or anything else if they want to. That's why I said we don't have to worry about this specific value (editvalues.cgi doesn't let you delete all values anyway).

Comment 30

[Frédéric Buclin]

Attachment: patch, v3

Fixed all your comments, except the "type should be < 0" as detaint_natural() doesn't allow negative values anyway (maybe you had detaint_signed() in mind). Also, I reverted your changes in Field::match() about "obsolete".

The patch is much smaller because custom fields are not required on bug creation. And so I had to revert all my changes in Object.pm and most of them in Bug.pm too. Probably this will be included back when fixing process_bug.cgi to use Object.pm (via Bug.pm).

Comment 31

[Max Kanat-Alexander]

(In reply to comment #29)
The problem is that the database default and the last remaining value have to be the same, because of how Bugzilla::Object->create works. That is, if the field isn't specified, it defaults to the *database* default. This makes a lot of sense, since that's the purpose of database defaults.

So really, it needs to always be either '' or '---' (and preferably '---' because I'm not sure how well editvalues.cgi deals with empty values).

(In reply to comment #30)
> And so I had to revert all my changes in Object.pm and most of them
> in Bug.pm too.

  What?? No, the fields are *allowed* on bug creation! They're just not required! I just meant that you should change the text of the comment!

  They still need to be validated, to make sure they have legal values!

Comment 32

[Max Kanat-Alexander]

Comment on attachment 237336 [details] [diff] [review]
patch, v3

The fields still need to have their values validated as legal in post_bug.cgi, as I stated above.

Comment 33

[Frédéric Buclin]

Attachment: patch, v3.1

Comment 34

[Max Kanat-Alexander]

Comment on attachment 237399 [details] [diff] [review]
patch, v3.1

Okay! This looks good. :-)

Comment 35

[Frédéric Buclin]

Checking in editfields.cgi;
/cvsroot/mozilla/webtools/bugzilla/editfields.cgi,v  <--  editfields.cgi
new revision: 1.4; previous revision: 1.3
done
Checking in editvalues.cgi;
/cvsroot/mozilla/webtools/bugzilla/editvalues.cgi,v  <--  editvalues.cgi
new revision: 1.16; previous revision: 1.15
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.174; previous revision: 1.173
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.341; previous revision: 1.340
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.149; previous revision: 1.148
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.51; previous revision: 1.50
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.86; previous revision: 1.85
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.21; previous revision: 1.20
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.140; previous revision: 1.139
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.71; previous revision: 1.70
done
Checking in template/en/default/admin/custom_fields/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/custom_fields/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/admin/custom_fields/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.83; previous revision: 1.82
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.18; previous revision: 1.17
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.189; previous revision: 1.188
done

Comment 36

[Max Kanat-Alexander]

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

Comment 37

[Max Kanat-Alexander]

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

Comment 38

[Frédéric Buclin]

Documentation updated as part of bug 344875.

Comment 39

[Max Kanat-Alexander]

Added to the relnotes currently attached to bug 349423.

Comment 40

[Robert Elves]

Custom fields should have their respective single select options available in the config.cgi rdf. I'll need this in order to support custom fields in Mylar's rich editor (http://eclipse.org/mylar) and other Bugzilla clients may rely on this information as well.

Currently the name is in the rdf but no option values are provided. Any chance of this making 3.0?

Comment 41

[Max Kanat-Alexander]

(In reply to comment #40)
> Custom fields should have their respective single select options available in
> the config.cgi rdf. I'll need this in order to support custom fields in Mylar's
> rich editor (http://eclipse.org/mylar) and other Bugzilla clients may rely on
> this information as well.

  You'll have to file a separate bug for that.

  Note that you can currently use Bug.legal_values in the WebService interface for this, and that does work just fine.

Comment 42

[Robert Elves]

 (In reply to comment #41)
> You'll have to file a separate bug for that.
> Note that you can currently use Bug.legal_values in the WebService interface
> for this, and that does work just fine.

Filed bug#374981.