Fix TimeEntry cleanup on work package bulk delete

Fix `WorkPackage.cleanup_associated_before_destructing_if_required`
semantics so that the first argument can either be an individual
`WorkPackage` (as the spec originally expected) or a collection (as
in actual usages).

Signed-off-by: Alex Coles <alex@alexbcoles.com>

app/models/work_package/time_entries.rb

@@ -45,14 +45,16 @@ module WorkPackage::TimeEntries
         true
         # nothing to do
       when 'nullify'
+        work_packages = Array(work_packages)
         WorkPackage.update_time_entries(work_packages, 'work_package_id = NULL')
       when 'reassign'
+        work_packages = Array(work_packages)
         reassign_to = WorkPackage.includes(:project)
                       .where(Project.allowed_to_condition(user, :edit_time_entries))
                       .find_by(id: to_do[:reassign_to_id])
 
         if reassign_to.nil?
-          Array(work_packages).each do |wp|
+          work_packages.each do |wp|
             wp.errors.add(:base, :is_not_a_valid_target_for_time_entries, id: to_do[:reassign_to_id])
           end
 
@@ -66,7 +68,7 @@ module WorkPackage::TimeEntries
     end
 
     def update_time_entries(work_packages, action)
-      TimeEntry.update_all(action, ['work_package_id IN (?)', work_packages])
+      TimeEntry.update_all(action, ['work_package_id IN (?)', work_packages.map(&:id)])
     end
   end
 end

spec/models/work_package/ask_before_destruction_spec.rb

@@ -131,7 +131,7 @@ describe WorkPackage, type: :model do
     end
 
     describe 'w/o a cleanup beeing necessary' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'reassign') }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, action: 'reassign') }
 
       before do
         time_entry.destroy
@@ -143,7 +143,7 @@ describe WorkPackage, type: :model do
     end
 
     describe 'w/ "destroy" as action' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'destroy') }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, action: 'destroy') }
 
       it 'should return true' do
         expect(action).to be_truthy
@@ -158,7 +158,7 @@ describe WorkPackage, type: :model do
     end
 
     describe 'w/o an action' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user) }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user) }
 
       it 'should return true' do
         expect(action).to be_truthy
@@ -173,7 +173,7 @@ describe WorkPackage, type: :model do
     end
 
     describe 'w/ "nullify" as action' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'nullify') }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, action: 'nullify') }
 
       it 'should return true' do
         expect(action).to be_truthy
@@ -189,37 +189,69 @@ describe WorkPackage, type: :model do
 
     describe 'w/ "reassign" as action
               w/ reassigning to a valid work_package' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'reassign', reassign_to_id: work_package2.id) }
 
-      before do
-        work_package2.save!
-        role2.permissions << :edit_time_entries
-        role2.save!
-        member2.save!
-      end
+      context 'with a single work package' do
+        let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'reassign', reassign_to_id: work_package2.id) }
 
-      it 'should return true' do
-        expect(action).to be_truthy
-      end
+        before do
+          work_package2.save!
+          role2.permissions << :edit_time_entries
+          role2.save!
+          member2.save!
+        end
 
-      it 'should set the work_package_id of all time entries to the new work package' do
-        action
+        it 'should return true' do
+          expect(action).to be_truthy
+        end
 
-        time_entry.reload
-        expect(time_entry.work_package_id).to eq(work_package2.id)
+        it 'should set the work_package_id of all time entries to the new work package' do
+          action
+
+          time_entry.reload
+          expect(time_entry.work_package_id).to eq(work_package2.id)
+        end
+
+        it "should set the project_id of all time entries to the new work package's project" do
+          action
+
+          time_entry.reload
+          expect(time_entry.project_id).to eq(work_package2.project_id)
+        end
       end
 
-      it "should set the project_id of all time entries to the new work package's project" do
-        action
+      context 'with a collection of work packages' do
+        let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, action: 'reassign', reassign_to_id: work_package2.id) }
 
-        time_entry.reload
-        expect(time_entry.project_id).to eq(work_package2.project_id)
+        before do
+          work_package2.save!
+          role2.permissions << :edit_time_entries
+          role2.save!
+          member2.save!
+        end
+
+        it 'should return true' do
+          expect(action).to be_truthy
+        end
+
+        it 'should set the work_package_id of all time entries to the new work package' do
+          action
+
+          time_entry.reload
+          expect(time_entry.work_package_id).to eq(work_package2.id)
+        end
+
+        it "should set the project_id of all time entries to the new work package's project" do
+          action
+
+          time_entry.reload
+          expect(time_entry.project_id).to eq(work_package2.project_id)
+        end
       end
     end
 
     describe 'w/ "reassign" as action
               w/ reassigning to a work_package the user is not allowed to see' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'reassign', reassign_to_id: work_package2.id) }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, action: 'reassign', reassign_to_id: work_package2.id) }
 
       before do
         work_package2.save!
@@ -239,7 +271,7 @@ describe WorkPackage, type: :model do
 
     describe 'w/ "reassign" as action
               w/ reassigning to a non existing work package' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'reassign', reassign_to_id: 0) }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, action: 'reassign', reassign_to_id: 0) }
 
       it 'should return true' do
         expect(action).to be_falsey
@@ -261,7 +293,7 @@ describe WorkPackage, type: :model do
 
     describe 'w/ "reassign" as action
               w/o providing a reassignment id' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'reassign') }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, action: 'reassign') }
 
       it 'should return true' do
         expect(action).to be_falsey
@@ -282,7 +314,7 @@ describe WorkPackage, type: :model do
     end
 
     describe 'w/ an invalid option' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, action: 'bogus') }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, action: 'bogus') }
 
       it 'should return false' do
         expect(action).to be_falsey
@@ -290,7 +322,7 @@ describe WorkPackage, type: :model do
     end
 
     describe 'w/ nil as invalid option' do
-      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required(work_package, user, nil) }
+      let(:action) { WorkPackage.cleanup_associated_before_destructing_if_required([work_package], user, nil) }
 
       it 'should return false' do
         expect(action).to be_falsey