From 7f94705675428a544f82d485f79f60f052e67fdf Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Sat, 4 Nov 2006 07:16:46 +0000 Subject: Bug 352403: Create an object for saved searches, and have Bugzilla::User use it Patch By Max Kanat-Alexander r=LpSolit, a=myk --- Bugzilla/Group.pm | 27 +++ Bugzilla/Search/Saved.pm | 186 +++++++++++++++++++++ Bugzilla/User.pm | 164 +++++++++--------- .../default/account/prefs/saved-searches.html.tmpl | 19 +-- .../en/default/global/per-bug-queries.html.tmpl | 2 +- .../en/default/global/site-navigation.html.tmpl | 8 + template/en/default/global/useful-links.html.tmpl | 29 ++-- template/en/default/global/user-error.html.tmpl | 2 +- userprefs.cgi | 148 ++++++---------- 9 files changed, 382 insertions(+), 203 deletions(-) create mode 100644 Bugzilla/Search/Saved.pm diff --git a/Bugzilla/Group.pm b/Bugzilla/Group.pm index 0f7771efe..c80d2333c 100644 --- a/Bugzilla/Group.pm +++ b/Bugzilla/Group.pm @@ -81,6 +81,20 @@ sub _rederive_regexp { RederiveRegexp($self->user_regexp, $self->id); } +sub members_non_inherited { + my ($self) = @_; + return $self->{members_non_inherited} + if exists $self->{members_non_inherited}; + + my $member_ids = Bugzilla->dbh->selectcol_arrayref( + 'SELECT DISTINCT user_id FROM user_group_map + WHERE isbless = 0 AND group_id = ?', + undef, $self->id) || []; + require Bugzilla::User; + $self->{members_non_inherited} = Bugzilla::User->new_from_list($member_ids); + return $self->{members_non_inherited}; +} + ################################ ##### Module Subroutines ### ################################ @@ -246,3 +260,16 @@ be a member of this group. and undef otherwise. =back + +=head1 METHODS + +=over + +=item C + +Returns an arrayref of L objects representing people who are +"directly" in this group, meaning that they're in it because they match +the group regular expression, or they have been actually added to the +group manually. + +=back diff --git a/Bugzilla/Search/Saved.pm b/Bugzilla/Search/Saved.pm new file mode 100644 index 000000000..2cb53439d --- /dev/null +++ b/Bugzilla/Search/Saved.pm @@ -0,0 +1,186 @@ +# -*- 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. +# Portions created by Everything Solved are Copyright (C) 2006 +# Everything Solved. All Rights Reserved. +# +# Contributor(s): Max Kanat-Alexander + +use strict; + +package Bugzilla::Search::Saved; + +use base qw(Bugzilla::Object); + +use Bugzilla::CGI; +use Bugzilla::Constants; +use Bugzilla::Group; +use Bugzilla::Search qw(IsValidQueryType); +use Bugzilla::User; + +############# +# Constants # +############# + +use constant DB_TABLE => 'namedqueries'; + +use constant DB_COLUMNS => qw( + id + userid + name + query + query_type +); + +##################### +# Complex Accessors # +##################### + +sub edit_link { + my ($self) = @_; + return $self->{edit_link} if defined $self->{edit_link}; + my $cgi = new Bugzilla::CGI($self->url); + if (!$cgi->param('query_type') + || !IsValidQueryType($cgi->param('query_type'))) + { + $cgi->param('query_type', 'advanced'); + } + $self->{edit_link} = $cgi->canonicalise_query; + return $self->{edit_link}; +} + +sub used_in_whine { + my ($self) = @_; + return $self->{used_in_whine} if exists $self->{used_in_whine}; + ($self->{used_in_whine}) = Bugzilla->dbh->selectrow_array( + 'SELECT 1 FROM whine_events INNER JOIN whine_queries + ON whine_events.id = whine_queries.eventid + WHERE whine_events.owner_userid = ? AND query_name = ?', undef, + $self->{userid}, $self->name) || 0; + return $self->{used_in_whine}; +} + +sub link_in_footer { + my ($self, $user) = @_; + # We only cache link_in_footer for the current Bugzilla->user. + return $self->{link_in_footer} if exists $self->{link_in_footer} && !$user; + my $user_id = $user ? $user->id : Bugzilla->user->id; + my $link_in_footer = Bugzilla->dbh->selectrow_array( + 'SELECT 1 FROM namedqueries_link_in_footer + WHERE namedquery_id = ? AND user_id = ?', + undef, $self->id, $user_id) || 0; + $self->{link_in_footer} = $link_in_footer if !$user; + return $link_in_footer; +} + +sub shared_with_group { + my ($self) = @_; + return $self->{shared_with_group} if exists $self->{shared_with_group}; + # Bugzilla only currently supports sharing with one group, even + # though the database backend allows for an infinite number. + my ($group_id) = Bugzilla->dbh->selectrow_array( + 'SELECT group_id FROM namedquery_group_map WHERE namedquery_id = ?', + undef, $self->id); + $self->{shared_with_group} = $group_id ? new Bugzilla::Group($group_id) + : undef; + return $self->{shared_with_group}; +} + +#################### +# Simple Accessors # +#################### + +sub bug_ids_only { return ($_[0]->{'query_type'} == LIST_OF_BUGS) ? 1 : 0; } +sub url { return $_[0]->{'query'}; } + +sub user { + my ($self) = @_; + return $self->{user} if defined $self->{user}; + $self->{user} = new Bugzilla::User($self->{userid}); + return $self->{user}; +} + +1; + +__END__ + +=head1 NAME + + Bugzilla::Search::Saved - A saved search + +=head1 SYNOPSIS + + use Bugzilla::Search::Saved; + + my $query = new Bugzilla::Search::Saved($query_id); + + my $edit_link = $query->edit_link; + my $search_url = $query->url; + my $owner = $query->user; + +=head1 DESCRIPTION + +This module exists to represent a L that has been +saved to the database. + +This is an implementation of L, and so has all the +same methods available as L, in addition to what is +documented below. + +=head1 METHODS + +=head2 Constructors and Database Manipulation + +=over + +=item C + +Does not accept a bare C argument. Instead, accepts only an id. + +See also: L. + +=back + + +=head2 Accessors + +These return data about the object, without modifying the object. + +=over + +=item C + +A url with which you can edit the search. + +=item C + +The CGI parameters for the search, as a string. + +=item C + +Whether or not this search should be displayed in the footer for the +I (not the owner of the search, but the person actually +using Bugzilla right now). + +=item C + +True if the search contains only a list of Bug IDs. + +=item C + +The L that this search is shared with. C if +this search isn't shared. + +=back diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm index 28c79254c..cadc8574d 100644 --- a/Bugzilla/User.pm +++ b/Bugzilla/User.pm @@ -272,56 +272,61 @@ sub nick { sub queries { my $self = shift; - return $self->{queries} if defined $self->{queries}; return [] unless $self->id; my $dbh = Bugzilla->dbh; - my $used_in_whine_ref = $dbh->selectall_hashref(' - SELECT DISTINCT query_name - FROM whine_events we - INNER JOIN whine_queries wq - ON we.id = wq.eventid - WHERE we.owner_userid = ?', - 'query_name', undef, $self->id); - - # If the user is in any group, there may be shared queries to be included. - my $or_nqgm_group_id_in_usergroups = ''; - if ($self->groups_as_string) { - $or_nqgm_group_id_in_usergroups = - 'OR MAX(nqgm.group_id) IN (' . $self->groups_as_string . ') '; - } + my $query_ids = $dbh->selectcol_arrayref( + 'SELECT id FROM namedqueries WHERE userid = ?', undef, $self->id); + require Bugzilla::Search::Saved; + $self->{queries} = Bugzilla::Search::Saved->new_from_list($query_ids); + return $self->{queries}; +} - my $queries_ref = $dbh->selectall_arrayref(' - SELECT nq.id, MAX(userid) AS userid, name, query, query_type, - MAX(nqgm.group_id) AS shared_with_group, - COUNT(nql.namedquery_id) AS link_in_footer - FROM namedqueries AS nq - LEFT JOIN namedquery_group_map nqgm - ON nqgm.namedquery_id = nq.id - LEFT JOIN namedqueries_link_in_footer AS nql - ON nql.namedquery_id = nq.id - AND nql.user_id = ? ' . - $dbh->sql_group_by('nq.id', 'name, query, query_type') . - ' HAVING MAX(nq.userid) = ? ' . - $or_nqgm_group_id_in_usergroups . - ' ORDER BY UPPER(name)', - {'Slice'=>{}}, $self->id, $self->id); - - foreach my $queries_hash (@$queries_ref) { - # For each query, determine whether it's being used in a whine. - if (exists($$used_in_whine_ref{$queries_hash->{'name'}})) { - $queries_hash->{'usedinwhine'} = 1; - } +sub queries_subscribed { + my $self = shift; + return $self->{queries_subscribed} if defined $self->{queries_subscribed}; + return [] unless $self->id; - # For shared queries, provide the sharer's user object. - if ($queries_hash->{'userid'} != $self->id) { - $queries_hash->{'user'} = new Bugzilla::User($queries_hash->{'userid'}); - } - } - $self->{queries} = $queries_ref; + # Exclude the user's own queries. + my @my_query_ids = map($_->id, @{$self->queries}); + my $query_id_string = join(',', @my_query_ids) || '-1'; + + # Only show subscriptions that we can still actually see. If a + # user changes the shared group of a query, our subscription + # will remain but we won't have access to the query anymore. + my $subscribed_query_ids = Bugzilla->dbh->selectcol_arrayref( + "SELECT lif.namedquery_id + FROM namedqueries_link_in_footer lif + INNER JOIN namedquery_group_map ngm + ON ngm.namedquery_id = lif.namedquery_id + WHERE lif.user_id = ? + AND lif.namedquery_id NOT IN ($query_id_string) + AND ngm.group_id IN (" . $self->groups_as_string . ")", + undef, $self->id); + require Bugzilla::Search::Saved; + $self->{queries_subscribed} = + Bugzilla::Search::Saved->new_from_list($subscribed_query_ids); + return $self->{queries_subscribed}; +} - return $self->{queries}; +sub queries_available { + my $self = shift; + return $self->{queries_available} if defined $self->{queries_available}; + return [] unless $self->id; + + # Exclude the user's own queries. + my @my_query_ids = map($_->id, @{$self->queries}); + my $query_id_string = join(',', @my_query_ids) || '-1'; + + my $avail_query_ids = Bugzilla->dbh->selectcol_arrayref( + 'SELECT namedquery_id FROM namedquery_group_map + WHERE group_id IN (' . $self->groups_as_string . ") + AND namedquery_id NOT IN ($query_id_string)"); + require Bugzilla::Search::Saved; + $self->{queries_available} = + Bugzilla::Search::Saved->new_from_list($avail_query_ids); + return $self->{queries_available}; } sub settings { @@ -345,6 +350,8 @@ sub flush_queries_cache { my $self = shift; delete $self->{queries}; + delete $self->{queries_subscribed}; + delete $self->{queries_available}; } sub groups { @@ -1663,6 +1670,42 @@ confirmation screen. =head1 METHODS +=head2 Saved and Shared Queries + +=over + +=item C + +Returns an arrayref of the user's own saved queries, sorted by name. The +array contains L objects. + +=item C + +Returns an arrayref of shared queries that the user has subscribed to. +That is, these are shared queries that the user sees in their footer. +This array contains L objects. + +=item C + +Returns an arrayref of all queries to which the user could possibly +subscribe. This includes the contents of L. +An array of L objects. + +=item C + +Some code modifies the set of stored queries. Because C does +not handle these modifications, but does cache the result of calling C +internally, such code must call this method to flush the cached result. + +=item C + +An arrayref of group ids. The user can share their own queries with these +groups. + +=back + +=head2 Other Methods + =over =item C @@ -1711,35 +1754,6 @@ returned. Sets the L object to be returned by C. Should only be called by C, for the most part. -=item C - -Returns an array of the user's named queries, sorted in a case-insensitive -order by name. Each entry is a hash with five keys: - -=over - -=item * - -id - The ID of the query - -=item * - -userid - The query owner's user ID - -=item * - -name - The name of the query - -=item * - -query - The text for the query - -=item * - -link_in_footer - Whether or not the query should be displayed in the footer. - -=back - =item C Returns the disable text of the user, if any. @@ -1758,12 +1772,6 @@ value - the value of this setting for this user. Will be the same is_default - a boolean to indicate whether the user has chosen to make a preference for themself or use the site default. -=item C - -Some code modifies the set of stored queries. Because C does -not handle these modifications, but does cache the result of calling C -internally, such code must call this method to flush the cached result. - =item C Returns a hashref of group names for groups the user is a member of. The keys diff --git a/template/en/default/account/prefs/saved-searches.html.tmpl b/template/en/default/account/prefs/saved-searches.html.tmpl index 0e2fdfba1..e4bbc51ae 100644 --- a/template/en/default/account/prefs/saved-searches.html.tmpl +++ b/template/en/default/account/prefs/saved-searches.html.tmpl @@ -20,8 +20,6 @@ #%] [%# INTERFACE: - # queries: list of the named queries visible to the user, both own and shared - # by others. Cleaned-up result of Bugzilla::User::queries. # queryshare_groups: list of groups the user may share queries with # (id, name). #%] @@ -81,18 +79,18 @@ — - [% FOREACH q = queries %] - [% NEXT UNLESS q.userid == user.id %] + [% FOREACH q = user.queries %] [% q.name FILTER html %] Run - Edit + Edit - [% IF q.usedinwhine %] + [% IF q.used_in_whine %] Remove from whining first [% ELSE %] Don't share [% FOREACH group = queryshare_groups %] + [% ' selected="selected"' + IF q.shared_with_group.id == group.id %] + >[% group.name FILTER html %] [% END %] [% ELSE %] @@ -144,8 +144,7 @@ [% found_shared_query = 0 %] - [% FOREACH q = queries %] - [% NEXT IF q.userid == user.id %] + [% FOREACH q = user.queries_available %] [% found_shared_query = 1 %] [% q.name FILTER html %] @@ -153,7 +152,7 @@ Run + [% q.user.id FILTER url_quote %]">Run [% END %] + [% FOREACH q = user.queries_subscribed %] + + [% END %] + [%# *** Bugzilla Administration Tools *** %] [% IF user.login %] [% '
- Saved Searches: + Saved Searches:
    [% IF user.showmybugslink %] @@ -99,7 +101,6 @@ [% END %] [% FOREACH q = user.queries %] - [% NEXT IF q.userid != user.id %] [% IF q.link_in_footer %]
  • [% '| ' IF print_pipe %] [% q.name FILTER html %]
    • @@ -108,22 +109,20 @@ [% END %] [% new_line = print_pipe %] [% print_pipe = 0 %] - [% FOREACH q = user.queries %] - [% NEXT IF q.userid == user.id %] - [% IF q.link_in_footer %] - [% IF new_line %] -
    - [% new_line = 0 %] - [% END %] -
  • + [% FOREACH q = user.queries_subscribed %] + [% IF new_line %] +
    + [% new_line = 0 %] + [% END %] +
  • [% '| ' IF print_pipe %] [% q.name FILTER html FILTER no_break %]
    • - [% print_pipe = 1 %] - [% END %] + title="Shared by [% q.user.identity FILTER html %]" + >[% q.name FILTER html FILTER no_break %] + [% print_pipe = 1 %] [% END %]
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index eebefa927..df5f767ea 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -1537,7 +1537,7 @@ [% FOREACH q = Bugzilla.user.queries %] [% IF q.name == namedcmd %] - or edit + or edit [% END %] [% END %] diff --git a/userprefs.cgi b/userprefs.cgi index e8a045c4e..4bb65c152 100755 --- a/userprefs.cgi +++ b/userprefs.cgi @@ -378,30 +378,9 @@ sub DoPermissions { sub DoSavedSearches { - # 2004-12-13 - colin.ogilvie@gmail.com, bug 274397 - # Need to work around the possibly missing query_format=advanced my $dbh = Bugzilla->dbh; my $user = Bugzilla->user; - my @queries = @{$user->queries}; - my @newqueries; - foreach my $q (@queries) { - if ($q->{'query'} =~ /query_format=([^&]*)/) { - my $format = $1; - if (!IsValidQueryType($format)) { - if ($format eq "") { - $q->{'query'} =~ s/query_format=/query_format=advanced/; - } - else { - $q->{'query'} .= '&query_format=advanced'; - } - } - } else { - $q->{'query'} .= '&query_format=advanced'; - } - push @newqueries, $q; - } - $vars->{'queries'} = \@newqueries; if ($user->queryshare_groups_as_string) { $vars->{'queryshare_groups'} = Bugzilla::Group->new_from_list($user->queryshare_groups); @@ -416,20 +395,12 @@ sub SaveSavedSearches { # We'll need this in a loop, so do the call once. my $user_id = $user->id; - my @queries = @{$user->queries}; - my $sth_check_nl = $dbh->prepare('SELECT COUNT(*) - FROM namedqueries_link_in_footer - WHERE namedquery_id = ? - AND user_id = ?'); my $sth_insert_nl = $dbh->prepare('INSERT INTO namedqueries_link_in_footer (namedquery_id, user_id) VALUES (?, ?)'); my $sth_delete_nl = $dbh->prepare('DELETE FROM namedqueries_link_in_footer WHERE namedquery_id = ? AND user_id = ?'); - my $sth_check_ngm = $dbh->prepare('SELECT COUNT(*) - FROM namedquery_group_map - WHERE namedquery_id = ?'); my $sth_insert_ngm = $dbh->prepare('INSERT INTO namedquery_group_map (namedquery_id, group_id) VALUES (?, ?)'); @@ -438,84 +409,65 @@ sub SaveSavedSearches { WHERE namedquery_id = ?'); my $sth_delete_ngm = $dbh->prepare('DELETE FROM namedquery_group_map WHERE namedquery_id = ?'); - my $sth_direct_group_members = - $dbh->prepare('SELECT user_id - FROM user_group_map - WHERE group_id = ? - AND isbless = ' . GROUP_MEMBERSHIP . ' - AND grant_type = ' . GRANT_DIRECT); - foreach my $q (@queries) { - # Update namedqueries_link_in_footer. - $sth_check_nl->execute($q->{'id'}, $user_id); - my ($link_in_footer_entries) = $sth_check_nl->fetchrow_array(); - if (defined($cgi->param("link_in_footer_$q->{'id'}"))) { - if ($link_in_footer_entries == 0) { - $sth_insert_nl->execute($q->{'id'}, $user_id); - } + + # Update namedqueries_link_in_footer for this user. + foreach my $q (@{$user->queries}, @{$user->queries_available}) { + if (defined $cgi->param("link_in_footer_" . $q->id)) { + $sth_insert_nl->execute($q->id, $user_id) if !$q->link_in_footer; } else { - if ($link_in_footer_entries > 0) { - $sth_delete_nl->execute($q->{'id'}, $user_id); - } + $sth_delete_nl->execute($q->id, $user_id) if $q->link_in_footer; } + } - # For user's own queries, update namedquery_group_map. - if ($q->{'userid'} == $user_id) { - my ($group_id, $group_map_entries); - if ($user->in_group(Bugzilla->params->{'querysharegroup'})) { - $sth_check_ngm->execute($q->{'id'}); - ($group_map_entries) = $sth_check_ngm->fetchrow_array(); - $group_id = $cgi->param("share_$q->{'id'}") || ''; - } - if ($group_id) { - if (grep(/^\Q$group_id\E$/, @{$user->queryshare_groups()})) { - # $group_id is now definitely a valid ID of a group the - # user can see, so we can trick_taint. - trick_taint($group_id); - if ($group_map_entries == 0) { - $sth_insert_ngm->execute($q->{'id'}, $group_id); - } - else { - $sth_update_ngm->execute($group_id, $q->{'id'}); - } - - # If we're sharing our query with a group we can bless, - # we're subscribing direct group members to our search - # automatically. Otherwise, the group members need to - # opt in. This behaviour is deemed most likely to fit - # users' needs. - if ($user->can_bless($group_id)) { - $sth_direct_group_members->execute($group_id); - my $user_id; - while ($user_id = - $sth_direct_group_members->fetchrow_array()) { - next if $user_id == $user->id; - - $sth_check_nl->execute($q->{'id'}, $user_id); - my ($already_shown_in_footer) = - $sth_check_nl->fetchrow_array(); - if (! $already_shown_in_footer) { - $sth_insert_nl->execute($q->{'id'}, $user_id); - } - } - } - } - else { - # In the unlikely case somebody removed visibility to the - # group in the meantime, don't modify sharing. - } + # For user's own queries, update namedquery_group_map. + foreach my $q (@{$user->queries}) { + my $group_id; + + if ($user->in_group(Bugzilla->params->{'querysharegroup'})) { + $group_id = $cgi->param("share_" . $q->id) || ''; + } + + if ($group_id) { + # Don't allow the user to share queries with groups he's not + # allowed to. + next unless grep($_ eq $group_id, @{$user->queryshare_groups}); + + # $group_id is now definitely a valid ID of a group the + # user can share queries with, so we can trick_taint. + detaint_natural($group_id); + if ($q->shared_with_group) { + $sth_update_ngm->execute($group_id, $q->id); } else { - if ($group_map_entries > 0) { - $sth_delete_ngm->execute($q->{'id'}); - } + $sth_insert_ngm->execute($q->id, $group_id); + } - # Don't remove namedqueries_link_in_footer entries for users - # subscribing to the shared query. The idea is that they will - # probably want to be subscribers again should the sharing - # user choose to share the query again. + # If we're sharing our query with a group we can bless, we + # subscribe direct group members to our search automatically. + # Otherwise, the group members need to opt in. This behaviour + # is deemed most likely to fit users' needs. + if ($user->can_bless($group_id)) { + my $group = new Bugzilla::Group($group_id); + my $members = $group->members_non_inherited; + foreach my $member (@$members) { + next if $member->id == $user->id; + $sth_insert_nl->execute($q->id, $member->id) + if !$q->link_in_footer($member); + } } } + else { + # They have unshared that query. + if ($q->shared_with_group) { + $sth_delete_ngm->execute($q->id); + } + + # Don't remove namedqueries_link_in_footer entries for users + # subscribing to the shared query. The idea is that they will + # probably want to be subscribers again should the sharing + # user choose to share the query again. + } } $user->flush_queries_cache; -- cgit v1.2.3-24-g4f1b