23473 - Able to reverse sort of bug_list.cgi ("Ascending" and "Descending")

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P1)

Description

[Henrik Gemal]

I have a link to all the bugs I've reported and would like the newest ones to be
at the top instead of at the buttom.

It would therefore be cool if if was possible to sort ascending and descending.
If could just be an addition to the dropdown box instead of just:
"Bug Number"
"Importance"
"Blabla"

the dropdown box would just be:
"Bug Number - Ascending"
"Bug Number - Descending"
"Importance - Ascending"
"Importance - Descending"
"Blabla - Ascending"
"Blabla - Descending"

Comment 1

[David Lawrence [:dkl]]

Mysql supports this sort of thing within the SQL statement so it should not be
too difficult to add. Basically it would be

if (defined $::FORM{'order'} && $::FORM{'order'} ne "") {
    $query .= "order by ";
    $::FORM{'order'} =~ s/votesum/bugs.votes/; # Silly backwards compatability
                                               # hack.
    ORDER: for ($::FORM{'order'}) {
        /\./ && do {
            # This (hopefully) already has fieldnames in it, so we're done.
            last ORDER;
        };
        /Number Ascending/ && do {
            $::FORM{'order'} = "bugs.bug_id ASC";
            last ORDER;
        };
        /Number Descending/ && do {
            $::FORM{'order'} = "bugs.bug_id DESC";
            last ORDER;
        };
        /Import/ && do {
            $::FORM{'order'} = "bugs.priority, bugs.bug_severity";
            last ORDER;
        };
        /Assign/ && do {
            $::FORM{'order'} = "assign.login_name, bugs.bug_status, priority,
bugs.bug_id";
            last ORDER;
        };
        # DEFAULT
        $::FORM{'order'} = "bugs.bug_status, bugs.priority, assign.login_name,
bugs.bug_id";
    }
    $query .= $::FORM{'order'};
}

I will incorporate these changes and test them in our bugzilla. Then hopefully
submit them to mozilla in the near future.

Comment 2

[Terry Weissman]

My problem is mostly one of UI, not functionality.  Specifying these sorts is
annoying. Doubling the number of entries in that pulldown menu seems really
ugly.  And you can click on column headers to sort by that column; how should I
allow reverse-sorts there?  Add a column footer you can click on?  Weird.

Comment 3

[Henrik Gemal]

One easy way of doing this could just be if its sorted by bugnumber, then if
your click on bug number again it's reversed. Like in all normal applications.

Comment 4

[Terry Weissman]

