615574 - When viewing a bug that is the result of a saved search, the buglist and 'show last search results' links belong to a different search

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Matt Hulse]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.10 (KHTML, like Gecko) Chrome/7.0.544.0 Safari/534.10
Build Identifier: BUGZILLA-4_0-BRANCH

After using a saved search which results in several bugs, I click the hyperlink to one of those bugs.  While viewing that bug, there is a listing showing where this bug fits in the bug list and allows me to navigate through that bug list or click the link to return to the search results.  (ie. Bug List: (1 of 2) First Last Prev Next   Show last search results)  The problem is that if the query that generated the bug list was a saved search or a command line query, the most recent buglist/search data is not updated.  The most recent buglist/search data is the last search done from the search interface of Bugzilla.  This occurs in the latest 4.0 branch.

Reproducible: Always

Steps to Reproduce:
1. Perform a search from the search interface of bugzilla that returns a certain number of bugs.  Click on one of those bugs, the buglist and show last search links work as expected.
2. Perform a search using a saved search that has different results than the first search.  Click on one of those bugs, the buglist will be the results from #1 and not from the saved search.  Clicking show last results will rerun the query from #1.
Actual Results:  
The recent search/buglist wasn't updated when the saved search was executed.

Expected Results:  
The recent search/buglist should be updated whether searching from the interface or using a saved query.

Comment 1

[Frédéric Buclin]

Yeah, I saw this problem for a long time, in 3.7.x. The problem is that Bugzilla now keeps in mind several buglists, but keep them in the order they are created, not in the order they are run. So the problem is that if you have a saved search which contains the same bug as a more recent search and you view this bug report, then Bugzilla will look for this bug in the more recent searches first, which is not what you want as the saved search is usually older (when talking about its creation date, not its last execution time).

I definitely think that we should look at the last execution time and not the creation date to determine which buglist to redisplay.

Comment 2

[Matt Hulse]

There is code in buglist.cgi that cleans up the search url, line 84:

># If query was POSTed, clean the URL from empty parameters and redirect back to
># itself. This will make advanced search URLs more tolerable.
>#
>if ($cgi->request_method() eq 'POST') {
>    $cgi->clean_search_url();
>    my $uri_length = length($cgi->self_url());
>
>    if (!$cgi->param('regetlastlist') and !$cgi->param('list_id')
>        and $user->id)
>    {
>        # Insert a placeholder Bugzilla::Search::Recent, so that we know what
>        # the id of the resulting search will be. This is then pulled out
>        # of the Referer header when viewing show_bug.cgi to know what
>        # bug list we came from.
>        my $recent_search = Bugzilla::Search::Recent->create_placeholder;
>        $cgi->param('list_id', $recent_search->id);
>    }
>
>    if ($uri_length < CGI_URI_LIMIT) {
>        print $cgi->redirect(-url => $cgi->self_url());
>        exit;
>    }
>}
>

The problem is that the list_id place holder is only created if the request
method is POST which is apparently not the case for saved searches.

To fix this in my installation, I moved the code that inserts the placeholder
up above the check for the POST request so that this list_id placeholder is
inserted regardless of the request method.  This has fixed the problem for me,
so I thought I would pass this along so others may benefit.

Comment 3

[Matt Hulse]

Attachment: Insert the list_id placeholder regardless of request method.

Comment 4

[Matt Hulse]

Whoa, that attachment went funky.  I will reattach.

Comment 5

[Matt Hulse]

Attachment: Insert the list_id placeholder regardless of request method

Comment 6

[Frédéric Buclin]

Comment on attachment 494018 [details] [diff] [review]
Insert the list_id placeholder regardless of request method

mkanat wrote the original code.

Comment 7

[Frédéric Buclin]

See also bug 578335 comment 7.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 494018 [details] [diff] [review]
Insert the list_id placeholder regardless of request method

No, there's no point in adding list_id unless we're doing a redirect--it would be pointless, because it's not going to show up in the referrer URL.

The actual problem that you're experiencing is that Saved Searches are not currently supported by the "multiple buglists" system. The best solution here would be to add some sort of explicit support for Saved Searches.

Comment 9

[Matt Hulse]

It looks to me like line 1214 of buglist.cgi uses the newly created list_id placeholder when calling Bugzilla->user->save_last_search.  Even though the list_id isn't in the referrer url, doesn't the buglist default to the last buglist that included the current bug?  By creating the list_id placeholder for every search, the last search including this bug will always be the last search.  

Maybe this isn't a good solution for everyone but the patch solves the problem for us.  The buglist is always correct now, even for saved searches.

Do what you will with it, but the lack of support for saved searches is a blocker in my opinion.

Comment 10

[Frédéric Buclin]

