466178 - Add an INTEGER custom field type

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P5)

Description

[Diego Prates]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.18) Gecko/20081029 Firefox/2.0.0.18
Build Identifier: 3.2rc2

I've added a custom field to hold integer values, say, "Personal Order". So when I try to sort by that column in buglist, I get the following:

1
10
100
2
20
200

Buzilla sorts values as strings instead of integer values. Nevertheless, there is no place where I could specify the value as being integer instead of something else.

Fix suggestion: add a "Value Type" field so I can specify the value type on custom field creation; this can initially hold the major types as "string", "integer/numeric", "date", "boolean".

Note: not sure, but I think this won't happen for the numeric internal bugzilla column "ID". If true, a solution can come from there.

Note2: There is another bug possibly related to this one (but not exactly the same): 190206

Reproducible: Always

Steps to Reproduce:
1. Create a custom field
2. Fill some bugs with an integer value on it, just like the example in the summary (1, 10, 2, 20, 3, 30 ...)

Actual Results:  
Sort order is incorrect according to the expectation for integer values:
1
10
2
20

Expected Results:  
Expected numeric natural order:
1
2
10
20

Comment 1

[Max Kanat-Alexander]

If you're holding bug numbers, there's another field type for that. Otherwise, we're unlikely to fix this any time soon, though you could hack your DB to change the column to an int.

Comment 2

[Diego Prates]

It's a numeric custom field. Could you please point me the table name/column name where I can hack the col type in the DB?

Thanks a lot

Comment 3

[Simon Green]

