From fbe9a7a9a22004e3cc23a61b84148da8a0c300e9 Mon Sep 17 00:00:00 2001 From: Max Kanat-Alexander Date: Tue, 15 Jun 2010 18:40:27 -0700 Subject: Bug 24896: Make the First/Last/Prev/Next navigation on bugs work with multiple buglists at once r=glob, a=mkanat --- Bugzilla/CGI.pm | 4 + Bugzilla/Constants.pm | 5 + Bugzilla/DB/Schema.pm | 12 ++ Bugzilla/Search/Recent.pm | 147 ++++++++++++++++++++++++ Bugzilla/Template.pm | 9 -- Bugzilla/User.pm | 147 ++++++++++++++++++++++++ Bugzilla/Util.pm | 32 +++++- buglist.cgi | 73 ++++++------ template/en/default/bug/navigate.html.tmpl | 61 +++++----- template/en/default/global/user-error.html.tmpl | 2 + 10 files changed, 417 insertions(+), 75 deletions(-) create mode 100644 Bugzilla/Search/Recent.pm diff --git a/Bugzilla/CGI.pm b/Bugzilla/CGI.pm index 00f23c393..224782152 100644 --- a/Bugzilla/CGI.pm +++ b/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) { diff --git a/Bugzilla/Constants.pm b/Bugzilla/Constants.pm index 875a50133..d4e8782c6 100644 --- a/Bugzilla/Constants.pm +++ b/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. diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm index 97e59efbd..2f74912c2 100644 --- a/Bugzilla/DB/Schema.pm +++ b/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, diff --git a/Bugzilla/Search/Recent.pm b/Bugzilla/Search/Recent.pm new file mode 100644 index 000000000..14d5ebef4 --- /dev/null +++ b/Bugzilla/Search/Recent.pm @@ -0,0 +1,147 @@ +# -*- 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 + +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, and so has all the +same methods available as L, in addition to what is +documented below. diff --git a/Bugzilla/Template.pm b/Bugzilla/Template.pm index 274ed8847..00d7b6d28 100644 --- a/Bugzilla/Template.pm +++ b/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 diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 936cf36e4..815b435fd 100644 --- a/Bugzilla/User.pm +++ b/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) = @_; diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index 03826c143..840225fbe 100644 --- a/Bugzilla/Util.pm +++ b/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 + +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 diff --git a/buglist.cgi b/buglist.cgi index 8741f434f..1972dd5b3 100755 --- a/buglist.cgi +++ b/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 { diff --git a/template/en/default/bug/navigate.html.tmpl b/template/en/default/bug/navigate.html.tmpl index 4a3d063af..869d338dc 100644 --- a/template/en/default/bug/navigate.html.tmpl +++ b/template/en/default/bug/navigate.html.tmpl @@ -36,45 +36,41 @@
-[% 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) %] [% terms.Bug %] List: - [% IF this_bug_idx != -1 %] - ([% this_bug_idx + 1 %] of [% last_bug_list.size %]) - [% END %] - [% IF this_bug_idx != -1 %] - First + ([% this_bug_idx + 1 %] of [% last_bug_list.size %]) + + First + Last + + [% IF this_bug_idx > 0 %] + [% prev_bug = this_bug_idx - 1 %] Last + [%- last_bug_list.$prev_bug FILTER url_quote %]&list_id= + [%- my_search.id FILTER url_quote %]">Prev + [% ELSE %] + Prev [% END %] - [% IF bug.bug_id %] - [% IF this_bug_idx != -1 %] - [% IF this_bug_idx > 0 %] - [% prev_bug = this_bug_idx - 1 %] - Prev - [% ELSE %] - Prev - [% END %] - - [% IF this_bug_idx + 1 < last_bug_list.size %] - [% next_bug = this_bug_idx + 1 %] - Next - [% ELSE %] - Next - [% 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 %] + Next [% ELSE %] -    + Next [% END %] -   Show last search results +   Show last search results [% ELSE %] [%# With no list, don't show link to search results %] First @@ -82,6 +78,7 @@ Prev Next    - No search results available + This [% terms.bug %] is not in your last + search results. [% END %]
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 1d5997861..669d6adb3 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/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::(.+)') ) %] -- cgit v1.2.3-24-g4f1b