Better handling of attachments when issue validation fails (#10253).

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

2 files changed, 44 insertions, 40 deletions

diff --git a/app/models/attachment.rb b/app/models/attachment.rb
index d57422db7..7fb9ed379 100644
--- a/app/models/attachment.rb
+++ b/app/models/attachment.rb
@@ -21,7 +21,7 @@ class Attachment < ActiveRecord::Base
   belongs_to :container, :polymorphic => true
   belongs_to :author, :class_name => "User", :foreign_key => "author_id"
 
-  validates_presence_of :container, :filename, :author
+  validates_presence_of :filename, :author
   validates_length_of :filename, :maximum => 255
   validates_length_of :disk_filename, :maximum => 255
   validate :validate_max_file_size
@@ -49,6 +49,15 @@ class Attachment < ActiveRecord::Base
   before_save :files_to_final_location
   after_destroy :delete_from_disk
 
+  def container_with_blank_type_check
+    if container_type.blank?
+      nil
+    else
+      container_without_blank_type_check
+    end
+  end
+  alias_method_chain :container, :blank_type_check unless method_defined?(:container_without_blank_type_check)
+
   # Returns an unsaved copy of the attachment
   def copy(attributes=nil)
     copy = self.class.new
@@ -121,15 +130,15 @@ class Attachment < ActiveRecord::Base
   end
 
   def project
-    container.project
+    container.try(:project)
   end
 
   def visible?(user=User.current)
-    container.attachments_visible?(user)
+    container && container.attachments_visible?(user)
   end
 
   def deletable?(user=User.current)
-    container.attachments_deletable?(user)
+    container && container.attachments_deletable?(user)
   end
 
   def image?
@@ -149,32 +158,31 @@ class Attachment < ActiveRecord::Base
     File.readable?(diskfile)
   end
 
+  # Returns the attachment token
+  def token
+    "#{id}.#{digest}"
+  end
+
+  # Finds an attachment that matches the given token and that has no container
+  def self.find_by_token(token)
+    if token.to_s =~ /^(\d+)\.([0-9a-f]+)$/
+      attachment_id, attachment_digest = $1, $2
+      attachment = Attachment.first(:conditions => {:id => attachment_id, :digest => attachment_digest})
+      if attachment && attachment.container.nil?
+        attachment
+      end
+    end
+  end
+
   # Bulk attaches a set of files to an object
   #
   # Returns a Hash of the results:
   # :files => array of the attached files
   # :unsaved => array of the files that could not be attached
   def self.attach_files(obj, attachments)
-    attached = []
-    if attachments && attachments.is_a?(Hash)
-      attachments.each_value do |attachment|
-        file = attachment['file']
-        next unless file && file.size > 0
-        a = Attachment.create(:container => obj,
-                              :file => file,
-                              :description => attachment['description'].to_s.strip,
-                              :author => User.current)
-        obj.attachments << a
-
-        if a.new_record?
-          obj.unsaved_attachments ||= []
-          obj.unsaved_attachments << a
-        else
-          attached << a
-        end
-      end
-    end
-    {:files => attached, :unsaved => obj.unsaved_attachments}
+    result = obj.save_attachments(attachments, User.current)
+    obj.attach_saved_attachments
+    result
   end
 
   def self.latest_attach(attachments, filename)
@@ -183,6 +191,11 @@ class Attachment < ActiveRecord::Base
      }
   end
 
+  def self.prune(age=1.day)
+    attachments = Attachment.all(:conditions => ["created_on < ? AND (container_type IS NULL OR container_type = ''", Time.now - age])
+    attachments.each(&:destroy)
+  end
+
   private
 
   # Physically deletes the file from the file system
diff --git a/app/models/issue.rb b/app/models/issue.rb
index 2750d7f63..eced77684 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -680,8 +680,7 @@ class Issue < ActiveRecord::Base
     s
   end
 
-  # Saves an issue, time_entry, attachments, and a journal from the parameters
-  # Returns false if save fails
+  # Saves an issue and a time_entry from the parameters
   def save_issue_with_child_records(params, existing_time_entry=nil)
     Issue.transaction do
       if params[:time_entry] && (params[:time_entry][:hours].present? || params[:time_entry][:comments].present?) && User.current.allowed_to?(:log_time, project)
@@ -694,21 +693,13 @@ class Issue < ActiveRecord::Base
         self.time_entries << @time_entry
       end
 
-      if valid?
-        attachments = Attachment.attach_files(self, params[:attachments])
+      # TODO: Rename hook
+      Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
+      if save
         # TODO: Rename hook
-        Redmine::Hook.call_hook(:controller_issues_edit_before_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
-        begin
-          if save
-            # TODO: Rename hook
-            Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
-          else
-            raise ActiveRecord::Rollback
-          end
-        rescue ActiveRecord::StaleObjectError
-          attachments[:files].each(&:destroy)
-          raise ActiveRecord::StaleObjectError
-        end
+        Redmine::Hook.call_hook(:controller_issues_edit_after_save, { :params => params, :issue => self, :time_entry => @time_entry, :journal => @current_journal})
+      else
+        raise ActiveRecord::Rollback
       end
     end
   end