Bug 453743: Decrease the number of calls to the DB about flags when viewing a bug - Patch by Frédéric Buclin <LpSolit@gmail.com> r/a=mkanat

5 files changed, 107 insertions, 41 deletions

diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm
index 314227c87..fcaf38c9f 100644
--- a/Bugzilla/Attachment.pm
+++ b/Bugzilla/Attachment.pm
@@ -139,8 +139,6 @@ the ID of the bug to which the attachment is attached
 
 =cut
 
-# XXX Once Bug.pm slims down sufficiently this should become a reference
-# to a bug object.
 sub bug_id {
     my $self = shift;
     return $self->{bug_id};
@@ -148,6 +146,24 @@ sub bug_id {
 
 =over
 
+=item C<bug>
+
+the bug object to which the attachment is attached
+
+=back
+
+=cut
+
+sub bug {
+    my $self = shift;
+
+    require Bugzilla::Bug;
+    $self->{bug} = Bugzilla::Bug->new($self->bug_id);
+    return $self->{bug};
+}
+
+=over
+
 =item C<description>
 
 user-provided text describing the attachment
@@ -430,6 +446,30 @@ sub flags {
     return $self->{flags};
 }
 
+=over
+
+=item C<flag_types>
+
+Return all flag types available for this attachment as well as flags
+already set, grouped by flag type.
+
+=back
+
+=cut
+
+sub flag_types {
+    my $self = shift;
+    return $self->{flag_types} if exists $self->{flag_types};
+
+    my $vars = { target_type  => 'attachment',
+                 product_id   => $self->bug->product_id,
+                 component_id => $self->bug->component_id,
+                 attach_id    => $self->id };
+
+    $self->{flag_types} = Bugzilla::Flag::_flag_types($vars);
+    return $self->{flag_types};
+}
+
 # Instance methods; no POD documentation here yet because the only ones so far
 # are private.
 
@@ -538,7 +578,7 @@ Returns:    a reference to an array of attachment objects.
 =cut
 
 sub get_attachments_by_bug {
-    my ($class, $bug_id) = @_;
+    my ($class, $bug_id, $vars) = @_;
     my $user = Bugzilla->user;
     my $dbh = Bugzilla->dbh;
 
@@ -556,6 +596,20 @@ sub get_attachments_by_bug {
                                                WHERE bug_id = ? $and_restriction",
                                                undef, @values);
     my $attachments = Bugzilla::Attachment->get_list($attach_ids);
+
+    # To avoid $attachment->flags to run SQL queries itself for each
+    # attachment listed here, we collect all the data at once and
+    # populate $attachment->{flags} ourselves.
+    if ($vars->{preload}) {
+        $_->{flags} = [] foreach @$attachments;
+        my %att = map { $_->id => $_ } @$attachments;
+
+        my $flags = Bugzilla::Flag->match({ bug_id      => $bug_id,
+                                            target_type => 'attachment' });
+
+        push(@{$att{$_->attach_id}->{flags}}, $_) foreach @$flags;
+        $attachments = [sort {$a->id <=> $b->id} values %att];
+    }
     return $attachments;
 }
 
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index d625953a9..14f781c42 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -2314,7 +2314,7 @@ sub attachments {
     return [] if $self->{'error'};
 
     $self->{'attachments'} =
-        Bugzilla::Attachment->get_attachments_by_bug($self->bug_id);
+        Bugzilla::Attachment->get_attachments_by_bug($self->bug_id, {preload => 1});
     return $self->{'attachments'};
 }
 
@@ -2421,22 +2421,12 @@ sub flag_types {
     return $self->{'flag_types'} if exists $self->{'flag_types'};
     return [] if $self->{'error'};
 
-    # The types of flags that can be set on this bug.
-    # If none, no UI for setting flags will be displayed.
-    my $flag_types = Bugzilla::FlagType::match(
-        {'target_type'  => 'bug',
-         'product_id'   => $self->{'product_id'}, 
-         'component_id' => $self->{'component_id'} });
-
-    foreach my $flag_type (@$flag_types) {
-        $flag_type->{'flags'} = Bugzilla::Flag->match(
-            { 'bug_id'      => $self->bug_id,
-              'type_id'     => $flag_type->{'id'},
-              'target_type' => 'bug' });
-    }
-
-    $self->{'flag_types'} = $flag_types;
+    my $vars = { target_type  => 'bug',
+                 product_id   => $self->{product_id},
+                 component_id => $self->{component_id},
+                 bug_id       => $self->bug_id };
 
+    $self->{'flag_types'} = Bugzilla::Flag::_flag_types($vars);
     return $self->{'flag_types'};
 }
 
diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm
index 8201a907d..ea3e940d8 100644
--- a/Bugzilla/Flag.pm
+++ b/Bugzilla/Flag.pm
@@ -1158,6 +1158,44 @@ sub CancelRequests {
                     \@old_summaries, \@new_summaries);
 }
 
+# This is an internal function used by $bug->flag_types
+# and $attachment->flag_types to collect data about available
+# flag types and existing flags set on them. You should never
+# call this function directly.
+sub _flag_types {
+    my $vars = shift;
+
+    my $target_type = $vars->{target_type};
+    my $flags;
+
+    # Retrieve all existing flags for this bug/attachment.
+    if ($target_type eq 'bug') {
+        my $bug_id = delete $vars->{bug_id};
+        $flags = Bugzilla::Flag->match({target_type => 'bug', bug_id => $bug_id});
+    }
+    elsif ($target_type eq 'attachment') {
+        my $attach_id = delete $vars->{attach_id};
+        $flags = Bugzilla::Flag->match({attach_id => $attach_id});
+    }
+    else {
+        ThrowCodeError('bad_arg', {argument => 'target_type',
+                                   function => 'Bugzilla::Flag::_flag_types'});
+    }
+
+    # Get all available flag types for the given product and component.
+    my $flag_types = Bugzilla::FlagType::match($vars);
+
+    $_->{flags} = [] foreach @$flag_types;
+    my %flagtypes = map { $_->id => $_ } @$flag_types;
+
+    # Group existing flags per type.
+    # Call the internal 'type_id' variable instead of the method
+    # to not create a flagtype object.
+    push(@{$flagtypes{$_->{type_id}}->{flags}}, $_) foreach @$flags;
+
+    return [sort {$a->sortkey <=> $b->sortkey || $a->name cmp $b->name} values %flagtypes];
+}
+
 =head1 SEE ALSO
 
 =over
diff --git a/attachment.cgi b/attachment.cgi
index c28a300a0..4f3dabd55 100755
--- a/attachment.cgi
+++ b/attachment.cgi
@@ -435,32 +435,14 @@ sub insert {
 # Validations are done later when the user submits changes.
 sub edit {
   my $attachment = validateID();
-  my $dbh = Bugzilla->dbh;
 
-  # Retrieve a list of attachments for this bug as well as a summary of the bug
-  # to use in a navigation bar across the top of the screen.
   my $bugattachments =
       Bugzilla::Attachment->get_attachments_by_bug($attachment->bug_id);
   # We only want attachment IDs.
   @$bugattachments = map { $_->id } @$bugattachments;
 
-  my ($bugsummary, $product_id, $component_id) =
-      $dbh->selectrow_array('SELECT short_desc, product_id, component_id
-                               FROM bugs
-                              WHERE bug_id = ?', undef, $attachment->bug_id);
-
-  # Get a list of flag types that can be set for this attachment.
-  my $flag_types = Bugzilla::FlagType::match({ 'target_type'  => 'attachment' ,
-                                               '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,
-                                                    '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, @{$attachment->flag_types});
   $vars->{'attachment'} = $attachment;
-  $vars->{'bugsummary'} = $bugsummary; 
   $vars->{'attachments'} = $bugattachments;
 
   print $cgi->header();
diff --git a/template/en/default/attachment/edit.html.tmpl b/template/en/default/attachment/edit.html.tmpl
index 49c28aa64..8d0422065 100644
--- a/template/en/default/attachment/edit.html.tmpl
+++ b/template/en/default/attachment/edit.html.tmpl
@@ -29,7 +29,7 @@
   Attachment [% attachment.id %] Details for
   [%+ "$terms.Bug ${attachment.bug_id}" FILTER bug_link(attachment.bug_id) FILTER none %]
 [% END %]
-[% subheader = BLOCK %][% bugsummary FILTER html %][% END %]
+[% subheader = BLOCK %][% attachment.bug.short_desc FILTER html %][% END %]
 
 [% PROCESS global/header.html.tmpl
   title = title
@@ -250,9 +250,11 @@
           <br>
         </small>
 
-        [% IF flag_types.size > 0 %]
+        [% IF attachment.flag_types.size > 0 %]
           [% PROCESS "flag/list.html.tmpl" bug_id = attachment.bug_id
-                                           attach_id = attachment.id %]<br>
+                                           attach_id = attachment.id
+                                           flag_types = attachment.flag_types
+          %]<br>
         [% END %]
 
         <div id="smallCommentFrame">