Partial custom fields design document

[Gervase Markham]

Sean McAfee wrote:
>>>display_name is the human-readable name of the field which appears on
>>>the Web interface.
> 
>>How does this interact with localisation?
> 
> It doesn't, yet.  As we've discussed before, I favor storing localized text
> in the database, while I know you'd rather see it in templates.  

It's not that I'd rather see it in templates, it is that is _is_ in 
templates right now.

On http://www.bugzilla.org/download.html we have ten localisation packs. 
To use any one (on a compatible version) you download it, unzip it and 
set a single parameter. If you want to get rid of it, you delete it and 
put the parameter back.

My view is that this simplicity should not be changed. Other than that, 
I have no implementation views.

> I'm willing
> to be convinced that templates are better, but I'd need to see a good
> example of how that would work.

Both this and the positioning issue touch on a basic philosophical 
question: do we make the addition of custom fields require template 
and/or config file changes?

There are several advantages of doing so:

- Bugzilla administrators can specify exactly where they want the field 
to be laid out in various places.
- They can specify exactly what it should be called in the UI, for all 
the languages they have defined.
- It makes it non-trivial to add a custom field. This is a customer 
protection mechanism; if it's trivial, the usability of their bug system 
will undoubtedly go down under management pressure.
- Much-reduced implementation complexity.

I'm sure someone who believes admins should be able to do it all from a 
web GUI can make the case for the other side.

> In the current implementation, custom fields appear in a table, one field
> per row, above the attachments.  I'd never really thought about it before,
> but it might be nifty to be able to store more sophisticated layout
> information in the DB and display the custom fields according to it.

See above; IMO layout information is best stored in templates.

> Hmmm, maybe not.  One of the changes I introduced since my original
> implementation was to represent scalar fields whose values are NULL as
> a gray-toned pair of hyphens.  The same could be done for multiselects, I
> suppose.

I don't quite follow you here. A multiselect with an empty set looks 
like a multi-select with nothing selected. Where do the hyphens come in?

>>- Have multiple defaults for a multi-select
>>- Have a meaningful default - e.g. P3 as a default priority.
>>- Emulate the current suggestion by merely adding the "unset label" as 
>>another option and making it the default.
> 
> A default value is a distinct concept from an unset value.  Let me mull this
> over a bit...

Indeed - you are correct. But if the admin wants an "unset" value, they 
can just have a single default value called "unset", "---" or whatever.

> Consider a situation like this:
> 
> if (fetch_by_product()) {
>     @args = (product => $product);
> } else {
>     @args = (product_id => $product_id);
> }

Why would you be doing both? If you have a product name, convert it to 
an ID using the function provided for this purpose and then call the API.

> Versus:
> 
> if (fetch_by_product()) {
>     ($func, $arg) = (\&get_custom_fields_by_product, $product);
> } else {
>     ($func, $arg) = (\&get_custom_fields_by_product_id, $product_id);
> }
> 
> @fields = $func->($arg);

That's a long-winded way of putting it. :-) In a particular piece of 
code, one should either be dealing with ones products as names or IDs, 
not both.

$id = getProductId($product);
...
@fields = get_custom_fields_by_product_id($id);

>>We don't tend to throw exceptions in Bugzilla... unless you mean 
>>ThrowCodeError()?
> 
> Well, no.  Actually, this touches on an important issue. 

Stop right there :-) The issue of whether Bugzilla starts throwing 
exceptions or not is utterly separate from the design of custom fields. 
Let's not confuse the two, otherwise arguments about one will hinder the 
other. If you want to have that debate, let's have it, but not in the 
context of custom fields.

>>toString() is the more usual name, I think.
> 
> I'm not really fond of the StudlyCaps convention, at least not in Perl.

Hmm. Looking through the existing modules, there seems to be an about 
even split between the two. This is not an ideal situation. We need to 
get Dave to pick one.

> Actually, reducing complexity was my motivation for introducing these
> elements.  Compare:
> 
> [% IF field.is_selection %]
>   ...
> [% ELSIF field.extended %]
>   ...
> [% END %]
> 
> Versus:
> 
> [% IF field.type == "e" or field.type == "m" %]
>   ...
> [% ELSIF field.type == "t" or field.type == "l" or field.type == "m" %]
>   ...
> [% END %]

Again, a straw man :-)

[% SWITCH field.type %]
   [% CASE "e" %]
   [% CASE "m" %]
   ...

or even

[% IF field.type.match(/[me]/) %]
   ...
[% ENDIF %]

Actually, thinking about it, you'd be better off using numbers in the 
database, and constants in the Bugzilla code, like the Auth stuff does 
now and like the new email stuff will.

[% SWITCH field.type %]
   [% CASE MULTI_SELECT %]
   [% CASE SINGLE_SELECT %]
   ...

>>>lblid
>>>    A hash reference.  The keys are the labels that make up the
>>>    field's domain, and the values are the corresponding numerical
>>>    label IDs.
> 
> 
>>Surely "labelid"? :-)
> 
> 
> Well, I suppose it doesn't matter much.  To me, "lblid" seems like a
> straightforward, if terse, name.

Why compress if you don't have to? Disk space is cheap ;-) "lblid" and 
"label" are inconsistent with each other.

>>Why is this code not an enhancement to the current query module? I can't 
>>quite see how the two are integrated.
> 
> They're not.  This module is intended as a complete replacement, for
> two reasons.  First, the original module is predicated on the assumption
> that a query result set can be obtained by a single SQL select statement,
> an assumption that no longer holds when selection fields enter the
> picture. 

So what does this mean for our set of supported databases?

> Rather than attempting to shoehorn custom field querying onto
> a fundamentally incompatible basis, I decided that a rewrite was in order.

Rewriting the search module (which is some of our oldest and therefore 
least buggy code) is a massive job. Yes, as you say, it's crufty and 
impenetrable, but it also _works_.

If there is a rewrite, we would need to adopt a strategy something like 
the following. It should begin as a reimplementation with exactly the 
same interface, so we can compare the two over a range of queries and a 
period of time in production usage. Then, we can change the interface to 
the new one, and then we can add custom field querying.

> Actually, I guess there's a third reason.  As I've stated, it's very
> important that I be able to search Bugzilla from a non-browser context.  The
> current scheme does not support this.  Vast amounts of preparation are
> performed in buglist.cgi before hanging the query off to Bugzilla::Search.

Anything needed to actually do a search is done in Bugzilla::Search. We 
know this, because places other than buglist.cgi do searches :-) You 
can't just incorporate all that buglist.cgi stuff into Bugzilla::Search 
or a replacement for it, because many callers don't need it.

>><jacket type="flameproof">
>>This design doesn't yet cover tracking bug_activity for custom fields...
>></jacket>
> 
> I was going to leave that for a later iteration of the design, but I can
> introduce it in the current one if you'd prefer.

Why not have all the arguments, that is, discussions, at once? :-)

Gerv