Bug 442031: Make Bugzilla::User::groups return an arrayref of Bugzilla::Group objects (instead of a hashref of group ids and names).

4 files changed, 60 insertions, 69 deletions

diff --git a/Bugzilla/BugMail.pm b/Bugzilla/BugMail.pm
index dc48e83de..20bb7e254 100644
--- a/Bugzilla/BugMail.pm
+++ b/Bugzilla/BugMail.pm
@@ -489,9 +489,7 @@ sub Send {
             # If we are using insiders, and the comment is private, only send 
             # to insiders
             my $insider_ok = 1;
-            $insider_ok = 0 if (Bugzilla->params->{"insidergroup"} && 
-                                ($anyprivate != 0) && 
-                                (!$user->groups->{Bugzilla->params->{"insidergroup"}}));
+            $insider_ok = 0 if $anyprivate && !$user->is_insider;
 
             # We shouldn't send mail if this is a dependency mail (i.e. there 
             # is something in @depbugs), and any of the depending bugs are not 
@@ -562,9 +560,9 @@ sub sendMail {
           next;
         }
         # Only send estimated_time if it is enabled and the user is in the group
-        if (($f ne 'estimated_time' && $f ne 'deadline') ||
-             $user->groups->{Bugzilla->params->{'timetrackinggroup'}}) {
-
+        if (($f ne 'estimated_time' && $f ne 'deadline') 
+            || $user->is_timetracker)
+        {
             my $desc = $fielddescription{$f};
             $head .= multiline_sprintf(FORMAT_DOUBLE, ["$desc:", $value], 
                                        FORMAT_2_SIZE);
@@ -584,14 +582,12 @@ sub sendMail {
             ($diff->{'fieldname'} eq 'estimated_time' ||
              $diff->{'fieldname'} eq 'remaining_time' ||
              $diff->{'fieldname'} eq 'work_time' ||
-             $diff->{'fieldname'} eq 'deadline')){
-            if ($user->groups->{Bugzilla->params->{"timetrackinggroup"}}) {
-                $add_diff = 1;
-            }
-        } elsif (($diff->{'isprivate'}) 
-                 && Bugzilla->params->{'insidergroup'}
-                 && !($user->groups->{Bugzilla->params->{'insidergroup'}})
-                ) {
+             $diff->{'fieldname'} eq 'deadline'))
+        {
+            $add_diff = 1 if $user->is_timetracker;
+        } elsif ($diff->{'isprivate'} 
+                 && !$user->is_insider)
+        {
             $add_diff = 0;
         } else {
             $add_diff = 1;
diff --git a/Bugzilla/Chart.pm b/Bugzilla/Chart.pm
index a119e4b7c..1f232f310 100644
--- a/Bugzilla/Chart.pm
+++ b/Bugzilla/Chart.pm
@@ -382,8 +382,7 @@ sub getSeriesIDs {
 sub getVisibleSeries {
     my %cats;
 
-    # List of groups the user is in; use -1 to make sure it's not empty.
-    my $grouplist = join(", ", (-1, values(%{Bugzilla->user->groups})));
+    my $grouplist = Bugzilla->user->groups_as_string;
     
     # Get all visible series
     my $dbh = Bugzilla->dbh;
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index 2f8d86c8e..77000ce31 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -775,8 +775,9 @@ sub init {
                 " ON bug_group_map.bug_id = bugs.bug_id ";
 
     if ($user->id) {
-        if (%{$user->groups}) {
-            $query .= " AND bug_group_map.group_id NOT IN (" . join(',', values(%{$user->groups})) . ") ";
+        if (scalar @{ $user->groups }) {
+            $query .= " AND bug_group_map.group_id NOT IN (" 
+                   . $user->groups_as_string . ") ";
         }
 
         $query .= " LEFT JOIN cc ON cc.bug_id = bugs.bug_id AND cc.who = " . $user->id;
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 26febfcd9..9bd5b9e9f 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -359,75 +359,62 @@ sub groups {
     my $self = shift;
 
     return $self->{groups} if defined $self->{groups};
-    return {} unless $self->id;
+    return [] unless $self->id;
 
     my $dbh = Bugzilla->dbh;
-    my $groups = $dbh->selectcol_arrayref(q{SELECT DISTINCT groups.name, group_id
-                                              FROM groups, user_group_map
-                                             WHERE groups.id=user_group_map.group_id
-                                               AND user_id=?
-                                               AND isbless=0},
-                                          { Columns=>[1,2] },
-                                          $self->id);
-
-    # The above gives us an arrayref [name, id, name, id, ...]
-    # Convert that into a hashref
-    my %groups = @$groups;
-    my @groupidstocheck = values(%groups);
-    my %groupidschecked = ();
+    my $groups_to_check = $dbh->selectcol_arrayref(
+        q{SELECT DISTINCT group_id
+            FROM user_group_map
+           WHERE user_id = ? AND isbless = 0}, undef, $self->id);
+
     my $rows = $dbh->selectall_arrayref(
-                "SELECT DISTINCT groups.name, groups.id, member_id
-                            FROM group_group_map
-                      INNER JOIN groups
-                              ON groups.id = grantor_id
-                           WHERE grant_type = " . GROUP_MEMBERSHIP);
-    my %group_names = ();
-    my %group_membership = ();
+        "SELECT DISTINCT grantor_id, member_id
+           FROM group_group_map
+          WHERE grant_type = " . GROUP_MEMBERSHIP);
+
+    my %group_membership;
     foreach my $row (@$rows) {
-        my ($member_name, $grantor_id, $member_id) = @$row; 
-        # Just save the group names
-        $group_names{$grantor_id} = $member_name;
-        
-        # And group membership
-        push (@{$group_membership{$member_id}}, $grantor_id);
+        my ($grantor_id, $member_id) = @$row; 
+        push (@{ $group_membership{$member_id} }, $grantor_id);
     }
     
     # Let's walk the groups hierarchy tree (using FIFO)
     # On the first iteration it's pre-filled with direct groups 
     # membership. Later on, each group can add its own members into the
     # FIFO. Circular dependencies are eliminated by checking
-    # $groupidschecked{$member_id} hash values.
+    # $checked_groups{$member_id} hash values.
     # As a result, %groups will have all the groups we are the member of.
-    while ($#groupidstocheck >= 0) {
+    my %checked_groups;
+    my %groups;
+    while (scalar(@$groups_to_check) > 0) {
         # Pop the head group from FIFO
-        my $member_id = shift @groupidstocheck;
+        my $member_id = shift @$groups_to_check;
         
         # Skip the group if we have already checked it
-        if (!$groupidschecked{$member_id}) {
+        if (!$checked_groups{$member_id}) {
             # Mark group as checked
-            $groupidschecked{$member_id} = 1;
+            $checked_groups{$member_id} = 1;
             
             # Add all its members to the FIFO check list
             # %group_membership contains arrays of group members 
             # for all groups. Accessible by group number.
-            foreach my $newgroupid (@{$group_membership{$member_id}}) {
-                push @groupidstocheck, $newgroupid 
-                    if (!$groupidschecked{$newgroupid});
-            }
-            # Note on if clause: we could have group in %groups from 1st
-            # query and do not have it in second one
-            $groups{$group_names{$member_id}} = $member_id 
-                if $group_names{$member_id} && $member_id;
+            my $members = $group_membership{$member_id};
+            my @new_to_check = grep(!$checked_groups{$_}, @$members);
+            push(@$groups_to_check, @new_to_check);
+
+            $groups{$member_id} = 1;
         }
     }
-    $self->{groups} = \%groups;
+
+    $self->{groups} = Bugzilla::Group->new_from_list([keys %groups]);
 
     return $self->{groups};
 }
 
 sub groups_as_string {
     my $self = shift;
-    return (join(',',values(%{$self->groups})) || '-1');
+    my @ids = map { $_->id } @{ $self->groups };
+    return scalar(@ids) ? join(',', @ids) : '-1';
 }
 
 sub bless_groups {
@@ -483,7 +470,7 @@ sub bless_groups {
 
 sub in_group {
     my ($self, $group, $product_id) = @_;
-    if (exists $self->groups->{$group}) {
+    if (scalar grep($_->name eq $group, @{ $self->groups })) {
         return 1;
     }
     elsif ($product_id && detaint_natural($product_id)) {
@@ -512,8 +499,7 @@ sub in_group {
 
 sub in_group_id {
     my ($self, $id) = @_;
-    my %j = reverse(%{$self->groups});
-    return exists $j{$id} ? 1 : 0;
+    return grep($_->id == $id, @{ $self->groups }) ? 1 : 0;
 }
 
 sub get_products_by_permission {
@@ -828,7 +814,7 @@ sub visible_groups_direct {
     my $sth;
    
     if (Bugzilla->params->{'usevisibilitygroups'}) {
-        my $glist = join(',',(-1,values(%{$self->groups})));
+        my $glist = $self->groups_as_string;
         $sth = $dbh->prepare("SELECT DISTINCT grantor_id
                                  FROM group_group_map
                                 WHERE member_id IN($glist)
@@ -873,7 +859,7 @@ sub queryshare_groups {
             }
         }
         else {
-            @queryshare_groups = values(%{$self->groups});
+            @queryshare_groups = map { $_->id } @{ $self->groups };
         }
     }
 
@@ -1531,6 +1517,17 @@ sub is_global_watcher {
     return  $self->{'is_global_watcher'};
 }
 
+sub is_timetracker {
+    my $self = shift;
+
+    if (!defined $self->{'is_timetracker'}) {
+        my $tt_group = Bugzilla->params->{'timetrackinggroup'};
+        $self->{'is_timetracker'} =
+            ($tt_group && $self->in_group($tt_group)) ? 1 : 0;
+    }
+    return $self->{'is_timetracker'};
+}
+
 sub get_userlist {
     my $self = shift;
 
@@ -1871,10 +1868,8 @@ is_default     - a boolean to indicate whether the user has chosen to make
 
 =item C<groups>
 
-Returns a hashref of group names for groups the user is a member of. The keys
-are the names of the groups, whilst the values are the respective group ids.
-(This is so that a set of all groupids for groups the user is in can be
-obtained by C<values(%{$user-E<gt>groups})>.)
+Returns an arrayref of L<Bugzilla::Group> objects representing
+groups that this user is a member of.
 
 =item C<groups_as_string>