Bug 5179: Need to be able to put attachment on new bug - Patch by Marc Schumann…

Bugzilla/Attachment.pm

@@ -19,6 +19,7 @@
 #
 # Contributor(s): Terry Weissman <terry@mozilla.org>
 #                 Myk Melez <myk@mozilla.org>
+#                 Marc Schumann <wurblzap@gmail.com>
 
 use strict;
 
@@ -49,9 +50,11 @@ that users upload to the Bugzilla server.
 # This module requires that its caller have said "require globals.pl"
 # to import relevant functions from that script.
 
+use Bugzilla::Error;
 use Bugzilla::Flag;
-use Bugzilla::Config qw(:locations);
+use Bugzilla::Config qw(:locations Param);
 use Bugzilla::User;
+use Bugzilla::Util qw(trick_taint);
 
 sub get {
     my $invocant = shift;
@@ -310,7 +313,7 @@ sub data {
             close(AH);
         }
     }
-    
+
     return $self->{data};
 }
 
@@ -377,8 +380,8 @@ sub flags {
     return $self->{flags};
 }
 
-# Instance methods; no POD documentation here yet because the only one so far
-# is private.
+# Instance methods; no POD documentation here yet because the only ones so far
+# are private.
 
 sub _get_local_filename {
     my $self = shift;
@@ -387,6 +390,84 @@ sub _get_local_filename {
     return "$attachdir/$hash/attachment." . $self->id;
 }
 
+sub _validate_filename {
+    my ($throw_error) = @_;
+    my $cgi = Bugzilla->cgi;
+    defined $cgi->upload('data')
+        || ($throw_error ? ThrowUserError("file_not_specified") : return 0);
+
+    my $filename = $cgi->upload('data');
+
+    # Remove path info (if any) from the file name.  The browser should do this
+    # for us, but some are buggy.  This may not work on Mac file names and could
+    # mess up file names with slashes in them, but them's the breaks.  We only
+    # use this as a hint to users downloading attachments anyway, so it's not
+    # a big deal if it munges incorrectly occasionally.
+    $filename =~ s/^.*[\/\\]//;
+
+    # Truncate the filename to 100 characters, counting from the end of the
+    # string to make sure we keep the filename extension.
+    $filename = substr($filename, -100, 100);
+
+    return $filename;
+}
+
+sub _validate_data {
+    my ($throw_error, $hr_vars) = @_;
+    my $cgi = Bugzilla->cgi;
+    my $maxsize = $cgi->param('ispatch') ? Param('maxpatchsize') : Param('maxattachmentsize');
+    $maxsize *= 1024; # Convert from K
+    my $fh;
+    # Skip uploading into a local variable if the user wants to upload huge
+    # attachments into local files.
+    if (!$cgi->param('bigfile')) {
+        $fh = $cgi->upload('data');
+    }
+    my $data;
+
+    # We could get away with reading only as much as required, except that then
+    # we wouldn't have a size to print to the error handler below.
+    if (!$cgi->param('bigfile')) {
+        # enable 'slurp' mode
+        local $/;
+        $data = <$fh>;
+    }
+
+    $data
+        || ($cgi->param('bigfile'))
+        || ($throw_error ? ThrowUserError("zero_length_file") : return 0);
+
+    # Windows screenshots are usually uncompressed BMP files which
+    # makes for a quick way to eat up disk space. Let's compress them.
+    # We do this before we check the size since the uncompressed version
+    # could easily be greater than maxattachmentsize.
+    if (Param('convert_uncompressed_images')
+        && $cgi->param('contenttype') eq 'image/bmp') {
+        require Image::Magick;
+        my $img = Image::Magick->new(magick=>'bmp');
+        $img->BlobToImage($data);
+        $img->set(magick=>'png');
+        my $imgdata = $img->ImageToBlob();
+        $data = $imgdata;
+        $cgi->param('contenttype', 'image/png');
+        $$hr_vars->{'convertedbmp'} = 1;
+    }
+
+    # Make sure the attachment does not exceed the maximum permitted size
+    my $len = $data ? length($data) : 0;
+    if ($maxsize && $len > $maxsize) {
+        my $vars = { filesize => sprintf("%.0f", $len/1024) };
+        if ($cgi->param('ispatch')) {
+            $throw_error ? ThrowUserError("patch_too_large", $vars) : return 0;
+        }
+        else {
+            $throw_error ? ThrowUserError("file_too_large", $vars) : return 0;
+        }
+    }
+
+    return $data || '';
+}
+
 =pod
 
 =head2 Class Methods
@@ -402,8 +483,6 @@ Params:     C<$bug_id> - integer - the ID of the bug for which
 
 Returns:    a reference to an array of attachment objects.
 
-=back
-
 =cut
 
 sub get_attachments_by_bug {
@@ -416,4 +495,237 @@ sub get_attachments_by_bug {
     return $attachments;
 }
 
+=pod
+
+=item C<validate_is_patch()>
+
+Description: validates the "patch" flag passed in by CGI.
+
+Returns:    1 on success.
+
+=cut
+
+sub validate_is_patch {
+    my ($class, $throw_error) = @_;
+    my $cgi = Bugzilla->cgi;
+
+    # Set the ispatch flag to zero if it is undefined, since the UI uses
+    # an HTML checkbox to represent this flag, and unchecked HTML checkboxes
+    # do not get sent in HTML requests.
+    $cgi->param('ispatch', $cgi->param('ispatch') ? 1 : 0);
+
+    # Set the content type to text/plain if the attachment is a patch.
+    $cgi->param('contenttype', 'text/plain') if $cgi->param('ispatch');
+
+    return 1;
+}
+
+=pod
+
+=item C<validate_description()>
+
+Description: validates the description passed in by CGI.
+
+Returns:    1 on success.
+
+=cut
+
+sub validate_description {
+    my ($class, $throw_error) = @_;
+    my $cgi = Bugzilla->cgi;
+
+    $cgi->param('description')
+        || ($throw_error ? ThrowUserError("missing_attachment_description") : return 0);
+
+    return 1;
+}
+
+=pod
+
+=item C<validate_content_type()>
+
+Description: validates the content type passed in by CGI.
+
+Returns:    1 on success.
+
+=cut
+
+sub validate_content_type {
+    my ($class, $throw_error) = @_;
+    my $cgi = Bugzilla->cgi;
+
+    if (!defined $cgi->param('contenttypemethod')) {
+        $throw_error ? ThrowUserError("missing_content_type_method") : return 0;
+    }
+    elsif ($cgi->param('contenttypemethod') eq 'autodetect') {
+        my $contenttype =
+            $cgi->uploadInfo($cgi->param('data'))->{'Content-Type'};
+        # The user asked us to auto-detect the content type, so use the type
+        # specified in the HTTP request headers.
+        if ( !$contenttype ) {
+            $throw_error ? ThrowUserError("missing_content_type") : return 0;
+        }
+        $cgi->param('contenttype', $contenttype);
+    }
+    elsif ($cgi->param('contenttypemethod') eq 'list') {
+        # The user selected a content type from the list, so use their
+        # selection.
+        $cgi->param('contenttype', $cgi->param('contenttypeselection'));
+    }
+    elsif ($cgi->param('contenttypemethod') eq 'manual') {
+        # The user entered a content type manually, so use their entry.
+        $cgi->param('contenttype', $cgi->param('contenttypeentry'));
+    }
+    else {
+        $throw_error ?
+            ThrowCodeError("illegal_content_type_method",
+                           { contenttypemethod => $cgi->param('contenttypemethod') }) :
+            return 0;
+    }
+
+    if ( $cgi->param('contenttype') !~
+           /^(application|audio|image|message|model|multipart|text|video)\/.+$/ ) {
+        $throw_error ?
+            ThrowUserError("invalid_content_type",
+                           { contenttype => $cgi->param('contenttype') }) :
+            return 0;
+    }
+
+    return 1;
+}
+
+=pod
+
+=item C<insert_attachment_for_bug($throw_error, $bug_id, $user, $timestamp, $hr_vars)>
+
+Description: inserts an attachment from CGI input for the given bug.
+
+Params:     C<$bug_id> - integer - the ID of the bug for which
+            to insert the attachment.
+            C<$user> - Bugzilla::User object - the user we're inserting an
+            attachment for.
+            C<$timestamp> - scalar - timestamp of the insert as returned
+            by SELECT NOW().
+            C<$hr_vars> - hash reference - reference to a hash of template
+            variables.
+
+Returns:    the ID of the new attachment.
+
+=back
+
+=cut
+
+sub insert_attachment_for_bug {
+    my ($class, $throw_error, $bug_id, $user, $timestamp, $hr_vars) = @_;
+
+    my $cgi = Bugzilla->cgi;
+    my $dbh = Bugzilla->dbh;
+    my $attachurl = $cgi->param('attachurl') || '';
+    my $data;
+    my $filename;
+    my $contenttype;
+    my $isurl;
+    $class->validate_is_patch($throw_error) || return 0;
+    $class->validate_description($throw_error) || return 0;
+
+    if (($attachurl =~ /^(http|https|ftp):\/\/\S+/)
+         && !(defined $cgi->upload('data'))) {
+        $filename = '';
+        $data = $attachurl;
+        $isurl = 1;
+        $contenttype = 'text/plain';
+        $cgi->param('ispatch', 0);
+        $cgi->delete('bigfile');
+    }
+    else {
+        $filename = _validate_filename($throw_error) || return 0;
+        # need to validate content type before data as
+        # we now check the content type for image/bmp in _validate_data()
+        unless ($cgi->param('ispatch')) {
+            $class->validate_content_type($throw_error) || return 0;
+        }
+        $data = _validate_data($hr_vars, $throw_error) || return 0;
+        $contenttype = $cgi->param('contenttype');
+
+        # These are inserted using placeholders so no need to panic
+        trick_taint($filename);
+        trick_taint($contenttype);
+        $isurl = 0;
+    }
+
+    # The order of these function calls is important, as both Flag::validate
+    # and FlagType::validate assume User::match_field has ensured that the
+    # values in the requestee fields are legitimate user email addresses.
+    my $match_status = Bugzilla::User::match_field($cgi, {
+        '^requestee(_type)?-(\d+)$' => { 'type' => 'multi' },
+    }, MATCH_SKIP_CONFIRM);
+
+    $$hr_vars->{'match_field'} = 'requestee';
+    if ($match_status == USER_MATCH_FAILED) {
+        $$hr_vars->{'message'} = 'user_match_failed';
+    }
+    elsif ($match_status == USER_MATCH_MULTIPLE) {
+        $$hr_vars->{'message'} = 'user_match_multiple';
+    }
+
+    # FlagType::validate() and Flag::validate() should not detect
+    # any reference to existing flags when creating a new attachment.
+    # Setting the third param to -1 will force this function to check this
+    # point.
+    # XXX needs $throw_error treatment
+    Bugzilla::Flag::validate($cgi, $bug_id, -1);
+    Bugzilla::FlagType::validate($cgi, $bug_id, -1);
+
+    # Escape characters in strings that will be used in SQL statements.
+    my $description = $cgi->param('description');
+    trick_taint($description);
+    my $isprivate = $cgi->param('isprivate') ? 1 : 0;
+
+    # Insert the attachment into the database.
+    my $sth = $dbh->do(
+        "INSERT INTO attachments
+            (bug_id, creation_ts, filename, description,
+             mimetype, ispatch, isurl, isprivate, submitter_id)
+         VALUES (?,?,?,?,?,?,?,?,?)", undef, ($bug_id, $timestamp, $filename,
+              $description, $contenttype, $cgi->param('ispatch'),
+              $isurl, $isprivate, $user->id));
+    # Retrieve the ID of the newly created attachment record.
+    my $attachid = $dbh->bz_last_key('attachments', 'attach_id');
+
+    # We only use $data here in this INSERT with a placeholder,
+    # so it's safe.
+    $sth = $dbh->prepare("INSERT INTO attach_data
+                         (id, thedata) VALUES ($attachid, ?)");
+    trick_taint($data);
+    $sth->bind_param(1, $data, $dbh->BLOB_TYPE);
+    $sth->execute();
+
+    # If the file is to be stored locally, stream the file from the webserver
+    # to the local file without reading it into a local variable.
+    if ($cgi->param('bigfile')) {
+        my $fh = $cgi->upload('data');
+        my $hash = ($attachid % 100) + 100;
+        $hash =~ s/.*(\d\d)$/group.$1/;
+        mkdir "$attachdir/$hash", 0770;
+        chmod 0770, "$attachdir/$hash";
+        open(AH, ">$attachdir/$hash/attachment.$attachid");
+        binmode AH;
+        my $sizecount = 0;
+        my $limit = (Param("maxlocalattachment") * 1048576);
+        while (<$fh>) {
+            print AH $_;
+            $sizecount += length($_);
+            if ($sizecount > $limit) {
+                close AH;
+                close $fh;
+                unlink "$attachdir/$hash/attachment.$attachid";
+                $throw_error ? ThrowUserError("local_file_too_large") : return 0;
+            }
+        }
+        close AH;
+        close $fh;
+    }
+    return $attachid;
+}
+
 1;

attachment.cgi

@@ -26,6 +26,7 @@
 #                 Max Kanat-Alexander <mkanat@bugzilla.org>
 #                 Greg Hendricks <ghendricks@novell.com>
 #                 Frédéric Buclin <LpSolit@gmail.com>
+#                 Marc Schumann <wurblzap@gmail.com>
 
 ################################################################################
 # Script Initialization
@@ -251,64 +252,6 @@ sub validateCanChangeBug
                         { bug_id => $bugid });
 }
 