(In reply to comment #9)
> Do what you will with it, but the lack of support for saved searches is a
> blocker in my opinion.

... which is why I set the flag to blocking4.0+.

Comment 11

[Max Kanat-Alexander]

FWIW, one problem with the patch is that *every single time* you run the Saved Search, you will get a new list_id. It will force every other recent list out if you just refresh or re-load it a few times.

Comment 12

[Matt Hulse]

That doesn't cause problems for us because we don't have any use for the recent searches other than the very last one.  In fact, I don't see the point of saving buglists from multiple recent searches.  I really only care about the last search when I'm iterating over a list of bugs from that search and that is the context I am currently in.  

As a user, if I want to see a bug list from two searches ago, I expect to have to rerun the query that generated the bug list.  If there were some sort of search history I could view with links to rerun old queries, I suppose that would be useful, but I don't see that capability anywhere.  I don't really understand the reason behind storing a list of recent searches.  

Also, if I pull up a bug directly by typing in it's bug#, it is confusing to see that it is magically associated with a prior search.  I didn't bring it up through a search result buglist, I went directly to it, so it shouldn't be in the context of a buglist.

That's my 2 cents (probably up to a nickel by now so I hope you'll forgive my rambling :-) )

Comment 13

[Matt Hulse]

Okay, I read through Bug 24896 and I now see that this feature exists for people who want to have multiple bug lists open at once.  This is not useful for me but apparently it is for others so a different solution is necessary.

Comment 14

[Max Kanat-Alexander]

Okay, I have an idea for a simple hack fix that should be enough to un-block 4.0.

Whenever we see a saved search being run in the referrer, we should use the "cookie" list instead of searching for the database lists.

Comment 15

[Max Kanat-Alexander]

Attachment: v1

Okay, this should solve *all* the problems that people have randomly been running into with Bugzilla::Search::Recent. This refactors the list_id-inserting code to *always* run if there isn't a *valid* list_id in the URL. So even saved searches will get a list_id when run.

Also, there was a problem with recent_search_for--it called check() in an eval{}, but didn't set error_mode, so if check() threw an error, that error would be displayed anyway.

The only people who don't get a list_id are logged-out users.

Comment 16

[Frédéric Buclin]

Comment on attachment 497862 [details] [diff] [review]
v1

>=== modified file 'Bugzilla/CGI.pm'
>+sub redirect_search_url {

>+        my $last_search;
>+        if ($list_id) {
>+            $last_search = 
>+                Bugzilla::Search::Recent->check_quietly({ id => $list_id });
>+        }
>+
>+        # If we have a valid list_id, no need to redirect or clean.
>+        return if $last_search;

$last_search is undefined if there is no $list_id. So to avoid confusion, you should write:

 if ($list_id) {
     my $last_search = Bugzilla::Search::Recent->check_quietly(...);
     return if $last_search;
 }

or even:

 if ($list_id) {
     return if Bugzilla::Search::Recent->check_quietly(...);
 }

which is even shorter.


Otherwise looks good and fixes the problem being reported. r=LpSolit


Note that if I click on the same saved search several times in a row in the footer, the counter is incremented at each click, despite this wouldn't be necessary. But I guess it would be too complex to do this check?

justdave said that there are ~25,000 searches made by logged in users on bmo per day. So this means ~10 millions per year. As profile_search.id is of type "integer auto-increment", which means a limit of 4 billion, bmo can live 400 years without reaching this limit. This is safe. But is it still safe on larger installations such as Yahoo? If their traffic is 100 times higher (is that realistic?), they would reach the 4 billion limit in only 4 years (though they have the manpower to fix the problem themselves). So I guess we don't have to worry about this. :)

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
>  if ($list_id) {
>      return if Bugzilla::Search::Recent->check_quietly(...);
>  }

  Oh, very good point. Will do on checkin.

> Note that if I click on the same saved search several times in a row in the
> footer, the counter is incremented at each click, despite this wouldn't be
> necessary. But I guess it would be too complex to do this check?

  Yeah, I realized that too. What I thought was that clicking on a Saved Search is actually running a new search every time. So it could return totally different results, and you don't want that buglist to override your other buglist from the same search.

> justdave said that there are ~25,000 searches made by logged in users on bmo
> per day. So this means ~10 millions per year. As profile_search.id is of type
> "integer auto-increment", which means a limit of 4 billion, bmo can live 400
> years without reaching this limit. This is safe. But is it still safe on larger
> installations such as Yahoo? If their traffic is 100 times higher (is that
> realistic?), they would reach the 4 billion limit in only 4 years (though they
> have the manpower to fix the problem themselves). So I guess we don't have to
> worry about this. :)

  Yeah, I think it should be okay. Hopefully by the time the limit is reached, it should be relatively safe to overlap and go back to the start.

  At some point it would be nice to make all of our SERIAL types be UNSIGNED so that we get twice the length, but even that wouldn't totally solve this problem for installations of Yahoo's size.

Comment 18

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified buglist.cgi
modified Bugzilla/CGI.pm
modified Bugzilla/User.pm
modified Bugzilla/Search/Recent.pm
Committed revision 7635.

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/4.0/                         
modified buglist.cgi
modified Bugzilla/CGI.pm
modified Bugzilla/User.pm
modified Bugzilla/Search/Recent.pm
Committed revision 7502.