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

app/controllers/issues_controller.rb

@@ -149,8 +149,8 @@ class IssuesController < ApplicationController
 
   def create
     call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue })
+    @issue.save_attachments(params[:attachments])
     if @issue.save
-      attachments = Attachment.attach_files(@issue, params[:attachments])
       call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue})
       respond_to do |format|
         format.html {
@@ -181,6 +181,7 @@ class IssuesController < ApplicationController
 
   def update
     return unless update_issue_from_params
+    @issue.save_attachments(params[:attachments])
     saved = false
     begin
       saved = @issue.save_issue_with_child_records(params, @time_entry)

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

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

app/views/attachments/_form.html.erb

@@ -1,3 +1,11 @@
+<% if defined?(container) && container && container.saved_attachments %>
+  <% container.saved_attachments.each_with_index do |attachment, i| %>
+    <span class="icon icon-attachment" style="display:block; line-height:1.5em;">
+      <%= h(attachment.filename) %> (<%= number_to_human_size(attachment.filesize) %>)
+      <%= hidden_field_tag "attachments[p#{i}][token]", "#{attachment.id}.#{attachment.digest}" %>
+    </span>
+  <% end %>
+<% end %>
 <span id="attachments_fields">
   <span>
     <%= file_field_tag 'attachments[1][file]', :size => 30, :id => nil, :class => 'file',

app/views/issues/_edit.html.erb

@@ -31,7 +31,7 @@
     <%= wikitoolbar_for 'notes' %>
     <%= call_hook(:view_issues_edit_notes_bottom, { :issue => @issue, :notes => @notes, :form => f }) %>
 
-    <p><%=l(:label_attachment_plural)%><br /><%= render :partial => 'attachments/form' %></p>
+    <p><%=l(:label_attachment_plural)%><br /><%= render :partial => 'attachments/form', :locals => {:container => @issue} %></p>
     </fieldset>
     </div>
 

app/views/issues/new.html.erb

@@ -18,7 +18,7 @@
     </p>
     <% end %>
 
-    <p id="attachments_form"><%= label_tag('attachments[1][file]', l(:label_attachment_plural))%><%= render :partial => 'attachments/form' %></p>
+    <p id="attachments_form"><%= label_tag('attachments[1][file]', l(:label_attachment_plural))%><%= render :partial => 'attachments/form', :locals => {:container => @issue} %></p>
 
     <% if @issue.safe_attribute? 'watcher_user_ids' -%>
       <p id="watchers_form"><label><%= l(:label_issue_watchers) %></label>

test/functional/attachments_controller_test.rb

@@ -210,6 +210,14 @@ class AttachmentsControllerTest < ActionController::TestCase
     set_tmp_attachments_directory
   end
 
+  def test_show_file_without_container_should_be_denied
+    attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
+
+    @request.session[:user_id] = 2
+    get :show, :id => attachment.id
+    assert_response 403
+  end
+
   def test_download_text_file
     get :download, :id => 4
     assert_response :success

test/functional/issues_controller_test.rb

@@ -1663,6 +1663,69 @@ class IssuesControllerTest < ActionController::TestCase
     assert_equal 59, File.size(attachment.diskfile)
   end
 
+  def test_post_create_with_failure_should_save_attachments
+    set_tmp_attachments_directory
+    @request.session[:user_id] = 2
+
+    assert_no_difference 'Issue.count' do
+      assert_difference 'Attachment.count' do
+        post :create, :project_id => 1,
+          :issue => { :tracker_id => '1', :subject => '' },
+          :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain'), 'description' => 'test file'}}
+        assert_response :success
+        assert_template 'new'
+      end
+    end
+
+    attachment = Attachment.first(:order => 'id DESC')
+    assert_equal 'testfile.txt', attachment.filename
+    assert File.exists?(attachment.diskfile)
+    assert_nil attachment.container
+
+    assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
+    assert_tag 'span', :content => /testfile.txt/
+  end
+
+  def test_post_create_with_failure_should_keep_saved_attachments
+    set_tmp_attachments_directory
+    attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
+    @request.session[:user_id] = 2
+
+    assert_no_difference 'Issue.count' do
+      assert_no_difference 'Attachment.count' do
+        post :create, :project_id => 1,
+          :issue => { :tracker_id => '1', :subject => '' },
+          :attachments => {'p0' => {'token' => attachment.token}}
+        assert_response :success
+        assert_template 'new'
+      end
+    end
+
+    assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
+    assert_tag 'span', :content => /testfile.txt/
+  end
+
+  def test_post_create_should_attach_saved_attachments
+    set_tmp_attachments_directory
+    attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
+    @request.session[:user_id] = 2
+
+    assert_difference 'Issue.count' do
+      assert_no_difference 'Attachment.count' do
+        post :create, :project_id => 1,
+          :issue => { :tracker_id => '1', :subject => 'Saved attachments' },
+          :attachments => {'p0' => {'token' => attachment.token}}
+        assert_response 302
+      end
+    end
+
+    issue = Issue.first(:order => 'id DESC')
+    assert_equal 1, issue.attachments.count
+
+    attachment.reload
+    assert_equal issue, attachment.container
+  end
+
   context "without workflow privilege" do
     setup do
       Workflow.delete_all(["role_id = ?", Role.anonymous.id])
@@ -2301,6 +2364,72 @@ class IssuesControllerTest < ActionController::TestCase
     assert mail.body.include?('testfile.txt')
   end
 
+  def test_put_update_with_failure_should_save_attachments
+    set_tmp_attachments_directory
+    @request.session[:user_id] = 2
+
+    assert_no_difference 'Journal.count' do
+      assert_difference 'Attachment.count' do
+        put :update, :id => 1,
+          :issue => { :subject => '' },
+          :attachments => {'1' => {'file' => uploaded_test_file('testfile.txt', 'text/plain'), 'description' => 'test file'}}
+        assert_response :success
+        assert_template 'edit'
+      end
+    end
+
+    attachment = Attachment.first(:order => 'id DESC')
+    assert_equal 'testfile.txt', attachment.filename
+    assert File.exists?(attachment.diskfile)
+    assert_nil attachment.container
+
+    assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
+    assert_tag 'span', :content => /testfile.txt/
+  end
+
+  def test_put_update_with_failure_should_keep_saved_attachments
+    set_tmp_attachments_directory
+    attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
+    @request.session[:user_id] = 2
+
+    assert_no_difference 'Journal.count' do
+      assert_no_difference 'Attachment.count' do
+        put :update, :id => 1,
+          :issue => { :subject => '' },
+          :attachments => {'p0' => {'token' => attachment.token}}
+        assert_response :success
+        assert_template 'edit'
+      end
+    end
+
+    assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
+    assert_tag 'span', :content => /testfile.txt/
+  end
+
+  def test_put_update_should_attach_saved_attachments
+    set_tmp_attachments_directory
+    attachment = Attachment.create!(:file => uploaded_test_file("testfile.txt", "text/plain"), :author_id => 2)
+    @request.session[:user_id] = 2
+
+    assert_difference 'Journal.count' do
+      assert_difference 'JournalDetail.count' do
+        assert_no_difference 'Attachment.count' do
+          put :update, :id => 1,
+            :notes => 'Attachment added',
+            :attachments => {'p0' => {'token' => attachment.token}}
+          assert_redirected_to '/issues/1'
+        end
+      end
+    end
+
+    attachment.reload
+    assert_equal Issue.find(1), attachment.container
+
+    journal = Journal.first(:order => 'id DESC')
+    assert_equal 1, journal.details.size
+    assert_equal 'testfile.txt', journal.details.first.value
+  end
+
   def test_put_update_with_attachment_that_fails_to_save
     set_tmp_attachments_directory
 

test/functional/issues_controller_transaction_test.rb

@@ -58,7 +58,33 @@ class IssuesControllerTransactionTest < ActionController::TestCase
 
     assert_no_difference 'Journal.count' do
       assert_no_difference 'TimeEntry.count' do
-        assert_no_difference 'Attachment.count' do
+        put :update,
+              :id => issue.id,
+              :issue => {
+                :fixed_version_id => 4,
+                :lock_version => (issue.lock_version - 1)
+              },
+              :notes => 'My notes',
+              :time_entry => { :hours => '2.5', :comments => '', :activity_id => TimeEntryActivity.first.id }
+      end
+    end
+
+    assert_response :success
+    assert_template 'edit'
+    assert_tag 'div', :attributes => {:class => 'conflict'}
+    assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'overwrite'}
+    assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'add_notes'}
+    assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'cancel'}
+  end
+
+  def test_update_stale_issue_should_save_attachments
+    set_tmp_attachments_directory
+    issue = Issue.find(2)
+    @request.session[:user_id] = 2
+
+    assert_no_difference 'Journal.count' do
+      assert_no_difference 'TimeEntry.count' do
+        assert_difference 'Attachment.count' do
           put :update,
                 :id => issue.id,
                 :issue => {
@@ -74,10 +100,9 @@ class IssuesControllerTransactionTest < ActionController::TestCase
 
     assert_response :success
     assert_template 'edit'
-    assert_tag 'div', :attributes => {:class => 'conflict'}
-    assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'overwrite'}
-    assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'add_notes'}
-    assert_tag 'input', :attributes => {:name => 'conflict_resolution', :value => 'cancel'}
+    attachment = Attachment.first(:order => 'id DESC')
+    assert_tag 'input', :attributes => {:name => 'attachments[p0][token]', :value => attachment.token}
+    assert_tag 'span', :content => /testfile.txt/
   end
 
   def test_update_stale_issue_without_notes_should_not_show_add_notes_option

