Field lists not sorted alphabetically

RESOLVED FIXED in Bugzilla 5.0

Status

()

Bugzilla
User Interface
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Simon Green, Assigned: Simon Green)

Tracking

Bugzilla 5.0
Bug Flags:
approval +

Details

Attachments

(1 attachment, 1 obsolete attachment)

2.98 KB, patch
dkl
: review+
Details | Diff | Splinter Review
(Assignee)

Description

4 years ago
Created attachment 780894 [details] [diff] [review]
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.
(Assignee)

Updated

4 years ago
Assignee: ui → sgreen
Status: NEW → ASSIGNED
(Assignee)

Updated

4 years ago
Attachment #780894 - Flags: review?(dkl)

Comment 1

4 years ago
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.
Attachment #780894 - Flags: review?(dkl) → review-

Comment 2

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

Comment 3

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

Comment 4

4 years ago
Created attachment 8348554 [details] [diff] [review]
v2.patch
Attachment #780894 - Attachment is obsolete: true
Attachment #8348554 - Flags: review?(dkl)

Updated

4 years ago
Duplicate of this bug: 986593
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 %]
(Assignee)

Comment 7

4 years ago
(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 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 ];
Attachment #8348554 - Flags: review?(dkl) → review+

Updated

3 years ago
Flags: approval?

Comment 9

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

Comment 11

3 years ago
To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git
   a6cb5aa..f4b9806  master -> master
Flags: approval? → approval+
Target Milestone: --- → Bugzilla 5.0

Comment 12

3 years ago
I guess we can close this bug as FIXED...
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.