274397 - Can't edit a saved search after choosing 'Show List' from a bug (and other situations)

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Dave Miller [:justdave]]

Steps to reproduce:

1) visit Specific search page so it sets your cookie preference to specific
2) click on a saved search in the footer
3) click on a bug in the list
4) click Show List
5) click Edit Search

Actual results:

you get the specific search page.

Expected results:

either:
1) The same search page your stored query was originally saved from
-or-
2) The advanced search page with the list of bug numbers in the "bugs to
include" field.

Comment 1

[Colin Ogilvie [:cso]]

This also occurs with the default "My Bugs" link as far as I can tell - I get
taken to the specific bug page rather than the advanced search page.

Comment 2

[Colin Ogilvie [:cso]]

Attachment: Patch v1

Add the "query_format=advanced" paramater if it is not there.

This came about after discussion with Justdave on IRC.

Comment 3

[Colin Ogilvie [:cso]]

There is still a case where you can not edit a Saved Search, in some circumstances. 

If you go to userprefs.cgi's Saved Search tab, then you can sometimes not save a
search. This is because the query_format parameter is stored in the query within
the database. I'm thinking this should be a seperate bug.

Comment 4

[Colin Ogilvie [:cso]]

Attachment: Userprefs Patch v1

Patch to userprefs.

Userprefs needs to work around the fact that this paramater may be missing, or
set to "blank".

Comment 5

[Marc Schumann [:Wurblzap]]

Comment on attachment 168634 [details] [diff] [review]
Userprefs Patch v1

This part works well.
Please combine it with the other patch so that there is only one patch file,
though.

Comment 6

[Marc Schumann [:Wurblzap]]

Comment on attachment 168628 [details] [diff] [review]
Patch v1

This does not work for me. The original "Edit search" link goes to
query.cgi?regetlastlist=1, which doesn't work. The new link goes to
query.cgi?bug_id=1234&bug_id=2345&bug_id=3456&query_format=advanced, which
displays an search page with "1234" at "Only include bugs numbered ___", and
empty otherwise.

Comment 7

[Colin Ogilvie [:cso]]