-sub validateDescription
-{
-    $cgi->param('description')
-      || ThrowUserError("missing_attachment_description");
-}
-
-sub validateIsPatch
-{
-    # Set the ispatch flag to zero if it is undefined, since the UI uses
-    # an HTML checkbox to represent this flag, and unchecked HTML checkboxes
-    # do not get sent in HTML requests.
-    $cgi->param('ispatch', $cgi->param('ispatch') ? 1 : 0);
-
-    # Set the content type to text/plain if the attachment is a patch.
-    $cgi->param('contenttype', 'text/plain') if $cgi->param('ispatch');
-}
-
-sub validateContentType
-{
-  if (!defined $cgi->param('contenttypemethod'))
-  {
-    ThrowUserError("missing_content_type_method");
-  }
-  elsif ($cgi->param('contenttypemethod') eq 'autodetect')
-  {
-    my $contenttype = $cgi->uploadInfo($cgi->param('data'))->{'Content-Type'};
-    # The user asked us to auto-detect the content type, so use the type
-    # specified in the HTTP request headers.
-    if ( !$contenttype )
-    {
-      ThrowUserError("missing_content_type");
-    }
-    $cgi->param('contenttype', $contenttype);
-  }
-  elsif ($cgi->param('contenttypemethod') eq 'list')
-  {
-    # The user selected a content type from the list, so use their selection.
-    $cgi->param('contenttype', $cgi->param('contenttypeselection'));
-  }
-  elsif ($cgi->param('contenttypemethod') eq 'manual')
-  {
-    # The user entered a content type manually, so use their entry.
-    $cgi->param('contenttype', $cgi->param('contenttypeentry'));
-  }
-  else
-  {
-    ThrowCodeError("illegal_content_type_method",
-                   { contenttypemethod => $cgi->param('contenttypemethod') });
-  }
-
-  if ( $cgi->param('contenttype') !~
-         /^(application|audio|image|message|model|multipart|text|video)\/.+$/ )
-  {
-    ThrowUserError("invalid_content_type",
-                   { contenttype => $cgi->param('contenttype') });
-  }
-}
-
 sub validateIsObsolete
 {
     # Set the isobsolete flag to zero if it is undefined, since the UI uses
@@ -325,82 +268,6 @@ sub validatePrivate
     $cgi->param('isprivate', $cgi->param('isprivate') ? 1 : 0);
 }
 
