Partial custom fields design document

[Gervase Markham]

Before I kick off, I should say that I rather like this design. My one 
overall concern is that some parts of it don't fit well with our 
object-oriented design (such as it is.)

I've reviewed the first bit in more detail than the later bits. Such is 
my attention span. ;-)

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?

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

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

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

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

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

> The cf_lonstring table holds data for long string custom fields.

Nit: typo.

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

Nit: typo.

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

> 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... As a 
matter of preference, I'm also generally a fan of having a different 
function for each.

Also, the bug ID one will be a method on the bug object.

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

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

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

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

> For selection fields, the following additional elements are present:
> 
> unset
>     The field's "unset label", taken from the unset_label column of
>     the CF_SELECTIONS table.
> 
> domain
>     A reference to an array containing the labels which make up the
>     field's domain, sorted into the appropriate order.

domain is probably technically the correct word, but we should consider 
a more user-friendly name if we can think of one.

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

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

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

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

And again.

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

<jacket type="flameproof">
This design doesn't yet cover tracking bug_activity for custom fields...
</jacket>

Gerv