Bug 364216: Flag::retarget() is broken (again) - Patch by Frédéric Buclin <LpSolit@gmail.com> r/a=myk

2 files changed, 43 insertions, 33 deletions

diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index f0acc3d2e..9e7483838 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -139,6 +139,11 @@ Returns the user who set the flag, as a Bugzilla::User object.
 Returns the user who has been requested to set the flag, as a
 Bugzilla::User object.
 
+=item C<attachment>
+
+Returns the attachment object the flag belongs to if the flag
+is an attachment flag, else undefined.
+
 =back
 
 =cut
@@ -166,6 +171,15 @@ sub requestee {
     return $self->{'requestee'};
 }
 
+sub attachment {
+    my $self = shift;
+    return undef unless $self->attach_id;
+
+    require Bugzilla::Attachment;
+    $self->{'attachment'} ||= Bugzilla::Attachment->get($self->attach_id);
+    return $self->{'attachment'};
+}
+
 ################################
 ## Searching/Retrieving Flags ##
 ################################
@@ -519,9 +533,10 @@ sub process {
             AND i.type_id IS NULL",
         undef, $bug_id);
 
-    foreach my $flag_id (@$flag_ids) {
-        my $is_retargetted = retarget($flag_id, $bug, $attachment);
-        clear($flag_id, $bug, $attachment) unless $is_retargetted;
+    my $flags = Bugzilla::Flag->new_from_list($flag_ids);
+    foreach my $flag (@$flags) {
+        my $is_retargetted = retarget($flag, $bug);
+        clear($flag, $bug, $flag->attachment) unless $is_retargetted;
     }
 
     $flag_ids = $dbh->selectcol_arrayref(
@@ -534,9 +549,10 @@ sub process {
         AND (bugs.component_id = e.component_id OR e.component_id IS NULL)",
         undef, $bug_id);
 
-    foreach my $flag_id (@$flag_ids) {
-        my $is_retargetted = retarget($flag_id, $bug, $attachment);
-        clear($flag_id, $bug, $attachment) unless $is_retargetted;
+    $flags = Bugzilla::Flag->new_from_list($flag_ids);
+    foreach my $flag (@$flags) {
+        my $is_retargetted = retarget($flag, $bug);
+        clear($flag, $bug, $flag->attachment) unless $is_retargetted;
     }
 
     # Take a snapshot of flags after changes.
@@ -751,7 +767,7 @@ sub modify {
             notify($flag, $bug, $attachment);
         }
         elsif ($status eq 'X') {
-            clear($flag->id, $bug, $attachment);
+            clear($flag, $bug, $attachment);
         }
 
         push(@flags, $flag);
@@ -764,11 +780,11 @@ sub modify {
 
 =over
 
-=item C<retarget($flag_id, $bug, $attachment)>
+=item C<retarget($flag, $bug)>
 
 Change the type of the flag, if possible. The new flag type must have
 the same name as the current flag type, must exist in the product and
-attachment the bug is in, and the current settings of the flag must pass
+component the bug is in, and the current settings of the flag must pass
 validation. If no such flag type can be found, the type remains unchanged.
 
 Retargetting flags is a good way to keep flags when moving bugs from one
@@ -782,16 +798,15 @@ the same meaning, but with different settings.
 =cut
 
 sub retarget {
-    my ($flag_id, $bug, $attachment) = @_;
+    my ($flag, $bug) = @_;
     my $dbh = Bugzilla->dbh;
 
-    my $flag = new Bugzilla::Flag($flag_id);
     # We are looking for flagtypes having the same name as the flagtype
     # to which the current flag belongs, and being in the new product and
     # component of the bug.
     my $flagtypes = Bugzilla::FlagType::match(
                         {'name'         => $flag->name,
-                         'target_type'  => $attachment ? 'attachment' : 'bug',
+                         'target_type'  => $flag->type->target_type,
                          'is_active'    => 1,
                          'product_id'   => $bug->product_id,
                          'component_id' => $bug->component_id});
@@ -803,17 +818,15 @@ sub retarget {
     # assuming the setter/requester is allowed to set/request flags
     # belonging to this flagtype.
     my $requestee = $flag->requestee ? [$flag->requestee->login] : [];
-    my $attach_id = $attachment ? $attachment->id : undef;
-    my $is_private = $attachment ? $attachment->isprivate : 0;
+    my $is_private = ($flag->attachment) ? $flag->attachment->isprivate : 0;
     my $is_retargetted = 0;
 
     foreach my $flagtype (@$flagtypes) {
         # Get the number of flags of this type already set for this target.
         my $has_flags = count(
             { 'type_id'     => $flagtype->id,
-              'target_type' => $attachment ? 'attachment' : 'bug',
               'bug_id'      => $bug->bug_id,
-              'attach_id'   => $attach_id });
+              'attach_id'   => $flag->attach_id });
 
         # Do not create a new flag of this type if this flag type is
         # not multiplicable and already has a flag set.
@@ -824,7 +837,7 @@ sub retarget {
         Bugzilla->error_mode(ERROR_MODE_DIE);
         eval {
             _validate(undef, $flagtype, $flag->status, $flag->setter,
-                      $requestee, $is_private, $bug->bug_id, $attach_id);
+                      $requestee, $is_private, $bug->bug_id, $flag->attach_id);
         };
         Bugzilla->error_mode($error_mode_cache);
         # If the validation failed, then we cannot use this flagtype.
@@ -844,7 +857,7 @@ sub retarget {
 
 =over
 
-=item C<clear($id, $bug, $attachment)>
+=item C<clear($flag, $bug, $attachment)>
 
 Remove a flag from the DB.
 
@@ -853,11 +866,10 @@ Remove a flag from the DB.
 =cut
 
 sub clear {
-    my ($id, $bug, $attachment) = @_;
+    my ($flag, $bug, $attachment) = @_;
     my $dbh = Bugzilla->dbh;
 
-    my $flag = new Bugzilla::Flag($id);
-    $dbh->do('DELETE FROM flags WHERE id = ?', undef, $id);
+    $dbh->do('DELETE FROM flags WHERE id = ?', undef, $flag->id);
 
     # If we cancel a pending request, we have to notify the requester
     # (if he wants to).
@@ -1042,7 +1054,8 @@ sub CancelRequests {
 
     # Take a snapshot of flags before any changes.
     my @old_summaries = snapshot($bug->bug_id, $attachment->id) if ($timestamp);
-    foreach my $flag (@$request_ids) { clear($flag, $bug, $attachment) }
+    my $flags = Bugzilla::Flag->new_from_list($request_ids);
+    foreach my $flag (@$flags) { clear($flag, $bug, $attachment) }
 
     # If $timestamp is undefined, do not update the activity table
     return unless ($timestamp);
diff --git a/editflagtypes.cgi b/editflagtypes.cgi
index 6e001a525..0aec0385d 100755
--- a/editflagtypes.cgi
+++ b/editflagtypes.cgi
@@ -387,8 +387,7 @@ sub update {
 
     # Clear existing flags for bugs/attachments in categories no longer on 
     # the list of inclusions or that have been added to the list of exclusions.
-    my $flags =    $dbh->selectall_arrayref('SELECT DISTINCT flags.id, flags.bug_id,
-                                                             flags.attach_id
+    my $flag_ids = $dbh->selectcol_arrayref('SELECT DISTINCT flags.id
                                                FROM flags
                                          INNER JOIN bugs
                                                  ON flags.bug_id = bugs.bug_id
@@ -401,14 +400,13 @@ sub update {
                                               WHERE flags.type_id = ?
                                                 AND i.type_id IS NULL',
                                              undef, $id);
+    my $flags = Bugzilla::Flag->new_from_list($flag_ids);
     foreach my $flag (@$flags) {
-        my ($flag_id, $bug_id, $attach_id) = @$flag;
-        my $bug = new Bugzilla::Bug($bug_id);
-        my $attachment = $attach_id ? Bugzilla::Attachment->get($attach_id) : undef;
-        Bugzilla::Flag::clear($flag_id, $bug, $attachment);
+        my $bug = new Bugzilla::Bug($flag->bug_id);
+        Bugzilla::Flag::clear($flag, $bug, $flag->attachment);
     }
 
-    $flags =    $dbh->selectall_arrayref('SELECT DISTINCT flags.id, flags.bug_id, flags.attach_id
+    $flag_ids = $dbh->selectcol_arrayref('SELECT DISTINCT flags.id
                                             FROM flags
                                       INNER JOIN bugs 
                                               ON flags.bug_id = bugs.bug_id
@@ -420,11 +418,10 @@ sub update {
                                              AND (bugs.component_id = e.component_id
                                                   OR e.component_id IS NULL)',
                                           undef, $id);
+    $flags = Bugzilla::Flag->new_from_list($flag_ids);
     foreach my $flag (@$flags) {
-        my ($flag_id, $bug_id, $attach_id) = @$flag;
-        my $bug = new Bugzilla::Bug($bug_id);
-        my $attachment = $attach_id ? Bugzilla::Attachment->get($attach_id) : undef;
-        Bugzilla::Flag::clear($flag_id, $bug, $attachment);
+        my $bug = new Bugzilla::Bug($flag->bug_id);
+        Bugzilla::Flag::clear($flag, $bug, $flag->attachment);
     }
 
     # Now silently remove requestees from flags which are no longer