Add an INTEGER custom field type

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
Administration
P5
enhancement
RESOLVED FIXED
10 years ago
3 years ago

People

(Reporter: Diego Prates, Assigned: Simon Green)

Tracking

(Blocks: 1 bug)

Bugzilla 5.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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

Updated

10 years ago
Severity: normal → minor
OS: Windows XP → All
Hardware: PC → All
Version: unspecified → 3.2

Comment 1

10 years ago
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.
Priority: -- → P5
(Reporter)

Comment 2

10 years ago
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
(Assignee)

Comment 3

6 years ago
(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.
(Assignee)

Comment 4

6 years ago
brc have code that allows numeric and integer based custom fields. Are upstream interested in having it?
Flags: needinfo?
(Reporter)

Comment 5

6 years ago
4-year anniversary in two weeks. Closing time?
Flags: needinfo?
(Assignee)

Comment 6

6 years ago
(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
Flags: needinfo?
(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.
Flags: needinfo?
(Assignee)

Updated

6 years ago
Assignee: query-and-buglist → sgreen+mozilla
(Reporter)

Comment 8

6 years ago
This is good news. Congrats to the team, great work.
(Assignee)

Comment 9

6 years ago
Created attachment 679539 [details] [diff] [review]
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
Attachment #679539 - Flags: review?(glob)

Comment 10

6 years ago
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.
Attachment #679539 - Flags: review?(glob) → review-
(Assignee)

Updated

6 years ago
Assignee: sgreen+mozilla → query-and-buglist
Status: UNCONFIRMED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → WONTFIX
(Assignee)

Updated

6 years ago
Attachment #679539 - Attachment is obsolete: true

Comment 11

6 years ago
(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.
(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

6 years ago
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.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 821601

Comment 15

6 years ago
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?
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: WONTFIX → ---

Comment 16

6 years ago
(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.
(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

6 years ago
(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.
(Assignee)

Updated

5 years ago
Assignee: query-and-buglist → sgreen
Status: REOPENED → ASSIGNED
(Assignee)

Comment 19

5 years ago
Created attachment 781382 [details] [diff] [review]
v2 patch

Updated patch.
Attachment #781382 - Flags: review?(glob)

Comment 20

5 years ago
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.
Attachment #781382 - Flags: review?(glob) → review-
(Assignee)

Comment 21

5 years ago
Created attachment 8349175 [details] [diff] [review]
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.
Attachment #781382 - Attachment is obsolete: true
Attachment #8349175 - Flags: review?(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
Attachment #8349175 - Flags: review?(glob) → review-
(Assignee)

Comment 23

4 years ago
Created attachment 8363450 [details] [diff] [review]
bug466178-v4.patch

TIL some HTML 5 :)
Attachment #8349175 - Attachment is obsolete: true
Attachment #8363450 - Flags: review?(glob)
Severity: minor → normal
Summary: [Sorting] Custom field sorts values incorrectly when holding integer values in buglist → Add an INTEGER custom field type

Comment 24

4 years ago
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.
(Assignee)

Comment 25

4 years ago
Created attachment 8363960 [details] [diff] [review]
bug466178-v5.patch
Attachment #8363450 - Attachment is obsolete: true
Attachment #8363450 - Flags: review?(glob)
Attachment #8363960 - Flags: review?(glob)

Comment 26

4 years ago
(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.
(Assignee)

Comment 27

4 years ago
Created attachment 8364001 [details] [diff] [review]
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
Attachment #8363960 - Attachment is obsolete: true
Attachment #8363960 - Flags: review?(glob)
Attachment #8364001 - Flags: review?(glob)
Comment on attachment 8364001 [details] [diff] [review]
bug466178-v5.patch

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

r=glob
Attachment #8364001 - Flags: review?(glob) → review+
Flags: approval?
Flags: approval? → approval+

Updated

4 years ago
Severity: normal → enhancement
Component: Query/Bug List → Administration
Keywords: relnote
Target Milestone: --- → Bugzilla 5.0
(Assignee)

Comment 29

4 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
See Also: → bug 1031274

Updated

4 years ago
Blocks: 1031274

Comment 30

3 years ago
Added to relnotes for 5.0rc1.
Keywords: relnote

Updated

3 years ago
Blocks: 1198556

Updated

3 years ago
Blocks: 1198659
You need to log in before you can comment on or make changes to this bug.