277466 - saved search's old name is used when running a search

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Jon]

1. click on a saved search
2. edit the search
3. run the search
4. It prefills with the name of the saved search - change the name to something
else and click "Remember search"
5. got to the saved searches tab of your user preferences
6. click on "Run" for your newly named saved search

Result: it prefills the "Remember search" box with the old search name instead
of the new name you just ran

Comment 1

[Rob Siklos [:robzilla]]

Attachment: robzilla_v1

patch attached with fix.

Chose to remove the "query_based_on" parameter from "urlquerypart" in the
template that saves the query.	Also changed the prefs panel "Run" links to run
the searches by name, instead of by query

Another option would be to remove query_based_on in the CGI by replacing
  $vars->{'urlquerypart'} =~ s/(order|cmdtype)=[^&]*&?//g;
with
  $vars->{'urlquerypart'} =~ s/(order|cmdtype|urlquerypart)=[^&]*&?//g;
in buglist.cgi, but then we lose it when we do other operations (like change
the columns).

Comment 2

[Jon]

If I apply the patch and run thru the scenario listed in the description it
works correctly.

Comment 3

[Frédéric Buclin]

Comment on attachment 172492 [details] [diff] [review]
robzilla_v1

>+                 value="[% urlquerypart.replace('query_based_on=[^&]*&?', '') FILTER html %]">


Looks good. r=LpSolit

Nit: I wonder if '&query_based_on=[^&]*' would work better. This would avoid a
trailing '&' when query_based_on is the last element of the URL.

Comment 4

[Rob Siklos [:robzilla]]

(In reply to comment #3)
> Nit: I wonder if '&query_based_on=[^&]*' would work better. This would avoid a
> trailing '&' when query_based_on is the last element of the URL.
> 

No, that wouldn't work, because it could be the first url parameter, in which
case there is no leading &.  e.g. query.cgi?query_based_on=abc&.....

Anyways, doesn't the regexp that I wrote remove the trailing '&' ?

Comment 5

[Rob Siklos [:robzilla]]

removing approval request until Nit from above is fixed (as per chat with
LpSolit on IRC)

Comment 6

[Rob Siklos [:robzilla]]

Attachment: robzilla_v2

ok, instead of making some complicated regexp, opted for a more readable
solution and just removed a trailing '&' after running the original regexp.

this patch also converts some line endings elsewhere in the file to UNIX
format.

Comment 7

[Frédéric Buclin]

Comment on attachment 185109 [details] [diff] [review]
robzilla_v2

>+                 value="[% urlquerypart.replace('query_based_on=[^&]*&?', '').replace('&$', '') FILTER html %]">

Robzilla, could you please have a look at Bugzilla::CGI::canonicalise_query() ?
What about calling this routine with @exclude = ("query_based_on") ?

Nit: moreover, please don't include line ending conversion in your patch if not
directly related to the bug.

Comment 8

[Rob Siklos [:robzilla]]

Attachment: robzilla_v3

Comment 9

[Marc Schumann [:Wurblzap]]

Comment on attachment 185152 [details] [diff] [review]
robzilla_v3

Nit: can you please change the comment in buglist.cgi, too, along these lines?

 # The list of query fields in URL query string format, used when creating
 # URLs to the same query results page with different parameters (such as
 # a different sort order or when taking some action on the set of query
-# results).  To get this string, we start with the raw URL query string
-# buffer that was created when we initially parsed the URL on script startup,
-# then we remove all non-query fields from it, f.e. the sort order (order)
-# and command type (cmdtype) fields.
-$vars->{'urlquerypart'} = $::buffer;
-$vars->{'urlquerypart'} =~ s/(order|cmdtype)=[^&]*&?//g;
+# results).
+# We are therefore interested in query fields only, which means we ignore
+# the non-query fields order, cmdtype and query_based_on.
+$vars->{'urlquerypart'} = $cgi->canonicalise_query('order',
+						    'cmdtype',
+						    'query_based_on');
 $vars->{'order'} = $order;

 # The user's login account name (i.e. email address).

Comment 10

[Rob Siklos [:robzilla]]

Attachment: robzilla_v4

ok, nit fixed.

Comment 11

[Frédéric Buclin]

Comment on attachment 185155 [details] [diff] [review]
robzilla_v4

>+$vars->{'urlquerypart'} = $cgi->canonicalise_query('order',
>+                                                   'cmdtype',
>+                                                   'query_based_on');

Does not work. You cannot edit a saved query anymore.

Comment 12

[Rob Siklos [:robzilla]]

Attachment: robzilla_v5

use $params->canonicalise_query, instead of $cgi->canonicalise_query (to fix
the problem stated above), and do it *before* calling Bugzilla::Search, so that
the query is not modified by that call.

Comment 13

[Frédéric Buclin]

Comment on attachment 185222 [details] [diff] [review]
robzilla_v5

>+$vars->{'urlquerypart'} = $params->canonicalise_query('order',
>+                                                      'cmdtype',
>+                                                      'query_based_on');

Per my discussion with joel and my own tests, it appears that the right fix is
to use your patch v4, but with $cgi replaced by $params, as above. It seems
that Search.pm optimizes the query, so it looks safe to keep it in its original
place.

Comment 14

[Marc Schumann [:Wurblzap]]

Attachment: robzilla v4.1

Untested modified v4 as per comment 13. I don't really know how this patch
works, I just got bitten by the bug and I comprehend the words in comment 13 :)

Comment 15

[Marc Schumann [:Wurblzap]]

Attachment: robzilla v4.1

Corrected line breaks.

Comment 16

[Frédéric Buclin]

Comment on attachment 190226 [details] [diff] [review]
robzilla v4.1

I want a second review on this. Should go into 2.21 only!

Comment 17

[Frédéric Buclin]

Attachment: unbitrotten patch for 2.20, v4.2

The previous patch is slightly bitrotten. Here is an updated one.

Comment 18

[Dave Miller [:justdave]]

I think this might conflict with the patch on bug 301508...

Comment 19

[Dave Miller [:justdave]]

re-request approval once you've ensured there's no conflict with that patch.

Comment 20

[Frédéric Buclin]

Attachment: unbitrotten patch for the tip, v5

$::buffer has been replaced by $buffer. Carrying forward r+

Comment 21

[Frédéric Buclin]

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.304; previous revision: 1.303
done
Checking in template/en/default/account/prefs/saved-searches.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/saved-searches.html.tmpl,v
 <--saved-searches.html.tmpl
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/list/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v 
<--  list.html.tmpl
new revision: 1.38; previous revision: 1.37
done

Comment 22

[Frédéric Buclin]

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