test/unit/attachment_test.rb

@@ -38,6 +38,10 @@ class AttachmentTest < ActiveSupport::TestCase
     set_tmp_attachments_directory
   end
 
+  def test_container_for_new_attachment_should_be_nil
+    assert_nil Attachment.new.container
+  end
+
   def test_create
     a = Attachment.new(:container => Issue.find(1),
                        :file => uploaded_test_file("testfile.txt", "text/plain"),

vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb

@@ -32,8 +32,8 @@ module Redmine
           has_many :attachments, options.merge(:as => :container,
                                                :order => "#{Attachment.table_name}.created_on",
                                                :dependent => :destroy)
-          attr_accessor :unsaved_attachments
           send :include, Redmine::Acts::Attachable::InstanceMethods
+          before_save :attach_saved_attachments
         end
       end
 
@@ -52,6 +52,43 @@ module Redmine
             user.allowed_to?(self.class.attachable_options[:delete_permission], self.project)
         end
 
+        def saved_attachments
+          @saved_attachments ||= []
+        end
+
+        def unsaved_attachments
+          @unsaved_attachments ||= []
+        end
+
+        def save_attachments(attachments, author=User.current)
+          if attachments && attachments.is_a?(Hash)
+            attachments.each_value do |attachment|
+              a = nil
+              if file = attachment['file']
+                next unless file && file.size > 0
+                a = Attachment.create(:file => file,
+                                      :description => attachment['description'].to_s.strip,
+                                      :author => author)
+              elsif token = attachment['token']
+                a = Attachment.find_by_token(token)
+              end
+              next unless a
+              if a.new_record?
+                unsaved_attachments << a
+              else
+                saved_attachments << a
+              end
+            end
+          end
+          {:files => saved_attachments, :unsaved => unsaved_attachments}
+        end
+
+        def attach_saved_attachments
+          saved_attachments.each do |attachment|
+            self.attachments << attachment
+          end
+        end
+
         module ClassMethods
         end
       end