From 8a3623733fa2ef879bb2a3f4355e0ce0df49b278 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Fri, 20 Jan 2012 17:56:28 +0000 Subject: [PATCH] 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 | 26 +++++++++++++--- app/models/issue.rb | 3 ++ test/functional/issues_controller_test.rb | 38 +++++++++++++++++++++++ test/unit/attachment_test.rb | 35 +++++++++++++++++++++ test/unit/project_test.rb | 12 +++++++ 5 files changed, 110 insertions(+), 4 deletions(-) diff --git a/app/models/attachment.rb b/app/models/attachment.rb index 09e4057cf..d57422db7 100644 --- a/app/models/attachment.rb +++ b/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(/^.*(\\|\/)/, '') diff --git a/app/models/issue.rb b/app/models/issue.rb index abd2d210a..16257bf83 100644 --- a/app/models/issue.rb +++ b/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 diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 1273767b4..55c4cb932 100644 --- a/test/functional/issues_controller_test.rb +++ b/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, diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb index 56474b051..0486529b3 100644 --- a/test/unit/attachment_test.rb +++ b/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", ""), diff --git a/test/unit/project_test.rb b/test/unit/project_test.rb index 39c0e8ab0..b3fd3de24 100644 --- a/test/unit/project_test.rb +++ b/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?