Bug 322285: Cancelling a flag should remove it completely from the DB - Patch by Frédéric Buclin <LpSolit@gmail.com> r/a=myk

12 files changed, 52 insertions, 87 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 8b0203924..350adfd72 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -373,8 +373,7 @@ sub flags {
     my $self = shift;
     return $self->{flags} if exists $self->{flags};
 
-    $self->{flags} = Bugzilla::Flag::match({ attach_id => $self->id,
-                                             is_active => 1 });
+    $self->{flags} = Bugzilla::Flag::match({ 'attach_id' => $self->id });
     return $self->{flags};
 }
 
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 445e13ec6..c0063aa14 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -429,8 +429,7 @@ sub flag_types {
         $flag_type->{'flags'} = Bugzilla::Flag::match(
             { 'bug_id'      => $self->bug_id,
               'type_id'     => $flag_type->{'id'},
-              'target_type' => 'bug',
-              'is_active'   => 1 });
+              'target_type' => 'bug' });
     }
 
     $self->{'flag_types'} = $flag_types;
@@ -515,8 +514,7 @@ sub show_attachment_flags {
           'component_id' => $self->{'component_id'} });
     my $num_attachment_flags = Bugzilla::Flag::count(
         { 'target_type'  => 'attachment',
-          'bug_id'       => $self->bug_id,
-          'is_active'    => 1 });
+          'bug_id'       => $self->bug_id });
 
     $self->{'show_attachment_flags'} =
         ($num_attachment_flag_types || $num_attachment_flags);
diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm
index 32f09a099..16b5320d6 100644
--- a/Bugzilla/DB/Schema.pm
+++ b/Bugzilla/DB/Schema.pm
@@ -375,7 +375,7 @@ use constant ABSTRACT_SCHEMA => {
     # "flags" stores one record for each flag on each bug/attachment.
     flags => {
         FIELDS => [
-            id                => {TYPE => 'INT3', NOTNULL => 1,
+            id                => {TYPE => 'MEDIUMSERIAL', NOTNULL => 1,
                                   PRIMARYKEY => 1},
             type_id           => {TYPE => 'INT2', NOTNULL => 1},
             status            => {TYPE => 'char(1)', NOTNULL => 1},
@@ -385,8 +385,6 @@ use constant ABSTRACT_SCHEMA => {
             modification_date => {TYPE => 'DATETIME'},
             setter_id         => {TYPE => 'INT3'},
             requestee_id      => {TYPE => 'INT3'},
-            is_active         => {TYPE => 'BOOLEAN', NOTNULL => 1,
-                                  DEFAULT => 'TRUE'},
         ],
         INDEXES => [
             flags_bug_id_idx       => [qw(bug_id attach_id)],
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index a3929a970..97d8e8c7c 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -94,7 +94,7 @@ database.  B<Used by get, match, sqlify_criteria and perlify_record>
 =cut
 
 my @base_columns = 
-  ("is_active", "id", "type_id", "bug_id", "attach_id", "requestee_id", 
+  ("id", "type_id", "bug_id", "attach_id", "requestee_id", 
    "setter_id", "status");
 
 =pod
@@ -284,11 +284,6 @@ sub validate {
         my $flag = get($id);
         $flag || ThrowCodeError("flag_nonexistent", { id => $id });
 
-        # Note that the deletedness of the flag (is_active or not) is not 
-        # checked here; we do want to allow changes to deleted flags in
-        # certain cases. Flag::modify() will revive the modified flags.
-        # See bug 223878 for details.
-
         # Make sure the user chose a valid status.
         grep($status eq $_, qw(X + - ?))
           || ThrowCodeError("flag_status_invalid", 
@@ -398,8 +393,7 @@ sub snapshot {
     my ($bug_id, $attach_id) = @_;
 
     my $flags = match({ 'bug_id'    => $bug_id,
-                        'attach_id' => $attach_id,
-                        'is_active' => 1 });
+                        'attach_id' => $attach_id });
     my @summaries;
     foreach my $flag (@$flags) {
         my $summary = $flag->{'type'}->{'name'} . $flag->{'status'};
@@ -466,7 +460,6 @@ sub process {
             AND (bugs.product_id = i.product_id OR i.product_id IS NULL)
             AND (bugs.component_id = i.component_id OR i.component_id IS NULL)
           WHERE bugs.bug_id = ?
-            AND flags.is_active = 1
             AND i.type_id IS NULL",
         undef, $bug_id);
 
@@ -478,7 +471,6 @@ sub process {
         WHERE bugs.bug_id = ?
         AND flags.bug_id = bugs.bug_id
         AND flags.type_id = e.type_id
-        AND flags.is_active = 1 
         AND (bugs.product_id = e.product_id OR e.product_id IS NULL)
         AND (bugs.component_id = e.component_id OR e.component_id IS NULL)",
         undef, $bug_id);
@@ -529,22 +521,15 @@ Creates a flag record in the database.
 sub create {
     my ($flag, $timestamp) = @_;
 
-    # Determine the ID for the flag record by retrieving the last ID used
-    # and incrementing it.
-    &::SendSQL("SELECT MAX(id) FROM flags");
-    $flag->{'id'} = (&::FetchOneColumn() || 0) + 1;
-    
-    # Insert a record for the flag into the flags table.
     my $attach_id =
       $flag->{target}->{attachment} ? $flag->{target}->{attachment}->{id}
                                     : "NULL";
     my $requestee_id = $flag->{'requestee'} ? $flag->{'requestee'}->id : "NULL";
-    &::SendSQL("INSERT INTO flags (id, type_id, 
-                                      bug_id, attach_id, 
-                                      requestee_id, setter_id, status, 
-                                      creation_date, modification_date)
-                VALUES ($flag->{'id'}, 
-                        $flag->{'type'}->{'id'}, 
+
+    &::SendSQL("INSERT INTO flags (type_id, bug_id, attach_id, 
+                                   requestee_id, setter_id, status, 
+                                   creation_date, modification_date)
+                VALUES ($flag->{'type'}->{'id'}, 
                         $flag->{'target'}->{'bug'}->{'id'}, 
                         $attach_id,
                         $requestee_id,
@@ -597,9 +582,6 @@ sub migrate {
 =item C<modify($cgi, $timestamp)>
 
 Modifies flags in the database when a user changes them.
-Note that modified flags are always set active (is_active = 1) -
-this will revive deleted flags that get changed through 
-attachment.cgi midairs. See bug 223878 for details.
 
 =back
 
@@ -681,8 +663,7 @@ sub modify {
                         SET    setter_id = " . $setter->id . ", 
                                requestee_id = NULL , 
                                status = '$status' , 
-                               modification_date = $timestamp ,
-                               is_active = 1
+                               modification_date = $timestamp
                         WHERE  id = $flag->{'id'}");
 
             # If the status of the flag was "?", we have to notify
@@ -722,8 +703,7 @@ sub modify {
                         SET    setter_id = " . $setter->id . ", 
                                requestee_id = $requestee_id , 
                                status = '$status' , 
-                               modification_date = $timestamp ,
-                               is_active = 1
+                               modification_date = $timestamp
                         WHERE  id = $flag->{'id'}");
 
             # Now update the flag object with its new values.
@@ -739,7 +719,6 @@ sub modify {
 
             notify($flag, "request/email.txt.tmpl");
         }
-        # The user unset the flag; set is_active = 0
         elsif ($status eq 'X') {
             clear($flag->{'id'});
         }
@@ -756,7 +735,7 @@ sub modify {
 
 =item C<clear($id)>
 
-Deactivate a flag.
+Remove a flag from the DB.
 
 =back
 
@@ -764,12 +743,10 @@ Deactivate a flag.
 
 sub clear {
     my ($id) = @_;
-    
+    my $dbh = Bugzilla->dbh;
+
     my $flag = get($id);
-    
-    &::PushGlobalSQLState();
-    &::SendSQL("UPDATE flags SET is_active = 0 WHERE id = $id");
-    &::PopGlobalSQLState();
+    $dbh->do('DELETE FROM flags WHERE id = ?', undef, $id);
 
     # If we cancel a pending request, we have to notify the requester
     # (if he wants to).
@@ -834,17 +811,16 @@ sub FormToNewFlags {
         # We are only interested in flags the user tries to create.
         next unless scalar(grep { $_ == $type_id } @type_ids);
 
-        # Get the number of active flags of this type already set for this target.
+        # Get the number of flags of this type already set for this target.
         my $has_flags = count(
             { 'type_id'     => $type_id,
               'target_type' => $target->{'type'},
               'bug_id'      => $target->{'bug'}->{'id'},
               'attach_id'   => $target->{'attachment'} ?
-                                 $target->{'attachment'}->{'id'} : undef,
-              'is_active'   => 1 });
+                                 $target->{'attachment'}->{'id'} : undef });
 
         # Do not create a new flag of this type if this flag type is
-        # not multiplicable and already has an active flag set.
+        # not multiplicable and already has a flag set.
         next if (!$flag_type->{'is_multiplicable'} && $has_flags);
 
         my $status = $cgi->param("flag_type-$type_id");
@@ -1032,7 +1008,6 @@ sub CancelRequests {
                                   LEFT JOIN attachments ON flags.attach_id = attachments.attach_id
                                   WHERE flags.attach_id = ?
                                   AND flags.status = '?'
-                                  AND flags.is_active = 1
                                   AND attachments.isobsolete = 0",
                                   undef, $attach_id);
 
@@ -1094,7 +1069,6 @@ sub sqlify_criteria {
         elsif ($field eq 'requestee_id') { push(@criteria, "requestee_id = $value") }
         elsif ($field eq 'setter_id')    { push(@criteria, "setter_id    = $value") }
         elsif ($field eq 'status')       { push(@criteria, "status       = '$value'") }
-        elsif ($field eq 'is_active')    { push(@criteria, "is_active    = $value") }
     }
     
     return @criteria;
@@ -1115,14 +1089,13 @@ Converts a row from the database into a Perl record.
 =cut
 
 sub perlify_record {
-    my ($exists, $id, $type_id, $bug_id, $attach_id, 
+    my ($id, $type_id, $bug_id, $attach_id, 
         $requestee_id, $setter_id, $status) = @_;
     
-    return undef unless defined($exists);
+    return undef unless $id;
     
     my $flag =
       {
-        exists    => $exists , 
         id        => $id ,
         type      => Bugzilla::FlagType::get($type_id) ,
         target    => get_target($bug_id, $attach_id) , 
@@ -1153,6 +1126,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/FlagType.pm b/Bugzilla/FlagType.pm
index 950aeea9a..b18c4a2de 100644
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -474,7 +474,6 @@ sub normalize {
             AND (bugs.product_id = i.product_id OR i.product_id IS NULL)
             AND (bugs.component_id = i.component_id OR i.component_id IS NULL))
         WHERE flags.type_id IN ($ids)
-        AND flags.is_active = 1
         AND i.type_id IS NULL
     ");
     Bugzilla::Flag::clear(&::FetchOneColumn()) while &::MoreSQLData();
@@ -485,7 +484,6 @@ sub normalize {
         WHERE flags.type_id IN ($ids)
         AND flags.bug_id = bugs.bug_id
         AND flags.type_id = e.type_id 
-        AND flags.is_active = 1
         AND (bugs.product_id = e.product_id OR e.product_id IS NULL)
         AND (bugs.component_id = e.component_id OR e.component_id IS NULL)
     ");
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index 22ab90f56..960ff336d 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -843,8 +843,7 @@ sub init {
              # negative conditions (f.e. "flag isn't review+").
              my $flags = "flags_$chartid";
              push(@supptables, "LEFT JOIN flags AS $flags " . 
-                               "ON bugs.bug_id = $flags.bug_id " .
-                               "AND $flags.is_active = 1");
+                               "ON bugs.bug_id = $flags.bug_id ");
              my $flagtypes = "flagtypes_$chartid";
              push(@supptables, "LEFT JOIN flagtypes AS $flagtypes " . 
                                "ON $flags.type_id = $flagtypes.id");
@@ -874,8 +873,7 @@ sub init {
          "^requestees.login_name," => sub {
              my $flags = "flags_$chartid";
              push(@supptables, "LEFT JOIN flags AS $flags " .
-                               "ON bugs.bug_id = $flags.bug_id " .
-                               "AND $flags.is_active = 1");
+                               "ON bugs.bug_id = $flags.bug_id ");
              push(@supptables, "LEFT JOIN profiles AS requestees_$chartid " .
                                "ON $flags.requestee_id = requestees_$chartid.userid");
              $f = "requestees_$chartid.login_name";
@@ -883,8 +881,7 @@ sub init {
          "^setters.login_name," => sub {
              my $flags = "flags_$chartid";
              push(@supptables, "LEFT JOIN flags AS $flags " .
-                               "ON bugs.bug_id = $flags.bug_id " .
-                               "AND $flags.is_active = 1");
+                               "ON bugs.bug_id = $flags.bug_id ");
              push(@supptables, "LEFT JOIN profiles AS setters_$chartid " .
                                "ON $flags.setter_id = setters_$chartid.userid");
              $f = "setters_$chartid.login_name";
diff --git a/attachment.cgi b/attachment.cgi
index a824ab71e..c212c6f36 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -836,8 +836,7 @@ sub viewall
   {
     
     $a->{'isviewable'} = isViewable($a->{'contenttype'});
-    $a->{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a->{'attachid'},
-                                          'is_active' => 1 });
+    $a->{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a->{'attachid'} });
   }
 
   # Retrieve the bug summary (for displaying on screen) and assignee.
@@ -1151,8 +1150,7 @@ sub edit {
                                                'component_id' => $component_id });
   foreach my $flag_type (@$flag_types) {
     $flag_type->{'flags'} = Bugzilla::Flag::match({ 'type_id'   => $flag_type->{'id'},
-                                                    'attach_id' => $attachment->id,
-                                                    'is_active' => 1 });
+                                                    'attach_id' => $attachment->id });
   }
   $vars->{'flag_types'} = $flag_types;
   $vars->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'}, @$flag_types);
diff --git a/checksetup.pl b/checksetup.pl
index ab847674b..d572c2bf3 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -3621,12 +3621,6 @@ if ($dbh->bz_column_info("user_group_map", "isderived")) {
     $dbh->do("UPDATE groups SET last_changed = NOW() WHERE name = 'admin'");
 }
 
-# 2004-07-03 - Make it possible to disable flags without deleting them
-# from the database. Bug 223878, jouni@heikniemi.net
-
-$dbh->bz_add_column('flags', 'is_active', 
-                    {TYPE => 'BOOLEAN', NOTNULL => 1, DEFAULT => 'TRUE'});
-
 # 2004-07-16 - Make it possible to have group-group relationships other than
 # membership and bless.
 if ($dbh->bz_column_info("group_group_map", "isbless")) {
@@ -4283,6 +4277,22 @@ $dbh->bz_add_column('fielddefs', 'custom',
 $dbh->bz_add_column('longdescs', 'comment_id',
                     {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
 
+# 2006-03-02 LpSolit@gmail.com - Bug 322285
+# Do not store inactive flags in the DB anymore.
+if ($dbh->bz_column_info('flags', 'id')->{'TYPE'} eq 'INT3') {
+    # We first have to remove all existing inactive flags.
+    if ($dbh->bz_column_info('flags', 'is_active')) {
+        $dbh->do('DELETE FROM flags WHERE is_active = 0');
+    }
+
+    # Now we convert the id column to the auto_increment format.
+    $dbh->bz_alter_column('flags', 'id',
+                          {TYPE => 'MEDIUMSERIAL', NOTNULL => 1, PRIMARYKEY => 1});
+
+    # And finally, we remove the is_active column.
+    $dbh->bz_drop_column('flags', 'is_active');
+}
+
 # If you had to change the --TABLE-- definition in any way, then add your
 # differential change code *** A B O V E *** this comment.
 #
diff --git a/editflagtypes.cgi b/editflagtypes.cgi
index 29a7467ad..bcab4e189 100755
--- a/editflagtypes.cgi
+++ b/editflagtypes.cgi
@@ -359,7 +359,6 @@ sub update {
                                                      AND (bugs.component_id = i.component_id
                                                           OR i.component_id IS NULL))
                                               WHERE flags.type_id = ?
-                                                AND flags.is_active = 1
                                                 AND i.type_id IS NULL',
                                              undef, $id);
     foreach my $flag_id (@$flag_ids) {
@@ -373,7 +372,6 @@ sub update {
                                       INNER JOIN flagexclusions AS e
                                               ON flags.type_id = e.type_id
                                            WHERE flags.type_id = ?
-                                             AND flags.is_active = 1
                                              AND (bugs.product_id = e.product_id
                                                   OR e.product_id IS NULL)
                                              AND (bugs.component_id = e.component_id
@@ -401,8 +399,7 @@ sub confirmDelete
 
   # check if we need confirmation to delete:
   
-  my $count = Bugzilla::Flag::count({ 'type_id' => $id,
-                                      'is_active' => 1 });
+  my $count = Bugzilla::Flag::count({ 'type_id' => $id });
   
   if ($count > 0) {
     $vars->{'flag_type'} = Bugzilla::FlagType::get($id);
diff --git a/editusers.cgi b/editusers.cgi
index 639e5ee1c..c5e67e28b 100755
--- a/editusers.cgi
+++ b/editusers.cgi
@@ -453,7 +453,7 @@ if ($action eq 'search') {
         'SELECT COUNT(*) FROM email_setting WHERE user_id = ?',
         undef, $otherUserID);
     $vars->{'flags'}{'requestee'} = $dbh->selectrow_array(
-        'SELECT COUNT(*) FROM flags WHERE requestee_id = ? AND is_active = 1',
+        'SELECT COUNT(*) FROM flags WHERE requestee_id = ?',
         undef, $otherUserID);
     $vars->{'flags'}{'setter'} = $dbh->selectrow_array(
         'SELECT COUNT(*) FROM flags WHERE setter_id = ?',
diff --git a/importxml.pl b/importxml.pl
index 494a00eca..47f886c1c 100755
--- a/importxml.pl
+++ b/importxml.pl
@@ -304,14 +304,12 @@ sub flag_handler {
             return $err;
         }
 
-        my ($fid) = $dbh->selectrow_array("SELECT MAX(id) FROM flags") || 0;
         $dbh->do("INSERT INTO flags 
-                 (id, type_id, status, bug_id, attach_id, creation_date, 
-                  setter_id, requestee_id, is_active)
-                  VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)", undef,
-            ++$fid,     $ftypeid,      $status, $bugid, $attachid, $timestamp,
-            $setter_id, $requestee_id, 1
-        );
+                 (type_id, status, bug_id, attach_id, creation_date, 
+                  setter_id, requestee_id)
+                  VALUES (?, ?, ?, ?, ?, ?, ?)", undef,
+            ($ftypeid, $status, $bugid, $attachid, $timestamp,
+            $setter_id, $requestee_id));
     }
     else {
         $err = "Dropping unknown $type flag: $name\n";
diff --git a/request.cgi b/request.cgi
index 1faeb1793..5563f32f7 100755
--- a/request.cgi
+++ b/request.cgi
@@ -128,9 +128,6 @@ sub queue {
                  (Param('useqacontact') ? "OR
                  (bugs.qa_contact = $userid))" : ")");
     
-    # Non-deleted flags only
-    $query .= " AND flags.is_active = 1 ";
-    
     # Limit query to pending requests.
     $query .= " AND flags.status = '?' " unless $status;