Bug 343810: Remove Bugzilla::FlagType::get() and implement real flagtype objects - Patch by Frédéric Buclin <LpSolit@gmail.com> a=myk

3 files changed, 208 insertions, 174 deletions

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 25541d06b..e46a9b17c 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -104,9 +104,9 @@ Returns the status '+', '-', '?' of the flag.
 
 =cut
 
-sub id     { return $_[0]->{'id'};         }
-sub name   { return $_[0]->type->{'name'}; }
-sub status { return $_[0]->{'status'};     }
+sub id     { return $_[0]->{'id'};     }
+sub name   { return $_[0]->type->name; }
+sub status { return $_[0]->{'status'}; }
 
 ###############################
 ####       Methods         ####
@@ -118,7 +118,7 @@ sub status { return $_[0]->{'status'};     }
 
 =item C<type>
 
-Returns the type of the flag, as pseudo Bugzilla::FlagType object.
+Returns the type of the flag, as a Bugzilla::FlagType object.
 
 =item C<setter>
 
@@ -136,7 +136,7 @@ Bugzilla::User object.
 sub type {
     my $self = shift;
 
-    $self->{'type'} ||= Bugzilla::FlagType::get($self->{'type_id'});
+    $self->{'type'} ||= new Bugzilla::FlagType($self->{'type_id'});
     return $self->{'type'};
 }
 