-sub validateData
-{
-  my $maxsize = $cgi->param('ispatch') ? Param('maxpatchsize') : Param('maxattachmentsize');
-  $maxsize *= 1024; # Convert from K
-  my $fh;
-  # Skip uploading into a local variable if the user wants to upload huge
-  # attachments into local files.
-  if (!$cgi->param('bigfile'))
-  {
-    $fh = $cgi->upload('data');
-  }
-  my $data;
-
-  # We could get away with reading only as much as required, except that then
-  # we wouldn't have a size to print to the error handler below.
-  if (!$cgi->param('bigfile'))
-  {
-      # enable 'slurp' mode
-      local $/;
-      $data = <$fh>;
-  }
-
-  $data
-    || ($cgi->param('bigfile'))
-    || ThrowUserError("zero_length_file");
-    
-    # Windows screenshots are usually uncompressed BMP files which
-    # makes for a quick way to eat up disk space. Let's compress them. 
-    # We do this before we check the size since the uncompressed version
-    # could easily be greater than maxattachmentsize.
-    if (Param('convert_uncompressed_images') && $cgi->param('contenttype') eq 'image/bmp'){
-      require Image::Magick; 
-      my $img = Image::Magick->new(magick=>'bmp');
-      $img->BlobToImage($data);
-      $img->set(magick=>'png');
-      my $imgdata = $img->ImageToBlob();
-      $data = $imgdata;
-      $cgi->param('contenttype', 'image/png');
-      $vars->{'convertedbmp'} = 1;
-    }
-    
-  # Make sure the attachment does not exceed the maximum permitted size
-  my $len = $data ? length($data) : 0;
-  if ($maxsize && $len > $maxsize) {
-      my $vars = { filesize => sprintf("%.0f", $len/1024) };
-      if ($cgi->param('ispatch')) {
-          ThrowUserError("patch_too_large", $vars);
-      } else {
-          ThrowUserError("file_too_large", $vars);
-      }
-  }
-
-  return $data || '';
-}
-
-sub validateFilename
-{
-  defined $cgi->upload('data')
-    || ThrowUserError("file_not_specified");
-
-  my $filename = $cgi->upload('data');
-  
-  # Remove path info (if any) from the file name.  The browser should do this
-  # for us, but some are buggy.  This may not work on Mac file names and could
-  # mess up file names with slashes in them, but them's the breaks.  We only
-  # use this as a hint to users downloading attachments anyway, so it's not 
-  # a big deal if it munges incorrectly occasionally.
-  $filename =~ s/^.*[\/\\]//;
-
-  # Truncate the filename to 100 characters, counting from the end of the string
-  # to make sure we keep the filename extension.
-  $filename = substr($filename, -100, 100);
-
-  return $filename;
-}
-
 sub validateObsolete
 {
   my @obsolete_ids = ();
@@ -501,7 +368,7 @@ sub view
     {
         $cgi->param('contenttypemethod', 'manual');
         $cgi->param('contenttypeentry', $cgi->param('content_type'));
-        validateContentType();
+        Bugzilla::Attachment->validate_content_type(THROW_ERROR);
         $contenttype = $cgi->param('content_type');
     }
 
@@ -915,130 +782,30 @@ sub enter
 # Insert a new attachment into the database.
 sub insert
 {
-    my $userid = Bugzilla->user->id;
+    my $dbh = Bugzilla->dbh;
+    my $user = Bugzilla->user;
 
     # Retrieve and validate parameters
     my $bugid = $cgi->param('bugid');
     ValidateBugID($bugid);
     validateCanChangeBug($bugid);
     ValidateComment(scalar $cgi->param('comment'));
-    my $attachurl = $cgi->param('attachurl') || '';
-    my $data;
-    my $filename;
-    my $contenttype;
-    my $isurl;
-    validateIsPatch();
-    validateDescription();
-    my $dbh = Bugzilla->dbh; 
-    
-    if (($attachurl =~ /^(http|https|ftp):\/\/\S+/) 
-         && !(defined $cgi->upload('data'))) {
-        $filename = '';
-        $data = $attachurl;
-        $isurl = 1;
-        $contenttype = 'text/plain';
-        $cgi->param('ispatch', 0);
-        $cgi->delete('bigfile');
-    } else {
-        $filename = validateFilename();
-        # need to validate content type before data as
-        # we now check the content type for image/bmp in validateData()
-        validateContentType() unless $cgi->param('ispatch');
-        $data = validateData();
-        $contenttype = $cgi->param('contenttype');
-
-        # These are inserted using placeholders so no need to panic
-        trick_taint($filename);
-        trick_taint($contenttype);
-        $isurl = 0;
-    }
+    my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); 
 
+    my $attachid =
+        Bugzilla::Attachment->insert_attachment_for_bug(THROW_ERROR,
+                                                        $bugid, $user,
+                                                        $timestamp, \$vars);
+    my $isprivate = $cgi->param('isprivate') ? 1 : 0;
     my @obsolete_ids = ();
     @obsolete_ids = validateObsolete() if $cgi->param('obsolete');
 
-    # The order of these function calls is important, as both Flag::validate
-    # and FlagType::validate assume User::match_field has ensured that the
-    # values in the requestee fields are legitimate user email addresses.
-    my $match_status = Bugzilla::User::match_field($cgi, {
-        '^requestee(_type)?-(\d+)$' => { 'type' => 'multi' },
-    }, MATCH_SKIP_CONFIRM);
-
-    $vars->{'match_field'} = 'requestee';
-    if ($match_status == USER_MATCH_FAILED) {
-        $vars->{'message'} = 'user_match_failed';
-    }
-    elsif ($match_status == USER_MATCH_MULTIPLE) {
-        $vars->{'message'} = 'user_match_multiple';
-    }
-
-    # FlagType::validate() and Flag::validate() should not detect
-    # any reference to existing flags when creating a new attachment.
-    # Setting the third param to -1 will force this function to check this point.
-    Bugzilla::Flag::validate($cgi, $bugid, -1);
-    Bugzilla::FlagType::validate($cgi, $bugid, -1);
-
-    # Escape characters in strings that will be used in SQL statements.
-    my $description = $cgi->param('description');
-    trick_taint($description);
-    my $isprivate = $cgi->param('isprivate') ? 1 : 0;
-
-  # Figure out when the changes were made.
-  my ($timestamp) = Bugzilla->dbh->selectrow_array("SELECT NOW()"); 
-  
-  # Insert the attachment into the database.
-  my $sth = $dbh->do(
-        "INSERT INTO attachments
-            (bug_id, creation_ts, filename, description,
-             mimetype, ispatch, isurl, isprivate, submitter_id) 
-         VALUES (?,?,?,?,?,?,?,?,?)", undef, ($bugid, $timestamp, $filename,
-              $description, $contenttype, $cgi->param('ispatch'),
-              $isurl, $isprivate, $userid));
-  # Retrieve the ID of the newly created attachment record.
-  my $attachid = $dbh->bz_last_key('attachments', 'attach_id');
-
-  # We only use $data here in this INSERT with a placeholder,
-  # so it's safe.
-  $sth = $dbh->prepare("INSERT INTO attach_data
-                           (id, thedata) VALUES ($attachid, ?)");
-  trick_taint($data);
-  $sth->bind_param(1, $data, $dbh->BLOB_TYPE);
-  $sth->execute();
-
-
-  # If the file is to be stored locally, stream the file from the webserver
-  # to the local file without reading it into a local variable.
-  if ($cgi->param('bigfile'))
-  {
-    my $fh = $cgi->upload('data');
-    my $hash = ($attachid % 100) + 100;
-    $hash =~ s/.*(\d\d)$/group.$1/;
-    mkdir "$attachdir/$hash", 0770;
-    chmod 0770, "$attachdir/$hash";
-    open(AH, ">$attachdir/$hash/attachment.$attachid");
-    binmode AH;
-    my $sizecount = 0;
-    my $limit = (Param("maxlocalattachment") * 1048576);
-    while (<$fh>) {
-        print AH $_;
-        $sizecount += length($_);
-        if ($sizecount > $limit) {
-            close AH;
-            close $fh;
-            unlink "$attachdir/$hash/attachment.$attachid";
-            ThrowUserError("local_file_too_large");
-        }
-    }
-    close AH;
-    close $fh;
-  }
-
-
   # Insert a comment about the new attachment into the database.
   my $comment = "Created an attachment (id=$attachid)\n" .
                 $cgi->param('description') . "\n";
   $comment .= ("\n" . $cgi->param('comment')) if defined $cgi->param('comment');
 
-  AppendComment($bugid, $userid, $comment, $isprivate, $timestamp);
+  AppendComment($bugid, $user->id, $comment, $isprivate, $timestamp);
 
   # Make existing attachments obsolete.
   my $fieldid = get_field_id('attachments.isobsolete');
@@ -1052,7 +819,7 @@ sub insert
       $dbh->do("INSERT INTO bugs_activity (bug_id, attach_id, who, bug_when,
                                           fieldid, removed, added) 
               VALUES (?,?,?,?,?,?,?)", undef, 
-              $bugid, $obsolete_id, $userid, $timestamp, $fieldid, 0, 1);
+              $bugid, $obsolete_id, $user->id, $timestamp, $fieldid, 0, 1);
   }
 
   # Assign the bug to the user, if they are allowed to take it
