These fields should take either a single bug id or a list of bug ids. The "blocked" and "dependson" fields are current good examples of this type of field.

We have a similar need at Red Hat and have experimented with converting one of our custom bug id to external tracker id mapping tables to a new custom field type. This seems similar to what is being proposed with this bug so I wanted to post a patch to add this ability. Basically it is creating an additional custom field type FIELD_TYPE_FREETEXT_MULTI_INTEGER which allows storing arbitrary integers in a custom field mapping table but displaying them in a free form text field instead of a select drop down. Currently it works in that it will add/remove values properly and they are searchable from the boolean charts like other custom fields. It does not do any validation yet except to make sure they are indeed positive integer values. We have Bugzilla::Hook::process() call in Bugzilla::Bug::_check_freetext_multi_integer() that uses a custom extension which does validation on the values entered for us. Also in template/en/default/bug/field.html.tmpl we added a Hook.process('links') which calls an extension template will linkifies the id's outside of the text field similar to what is occuring with the dependency id's now. Let me know what you think. Dave

Comment on attachment 308485 [details] [diff] [review] Patch to implement new custom field type for storing list of ids (integers) (v1) I think I'd instead prefer to implement this as a an option for multi-select fields, as opposed to its own whole field type. As far as this bug goes (the list of bug IDs) it would need to be special because it would show up as a list of links with an (edit) thing. So this patch (list of integers) should go into a different bug.

So maybe have an additional check box on the editfields.cgi add/edit screen that says: Allow arbitrary values to be entered via text box for multi-select [ ] Dave

We shouldn't mess up multi-select fields with this RFE. If we want something similar to dependency fields, then you need extra bug ID validation, which is something multi-select fields do not provide. Also, I'm not sure it makes sense to have a multi-select fields with bug IDs only. You probably want more information for them, such as tooltips when hovering them.

I think we should allow custom bug id's to support any arbitrary text. In fact, we're referencing a ticket system that uses a N-NNNN nomenclature.

