238352 - Sorting in reports makes them unclear

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Gervase Markham [:gerv]]

Reports currently display rows and columns in sorted order. This rather sucks.
For example, you get:

blocker  critical  enhancement  major  minor  normal  trivial

when you should get:

enhancement  trivial  minor  normal  major  critical  blocker

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

This change preserves the standard ordering for priority, severity, platform,
OS, status and resolution.

Gerv

Comment 2

[Gervase Markham [:gerv]]

Comment on attachment 144614 [details] [diff] [review]
Patch v.1

Joel: you've reviewed reporting fixes in the past. Any chance you could take a
stab at this one?

Thanks,

Gerv

Comment 3

[Christian Reis]

Comment on attachment 144614 [details] [diff] [review]
Patch v.1

>Index: report.cgi
>+my @col_names = @{get_names($names{"col"}, $col_isnumeric, $col_field)};

I am curious; why do we need the @{...} around get_names? Doesn't get_names
return a reference to a new array (produced by keps or lsearch) anyway?

>+sub get_names {
>+    my ($names, $isnumeric, $field) = @_;
>+  
>+    my @unsorted = keys %{$names};
>+    my @sorted;

nits: 

   - move unsorted into the branch where it's used (it's only used in the first
branch)
   - move sorted to right before the if () statement

>+        @sorted = map {lsearch(\@unsorted, $_) == -1 ? () : $_} @{$field_list};

Hmmm, I wonder if there isn't a simpler way to check if an item is in an array?

Comment 4

[Dave Miller [:justdave]]

(In reply to comment #3)
> (From update of attachment 144614 [details] [diff] [review])
> >Index: report.cgi
> >+my @col_names = @{get_names($names{"col"}, $col_isnumeric, $col_field)};
> 
> I am curious; why do we need the @{...} around get_names? Doesn't get_names
> return a reference to a new array (produced by keps or lsearch) anyway?

Exactly, which is why it needs the @{} to dereference it, since the variable
you're sticking it in is actually an array and not an array reference.

Comment 5

[Gervase Markham [:gerv]]

Attachment: Patch v.2

Fixed Kiko's points.

Gerv

Comment 6

[Christian Reis]

Comment on attachment 144907 [details] [diff] [review]
Patch v.2

Looks good.

Comment 7

[Gervase Markham [:gerv]]

Fixed. Thanks to kiko for reviewing :-)

Checking in report.cgi;
/cvsroot/mozilla/webtools/bugzilla/report.cgi,v  <--  report.cgi
new revision: 1.23; previous revision: 1.22
done

Gerv