@@ -1071,7 +838,7 @@ sub insert
               "ON profiles.userid = bugs.assigned_to " .
               "WHERE bugs.bug_id = ?", undef, $bugid);
       
-      my @newvalues = ($userid, "ASSIGNED", "", 1, Bugzilla->user->login);
+      my @newvalues = ($user->id, "ASSIGNED", "", 1, $user->login);
       
       # Make sure the person we are taking the bug from gets mail.
       $owner = $oldvalues[4];  
@@ -1092,7 +859,7 @@ sub insert
       for (my $i = 0; $i < 4; $i++) {
           if ($oldvalues[$i] ne $newvalues[$i]) {
               LogActivityEntry($bugid, $fields[$i], $oldvalues[$i],
-                               $newvalues[$i], $userid, $timestamp);
+                               $newvalues[$i], $user->id, $timestamp);
           }
       }      
   }   
@@ -1101,11 +868,11 @@ sub insert
   Bugzilla::Flag::process($bugid, $attachid, $timestamp, $cgi);
    
   # Define the variables and functions that will be passed to the UI template.
-  $vars->{'mailrecipients'} =  { 'changer' => Bugzilla->user->login,
+  $vars->{'mailrecipients'} =  { 'changer' => $user->login,
                                  'owner'   => $owner };
   $vars->{'bugid'} = $bugid;
   $vars->{'attachid'} = $attachid;
-  $vars->{'description'} = $description;
+  $vars->{'description'} = $cgi->param('description');
   $vars->{'contenttypemethod'} = $cgi->param('contenttypemethod');
   $vars->{'contenttype'} = $cgi->param('contenttype');
 
@@ -1180,9 +947,9 @@ sub update
     my ($attach_id, $bugid) = validateID();
     validateCanEdit($attach_id);
     validateCanChangeAttachment($attach_id);
-    validateDescription();
-    validateIsPatch();
-    validateContentType() unless $cgi->param('ispatch');
+    Bugzilla::Attachment->validate_description(THROW_ERROR);
+    Bugzilla::Attachment->validate_is_patch(THROW_ERROR);
+    Bugzilla::Attachment->validate_content_type(THROW_ERROR) unless $cgi->param('ispatch');
     validateIsObsolete();
     validatePrivate();
     my $dbh = Bugzilla->dbh;

js/attachment.js

+/* The contents of this file are subject to the Mozilla Public
+ * License Version 1.1 (the "License"); you may not use this file
+ * except in compliance with the License. You may obtain a copy of
+ * the License at http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS
+ * IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+ * implied. See the License for the specific language governing
+ * rights and limitations under the License.
+ *
+ * The Original Code is the Bugzilla Bug Tracking System.
+ *
+ * The Initial Developer of the Original Code is Netscape Communications
+ * Corporation. Portions created by Netscape are
+ * Copyright (C) 1998 Netscape Communications Corporation. All
+ * Rights Reserved.
+ *
+ * Contributor(s): Myk Melez <myk@mozilla.org>
+ *                 Joel Peshkin <bugreport@peshkin.net>
+ *                 Erik Stambaugh <erik@dasbistro.com>
+ *                 Marc Schumann <wurblzap@gmail.com>
+ */
+
+function updateCommentPrivacy(checkbox) {
+    var text_elem = document.getElementById('comment');
+    if (checkbox.checked) {
+        text_elem.className='bz_private';
+    } else {
+        text_elem.className='';
+    }
+}
+
+function setContentTypeDisabledState(form)
+{
+    var isdisabled = false;
+    if (form.ispatch.checked)
+        isdisabled = true;
+
+    for (var i=0 ; i<form.contenttypemethod.length ; i++)
+        form.contenttypemethod[i].disabled = isdisabled;
+
+    form.contenttypeselection.disabled = isdisabled;
+    form.contenttypeentry.disabled = isdisabled;
+}
+
+function URLFieldHandler() {
+    var field_attachurl = document.getElementById("attachurl");
+    var greyfields = new Array("data", "ispatch", "autodetect",
+                               "list", "manual", "bigfile",
+                               "contenttypeselection",
+                               "contenttypeentry");
+    var i;
+    if (field_attachurl.value.match(/^\s*$/)) {
+        for (i = 0; i < greyfields.length; i++) {
+            thisfield = document.getElementById(greyfields[i]);
+            if (thisfield) {
+                thisfield.removeAttribute("disabled");
+            }
+        }
+    } else {
+        for (i = 0; i < greyfields.length; i++) {
+            thisfield = document.getElementById(greyfields[i]);
+            if (thisfield) {
+                thisfield.setAttribute("disabled", "disabled");
+            }
+        }
+    }
+}
+
+function DataFieldHandler() {
+    var field_data = document.getElementById("data");
+    var greyfields = new Array("attachurl");
+    if (field_data.value.match(/^\s*$/)) {
+        var i;
+        for (i = 0; i < greyfields.length; i++) {
+            thisfield = document.getElementById(greyfields[i]);
+            if (thisfield) {
+                thisfield.removeAttribute("disabled");
+            }
+        }
+    } else {
+        for (i = 0; i < greyfields.length; i++) {
+            thisfield = document.getElementById(greyfields[i]);
+            if (thisfield) {
+                thisfield.setAttribute("disabled", "disabled");
+            }
+        }
+    }
+}
+
+function clearAttachmentFields() {
+    var element;
+
+    document.getElementById('data').value = '';
+    DataFieldHandler();
+    if (element = document.getElementById('bigfile'))
+        element.checked = '';
+    if (element = document.getElementById('attachurl')) {
+        element.value = '';
+        URLFieldHandler();
+    }
+    document.getElementById('description').value = '';
+    document.getElementById('ispatch').checked = '';
+    if (element = document.getElementById('isprivate'))
+        element.checked = '';
+}

post_bug.cgi

@@ -22,12 +22,14 @@
 #                 Dan Mosedale <dmose@mozilla.org>
 #                 Joe Robins <jmrobins@tgix.com>
 #                 Gervase Markham <gerv@gerv.net>
+#                 Marc Schumann <wurblzap@gmail.com>
 
 use strict;
 use lib qw(.);
 
 require "globals.pl";
 use Bugzilla;
+use Bugzilla::Attachment;
 use Bugzilla::Constants;
 use Bugzilla::Util;
 use Bugzilla::Bug;
@@ -546,6 +548,21 @@ $dbh->do("UPDATE bugs SET creation_ts = ? WHERE bug_id = ?",
 
 $dbh->bz_unlock_tables();
 
+# Add an attachment if requested.
+if (defined($cgi->upload('data')) || $cgi->param('attachurl')) {
+    $cgi->param('isprivate', $cgi->param('commentprivacy'));
+    Bugzilla::Attachment->insert_attachment_for_bug(!THROW_ERROR,
+                                                    $id, $user, $timestamp,
+                                                    \$vars)
+        || ($vars->{'message'} = 'attachment_creation_failed');
+
+    # Determine if Patch Viewer is installed, for Diff link
+    eval {
+        require PatchReader;
+        $vars->{'patchviewerinstalled'} = 1;
+    };
+}
+
 # Email everyone the details of the new bug 
 $vars->{'mailrecipients'} = {'changer' => $user->login};
 

skins/standard/create_attachment.css

+ /* The contents of this file are subject to the Mozilla Public
+  * License Version 1.1 (the "License"); you may not use this file
+  * except in compliance with the License. You may obtain a copy of
+  * the License at http://www.mozilla.org/MPL/
+  *
+  * Software distributed under the License is distributed on an "AS
+  * IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+  * implied. See the License for the specific language governing
+  * rights and limitations under the License.
+  *
+  * The Original Code is the Bugzilla Bug Tracking System.
+  *
+  * Contributor(s): Myk Melez <myk@mozilla.org>
+  *                 Joel Peshkin <bugreport@peshkin.net>
+  *                 Erik Stambaugh <erik@dasbistro.com>
+  *                 Marc Schumann <wurblzap@gmail.com>
+  */
+
+table.attachment_entry th {
+    text-align: right;
+    vertical-align: baseline;
+    white-space: nowrap;
+}
+
+table.attachment_entry td {
+    text-align: left;
+    vertical-align: baseline;
+    padding-bottom: 5px;
+}
+
+table#flags th,
+table#flags td {
+    text-align: left;
+    vertical-align: baseline;
+    font-size: small;
+}

template/en/default/attachment/create.html.tmpl

@@ -19,6 +19,7 @@
   # Contributor(s): Myk Melez <myk@mozilla.org>
   #                 Joel Peshkin <bugreport@peshkin.net>
   #                 Erik Stambaugh <erik@dasbistro.com>
+  #                 Marc Schumann <wurblzap@gmail.com>
   #%]
 
 [% PROCESS global/variables.none.tmpl %]
@@ -33,180 +34,19 @@
   title = title
   h1 = h1
   h2 = h2
-  style = "
-    table.attachment_entry th {
-      text-align: right;
-      vertical-align: baseline;
-      white-space: nowrap;
-    }
-
-    table.attachment_entry td {
-      text-align: left;
-      vertical-align: baseline;
-      padding-bottom: 5px;
-    }
-    
-    table#flags th, table#flags td {
-      text-align: left;
-      vertical-align: baseline;
-      font-size: small;
-    }
-  "
-  onload="setContentTypeDisabledState();"
+  onload="setContentTypeDisabledState(document.entryform);"
+  style_urls = [ 'skins/standard/create_attachment.css' ]
+  javascript_urls = [ "js/attachment.js" ]
 %]
 
