Merge pull request #835 from opf/fix/moves_controller_notes_processing_3401_3863

[FIX] Moves controller notes processing 3401 3863

app/controllers/work_packages/moves_controller.rb

@@ -46,8 +46,6 @@ class WorkPackages::MovesController < ApplicationController
     @work_packages.each do |work_package|
       work_package.reload
 
-      JournalManager.add_journal work_package, User.current, @notes || ""
-
       call_hook(:controller_work_packages_move_before_save, { :params => params, :work_package => work_package, :target_project => @target_project, :copy => !!@copy })
 
       permitted_params = params.permit(:copy,
@@ -62,7 +60,9 @@ class WorkPackages::MovesController < ApplicationController
                                        ids:[],
                                        status_id:[])
 
-      if r = work_package.move_to_project(@target_project, new_type, {:copy => @copy, :attributes => permitted_params})
+      if r = work_package.move_to_project(@target_project, new_type, { copy: @copy,
+                                                                       attributes: permitted_params,
+                                                                       journal_note: @notes })
         moved_work_packages << r
       else
         unsaved_work_package_ids << work_package.id

app/models/work_package.rb

@@ -664,10 +664,14 @@ class WorkPackage < ActiveRecord::Base
                             else
                               self.status
                             end
+    else
+      work_package.add_journal User.current, options[:journal_note] if options[:journal_note]
     end
 
     if work_package.save
-      unless options[:copy]
+      if options[:copy]
+        create_and_save_journal_note work_package, options[:journal_note]
+      else
         # Manually update project_id on related time entries
         TimeEntry.update_all("project_id = #{new_project.id}", {:work_package_id => id})
 
@@ -1007,4 +1011,11 @@ class WorkPackage < ActiveRecord::Base
       errors.messages[:attachments].first << " - #{invalid_attachment.errors.full_messages.first}"
     end
   end
+
+  def create_and_save_journal_note(work_package, journal_note)
+    if work_package && journal_note
+      work_package.add_journal User.current, journal_note
+      work_package.save!
+    end
+  end
 end

doc/CHANGELOG.md

@@ -44,12 +44,14 @@ See doc/COPYRIGHT.rdoc for more details.
 * `#3120` Implement a test suite the spikes can be developed against
 * `#3251` [Timelines] Filtering for Responsible filters everything
 * `#3393` [Timelines] Filter Work Packages by Assignee
+* `#3401` [Work package tracking] Notes are not saved when copying a work package
 * `#3409` New Layout for fallback Login page
 * `#3453` Highlight project in bread crumb
 * `#3546` Better icon for Timelines Module
 * `#3547` Change color of Apply button in Activity
 * `#3667` Better icon for Roadmap
 * `#3065` Fixed internal error when selecting costs-columns and displaying sums in work package list
+* `#3863` Strange additional journal entry when moving work package
 
 ## 3.0.0pre42
 

spec/controllers/work_packages/moves_controller_spec.rb

@@ -219,22 +219,40 @@ describe WorkPackages::MovesController do
         end
       end
 
-      context "with given note" do
-        let(:note) { "Moving two work packages" }
+      shared_examples_for 'single note for moved work package' do
+        it { expect(moved_work_package.journals.count).to eq(2) }
 
-        before do
-          post :create,
-               :ids => [work_package.id, work_package_2.id],
-               :notes => note
+        it { expect(moved_work_package.journals.sort_by(&:id).last.notes).to eq(note) }
+      end
 
-          work_package.reload
-          work_package_2.reload
+      describe "move with given note" do
+        let(:note) { "Moving a work package" }
+
+        context "w/o work package changes" do
+          before do
+            post :create,
+                 ids: [work_package.id],
+                 notes: note
+          end
+
+          it_behaves_like 'single note for moved work package' do
+            let(:moved_work_package) { work_package.reload }
+          end
         end
 
-        it "adds note to work packages" do
-          work_package.journals.sort_by(&:id).last.notes.should eq(note)
-          work_package_2.journals.sort_by(&:id).last.notes.should eq(note)
+        context "w/o work package changes" do
+          before do
+            post :create,
+                 ids: [work_package.id],
+                 notes: note,
+                 priority_id: target_priority.id
+          end
+
+          it_behaves_like 'single note for moved work package' do
+            let(:moved_work_package) { work_package.reload }
+          end
         end
+
       end
 
       describe '&copy' do
@@ -330,6 +348,23 @@ describe WorkPackages::MovesController do
             end
           end
         end
+
+        context "with given note" do
+          let(:note) { "Copying a work package" }
+
+          before do
+            post :create,
+                 ids: [work_package.id],
+                 copy: '',
+                 notes: note
+          end
+
+          subject { WorkPackage.all(limit: 1, order: 'id desc').last.journals }
+
+          it { expect(subject.count).to eq(2) }
+
+          it { expect(subject.sort_by(&:id).last.notes).to eq(note) }
+        end
       end
     end
   end