Bug 223878: Flag system dies when changing a deleted flag.

r=joel, justdave
a=justdave

9 files changed, 61 insertions, 20 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 9f0467bb7..e7b3ffe86 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -90,7 +90,8 @@ sub query
      $a{'datasize'}) = &::FetchSQLData();
 
     # Retrieve a list of flags for this attachment.
-    $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'} });
+    $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'},
+                                          'is_active' => 1 });
     
     # We will display the edit link if the user can edit the attachment;
     # ie the are the submitter, or they have canedit.
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 91cd0d8c8..f1a1cf341 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -226,7 +226,8 @@ sub initBug  {
       $flag_type->{'flags'} = 
         Bugzilla::Flag::match({ 'bug_id'      => $self->{bug_id},
                                 'type_id'     => $flag_type->{'id'},
-                                'target_type' => 'bug' });
+                                'target_type' => 'bug',
+                                'is_active'   => 1 });
   }
   $self->{'flag_types'} = $flag_types;
   $self->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'}, @$flag_types);
@@ -238,11 +239,11 @@ sub initBug  {
   my $num_attachment_flag_types =
     Bugzilla::FlagType::count({ 'target_type'  => 'attachment',
                                 'product_id'   => $self->{'product_id'},
-                                'component_id' => $self->{'component_id'},
-                                'is_active'    => 1 });
+                                'component_id' => $self->{'component_id'} });
   my $num_attachment_flags =
     Bugzilla::Flag::count({ 'target_type'  => 'attachment',
-                            'bug_id'       => $self->{bug_id} });
+                            'bug_id'       => $self->{bug_id},
+                            'is_active'    => 1 });
 
   $self->{'show_attachment_flags'}
     = $num_attachment_flag_types || $num_attachment_flags;
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 3272b8409..707316ce1 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -18,6 +18,7 @@
 # Rights Reserved.
 #
 # Contributor(s): Myk Melez <myk@mozilla.org>
+#                 Jouni Heikniemi <jouni@heikniemi.net>
 
 ################################################################################
 # Module Initialization
@@ -52,8 +53,8 @@ use vars qw($template $vars);
 # basic sets of columns and tables for getting flags from the database
 
 my @base_columns = 
-  ("1", "id", "type_id", "bug_id", "attach_id", "requestee_id", "setter_id",
-   "status");
+  ("is_active", "id", "type_id", "bug_id", "attach_id", "requestee_id", 
+   "setter_id", "status");
 
 # Note: when adding tables to @base_tables, make sure to include the separator 
 # (i.e. a comma or words like "LEFT OUTER JOIN") before the table name, 
