Bug 24896: Make the First/Last/Prev/Next navigation on bugs work with

Bugzilla/CGI.pm

@@ -204,6 +204,10 @@ sub clean_search_url {
         $self->delete('order');
     }
 
+    # list_id is added in buglist.cgi after calling clean_search_url,
+    # and doesn't need to be saved in saved searches.
+    $self->delete('list_id'); 
+
     # And now finally, if query_format is our only parameter, that
     # really means we have no parameters, so we should delete query_format.
     if ($self->param('query_format') && scalar($self->param()) == 1) {

Bugzilla/Constants.pm

@@ -84,6 +84,8 @@ use File::Basename;
     QUERY_LIST
     LIST_OF_BUGS
 
+    SAVE_NUM_SEARCHES
+
     COMMENT_COLS
     MAX_COMMENT_LENGTH
 
@@ -284,6 +286,9 @@ use constant DEFAULT_MILESTONE => '---';
 use constant QUERY_LIST => 0;
 use constant LIST_OF_BUGS => 1;
 
+# How many of the user's most recent searches to save.
+use constant SAVE_NUM_SEARCHES => 10;
+
 # The column length for displayed (and wrapped) bug comments.
 use constant COMMENT_COLS => 80;
 # Used in _check_comment(). Gives the max length allowed for a comment.

Bugzilla/DB/Schema.pm

@@ -842,6 +842,18 @@ use constant ABSTRACT_SCHEMA => {
         ],
     },
 