(In reply to Diego Prates from comment #2)
> It's a numeric custom field. Could you please point me the table name/column
> name where I can hack the col type in the DB?

It will be the cf_NAME in the bugs table.

Comment 4

[Simon Green]

brc have code that allows numeric and integer based custom fields. Are upstream interested in having it?

Comment 5

[Diego Prates]

4-year anniversary in two weeks. Closing time?

Comment 6

[Simon Green]

(In reply to Diego Prates from comment #5)
> 4-year anniversary in two weeks. Closing time?

Not necessarily. brc have the code, and if upstream Bugzilla want it, we are happy to supply a patch. NEEDINFO is asking if upstream want a patch. If they don't, then this bug should be closed.

  -- simon

Comment 7

[:glob ✱]

(In reply to Simon Green from comment #4)
> brc have code that allows numeric and integer based custom fields. Are
> upstream interested in having it?

yes, i think that would be excellent to have upstream.

Comment 8

[Diego Prates]

This is good news. Congrats to the team, great work.

Comment 9

[Simon Green]

Attachment: v1 patch

Patch against trunk. I have only checked this works against MySQL v5. Checks should be made against the other database to make sure it doesn't break.

  -- simon

Comment 10

[Frédéric Buclin]

Comment on attachment 679539 [details] [diff] [review]
v1 patch

I honestly don't think we need these new field types at all. IMO, it's enough to let Bugzilla check that all values are numeric, and if it's the case, sort the list before displaying it.

Comment 11

[Frédéric Buclin]

(In reply to Frédéric Buclin from comment #10)
> I honestly don't think we need these new field types at all. IMO, it's
> enough to let Bugzilla check that all values are numeric, and if it's the
> case, sort the list before displaying it.

In fact, all values have a sortkey, and so sortkeys should be used to sort columns. This would address this problem automatically, which is why I don't want new fields just to enter integers in them.

Comment 12

[:glob ✱]

(In reply to Frédéric Buclin from comment #11)
> In fact, all values have a sortkey, and so sortkeys should be used to sort
> columns. This would address this problem automatically, which is why I don't
> want new fields just to enter integers in them.

can you please explain further how sortkeys can be used to order user entered data?

as per comment 0, if you have bugs with a string custom field that have the values:

1
10
100
2
20
200

where would you apply the sortkey, and what value would you use to sort these numerically?

Comment 13

[Frédéric Buclin]

I'm talking about drop-down custom fields. Values have a sortkey which can be used for sorting data.

For free text custom fields, it's easy for Perl to check if all values are numeric and do a numeric comparison when sorting the data.

Comment 15

[Jens Bannmann]

Reopening.  Frédéric rejected Simon's patch, but did not indicate that another approach would be rejected either.

Frédéric, as far as I understood the code, Bugzilla lets the database handle sorting. Are you proposing to change that and add some post-processing to do numerical sorting inside Perl?

Comment 16

[Frédéric Buclin]

(In reply to Jens Bannmann from comment #15)
> Are you proposing to change that and add some post-processing to do
> numerical sorting inside Perl?

Yes. Currently, columns in buglists are sorted based on their sortkey when there is one. This includes custom fields. For custom free text fields, it should be easy to add some code in buglist.cgi (or Search.pm?) which detects if the column you want to use for sorting contains only numbers, and override the default sorting.

Comment 17

[:glob ✱]

(In reply to Frédéric Buclin from comment #16)
> Yes. Currently, columns in buglists are sorted based on their sortkey when
> there is one. This includes custom fields. For custom free text fields, it
> should be easy to add some code in buglist.cgi (or Search.pm?) which detects
> if the column you want to use for sorting contains only numbers, and
> override the default sorting.

i don't think that's a good idea, as it would result in a column sorting differently based on the search results, resulting in inconsistent and unexpected ordering.  it would also be very inefficient, especially when dealing with large bug lists.

adding an integer custom field type fixes this issue at the correct place -- the database -- and provides the additional benefit of restricting numeric fields to just numeric data.  i'm disappointed that you've decided this isn't the right approach without any reasoning.

Comment 18

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #17)
> fields to just numeric data.  i'm disappointed that you've decided this
> isn't the right approach without any reasoning.

I gave the reasoning above.

Comment 19

[Simon Green]

Attachment: v2 patch

Updated patch.

Comment 20

[Frédéric Buclin]

Comment on attachment 781382 [details] [diff] [review]
v2 patch

As you were mentioning this patch on IRC, here is my review.


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

>+use Regexp::Common;

I really think requiring a new Perl module for such a minor thing as validating integers and real numbers is overkill. It's trivial enough to write our own regexps for them. For integers, we already have detaint_signed() in Bugzilla::Util which does exactly that.


>+sub _check_integer_field {

You must also check that the integer is small enough, below MAX_INT_32, else PostgreSQL crashes:

DBD::Pg::db do failed: ERREUR:  la valeur « 2147483648 » est en dehors des limites du type integer



>+sub _check_numeric_field {

When I enter a very large number, such as 1234567890123456, PostgreSQL converts it into scientific notation 1.23456789012346e+15 and when reloading the bug, it's no longer possible to edit it because you get:

"The value '1.23456789012346e+15' in the Nombre décimal field is not a numeric value.".

So you must check that the value remains within some predefined limits.



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

>+    FIELD_TYPE_INTEGER,       { TYPE => 'INT4',  NOTNULL => 1, DEFAULT => 0 },
>+    FIELD_TYPE_NUMERIC,       { TYPE => 'FLOAT', NOTNULL => 1, DEFAULT => 0 },

The FLOAT type is undefined in the various DB/Schema/*.pm files and so you have no control on their real type. For instance, PostgreSQL converted FLOAT into "double precision" which uses 8 bytes. This may be what you want, but this must be explicit in the DB schema modules.

Comment 21

[Simon Green]

Attachment: bug466178-v3.patch

Fixes the issues in LpS's review. I've dropped support for floating number because it wasn't in the original request, not required for brc's needs, and requires some more investigation to work correctly.

Comment 22

[:glob ✱]

Comment on attachment 8349175 [details] [diff] [review]
bug466178-v3.patch

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

this looks mostly good, just a few minor issues to address..

::: Bugzilla/Bug.pm
@@ +2119,5 @@
> +        return 0;
> +    }
> +
> +    my $orig_value = $value;
> +    if (! detaint_signed($value)) {

nit: remove space after !

@@ +2123,5 @@
> +    if (! detaint_signed($value)) {
> +        ThrowUserError("number_not_integer",
> +                       {field => $field, num => $orig_value});
> +    }
> +    elsif($value > MAX_INT_32) {

nit: add space after elsif

::: template/en/default/bug/field.html.tmpl
@@ +40,3 @@
>          <input id="[% field.name FILTER html %]" class="text_input"
>                 name="[% field.name FILTER html %]"
>                 value="[% value FILTER html %]" size="40"

for FIELD_TYPE_INTEGER add pattern="^\d+$"
(type="number" won't work as it accepts floats)

::: template/en/default/global/user-error.html.tmpl
@@ +1359,5 @@
> +  [% ELSIF error == "number_not_integer" %]
> +    [% title = "Integer Value Required" %]
> +    The value '[% num FILTER html %]' in the 
> +    <em>[% field_descs.$field FILTER html %]</em> field 
> +    is not an integer value (i.e. a whole number).

remove trailing whitespace

Comment 23

[Simon Green]

Attachment: bug466178-v4.patch

TIL some HTML 5 :)

Comment 24

[Frédéric Buclin]

Comment on attachment 8363450 [details] [diff] [review]
bug466178-v4.patch

>=== modified file 'index.cgi'

>-      $dbh->selectrow_array('SELECT COUNT(*) FROM bugs WHERE assigned_to = ?
>-                             AND resolution = ""', undef, $user->id);
>+      $dbh->selectrow_array("SELECT COUNT(*) FROM bugs WHERE assigned_to = ?
>+                             AND resolution = ''", undef, $user->id);

This is another bug.



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

>       [% PROCESS bug/field.html.tmpl value = bug.${field.name}
>-                                     editable = bug.check_can_change_field(field.name, 0, 1)
>+                                     editable = 1

Why this change? It shouldn't be editable if you are not allowed to edit it.

Comment 25

[Simon Green]

Attachment: bug466178-v5.patch

Comment 26

[Frédéric Buclin]

(In reply to Byron Jones ‹:glob› from comment #22)
> for FIELD_TYPE_INTEGER add pattern="^\d+$"

You should simply write: pattern="\d+". The HTML5 specs says "the pattern attribute is matched against the entire value, not just any subset (somewhat as if it implied a ^(?: at the start of the pattern and a )$ at the end)."

Please fix that on checkin, for readability.

Comment 27

[Simon Green]

Attachment: bug466178-v5.patch

With the two character fix Frédéric suggested. If I don't fix it now, I'm likely to forget it by the time the code is reviewed and approved.

  -- simon

Comment 28

[:glob ✱]

Comment on attachment 8364001 [details] [diff] [review]
bug466178-v5.patch

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

r=glob

Comment 29

[Simon Green]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified Bugzilla/Bug.pm
modified Bugzilla/Constants.pm
modified Bugzilla/Field.pm
modified template/en/default/bug/field.html.tmpl
modified template/en/default/global/field-descs.none.tmpl
modified template/en/default/global/user-error.html.tmpl                       
Committed revision 8934.

Comment 30

[Frédéric Buclin]

Added to relnotes for 5.0rc1.