From a3cbf5618bb6133f8cd56d8b1d1414fe4c87e469 Mon Sep 17 00:00:00 2001 From: "myk%mozilla.org" <> Date: Thu, 1 Sep 2005 21:02:26 +0000 Subject: Partial fix for bug 302669: rewrites Attachment.pm to provide a real Attachment class; r=lpsolit --- Bugzilla/Attachment.pm | 432 ++++++++++++++++++++---- Bugzilla/Bug.pm | 4 +- Bugzilla/Flag.pm | 51 +-- template/en/default/attachment/list.html.tmpl | 12 +- template/en/default/bug/show.xml.tmpl | 4 +- template/en/default/filterexceptions.pl | 4 +- template/en/default/global/user-error.html.tmpl | 11 +- 7 files changed, 407 insertions(+), 111 deletions(-) diff --git a/Bugzilla/Attachment.pm b/Bugzilla/Attachment.pm index 558d7f8bc..578f67b1f 100644 --- a/Bugzilla/Attachment.pm +++ b/Bugzilla/Attachment.pm @@ -20,96 +20,384 @@ # Contributor(s): Terry Weissman # Myk Melez -############################################################################ -# Module Initialization -############################################################################ - use strict; package Bugzilla::Attachment; -# This module requires that its caller have said "require globals.pl" to import -# relevant functions from that script. +=head1 NAME + +Bugzilla::Attachment - a file related to a bug that a user has uploaded + to the Bugzilla server + +=head1 SYNOPSIS + + use Bugzilla::Attachment; + + # Get the attachment with the given ID. + my $attachment = Bugzilla::Attachment->get($attach_id); + + # Get the attachments with the given IDs. + my $attachments = Bugzilla::Attachment->get_list($attach_ids); + +=head1 DESCRIPTION + +This module defines attachment objects, which represent files related to bugs +that users upload to the Bugzilla server. + +=cut + +# This module requires that its caller have said "require globals.pl" +# to import relevant functions from that script. -# Use the Flag module to handle flags. use Bugzilla::Flag; use Bugzilla::Config qw(:locations); use Bugzilla::User; -############################################################################ -# Functions -############################################################################ - -sub new { - # Returns a hash of information about the attachment with the given ID. +sub get { + my $invocant = shift; + my $id = shift; - my ($invocant, $id) = @_; - return undef if !$id; - my $self = { 'id' => $id }; - my $class = ref($invocant) || $invocant; - bless($self, $class); - - &::PushGlobalSQLState(); - &::SendSQL("SELECT 1, description, bug_id, isprivate FROM attachments " . - "WHERE attach_id = $id"); - ($self->{'exists'}, - $self->{'summary'}, - $self->{'bug_id'}, - $self->{'isprivate'}) = &::FetchSQLData(); - &::PopGlobalSQLState(); + my $attachments = _retrieve([$id]); + my $self = $attachments->[0]; + bless($self, ref($invocant) || $invocant) if $self; return $self; } -sub query -{ - # Retrieves and returns an array of attachment records for a given bug. - # This data should be given to attachment/list.html.tmpl in an - # "attachments" variable. - my ($bugid) = @_; - - my $dbh = Bugzilla->dbh; - - # Retrieve a list of attachments for this bug and write them into an array - # of hashes in which each hash represents a single attachment. - my $list = $dbh->selectall_arrayref("SELECT attach_id, " . - $dbh->sql_date_format('creation_ts', '%Y.%m.%d %H:%i') . - ", mimetype, description, ispatch, - isobsolete, isprivate, LENGTH(thedata), - submitter_id - FROM attachments - INNER JOIN attach_data - ON id = attach_id - WHERE bug_id = ? ORDER BY attach_id", - undef, $bugid); - - my @attachments = (); - foreach my $row (@$list) { - my %a; - ($a{'attachid'}, $a{'date'}, $a{'contenttype'}, - $a{'description'}, $a{'ispatch'}, $a{'isobsolete'}, - $a{'isprivate'}, $a{'datasize'}, $a{'submitter_id'}) = @$row; - - $a{'submitter'} = new Bugzilla::User($a{'submitter_id'}); - - # Retrieve a list of flags for this attachment. - $a{'flags'} = Bugzilla::Flag::match({ 'attach_id' => $a{'attachid'}, - 'is_active' => 1 }); - - # A zero size indicates that the attachment is stored locally. - if ($a{'datasize'} == 0) { - my $attachid = $a{'attachid'}; - my $hash = ($attachid % 100) + 100; - $hash =~ s/.*(\d\d)$/group.$1/; - if (open(AH, "$attachdir/$hash/attachment.$attachid")) { - $a{'datasize'} = (stat(AH))[7]; +sub get_list { + my $invocant = shift; + my $ids = shift; + + my $attachments = _retrieve($ids); + foreach my $attachment (@$attachments) { + bless($attachment, ref($invocant) || $invocant); + } + + return $attachments; +} + +sub _retrieve { + my ($ids) = @_; + + return [] if scalar(@$ids) == 0; + + my @columns = ( + 'attachments.attach_id AS id', + 'attachments.bug_id AS bug_id', + 'attachments.description AS description', + 'attachments.mimetype AS contenttype', + 'attachments.submitter_id AS _attacher_id', + Bugzilla->dbh->sql_date_format('attachments.creation_ts', + '%Y.%m.%d %H:%i') . " AS attached", + 'attachments.filename AS filename', + 'attachments.ispatch AS ispatch', + 'attachments.isobsolete AS isobsolete', + 'attachments.isprivate AS isprivate' + ); + my $columns = join(", ", @columns); + + my $records = Bugzilla->dbh->selectall_arrayref("SELECT $columns + FROM attachments + WHERE attach_id IN (" . + join(",", @$ids) . ")", + { Slice => {} }); + return $records; +} + +=pod + +=head2 Instance Properties + +=over + +=item C + +the unique identifier for the attachment + +=back + +=cut + +sub id { + my $self = shift; + return $self->{id}; +} + +=over + +=item C + +the ID of the bug to which the attachment is attached + +=back + +=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}; +} + +=over + +=item C + +user-provided text describing the attachment + +=back + +=cut + +sub description { + my $self = shift; + return $self->{description}; +} + +=over + +=item C + +the attachment's MIME media type + +=back + +=cut + +sub contenttype { + my $self = shift; + return $self->{contenttype}; +} + +=over + +=item C + +the user who attached the attachment + +=back + +=cut + +sub attacher { + my $self = shift; + return $self->{attacher} if exists $self->{attacher}; + $self->{attacher} = new Bugzilla::User($self->{_attacher_id}); + return $self->{attacher}; +} + +=over + +=item C + +the date and time on which the attacher attached the attachment + +=back + +=cut + +sub attached { + my $self = shift; + return $self->{attached}; +} + +=over + +=item C + +the name of the file the attacher attached + +=back + +=cut + +sub filename { + my $self = shift; + return $self->{filename}; +} + +=over + +=item C + +whether or not the attachment is a patch + +=back + +=cut + +sub ispatch { + my $self = shift; + return $self->{ispatch}; +} + +=over + +=item C + +whether or not the attachment is obsolete + +=back + +=cut + +sub isobsolete { + my $self = shift; + return $self->{isobsolete}; +} + +=over + +=item C + +whether or not the attachment is private + +=back + +=cut + +sub isprivate { + my $self = shift; + return $self->{isprivate}; +} + +=over + +=item C + +the content of the attachment + +=back + +=cut + +sub data { + my $self = shift; + return $self->{data} if exists $self->{data}; + + # First try to get the attachment data from the database. + ($self->{data}) = Bugzilla->dbh->selectrow_array("SELECT thedata + FROM attach_data + WHERE id = ?", + undef, + $self->{id}); + + # If there's no attachment data in the database, the attachment is stored + # in a local file, so retrieve it from there. + if (length($self->{data}) == 0) { + if (open(AH, $self->_get_local_filename())) { + binmode AH; + $self->{data} = ; close(AH); } } - push @attachments, \%a; - } + + return $self->{data}; +} + +=over + +=item C + +the length (in characters) of the attachment content + +=back + +=cut + +# datasize is a property of the data itself, and it's unclear whether we should +# expose it at all, since you can easily derive it from the data itself: in TT, +# attachment.data.size; in Perl, length($attachment->{data}). But perhaps +# it makes sense for performance reasons, since accessing the data forces it +# to get retrieved from the database/filesystem and loaded into memory, +# while datasize avoids loading the attachment into memory, calling SQL's +# LENGTH() function or stat()ing the file instead. I've left it in for now. + +sub datasize { + my $self = shift; + return $self->{datasize} if exists $self->{datasize}; + + # If we have already retrieved the data, return its size. + return length($self->{data}) if exists $self->{data}; + + ($self->{datasize}) = + Bugzilla->dbh->selectrow_array("SELECT LENGTH(thedata) + FROM attach_data + WHERE id = ?", + undef, + $self->{id}); + + # If there's no attachment data in the database, the attachment + # is stored in a local file, so retrieve its size from the file. + if ($self->{datasize} == 0) { + if (open(AH, $self->_get_local_filename())) { + binmode AH; + $self->{datasize} = (stat(AH))[7]; + close(AH); + } + } + + return $self->{datasize}; +} + +=over + +=item C + +flags that have been set on the attachment + +=back + +=cut + +sub flags { + my $self = shift; + return $self->{flags} if exists $self->{flags}; + + $self->{flags} = Bugzilla::Flag::match({ attach_id => $self->id, + is_active => 1 }); + return $self->{flags}; +} + +# Instance methods; no POD documentation here yet because the only one so far +# is private. + +sub _get_local_filename { + my $self = shift; + my $hash = ($self->id % 100) + 100; + $hash =~ s/.*(\d\d)$/group.$1/; + return "$attachdir/$hash/attachment.$self->id"; +} + +=pod + +=head2 Class Methods + +=over + +=item C + +Description: retrieves and returns the attachments for the given bug. + +Params: C<$bug_id> - integer - the ID of the bug for which + to retrieve and return attachments. + +Returns: a reference to an array of attachment objects. + +=back + +=cut - return \@attachments; +sub get_attachments_by_bug { + my ($class, $bug_id) = @_; + my $attach_ids = Bugzilla->dbh->selectcol_arrayref("SELECT attach_id + FROM attachments + WHERE bug_id = ? + ORDER BY attach_id", + undef, $bug_id); + my $attachments = Bugzilla::Attachment->get_list($attach_ids); + return $attachments; } 1; diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index c287d0c0c..dfa419316 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -354,7 +354,9 @@ sub attachments { my ($self) = @_; return $self->{'attachments'} if exists $self->{'attachments'}; return [] if $self->{'error'}; - $self->{'attachments'} = Bugzilla::Attachment::query($self->{bug_id}); + + $self->{'attachments'} = + Bugzilla::Attachment->get_attachments_by_bug($self->bug_id); return $self->{'attachments'}; } diff --git a/Bugzilla/Flag.pm b/Bugzilla/Flag.pm index 3e72c5933..89dda08a5 100644 --- a/Bugzilla/Flag.pm +++ b/Bugzilla/Flag.pm @@ -347,26 +347,26 @@ sub validate { # can_see_bug() will refer to old settings. if (!$requestee->can_see_bug($bug_id)) { ThrowUserError("flag_requestee_unauthorized", - { flag_type => $flag->{'type'}, - requestee => $requestee, - bug_id => $bug_id, - attach_id => - $flag->{target}->{attachment}->{id} }); + { flag_type => $flag->{'type'}, + requestee => $requestee, + bug_id => $bug_id, + attachment => $flag->{target}->{attachment} + }); } # Throw an error if the target is a private attachment and # the requestee isn't in the group of insiders who can see it. - if ($flag->{target}->{attachment}->{exists} + if ($flag->{target}->{attachment} && $cgi->param('isprivate') && Param("insidergroup") && !$requestee->in_group(Param("insidergroup"))) { ThrowUserError("flag_requestee_unauthorized_attachment", - { flag_type => $flag->{'type'}, - requestee => $requestee, - bug_id => $bug_id, - attach_id => - $flag->{target}->{attachment}->{id} }); + { flag_type => $flag->{'type'}, + requestee => $requestee, + bug_id => $bug_id, + attachment => $flag->{target}->{attachment} + }); } } } @@ -532,7 +532,9 @@ sub create { $flag->{'id'} = (&::FetchOneColumn() || 0) + 1; # Insert a record for the flag into the flags table. - my $attach_id = $flag->{'target'}->{'attachment'}->{'id'} || "NULL"; + 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, @@ -807,7 +809,8 @@ sub FormToNewFlags { { 'type_id' => $type_id, 'target_type' => $target->{'type'}, 'bug_id' => $target->{'bug'}->{'id'}, - 'attach_id' => $target->{'attachment'}->{'id'}, + 'attach_id' => $target->{'attachment'} ? + $target->{'attachment'}->{'id'} : undef, 'is_active' => 1 }); # Do not create a new flag of this type if this flag type is @@ -902,15 +905,18 @@ sub get_target { my $target = { 'exists' => 0 }; if ($attach_id) { - $target->{'attachment'} = new Bugzilla::Attachment($attach_id); + $target->{'attachment'} = Bugzilla::Attachment->get($attach_id); if ($bug_id) { # Make sure the bug and attachment IDs correspond to each other # (i.e. this is the bug to which this attachment is attached). - $bug_id == $target->{'attachment'}->{'bug_id'} - || return { 'exists' => 0 }; + if (!$target->{'attachment'} + || $target->{'attachment'}->{'bug_id'} != $bug_id) + { + return { 'exists' => 0 }; + } } - $target->{'bug'} = GetBug($target->{'attachment'}->{'bug_id'}); - $target->{'exists'} = $target->{'attachment'}->{'exists'}; + $target->{'bug'} = GetBug($bug_id); + $target->{'exists'} = 1; $target->{'type'} = "attachment"; } elsif ($bug_id) { @@ -937,20 +943,21 @@ Sends an email notification about a flag being created or fulfilled. sub notify { my ($flag, $template_file) = @_; + my $attachment_is_private = $flag->{'target'}->{'attachment'} ? + $flag->{'target'}->{'attachment'}->{'isprivate'} : undef; + # If the target bug is restricted to one or more groups, then we need # to make sure we don't send email about it to unauthorized users # on the request type's CC: list, so we have to trawl the list for users # not in those groups or email addresses that don't have an account. - if ($flag->{'target'}->{'bug'}->{'restricted'} - || $flag->{'target'}->{'attachment'}->{'isprivate'}) - { + if ($flag->{'target'}->{'bug'}->{'restricted'} || $attachment_is_private) { my @new_cc_list; foreach my $cc (split(/[, ]+/, $flag->{'type'}->{'cc_list'})) { my $ccuser = Bugzilla::User->new_from_login($cc) || next; next if $flag->{'target'}->{'bug'}->{'restricted'} && !$ccuser->can_see_bug($flag->{'target'}->{'bug'}->{'id'}); - next if $flag->{'target'}->{'attachment'}->{'isprivate'} + next if $attachment_is_private && Param("insidergroup") && !$ccuser->in_group(Param("insidergroup")); push(@new_cc_list, $cc); diff --git a/template/en/default/attachment/list.html.tmpl b/template/en/default/attachment/list.html.tmpl index 4f66a5eb8..61b68ee53 100644 --- a/template/en/default/attachment/list.html.tmpl +++ b/template/en/default/attachment/list.html.tmpl @@ -37,7 +37,7 @@ [% IF !attachment.isprivate || canseeprivate %] - [% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %] + [% attachment.description FILTER html FILTER obsolete(attachment.isobsolete) %] @@ -49,11 +49,11 @@ - - [% attachment.submitter.name || attachment.submitter.login FILTER html %] + + [% attachment.attacher.name || attachment.attacher.login FILTER html %] - [% attachment.date FILTER time %] + [% attachment.attached FILTER time %] [% attachment.datasize FILTER unitconvert %] [% IF show_attachment_flags %] @@ -75,9 +75,9 @@ [% END %] - Edit + Edit [% IF attachment.ispatch && patchviewerinstalled %] - | Diff + | Diff [% END %] [% Hook.process("action") %] diff --git a/template/en/default/bug/show.xml.tmpl b/template/en/default/bug/show.xml.tmpl index 222204936..2555600ed 100644 --- a/template/en/default/bug/show.xml.tmpl +++ b/template/en/default/bug/show.xml.tmpl @@ -73,8 +73,8 @@ ispatch="1" [% END %] > - [% a.attachid %] - [% a.date FILTER time FILTER xml %] + [% a.id %] + [% a.attached FILTER time FILTER xml %] [% a.description FILTER xml %] [% a.contenttype FILTER xml %] [% FOREACH flag = a.flags %] diff --git a/template/en/default/filterexceptions.pl b/template/en/default/filterexceptions.pl index cc5a19d09..8ed71f008 100644 --- a/template/en/default/filterexceptions.pl +++ b/template/en/default/filterexceptions.pl @@ -368,7 +368,7 @@ 'bug/show.xml.tmpl' => [ 'VERSION', - 'a.attachid', + 'a.id', 'field', ], @@ -458,7 +458,7 @@ ], 'attachment/list.html.tmpl' => [ - 'attachment.attachid', + 'attachment.id', 'flag.status', 'bugid', ], diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index 3e1a2a3ef..53fdcc59f 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -442,7 +442,7 @@ You asked [% requestee.identity FILTER html %] for [% flag_type.name FILTER html %] on [% terms.bug %] [% bug_id FILTER html -%] - [% IF attach_id %], attachment [% attach_id FILTER html %][% END %], + [% IF attachment %], attachment [% attachment.id FILTER html %][% END %], but that [% terms.bug %] has been restricted to users in certain groups, and the user you asked isn't in all the groups to which the [% terms.bug %] has been restricted. @@ -455,11 +455,10 @@ You asked [% requestee.identity FILTER html %] for [% flag_type.name FILTER html %] on [%+ terms.bug %] [%+ bug_id FILTER html %], - attachment [% attach_id FILTER html %], but that attachment is restricted - to users - in the [% Param("insidergroup") FILTER html %] group, and the user - you asked isn't in that group. Please choose someone else to ask, - or ask an administrator to add the user to the group. + attachment [% attachment.id FILTER html %], but that attachment + is restricted to users in the [% Param("insidergroup") FILTER html %] group, + and the user you asked isn't in that group. Please choose someone else + to ask, or ask an administrator to add the user to the group. [% ELSIF error == "flag_type_cc_list_invalid" %] [% title = "Flag Type CC List Invalid" %] -- cgit v1.2.3-24-g4f1b