Bug 442016: Bugzilla::User::bless_groups should be returning Bugzilla::Group objects

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=LpSolit, a=LpSolit

3 files changed, 34 insertions, 45 deletions

diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index bc64e43eb..760723279 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -425,49 +425,39 @@ sub bless_groups {
     return $self->{'bless_groups'} if defined $self->{'bless_groups'};
     return [] unless $self->id;
 
-    my $dbh = Bugzilla->dbh;
-    my $query;
-    my $connector;
-    my @bindValues;
-
     if ($self->in_group('editusers')) {
         # Users having editusers permissions may bless all groups.
-        $query = 'SELECT DISTINCT id, name, description FROM groups';
-        $connector = 'WHERE';
-    }
-    else {
-        # Get all groups for the user where:
-        #    + They have direct bless privileges
-        #    + They are a member of a group that inherits bless privs.
-        $query = q{
-            SELECT DISTINCT groups.id, groups.name, groups.description
-                       FROM groups, user_group_map, group_group_map AS ggm
-                      WHERE user_group_map.user_id = ?
-                        AND ((user_group_map.isbless = 1
-                              AND groups.id=user_group_map.group_id)
-                             OR (groups.id = ggm.grantor_id
-                                 AND ggm.grant_type = ?
-                                 AND ggm.member_id IN(} .
-                                 $self->groups_as_string . 
-                               q{)))};
-        $connector = 'AND';
-        @bindValues = ($self->id, GROUP_BLESS);
+        $self->{'bless_groups'} = [Bugzilla::Group->get_all];
+        return $self->{'bless_groups'};
     }
 
+    my $dbh = Bugzilla->dbh;
+
+    # Get all groups for the user where:
+    #    + They have direct bless privileges
+    #    + They are a member of a group that inherits bless privs.
+    my @group_ids = (map {$_->id} @{ $self->groups }) || (-1);
+    my $query =
+        'SELECT DISTINCT groups.id
+           FROM groups, user_group_map, group_group_map AS ggm
+          WHERE user_group_map.user_id = ?
+                AND ( (user_group_map.isbless = 1
+                       AND groups.id=user_group_map.group_id)
+                     OR (groups.id = ggm.grantor_id
+                         AND ggm.grant_type = ' . GROUP_BLESS . '
+                         AND ggm.member_id ' . $dbh->sql_in(\@group_ids)
+                     . ') )';
+
     # If visibilitygroups are used, restrict the set of groups.
-    if (!$self->in_group('editusers')
-        && Bugzilla->params->{'usevisibilitygroups'}) 
-    {
+    if (Bugzilla->params->{'usevisibilitygroups'}) {
+        return [] if !$self->visible_groups_as_string;
         # Users need to see a group in order to bless it.
-        my $visibleGroups = join(', ', @{$self->visible_groups_direct()})
-            || return $self->{'bless_groups'} = [];
-        $query .= " $connector id in ($visibleGroups)";
+        $query .= " AND groups.id "
+                  . $dbh->sql_in($self->visible_groups_inherited);
     }
 
-    $query .= ' ORDER BY name';
-
-    return $self->{'bless_groups'} =
-        $dbh->selectall_arrayref($query, {'Slice' => {}}, @bindValues);
+    my $ids = $dbh->selectcol_arrayref($query, undef, $self->id);
+    return $self->{'bless_groups'} = Bugzilla::Group->new_from_list($ids);
 }
 
 sub in_group {
@@ -979,12 +969,12 @@ sub can_bless {
     if (!scalar(@_)) {
         # If we're called without an argument, just return 
         # whether or not we can bless at all.
-        return scalar(@{$self->bless_groups}) ? 1 : 0;
+        return scalar(@{ $self->bless_groups }) ? 1 : 0;
     }
 
     # Otherwise, we're checking a specific group
     my $group_id = shift;
-    return (grep {$$_{'id'} eq $group_id} (@{$self->bless_groups})) ? 1 : 0;
+    return grep($_->id == $group_id, @{ $self->bless_groups }) ? 1 : 0;
 }
 
 sub flatten_group_membership {
@@ -1917,12 +1907,11 @@ Determines whether or not a user is in the given group by id.
 
 =item C<bless_groups>
 
-Returns an arrayref of hashes of C<groups> entries, where the keys of each hash
-are the names of C<id>, C<name> and C<description> columns of the C<groups>
-table.
+Returns an arrayref of L<Bugzilla::Group> objects.
+
 The arrayref consists of the groups the user can bless, taking into account
 that having editusers permissions means that you can bless all groups, and
-that you need to be aware of a group in order to bless a group.
+that you need to be able to see a group in order to bless it.
 
 =item C<get_products_by_permission($group)>
 
diff --git a/editusers.cgi b/editusers.cgi
index 4e75d6df4..f133b7409 100755
--- a/editusers.cgi
+++ b/editusers.cgi
@@ -276,9 +276,9 @@ if ($action eq 'search') {
     #      would allow to display a friendlier error message on page reloads.
     userDataToVars($otherUserID);
     my $permissions = $vars->{'permissions'};
-    foreach (@{$user->bless_groups()}) {
-        my $id = $$_{'id'};
-        my $name = $$_{'name'};
+    foreach my $blessable (@{$user->bless_groups()}) {
+        my $id = $blessable->id;
+        my $name = $blessable->name;
 
         # Change memberships.
         my $groupid = $cgi->param("group_$id") || 0;
diff --git a/userprefs.cgi b/userprefs.cgi
index b281fd214..3ccfe820a 100755
--- a/userprefs.cgi
+++ b/userprefs.cgi
@@ -400,7 +400,7 @@ sub DoSavedSearches {
         $vars->{'queryshare_groups'} =
             Bugzilla::Group->new_from_list($user->queryshare_groups);
     }
-    $vars->{'bless_group_ids'} = [map {$_->{'id'}} @{$user->bless_groups}];
+    $vars->{'bless_group_ids'} = [map { $_->id } @{$user->bless_groups}];
 }
 
 sub SaveSavedSearches {