+    profile_search => {
+        FIELDS => [
+            id         => {TYPE => 'INTSERIAL', NOTNULL => 1, PRIMARYKEY => 1},
+            user_id    => {TYPE => 'INT3', NOTNULL => 1},
+            bug_list   => {TYPE => 'MEDIUMTEXT', NOTNULL => 1},
+            list_order => {TYPE => 'MEDIUMTEXT'},
+        ],
+        INDEXES => [
+            profile_search_user_id => [qw(user_id)],
+        ],
+    },
+
     profiles_activity => {
         FIELDS => [
             userid        => {TYPE => 'INT3', NOTNULL => 1,

Bugzilla/Search/Recent.pm

+# -*- Mode: perl; indent-tabs-mode: nil -*-
+#
+# The contents of this file are subject to the Mozilla Public
+# License Version 1.1 (the "License"); you may not use this file
+# except in compliance with the License. You may obtain a copy of
+# the License at http://www.mozilla.org/MPL/
+#
+# Software distributed under the License is distributed on an "AS
+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+# implied. See the License for the specific language governing
+# rights and limitations under the License.
+#
+# The Original Code is the Bugzilla Bug Tracking System.
+#
+# The Initial Developer of the Original Code is Everything Solved, Inc.
+# Portions created by the Initial Developer are Copyright (C) 2010 the
+# Initial Developer. All Rights Reserved.
+#
+# Contributor(s):
+#   Max Kanat-Alexander <mkanat@bugzilla.org>
+
+package Bugzilla::Search::Recent;
+use strict;
+use base qw(Bugzilla::Object);
+
+use Bugzilla::Constants;
+use Bugzilla::Error;
+use Bugzilla::Util;
+
+#############
+# Constants #
+#############
+
+use constant DB_TABLE => 'profile_search';
+use constant LIST_ORDER => 'id';
+
+use constant REQUIRED_CREATE_FIELDS => ();
+
+use constant DB_COLUMNS => qw(
+    id
+    user_id
+    bug_list
+    list_order
+);
+
+use constant VALIDATORS => {
+    user_id    => \&_check_user_id,
+    bug_list   => \&_check_bug_list,
+    list_order => \&_check_list_order,
+};
+
+use constant UPDATE_COLUMNS => qw(bug_list list_order);
+
+###################
+# DB Manipulation #
+###################
+
+sub create {
+    my $class = shift;
+    my $dbh = Bugzilla->dbh;
+    $dbh->bz_start_transaction();
+    my $search = $class->SUPER::create(@_);   
+
+    # Enforce there only being SAVE_NUM_SEARCHES per user.
+    my ($num_searches, $min_id) = $dbh->selectrow_array(
+        'SELECT COUNT(*), MIN(id) FROM profile_search WHERE user_id = ?',
+        undef, $search->user_id);
+    if ($num_searches > SAVE_NUM_SEARCHES) {
+        $dbh->do('DELETE FROM profile_search WHERE id = ?', undef, $min_id);
+    }
+    $dbh->bz_commit_transaction();
+    return $search;
+}
+
+sub create_placeholder {
+    my $class = shift;
+    return $class->create({ user_id  => Bugzilla->user->id,
+                            bug_list => '' });
+}
+
+###############
+# Constructor #
+###############
+
+sub check {
+    my $class = shift;
+    my $search = $class->SUPER::check(@_);
+    my $user = Bugzilla->user;
+    if ($search->user_id != $user->id) {
+        ThrowUserError('object_not_found', { id => $search->id });
+    }
+    return $search;
+}
+
+####################
+# Simple Accessors #
+####################
+
+sub bug_list   { return [split(',', $_[0]->{'bug_list'})]; }
+sub list_order { return $_[0]->{'list_order'}; }
+sub user_id    { return $_[0]->{'user_id'}; }
+
+############
+# Mutators #
+############
+
+sub set_bug_list   { $_[0]->set('bug_list',   $_[1]); }
+sub set_list_order { $_[0]->set('list_order', $_[1]); }
+
+##############
+# Validators #
+##############
+
+sub _check_user_id {
+    my ($invocant, $id) = @_;
+    require Bugzilla::User;
+    return Bugzilla::User->check({ id => $id })->id;
+}
+
+sub _check_bug_list {
+    my ($invocant, $list) = @_;
+
+    my @bug_ids = ref($list) ? @$list : split(',', $list);
+    detaint_natural($_) foreach @bug_ids;
+    return join(',', @bug_ids);
+}
+
+sub _check_list_order { defined $_[1] ? trim($_[1]) : '' }
+
+1;
+
+__END__
+
+=head1 NAME
+
+Bugzilla::Search::Recent - A search recently run by a logged-in user.
+
+=head1 SYNOPSIS
+
+ use Bugzilla::Search::Recent;
+
+
+=head1 DESCRIPTION
+
+This is an implementation of L<Bugzilla::Object>, and so has all the
+same methods available as L<Bugzilla::Object>, in addition to what is
+documented below.

Bugzilla/Template.pm

@@ -764,15 +764,6 @@ sub create {
             # Whether or not keywords are enabled, in this Bugzilla.
             'use_keywords' => sub { return Bugzilla::Keyword->any_exist; },
 
-            'last_bug_list' => sub {
-                my @bug_list;
-                my $cgi = Bugzilla->cgi;
-                if ($cgi->cookie("BUGLIST")) {
-                    @bug_list = split(/:/, $cgi->cookie("BUGLIST"));
-                }
-                return \@bug_list;
-            },
-
             'feature_enabled' => sub { return Bugzilla->feature(@_); },
 
             # field_descs can be somewhat slow to generate, so we generate

Bugzilla/User.pm

@@ -44,6 +44,7 @@ package Bugzilla::User;
 use Bugzilla::Error;
 use Bugzilla::Util;
 use Bugzilla::Constants;
+use Bugzilla::Search::Recent;
 use Bugzilla::User::Setting;
 use Bugzilla::Product;
 use Bugzilla::Classification;
@@ -51,8 +52,11 @@ use Bugzilla::Field;
 use Bugzilla::Group;
 
 use DateTime::TimeZone;
+use List::Util qw(max);
 use Scalar::Util qw(blessed);
 use Storable qw(dclone);
+use URI;
+use URI::QueryParam;
 
 use base qw(Bugzilla::Object Exporter);
 @Bugzilla::User::EXPORT = qw(is_available_username
@@ -361,6 +365,149 @@ sub queries_available {
     return $self->{queries_available};
 }
 
+##########################
+# Saved Recent Bug Lists #
+##########################
+
+sub recent_searches {
+    my $self = shift;
+    $self->{recent_searches} ||= 
+        Bugzilla::Search::Recent->match({ user_id => $self->id });
+    return $self->{recent_searches};
+}
+
+sub recent_search_containing {
+    my ($self, $bug_id) = @_;
+    my $searches = $self->recent_searches;
+
+    foreach my $search (@$searches) {
+        return $search if grep($_ == $bug_id, @{ $search->bug_list });
+    }
+
+    return undef;
+}
+
+sub recent_search_for {
+    my ($self, $bug) = @_;
+    my $params = Bugzilla->input_params;
+    my $cgi = Bugzilla->cgi;
+
+    if ($self->id) {
+        # First see if there's a list_id parameter in the query string.
+        my $list_id = $params->{list_id};
+        if (!$list_id) {
+            # If not, check for "list_id" in the query string of the referer.
+            my $referer = $cgi->referer;
+            if ($referer) {
+                my $uri = URI->new($referer);
+                if ($uri->path =~ /buglist\.cgi$/) {
+                    $list_id = $uri->query_param('list_id')
+                               || $uri->query_param('regetlastlist');
+                }
+            }
+        }
+
+        if ($list_id) {
+            # If we got a bad list_id (either some other user's or an expired
+            # one) don't crash, just don't return that list.
+            my $search = 
+                eval { Bugzilla::Search::Recent->check({ id => $list_id }) };
+            return $search if $search;
+        }
+
+        # If there's no list_id, see if the current bug's id is contained
+        # in any of the user's saved lists.
+        my $search = $self->recent_search_containing($bug->id);
+        return $search if $search;
+    }
+
+    # Finally (or always, if we're logged out), if there's a BUGLIST cookie
+    # and the selected bug is in the list, then return the cookie as a fake
+    # Search::Recent object.
+    if (my $list = $cgi->cookie('BUGLIST')) {
+        my @bug_ids = split(':', $list);
+        if (grep { $_ == $bug->id } @bug_ids) {
+            return { id => 'cookie', bug_list => \@bug_ids };
+        }
+    }
+
+    return undef;
+}
+
+sub save_last_search {
+    my ($self, $params) = @_;
+    my ($bug_ids, $order, $vars, $list_id) = 
+        @$params{qw(bugs order vars list_id)};
+
+    my $cgi = Bugzilla->cgi;
+    if ($order) {
+        $cgi->send_cookie(-name => 'LASTORDER',
+                          -value => $order,
+                          -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
+    }
+
+    return if !@$bug_ids;
+
+    if ($self->id) {
+        on_main_db {
+            my $search;
+            if ($list_id) {
+                # Use eval so that people can still use old search links or
+                # links that don't belong to them.
+                $search = eval { Bugzilla::Search::Recent->check(
+                                    { id => $list_id }) };
+            }
+
+            if ($search) {
+                # We only update placeholders. (Placeholders are
+                # Saved::Search::Recent objects with empty bug lists.)
+                # Otherwise, we could just keep creating new searches
+                # for the same refreshed list over and over.
+                if (!@{ $search->bug_list }) {
+                    $search->set_list_order($order);
+                    $search->set_bug_list($bug_ids);
+                    $search->update();
+                }
+            }
+            else {
+                # If we already have an existing search with a totally
+                # identical bug list, then don't create a new one. This
+                # prevents people from writing over their whole 
+                # recent-search list by just refreshing a saved search
+                # (which doesn't have list_id in the header) over and over.
+                my $list_string = join(',', @$bug_ids);
+                my $existing_search = Bugzilla::Search::Recent->match({
+                    user_id => $self->id, bug_list => $list_string });
+           
+                if (!scalar(@$existing_search)) {
+                    Bugzilla::Search::Recent->create({
+                        user_id    => $self->id,
+                        bug_list   => $bug_ids,
+                        list_order => $order });
+                }
+            }
+        };
+        delete $self->{recent_searches};
+    }
+    # Logged-out users use a cookie to store a single last search. We don't
+    # override that cookie with the logged-in user's latest search, because
+    # if they did one search while logged out and another while logged in,
+    # they may still want to navigate through the search they made while
+    # logged out.
+    else {
+        my $bug_list = join(":", @$bug_ids);
+        if (length($bug_list) < 4000) {
+            $cgi->send_cookie(-name => 'BUGLIST',
+                              -value => $bug_list,
+                              -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
+        }
+        else {
+            $cgi->remove_cookie('BUGLIST');
+            $vars->{'toolong'} = 1;
+        }
+    }
+}
+
 sub settings {
     my ($self) = @_;
 

Bugzilla/Util.pm

@@ -37,7 +37,7 @@ use base qw(Exporter);
                              css_class_quote html_light_quote url_decode
                              i_am_cgi correct_urlbase remote_ip
                              do_ssl_redirect_if_required use_attachbase
-                             diff_arrays
+                             diff_arrays on_main_db
                              trim wrap_hard wrap_comment find_wrap_point
                              format_time format_time_decimal validate_date
                              validate_time datetime_from
@@ -597,6 +597,14 @@ sub clean_text {
     return trim($dtext);
 }
 
+sub on_main_db (&) {
+    my $code = shift;
+    my $original_dbh = Bugzilla->dbh;
+    Bugzilla->request_cache->{dbh} = Bugzilla->dbh_main;
+    $code->();
+    Bugzilla->request_cache->{dbh} = $original_dbh;
+}
+
 sub get_text {
     my ($name, $vars) = @_;
     my $template = Bugzilla->template_inner;
@@ -690,6 +698,11 @@ Bugzilla::Util - Generic utility functions for bugzilla
   validate_email_syntax($email);
   validate_date($date);
 
+  # DB-related functions
+  on_main_db {
+     ... code here ...
+  };
+
 =head1 DESCRIPTION
 
 This package contains various utility functions which do not belong anywhere
@@ -994,3 +1007,20 @@ Make sure the date has the correct format and returns 1 if
 the check is successful, else returns 0.
 
 =back
+
+=head2 Database
+
+=over
+
+=item C<on_main_db>
+
+Runs a block of code always on the main DB. Useful for when you're inside
+a subroutine and need to do some writes to the database, but don't know
+if Bugzilla is currently using the shadowdb or not. Used like:
+
+ on_main_db {
+     my $dbh = Bugzilla->dbh;
+     $dbh->do("INSERT ...");
+ }
+
+back

buglist.cgi

@@ -40,6 +40,7 @@ use Bugzilla::Error;
 use Bugzilla::Util;
 use Bugzilla::Search;
 use Bugzilla::Search::Quicksearch;
+use Bugzilla::Search::Recent;
 use Bugzilla::Search::Saved;
 use Bugzilla::User;
 use Bugzilla::Bug;
@@ -60,7 +61,7 @@ my $buffer = $cgi->query_string();
 # We have to check the login here to get the correct footer if an error is
 # thrown and to prevent a logged out user to use QuickSearch if 'requirelogin'
 # is turned 'on'.
-Bugzilla->login();
+my $user = Bugzilla->login();
 
 if (length($buffer) == 0) {
     print $cgi->header(-refresh=> '10; URL=query.cgi');
@@ -86,6 +87,18 @@ if (grep { $_ =~ /^cmd\-/ } $cgi->param()) {
 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;
@@ -185,17 +198,26 @@ my $params;
 
 # If the user is retrieving the last bug list they looked at, hack the buffer
 # storing the query string so that it looks like a query retrieving those bugs.
-if (defined $cgi->param('regetlastlist')) {
-    $cgi->cookie('BUGLIST') || ThrowUserError("missing_cookie");
-
-    $order = "reuse last sort" unless $order;
-    my $bug_id = $cgi->cookie('BUGLIST');
-    $bug_id =~ s/:/,/g;
+if (my $last_list = $cgi->param('regetlastlist')) {
+    my ($bug_ids, $order);
+
+    # Logged-out users use the old cookie method for storing the last search.
+    if (!$user->id or $last_list eq 'cookie') {
+        $cgi->cookie('BUGLIST') || ThrowUserError("missing_cookie");
+        $order = "reuse last sort" unless $order;
+        $bug_ids = $cgi->cookie('BUGLIST');
+        $bug_ids =~ s/:/,/g;
+    }
+    # But logged in users store the last X searches in the DB so they can
+    # have multiple bug lists available.
+    else {
+        my $last_search = Bugzilla::Search::Recent->check(
+            { id => $last_list });
+        $bug_ids = join(',', @{ $last_search->bug_list });
+        $order   = $last_search->list_order if !$order;
+    }
     # set up the params for this new query
-    $params = new Bugzilla::CGI({
-                                 bug_id => $bug_id,
-                                 order => $order,
-                                });
+    $params = new Bugzilla::CGI({ bug_id => $bug_ids, order => $order });
 }
 
 # Figure out whether or not the user is doing a fulltext search.  If not,
@@ -431,7 +453,7 @@ if ($cmdtype eq "dorem") {
         $order = $params->param('order') || $order;
     }
     elsif ($remaction eq "forget") {
-        my $user = Bugzilla->login(LOGIN_REQUIRED);
+        $user = Bugzilla->login(LOGIN_REQUIRED);
         # Copy the name into a variable, so that we can trick_taint it for
         # the DB. We know it's safe, because we're using placeholders in 
         # the SQL, and the SQL is only a DELETE.
@@ -495,12 +517,12 @@ if ($cmdtype eq "dorem") {
 }
 elsif (($cmdtype eq "doit") && defined $cgi->param('remtype')) {
     if ($cgi->param('remtype') eq "asdefault") {
-        my $user = Bugzilla->login(LOGIN_REQUIRED);
+        $user = Bugzilla->login(LOGIN_REQUIRED);
         InsertNamedQuery(DEFAULT_QUERY_NAME, $buffer);
         $vars->{'message'} = "buglist_new_default_query";
     }
     elsif ($cgi->param('remtype') eq "asnamed") {
-        my $user = Bugzilla->login(LOGIN_REQUIRED);
+        $user = Bugzilla->login(LOGIN_REQUIRED);
         my $query_name = $cgi->param('newqueryname');
         my $new_query = $cgi->param('newquery');
         my $query_type = QUERY_LIST;
@@ -1177,26 +1199,11 @@ my $contenttype;
 my $disposition = "inline";
 
 if ($format->{'extension'} eq "html" && !$agent) {
-    if ($order && !$cgi->param('sharer_id')) {
-        $cgi->send_cookie(-name => 'LASTORDER',
-                          -value => $order,
-                          -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
-    }
-    my $bugids = join(":", @bugidlist);
-    # See also Bug 111999
-    if (length($bugids) == 0) {
-        $cgi->remove_cookie('BUGLIST');
-    }
-    elsif (length($bugids) < 4000) {
-        $cgi->send_cookie(-name => 'BUGLIST',
-                          -value => $bugids,
-                          -expires => 'Fri, 01-Jan-2038 00:00:00 GMT');
+    if (!$cgi->param('regetlastlist')) {
+        Bugzilla->user->save_last_search(
+            { bugs => \@bugidlist, order => $order, vars => $vars,
+              list_id => scalar $cgi->param('list_id') });
     }
-    else {
-        $cgi->remove_cookie('BUGLIST');
-        $vars->{'toolong'} = 1;
-    }
-
     $contenttype = "text/html";
 }
 else {

template/en/default/bug/navigate.html.tmpl

@@ -36,45 +36,41 @@
 
 
 <div class="navigation">
-[% IF last_bug_list.size > 0 %]
-  [% this_bug_idx = lsearch(last_bug_list, bug.id) %]
+[% SET my_search = user.recent_search_for(bug) %]
+[% IF my_search %]
+  [% SET last_bug_list = my_search.bug_list %]
+  [% SET this_bug_idx = lsearch(last_bug_list, bug.id) %]
   <b>[% terms.Bug %] List:</b>
-  [% IF this_bug_idx != -1 %]
-    ([% this_bug_idx + 1 %] of [% last_bug_list.size %])
-  [% END %]
 
-  [% IF this_bug_idx != -1 %]
-    <a href="show_bug.cgi?id=
-             [%- last_bug_list.first FILTER url_quote %]">First</a>
+  ([% this_bug_idx + 1 %] of [% last_bug_list.size %])
+
+  <a href="show_bug.cgi?id=
+           [%- last_bug_list.first FILTER url_quote %]&amp;list_id=
+           [%- my_search.id FILTER url_quote %]">First</a>
+  <a href="show_bug.cgi?id=
+           [%- last_bug_list.last FILTER url_quote %]&amp;list_id=
+           [%- my_search.id FILTER url_quote %]">Last</a>
+
+  [% IF this_bug_idx > 0 %]
+    [% prev_bug = this_bug_idx - 1 %]
     <a href="show_bug.cgi?id=
-             [%- last_bug_list.last FILTER url_quote %]">Last</a>
+             [%- last_bug_list.$prev_bug FILTER url_quote %]&amp;list_id=
+             [%- my_search.id FILTER url_quote %]">Prev</a>
+  [% ELSE %]
+    <i><font color="#777777">Prev</font></i>
   [% END %]
 
-  [% IF bug.bug_id %]
-    [% IF this_bug_idx != -1 %]
-      [% IF this_bug_idx > 0 %]
-        [% prev_bug = this_bug_idx - 1 %]
-        <a href="show_bug.cgi?id=
-                 [%- last_bug_list.$prev_bug FILTER url_quote %]">Prev</a>
-      [% ELSE %]
-        <i><font color="#777777">Prev</font></i>
-      [% END %]
-
-      [% IF this_bug_idx + 1 < last_bug_list.size %]
-        [% next_bug = this_bug_idx + 1 %]
-        <a href="show_bug.cgi?id=
-                 [%- last_bug_list.$next_bug FILTER url_quote %]">Next</a>
-      [% ELSE %]
-        <i><font color="#777777">Next</font></i>
-      [% END %]
-    [% ELSE %]
-      (This [% terms.bug %] is not in your last search results)
-    [% END %]
+  [% IF this_bug_idx + 1 < last_bug_list.size %]
+    [% next_bug = this_bug_idx + 1 %]
+    <a href="show_bug.cgi?id=
+             [%- last_bug_list.$next_bug FILTER url_quote %]&amp;list_id=
+             [%- my_search.id FILTER url_quote %]">Next</a>
   [% ELSE %]
-    &nbsp;&nbsp;
+    <i><font color="#777777">Next</font></i>
   [% END %]
 
-  &nbsp;&nbsp;<a href="buglist.cgi?regetlastlist=1">Show last search results</a>
+  &nbsp;&nbsp;<a href="buglist.cgi?regetlastlist=
+              [%- my_search.id FILTER url_quote %]">Show last search results</a>
 [% ELSE %]
   [%# With no list, don't show link to search results %]
   <i><font color="#777777">First</font></i>
@@ -82,6 +78,7 @@
   <i><font color="#777777">Prev</font></i>
   <i><font color="#777777">Next</font></i>
   &nbsp;&nbsp;
-  <i><font color="#777777">No search results available</font></i>
+  <i><font color="#777777">This [% terms.bug %] is not in your last
+    search results.</font></i>
 [% END %]
 </div>

template/en/default/global/user-error.html.tmpl

@@ -1783,6 +1783,8 @@
     group
   [% ELSIF class == "Bugzilla::Product" %]
     product
+  [% ELSIF class == "Bugzilla::Search::Recent" %]
+    recent search
   [% ELSIF class == "Bugzilla::Search::Saved" %]
     saved search
   [% ELSIF ( matches = class.match('^Bugzilla::Field::Choice::(.+)') ) %]