Store attachments in subdirectories (#5298).

Existing files can be moved to their target subdirectories using rake redmine:attachments:move_to_subdirectories.

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

11 files changed, 118 insertions, 7 deletions

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index 6fa079c91..c288d588b 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -16,6 +16,7 @@
 # Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
 
 require "digest/md5"
+require "fileutils"
 
 class Attachment < ActiveRecord::Base
   belongs_to :container, :polymorphic => true
@@ -92,9 +93,6 @@ class Attachment < ActiveRecord::Base
 
   def filename=(arg)
     write_attribute :filename, sanitize_filename(arg.to_s)
-    if new_record? && disk_filename.blank?
-      self.disk_filename = Attachment.disk_filename(filename)
-    end
     filename
   end
 
@@ -102,7 +100,13 @@ class Attachment < ActiveRecord::Base
   # and computes its MD5 hash
   def files_to_final_location
     if @temp_file && (@temp_file.size > 0)
+      self.disk_directory = target_directory
+      self.disk_filename = Attachment.disk_filename(filename, disk_directory)
       logger.info("Saving attachment '#{self.diskfile}' (#{@temp_file.size} bytes)")
+      path = File.dirname(diskfile)
+      unless File.directory?(path)
+        FileUtils.mkdir_p(path)
+      end
       md5 = Digest::MD5.new
       File.open(diskfile, "wb") do |f|
         if @temp_file.respond_to?(:read)
@@ -134,7 +138,7 @@ class Attachment < ActiveRecord::Base
 
   # Returns file's location on disk
   def diskfile
-    File.join(self.class.storage_path, disk_filename.to_s)
+    File.join(self.class.storage_path, disk_directory.to_s, disk_filename.to_s)
   end
 
   def title
@@ -259,6 +263,26 @@ class Attachment < ActiveRecord::Base
     Attachment.where("created_on < ? AND (container_type IS NULL OR container_type = '')", Time.now - age).destroy_all
   end
 
+  # Moves an existing attachment to its target directory 
+  def move_to_target_directory!
+    if !new_record? & readable?
+      src = diskfile
+      self.disk_directory = target_directory
+      dest = diskfile
+      if src != dest && FileUtils.mkdir_p(File.dirname(dest)) && FileUtils.mv(src, dest)
+        update_column :disk_directory, disk_directory
+      end
+    end
+  end
+
+  # Moves existing attachments that are stored at the root of the files
+  # directory (ie. created before Redmine 2.3) to their target subdirectories
+  def self.move_from_root_to_target_directory
+    Attachment.where("disk_directory IS NULL OR disk_directory = ''").find_each do |attachment|
+      attachment.move_to_target_directory!
+    end
+  end
+
   private
 
   # Physically deletes the file from the file system
@@ -276,8 +300,15 @@ class Attachment < ActiveRecord::Base
     @filename = just_filename.gsub(/[\/\?\%\*\:\|\"\'<>]+/, '_')
   end
 
-  # Returns an ASCII or hashed filename
-  def self.disk_filename(filename)
+  # Returns the subdirectory in which the attachment will be saved
+  def target_directory
+    time = created_on || DateTime.now
+    time.strftime("%Y/%m")
+  end
+
+  # Returns an ASCII or hashed filename that do not
+  # exists yet in the given subdirectory
+  def self.disk_filename(filename, directory=nil)
     timestamp = DateTime.now.strftime("%y%m%d%H%M%S")
     ascii = ''
     if filename =~ %r{^[a-zA-Z0-9_\.\-]*$}
@@ -287,7 +318,7 @@ class Attachment < ActiveRecord::Base
       # keep the extension if any
       ascii << $1 if filename =~ %r{(\.[a-zA-Z0-9]+)$}
     end
-    while File.exist?(File.join(@@storage_path, "#{timestamp}_#{ascii}"))
+    while File.exist?(File.join(storage_path, directory.to_s, "#{timestamp}_#{ascii}"))
       timestamp.succ!
     end
     "#{timestamp}_#{ascii}"
diff --git a/db/migrate/20121213084931_add_attachments_disk_directory.rb b/db/migrate/20121213084931_add_attachments_disk_directory.rb
new file mode 100644
index 000000000..d11fcad9b
--- /dev/null
+++ b/db/migrate/20121213084931_add_attachments_disk_directory.rb
@@ -0,0 +1,9 @@
+class AddAttachmentsDiskDirectory < ActiveRecord::Migration
+  def up
+    add_column :attachments, :disk_directory, :string
+  end
+
+  def down
+    remove_column :attachments, :disk_directory
+  end
+end
diff --git a/lib/tasks/redmine.rake b/lib/tasks/redmine.rake
index 01a4e6227..1ecf507ba 100644
--- a/lib/tasks/redmine.rake
+++ b/lib/tasks/redmine.rake
@@ -21,6 +21,11 @@ namespace :redmine do
     task :prune => :environment do
       Attachment.prune
     end
+
+    desc 'Moves attachments stored at the root of the file directory (ie. created before Redmine 2.3) to their subdirectories'
+    task :move_to_subdirectories => :environment do
+      Attachment.move_from_root_to_target_directory
+    end
   end
 
   namespace :tokens do
diff --git a/test/fixtures/attachments.yml b/test/fixtures/attachments.yml
index 604ac1ac4..d50d0357a 100644
--- a/test/fixtures/attachments.yml
+++ b/test/fixtures/attachments.yml
@@ -4,6 +4,7 @@ attachments_001:
   downloads: 0
   content_type: text/plain
   disk_filename: 060719210727_error281.txt
+  disk_directory: "2006/07"
   container_id: 3
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 1
@@ -16,6 +17,7 @@ attachments_002:
   downloads: 0
   content_type: text/plain
   disk_filename: 060719210727_document.txt
+  disk_directory: "2006/07"
   container_id: 1
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 2
@@ -28,6 +30,7 @@ attachments_003:
   downloads: 0
   content_type: image/gif
   disk_filename: 060719210727_logo.gif
+  disk_directory: "2006/07"
   container_id: 4
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 3
@@ -42,6 +45,7 @@ attachments_004:
   container_id: 3
   downloads: 0
   disk_filename: 060719210727_source.rb
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 4
   filesize: 153
@@ -55,6 +59,7 @@ attachments_005:
   container_id: 3
   downloads: 0
   disk_filename: 060719210727_changeset_iso8859-1.diff
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 5
   filesize: 687
@@ -67,6 +72,7 @@ attachments_006:
   container_id: 3
   downloads: 0
   disk_filename: 060719210727_archive.zip
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 6
   filesize: 157
@@ -79,6 +85,7 @@ attachments_007:
   container_id: 4
   downloads: 0
   disk_filename: 060719210727_archive.zip
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 7
   filesize: 157
@@ -91,6 +98,7 @@ attachments_008:
   container_id: 1
   downloads: 0
   disk_filename: 060719210727_project_file.zip
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 8
   filesize: 320
@@ -103,6 +111,7 @@ attachments_009:
   container_id: 1
   downloads: 0
   disk_filename: 060719210727_archive.zip
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 9
   filesize: 452
@@ -115,6 +124,7 @@ attachments_010:
   container_id: 2
   downloads: 0
   disk_filename: 060719210727_picture.jpg
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 10
   filesize: 452
@@ -127,6 +137,7 @@ attachments_011:
   container_id: 1
   downloads: 0
   disk_filename: 060719210727_picture.jpg
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 11
   filesize: 452
@@ -139,6 +150,7 @@ attachments_012:
   container_id: 1
   downloads: 0
   disk_filename: 060719210727_version_file.zip
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 12
   filesize: 452
@@ -151,6 +163,7 @@ attachments_013:
   container_id: 1
   downloads: 0
   disk_filename: 060719210727_foo.zip
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 13
   filesize: 452
@@ -163,6 +176,7 @@ attachments_014:
   container_id: 3
   downloads: 0
   disk_filename: 060719210727_changeset_utf8.diff
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   id: 14
   filesize: 687
@@ -176,6 +190,7 @@ attachments_015:
   container_id: 14
   downloads: 0
   disk_filename: 060719210727_changeset_utf8.diff
+  disk_directory: "2006/07"
   digest: b91e08d0cf966d5c6ff411bd8c4cc3a2
   filesize: 687
   filename: private.diff
@@ -187,6 +202,7 @@ attachments_016:
   downloads: 0
   created_on: 2010-11-23 16:14:50 +09:00
   disk_filename: 101123161450_testfile_1.png
+  disk_directory: "2010/11"
   container_id: 14
   digest: 8e0294de2441577c529f170b6fb8f638
   id: 16
@@ -200,6 +216,7 @@ attachments_017:
   downloads: 0
   created_on: 2010-12-23 16:14:50 +09:00
   disk_filename: 101223161450_testfile_2.png
+  disk_directory: "2010/12"
   container_id: 14
   digest: 6bc2963e8d7ea0d3e68d12d1fba3d6ca
   id: 17
@@ -213,6 +230,7 @@ attachments_018:
   downloads: 0
   created_on: 2011-01-23 16:14:50 +09:00
   disk_filename: 101123161450_testfile_1.png
+  disk_directory: "2010/11"
   container_id: 14
   digest: 8e0294de2441577c529f170b6fb8f638
   id: 18
@@ -226,6 +244,7 @@ attachments_019:
   downloads: 0
   created_on: 2011-02-23 16:14:50 +09:00
   disk_filename: 101223161450_testfile_2.png
+  disk_directory: "2010/12"
   container_id: 14
   digest: 6bc2963e8d7ea0d3e68d12d1fba3d6ca
   id: 19
@@ -234,3 +253,17 @@ attachments_019:
   filename: Testテスト.PNG
   filesize: 3582
   author_id: 2
+attachments_020: 
+  content_type: text/plain
+  downloads: 0
+  created_on: 2012-05-12 16:14:50 +09:00
+  disk_filename: 120512161450_root_attachment.txt
+  disk_directory:
+  container_id: 14
+  digest: b0fe2abdb2599743d554a61d7da7ff74
+  id: 20
+  container_type: Issue
+  description: ""
+  filename: root_attachment.txt
+  filesize: 54
+  author_id: 2
diff --git a/test/fixtures/files/060719210727_archive.zip b/test/fixtures/files/2006/07/060719210727_archive.zip
index 5467885d4..5467885d4 100644
--- a/test/fixtures/files/060719210727_archive.zip
+++ b/test/fixtures/files/2006/07/060719210727_archive.zip
diff --git a/test/fixtures/files/060719210727_changeset_iso8859-1.diff b/test/fixtures/files/2006/07/060719210727_changeset_iso8859-1.diff
index 9bade6ab9..9bade6ab9 100644
--- a/test/fixtures/files/060719210727_changeset_iso8859-1.diff
+++ b/test/fixtures/files/2006/07/060719210727_changeset_iso8859-1.diff
diff --git a/test/fixtures/files/060719210727_changeset_utf8.diff b/test/fixtures/files/2006/07/060719210727_changeset_utf8.diff
index e594f203a..e594f203a 100644
--- a/test/fixtures/files/060719210727_changeset_utf8.diff
+++ b/test/fixtures/files/2006/07/060719210727_changeset_utf8.diff
diff --git a/test/fixtures/files/060719210727_source.rb b/test/fixtures/files/2006/07/060719210727_source.rb
index dccb59165..dccb59165 100644
--- a/test/fixtures/files/060719210727_source.rb
+++ b/test/fixtures/files/2006/07/060719210727_source.rb
diff --git a/test/fixtures/files/101123161450_testfile_1.png b/test/fixtures/files/2010/11/101123161450_testfile_1.png
index 6dad7f1d8..6dad7f1d8 100644
--- a/test/fixtures/files/101123161450_testfile_1.png
+++ b/test/fixtures/files/2010/11/101123161450_testfile_1.png
diff --git a/test/fixtures/files/101223161450_testfile_2.png b/test/fixtures/files/2010/12/101223161450_testfile_2.png
index aec3fcfdf..aec3fcfdf 100644
--- a/test/fixtures/files/101223161450_testfile_2.png
+++ b/test/fixtures/files/2010/12/101223161450_testfile_2.png
diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb
index 61ff048a7..ad759152f 100644
--- a/test/unit/attachment_test.rb
+++ b/test/unit/attachment_test.rb
@@ -52,10 +52,25 @@ class AttachmentTest < ActiveSupport::TestCase
     assert_equal 'text/plain', a.content_type
     assert_equal 0, a.downloads
     assert_equal '1478adae0d4eb06d35897518540e25d6', a.digest
+
+    assert a.disk_directory
+    assert_match %r{\A\d{4}/\d{2}\z}, a.disk_directory
+
     assert File.exist?(a.diskfile)
     assert_equal 59, File.size(a.diskfile)
   end
 
+  def test_copy_should_preserve_attributes
+    a = Attachment.find(1)
+    copy = a.copy
+
+    assert_save copy
+    copy = Attachment.order('id DESC').first
+    %w(filename filesize content_type author_id created_on description digest disk_filename disk_directory diskfile).each do |attribute|
+      assert_equal a.send(attribute), copy.send(attribute), "#{attribute} was different"
+    end
+  end
+
   def test_size_should_be_validated_for_new_file
     with_settings :attachment_max_size => 0 do
       a = Attachment.new(:container => Issue.find(1),
@@ -123,6 +138,9 @@ class AttachmentTest < ActiveSupport::TestCase
   end
 
   def test_identical_attachments_at_the_same_time_should_not_overwrite
+    time = DateTime.now
+    DateTime.stubs(:now).returns(time)
+
     a1 = Attachment.create!(:container => Issue.find(1),
                             :file => uploaded_test_file("testfile.txt", ""),
                             :author => User.find(1))
@@ -168,6 +186,21 @@ class AttachmentTest < ActiveSupport::TestCase
     end
   end
 
+  def test_move_from_root_to_target_directory_should_move_root_files
+    a = Attachment.find(20)
+    assert a.disk_directory.blank?
+    # Create a real file for this fixture
+    File.open(a.diskfile, "w") do |f|
+      f.write "test file at the root of files directory"
+    end
+    assert a.readable?
+    Attachment.move_from_root_to_target_directory
+
+    a.reload
+    assert_equal '2012/05', a.disk_directory
+    assert a.readable?
+  end
+
   context "Attachmnet.attach_files" do
     should "attach the file" do
       issue = Issue.first