Closed
Bug 23473
Opened 25 years ago
Closed 17 years ago
Able to reverse sort of bug_list.cgi ("Ascending" and "Descending")
Categories
(Bugzilla :: Query/Bug List, enhancement, P1)
Bugzilla
Query/Bug List
Tracking
()
RESOLVED
FIXED
Bugzilla 3.2
People
(Reporter: bugzilla, Assigned: alex)
References
Details
Attachments
(2 files, 7 obsolete files)
1.20 KB,
patch
|
Details | Diff | Splinter Review | |
3.45 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
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•25 years ago
|
||
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•25 years ago
|
||
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.
Reporter | ||
Comment 3•25 years ago
|
||
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•25 years ago
|
||
That's a good idea. It's gonna be a pain (especially if I don't want to break
people's existing bookmarks).
Status: NEW → ASSIGNED
Priority: P3 → P2
Comment 5•25 years ago
|
||
tara@tequilarista.org 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 .)
Assignee: terry → tara
Status: ASSIGNED → NEW
Updated•25 years ago
|
Status: NEW → ASSIGNED
Comment 6•24 years ago
|
||
Adding default QA contact to all open Webtools/Bugzilla bugs lacking one.
Sorry for the spam.
QA Contact: matty
Comment 7•24 years ago
|
||
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
Target Milestone: --- → Bugzilla 2.16
Updated•23 years ago
|
Component: Bugzilla → Query/Bug List
Product: Webtools → Bugzilla
Version: other → 2.10
Comment 9•23 years ago
|
||
RFE with no patch; -> 2.18.
Gerv
Severity: normal → enhancement
Target Milestone: Bugzilla 2.16 → Bugzilla 2.18
Comment 10•23 years ago
|
||
dkl@redhat.com, did you guys ever do this?
Comment 11•23 years ago
|
||
dkl: see Asa's question above
Comment 12•23 years ago
|
||
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•23 years ago
|
||
*** Bug 146331 has been marked as a duplicate of this bug. ***
Comment 14•22 years ago
|
||
*** Bug 57843 has been marked as a duplicate of this bug. ***
Comment 15•22 years ago
|
||
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
Assignee: tara → endico
Status: ASSIGNED → NEW
Comment 16•22 years ago
|
||
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 18•22 years ago
|
||
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•22 years ago
|
||
Actually, I'm probably not going to get to this soon.
Gerv
Assignee: gerv → endico
Comment 20•22 years ago
|
||
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 %]&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•22 years ago
|
||
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 %]&order=bugs.bug_id[% fliplist
%]">ID</a>[% IF order.match('bugs.bug_id') %][% IF
fliplist.match('asc')%]↑[% ELSE %↓[% END %][% END %]]
otherwise, everything is in the columnheader block...
[% BLOCK columnheader %]
<th colspan="[% splitheader ? 2 : 1 %]">
<a href="buglist.cgi?[% urlquerypart %]&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')%]↑[% ELSE
%]↓[% END %][% END %]
</th>
[% END %]
Updated•21 years ago
|
OS: Windows 98 → All
Hardware: PC → All
Comment 22•21 years ago
|
||
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.
Summary: Able to sort by "Ascending" and "Descending" → Able to reverse sort of bug_list.cgi ("Ascending" and "Descending")
Updated•21 years ago
|
Assignee: endico → nobody
Comment 23•21 years ago
|
||
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)
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Comment 24•21 years ago
|
||
*** Bug 238828 has been marked as a duplicate of this bug. ***
Comment 25•21 years ago
|
||
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•20 years ago
|
||
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.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Comment 27•20 years ago
|
||
Please see bug #267859 (Attachment #180373 [details] [diff]) for a discussion about a patch
fixing that.
Comment 28•20 years ago
|
||
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•19 years ago
|
||
The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
Updated•18 years ago
|
QA Contact: mattyt-bugzilla → default-qa
Comment 30•18 years ago
|
||
(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•18 years ago
|
||
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".
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Updated•18 years ago
|
Assignee | ||
Comment 32•17 years ago
|
||
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•17 years ago
|
||
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?
Attachment #276107 -
Flags: review?
Assignee | ||
Comment 34•17 years ago
|
||
This is a slightly modified patch which overcomes one of the problems reported in older versions of the Template::Toolkit (Thanks Dave)
Attachment #276107 -
Attachment is obsolete: true
Attachment #276107 -
Flags: review?
Comment 35•17 years ago
|
||
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.
Attachment #276124 -
Flags: review?
Updated•17 years ago
|
Assignee: nobody → alex
Comment 36•17 years ago
|
||
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.
Attachment #276124 -
Flags: review? → review-
Assignee | ||
Comment 37•17 years ago
|
||
Patch fixed as per Comment #36
Thanks for the additional info - runtests shows vulnerabilities in my custom templates (now fixed)
Attachment #276124 -
Attachment is obsolete: true
Attachment #276358 -
Flags: review-
Comment 38•17 years ago
|
||
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)
Attachment #276358 -
Flags: review- → review?(vladd)
Comment 39•17 years ago
|
||
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!
Attachment #276358 -
Flags: review?(vladd) → review-
Assignee | ||
Comment 40•17 years ago
|
||
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.
Attachment #276358 -
Attachment is obsolete: true
Attachment #276426 -
Flags: review?(vladd)
Comment 41•17 years ago
|
||
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?).
Attachment #276426 -
Flags: review?(vladd) → review-
Comment 42•17 years ago
|
||
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•17 years ago
|
||
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 1.0@bugzilla.org.
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."
Assignee | ||
Comment 44•17 years ago
|
||
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.
Attachment #276426 -
Attachment is obsolete: true
Attachment #276628 -
Flags: review?(vladd)
Assignee | ||
Comment 45•17 years ago
|
||
This is an alternative to Patch 276628 which IMHO is slightly less readable.
Attachment #276630 -
Flags: review?(vladd)
Comment 46•17 years ago
|
||
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 @@
-[%# 1.0@bugzilla.org %]
+[%# 1.1@bugzilla.org %]
[%# 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 :) ).
Attachment #276628 -
Flags: review?(vladd) → review?(LpSolit)
Updated•17 years ago
|
Attachment #276630 -
Flags: review?(vladd) → review?(LpSolit)
Comment 47•17 years ago
|
||
I will try to review them in the coming days, but my review queue is quite huge these days.
Assignee | ||
Comment 48•17 years ago
|
||
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 LpSolit@gmail.com 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•17 years ago
|
||
(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.
Assignee | ||
Comment 50•17 years ago
|
||
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•17 years ago
|
||
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.
Assignee | ||
Comment 52•17 years ago
|
||
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•17 years ago
|
||
(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•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 276628 [details] [diff] [review]
Patch to toggle column sort order between asc and desc
>-[%# 1.0@bugzilla.org %]
>+[%# 1.1@bugzilla.org %]
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?
Assignee | ||
Comment 56•17 years ago
|
||
> 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•17 years ago
|
||
So you should rather use Bugzilla::CGI::canonicalise_query() instead. It's cleaner.
Assignee | ||
Comment 58•17 years ago
|
||
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¶m2=value2&order=colorder¶m4=value4
"colorder" needs to be cleaned of leading, trailing and double commas.
Comment 59•17 years ago
|
||
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.
Assignee | ||
Comment 60•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 276628 [details] [diff] [review]
Patch to toggle column sort order between asc and desc
>-[%# 1.0@bugzilla.org %]
>+[%# 1.1@bugzilla.org %]
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.
Attachment #276628 -
Flags: review?(LpSolit) → review-
Comment 62•17 years ago
|
||
Comment on attachment 276630 [details] [diff] [review]
Alternative patch to toggle column sort order between asc and desc
I prefer the previous patch.
Attachment #276630 -
Flags: review?(LpSolit) → review-
Assignee | ||
Comment 63•17 years ago
|
||
>>- [% ",$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•17 years ago
|
||
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.
Attachment #276630 -
Attachment is obsolete: true
Comment 65•17 years ago
|
||
(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.
Assignee | ||
Comment 66•17 years ago
|
||
> 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•17 years ago
|
||
(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.
Assignee | ||
Comment 69•17 years ago
|
||
Should be able to get back to it this week.
Assignee | ||
Comment 70•17 years ago
|
||
Hopefully this patch has all the changes requested :-) Also applies against 3.0.2
Attachment #276628 -
Attachment is obsolete: true
Attachment #286835 -
Flags: review?(LpSolit)
Comment 71•17 years ago
|
||
Where are changes in buglist.cgi?
Assignee | ||
Comment 72•17 years ago
|
||
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•17 years ago
|
||
(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.
Assignee | ||
Comment 74•17 years ago
|
||
Complete set of patches required.
Attachment #286835 -
Attachment is obsolete: true
Attachment #287179 -
Flags: review?(LpSolit)
Attachment #286835 -
Flags: review?(LpSolit)
Comment 75•17 years ago
|
||
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 %]&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.
Attachment #287179 -
Flags: review?(LpSolit) → review+
Comment 76•17 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Flags: approval+
Keywords: relnote
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•