Bug 352403: Create an object for saved searches, and have Bugzilla::User use it

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<members_non_inherited>
+
+Returns an arrayref of L<Bugzilla::User> 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

Bugzilla/Search/Saved.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.
+# Portions created by Everything Solved are Copyright (C) 2006 
+# Everything Solved. All Rights Reserved.
+#
+# Contributor(s): Max Kanat-Alexander <mkanat@bugzilla.org>
+
+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<Bugzilla::Search> that has been
+saved to the database.
+
+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.
+
+=head1 METHODS
+
+=head2 Constructors and Database Manipulation
+
+=over
+
+=item C<new>
+
+Does not accept a bare C<name> argument. Instead, accepts only an id.
+
+See also: L<Bugzilla::Object/new>.
+
+=back
+
+
+=head2 Accessors
+
+These return data about the object, without modifying the object.
+
+=over
+
+=item C<edit_link>
+
+A url with which you can edit the search.
+
+=item C<url>
+
+The CGI parameters for the search, as a string.
+
+=item C<link_in_footer>
+
+Whether or not this search should be displayed in the footer for the
+I<current user> (not the owner of the search, but the person actually
+using Bugzilla right now).
+
+=item C<bug_ids_only>
+
+True if the search contains only a list of Bug IDs.
+
+=item C<shared_with_group>
+
+The L<Bugzilla::Group> that this search is shared with. C<undef> if
+this search isn't shared.
+
+=back

Bugzilla/User.pm

@@ -272,58 +272,63 @@ 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 $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;
-        }
-
-        # 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;
-
+    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};
 }
 
