diff --git a/app/controllers/work_packages_controller.rb b/app/controllers/work_packages_controller.rb index 9ee338777b..9004d0e94f 100644 --- a/app/controllers/work_packages_controller.rb +++ b/app/controllers/work_packages_controller.rb @@ -128,12 +128,11 @@ class WorkPackagesController < ApplicationController WorkPackageObserver.instance.send_notification = send_notifications? + work_package.attach_files(params[:attachments]) + if work_package.save flash[:notice] = I18n.t(:notice_successful_create) - Attachment.attach_files(work_package, params[:attachments]) - render_attachment_warning_if_needed(work_package) - call_hook(:controller_work_package_new_after_save, { :params => params, :work_package => work_package }) redirect_to(work_package_path(work_package)) diff --git a/app/models/work_package.rb b/app/models/work_package.rb index ce2d50227e..408181b24a 100644 --- a/app/models/work_package.rb +++ b/app/models/work_package.rb @@ -141,6 +141,8 @@ class WorkPackage < ActiveRecord::Base acts_as_attachable :after_add => :attachments_changed, :after_remove => :attachments_changed + after_validation :set_attachments_error_details, if: lambda {|work_package| work_package.errors.messages.has_key? :attachments} + # Mapping attributes, that are passed in as id's onto their respective associations # (eg. type=4711 onto type=Type.find(4711)) include AssociationsMapper @@ -284,8 +286,10 @@ class WorkPackage < ActiveRecord::Base # ACTS AS ATTACHABLE # Callback on attachment deletion def attachments_changed(obj) - add_journal - save! + unless new_record? + add_journal + save + end end # ACTS AS JOURNALIZED @@ -407,13 +411,9 @@ class WorkPackage < ActiveRecord::Base update_by(user, attributes) - if save - # as attach_files always saves an attachment right away - # it is not possible to stage attaching and check for - # valid. If this would be possible, we could check - # for this along with update_attributes - attachments = Attachment.attach_files(self, raw_attachments) - end + attach_files(raw_attachments) + + save end def update_by(user, attributes) @@ -1001,4 +1001,12 @@ class WorkPackage < ActiveRecord::Base end # <<< issues.rb <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<< + def set_attachments_error_details + # Remark: the pseudo loop is already refactored in the next pull request + self.attachments.each do |attachment| + next if attachment.valid? + errors.messages[:attachments].first << " - #{attachment.errors.full_messages.first}" + break + end + end end diff --git a/doc/CHANGELOG.md b/doc/CHANGELOG.md index 027ca65825..f43f57c460 100644 --- a/doc/CHANGELOG.md +++ b/doc/CHANGELOG.md @@ -33,6 +33,7 @@ See doc/COPYRIGHT.rdoc for more details. * `#1738` Forum problem when no description given. * `#1916` Work package update screen is closed when attached file is deleted * `#1935` Fixed bug: Default submenu for wiki pages is wrong (Configure menu item) +* `#2009` No journal entry created for attachments if the attachment is added on container creation * `#2371` Add support for IE10 to Timelines * `#2448` Accelerate work package updates * `#2479` Remove TinyMCE spike diff --git a/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb b/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb index 532b88fb21..c385d80ddf 100644 --- a/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb +++ b/lib/plugins/acts_as_attachable/lib/acts_as_attachable.rb @@ -66,6 +66,20 @@ module Redmine @unsaved_attachments ||= [] end + # Bulk attaches a set of files to an object + def attach_files(attachments) + if attachments && attachments.is_a?(Hash) + attachments.each_value do |attachment| + file = attachment['file'] + next unless file && file.size > 0 + self.attachments.build(file: file, + container: self, + description: attachment['description'].to_s.strip, + author: User.current) + end + end + end + module ClassMethods end end diff --git a/spec/controllers/work_packages_controller_spec.rb b/spec/controllers/work_packages_controller_spec.rb index 20e0e4f165..644cf67845 100644 --- a/spec/controllers/work_packages_controller_spec.rb +++ b/spec/controllers/work_packages_controller_spec.rb @@ -345,7 +345,7 @@ describe WorkPackagesController do it 'should attach attachments if those are provided' do params[:attachments] = 'attachment-blubs-data' - Attachment.should_receive(:attach_files).with(stub_work_package, params[:attachments]) + stub_work_package.should_receive(:attach_files).with(params[:attachments]) controller.stub(:render_attachment_warning_if_needed) call_action @@ -760,4 +760,123 @@ describe WorkPackagesController do end end end + + let(:filename) { "test1.test" } + + describe :create do + let(:type) { FactoryGirl.create :type } + let(:project) { FactoryGirl.create :project, + types: [type] } + let(:status) { FactoryGirl.create :default_status } + let(:priority) { FactoryGirl.create :priority } + + context :attachments do + let(:new_work_package) { FactoryGirl.build(:work_package, + project: project, + type: type, + description: "Description", + priority: priority) } + let(:params) { { project_id: project.id, + attachments: { file: { file: filename, + description: '' } } } } + + before do + controller.stub(:work_package).and_return(new_work_package) + controller.should_receive(:authorize).and_return(true) + + Attachment.any_instance.stub(:filename).and_return(filename) + Attachment.any_instance.stub(:copy_file_to_destination) + end + + # see ticket #2009 on OpenProject.org + context "new attachment on new work package" do + before { post 'create', params } + + describe :journal do + let(:attachment_id) { "attachments_#{new_work_package.attachments.first.id}".to_sym } + + subject { new_work_package.journals.last.changed_data } + + it { should have_key attachment_id } + + it { subject[attachment_id].should eq([nil, filename]) } + end + end + + context "invalid attachment" do + let(:max_filesize) { Setting.attachment_max_size.to_i.kilobytes } + + before do + Attachment.any_instance.stub(:filesize).and_return(max_filesize + 1) + + post :create, params + end + + describe :view do + subject { response } + + it { should render_template('work_packages/new', formats: ["html"]) } + end + + describe :error do + subject { new_work_package.errors.messages } + + it { should have_key(:attachments) } + + it { subject[:attachments] =~ /too long/ } + end + end + end + end + + describe :update do + let(:type) { FactoryGirl.create :type } + let(:project) { FactoryGirl.create :project, + types: [type] } + let(:status) { FactoryGirl.create :default_status } + let(:priority) { FactoryGirl.create :priority } + + context :attachments do + let(:work_package) { FactoryGirl.build(:work_package, + project: project, + type: type, + description: "Description", + priority: priority) } + let(:params) { { id: work_package.id, + work_package: { attachments: { '1' => { file: filename, + description: '' } } } } } + + before do + controller.stub(:work_package).and_return(work_package) + controller.should_receive(:authorize).and_return(true) + + Attachment.any_instance.stub(:filename).and_return(filename) + Attachment.any_instance.stub(:copy_file_to_destination) + end + + context "invalid attachment" do + let(:max_filesize) { Setting.attachment_max_size.to_i.kilobytes } + + before do + Attachment.any_instance.stub(:filesize).and_return(max_filesize + 1) + + post :update, params + end + + describe :view do + subject { response } + + it { should render_template('work_packages/edit', formats: ["html"]) } + end + + describe :error do + subject { work_package.errors.messages } + + it { should have_key(:attachments) } + + it { subject[:attachments] =~ /too long/ } + end + end + end + end end diff --git a/spec/models/work_package_spec.rb b/spec/models/work_package_spec.rb index c6b5bea9e9..bd39aefef0 100644 --- a/spec/models/work_package_spec.rb +++ b/spec/models/work_package_spec.rb @@ -1047,9 +1047,9 @@ describe WorkPackage do raw_attachments = [double('attachment')] attachment = FactoryGirl.build(:attachment) - Attachment.should_receive(:attach_files) - .with(instance, raw_attachments) - .and_return(attachment) + instance.should_receive(:attach_files) + .with(raw_attachments) + .and_return(attachment) instance.update_by!(user, { :attachments => raw_attachments }) end