diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index 16a0fe723..7adad0008 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 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
diff --git a/app/views/attachments/_form.html.erb b/app/views/attachments/_form.html.erb
index a7c463989..464d3a2d7 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| %>
+
<%= 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 809194779..690fa9386 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)%> <%=l(:label_attachment_plural)%>
<%= render :partial => 'attachments/form' %>
<%= render :partial => 'attachments/form', :locals => {:container => @issue} %>
<%= 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 2cf79672d..311c72588 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 6f1bea304..371de69a6 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 75c0b6a83..58af193ac 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 0486529b3..fb68ab655 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 e840770a3..346e0ab8f 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