Partial custom fields design document

[Sean McAfee]

Gervase Markham <gerv at mozilla.org> wrote:
>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.  I'm willing
to be convinced that templates are better, but I'd need to see a good
example of how that would work.

>> The cf_membership table describes which fields belong to which
>> products, and in what order those fields are to be displayed on the
>> web interface.

>Currently, fields are not really displayed in the web interface in an 
>"order", but in a two-dimensional layout. How would the "sortkey" 
>parameter map to a position in that layout? How would it interact with 
>permanent fields?

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.

>> The cf_selections table enumerates every selection field defined at
>> this Bugzilla installation, and provides an "unset label" for each.
>> An "unset label" is a short string which is displayed on the Web
>> interface when the value is a selection field is the empty set.

>Multi-select fields can have an empty membership, but do not need a 
>special value to denote that.

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.

>Arguably single-select fields could have 
>an empty set - but would it not be better to just have a "default" flag 
>in cf_selection_labels? That way, you could:
>
>- 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...

>> inactive is true if the label is inactive, and false otherwise.  A
>> label may be made inactive when it should no longer be possible to
>> store new data with that label.  The label must remain in the
>> database, however, so that old data can refer to it.

>How would this work in practice?

Well, the primary use for it here is with selection fields whose domains
are sets of employees.  When an employee leaves the company, it should no
longer be possible to set the field to his name.  However, it's still
important to be able to retain the employee's name on old bugs.

>Would it be present in lists only when 
>selected, and absent otherwise, allowing people to remove the value but 
>not add it? I suggest that might be confusing.

Perhaps, but unavoidably so IMHO.

>Deprecation of values is IMO better handled by a mass-query to remove 
>the value from bugs, and then removal from the list.

Eradicating old data like this isn't really acceptable.

>> sortkey is an integer used for ordering the labels within a selection
>> set.

>Is this necessary? I would have thought most people would be happy with 
>alphabetical order. Having a sortkey necessitates more UI and complexity...

Yes, but not much more.  As a simple example of when alphabetic order
isn't the best choice, consider a selection field describing something
similar to priority, with three values "Low", "Medium", and "High".  That
order is more natural than "High", "Low", "Medium".

>> The cf_data table holds data for date and date/time custom fields.

>Better to call it cf_datetime, as it also contains times?

Well, perhaps.

>> get_custom_fields
>> -----------------
>> 
>> This function returns information on zero or more custom fields.  Its
>> arguments should be in the form of zero or one name-value pairs, as
>> enumerated in the following prototype:
>> 
>> @custom_fields = get_custom_fields(
>>     [ product    => $product,    ]
>>     [ product_id => $product_id, ]
>>     [ bug_id     => $bug_id,     ]
>>     [ fields     => \@fields,    ]
>>     [ field_ids  => \@field_ids, ]
>> );

>I'm not convinced we'll need all these options in practice...

Actually, usefulness in practice is why I introduced most of them.

>As a 
>matter of preference, I'm also generally a fan of having a different 
>function for each.

I'd rather limit the number of functions exported by the API.

Consider a situation like this:

if (fetch_by_product()) {
    @args = (product => $product);
} else {
    @args = (product_id => $product_id);
}

@fields = get_custom_fields(@args);

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

I think the unified API provides a cleaner approach.

>> the fields appear in in that product.  If no such product exists, an
>> exception is thrown.

>We don't tend to throw exceptions in Bugzilla... unless you mean 
>ThrowCodeError()?

Well, no.  Actually, this touches on an important issue.  It's vital for
my purposes to be able to access Bugzilla data from programs--cron jobs and
the like--which lack an enclosing browser context.  Right now I'm just kind
of grinning and bearing the fact that errors in such programs can generate
error messages that look like HTTP responses, thanks to the $SIG{__DIE__}
handler that gets installed somewhere.  Perhaps some kind of global switch
is called for which determines whether a Bugzilla program has a browser
context or not.  I certainly don't mind making the default answer to this
question "Yes" and requiring other programs to take an extra step to make it
"No".

>> extended
>>     A boolean value, true if the field is of type long string,
>>     date/time, or multiselection, and false otherwise.
>> 
>> is_integer
>>     A boolean value, true if the field is of type integer, and false
>>     otherwise.
><snip>

>What's wrong with a "type" field?

Nothing.  There is one.

>> as_string
>>     A reference to a subroutine which can convert the field's value to
>>     a simple string representation.  It will be undefined for
>>     selection fields.

>toString() is the more usual name, I think.

I'm not really fond of the StudlyCaps convention, at least not in Perl.

>> [ The is_integer, is_date, is_string, is_selection, and extended
>>   elements are redundant with the type element, but they are more
>>   convenient to use from templates. ]

>We should probably make that determination at implementation time; at 
>first sight, I don't think it's worth the additional interface complexity.

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 %]

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

>> label
>>     A hash reference.  The keys are numerical IDs of the labels which
>>     make up the field's domain, and the corresponding values are
>>     references to hashes which describe that label. 

>Why do we need both hashes? Can they not be combined?

In theory, yes, but again, reducing complexity was my motivation for the
redundancy.  "lblid" is used for converting labels supplied by a user into
numerical selection IDs, eg:

my $lblid    = $field->{lblid};
my @userdata = Bugzilla->cgi->param('selection_field');
my @invalid  = grep !exists $lblid->{$_}, @userdata;
if (@invalid) {
    throw_invalid_selection_label_error(@invalid);
}
my @selection_id = @$lblid{ @userdata };

...whereas "label" is used for the opposite purpose, ie. converting
numerical selection IDs from the database into textual labels for
presentation to the user, eg:

my $label = $field->{label};
my @selection_id = read_selection_table($bug_id, $field_id);
my @text = map  { $_->{text} }
           sort { $a->{order} <=> $b->{order} }
           @$label{ @selection_id };

>> store_custom_fields
>> -------------------
>> 
>> This function adds new custom field data to the database.  It should
>> be called only when a new bug is being created.

>Again, I think this code should be internal to the Bug object.

No objection here.

>> update_custom_fields
>> --------------------

>And again.

Nor here.

>> QUERYING
>> ========
>> 
>> An interface for obtaining lists of bugs which meet user-specified
>> criteria is provided by the module Bugzilla::CustomFields::Query.

>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.  Rather than attempting to shoehorn custom field querying onto
a fundamentally incompatible basis, I decided that a rewrite was in order.
Secondly, well... I mean no offense to the original author or authors, but
the existing query code appears exceptionally crufty and impenetrable to me.
I had to pore over it for a long time before I finally began to figure it
out.  No doubt this is due largely to the long evolution of the codebase,
but whatever the cause, I think a clean rewrite can only help everyone.

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.
Having the entirety of the query code in one module is much more convenient
for me, as well being a cleaner design in general.

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


--Sean