-[% IF Param("allow_attach_url") %]
-    <script type="text/javascript">
-
-        function URLFieldHandler() {
-            var field_attachurl = document.getElementById("attachurl");
-            var greyfields = new Array("data", "ispatch", "autodetect",
-                                       "list", "manual", "bigfile",
-                                       "contenttypeselection",
-                                       "contenttypeentry");
-            var i;
-            if (field_attachurl.value.match(/^\s*$/)) {
-                for (i = 0; i < greyfields.length; i++) {
-                    thisfield = document.getElementById(greyfields[i]);
-                    if (thisfield) {
-                        thisfield.removeAttribute("disabled");
-                    }
-                }
-            } else {
-                for (i = 0; i < greyfields.length; i++) {
-                    thisfield = document.getElementById(greyfields[i]);
-                    if (thisfield) {
-                        thisfield.setAttribute("disabled", "disabled");
-                    }
-                }
-            }
-        }
-
-        function DataFieldHandler() {
-            var field_data = document.getElementById("data");
-            var greyfields = new Array("attachurl");
-            if (field_data.value.match(/^\s*$/)) {
-                var i;
-                for (i = 0; i < greyfields.length; i++) {
-                    thisfield = document.getElementById(greyfields[i]);
-                    if (thisfield) {
-                        thisfield.removeAttribute("disabled");
-                    }
-                }
-            } else {
-                for (i = 0; i < greyfields.length; i++) {
-                    thisfield = document.getElementById(greyfields[i]);
-                    if (thisfield) {
-                        thisfield.setAttribute("disabled", "disabled");
-                    }
-                }
-            }
-        }
-
-    </script>
-[% END %]
-<script type="text/javascript">
-    function updateCommentPrivacy(checkbox) {
-        var text_elem = document.getElementById('comment');
-        if (checkbox.checked) {
-            text_elem.className='bz_private';
-        } else {
-            text_elem.className='';
-        }
-    }
-
-</script>
-
-
 <form name="entryform" method="post" action="attachment.cgi" enctype="multipart/form-data">
   <input type="hidden" name="bugid" value="[% bugid %]">
   <input type="hidden" name="action" value="insert">
 
   <table class="attachment_entry">
