From d4e6355eb3f220787e9374eeef6fb496f213f0d1 2012-02-16 21:00:11 From: Jean-Philippe Lang Date: 2012-02-16 21:00:11 Subject: [PATCH] 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 --- diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index 16a0fe7..7adad00 100644 --- a/app/controllers/issues_controller.rb +++ b/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) diff --git a/app/models/attachment.rb b/app/models/attachment.rb index d57422d..7fb9ed3 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 2750d7f..eced776 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 diff --git a/app/views/attachments/_form.html.erb b/app/views/attachments/_form.html.erb index a7c4639..464d3a2 100644 --- a/app/views/attachments/_form.html.erb +++ b/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| %> + + <%= h(attachment.filename) %> (<%= number_to_human_size(attachment.filesize) %>) + <%= hidden_field_tag "attachments[p#{i}][token]", "#{attachment.id}.#{attachment.digest}" %> + + <% end %> +<% end %> <%= file_field_tag 'attachments[1][file]', :size => 30, :id => nil, :class => 'file', diff --git a/app/views/issues/_edit.html.erb b/app/views/issues/_edit.html.erb index 8091947..690fa93 100644 --- a/app/views/issues/_edit.html.erb +++ b/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 }) %> -

<%=l(:label_attachment_plural)%>
<%= render :partial => 'attachments/form' %>

+

<%=l(:label_attachment_plural)%>
<%= render :partial => 'attachments/form', :locals => {:container => @issue} %>

diff --git a/app/views/issues/new.html.erb b/app/views/issues/new.html.erb index f8da7e9..1126821 100644 --- a/app/views/issues/new.html.erb +++ b/app/views/issues/new.html.erb @@ -18,7 +18,7 @@

<% end %> -

<%= label_tag('attachments[1][file]', l(:label_attachment_plural))%><%= render :partial => 'attachments/form' %>

+

<%= label_tag('attachments[1][file]', l(:label_attachment_plural))%><%= render :partial => 'attachments/form', :locals => {:container => @issue} %>

<% if @issue.safe_attribute? 'watcher_user_ids' -%>

diff --git a/test/functional/attachments_controller_test.rb b/test/functional/attachments_controller_test.rb index 2cf7967..311c725 100644 --- a/test/functional/attachments_controller_test.rb +++ b/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 diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 6f1bea3..371de69 100644 --- a/test/functional/issues_controller_test.rb +++ b/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 diff --git a/test/functional/issues_controller_transaction_test.rb b/test/functional/issues_controller_transaction_test.rb index 75c0b6a..58af193 100644 --- a/test/functional/issues_controller_transaction_test.rb +++ b/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 diff --git a/test/unit/attachment_test.rb b/test/unit/attachment_test.rb index 0486529..fb68ab6 100644 --- a/test/unit/attachment_test.rb +++ b/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"), diff --git a/vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb b/vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb index e840770..346e0ab 100644 --- a/vendor/plugins/acts_as_attachable/lib/acts_as_attachable.rb +++ b/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