@@ -291,7 +291,7 @@ sub validate {
         # as is.
         if ($status eq '?'
             && $flag->status ne '?'
-            && !$flag->type->{is_requestable})
+            && !$flag->type->is_requestable)
         {
             ThrowCodeError("flag_status_invalid", 
                            { id => $id, status => $status });
@@ -302,7 +302,7 @@ sub validate {
         # the flag became specifically unrequestable, don't let the user
         # change the requestee, but let the user remove it by entering
         # an empty string for the requestee.
-        if ($status eq '?' && !$flag->type->{is_requesteeble}) {
+        if ($status eq '?' && !$flag->type->is_requesteeble) {
             my $old_requestee = $flag->requestee ? $flag->requestee->login : '';
             my $new_requestee = join('', @requestees);
             if ($new_requestee && $new_requestee ne $old_requestee) {
@@ -314,7 +314,7 @@ sub validate {
         # Make sure the user didn't enter multiple requestees for a flag
         # that can't be requested from more than one person at a time.
         if ($status eq '?'
-            && !$flag->type->{is_multiplicable}
+            && !$flag->type->is_multiplicable
             && scalar(@requestees) > 1)
         {
             ThrowUserError("flag_not_multiplicable", { type => $flag->type });
@@ -323,7 +323,7 @@ sub validate {
         # Make sure the requestees are authorized to access the bug.
         # (and attachment, if this installation is using the "insider group"
         # feature and the attachment is marked private).
-        if ($status eq '?' && $flag->type->{is_requesteeble}) {
+        if ($status eq '?' && $flag->type->is_requesteeble) {
             my $old_requestee = $flag->requestee ? $flag->requestee->login : '';
             foreach my $login (@requestees) {
                 next if $login eq $old_requestee;
@@ -365,20 +365,19 @@ sub validate {
         # - The flag is unchanged
         next if ($status eq $flag->status);
 
-        # - User in the $request_gid group can clear pending requests and set flags
+        # - User in the request_group can clear pending requests and set flags
         #   and can rerequest set flags.
         next if (($status eq 'X' || $status eq '?')
-                 && (!$flag->type->{request_gid}
-                     || $user->in_group_id($flag->type->{request_gid})));
+                 && (!$flag->type->request_group
+                     || $user->in_group_id($flag->type->request_group->id)));
 
-        # - User in the $grant_gid group can set/clear flags,
-        #   including "+" and "-"
-        next if (!$flag->type->{grant_gid}
-                 || $user->in_group_id($flag->type->{grant_gid}));
+        # - User in the grant_group can set/clear flags, including "+" and "-".
+        next if (!$flag->type->grant_group
+                 || $user->in_group_id($flag->type->grant_group->id));
 
         # - Any other flag modification is denied
         ThrowUserError("flag_update_denied",
-                        { name       => $flag->type->{name},
+                        { name       => $flag->type->name,
                           status     => $status,
                           old_status => $flag->status });
     }
@@ -391,7 +390,7 @@ sub snapshot {
                         'attach_id' => $attach_id });
     my @summaries;
     foreach my $flag (@$flags) {
-        my $summary = $flag->type->{'name'} . $flag->status;
+        my $summary = $flag->type->name . $flag->status;
         $summary .= "(" . $flag->requestee->login . ")" if $flag->requestee;
         push(@summaries, $summary);
     }
@@ -524,7 +523,7 @@ sub create {
     $dbh->do('INSERT INTO flags (type_id, bug_id, attach_id, requestee_id,
                                  setter_id, status, creation_date, modification_date)
               VALUES (?, ?, ?, ?, ?, ?, ?, ?)',
-              undef, ($flag->{'type'}->{'id'}, $bug->bug_id,
+              undef, ($flag->{'type'}->id, $bug->bug_id,
                       $attach_id, $requestee_id, $flag->{'setter'}->id,
                       $flag->{'status'}, $timestamp, $timestamp));
 
@@ -571,6 +570,8 @@ sub modify {
     my @flags;
     foreach my $id (@ids) {
         my $flag = new Bugzilla::Flag($id);
+        # If the flag no longer exists, ignore it.
+        next unless $flag;
 
         my $status = $cgi->param("flag-$id");
 
@@ -582,7 +583,7 @@ sub modify {
         my $requestee_email;
         if ($status eq "?"
             && scalar(@requestees) > 1
-            && $flag->type->{is_multiplicable})
+            && $flag->type->is_multiplicable)
         {
             # The first person, for which we'll reuse the existing flag.
             $requestee_email = shift(@requestees);
@@ -616,7 +617,7 @@ sub modify {
 
         my $requestee_changed = 
           ($status eq "?" && 
-           $flag->type->{'is_requesteeble'} &&
+           $flag->type->is_requesteeble &&
            $old_requestee ne $requestee_email);
 
         next unless ($status_changed || $requestee_changed);
@@ -770,7 +771,7 @@ sub FormToNewFlags {
 
     my @flags;
     foreach my $flag_type (@$flag_types) {
-        my $type_id = $flag_type->{'id'};
+        my $type_id = $flag_type->id;
 
         # We are only interested in flags the user tries to create.
         next unless scalar(grep { $_ == $type_id } @type_ids);
@@ -784,7 +785,7 @@ sub FormToNewFlags {
 
         # Do not create a new flag of this type if this flag type is
         # not multiplicable and already has a flag set.
-        next if (!$flag_type->{'is_multiplicable'} && $has_flags);
+        next if (!$flag_type->is_multiplicable && $has_flags);
 
         my $status = $cgi->param("flag_type-$type_id");
         trick_taint($status);
@@ -796,7 +797,7 @@ sub FormToNewFlags {
                                 setter    => $setter , 
                                 status    => $status ,
                                 requestee => Bugzilla::User->new_from_login($login) });
-                last if !$flag_type->{'is_multiplicable'};
+                last unless $flag_type->is_multiplicable;
             }
         }
         else {
@@ -829,7 +830,7 @@ sub notify {
     my $template = Bugzilla->template;
 
     # There is nobody to notify.
-    return unless ($flag->{'addressee'} || $flag->type->{'cc_list'});
+    return unless ($flag->{'addressee'} || $flag->type->cc_list);
 
     my $attachment_is_private = $attachment ? $attachment->isprivate : undef;
 
@@ -839,7 +840,7 @@ sub notify {
     # not in those groups or email addresses that don't have an account.
     if ($bug->groups || $attachment_is_private) {
         my @new_cc_list;
-        foreach my $cc (split(/[, ]+/, $flag->type->{'cc_list'})) {
+        foreach my $cc (split(/[, ]+/, $flag->type->cc_list)) {
             my $ccuser = Bugzilla::User->new_from_login($cc) || next;
 
             next if ($bug->groups && !$ccuser->can_see_bug($bug->bug_id));
@@ -852,11 +853,11 @@ sub notify {
     }
 
     # If there is nobody left to notify, return.
-    return unless ($flag->{'addressee'} || $flag->type->{'cc_list'});
+    return unless ($flag->{'addressee'} || $flag->type->cc_list);
 
     # Process and send notification for each recipient
     foreach my $to ($flag->{'addressee'} ? $flag->{'addressee'}->email : '',
-                    split(/[, ]+/, $flag->type->{'cc_list'}))
+                    split(/[, ]+/, $flag->type->cc_list))
     {
         next unless $to;
         my $vars = { 'flag'       => $flag,
diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm
index 6b3b7d15c..048596a51 100644
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -20,6 +20,10 @@
 # Contributor(s): Myk Melez <myk@mozilla.org>
 #                 Frédéric Buclin <LpSolit@gmail.com>
 
+use strict;
+
+package Bugzilla::FlagType;
+
 =head1 NAME
 
 Bugzilla::FlagType - A module to deal with Bugzilla flag types.
@@ -44,25 +48,16 @@ whose names start with _ or are specifically noted as being private.
 
 =cut
 
-######################################################################
-# Module Initialization
-######################################################################
-
-# Make it harder for us to do dangerous things in Perl.
-use strict;
-
-# This module implements flag types for the flag tracker.
-package Bugzilla::FlagType;
-
-# Use Bugzilla's User module which contains utilities for handling users.
 use Bugzilla::User;
-
 use Bugzilla::Error;
 use Bugzilla::Util;
+use Bugzilla::Group;
 
-######################################################################
-# Global Variables
-######################################################################
+use base qw(Bugzilla::Object);
+
+###############################
+####    Initialization     ####
+###############################
 
 =begin private
 
@@ -70,36 +65,42 @@ use Bugzilla::Util;
 
 =over
 
-=item C<BASE_COLUMNS>
+=item C<DB_COLUMNS>
 
 basic sets of columns and tables for getting flag types from the
-database.  B<Used by get, match, sqlify_criteria and perlify_record>
+database.
 
 =back
 
 =cut
 
-use constant BASE_COLUMNS => (
-    "1", "flagtypes.id", "flagtypes.name", "flagtypes.description", 
-    "flagtypes.cc_list", "flagtypes.target_type", "flagtypes.sortkey", 
-    "flagtypes.is_active", "flagtypes.is_requestable", 
-    "flagtypes.is_requesteeble", "flagtypes.is_multiplicable", 
-    "flagtypes.grant_group_id", "flagtypes.request_group_id",
+use constant DB_COLUMNS => qw(
+    flagtypes.id
+    flagtypes.name
+    flagtypes.description
+    flagtypes.cc_list
+    flagtypes.target_type
+    flagtypes.sortkey
+    flagtypes.is_active
+    flagtypes.is_requestable
+    flagtypes.is_requesteeble
+    flagtypes.is_multiplicable
+    flagtypes.grant_group_id
+    flagtypes.request_group_id
 );
 
 =pod
 
 =over
 
-=item C<BASE_TABLES>
+=item C<DB_TABLE>
 
 Which database(s) is the data coming from?
 
-Note: when adding tables to BASE_TABLES, make sure to include the separator 
+Note: when adding tables to DB_TABLE, make sure to include the separator
 (i.e. words like "LEFT OUTER JOIN") before the table name, since tables take
 multiple separators based on the join type, and therefore it is not possible
 to join them later using a single known separator.
-B<Used by get, match, sqlify_criteria and perlify_record>
 
 =back
 
@@ -107,72 +108,162 @@ B<Used by get, match, sqlify_criteria and perlify_record>
 
 =cut
 
-use constant BASE_TABLES => ("flagtypes");
+use constant DB_TABLE => 'flagtypes';
+use constant LIST_ORDER => 'flagtypes.sortkey, flagtypes.name';
 
-######################################################################
-# Public Functions
-######################################################################
+###############################
+####      Accessors      ######
+###############################
 
-=head1 PUBLIC FUNCTIONS/METHODS
+=head2 METHODS
 
 =over
 
-=item C<get($id)>
+=item C<id>
 
-Returns a hash of information about a flag type.
+Returns the ID of the flagtype.
 
-=back
+=item C<name>
 
-=cut
+Returns the name of the flagtype.
 
-sub get {
-    my ($id) = @_;
-    my $dbh = Bugzilla->dbh;
+=item C<description>
 
-    my $columns = join(", ", BASE_COLUMNS);
+Returns the description of the flagtype.
 
-    my @data = $dbh->selectrow_array("SELECT $columns FROM flagtypes
-                                      WHERE id = ?", undef, $id);
+=item C<cc_list>
 
-    return perlify_record(@data);
-}
+Returns the concatenated CC list for the flagtype, as a single string.
 
-=pod
+=item C<target_type>
 
-=over
+Returns whether the flagtype applies to bugs or attachments.
+
+=item C<is_active>
+
+Returns whether the flagtype is active or disabled. Flags being
+in a disabled flagtype are not deleted. It only prevents you from
+adding new flags to it.
+
+=item C<is_requestable>
+
+Returns whether you can request for the given flagtype
+(i.e. whether the '?' flag is available or not).
+
+=item C<is_requesteeble>
 
-=item C<get_inclusions($id)>
+Returns whether you can ask someone specifically or not.
 
-Someone please document this
+=item C<is_multiplicable>
+
+Returns whether you can have more than one flag for the given
+flagtype in a given bug/attachment.
+
+=item C<sortkey>
+
+Returns the sortkey of the flagtype.
 
 =back
 
 =cut
 
-sub get_inclusions {
-    my ($id) = @_;
-    return get_clusions($id, "in");
-}
+sub id               { return $_[0]->{'id'};               }
+sub name             { return $_[0]->{'name'};             }
+sub description      { return $_[0]->{'description'};      }
+sub cc_list          { return $_[0]->{'cc_list'};          }
+sub target_type      { return $_[0]->{'target_type'} eq 'b' ? 'bug' : 'attachment'; }
+sub is_active        { return $_[0]->{'is_active'};        }
+sub is_requestable   { return $_[0]->{'is_requestable'};   }
+sub is_requesteeble  { return $_[0]->{'is_requesteeble'};  }
+sub is_multiplicable { return $_[0]->{'is_multiplicable'}; }
+sub sortkey          { return $_[0]->{'sortkey'};          }
+
+###############################
+####       Methods         ####
+###############################
 
 =pod
 
 =over
 
-=item C<get_exclusions($id)>
+=item C<grant_group>
+
+Returns the group (as a Bugzilla::Group object) in which a user
+must be in order to grant or deny a request.
+
+=item C<request_group>
 
-Someone please document this
+Returns the group (as a Bugzilla::Group object) in which a user
+must be in order to request or clear a flag.
+
+=item C<flag_count>
+
+Returns the number of flags belonging to the flagtype.
+
+=item C<inclusions>
+
+Return a hash of product/component IDs and names
+explicitly associated with the flagtype.
+
+=item C<exclusions>
+
+Return a hash of product/component IDs and names
+explicitly excluded from the flagtype.
 
 =back
 
 =cut
 
-sub get_exclusions {
-    my ($id) = @_;
-    return get_clusions($id, "ex");
+sub grant_group {
+    my $self = shift;
+
+    if (!defined $self->{'grant_group'} && $self->{'grant_group_id'}) {
+        $self->{'grant_group'} = new Bugzilla::Group($self->{'grant_group_id'});
+    }
+    return $self->{'grant_group'};
 }
 
+sub request_group {
+    my $self = shift;
+
+    if (!defined $self->{'request_group'} && $self->{'request_group_id'}) {
+        $self->{'request_group'} = new Bugzilla::Group($self->{'request_group_id'});
+    }
+    return $self->{'request_group'};
+}
+
+sub flag_count {
+    my $self = shift;
+
+    if (!defined $self->{'bug_count'}) {
+        $self->{'bug_count'} =
+            Bugzilla->dbh->selectrow_array('SELECT COUNT(*) FROM flags
+                                            WHERE type_id = ?', undef, $self->{'id'});
+    }
+}
+
+sub inclusions {
+    my $self = shift;
+
+    $self->{'inclusions'} ||= get_clusions($self->id, 'in');
+    return $self->{'inclusions'};
+}
+
+sub exclusions {
+    my $self = shift;
+
+    $self->{'exclusions'} ||= get_clusions($self->id, 'ex');
+    return $self->{'exclusions'};
+}
+
+######################################################################
+# Public Functions
+######################################################################
+
 =pod
 
+=head1 PUBLIC FUNCTIONS/METHODS
+
 =over
 
 =item C<get_clusions($id, $type)>
@@ -216,53 +307,28 @@ sub get_clusions {
 
 =over
 
-=item C<match($criteria, $include_count)>
+=item C<match($criteria)>
 
 Queries the database for flag types matching the given criteria
-and returns the set of matching types.
+and returns a list of matching flagtype objects.
 
 =back
 
 =cut
 
 sub match {
-    my ($criteria, $include_count) = @_;
-
-    my @tables       = BASE_TABLES;
-    my @base_columns = BASE_COLUMNS;
-    my @columns      = BASE_COLUMNS;
+    my ($criteria) = @_;
     my $dbh = Bugzilla->dbh;
 
-    # Include a count of the number of flags per type if requested.
-    if ($include_count) { 
-        push(@columns, "COUNT(flags.id)");
-        push(@tables, "LEFT OUTER JOIN flags ON flagtypes.id = flags.type_id");
-    }
-    
-    # Generate the SQL WHERE criteria.
-    my @criteria = sqlify_criteria($criteria, \@tables);
-    
-    # Build the query, grouping the types if we are counting flags.
-    # DISTINCT is used in order to count flag types only once when
-    # they appear several times in the flaginclusions table.
-    my $select_clause = "SELECT DISTINCT " . join(", ", @columns);
-    my $from_clause = "FROM " . join(" ", @tables);
-    my $where_clause = "WHERE " . join(" AND ", @criteria);
-    
-    my $query = "$select_clause $from_clause $where_clause";
-    $query .= " " . $dbh->sql_group_by('flagtypes.id',
-              join(', ', @base_columns[2..$#base_columns]))
-                    if $include_count;
-    $query .= " ORDER BY flagtypes.sortkey, flagtypes.name";
-
-    my $flagtypes = $dbh->selectall_arrayref($query);
+    # Depending on the criteria, we may have to append additional tables.
+    my $tables = [DB_TABLE];
+    my @criteria = sqlify_criteria($criteria, $tables);
+    $tables = join(' ', @$tables);
+    $criteria = join(' AND ', @criteria);
 
-    my @types;
-    foreach my $flagtype (@$flagtypes) {
-        push(@types, perlify_record(@$flagtype));
-    }
+    my $flagtype_ids = $dbh->selectcol_arrayref("SELECT id FROM $tables WHERE $criteria");
 
-    return \@types;
+    return Bugzilla::FlagType->new_from_list($flagtype_ids);
 }
 
 =pod
@@ -281,15 +347,14 @@ sub count {
     my ($criteria) = @_;
     my $dbh = Bugzilla->dbh;
 
-    my @tables = BASE_TABLES;
-    my @criteria = sqlify_criteria($criteria, \@tables);
-    # The way tables are joined is already included in @tables.
-    my $tables = join(' ', @tables);
+    # Depending on the criteria, we may have to append additional tables.
+    my $tables = [DB_TABLE];
+    my @criteria = sqlify_criteria($criteria, $tables);
+    $tables = join(' ', @$tables);
     $criteria = join(' AND ', @criteria);
 
-    my $count = $dbh->selectrow_array("SELECT COUNT(flagtypes.id) FROM $tables
-                                       WHERE $criteria");
-
+    my $count = $dbh->selectrow_array("SELECT COUNT(flagtypes.id)
+                                       FROM $tables WHERE $criteria");
     return $count;
 }
 
@@ -345,7 +410,7 @@ sub validate {
         next if $status eq "X";
 
         # Make sure the flag type exists.
-        my $flag_type = get($id);
+        my $flag_type = new Bugzilla::FlagType($id);
         $flag_type 
           || ThrowCodeError("flag_type_nonexistent", { id => $id });
 
@@ -355,15 +420,15 @@ sub validate {
                             { id => $id , status => $status });
 
         # Make sure the user didn't request the flag unless it's requestable.
-        if ($status eq '?' && !$flag_type->{is_requestable}) {
+        if ($status eq '?' && !$flag_type->is_requestable) {
             ThrowCodeError("flag_status_invalid", 
                            { id => $id , status => $status });
         }
-        
+
         # Make sure the user didn't specify a requestee unless the flag
         # is specifically requestable.
         if ($status eq '?'
-            && !$flag_type->{is_requesteeble}
+            && !$flag_type->is_requesteeble
             && scalar(@requestees) > 0)
         {
             ThrowCodeError("flag_requestee_disabled", { type => $flag_type });
@@ -372,7 +437,7 @@ sub validate {
         # Make sure the user didn't enter multiple requestees for a flag
         # that can't be requested from more than one person at a time.
         if ($status eq '?'
-            && !$flag_type->{is_multiplicable}
+            && !$flag_type->is_multiplicable
             && scalar(@requestees) > 1)
         {
             ThrowUserError("flag_not_multiplicable", { type => $flag_type });
@@ -381,7 +446,7 @@ sub validate {
         # Make sure the requestees are authorized to access the bug
         # (and attachment, if this installation is using the "insider group"
         # feature and the attachment is marked private).
-        if ($status eq '?' && $flag_type->{is_requesteeble}) {
+        if ($status eq '?' && $flag_type->is_requesteeble) {
             foreach my $login (@requestees) {
                 # We know the requestee exists because we ran
                 # Bugzilla::User::match_field before getting here.
@@ -413,18 +478,18 @@ sub validate {
         }
 
         # Make sure the user is authorized to modify flags, see bug 180879
-        # - User in the $grant_gid group can set flags, including "+" and "-"
-        next if (!$flag_type->{grant_gid}
-                 || $user->in_group_id($flag_type->{grant_gid}));
+        # - User in the grant_group can set flags, including "+" and "-".
+        next if (!$flag_type->grant_group
+                 || $user->in_group_id($flag_type->grant_group->id));
 
-        # - User in the $request_gid group can request flags
+        # - User in the request_group can request flags.
         next if ($status eq '?'
-                 && (!$flag_type->{request_gid}
-                     || $user->in_group_id($flag_type->{request_gid})));
+                 && (!$flag_type->request_group
+                     || $user->in_group_id($flag_type->request_group->id)));
 
         # - Any other flag modification is denied
         ThrowUserError("flag_update_denied",
-                        { name       => $flag_type->{name},
+                        { name       => $flag_type->name,
                           status     => $status,
                           old_status => "X" });
     }
@@ -508,40 +573,6 @@ sub sqlify_criteria {
     return @criteria;
 }
 
-=pod
-
-=over
-
-=item C<perlify_record()>
-
-Converts data retrieved from the database into a Perl record.  Depends on the
-formatting as described in C<BASE_COLUMNS>.
-
-=back
-
-=cut
-
-sub perlify_record {
-    my $type = {};
-    
-    $type->{'exists'} = $_[0];
-    $type->{'id'} = $_[1];
-    $type->{'name'} = $_[2];
-    $type->{'description'} = $_[3];
-    $type->{'cc_list'} = $_[4];
-    $type->{'target_type'} = $_[5] eq "b" ? "bug" : "attachment";
-    $type->{'sortkey'} = $_[6];
-    $type->{'is_active'} = $_[7];
-    $type->{'is_requestable'} = $_[8];
-    $type->{'is_requesteeble'} = $_[9];
-    $type->{'is_multiplicable'} = $_[10];
-    $type->{'grant_gid'} = $_[11];
-    $type->{'request_gid'} = $_[12];
-    $type->{'flag_count'} = $_[13];
-        
-    return $type;
-}
-
 1;
 
 =end private
@@ -562,6 +593,8 @@ sub perlify_record {
 
 =item Kevin Benton <kevin.benton@amd.com>
 
+=item Frédéric Buclin <LpSolit@gmail.com>
+
 =back
 
 =cut
diff --git a/Bugzilla/User.pm b/Bugzilla/User.pm
index 1b3f161ee..48502326e 100644
--- a/Bugzilla/User.pm
+++ b/Bugzilla/User.pm
@@ -980,7 +980,7 @@ sub match_field {
                 elsif ($field_name =~ /^requestee_type-(\d+)$/) {
                     require Bugzilla::FlagType;
                     $expanded_fields->{$field_name}->{'flag_type'} = 
-                      Bugzilla::FlagType::get($1);
+                      new Bugzilla::FlagType($1);
                 }
             }
         }