-    <tr>
-      <th><label for="data">File:</label></th>
-      <td>
-        <em>Enter the path to the file on your computer.</em><br>
-        <input type="file" id="data" name="data" size="50"
-          [% IF Param("allow_attach_url") %]
-            onchange="DataFieldHandler()" 
-          [% END %]
-        >
-      </td>
-    </tr>
-    [% IF Param("maxlocalattachment") %]
-    <tr>
-      <th>BigFile:</th>
-      <td>
-        <input type="checkbox" id="bigfile"
-               name="bigfile" value="bigfile">
-        <label for="bigfile">
-          Big File - Stored locally and may be purged
-        </label>
-      </td>
-    </tr>
-    [% END %]
-    [% IF Param("allow_attach_url") %]
-    <tr>
-      <th><label for="attachurl">AttachURL:</label></th>
-      <td>
-        <em>URL to be attached instead.</em><br>
-        <input type="text" id="attachurl" name="attachurl" size="60" 
-               maxlength="2000"
-               onkeyup="URLFieldHandler()" onblur="URLFieldHandler()">
-      </td>
-    </tr>
-    [% END %]
-    <tr>
-      <th><label for="description">Description:</label></th>
-      <td>
-        <em>Describe the attachment briefly.</em><br>
-        <input type="text" id="description" name="description" size="60" maxlength="200">
-      </td>
-    </tr>
-    <tr>
-      <th></th>
-      <td>
-      </td>
-    </tr>
-    <tr>
-      <th>Content Type:</th>
-      <td>
-        <em>If the attachment is a patch, check the box below.</em><br>
-        <input type="checkbox" id="ispatch" name="ispatch" value="1"
-               onchange="setContentTypeDisabledState();">
-        <label for="ispatch">patch</label><br><br>
+    [% PROCESS attachment/createformcontents.html.tmpl %]
 
