Copy attachments on issue and project copy (#3055).

git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@8676 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/models/attachment.rb

@@ -49,8 +49,16 @@ class Attachment < ActiveRecord::Base
   before_save :files_to_final_location
   after_destroy :delete_from_disk
 
+  # Returns an unsaved copy of the attachment
+  def copy(attributes=nil)
+    copy = self.class.new
+    copy.attributes = self.attributes.dup.except("id", "downloads")
+    copy.attributes = attributes if attributes
+    copy
+  end
+
   def validate_max_file_size
-    if self.filesize > Setting.attachment_max_size.to_i.kilobytes
+    if @temp_file && self.filesize > Setting.attachment_max_size.to_i.kilobytes
       errors.add(:base, :too_long, :count => Setting.attachment_max_size.to_i.kilobytes)
     end
   end
@@ -96,9 +104,11 @@ class Attachment < ActiveRecord::Base
     end
   end
 
-  # Deletes file on the disk
+  # Deletes the file from the file system if it's not referenced by other attachments
   def delete_from_disk
-    File.delete(diskfile) if !filename.blank? && File.exist?(diskfile)
+    if Attachment.first(:conditions => ["disk_filename = ? AND id <> ?", disk_filename, id]).nil?
+      delete_from_disk!
+    end
   end
 
   # Returns file's location on disk
@@ -173,7 +183,15 @@ class Attachment < ActiveRecord::Base
      }
   end
 
-private
+  private
+
+  # Physically deletes the file from the file system
+  def delete_from_disk!
+    if disk_filename.present? && File.exist?(diskfile)
+      File.delete(diskfile)
+    end
+  end
+
   def sanitize_filename(value)
     # get only the filename, not the whole path
     just_filename = value.gsub(/^.*(\\|\/)/, '')

app/models/issue.rb

@@ -135,6 +135,9 @@ class Issue < ActiveRecord::Base
     self.custom_field_values = issue.custom_field_values.inject({}) {|h,v| h[v.custom_field_id] = v.value; h}
     self.status = issue.status
     self.author = User.current
+    self.attachments = issue.attachments.map do |attachement| 
+      attachement.copy(:container => self)
+    end
     @copied_from = issue
     self
   end

test/functional/issues_controller_test.rb

@@ -1654,6 +1654,44 @@ class IssuesControllerTest < ActionController::TestCase
     assert_equal 'Copy', issue.subject
   end
 
+  def test_create_as_copy_should_copy_attachments
+    @request.session[:user_id] = 2
+    issue = Issue.find(3)
+    count = issue.attachments.count
+    assert count > 0
+
+    assert_difference 'Issue.count' do
+      assert_difference 'Attachment.count', count do
+        assert_no_difference 'Journal.count' do
+          post :create, :project_id => 1, :copy_from => 3,
+            :issue => {:project_id => '1', :tracker_id => '3', :status_id => '1', :subject => 'Copy with attachments'}
+        end
+      end
+    end
+    copy = Issue.first(:order => 'id DESC')
+    assert_equal count, copy.attachments.count
+    assert_equal issue.attachments.map(&:filename).sort, copy.attachments.map(&:filename).sort
+  end
+
+  def test_create_as_copy_with_attachments_should_add_new_files
+    @request.session[:user_id] = 2
+    issue = Issue.find(3)
+    count = issue.attachments.count
+    assert count > 0
+
+    assert_difference 'Issue.count' do
+      assert_difference 'Attachment.count', count + 1 do
+        assert_no_difference 'Journal.count' do
+          post :create, :project_id => 1, :copy_from => 3,
+            :issue => {:project_id => '1', :tracker_id => '3', :status_id => '1', :subject => 'Copy with attachments'},
+            :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain'), 'description' => 'test file'}}
+        end
+      end
+    end
+    copy = Issue.first(:order => 'id DESC')
+    assert_equal count + 1, copy.attachments.count
+  end
+
   def test_create_as_copy_with_failure
     @request.session[:user_id] = 2
     post :create, :project_id => 1, :copy_from => 1,

test/unit/attachment_test.rb

@@ -52,6 +52,25 @@ class AttachmentTest < ActiveSupport::TestCase
     assert_equal 59, File.size(a.diskfile)
   end
 
+  def test_size_should_be_validated_for_new_file
+    with_settings :attachment_max_size => 0 do
+      a = Attachment.new(:container => Issue.find(1),
+                         :file => uploaded_test_file("testfile.txt", "text/plain"),
+                         :author => User.find(1))
+      assert !a.save
+    end
+  end
+
+  def test_size_should_not_be_validated_when_copying
+    a = Attachment.create!(:container => Issue.find(1),
+                           :file => uploaded_test_file("testfile.txt", "text/plain"),
+                           :author => User.find(1))
+    with_settings :attachment_max_size => 0 do
+      copy = a.copy
+      assert copy.save
+    end
+  end
+
   def test_destroy
     a = Attachment.new(:container => Issue.find(1),
                        :file => uploaded_test_file("testfile.txt", "text/plain"),
@@ -69,6 +88,22 @@ class AttachmentTest < ActiveSupport::TestCase
     assert !File.exist?(diskfile)
   end
 
+  def test_destroy_should_not_delete_file_referenced_by_other_attachment
+    a = Attachment.create!(:container => Issue.find(1),
+                           :file => uploaded_test_file("testfile.txt", "text/plain"),
+                           :author => User.find(1))
+    diskfile = a.diskfile
+
+    copy = a.copy
+    copy.save!
+
+    assert File.exists?(diskfile)
+    a.destroy
+    assert File.exists?(diskfile)
+    copy.destroy
+    assert !File.exists?(diskfile)
+  end
+
   def test_create_should_auto_assign_content_type
     a = Attachment.new(:container => Issue.find(1),
                        :file => uploaded_test_file("testfile.txt", ""),

test/unit/project_test.rb

@@ -870,6 +870,18 @@ class ProjectTest < ActiveSupport::TestCase
       assert_not_equal source_relation_cross_project.id, copied_relation.id
     end
 
+    should "copy issue attachments" do
+      issue = Issue.generate!(:subject => "copy with attachment", :tracker_id => 1, :project_id => @source_project.id)
+      Attachment.create!(:container => issue, :file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 1)
+      @source_project.issues << issue
+      assert @project.copy(@source_project)
+
+      copied_issue = @project.issues.first(:conditions => {:subject => "copy with attachment"})
+      assert_not_nil copied_issue
+      assert_equal 1, copied_issue.attachments.count, "Attachment not copied"
+      assert_equal "testfile.txt", copied_issue.attachments.first.filename
+    end
+
     should "copy memberships" do
       assert @project.valid?
       assert @project.members.empty?