130373 - Sorting by Target Milestone in templatized buglist.cgi generates bogus error

Status: RESOLVED FIXED

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

Description

[David D. Kilzer (ddk)]

If you attempt to sort buglist.cgi output by Target Milestone, you
will get an error similar to the following:

 Error
 
 The custom sort order you specified in your form submission or 
 cookie contains an invalid column name ms_order.sortkey.

Patch to follow shortly.

Comment 1

[David D. Kilzer (ddk)]

Attachment: Patch v.1

Adds 'ms_order.sortkey' and 'ms_order.value' as valid column names
to @columnnames.  These columns are used when sorting by Target
Milestone.

Comment 2

[David D. Kilzer (ddk)]

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

Ick!  After reloading another buglist.cgi page, I got an error:

Software error:
SELECT DISTINCT bugs.bug_id, bugs.groupset, bugs.bug_severity, bugs.priority,
bugs.rep_platform, map_assigned_to.login_name, bugs.bug_status, bugs.component,
bugs.target_milestone, bugs.short_desc FROM bugs, profiles map_assigned_to,
profiles map_reporter LEFT JOIN profiles map_qa_contact ON bugs.qa_contact =
map_qa_contact.userid LEFT JOIN cc selectVisible_cc ON bugs.bug_id =
selectVisible_cc.bug_id AND selectVisible_cc.who = 1 WHERE ((bugs.groupset &
9223372036854775807) = bugs.groupset OR (bugs.reporter_accessible = 1 AND
bugs.reporter = 1) OR (bugs.cclist_accessible = 1 AND selectVisible_cc.who = 1
AND not isnull(selectVisible_cc.who))) AND bugs.assigned_to =
map_assigned_to.userid AND bugs.reporter = map_reporter.userid AND
(bugs.product = 'TheRacingWorld.com') AND (bugs.target_milestone = '2.2') AND
(bugs.bug_status = 'NEW' OR bugs.bug_status = 'ASSIGNED' OR bugs.bug_status =
'REOPENED') ORDER BY ms_order.sortkey,ms_order.value,bugs.bug_id : Unknown
table 'ms_order' in order clause at /usr/lib/bugzilla/lib/perl5/globals.pl line
255.

Comment 3

[Myk Melez [:myk] [@mykmelez]]

Looks like you ASSIGNED the bug without reassigning it to yourself.  Correcting.

Comment 4

[David D. Kilzer (ddk)]

Attachment: Patch v.2

Still had to modify $query when $order containing
'ms_order.(sortkey|value)' was grabbed from a cookie.  This fixes
the reload problem on buglist.cgi when sort was last set to 
Target Milestone in a second browser window.

There may be a better way to fix this, though.	Will attach an
alternative patch as well.

Comment 5

[David D. Kilzer (ddk)]

For some reason, I'm having problems reproducing this on bugzilla-tip
on landfill (does it not update immediately throughout the day?).

  http://landfill.tequilarista.org/bugzilla-tip/

To reproduce this bug:

1. Click on "Query" link to get to query.cgi page.
2. Set "Sort results by:" to "Bug Number".
3. Set/enter other search parameters as desired.
4. Hit "Search" button.
5. Hit "Change Columns" to add the "TargetM" (target_milestone)
   column if not already available.  Hit "Submit" button to return.
6. Click on "TargetM" column to sort by this column.
7. Click on "My bugs", or another saved query that "Reuses last
   sort".  The error page about an invalid column name,
   ms_order.sortkey, should appear.

   Alternately, click on "Query" again, enter/set search parameters,
   set "Sort results by" to "Reuse same sort as last time", then
   hit "Search" button.  The error page should also appear.

Comment 6

[David D. Kilzer (ddk)]

Attachment: Alt Patch v.1

Alternate patch that prevents the 'ms_order.*' columns from ever
being set in the LASTORDER cookie and clears the cookie for the
user if it contains invalid columns.

Comment 7

[David D. Kilzer (ddk)]

Attachment: Alt Patch v.2

Grr!  It's =~ not ~= !

Comment 8

[David D. Kilzer (ddk)]

Whew!  After justdave updated bugzilla-tip, it now exhibits the
incorrect behavior that this bug describes.

Pick a patch and let me know how to correct it.

Comment 9

[David D. Kilzer (ddk)]

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

This is a silly patch.	It promotes incorrect behavior and deviates from the
original behavior of this page.  See Alt Patch v.2 instead.

Marked obsolete.

Comment 10

[Bradley Baetz (:bbaetz)]

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

The milestone column header from teh generated html still refers to msorder -
you need to push lastorder to the template instead, I think (and possibly
rename the var from $lastorder, or rename $order to $dbOrder, or something

Comment 11

[David D. Kilzer (ddk)]

Attachment: Alt Patch v.3

Removed $lastorder variable (changing $lastorder back to $order,
and restoring use of $qorder).	Added $db_order variable for
changes originally applied to $order for inclusion in SQL query.

Comment 12

[David D. Kilzer (ddk)]

Comment on attachment 74752 [details] [diff] [review]
Alt Patch v.3

<bbaetz> ddk: the path for the cookie is wrong
<bbaetz> ddk: and you should expire it in the past, so that its automatically
removed, I think
<bbaetz> not sure about that, though
<bbaetz> thats what relogin.cgi doesn, though

Comment 13

[David D. Kilzer (ddk)]

Attachment: Patch v.4

(Dropping the "Alt" text to the patch description.)

Cleaned up Set-Cookie print statements to use the cookiepath
parameter, and to set dates (when they weren't set before).

See also Bug 133063.

Comment 14

[Bradley Baetz (:bbaetz)]

Comment on attachment 75797 [details] [diff] [review]
Patch v.4

'one issue per patch'

r=bbaetz - I can't break this one

Comment 15

[David D. Kilzer (ddk)]

Add keyword: regression.  Add CCs.

Original templatization bug was Bug 103778.

Comment 16

[Andreas Franke (gone)]

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

Comment 17

[Dave Miller [:justdave]]

Comment on attachment 75797 [details] [diff] [review]
Patch v.4

r= justdave

Comment 18

[Bradley Baetz (:bbaetz)]

Checked in for ddk

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.163; previous revision: 1.162
done

Comment 19

[Bradley Baetz (:bbaetz)]

and if its checked in, I should mark it FIXED...