@@ -154,6 +155,11 @@ 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", 
@@ -226,7 +232,8 @@ sub process {
     
     # Take a snapshot of flags before any changes.
     my $flags = match({ 'bug_id'    => $target->{'bug'}->{'id'} , 
-                        'attach_id' => $target->{'attachment'}->{'id'} });
+                        'attach_id' => $target->{'attachment'}->{'id'} ,
+                        'is_active' => 1 });
     my @old_summaries;
     foreach my $flag (@$flags) {
         my $summary = $flag->{'type'}->{'name'} . $flag->{'status'};
@@ -249,6 +256,7 @@ 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 = $target->{'bug'}->{'id'} 
+        AND flags.is_active = 1
         AND i.type_id IS NULL
     ");
     clear(&::FetchOneColumn()) while &::MoreSQLData();
@@ -257,7 +265,8 @@ sub process {
         FROM flags, bugs, flagexclusions e
         WHERE bugs.bug_id = $target->{'bug'}->{'id'}
         AND flags.bug_id = bugs.bug_id
-        AND flags.type_id = e.type_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)
     ");
@@ -265,7 +274,8 @@ sub process {
     
     # Take a snapshot of flags after changes.
     $flags = match({ 'bug_id'    => $target->{'bug'}->{'id'} , 
-                     'attach_id' => $target->{'attachment'}->{'id'} });
+                     'attach_id' => $target->{'attachment'}->{'id'} ,
+                     'is_active' => 1 });
     my @new_summaries;
     foreach my $flag (@$flags) {
         my $summary = $flag->{'type'}->{'name'} . $flag->{'status'};
@@ -340,6 +350,9 @@ sub migrate {
 
 sub modify {
     # 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.
 
     my ($data, $timestamp) = @_;
 
@@ -385,7 +398,8 @@ sub modify {
                         SET    setter_id = $::userid , 
                                requestee_id = NULL , 
                                status = '$status' , 
-                               modification_date = $timestamp
+                               modification_date = $timestamp ,
+                               is_active = 1
                         WHERE  id = $flag->{'id'}");
             
             # Send an email notifying the relevant parties about the fulfillment.
@@ -409,7 +423,8 @@ sub modify {
                         SET    setter_id = $::userid , 
                                requestee_id = $requestee_id , 
                                status = '$status' , 
-                               modification_date = $timestamp
+                               modification_date = $timestamp ,
+                               is_active = 1
                         WHERE  id = $flag->{'id'}");
             
             # Send an email notifying the relevant parties about the request.
@@ -420,7 +435,7 @@ sub modify {
                 notify($flag, "request/email.txt.tmpl");
             }
         }
-        # The user unset the flag, so delete it from the database.
+        # The user unset the flag; set is_active = 0
         elsif ($status eq 'X') {
             clear($flag->{'id'});
         }
@@ -437,9 +452,10 @@ sub clear {
     my $flag = get($id);
     
     &::PushGlobalSQLState();
-    &::SendSQL("DELETE FROM flags WHERE id = $id");
+    &::SendSQL("UPDATE flags SET is_active = 0 WHERE id = $id");
     &::PopGlobalSQLState();
-    
+
+    $flag->{'exists'} = 0;    
     # Set the flag's status to "cleared" so the email template
     # knows why email is being sent about the request.
     $flag->{'status'} = "X";
@@ -625,6 +641,7 @@ 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;
@@ -635,6 +652,8 @@ sub perlify_record {
     my ($exists, $id, $type_id, $bug_id, $attach_id, 
         $requestee_id, $setter_id, $status) = @_;
     
+    return undef unless defined($exists);
+    
     my $flag =
       {
         exists    => $exists , 
diff --git a/Bugzilla/FlagType.pm b/Bugzilla/FlagType.pm
index e6bfaf7ef..a428d5389 100644
--- a/Bugzilla/FlagType.pm
+++ b/Bugzilla/FlagType.pm
@@ -270,6 +270,7 @@ 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();
@@ -280,6 +281,7 @@ 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 2f92131fc..0ebf1e070 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -586,7 +586,8 @@ sub init {
              # negative conditions (f.e. "flag isn't review+").
              my $flags = "flags_$chartid";
              push(@supptables, "LEFT JOIN flags $flags " . 
-                               "ON bugs.bug_id = $flags.bug_id");
+                               "ON bugs.bug_id = $flags.bug_id " .
+                               "AND $flags.is_active = 1");
              my $flagtypes = "flagtypes_$chartid";
              push(@supptables, "LEFT JOIN flagtypes $flagtypes " . 
                                "ON $flags.type_id = $flagtypes.id");
diff --git a/attachment.cgi b/attachment.cgi
index c1e8f9dd0..06a04291e 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -768,7 +768,8 @@ sub viewall
      $a{'description'}, $a{'ispatch'}, $a{'isobsolete'}, $a{'isprivate'},
      $a{'datasize'}) = FetchSQLData();
     $a{'isviewable'} = isViewable($a{'contenttype'});
-    $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'} });
+    $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'},
+                                          'is_active' => 1 });
 
     # Add the hash representing the attachment to the array of attachments.
     push @attachments, \%a;
@@ -880,7 +881,9 @@ sub insert
       SendSQL("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when, fieldid, removed, added) 
                VALUES ($::FORM{'bugid'}, $obsolete_id, $::userid, NOW(), $fieldid, '0', '1')");
       # If the obsolete attachment has pending flags, migrate them to the new attachment.
-      if (Bugzilla::Flag::count({ 'attach_id' => $obsolete_id , 'status' => 'pending' })) {
+      if (Bugzilla::Flag::count({ 'attach_id' => $obsolete_id , 
+                                  'status' => 'pending',
+                                  'is_active' => 1 })) {
         Bugzilla::Flag::migrate($obsolete_id, $attachid);
       }
   }
@@ -984,7 +987,8 @@ 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' => $::FORM{'id'} });
+                                                    'attach_id' => $::FORM{'id'},
+                                                    'is_active' => 1 });
   }
   $vars->{'flag_types'} = $flag_types;
   $vars->{'any_flags_requesteeble'} = grep($_->{'is_requesteeble'}, @$flag_types);
diff --git a/checksetup.pl b/checksetup.pl
index 684a8ca7e..3c4793407 100755
--- a/checksetup.pl
+++ b/checksetup.pl
@@ -1609,6 +1609,8 @@ $table{flags} =
      
      setter_id          MEDIUMINT     NULL , 
      requestee_id       MEDIUMINT     NULL , 
+     
+     is_active          TINYINT       NOT NULL  DEFAULT 1, 
    
      INDEX(bug_id, attach_id) , 
      INDEX(setter_id) , 
@@ -3935,6 +3937,11 @@ if (GetFieldDef("user_group_map", "isderived")) {
     }
 }
 
+# 2004-07-03 - Make it possible to disable flags without deleting them
+# from the database. Bug 223878, jouni@heikniemi.net
+
+AddField('flags', 'is_active', 'tinyint not null default 1');
+
     
 
 # If you had to change the --TABLE-- definition in any way, then add your
diff --git a/editflagtypes.cgi b/editflagtypes.cgi
index 5fcabd73f..bb1b86ec0 100755
--- a/editflagtypes.cgi
+++ b/editflagtypes.cgi
@@ -308,6 +308,7 @@ sub update {
             AND (bugs.component_id = i.component_id OR i.component_id IS NULL))
         WHERE flags.type_id = $::FORM{'id'} 
         AND flags.bug_id = bugs.bug_id
+        AND flags.is_active = 1
         AND i.type_id IS NULL
     ");
     Bugzilla::Flag::clear(FetchOneColumn()) while MoreSQLData();
@@ -318,6 +319,7 @@ sub update {
         WHERE flags.type_id = $::FORM{'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)
     ");
@@ -340,7 +342,8 @@ sub confirmDelete
   validateID();
   # check if we need confirmation to delete:
   
-  my $count = Bugzilla::Flag::count({ 'type_id' => $::FORM{'id'} });
+  my $count = Bugzilla::Flag::count({ 'type_id' => $::FORM{'id'},
+                                      'is_active' => 1 });
   
   if ($count > 0) {
     $vars->{'flag_type'} = Bugzilla::FlagType::get($::FORM{'id'});
diff --git a/request.cgi b/request.cgi
index e330c2c83..047c4fa14 100755
--- a/request.cgi
+++ b/request.cgi
@@ -116,6 +116,9 @@ sub queue {
       AND       flags.bug_id        = bugs.bug_id
     ";
     
+    # Non-deleted flags only
+    $query .= " AND flags.is_active = 1 ";
+    
     # Limit query to pending requests.
     $query .= " AND flags.status = '?' " unless $cgi->param('status');