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

12 files changed, 326 insertions, 320 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);
                 }
             }
         }
diff --git a/attachment.cgi b/attachment.cgi
index 7f66c9984..3939564a8 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -713,8 +713,7 @@ sub enter
                                               'product_id'   => $product_id,
                                               'component_id' => $component_id});
   $vars->{'flag_types'} = $flag_types;
-  $vars->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'},
-                                           @$flag_types);
+  $vars->{'any_flags_requesteeble'} = grep($_->is_requesteeble, @$flag_types);
 
   print $cgi->header();
 
@@ -835,11 +834,11 @@ sub edit {
                                                'product_id'   => $product_id ,
                                                'component_id' => $component_id });
   foreach my $flag_type (@$flag_types) {
-    $flag_type->{'flags'} = Bugzilla::Flag::match({ 'type_id'   => $flag_type->{'id'},
+    $flag_type->{'flags'} = Bugzilla::Flag::match({ 'type_id'   => $flag_type->id,
                                                     'attach_id' => $attachment->id });
   }
   $vars->{'flag_types'} = $flag_types;
-  $vars->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'}, @$flag_types);
+  $vars->{'any_flags_requesteeble'} = grep($_->is_requesteeble, @$flag_types);
   $vars->{'attachment'} = $attachment;
   $vars->{'bugsummary'} = $bugsummary; 
   $vars->{'isviewable'} = $isviewable; 
diff --git a/editflagtypes.cgi b/editflagtypes.cgi
index 1ca6bd94d..3c71d5bdb 100755
--- a/editflagtypes.cgi
+++ b/editflagtypes.cgi
@@ -42,8 +42,6 @@ use Bugzilla::Component;
 use Bugzilla::Bug;
 use Bugzilla::Attachment;
 
-use List::Util qw(reduce);
-
 local our $cgi = Bugzilla->cgi;
 local our $template = Bugzilla->template;
 local our $vars = {};
@@ -105,57 +103,44 @@ sub list {
     $vars->{'selected_product'} = $cgi->param('product');
     $vars->{'selected_component'} = $cgi->param('component');
 
-    # If only a product was specified but no component, then we restrict
-    # the list to flag types available in ALL components of that product.
-    my @comp_ids = ($component_id);
-    if ($product_id && !$component_id) {
-        @comp_ids = map {$_->id} @{$product->components};
-    }
+    my $bug_flagtypes;
+    my $attach_flagtypes;
 
-    my @bug_flagtypes;
-    my @attach_flagtypes;
-
-    foreach my $comp_id (@comp_ids) {
-        my $bug_types =
-          Bugzilla::FlagType::match({ 'target_type' => 'bug',
-                                      'group' => scalar $cgi->param('group'),
-                                      'product_id' => $product_id,
-                                      'component_id' => $comp_id }, 1);
-        push(@bug_flagtypes, $bug_types);
-
-        my $attach_types =
-          Bugzilla::FlagType::match({ 'target_type' => 'attachment',
-                                      'group' => scalar $cgi->param('group'),
-                                      'product_id' => $product_id,
-                                      'component_id' => $comp_id }, 1);
-        push(@attach_flagtypes, $attach_types);
-    }
+    # If a component is given, restrict the list to flag types available
+    # for this component.
+    if ($component) {
+        $bug_flagtypes = $component->flag_types->{'bug'};
+        $attach_flagtypes = $component->flag_types->{'attachment'};
+
+        # Filter flag types if a group ID is given.
+        $bug_flagtypes = filter_group($bug_flagtypes);
+        $attach_flagtypes = filter_group($attach_flagtypes);
 
-    sub intersection {
-        my ($aa, $bb) = @_;
-        my %union;
-        my %isect;
-        foreach my $e (@$aa, @$bb) { $union{$e->{'id'}}++ && ($isect{$e->{'id'}} ||= $e) };
-        return [sort { $a->{'sortkey'} <=> $b->{'sortkey'}
-                       || $a->{'name'} cmp $b->{'name'} } values %isect];
     }
-    $vars->{'bug_types'} = reduce { intersection($a, $b) } @bug_flagtypes;
-    $vars->{'attachment_types'} = reduce { intersection($a, $b) } @attach_flagtypes;
-
-    # Users want to see group names, not IDs
-    # So get the group names
-    my %group_name_cache = ();
-    foreach my $flag_type_set ("bug_types", "attachment_types") {
-        foreach my $flag_type (@{$vars->{$flag_type_set}}) {
-            foreach my $group ("grant", "request") {
-                my $gid = $flag_type->{$group . "_gid"};
-                next if (!$gid);
-                $group_name_cache{$gid} ||= new Bugzilla::Group($gid)->name();
-                $flag_type->{$group . "_group_name"} = $group_name_cache{$gid};
-            }
-        }
+    # If only a product is specified but no component, then restrict the list
+    # to flag types available in at least one component of that product.
+    elsif ($product) {
+        $bug_flagtypes = $product->flag_types->{'bug'};
+        $attach_flagtypes = $product->flag_types->{'attachment'};
+
+        # Filter flag types if a group ID is given.
+        $bug_flagtypes = filter_group($bug_flagtypes);
+        $attach_flagtypes = filter_group($attach_flagtypes);
+    }
+    # If no product is given, then show all flag types available.
+    else {
+        $bug_flagtypes =
+            Bugzilla::FlagType::match({'target_type' => 'bug',
+                                       'group' => scalar $cgi->param('group')});
+
+        $attach_flagtypes =
+            Bugzilla::FlagType::match({'target_type' => 'attachment',
+                                         'group' => scalar $cgi->param('group')});
     }
 
+    $vars->{'bug_types'} = $bug_flagtypes;
+    $vars->{'attachment_types'} = $attach_flagtypes;
+
     # Return the appropriate HTTP response headers.
     print $cgi->header();
 
@@ -167,8 +152,14 @@ sub list {
 
 sub edit {
     my ($action) = @_;
-    $action eq 'enter' ? validateTargetType() : (my $id = validateID());
-    my $dbh = Bugzilla->dbh;
+
+    my $flag_type;
+    if ($action eq 'enter') {
+        validateTargetType();
+    }
+    else {
+        $flag_type = validateID();
+    }
 
     # Fill $vars with products and components data.
     $vars = get_products_and_components($vars);
@@ -180,20 +171,10 @@ sub edit {
     else { 
         $vars->{'action'} = "update";
     }
-    
+
     # If copying or editing an existing flag type, retrieve it.
     if ($cgi->param('action') eq 'copy' || $cgi->param('action') eq 'edit') { 
-        $vars->{'type'} = Bugzilla::FlagType::get($id);
-        $vars->{'type'}->{'inclusions'} = Bugzilla::FlagType::get_inclusions($id);
-        $vars->{'type'}->{'exclusions'} = Bugzilla::FlagType::get_exclusions($id);
-        # Users want to see group names, not IDs
-        foreach my $group ("grant_gid", "request_gid") {
-            my $gid = $vars->{'type'}->{$group};
-            next if (!$gid);
-            ($vars->{'type'}->{$group}) =
-                $dbh->selectrow_array('SELECT name FROM groups WHERE id = ?',
-                                       undef, $gid);
-        }
+        $vars->{'type'} = $flag_type;
     }
     # Otherwise set the target type (the minimal information about the type
     # that the template needs to know) from the URL parameter and default
@@ -261,6 +242,13 @@ sub processCategoryChange {
 
     my $type = {};
     foreach my $key ($cgi->param()) { $type->{$key} = $cgi->param($key) }
+    # That's what I call a big hack. The template expects to see a group object.
+    # This script needs some rewrite anyway.
+    $type->{'grant_group'} = {};
+    $type->{'grant_group'}->{'name'} = $cgi->param('grant_group');
+    $type->{'request_group'} = {};
+    $type->{'request_group'}->{'name'} = $cgi->param('request_group');
+
     $type->{'inclusions'} = \%inclusions;
     $type->{'exclusions'} = \%exclusions;
     $vars->{'type'} = $type;
@@ -352,7 +340,8 @@ sub insert {
 
 
 sub update {
-    my $id = validateID();
+    my $flag_type = validateID();
+    my $id = $flag_type->id;
     my $name = validateName();
     my $description = validateDescription();
     my $cc_list = validateCCList();
@@ -440,17 +429,11 @@ sub update {
 }
 
 
-sub confirmDelete 
-{
-  my $id = validateID();
+sub confirmDelete {
+  my $flag_type = validateID();
 
-  # check if we need confirmation to delete:
-  
-  my $count = Bugzilla::Flag::count({ 'type_id' => $id });
-  
-  if ($count > 0) {
-    $vars->{'flag_type'} = Bugzilla::FlagType::get($id);
-    $vars->{'flag_count'} = scalar($count);
+  if ($flag_type->flag_count) {
+    $vars->{'flag_type'} = $flag_type;
 
     # Return the appropriate HTTP response headers.
     print $cgi->header();
@@ -460,22 +443,22 @@ sub confirmDelete
       || ThrowTemplateError($template->error());
   } 
   else {
-    deleteType();
+    deleteType($flag_type);
   }
 }
 
 
 sub deleteType {
-    my $id = validateID();
+    my $flag_type = shift || validateID();
+    my $id = $flag_type->id;
     my $dbh = Bugzilla->dbh;
 
     $dbh->bz_lock_tables('flagtypes WRITE', 'flags WRITE',
                          'flaginclusions WRITE', 'flagexclusions WRITE');
-    
+
     # Get the name of the flag type so we can tell users
     # what was deleted.
-    ($vars->{'name'}) = $dbh->selectrow_array('SELECT name FROM flagtypes
-                                               WHERE id = ?', undef, $id);
+    $vars->{'name'} = $flag_type->name;
 
     $dbh->do('DELETE FROM flags WHERE type_id = ?', undef, $id);
     $dbh->do('DELETE FROM flaginclusions WHERE type_id = ?', undef, $id);
@@ -495,18 +478,18 @@ sub deleteType {
 
 
 sub deactivate {
-    my $id = validateID();
+    my $flag_type = validateID();
     validateIsActive();
 
     my $dbh = Bugzilla->dbh;
 
     $dbh->bz_lock_tables('flagtypes WRITE');
-    $dbh->do('UPDATE flagtypes SET is_active = 0 WHERE id = ?', undef, $id);
+    $dbh->do('UPDATE flagtypes SET is_active = 0 WHERE id = ?', undef, $flag_type->id);
     $dbh->bz_unlock_tables();
-    
+
     $vars->{'message'} = "flag_type_deactivated";
-    $vars->{'flag_type'} = Bugzilla::FlagType::get($id);
-    
+    $vars->{'flag_type'} = $flag_type;
+
     # Return the appropriate HTTP response headers.
     print $cgi->header();
 
@@ -536,20 +519,11 @@ sub get_products_and_components {
 ################################################################################
 
 sub validateID {
-    my $dbh = Bugzilla->dbh;
-    # $flagtype_id is destroyed if detaint_natural fails.
-    my $flagtype_id = $cgi->param('id');
-    detaint_natural($flagtype_id)
-      || ThrowCodeError("flag_type_id_invalid",
-                        { id => scalar $cgi->param('id') });
-
-    my $flagtype_exists =
-        $dbh->selectrow_array('SELECT 1 FROM flagtypes WHERE id = ?',
-                               undef, $flagtype_id);
-    $flagtype_exists
-      || ThrowCodeError("flag_type_nonexistent", { id => $flagtype_id });
-
-    return $flagtype_id;
+    my $id = $cgi->param('id');
+    my $flag_type = new Bugzilla::FlagType($id)
+        || ThrowCodeError('flag_type_nonexistent', { id => $id });
+
+    return $flag_type;
 }
 
 sub validateName {
@@ -646,18 +620,15 @@ sub validateAllowMultiple {
 sub validateGroups {
     my $dbh = Bugzilla->dbh;
     # Convert group names to group IDs
-    foreach my $col ("grant_gid", "request_gid") {
-      my $name = $cgi->param($col);
-      if ($name) {
-          trick_taint($name);
-          my $gid = $dbh->selectrow_array('SELECT id FROM groups
-                                           WHERE name = ?', undef, $name);
-          $gid || ThrowUserError("group_unknown", { name => $name });
-          $cgi->param($col, $gid);
-      }
-      else {
-          $cgi->delete($col);
-      }
+    foreach my $col ('grant', 'request') {
+        my $name = $cgi->param($col . '_group');
+        if ($name) {
+            trick_taint($name);
+            my $gid = $dbh->selectrow_array('SELECT id FROM groups
+                                             WHERE name = ?', undef, $name);
+            $gid || ThrowUserError("group_unknown", { name => $name });
+            $cgi->param($col . '_gid', $gid);
+        }
     }
 }
 
@@ -699,3 +670,14 @@ sub validateAndSubmit {
         }
     }
 }
+
+sub filter_group {
+    my $flag_types = shift;
+    return $flag_types unless Bugzilla->cgi->param('group');
+
+    my $gid = scalar $cgi->param('group');
+    my @flag_types = grep {($_->grant_group && $_->grant_group->id == $gid)
+                           || ($_->request_group && $_->request_group->id == $gid)} @$flag_types;
+
+    return \@flag_types;
+}
\ No newline at end of file
diff --git a/importxml.pl b/importxml.pl
index 4796f35fd..4ca9c8745 100755
--- a/importxml.pl
+++ b/importxml.pl
@@ -238,24 +238,24 @@ sub flag_handler {
     # If this is the case, we will only match the first one.
     my $ftype;
     foreach my $f ( @{$flag_types} ) {
-        if ( $f->{'name'} eq $name) {
+        if ( $f->name eq $name) {
             $ftype = $f;
             last;
         }
     }
 
     if ($ftype) {    # We found the flag in the list
-        my $grant_gid = $ftype->{'grant_gid'};
+        my $grant_group = $ftype->grant_group;
         if (( $status eq '+' || $status eq '-' ) 
-            && $grant_gid && !$setter->in_group_id($grant_gid)) {
+            && $grant_group && !$setter->in_group_id($grant_group->id)) {
             $err = "Setter $setter_login on $type flag $name ";
             $err .= "is not in the Grant Group\n";
             $err .= "   Dropping flag $name\n";
             return $err;
         }
-        my $request_gid = $ftype->{'request_gid'};
-        if ($request_gid 
-            && $status eq '?' && !$setter->in_group_id($request_gid)) {
+        my $request_group = $ftype->request_group;
+        if ($request_group
+            && $status eq '?' && !$setter->in_group_id($request_group->id)) {
             $err = "Setter $setter_login on $type flag $name ";
             $err .= "is not in the Request Group\n";
             $err .= "   Dropping flag $name\n";
@@ -263,9 +263,7 @@ sub flag_handler {
         }
 
         # Take the first flag_type that matches
-        my $ftypeid   = $ftype->{'id'};
-        my $is_active = $ftype->{'is_active'};
-        unless ($is_active) {
+        unless ($ftype->is_active) {
             $err = "Flag $name is not active in this database\n";
             $err .= "   Dropping flag $name\n";
             return $err;
@@ -275,7 +273,7 @@ sub flag_handler {
                  (type_id, status, bug_id, attach_id, creation_date, 
                   setter_id, requestee_id)
                   VALUES (?, ?, ?, ?, ?, ?, ?)", undef,
-            ($ftypeid, $status, $bugid, $attachid, $timestamp,
+            ($ftype->id, $status, $bugid, $attachid, $timestamp,
             $setter_id, $requestee_id));
     }
     else {
diff --git a/request.cgi b/request.cgi
index aee9c0387..b41361963 100755
--- a/request.cgi
+++ b/request.cgi
@@ -198,14 +198,10 @@ sub queue {
     if (defined $form_type && !grep($form_type eq $_, ("", "all"))) {
         # Check if any matching types are for attachments.  If not, don't show
         # the attachment column in the report.
-        my $types = Bugzilla::FlagType::match({ 'name' => $form_type });
-        my $has_attachment_type = 0;
-        foreach my $type (@$types) {
-            if ($type->{'target_type'} eq "attachment") {
-                $has_attachment_type = 1;
-                last;
-            }
-        }
+        my $has_attachment_type =
+            Bugzilla::FlagType::count({ 'name' => $form_type,
+                                        'target_type' => 'attachment' });
+
         if (!$has_attachment_type) { push(@excluded_columns, 'attachment') }
 
         my $quoted_form_type = $dbh->quote($form_type);
diff --git a/template/en/default/admin/flag-type/confirm-delete.html.tmpl b/template/en/default/admin/flag-type/confirm-delete.html.tmpl
index 99dba148e..fda34e3b1 100644
--- a/template/en/default/admin/flag-type/confirm-delete.html.tmpl
+++ b/template/en/default/admin/flag-type/confirm-delete.html.tmpl
@@ -29,7 +29,7 @@
 %]
 
 <p>
-   There are [% flag_count %] flags of type [% name FILTER html %].
+   There are [% flag_type.flag_count %] flags of type [% name FILTER html %].
    If you delete this type, those flags will also be deleted.  Note that
    instead of deleting the type you can
    <a href="editflagtypes.cgi?action=deactivate&amp;id=[% flag_type.id %]">deactivate it</a>,
diff --git a/template/en/default/admin/flag-type/edit.html.tmpl b/template/en/default/admin/flag-type/edit.html.tmpl
index 7f88c62bc..5985e8db0 100644
--- a/template/en/default/admin/flag-type/edit.html.tmpl
+++ b/template/en/default/admin/flag-type/edit.html.tmpl
@@ -192,7 +192,7 @@
       <td>
         the group allowed to grant/deny flags of this type
         (to allow all users to grant/deny these flags, select no group)<br>
-        [% PROCESS select selname = "grant_gid" %]
+        [% PROCESS select selname = "grant_group" %]
       </td>
     </tr>
 
@@ -202,7 +202,7 @@
         if flags of this type are requestable, the group allowed to request them
         (to allow all users to request these flags, select no group)<br>
         Note that the request group alone has no effect if the grant group is not defined!<br>
-        [% PROCESS select selname = "request_gid" %]
+        [% PROCESS select selname = "request_group" %]
       </td>
     </tr>
 
@@ -232,7 +232,8 @@
     <option value="">(no group)</option>
     [% FOREACH group = groups %]
       <option value="[% group.name FILTER html %]"
-        [% " selected" IF type.${selname} == group.name %]>[% group.name FILTER html %]
+        [% " selected" IF (type.${selname} && type.${selname}.name == group.name) %]>
+      [%- group.name FILTER html %]
       </option>
     [% END %]
   </select>
diff --git a/template/en/default/admin/flag-type/list.html.tmpl b/template/en/default/admin/flag-type/list.html.tmpl
index 7ca897ecd..f578c3a17 100644
--- a/template/en/default/admin/flag-type/list.html.tmpl
+++ b/template/en/default/admin/flag-type/list.html.tmpl
@@ -51,7 +51,7 @@
 <p>
   You can restrict the list of flag types to those available for a given product
   and component. If a product is selected with no component, only flag types
-  which are available to ALL components of the product are shown.
+  which are available to at least one component of the product are shown.
 </p>
 
 <form action="editflagtypes.cgi" method="get">
@@ -59,8 +59,8 @@
     <tr>
       <th><label for="product">Product:</label></th>
       <td>
-        <select name="product" onchange="selectProduct(this.form, 'product', 'component', '__All__');">
-          <option value="">__All__</option>
+        <select name="product" onchange="selectProduct(this.form, 'product', 'component', '__Any__');">
+          <option value="">__Any__</option>
           [% FOREACH prod = products %]
             <option value="[% prod.name FILTER html %]"
                     [% " selected" IF selected_product == prod.name %]>
@@ -71,7 +71,7 @@
       <th><label for="component">Component:</label></th>
       <td>
         <select name="component">
-          <option value="">__All__</option>
+          <option value="">__Any__</option>
           [% FOREACH comp = components %]
             <option value="[% comp FILTER html %]"
                     [% " selected" IF selected_component == comp %]>
@@ -152,8 +152,8 @@
             <span class="multiplicable">multiplicable</span>
           [% END %]
         </td>
-        <td>[% type.grant_group_name FILTER html %]</td>
-        <td>[% type.request_group_name FILTER html %]</td>
+        <td>[% IF type.grant_group %][% type.grant_group.name FILTER html %][% END %]</td>
+        <td>[% IF type.request_group %][% type.request_group.name FILTER html %][% END %]</td>
         <td>
           <a href="editflagtypes.cgi?action=copy&amp;id=[% type.id %]">Copy</a>
           | <a href="editflagtypes.cgi?action=confirmdelete&amp;id=[% type.id %]"
diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl
index bc2ee0fa8..062408a09 100644
--- a/template/en/default/filterexceptions.pl
+++ b/template/en/default/filterexceptions.pl
@@ -527,7 +527,7 @@
 ],
 
 'admin/flag-type/confirm-delete.html.tmpl' => [
-  'flag_count', 
+  'flag_type.flag_count',
   'flag_type.id', 
 ],
 
diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl
index a131d25d6..f64cf6411 100644
--- a/template/en/default/global/code-error.html.tmpl
+++ b/template/en/default/global/code-error.html.tmpl
@@ -227,10 +227,6 @@
     [% END %]
     is invalid.
 
-  [% ELSIF error == "flag_type_id_invalid" %]
-    The flag type ID <em>[% id FILTER html %]</em> is not
-    a positive integer.
-
   [% ELSIF error == "flag_type_inactive" %]
     [% title = "Inactive Flag Types" %]
     Some flag types are inactive and cannot be used to create new flags.