897915 - Field lists not sorted alphabetically

Status: RESOLVED FIXED

(Bugzilla :: User Interface, defect)

Description

[Simon Green]

Attachment: v1 patch

(from brc bug)

Description of problem:
In the Advanced Search form, under Custom Search, the drop down lists showing the possible fields are not correctly sorted in alphabetic order, which makes it unnecessarily difficult to find certain fields in what is rather a long list.

It appears that the list is mostly sorted, with a few fields out of order. For example, Hours Left appears towards the bottom of the list between Regression and Reporter.

Version-Release number of selected component (if applicable):
4.4

How reproducible:
Always

Steps to Reproduce:
1. Go to http://bugzilla.redhat.com
2. Click "Search"
3. Scroll down to "Custom Search"
4. Click on the leftmost drop down list on the second row.
5. Scroll the list until you find "Hours Left" (not to be confused with "Hours Worked")

Actual results:
The "Hours Left" field appears out-of-order (between "Regression" and "Reporter").

Expected results:
The list should be presented in alphabetic order.

Comment 1

[Frédéric Buclin]

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

This doesn't work with localized templates as $field->description contains the hardcoded string in english. We must access field_descs.${field.name} to correctly sort stuff. Also, all these "if ref" make the code hard to read. It should accept only one format to make it more readable.

Comment 2

[Frédéric Buclin]

Also, query.cgi already does:

@fields = sort {lc($a->description) cmp lc($b->description)} @fields;

(which is suboptimal anyway as it doesn't take field_descs into account)

Comment 3

[Simon Green]

(In reply to Frédéric Buclin from comment #1)
> Comment on attachment 780894 [details] [diff] [review]
> v1 patch
> 
> This doesn't work with localized templates as $field->description contains
> the hardcoded string in english. We must access field_descs.${field.name} to
> correctly sort stuff.

I was only using the description as a backup if the field_desc didn't exist (like for the noop value). I have specifically worked around the noop situation (made sure it was sorted first)

> Also, all these "if ref" make the code hard to read.
> It should accept only one format to make it more readable.

Done. Had to do some funky stuff in boolean-charts.html.tmpl to get a list of field name's, but still use the hash. Open to ideas on how to do this better.

(In reply to Frédéric Buclin from comment #2)
> Also, query.cgi already does:
> 
> @fields = sort {lc($a->description) cmp lc($b->description)} @fields;
> 
> (which is suboptimal anyway as it doesn't take field_descs into account)

I'm leaving that code alone.

Comment 4

[Simon Green]

Attachment: v2.patch

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 8348554 [details] [diff] [review]
v2.patch

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

::: Bugzilla/Template.pm
@@ +561,5 @@
> +            return $_[1]{$_[0]} || $_[0];
> +        }
> +        my ($list, $field_desc) = @_;
> +        return [ sort { field_name($a, $field_desc) cmp field_name($b, $field_desc) } @$list ];
> +    };

Why not just work on the objects directly and return a sorted list of objects:

# Allow us to sort the list of fields correctly
$Template::Stash::LIST_OPS->{ sort_by_field_desc } =
    sub {
        my $list = shift;
        my (@sorted, @unsorted);
        foreach my $field (@$list) {
            # Put non field objects to the front
            if (!blessed $field) {
                push(@sorted, $field);
            }
            else {
                push(@unsorted, $field);
            }
        }
        push(@sorted, sort { lc $a->description cmp lc $b->description } @unsorted);
        return \@sorted;
    };


Then in the template you can simply do:

template/en/default/search/boolean-charts.html.tmpl:

        [% FOREACH field = fields.sort_by_field_desc %]
          <option value="[% field.name FILTER html %]"
                  [%~ ' selected="selected"' IF field.name == condition.f %]>
            [% field_descs.${field.name} || field.description FILTER html %]
          </option>
        [% END %]

Comment 7

[Simon Green]

(In reply to David Lawrence [:dkl] from comment #6)
> Why not just work on the objects directly and return a sorted list of
> objects:

The list is currently sorted alphabetically by the field name (this comes from Bugzilla->fields). I can't work on the objects directly, since field names are in the field_descs hash in the templates. This is especially true for translations, but is also true of some other values in English.

(see http://git.mozilla.org/?p=bugzilla/bugzilla.git;a=blob;f=template/en/default/global/field-descs.none.tmpl;hb=HEAD#l62 [github] )

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 8348554 [details] [diff] [review]
v2.patch

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

Can fix on commit. Looks good. r=dkl

::: Bugzilla/Template.pm
@@ +560,5 @@
> +            # Otherwise sort by field_desc or description
> +            return $_[1]{$_[0]} || $_[0];
> +        }
> +        my ($list, $field_desc) = @_;
> +        return [ sort { field_name($a, $field_desc) cmp field_name($b, $field_desc) } @$list ];

Use CORE::fc() to do case folding. Otherwise we get all capitalized descriptions at the top with non-capitalized description names sorted separately at the bottom.

        return [ sort { CORE::fc(field_name($a, $field_desc)) cmp CORE::fc(field_name($b, $field_desc)) } @$list ];

Comment 9

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #8)
> Use CORE::fc() to do case folding.

fc() is only available since Perl 5.16, so you cannot use it:

http://search.cpan.org/~rjbs/perl-5.16.0/pod/perldelta.pod#New_function_fc_and_corresponding_escape_sequence_\F_for_Unicode_foldcase


All field names are supposed to begin with a capital letter, so this is not an issue. If you have some field names starting with a lowercase letter, you could use lc() instead, which is what we use everywhere else.

Comment 10

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #9)
> (In reply to David Lawrence [:dkl] from comment #8)
> > Use CORE::fc() to do case folding.
> 
> fc() is only available since Perl 5.16, so you cannot use it:
> 
> http://search.cpan.org/~rjbs/perl-5.16.0/pod/perldelta.
> pod#New_function_fc_and_corresponding_escape_sequence_\F_for_Unicode_foldcase
> 
> 
> All field names are supposed to begin with a capital letter, so this is not
> an issue. If you have some field names starting with a lowercase letter, you
> could use lc() instead, which is what we use everywhere else.

Yeah I had a couple custom fields I had created that had simple descriptions such as "a text field" or "test date" and I noticed they were shoved to the bottom and not sorted where I expected them to be.

Simon, feel free to use lc() as it will accomplish same and does not require newer Perl.

dkl

Comment 11

[Simon Green]

To ssh://[e-mail]/bugzilla/bugzilla.git
   a6cb5aa..f4b9806  master -> master

Comment 12

[Frédéric Buclin]

I guess we can close this bug as FIXED...