801664 - Add DATE type for custom fields

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Gervase Markham [:gerv]]

A long time ago, in bug 357324, we added Date/Time custom fields. Bug 397098 was filed to allow "an option that specifies the precision that a date is stored and displayed with."

That was 5 years ago.

I submit that by far the most normal use for such a field, more normal perhaps even than tracking a date/time, is tracking a date. Instead of the architecture astronautics of bug 397098, we should add an additional custom field type, called "Date", which simply stores a date.

At the moment, if you want to track a date, you have to put up with "00:00" in the UI representation of it all the time. This widens bug lists and looks ugly in bug editing. It's also wrong due to being over-precise, and causes a "Huh?" because you don't actually mean _midnight_ on that day. According to your working practices, you probably mean either 9am or 5pm.

Gerv

Comment 1

[David Lawrence [:dkl]]

Hopefully noone requests yet other date types such as "Time Only", etc., I do not see any reason why we this could not be added as a second date choice.

Comment 2

[Gervase Markham [:gerv]]

Attachment: Patch v.1

dkl: might you be able to review this?

Thanks,

Gerv

Comment 3

[Gervase Markham [:gerv]]

dkl: polite ping to remind you that you said you might be able to review this today :-)

Gerv

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 671898 [details] [diff] [review]
Patch v.1

Review of attachment 671898 [details] [diff] [review]:
-----------------------------------------------------------------

t/008filter.t ........ 14/277 
#   Failed test '(en/default) template/en/default/bug/field.html.tmpl has unfiltered directives:
#   47: size 
# --ERROR'
#   at t/008filter.t line 116.

