Last Comment Bug 510944 - Sorting bug lists on some fields can fail
: Sorting bug lists on some fields can fail
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 3.4.1
: All All
: -- normal (vote)
: Bugzilla 3.4
Assigned To: Frédéric Buclin
: default-qa
: 284667 515018 519272 (view as bug list)
Depends on:
Blocks: 164009 470214
  Show dependency treegraph
Reported: 2009-08-17 12:13 PDT by Greg Hendricks
Modified: 2010-02-28 10:52 PST (History)
5 users (show)
LpSolit: approval+
LpSolit: approval3.4+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---

V1 (815 bytes, patch)
2009-08-17 13:12 PDT, Greg Hendricks
no flags Details | Diff | Splinter Review
Alternate (811 bytes, patch)
2009-08-17 13:40 PDT, Greg Hendricks
LpSolit: review-
Details | Diff | Splinter Review
patch, v2 (824 bytes, patch)
2009-08-17 16:59 PDT, Frédéric Buclin
gregaryh: review+
Details | Diff | Splinter Review

Description Greg Hendricks 2009-08-17 12:13:08 PDT
In the case where a custom field name matches part of another field name, it can result in the following error: The custom sort order specified in your form submission contains an invalid column name.

This appears to be cause by the code in list/table.html.tmpl which removes items in the order list around line 137:

[% order = order.remove("$id( DESC)?,?") %]

If I have a custom field named cf_my_priority for example, and I have sorted on the regular priority field, the name of this custom field is munged in the order list to remove the 'priority' string leaving only cf_my_

It seems this ought to be fixable by doing a whole word match in the regex but I have not had success using modifiers like \b in the Template code.
Comment 1 Greg Hendricks 2009-08-17 13:12:48 PDT
Created attachment 394871 [details] [diff] [review]

OK, so it seems that placing \b inside the regex doesn't work directly, but concatenation with the regex does... Wierd.
Comment 2 Greg Hendricks 2009-08-17 13:40:09 PDT
Created attachment 394880 [details] [diff] [review]

This works too. I guess I should have tried that first. 
Pick one. :)
Comment 3 Frédéric Buclin 2009-08-17 15:49:14 PDT
Yes, this error was mentioned in bug 164009 comment 26. Your "alternate" patch is the way to go.
Comment 4 Frédéric Buclin 2009-08-17 16:45:23 PDT
Comment on attachment 394880 [details] [diff] [review]

If I click twice on the "assignee realname" header, I still get the error, with the "assignee" field also displayed.
Comment 5 Frédéric Buclin 2009-08-17 16:59:37 PDT
Created attachment 394950 [details] [diff] [review]
patch, v2

I hope you don't mind if I take this bug, but I was already working on this issue as part of my initial patch in bug 491467 (which had a syntax error, btw). The missing point in your patch was the way the trailing comma was handled. The current code is wrong as it looks for ",?", making the comma optional, which is a bad assumption. If the comma is not present, then we must be at the end of the string.
Comment 6 Greg Hendricks 2009-08-18 08:15:14 PDT
Comment on attachment 394950 [details] [diff] [review]
patch, v2

I have no problem with you taking this. I am not very l33t with the regex so I am glad you caught. I just assumed that everything else was working with it when I patched it, but I guess I shouldn't assume huh. ;)
Comment 7 Frédéric Buclin 2009-08-18 08:48:06 PDT

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.46; previous revision: 1.45


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:; previous revision:
Comment 8 Frédéric Buclin 2009-09-07 04:09:57 PDT
*** Bug 515018 has been marked as a duplicate of this bug. ***
Comment 9 Frédéric Buclin 2009-09-16 10:51:42 PDT
*** Bug 284667 has been marked as a duplicate of this bug. ***
Comment 10 Frédéric Buclin 2009-09-28 13:15:19 PDT
*** Bug 519272 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.