-        <em>Otherwise, choose a method for determining the content type.</em><br>
-        <input type="radio" id="autodetect"
-               name="contenttypemethod" value="autodetect" checked="checked">
-          <label for="autodetect">auto-detect</label><br>
-        <input type="radio" id="list"
-               name="contenttypemethod" value="list">
-          <label for="list">select from list:</label>
-          <select name="contenttypeselection" id="contenttypeselection"
-                  onchange="this.form.contenttypemethod[1].checked = true;">
-            [% PROCESS "attachment/content-types.html.tmpl" %]
-          </select><br>
-        <input type="radio" id="manual"
-                     name="contenttypemethod" value="manual">
-          <label for="manual">enter manually:</label>
-          <input type="text" name="contenttypeentry" id="contenttypeentry"
-                 size="30" maxlength="200"
-                 onchange="if (this.value) this.form.contenttypemethod[2].checked = true;">
-      </td>
-    </tr>
-    [% IF (Param("insidergroup") && UserInGroup(Param("insidergroup"))) %]
-      <tr>
-        <th>Privacy:</th>
-        <td>
-          <em>If the attachment is private, check the box below.</em><br>
-          <input type="checkbox" name="isprivate" id="isprivate"
-          value="1" onClick="updateCommentPrivacy(this)">
-        <label for="isprivate">Private</label>
-        </td>
-      </tr>
-    [% END %]
+    [%# Additional fields for attachments on existing bugs: %]
     <tr>
       <th>Obsoletes:</th>
       <td>
@@ -238,14 +78,6 @@
       </tr>
     [% END %]
     <tr>
-      <td> </td>
-      <td>
-        [% IF flag_types.size > 0 %]
-          [% PROCESS "flag/list.html.tmpl" bug_id=bugid attach_id=attachid %]<br>
-        [% END %]
-      </td>
-    </tr>
-    <tr>
       <th><label for="comment">Comment:</label></th>
       <td>
         <em>(optional) Add a comment about this attachment to the [% terms.bug %].</em><br>
@@ -259,6 +91,17 @@
         %]
       </td>
     </tr>
+    [% IF (Param("insidergroup") && UserInGroup(Param("insidergroup"))) %]
+      <tr>
+        <th>Privacy:</th>
+        <td>
+          <em>If the attachment is private, check the box below.</em><br>
+          <input type="checkbox" name="isprivate" id="isprivate"
+          value="1" onClick="updateCommentPrivacy(this)">
+        <label for="isprivate">Private</label>
+        </td>
+      </tr>
+    [% END %]
     <tr>
       <th>&nbsp;</th>
       <td><input type="submit" value="Submit"></td>
@@ -267,23 +110,4 @@
 
 </form>
 
-<script type="text/javascript">
-  <!--
-  function setContentTypeDisabledState()
-  {
-    var entryform = document.entryform;
-
-    var isdisabled = false;
-    if (entryform.ispatch.checked)
-      isdisabled = true;
-
-    for (var i=0 ; i<entryform.contenttypemethod.length ; i++)
-      entryform.contenttypemethod[i].disabled = isdisabled;
-
-    entryform.contenttypeselection.disabled = isdisabled;
-    entryform.contenttypeentry.disabled = isdisabled;
-  }
-  //-->
-</script>
-
 [% PROCESS global/footer.html.tmpl %]

template/en/default/attachment/createformcontents.html.tmpl