::: Bugzilla/Bug.pm
@@ +1953,5 @@
>  }
>  
> +sub _check_date_field {
> +    my ($invocant, $date) = @_;
> +    return _check_datetime_field($invocant, $date, 1);

This causes errors for me when updating date fields.

Params passed to validators include a $field value as the third param so you need to pass 1 as the fourth param:

return _check_datetime_field($invocant, $date, "", 1);

@@ +1960,2 @@
>  sub _check_datetime_field {
> +    my ($invocant, $date_time, $date_only) = @_;

As explained above this needs to be changed to:

   my ($invocant, $date_time, $field, $date_only) = @_;

::: Bugzilla/Migrate.pm
@@ -442,1 @@
>      

Unable to test Migrate.pm

::: template/en/default/bug/field.html.tmpl
@@ +41,5 @@
>                 value="[% value FILTER html %]" size="40"
>                 maxlength="[% constants.MAX_FREETEXT_LENGTH FILTER none %]"
>                 [% ' aria-required="true"' IF field.is_mandatory %]>
>      [% CASE constants.FIELD_TYPE_DATETIME %]
> +    [% CASE constants.FIELD_TYPE_DATE %]

This doesn't work for FIELD_TYPE_DATETIME types as it doesn't show an imput for those. This instead works if you do:

    [% CASE [constants.FIELD_TYPE_DATETIME, constants.FIELD_TYPE_DATE] %]

::: template/en/default/search/field.html.tmpl
@@ +52,5 @@
>        YAHOO.bugzilla.fieldAutocomplete.init('[% field.name FILTER js %]',
>                                              '[% field.name FILTER js %]_autocomplete');
>      </script>
>    [% CASE constants.FIELD_TYPE_DATETIME %]
> +  [% CASE constants.FIELD_TYPE_DATE %]

Same here:

[% CASE [constants.FIELD_TYPE_DATETIME, constants.FIELD_TYPE_DATE] %]

Comment 5

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Review comments addressed.

Gerv

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 683983 [details] [diff] [review]
Patch v.2

Review of attachment 683983 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works as expected. r=dkl

Comment 7

[David Lawrence [:dkl]]

I could see this being useful for 4.4 but would be fine if just trunk. Setting 4.4 to see what LpSolit thinks.

Comment 8

[Frédéric Buclin]

Comment on attachment 683983 [details] [diff] [review]
Patch v.2

>=== modified file 'Bugzilla/Constants.pm'

>     FIELD_TYPE_DATETIME
>     FIELD_TYPE_BUG_ID
>     FIELD_TYPE_BUG_URLS
>     FIELD_TYPE_KEYWORDS
>-
>+    FIELD_TYPE_DATE

Please group similar fields together. Put FIELD_TYPE_DATE right above or below FIELD_TYPE_DATETIME.



>=== modified file 'Bugzilla/Field.pm'

>     FIELD_TYPE_DATETIME,      { TYPE => 'DATETIME'   },
>     FIELD_TYPE_BUG_ID,        { TYPE => 'INT3'       },
>+    FIELD_TYPE_DATE,          { TYPE => 'DATE'       },

Same here. For developers, it's easier to read what's implemented yet.
Also, I don't see DATE defined in $self->{db_specific} in each Bugzilla/DB/Schema/*.pm module. Also, Sqlite has special code for dates which needs to be updated to take this new date format into account. See bug 337776 comment 13 for the reason of this specific code for dates.



>=== modified file 'Bugzilla/Migrate.pm'

>+    if ($field eq 'creation_ts'
>+        or $field eq 'delta_ts'
>+        or ($field_obj and
>+             ($field_obj->type == FIELD_TYPE_DATETIME ||
>+              $field_obj->type == FIELD_TYPE_DATE)))

Per our guidelines, move || at the beginning of the next line. Also, mixing "or" and || looks really weird. Please be consistent.



>=== modified file 'Bugzilla/Search.pm'

>     FIELD_TYPE_DATETIME, { _non_changed => \&_nullable_datetime },
>     FIELD_TYPE_TEXTAREA, { _non_changed => \&_nullable },
>     FIELD_TYPE_MULTI_SELECT, MULTI_SELECT_OVERRIDE,
>     FIELD_TYPE_BUG_URLS,     MULTI_SELECT_OVERRIDE,    
>+    FIELD_TYPE_DATE,     { _non_changed => \&_nullable_date },

Same here, please group FIELD_TYPE_DATETIME and FIELD_TYPE_DATE together. Much easier to read.



>=== modified file 'Bugzilla/WebService/Bug.pm'

>+        elsif ($field->type == FIELD_TYPE_DATETIME ||
>+               $field->type == FIELD_TYPE_DATE)

Per our guidelines, please put || at the beginning of the next line.



>=== modified file 'template/en/default/bug/field.html.tmpl'

>+    [% CASE [constants.FIELD_TYPE_DATETIME, constants.FIELD_TYPE_DATE] %]
>+      [% size = (field.type == constants.FIELD_TYPE_DATE) ? 10 : 20 %]

Though I understand why you do it, I'm not a fan of having each field type being of different width. IMO, this makes the UI uglier. But maybe that's less confusing for user? I don't know.



>=== modified file 'template/en/default/global/field-descs.none.tmpl'

>                    ${constants.FIELD_TYPE_DATETIME}      => "Date/Time",
>                    ${constants.FIELD_TYPE_BUG_ID}        => "$terms.Bug ID",
>+                   ${constants.FIELD_TYPE_DATE}          => "Date",

Here too, group them.


Everything mentioned here is pretty minor and mostly cosmetics, except the SQLite fix which is required to make it work with this DB. That's the reason of my r-.

Comment 9

[Frédéric Buclin]

Too late for 4.4. We are past rc1.

Comment 10

[David Lawrence [:dkl]]

Ok, so trunk then?

Comment 11

[Frédéric Buclin]

The patch has r-. It needs to be updated first.

Comment 12

[Gervase Markham [:gerv]]

Attachment: Patch v.3

Review comments addressed. I am using DATETIME again for SQLite so, as I read it, no additional code save the type definition should be needed. But please let me know if I'm wrong.

Gerv

Comment 13

[Frédéric Buclin]

Comment on attachment 689498 [details] [diff] [review]
Patch v.3

>=== modified file 'Bugzilla/Bug.pm'

> sub DATE_COLUMNS {
>+    my @fields = (@{ Bugzilla->fields({ type => FIELD_TYPE_DATETIME }) },
>+                  @{ Bugzilla->fields({ type => FIELD_TYPE_DATE     }) });

It's faster to do one single call to Bugzilla->fields. You can pass several types at once:

    Bugzilla->fields({ type => [FIELD_TYPE_DATETIME, FIELD_TYPE_DATE] })


>+sub _check_date_field {
>+    my ($invocant, $date) = @_;
>+    return _check_datetime_field($invocant, $date, "", 1);
>+}

Remove the useless 3rd argument, see below.


> sub _check_datetime_field {
>-    my ($invocant, $date_time) = @_;
>+    my ($invocant, $date_time, $field, $date_only) = @_;

$field is used nowhere, so this argument is useless. Please remove it (also in callers).


r=LpSolit with these comments fixed.

Comment 14

[Gervase Markham [:gerv]]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/DB/Schema/Mysql.pm
modified importxml.pl
modified Bugzilla/Search.pm
modified Bugzilla/Field.pm
modified Bugzilla/Migrate.pm
modified template/en/default/search/field.html.tmpl
modified Bugzilla/DB/Schema/Pg.pm
modified template/en/default/global/field-descs.none.tmpl
modified template/en/default/bug/field.html.tmpl
modified Bugzilla/Constants.pm
modified Bugzilla/Bug.pm
modified Bugzilla/DB/Schema/Sqlite.pm
modified Bugzilla/WebService/Bug.pm
modified Bugzilla/DB/Schema/Oracle.pm
Committed revision 8544.                

Gerv

Comment 15

[Frédéric Buclin]

Attachment: fix bustage

Bugzilla::Object::run_create_validators() calls validators with ($value, $field, \%params) as arguments, and so date_only must 1) be the 3rd argument (instead of being the 2nd one), and be defined in a hash.

Comment 16

[Frédéric Buclin]

Bustage fix:

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8545.

Comment 17

[Frédéric Buclin]

Added to relnotes for 5.0rc1.