(In reply to comment #6)
> This does not work for me. The original "Edit search" link goes to
> query.cgi?regetlastlist=1, which doesn't work. The new link goes to
> query.cgi?bug_id=1234&bug_id=2345&bug_id=3456&query_format=advanced, which
> displays an search page with "1234" at "Only include bugs numbered ___", and
> empty otherwise.

Can you provide a link to where that occurs, as I can't reproduce it on my test
install - all Edit links point to a particular query, loaded from the database -
none point to regetlastlist=1.

Comment 8

[Colin Ogilvie [:cso]]

(In reply to comment #5)
> (From update of attachment 168634 [details] [diff] [review] [edit])
> This part works well.
> Please combine it with the other patch so that there is only one patch file,
> though.

I am confused - does this pass or not?

Comment 9

[Marc Schumann [:Wurblzap]]

(In reply to comment #7)
> Can you provide a link to where that occurs, as I can't reproduce it on my
> test install - all Edit links point to a particular query, loaded from the
> database - none point to regetlastlist=1.

This happens on my notebook :-/
You're right that with your patch, none point to regetlastlist=1 any more. What
I was trying to say is that the old (original) link to regetlastlist=1 obviously
doesn't work, and your patch wants to cure that.

The point is that the new link (from your patch) to
query.cgi?bug_id=1234&bug_id=2345&bug_id=3456&query_format=advanced is in error,
too. When I follow the steps to reproduce, having your patch applied, it opens
the advanced query page all right, but it doesn't fill the fields correctly.

(In reply to comment #8)
> I am confused - does this pass or not?

The code is all right and works. I marked it r- because I'd like you to generate
one single combined patch file for both buglist.cgi *and* userprefs.cgi instead
of two partial patch files.

Comment 10

[Colin Ogilvie [:cso]]

Comment on attachment 168628 [details] [diff] [review]
Patch v1

After talking to wurblzap on IRC, I discovered that I was confused as to which
patch his comments were about.

This goal of this patch was to implement justdave's expected result 2 (The
advanced search page with the list of bug numbers in the "bugs to
include" field.) as this would give 2.18/2.19 compatibility with the way 2.16.6
behaves. However, as wurblzap pointed out in his review, it causes the list of
bugs to edit to be "bug_id=x&bug_id=y&bug_id=z" whereas the query.cgi page
expects it to be "bug_id=x,y,z"

I have tracked it down to the way |$::buffer = $params->query_string;| seems to
act on the Query String, except I can not work out how to convert it to do do
it in the comma-seperated way, without it encoding the commas.

Any ideas?

Comment 11

[Marc Schumann [:Wurblzap]]

I can make this peculiar thing work if I modify how $params is generated for the
regetlastlist case in buglist.cgi. There is a part saying

   bug_id => [split(/:/, ...)]

and if I replace this by

   bug_id => join(',',split(/:/, ...))

the correct link for Edit Search is being generated. (Coming to think of it,
replacing the colons by commas is more readable than splitting and joining.)

I don't know whether I'm breaking something else with this (everything seems to
continue to work fine, though), but maybe this gives you an idea how to go on.

Comment 12

[Dave Miller [:justdave]]

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

Comment 13

[Colin Ogilvie [:cso]]

Attachment: Combined Patch v2

Combined patch, designed to implement 2) in the initial comment.

This includes a changed buglist.cgi patch, and the previous "Userprefs Patch
v1".

Note that the HTML-Escaped URL occurs by default, with or without this patch.

Comment 14

[Colin Ogilvie [:cso]]

Attachment: 2.18 Combined Patch v1

attachment 169228 [details] [diff] [review] backported to the 2.18 Branch.

Thanks to Travis for the advice

Comment 15

[Dave Miller [:justdave]]

(In reply to comment #10)
> I have tracked it down to the way |$::buffer = $params->query_string;| seems to
> act on the Query String, except I can not work out how to convert it to do do
> it in the comma-seperated way, without it encoding the commas.

I don't think that matters...  they should get decoded on submit anyway...

Comment 16

[Marc Schumann [:Wurblzap]]

Comment on attachment 169228 [details] [diff] [review]
Combined Patch v2

This is good. The patch applies to the branch, too (it is, in fact, identical),
so it wouldn't have been necessary :-)

+    # 2004-12-13 - [e-mail], bug 274397

I'm not sure about the policy of such comments. It seems to me that they are
common in checksetup.pl but uncommon everywhere else, and I seem to recall
somebody saying that it causes confusion in the long run and that it's what
cvsblame is for.
-> Dave?

Comment 17

[Marc Schumann [:Wurblzap]]

Comment on attachment 169240 [details] [diff] [review]
2.18 Combined Patch v1

Same here.

Comment 18

[Marc Schumann [:Wurblzap]]

(In reply to comment #16)
> The patch applies to the branch, too (it is, in fact, identical),

They aren't, even though Bugzilla interdiff tells me they are.

Comment 19

[Dave Miller [:justdave]]

Comment on attachment 169228 [details] [diff] [review]
Combined Patch v2

>Index: userprefs.cgi

>+        if ($q->{'query'} !~ /query_format=(advanced|specific)/) {

This kind of thing has me squirming.  There's somewhere else in query.cgi (the
cookie-setting code I think) that does this already.  We need to centralize
this sooner rather than later before we forget about it, to make life easier on
customizers.  We need ONE line of code somewhere that defines legal query
formats that can be read by any of the scripts.  I don't mind if it isn't fixed
with this bug, as long as a bug is filed to deal with it (said bug will block
2.18 if you don't fix it here).

As far as Colin's comment, I'm abivalent about that.  I'm not going to enforce
it either way, but yet, cvsblame does tell us that kind of stuff.

Comment 20

[Colin Ogilvie [:cso]]

(In reply to comment #19)
> This kind of thing has me squirming.  There's somewhere else in query.cgi (the
> cookie-setting code I think) that does this already.  We need to centralize
> this sooner rather than later before we forget about it, to make life easier on
> customizers.  We need ONE line of code somewhere that defines legal query
> formats that can be read by any of the scripts.  I don't mind if it isn't fixed
> with this bug, as long as a bug is filed to deal with it (said bug will block
> 2.18 if you don't fix it here).

I think this is refering to:

# Set default page to "specific" if none proviced
if (!($cgi->param('query_format') || $cgi->param('format'))) {
    if (defined $cgi->cookie('DEFAULTFORMAT')) {
        $vars->{'format'} = $cgi->cookie('DEFAULTFORMAT');
    } else {
        $vars->{'format'} = 'specific';
    }
}

If this is the stuff that Dave was talking about, then I will abstract it out to
a sub-routine later on tonight (UK time), unless theres a strong case for not
doing that, but either way its going to block 2.18 apparently.

Comment 21

[Dave Miller [:justdave]]

(In reply to comment #20)
> I think this is refering to:
> 
> # Set default page to "specific" if none proviced

Nope, I was referring to this:

# Set cookie to current format as default, but only if the format
# one that we should remember.
if (grep { $_ eq $vars->{'format'} } qw(specific advanced)) {
    $cgi->send_cookie(-name => 'DEFAULTFORMAT',
                      -value => $vars->{'format'},
                      -expires => "Fri, 01-Jan-2038 00:00:00 GMT");
}

Note the little qw(specific advanced) on the end...   a "list of legal formats
to set cookies for".

Comment 22

[Jacob Steenhagen]

Checked into tip and 2.18. Somebody wanna file that bug Dave mentioned in
comment 19?

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.265; previous revision: 1.264
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.60; previous revision: 1.59
done



Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.255.2.4; previous revision: 1.255.2.3
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.58.2.2; previous revision: 1.58.2.1
done

Comment 23

[Colin Ogilvie [:cso]]

(In reply to comment #22)
> Checked into tip and 2.18. Somebody wanna file that bug Dave mentioned in
> comment 19?

Bug 275788