Merge pull request #512 from opf/bug/no_journal_entry_for_attachments_on_creation

[BUG] No journal entry for attachments on creation

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))

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)
+    unless new_record?
       add_journal
-    save!
+      save
+    end
   end
 
   # ACTS AS JOURNALIZED
@@ -407,13 +411,9 @@ class WorkPackage < ActiveRecord::Base
 
     update_by(user, attributes)
 
-    if save
+    attach_files(raw_attachments)
-      # as attach_files always saves an attachment right away
+
-      # it is not possible to stage attaching and check for
+    save
-      # valid. If this would be possible, we could check
-      # for this along with update_attributes
-      attachments = Attachment.attach_files(self, raw_attachments)
-    end
   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

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

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

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

spec/models/work_package_spec.rb

@@ -1047,8 +1047,8 @@ describe WorkPackage do
       raw_attachments = [double('attachment')]
       attachment = FactoryGirl.build(:attachment)
 
-      Attachment.should_receive(:attach_files)
+      instance.should_receive(:attach_files)
-                .with(instance, raw_attachments)
+              .with(raw_attachments)
               .and_return(attachment)
 
       instance.update_by!(user, { :attachments => raw_attachments })