+[%# 1.0@bugzilla.org %]
+[%# The contents of this file are subject to the Mozilla Public
+  # License Version 1.1 (the "License"); you may not use this file
+  # except in compliance with the License. You may obtain a copy of
+  # the License at http://www.mozilla.org/MPL/
+  #
+  # Software distributed under the License is distributed on an "AS
+  # IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
+  # implied. See the License for the specific language governing
+  # rights and limitations under the License.
+  #
+  # The Original Code is the Bugzilla Bug Tracking System.
+  #
+  # The Initial Developer of the Original Code is Netscape Communications
+  # Corporation. Portions created by Netscape are
+  # Copyright (C) 1998 Netscape Communications Corporation. All
+  # Rights Reserved.
+  #
+  # Contributor(s): Myk Melez <myk@mozilla.org>
+  #                 Joel Peshkin <bugreport@peshkin.net>
+  #                 Erik Stambaugh <erik@dasbistro.com>
+  #                 Marc Schumann <wurblzap@gmail.com>
+  #%]
+
+<tr>
+  <th><label for="data">File</label>:</th>
+  <td>
+    <em>Enter the path to the file on your computer.</em><br>
+    <input type="file" id="data" name="data" size="50"
+      [% IF Param("allow_attach_url") %]
+        onchange="DataFieldHandler()" 
+      [% END %]
+    >
+  </td>
+</tr>
+[% IF Param("maxlocalattachment") %]
+<tr>
+  <th>BigFile:</th>
+  <td>
+    <input type="checkbox" id="bigfile"
+           name="bigfile" value="bigfile">
+    <label for="bigfile">
+      Big File - Stored locally and may be purged
+    </label>
+  </td>
+</tr>
+[% END %]
+[% IF Param("allow_attach_url") %]
+<tr>
+  <th><label for="attachurl">AttachURL</label>:</th>
+  <td>
+    <em>URL to be attached instead.</em><br>
+    <input type="text" id="attachurl" name="attachurl" size="60" 
+           maxlength="2000"
+           onkeyup="URLFieldHandler()" onblur="URLFieldHandler()">
+  </td>
+</tr>
+[% END %]
+<tr>
+  <th><label for="description">Description</label>:</th>
+  <td>
+    <em>Describe the attachment briefly.</em><br>
+    <input type="text" id="description" name="description" size="60" maxlength="200">
+  </td>
+</tr>
+<tr>
+  <th>Content Type:</th>
+  <td>
+    <em>If the attachment is a patch, check the box below.</em><br>
+    <input type="checkbox" id="ispatch" name="ispatch" value="1"
+           onchange="setContentTypeDisabledState(this.form);">
+    <label for="ispatch">patch</label><br><br>
+
+    <em>Otherwise, choose a method for determining the content type.</em><br>
+    <input type="radio" id="autodetect"
+           name="contenttypemethod" value="autodetect" checked="checked">
+      <label for="autodetect">auto-detect</label><br>
+    <input type="radio" id="list"
+           name="contenttypemethod" value="list">
+      <label for="list">select from list</label>:
+      <select name="contenttypeselection" id="contenttypeselection"
+              onchange="this.form.contenttypemethod[1].checked = true;">
+        [% PROCESS "attachment/content-types.html.tmpl" %]
+      </select><br>
+    <input type="radio" id="manual"
+                 name="contenttypemethod" value="manual">
+      <label for="manual">enter manually</label>:
+      <input type="text" name="contenttypeentry" id="contenttypeentry"
+             size="30" maxlength="200"
+             onchange="if (this.value) this.form.contenttypemethod[2].checked = true;">
+  </td>
+</tr>
+<tr>
+  <td> </td>
+  <td>
+    [% IF flag_types && flag_types.size > 0 %]
+      [% PROCESS "flag/list.html.tmpl" bug_id=bugid attach_id=attachid %]<br>
+    [% END %]
+  </td>
+</tr>

template/en/default/bug/create/create.html.tmpl

@@ -19,12 +19,15 @@
   # Contributor(s): Gervase Markham <gerv@gerv.net>
   #                 Ville Skyttä <ville.skytta@iki.fi>
   #                 Shane H. W. Travis <travis@sedsystems.ca>
+  #                 Marc Schumann <wurblzap@gmail.com>
   #%]
 
 [% PROCESS "global/field-descs.none.tmpl" %]
 
 [% PROCESS global/header.html.tmpl
   title = "Enter $terms.Bug: $product.name"
+  style_urls = [ 'skins/standard/create_attachment.css' ]
+  javascript_urls = [ "js/attachment.js" ]
   onload="set_assign_to();"
 %]
 
@@ -86,10 +89,24 @@ function set_assign_to() {
         [% END %]
     }
 }
+
+function handleWantsAttachment(wants_attachment) {
+    if (wants_attachment) {
+        document.getElementById('attachment_false').style.display = 'none';
+        document.getElementById('attachment_true').style.display = 'block';
+    }
+    else {
+        document.getElementById('attachment_false').style.display = 'block';
+        document.getElementById('attachment_true').style.display = 'none';
+        clearAttachmentFields();
+    }
+}
+
 -->
 </script>
 
-<form name="Create" id="Create" method="post" action="post_bug.cgi">
+<form name="Create" id="Create" method="post" action="post_bug.cgi"
+      enctype="multipart/form-data">
 <input type="hidden" name="product" value="[% product.name FILTER html %]">
 <input type="hidden" name="token" value="[% token FILTER html %]">
 
@@ -316,6 +333,7 @@ function set_assign_to() {
       [%- END %]
       [% INCLUDE global/textarea.html.tmpl
          name           = 'comment'
+         id             = 'comment'
          minrows        = 10
          maxrows        = 25
          cols           = constants.COMMENT_COLS
@@ -341,6 +359,38 @@ function set_assign_to() {
     <input type="hidden" name="commentprivacy" value="0">
   [% END %]
 
+  <tr>
+    <th align="right" valign="top">Attachment:</th>
+    <td colspan="3">
+      <script type="text/javascript">
+        <!--
+        document.write( '<div id="attachment_false">'
+                      +   '<input type="button" value="Add an attachment" '
+                      +          'onClick="handleWantsAttachment(true)"> '
+                      +   '<em style="display: none">This button has no '
+                      +   'functionality for you because your browser does '
+                      +   'not support CSS or does not use it.</em>'
+                      + '</div>'
+                      + '<div id="attachment_true" style="display: none">'
+                      +   '<input type="button" '
+                      +          'value="Don\'t add an attachment " '
+                      +          'onClick="handleWantsAttachment(false)">');
+        //-->
+      </script>
+        <fieldset>
+          <legend>Add an attachment</legend>
+          <table class="attachment_entry">
+            [% PROCESS attachment/createformcontents.html.tmpl %]
+          </table>
+        </fieldset>
+      <script type="text/javascript">
+        <!--
+        document.write('</div>');
+        //-->
+      </script>
+    </td>
+  </tr>
+
   [% IF UserInGroup('editbugs') %]
     [% IF use_keywords %]
       <tr>

template/en/default/global/messages.html.tmpl

@@ -100,6 +100,12 @@
     The user account [% otheruser.login FILTER html %] has been deleted
     successfully.
 
+  [% ELSIF message_tag == "attachment_creation_failed" %]
+    The [% terms.bug %] was created successfully, but attachment creation
+    failed.
+    Please add your attachment by clicking the "Create a New Attachment" link
+    below.
+
   [% ELSIF message_tag == "buglist_adding_field" %]
     [% title = "Adding field to search page..." %]
     [% link  = "Click here if the page does not redisplay automatically." %]