That's a good idea.  It's gonna be a pain (especially if I don't want to break
people's existing bookmarks).

Comment 5

[Terry Weissman]

[e-mail] is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Comment 6

[Stephan Niemz]

Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.

Comment 7

[Matthew Tuck [:CodeMachine]]

I think we should separate out the different parts of the query.  Something
like:

Bugs That Appear:

Ra ra ra.

Columns That Appear:

Blah Blah Blah.

Sort Order:

Sort By [        ]   ( ) Ascending ( ) Descending

Comment 8

[Dave Miller [:justdave]]

correcting version field lost in product move

Comment 9

[Gervase Markham [:gerv]]

RFE with no patch; -> 2.18.

Gerv

Comment 10

[Asa Dotzler [:asa]]

[e-mail], did you guys ever do this? 

Comment 11

[Andreas Franke (gone)]

dkl: see Asa's question above

Comment 12

[David Lawrence [:dkl]]

Yeah we did this in the Oracle port (2.8) but we did it the way suggested earlier.

Sort by:
* Bug Number Ascending (default)
* Bug Number Descending
* Importance
* Assignee

Using the code similar to the example I posted.

I admit that this is probably not the most elegant way of doing this cause it
starts to look ugly as more choices are added. It didnt effect any stored
queries since we added the choices in early on. I thing maybe having the check
boxes for ascending and descending added would be better for the time being and
would not hurt stored queries for those who havent used the new options.

Comment 13

[Asa Dotzler [:asa]]

*** Bug 146331 has been marked as a duplicate of this bug. ***

Comment 14

[Jouni Heikniemi]

*** Bug 57843 has been marked as a duplicate of this bug. ***

Comment 15

[Dave Miller [:justdave]]

Tara "accepted" this bug about 2 1/2 years ago and hasn't touched it since. 
Changing status to NEW and reassigning to default owner

Comment 16

[Gervase Markham [:gerv]]

Actually, the Template Toolkit has great support for this sort of thing, due to
a few really excellent idioms it has for hash access. I used it in
duplicates.cgi to get the two-way sorting there on all the columns. 

I've been meaning to port it to buglist.cgi; as this is a Zippy thing, I'll see
what I can do about it soon.

Gerv

Comment 17

[Dave Miller [:justdave]]

reassigning per comment 16

Comment 18

[Bradley Baetz (:bbaetz)]

gerv: This really needs to be done on the db side - it'll be a lot quicker,
since the db can change the search (eg indexscan may be slower than a seq scan,
but faster than a seqscan+sort)

Comment 19

[Gervase Markham [:gerv]]

Actually, I'm probably not going to get to this soon.

Gerv

Comment 20

[Michael Morgan [:morgamic]]

it's possible to do this with the template toolkit using a few conditionals...

this is what i did in /template/en/default/list/table.html.tmpl and it "worksforme":

[% BLOCK fliporder %]
  [% IF order.match(' asc') %]
    [% fliplist="+desc" %]
  [% ELSE %]
    [% IF order.match(' desc') %]
      [% fliplist="+asc" %]
    [% END %]
  [% END %]
  [% IF !order.match(' asc') && !order.match(' desc') %]
    [% fliplist="+asc" %]
  [% END %]
[% END %]

[% PROCESS fliporder %]

and then in the column header block i append fliplist to the columnheader URL:

[% BLOCK columnheader %]
  <th colspan="[% splitheader ? 2 : 1 %]">
    <a href="buglist.cgi?[% urlquerypart %]&amp;order=
      [% column.name FILTER url_quote FILTER html %][% fliplist %]">
        [%- abbrev.$id.title || field_descs.$id || column.title -%]</a>
  </th>
[% END %]

this is probably not the best way to do this, but it works.

Comment 21

[Michael Morgan [:morgamic]]

made some quick mods to add arrows:

for the bugs.bug_id column, we have a special case...

 <tr align="left">
      <th colspan="[% splitheader ? 2 : 1 %]">
        <a href="buglist.cgi?[% urlquerypart %]&amp;order=bugs.bug_id[% fliplist
%]">ID</a>[% IF order.match('bugs.bug_id') %][% IF
fliplist.match('asc')%]&uarr;[% ELSE %&darr;[% END %][% END %]]

otherwise, everything is in the columnheader block...

[% BLOCK columnheader %]
  <th colspan="[% splitheader ? 2 : 1 %]">
    <a href="buglist.cgi?[% urlquerypart %]&amp;order=
      [% column.name FILTER url_quote FILTER html %][% fliplist %]">
        [%- abbrev.$id.title || field_descs.$id || column.title -%]</a>
    [% IF order.match(column.name) %][% IF fliplist.match('asc')%]&uarr;[% ELSE
%]&darr;[% END %][% END %]
  </th>
[% END %]

Comment 22

[Christian Reis]

Hey michael -- how about producing a diff -u of your changes? That's the
standard way of contributing code to bugzilla (or any mozilla project, for that
reason). I'll integrate/review as necessary.

Comment 23

[Dave Miller [:justdave]]

Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)

Comment 24

[Dave Miller [:justdave]]

*** Bug 238828 has been marked as a duplicate of this bug. ***

Comment 25

[Lloyd Wood]

In bug 238828 I describe a really simple way to add this functionality without
changing the popup menu, with a far simpler and easier to implement UI.
Basically expanding on Henrik's comment #3.

Comment 26

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 27

[Klaas Freitag]

Please see bug #267859 (Attachment #180373 [details] [diff]) for a discussion about a patch
fixing that.

Comment 28

[Jesse Ruderman]

I wrote a user script to work around this bug and bug 151686.  You can get it
from http://www.squarefree.com/2005/04/18/bug-sort-user-script/.

Comment 29

[Frédéric Buclin]

The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.

Comment 30

[alanjstr]