(In reply to comment #6) > I think we should allow custom bug id's to support any arbitrary text. In > fact, we're referencing a ticket system that uses a N-NNNN nomenclature. Except that this nomenclature is not the one used by Bugzilla.

(In reply to comment #7) > (In reply to comment #6) > > I think we should allow custom bug id's to support any arbitrary text. In > > fact, we're referencing a ticket system that uses a N-NNNN nomenclature. > > Except that this nomenclature is not the one used by Bugzilla. I misunderstood the original intent of this ticket, which is to add more bug id fields, perhaps to replace the hardcoded block/dependson fields. That sounds good. The latest patch seems to link BZ tickets to an external tracker id. If so, then perhaps it should be a new ticket. Regardless, we shouldn't assume the external tracker id is an integer. For us, we're using the Alias field to track external bug id's.

(In reply to comment #6) > I think we should allow custom bug id's to support any arbitrary text. In > fact, we're referencing a ticket system that uses a N-NNNN nomenclature. Currently the table schema for the multi select mapping table is varchar values so you would be able to input arbitrary text up to 64 characters. My patch actually creates a separate table structure that has integer values. I suppose the integers could be placed in the varchar values and have a single table schema for both but that may cause problems later with certain SQL operations.

(In reply to comment #8) > I misunderstood the original intent of this ticket, which is to add more bug id > fields, perhaps to replace the hardcoded block/dependson fields. That sounds > good. > That is what the patch could be used for as well. > The latest patch seems to link BZ tickets to an external tracker id. If so, > then perhaps it should be a new ticket. Regardless, we shouldn't assume the > external tracker id is an integer. For us, we're using the Alias field to > track external bug id's. We were going to use the extension functionality to make the field work as an tracker for ticket ids from another internal tool we use here at Red Hat. But with different extensions or permanent code in the Bugzilla code base, it could be used just as well for mapping internal Bugzilla id's.

(In reply to comment #5) > We shouldn't mess up multi-select fields with this RFE. If we want something > similar to dependency fields, then you need extra bug ID validation, which is > something multi-select fields do not provide. Also, I'm not sure it makes sense > to have a multi-select fields with bug IDs only. You probably want more > information for them, such as tooltips when hovering them. Good point. Maybe it would be best to keep the types separate in that case so that each can be used to do a specific job and keep complexity down by not having to overload one type too much. Also with extensions, it is not difficult to validate and use the id's for whatever purpose they like.

This patch creates a custom field type called "Bug ID" which takes a single valid bug id. It displays as an edit box or a link as the dependson and blocks fields.

Comment on attachment 330501 [details] [diff] [review] Patch to add a custom filed of type Bug ID >+sub _check_bugid_field { >+ my ($invocant, $value, $field) = @_; >+ return [] if !$value; An empty arrayref? >+ $self->{_bugid_fields} ||= [Bugzilla->get_fields( >+ {custom => 1, type => FIELD_TYPE_BUGID })]; You never use that. >Index: template/en/default/bug/field.html.tmpl >+ [% CASE constants.FIELD_TYPE_BUGID %] >+ <span id="[% field.name %]_input_area"> >+ [% IF bug.check_can_change_field(field.name, 0, 1) %] >+ <input name="[% field.name %]" id="[% field.name %]" >+ value="[% value.join(', ') %]"> It only takes one value, right now, so you don't need to join it. >+ [% IF bug.check_can_change_field(field.name, 0, 1) %] >+ <span id="[% field.name %]_edit_container" class="edit_me bz_default_hidden" > Nit: Space before the closing > >Index: template/en/default/bug/create/create.html.tmpl > [snip] >+ [% IF field.type == constants.FIELD_TYPE_BUGID %] >+ <tr> >+ <th>[% field.name %]</th> >+ <td colspan="3"> >+ <input name="[% field.name %]" value="[% ${field.name} FILTER html %]"> >+ </td> >+ </tr> Um, you should still be using bug/field for that. And if you can't, then you should fix bug/field.

Is this patch addressing the original request of a field that will take a list of valid bug ids or is it only allowing a single bug id? From what I can see it looks like the latter. What usage case would you need to record only a single bug id? I know of one that we would be able to use it for would be to have a way to see which bug it was originally cloned if it is a cloned bug from which a single id would suffice. But for dependencies, you would need to be able to enter multiple bug ids. Dave

(In reply to comment #15) > Is this patch addressing the original request of a field that will take a list > of valid bug ids or is it only allowing a single bug id? From what I can see > it looks like the latter. For this bug it is the latter. Take a look at 251556. I also submitted a patch there that allows for multiple bug ids and dependencies. Elliotte

(In reply to comment #16) > For this bug it is the latter. Take a look at 251556. Bug if it accepts a single bug ID, this one can be stored in an additional column to the bugs table. If you later convert it to accept multiple bug IDs, then you no longer can store it there; you need a separate DB table. I don't think that's a good idea to implement both separately, because changes required in the DB are different and mutually exclusive.

(In reply to comment #17) > (In reply to comment #16) > > For this bug it is the latter. Take a look at 251556. > > Bug if it accepts a single bug ID, this one can be stored in an additional > column to the bugs table. If you later convert it to accept multiple bug IDs, > then you no longer can store it there; you need a separate DB table. I don't > think that's a good idea to implement both separately, because changes required > in the DB are different and mutually exclusive. > The patch I submitted earlier in this bug would do this, it uses a separate mapping table which could either be one bug id or many. To the user it would look the same. Dave

For now we will implement this as a single bug id. That should be enough to do "what bug regressed this one?" We can later change the type to take a list.

Comment on attachment 331700 [details] [diff] [review] Patch to add a custom field of type Bug ID (v2) >Index: Bugzilla/Bug.pm >+sub _check_bugid_field { >+ my ($invocant, $value, $field) = @_; >+ return 0 if !$value; No, it should be getting undef, not 0. >Index: template/en/default/bug/field.html.tmpl >@@ -61,6 +62,29 @@ >+ [% IF bug.check_can_change_field(field.name, 0, 1) %] >+ <input name="[% field.name %]" id="[% field.name %]" >+ value="[% value %]"> >+ [% END %] No, that's incorrect. That code doesn't belong here. Note the [% IF editable %] check higher up, that's the correct division. So you need special-case code in the [% ELSE %] for "IF editable.", like we do for textareas now. >+ [% FOREACH depbug = bug.${field.name} %] >+ [% depbug FILTER bug_link(depbug) FILTER none %][% " " %] >+ [% END %] You don't need a FOREACH, there's only one bug id. >+ [% IF bug.check_can_change_field(field.name, 0, 1) %] Same comment here about editable.

Comment on attachment 332901 [details] [diff] [review] Patch to add a custom field of type Bug ID (v3) >Index: Bugzilla/Bug.pm >+sub _check_bugid_field { >+ my ($invocant, $value, $field) = @_; >+ return undef if !$value; >+ return $invocant->check($value, $field)->id; My only concern about this: make sure that check() works when $invocant is an instance (and not the Class). We don't test things like $self->new and $self->check. >+ FIELD_TYPE_BUGID I have a feeling that that should be BUG_ID, or I'm going to forget every time. Let's use bug_id everywhere in the code, not bugid. > FIELD_TYPE_DATETIME, { TYPE => 'DATETIME' }, >+ FIELD_TYPE_BUGID, { TYPE => 'INT3' }, Nit: Either the closing brace should be aligned with the one above it, or there should only be one space before it. >Index: template/en/default/bug/field.html.tmpl >+ [% CASE constants.FIELD_TYPE_BUGID %] >+ <span id="[% field.name %]_input_area"> field.name FILTER html (or maybe css_class_quote?) That needs to be fixed everywhere, as appropriate. >+ <input name="[% field.name %]" id="[% field.name %]" >+ value="[% value %]"> And value definitely needs a FILTER html. Also, the size of that input should be made very small, to fit only one (six or seven digit) bug id. >+ <span id="[% field.name %]_edit_container" class="edit_me bz_default_hidden" > Nit: No space before >

_check_bugid_field does work when $invocant is an instance.

Comment on attachment 333884 [details] [diff] [review] Patch to add a custom field of type Bug ID (v4) Code-wise, this looks great. I just have to test it a bit and then we'll be all good.

Putting in approval queue for testing.

Seems to work just fine! :-)

Checking in Bugzilla/Bug.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v <-- Bug.pm new revision: 1.255; previous revision: 1.254 done Checking in Bugzilla/Constants.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v <-- Constants.pm new revision: 1.96; previous revision: 1.95 done Checking in Bugzilla/Field.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v <-- Field.pm new revision: 1.32; previous revision: 1.31 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.15; previous revision: 1.14 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.28; previous revision: 1.27 done

Burn burn... The filtering in bug/field.html.tmpl is doubly wrong; all tinderboxen are burning. Patch coming.

Data which is passed to a JS script must be filtered using FILTER js (instead of the usual FILTER html). And one value wasn't filtered at all.

Comment on attachment 334980 [details] [diff] [review] fix bustage Oh, yeah, definitely.

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.16; previous revision: 1.15 done

Before we add even more special-cased field types, shouldn't we convert the ones we have to proper subclasses of a general field class (Field.pm, say)?

Added to the release notes for Bugzilla 3.4 in bug 494037.