Closed Bug 801664 Opened 8 years ago Closed 7 years ago

Add DATE type for custom fields

Categories

(Bugzilla :: Administration, task)

task
Not set

Tracking

()

RESOLVED FIXED
Bugzilla 5.0

People

(Reporter: gerv, Assigned: gerv)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

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
Severity: normal → enhancement
OS: Linux → All
Hardware: x86 → All
Version: unspecified → 4.5
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.
Attached patch Patch v.1 (obsolete) — Splinter Review
dkl: might you be able to review this?

Thanks,

Gerv
Assignee: administration → gerv
Status: NEW → ASSIGNED
Attachment #671898 - Flags: review?(dkl)
dkl: polite ping to remind you that you said you might be able to review this today :-)

Gerv
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] %]
Attachment #671898 - Flags: review?(dkl) → review-
Attached patch Patch v.2 (obsolete) — Splinter Review
Review comments addressed.

Gerv
Attachment #671898 - Attachment is obsolete: true
Attachment #683983 - Flags: review?(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
Attachment #683983 - Flags: review?(dkl) → review+
I could see this being useful for 4.4 but would be fine if just trunk. Setting 4.4 to see what LpSolit thinks.
Flags: approval?
Flags: approval4.4?
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-.
Attachment #683983 - Flags: review-
Too late for 4.4. We are past rc1.
Flags: approval?
Flags: approval4.4?
Flags: approval4.4-
Keywords: relnote
Target Milestone: --- → Bugzilla 5.0
Ok, so trunk then?
Flags: approval?
The patch has r-. It needs to be updated first.
Flags: approval?
Attached patch Patch v.3Splinter Review
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
Attachment #683983 - Attachment is obsolete: true
Attachment #689498 - Flags: review?(LpSolit)
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.
Attachment #689498 - Flags: review?(LpSolit) → review+
Flags: approval+
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
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attached patch fix bustageSplinter Review
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.
Bustage fix:

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
Committed revision 8545.
Blocks: 1046168
Added to relnotes for 5.0rc1.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.