(In reply to comment #22)
> Hey michael -- how about producing a diff -u of your changes? 

mogramic, are you interested in creating a patch for this still?

Comment 31

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 32

[Alex Schuilenburg]

Attachment: Patch to toggle column sort order between asc and desc

This patch is against the stock 3.0.  It allows you to toggle the search order of any column between asc and desc as well as preserving the second, third, ... column orders. For example, 
"... ORDER BY bugs.creation_ts  desc,bugs.bug_severity,bugs.priority desc"

It also resolves the issue where the order list builds up infinitely through the re-selection of columns by removing the column from the search order before appending the column to the front of the search order.

Comment 33

[Frédéric Buclin]

Comment on attachment 276107 [details] [diff] [review]
Patch to toggle column sort order between asc and desc

Putting this patch in our radar. Is |FILTER remove| supported in TT 2.12? Or is it new to TT 2.18?

Comment 34

[Alex Schuilenburg]

Attachment: Patch to toggle column sort order between asc and desc

This is a slightly modified patch which overcomes one of the problems reported in older versions of the Template::Toolkit (Thanks Dave)

Comment 35

[Vlad Dascalu]

Comment on attachment 276124 [details] [diff] [review]
Patch to toggle column sort order between asc and desc

Keeping it on radar since the uploader didn't request review himself.

Comment 36

[Vlad Dascalu]

Comment on attachment 276124 [details] [diff] [review]
Patch to toggle column sort order between asc and desc

Hey Alex, thanks for the patch!

We have a testing suite that must validate the newly commited code, simply launch "perl runtests.pl" in the Bugzilla directory.

Or "perl runtests.pl -v" for the verbose debug output.

Your patch fails the filtering test:

t/008filter..........NOK 271
#   Failed test '(en/default) template/en/default/list/table.html.tmpl has unfiltered directives:
#   146: desc -

Any variable that is displayed from the template needs some sort of filtering in order to prevent vulnerabilities.

The line causing it is:

+        [% column.sortalias FILTER url_quote %] [% desc -%]

(as you see desc is displayed unfiltered).

If you fix it and upload another version don't forget to request review again.

Comment 37

[Alex Schuilenburg]

Attachment: Patch to toggle column sort order between asc and desc

Patch fixed as per Comment #36

Thanks for the additional info - runtests shows vulnerabilities in my custom templates (now fixed)

Comment 38

[timeless]

Comment on attachment 276358 [details] [diff] [review]
Patch to toggle column sort order between asc and desc

set review by selecting [?|v] not [-|v] from the drop down and picking someone. (vladd@b since he's commented so far is a good choice)

Comment 39

[Vlad Dascalu]

Comment on attachment 276358 [details] [diff] [review]
Patch to toggle column sort order between asc and desc

Nice work!

This works in my tests, except for the first column ("ID").

That's probably because the ID column doesn't use the columnheader BLOCK for rendering (instead it's displayed manually one screen above).

Either you tweak it as well, or you modify it in order to use the template as well.

Looking forward for the updated patch, this is cool!

Comment 40

[Alex Schuilenburg]

Attachment: Patch to toggle column sort order between asc and desc

Amended patch to allow switching sort order on ID as requested.

I have preserved the symantics when you select the ID by dropping the remaining sort order items since the ID is unique, rendering the remaining sort order useless.  This was therefore done in the block above since it made more sense.

Comment 41

[Vlad Dascalu]

Comment on attachment 276426 [details] [diff] [review]
 Patch to toggle column sort order between asc and desc

Template Toolkit doesn't use the concept of "local variables". Basically, once you set the "desc" to ' desc', it remains in that way.

Your code never resets desc to the empty string, for example in cases where order.match doesn't match the column at all.

For example here:

+          [% IF (om = order.match("^bugs\.bug_id( desc)?")) %]
+            [% desc = ' desc' IF not om.0 %]
+          [% END %]

you should add the [% ELSE %] scenario in which you reset desc to the empty string.

Test-case:

Click the first column (the "ID" thing) and sort the list in ASC order based on IDs. After that, in the resulting page, right-click the other column headers and copy the shortcuts for examination; you will see that all of them contain the "desc" parameter (this isn't the expected behaviour, right?).

Comment 42

[Vlad Dascalu]

Comment on attachment 276426 [details] [diff] [review]
 Patch to toggle column sort order between asc and desc

Also, if you could change the template version at the top (the 1.0 thing) per http://www.bugzilla.org/docs/developer.html#templates , that would be great.

Comment 43

[Vlad Dascalu]

To be more clear, I mean the versioning part:

"The testing suite will make sure every template has a version number on the first line such as [e-mail].

You should update the template version number when you edit a template.

You need to increment the major part if your updates make the interface to this template incompatible with the interface of previous versions. Otherwise, you need to increment the minor part."

Comment 44

[Alex Schuilenburg]

Attachment: Patch to toggle column sort order between asc and desc

Vlad, you were correct, that was not the desired behaviour.

This patch fixes the behaviour you describe, but is different from what you suggest.  Instead I simply initialise desc once to the empty string rather than within each conditional necessary or the introduction of ELSE.

Since this is a matter of personal preference, I will provide an alternative solution which does the ELSE and initialisations required. 

I also updated the minor version number, as required.

Comment 45

[Alex Schuilenburg]

Attachment: Alternative patch to toggle column sort order between asc and desc

This is an alternative to Patch 276628 which IMHO is slightly less readable.

Comment 46

[Vlad Dascalu]

Comment on attachment 276628 [details] [diff] [review]
Patch to toggle column sort order between asc and desc  

Hi Alex. I apologize for the delay.

Bugzilla was locked in for the 3.0.1 release, but you should be able to get this in when the tree reopens within some days.

During those 10 days, LpSolit initiated a change that removed (eventually) the versioning number at the top of the templates (for years we had those but apparently we weren't doing a good job of maintaining them). So in order to apply your patch this hook will need to be removed from it:

@@ -1,4 +1,4 @@
-[%# [e-mail] %]
+[%# [e-mail] %]
 [%# The contents of this file are subject to the Mozilla Public
   # License Version 1.1 (the "License"); you may not use this file
   # except in compliance with the License. You may obtain a copy of

Otherwise this looks like an interesting feature, thanks for writing it, I'll let LpSolit review it and rubber-stamp the remaining issues as it looks I'll be busy in the following period of time (I know he loves low-bug numbers :) ).

Comment 47

[Frédéric Buclin]

I will try to review them in the coming days, but my review queue is quite huge these days.

Comment 48

[Alex Schuilenburg]

I found that my patches do not work with version 2.14 of the Template-Toolkit (our live server), but work just fine on 2.19 (our development server).  Upgrading our live server to 2.19 fixed the problem.  I see a comment above by [e-mail] asking if remove is supported by TT 2.12 which makes me wonder if the BZ team have a requirement of a specific version of the TT.

Comment 49

[Frédéric Buclin]

(In reply to comment #48)
> wonder if the BZ team have a requirement of a specific version of the TT.

We must support all TT versions down to 2.12 included. We will only increase the min requirement when a very good reason is given, which is not the case here.

Comment 50

[Alex Schuilenburg]

I am afraid FILTER remove was added in TT 2.15 which is what my patch uses. 

The only way around this is either to (a) upgrade the minimum requirement to 2.15; (b) introduce a TT PROCESS that matches and allows you to remove substrings instead of using the native call introduced in 2.15, or (c) to add the remove filter ($template->context->define_filter(remove => sub {...})) for backward compatability.  

(b) and (c) is IMHO a largely unnecessary overhead, just to maintain backwards compatibility.

Also, without removing items from the search order, it currently can build up infinitely if the user plays around with the column sort order.  As this bug is also fixed by my patch and the introduction of remove, I would argue for an upgrade to a minimum of TT 2.15.

Without the remove filter, my patch will break the ability to sort on multiple columns, but then this will fix the infinitely expanding sort order as a side effect ;-)  For those running TT 2.15 and above, it will work just fine - allowing sorting of multiple columns as well as the order (ascending/descending) of each column.  IMHO this is an acceptable compromise - enhanced functionality and bug fix for those upgrading while slightly reduced functionality for those who whish to stay with versions prior to TT 2.15

If you insist on staying with versions of TT prior to 2.15, I suggest either putting this bug on ice until you do upgrade or assign it to someone who is willing to write throw-away code (for when you do upgrade to TT 2.15 or above).

OOI, why you are tying 3.0 to TT 2.12 especially since 2.13 was released two weeks after 2.12 to address a minor but annoying bug that was overlooked?

Comment 51

[Frédéric Buclin]

I looked at the changelog http://search.cpan.org/src/ABW/Template-Toolkit-2.19/Changes and 'remove' has been added in 2.14a, which is a developer release. So the first stable release to support it is 2.15 which was released on May 26, 2006. This is IMO too recent to require TT 2.15 as many distros don't have it yet.

Comment 52

[Alex Schuilenburg]

I wont dispute that because a fair amount of distros I know of dont include the TT or a lot of the other modules required by bugzilla.  It is not that hard to type
  perl -MCPAN -e 'install Template'
;-)

OOI, Red Hat shipped TT2.09 with RH7.2 but dropped it for RH8. It only started shipping again in RHEL5 (TT 2.19) (I believe it got dropped from RH8 to RHEL4 incl.).  It was in FC5 (2.18) 

I would be interested to know what the majority of BZ 3.x installations are using anyway, given it was also only released earlier this year.  I would think that most 3.x installations are running TT2.15 and above.

Still, you cannot keep everyone happy all the time so I guess you have to go for what the majority want - backwards compatibility or the additional feature and fix.  At least a patch is here and people can make the choice themselves :-)

Nuff said

Comment 53

[Frédéric Buclin]

(In reply to comment #52)
> I wont dispute that because a fair amount of distros I know of dont include the
> TT or a lot of the other modules required by bugzilla.  It is not that hard to
> type
>   perl -MCPAN -e 'install Template'

It's not entirely true. We sometimes have people joining us on IRC complaining that they cannot compile TT from CPAN (e.g. because a test fail). On the other hand, if most 3.2 installations use TT 2.15 or higher, I would be fine changing the min requirement for TT to 2.15. Max, what's your opinion?

Comment 54

[Max Kanat-Alexander]

Adding a minimum of 2.15 would also make life slightly easier in the area of checking for the Template::Plugin::GD requirement. I think it's fine to raise the requirement for 3.2.

The systems that I work on  regularly (Fedora 7, RHEL4) have Template-Toolkit 2.19 available as an RPM. I imagine other relatively modern systems are similar, or at least it's possible to get a 2.15+ package.

It's true that earlier versions of TT fail tests and thus are hard to install. I'm not sure if that's still the case for 2.19. In any case, 2.15 is fine for 3.2.

Comment 55

[Frédéric Buclin]

Comment on attachment 276628 [details] [diff] [review]
Patch to toggle column sort order between asc and desc  

>-[%# [e-mail] %]
>+[%# [e-mail] %]

This can go away. We no longer support template versioning.


>+        [% norder = order.remove("^$column.sortalias( desc)?,?") %]

>+        [% ",$norder" FILTER remove(",$column.sortalias( desc)?") FILTER url_quote IF norder %]

Why do you need |FILTER remove| as you already removed it above?

Comment 56

[Alex Schuilenburg]

> This can go away. We no longer support template versioning.

Fine by me - I was asked to add it by vlad (see comment #46)


[...]
> Why do you need |FILTER remove| as you already removed it above?

Not quite.  You also have either a trailing or a leading comma before the column which need to be removed as well. The first remove drops the column and trailing comma if it appeared at the start of the column order.  This remove is necessary if there is only one column in the sort order, or this is the first in the list. The second remove removes the leading comma and the column if the column already appears further down the list. 

This avoids trailing commas or ",," appearing in the list where the column used to appear.

Comment 57

[Frédéric Buclin]

So you should rather use Bugzilla::CGI::canonicalise_query() instead. It's cleaner.

Comment 58

[Alex Schuilenburg]

Unless I am missing something, canonicalise_query() cleans up empty parameters of a query and not the comma-separated values within individual parameters, because that is what we are talking about.  The order parameter is currently a comma-separated list of columns.

For example, the url will be ...?param1=value1&param2=value2&order=colorder&param4=value4

"colorder" needs to be cleaned of leading, trailing and double commas.

Comment 59

[Frédéric Buclin]

You are right. Then you could split the list on commas and join those you want to keep. Something like:

my @list = split(',', $col_order);
@list = map {$_ != $value_to_remove} @list;
$col_order = join(',', @list);

but adapted to Template-Toolkit. I still think we don't need |FILTER remove| and so don't need to require TT 2.15 to remove a comma only.

Comment 60

[Alex Schuilenburg]

It is pretty obvious you don't want to upgrade to TT 2.15 :-)

Adapting that little bit of perl code to TT 2.12 is not a clean solution by any means unless you are going to allow inline perl and it will never be anything other than a hack.  And for what? I still argue that most BZ 3.X installations will be running TT 2.15 or higher and those that don't will have newer versions easily available to them. It is the older ones after all that were harder to install - TT 2.13 was released just weeks after TT 2.12 because of just such an issue.

Sure, you can also add a remove FILTER for TT 2.12 or even put a perl callback to clean up the field, but I am not into reinventing the wheel for no reason either.  There are plenty of ways to skin a cat...

As I said before, I guess you have to go for what the majority want - backwards compatibility or the additional feature and fix.  At least a patch is here and people can make the choice themselves, even if you dont want to upgrade to TT 2.15 :-)

Comment 61

[Frédéric Buclin]

Comment on attachment 276628 [details] [diff] [review]
Patch to toggle column sort order between asc and desc  

>-[%# [e-mail] %]
>+[%# [e-mail] %]

Template versioning no longer exists. You can remove this change.


>+          [% IF (om = order.match("^bugs\.bug_id( desc)?")) %]
>+            [% desc = ' desc' IF not om.0 %]

Nit: Write 'not' uppercase -> 'NOT'.


>+      [% IF (om = order.match("^$column.sortalias( desc)?")) %]
>+        [% norder = order.remove("^$column.sortalias( desc)?,?") %]
>+        [% desc = ' desc' IF not om.0 %]
>+      [% ELSE %]
>+        [% norder = order %]
>+      [% END %]

Move [% norder = order.remove("^$column.sortalias( desc)?,?") %] out of the IF-ELSE-END block and remove *all* occurences of the column, i.e. don't restrict it to "^$foo" but write "$foo" instead. This will make the logic simpler as the ELSE block now disappears.

Nit: also, capitalize "not" -> "NOT".


>-        [% ",$order" FILTER url_quote IF order %]
>+        [% ",$norder" FILTER remove(",$column.sortalias( desc)?") FILTER url_quote IF norder %]

Revert this change. A much cleaner way to do it is to let buglist.cgi ignore leading/trailing commas and consecutive commas, see line 873 of buglist.cgi. I already have a patch ready for the buglist.cgi part if you want to.

Also, as I will accept the bump of the min requirement for TT to 2.15 (you can thank mkanat), you will also have to fix this in Bugzilla/Install/Requirements.pm, line 81.

Comment 62

[Frédéric Buclin]

Comment on attachment 276630 [details] [diff] [review]
Alternative patch to toggle column sort order between asc and desc

I prefer the previous patch.

Comment 63

[Alex Schuilenburg]

>>-        [% ",$order" FILTER url_quote IF order %]
>>+        [% ",$norder" FILTER remove(",$column.sortalias( desc)?") FILTER url_quote IF norder %]
>
> Revert this change. A much cleaner way to do it is to let buglist.cgi ignore
> leading/trailing commas and consecutive commas, see line 873 of buglist.cgi.
> I already have a patch ready for the buglist.cgi part if you want to.

Yes please, can I have this patch as I am unsure how it could clean up the leading/trailing commas and consecutive commas.  Ignoring them is one thing, but the main issue is that over time this comma list builds up infinitely as users change the column list and ordering. We make tweaks to the column lists depending on the customers we wish to send search URLs to and it will be a PITA to have to edit these URLs down or reset the column order every time.


> Also, as I will accept the bump of the min requirement for TT to 2.15 (you 
> can thank mkanat), you will also have to fix this in
> Bugzilla/Install/Requirements.pm, line 81.

Do you still want this change given you just added a depends on bug 398701?

Comment 64

[Frédéric Buclin]

Attachment: fix for buglist.cgi

buglist.cgi will ignore leading and trailing commas, as well as consecutive commas. If the order list size decreases to 0, it will fall back to the default order list.

Comment 65

[Frédéric Buclin]

(In reply to comment #63)
> but the main issue is that over time this comma list builds up infinitely as
> users change the column list and ordering.

No. As I said in comment 61, writing "$foo" instead of "^$foo" will remove *all* occurences of $foo, not only when being the first item of the list. The list will always remain short as no column will appear twice in the list.


> Do you still want this change given you just added a depends on bug 398701?

Yes. We finally retargetted bug 398701 to Bugzilla 4.0 as TT 2.18 has only been released this year, which we think is too recent to require it for Bugzilla 3.2. So the min requirement for Bugzilla 3.2 will be TT 2.15, and then TT 2.16 in Bugzilla 4.0.

Comment 66

[Alex Schuilenburg]

> No. As I said in comment 61, writing "$foo" instead of "^$foo" will remove
> *all* occurences of $foo, not only when being the first item of the list. The
> list will always remain short as no column will appear twice in the list.

Sorry, I dont think I explained myself properly.  Yes, all the columns will be removed, but not the leading or trailing commas.  My original unpublished solution was as you suggested, but then I ended up with 
  order=col1,col2,,,col3,,,,,,,,,,,,,, 
as I switched between columns.  The commas build up in your proposal and my original unpublished solution, resulting in the need for your patch to buglist.cgi (to skip empty fragments).

For example, selecting col3 and them simply switching orders continuously between col1 and col2 results in commas being left behind resulting in more and more empty fragments before col3 appears. Then select col3 and start switching between col1 and col2 again. You will end up with trailing commas and an order value similar to my example above.

IMHO allowing the commas and empty fragments to build up unnecessarily in the order value is a bug which your patch to buglist.cgi works around. I prefer to fix the bug at source.

Comment 67

[Frédéric Buclin]

(In reply to comment #66)
> The commas build up in your proposal

No, they don't. Look at my patch, and you will see that $order is built at each call with valid fragments only. My patch also makes buglist.cgi more robust to such URLs.

Comment 68

[Frédéric Buclin]

Alex, any new patch coming soon?

Comment 69

[Alex Schuilenburg]

Should be able to get back to it this week.

Comment 70

[Alex Schuilenburg]

Attachment: Patch to toggle column sort order between asc and desc

Hopefully this patch has all the changes requested :-)  Also applies against 3.0.2

Comment 71

[Frédéric Buclin]

Where are changes in buglist.cgi?

Comment 72

[Alex Schuilenburg]

Sorry, as the changes were yours from attachment 283936 [details] [diff] [review], I left them out assuming they were part of some other bugfix and patch.

Do you want me to append your patch to mine, or is it just sufficient to apply both patches?

Comment 73

[Frédéric Buclin]

(In reply to comment #72)
> Do you want me to append your patch to mine, or is it just sufficient to apply
> both patches?

Yes, please merge both patches.

Comment 74

[Alex Schuilenburg]

Attachment: Patch to toggle column sort order between asc and desc

Complete set of patches required.

Comment 75

[Frédéric Buclin]

Comment on attachment 287179 [details] [diff] [review]
Patch to toggle column sort order between asc and desc


>Index: template/en/default/list/table.html.tmpl

>       <a href="buglist.cgi?[% urlquerypart FILTER html %]&amp;order=
>-        [% column.sortalias FILTER url_quote %]
>+        [% column.sortalias FILTER url_quote %] [% desc FILTER url_quote -%]

I had to remove both the whitespace before [% desc ... %] and the minus sign in -%] to get pages to load correctly. Else pages never finished loading (no idea why). r=LpSolit with these two changes applied.

Comment 76

[Frédéric Buclin]

We now require TT 2.15. This must be mentioned in the relnotes.

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.367; previous revision: 1.366
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.40; previous revision: 1.39
done
Checking in template/en/default/list/table.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/table.html.tmpl,v  <--  table.html.tmpl
new revision: 1.38; previous revision: 1.37
done

Comment 78

[Max Kanat-Alexander]

Added to the Bugzilla 3.2 release notes in bug 432331.