+sub queries_subscribed {
+    my $self = shift;
+    return $self->{queries_subscribed} if defined $self->{queries_subscribed};
+    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';
+
+    # 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};
+}
+
+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 {
     my ($self) = @_;
 
@@ -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<queries>
+
+Returns an arrayref of the user's own saved queries, sorted by name. The 
+array contains L<Bugzilla::Search::Saved> objects.
+
+=item C<queries_subscribed>
+
+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<Bugzilla::Search::Saved> objects.
+
+=item C<queries_available>
+
+Returns an arrayref of all queries to which the user could possibly
+subscribe. This includes the contents of L</queries_subscribed>.
+An array of L<Bugzilla::Search::Saved> objects.
+
+=item C<flush_queries_cache>
+
+Some code modifies the set of stored queries. Because C<Bugzilla::User> does
+not handle these modifications, but does cache the result of calling C<queries>
+internally, such code must call this method to flush the cached result.
+
+=item C<queryshare_groups>
+
+An arrayref of group ids. The user can share their own queries with these
+groups.
+
+=back
+
+=head2 Other Methods
+
 =over
 
 =item C<id>
@@ -1711,35 +1754,6 @@ returned.
 Sets the L<Bugzilla::Auth> object to be returned by C<authorizer()>.
 Should only be called by C<Bugzilla::Auth::login>, for the most part.
 
-=item C<queries>
-
-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<disabledtext>
 
 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<flush_queries_cache>
-
-Some code modifies the set of stored queries. Because C<Bugzilla::User> does
-not handle these modifications, but does cache the result of calling C<queries>
-internally, such code must call this method to flush the cached result.
-
 =item C<groups>
 
 Returns a hashref of group names for groups the user is a member of. The keys

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 @@
         —
       </td>
     </tr>
-    [% FOREACH q = queries %]
-      [% NEXT UNLESS q.userid == user.id %]
+    [% FOREACH q = user.queries %]
       <tr>
         <td>[% q.name FILTER html %]</td>
         <td>
           <a href="buglist.cgi?cmdtype=runnamed&amp;namedcmd=[% q.name FILTER url_quote %]">Run</a>
         </td>
         <td>
-          <a href="query.cgi?[% q.query FILTER html %]&amp;known_name=[% q.name FILTER url_quote %]">Edit</a>
+          <a href="query.cgi?[% q.edit_link FILTER html %]&amp;known_name=
+                   [% q.name FILTER url_quote %]">Edit</a>
         </td>
         <td>
-          [% IF q.usedinwhine %]
+          [% IF q.used_in_whine %]
             Remove from <a href="editwhines.cgi">whining</a> first
           [% ELSE %]
             <a href="buglist.cgi?cmdtype=dorem&amp;remaction=forget&amp;namedcmd=
@@ -112,7 +110,9 @@
               <option value="">Don't share</option>
               [% FOREACH group = queryshare_groups %]
                 <option value="[% group.id %]"
-                        [% ' selected="selected"' IF q.shared_with_group == group.id %]>[% group.name FILTER html %]</option>
+                        [% ' selected="selected"' 
+                           IF q.shared_with_group.id == group.id %]
+                >[% group.name FILTER html %]</option>
               [% END %]
             </select>
           [% ELSE %]
@@ -144,8 +144,7 @@
       </th>
     </tr>
     [% found_shared_query = 0 %]
-    [% FOREACH q = queries %]
-      [% NEXT IF q.userid == user.id %]
+    [% FOREACH q = user.queries_available %]
       [% found_shared_query = 1 %]
       <tr>
         <td>[% q.name FILTER html %]</td>
@@ -153,7 +152,7 @@
         <td>
           <a href="buglist.cgi?cmdtype=dorem&amp;remaction=run&amp;namedcmd=
                    [% q.name FILTER url_quote %]&amp;sharer_id=
-                   [% q.userid FILTER url_quote %]">Run</a>
+                   [% q.user.id FILTER url_quote %]">Run</a>
         </td>
         <td align="center">
           <input type="checkbox" 

template/en/default/global/per-bug-queries.html.tmpl

@@ -54,7 +54,7 @@
   [%# Get existing lists of bugs for this user %]
   [% lists_of_bugs = [] %]
   [% FOREACH q = user.queries %]
-    [% NEXT UNLESS q.query_type == constants.LIST_OF_BUGS %]
+    [% NEXT UNLESS q.bug_ids_only %]
     [% lists_of_bugs.push(q.name) %]
   [% END %]
 

template/en/default/global/site-navigation.html.tmpl

@@ -97,6 +97,14 @@
           href="buglist.cgi?cmdtype=runnamed&namedcmd=[% q.name FILTER url_quote %]">
   [% END %]
 
+  [% FOREACH q = user.queries_subscribed %]
+    <link rel="Saved&nbsp;Search"
+          title="[% q.name FILTER html %] ([% q.user.login FILTER html %])"
+          href="buglist.cgi?cmdtype=dorem&amp;remaction=run&amp;namedcmd=
+                 [% q.name FILTER url_quote %]&amp;sharer_id=
+                 [% q.user.id FILTER url_quote %]">
+  [% END %]
+
   [%# *** Bugzilla Administration Tools *** %]
   [% IF user.login %] 
     [% '<link rel="Administration" title="Parameters"    

template/en/default/global/useful-links.html.tmpl

@@ -86,7 +86,9 @@
     
     [%# Saved searches %]
     
-  [% IF user.showmybugslink OR user.queries.size %]
+  [% IF user.showmybugslink OR user.queries.size 
+        OR user.queries_subscribed.size 
+  %]
     <li id="links-saved">
       <div class="label">
         Saved Searches:
@@ -99,7 +101,6 @@
         [% END %]
 
         [% FOREACH q = user.queries %]
-          [% NEXT IF q.userid != user.id %]
           [% IF q.link_in_footer %]
             <li>[% '<span class="separator">| </span>' IF print_pipe %]
             <a href="buglist.cgi?cmdtype=runnamed&amp;namedcmd=[% q.name FILTER url_quote %]">[% q.name FILTER html %]</a></li>
@@ -108,9 +109,7 @@
         [% END %]
         [% new_line = print_pipe %]
         [% print_pipe = 0 %]
-        [% FOREACH q = user.queries %]
-          [% NEXT IF q.userid == user.id %]
-          [% IF q.link_in_footer %]
+        [% FOREACH q = user.queries_subscribed %]
           [% IF new_line %]
             <br>
             [% new_line = 0 %]
@@ -119,12 +118,12 @@
             [% '<span class="separator">| </span>' IF print_pipe %]
             <a href="buglist.cgi?cmdtype=dorem&amp;remaction=run&amp;namedcmd=
                      [% q.name FILTER url_quote %]&amp;sharer_id=
-                     [% q.userid FILTER url_quote %]"
+                     [% q.user.id FILTER url_quote %]"
                class="shared"
-               title="Shared by [% q.user.identity FILTER html %]">[% q.name FILTER html FILTER no_break %]</a></li>
+               title="Shared by [% q.user.identity FILTER html %]"
+               >[% q.name FILTER html FILTER no_break %]</a></li>
           [% print_pipe = 1 %]
         [% END %]
-        [% END %]
       </ul>
     </li>
   [% END %]

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

@@ -1537,7 +1537,7 @@
                   
     [% FOREACH q = Bugzilla.user.queries %]
       [% IF q.name == namedcmd %]
-        or <a href="query.cgi?[% q.query FILTER html %]">edit</a>
+        or <a href="query.cgi?[% q.url FILTER html %]">edit</a>
       [% END %]
     [% END %]
     

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,76 +409,58 @@ 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);
+    foreach my $q (@{$user->queries}) {
+        my $group_id;
+
         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'}") || '';
+            $group_id = $cgi->param("share_" . $q->id) || '';
         }
+
         if ($group_id) {
-                if (grep(/^\Q$group_id\E$/, @{$user->queryshare_groups()})) {
+            # 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 see, so we can trick_taint.
-                    trick_taint($group_id);
-                    if ($group_map_entries == 0) {
-                        $sth_insert_ngm->execute($q->{'id'}, $group_id);
+            # 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 {
-                        $sth_update_ngm->execute($group_id, $q->{'id'});
+                $sth_insert_ngm->execute($q->id, $group_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 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)) {
-                        $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);
-                            }
+                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 {
-                    # In the unlikely case somebody removed visibility to the
-                    # group in the meantime, don't modify sharing.
-                }
-            }
-            else {
-                if ($group_map_entries > 0) {
-                    $sth_delete_ngm->execute($q->{'id'});
+            # 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
@@ -516,7 +469,6 @@ sub SaveSavedSearches {
             # user choose to share the query again.
         }
     }
-    }
 
     $user->flush_queries_cache;