Handle time entries on subtasks and prevent from reassigning an issue that is going to be deleted (#24718, #24693).

git-svn-id: http://svn.redmine.org/redmine/trunk@16118 e93f8b46-1217-0410-a6f0-8f06a7374b81

app/controllers/issues_controller.rb

@@ -343,21 +343,28 @@ class IssuesController < ApplicationController
 
   def destroy
     raise Unauthorized unless @issues.all?(&:deletable?)
-    @hours = TimeEntry.where(:issue_id => @issues.map(&:id)).sum(:hours).to_f
+
+    # all issues and their descendants are about to be deleted
+    issues_and_descendants_ids = Issue.self_and_descendants(@issues).pluck(:id)
+    time_entries = TimeEntry.where(:issue_id => issues_and_descendants_ids)
+    @hours = time_entries.sum(:hours).to_f
+
     if @hours > 0
       case params[:todo]
       when 'destroy'
         # nothing to do
       when 'nullify'
-        TimeEntry.where(['issue_id IN (?)', @issues]).update_all('issue_id = NULL')
+        time_entries.update_all(:issue_id => nil)
       when 'reassign'
         reassign_to = @project.issues.find_by_id(params[:reassign_to_id])
         if reassign_to.nil?
           flash.now[:error] = l(:error_issue_not_found_in_project)
           return
+        elsif issues_and_descendants_ids.include?(reassign_to.id)
+          flash.now[:error] = l(:error_cannot_reassign_time_entries_to_an_issue_about_to_be_deleted)
+          return
         else
-          TimeEntry.where(['issue_id IN (?)', @issues]).
+          time_entries.update_all(:issue_id => reassign_to.id)
-            update_all("issue_id = #{reassign_to.id}")
         end
       else
         # display the destroy form if it's a user request

app/models/issue.rb

@@ -1102,6 +1102,15 @@ class Issue < ActiveRecord::Base
     end
   end
 
+  # Returns a scope of the given issues and their descendants
+  def self.self_and_descendants(issues)
+    issue_ids = Issue.joins("JOIN #{Issue.table_name} ancestors" +
+        " ON ancestors.root_id = #{Issue.table_name}.root_id" +
+        " AND ancestors.lft <= #{Issue.table_name}.lft AND ancestors.rgt >= #{Issue.table_name}.rgt"
+      ).
+      where(:ancestors => {:id => issues.map(&:id)})
+  end
+
   # Finds an issue relation given its id.
   def find_relation(relation_id)
     IssueRelation.where("issue_to_id = ? OR issue_from_id = ?", id, id).find(relation_id)

config/locales/en.yml

@@ -218,6 +218,7 @@ en:
   error_no_tracker_allowed_for_new_issue_in_project: "The project doesn't have any trackers for which you can create an issue"
   error_no_projects_with_tracker_allowed_for_new_issue: "There are no projects with trackers for which you can create an issue"
   error_move_of_child_not_possible: "Subtask %{child} could not be moved to the new project: %{errors}"
+  error_cannot_reassign_time_entries_to_an_issue_about_to_be_deleted: "Spent time cannot be reassigned to an issue that is about to be deleted"
 
   mail_subject_lost_password: "Your %{value} password"
   mail_body_lost_password: 'To change your password, click on the following link:'

config/locales/fr.yml

@@ -238,6 +238,7 @@ fr:
   error_no_tracker_allowed_for_new_issue_in_project: "Le projet ne dispose d'aucun tracker sur lequel vous pouvez créer une demande"
   error_no_projects_with_tracker_allowed_for_new_issue: "Aucun projet ne dispose d'un tracker sur lequel vous pouvez créer une demande"
   error_move_of_child_not_possible: "La sous-tâche %{child} n'a pas pu être déplacée dans le nouveau projet : %{errors}"
+  error_cannot_reassign_time_entries_to_an_issue_about_to_be_deleted: "Le temps passé ne peut pas être réaffecté à une demande qui va être supprimée"
 
   mail_subject_lost_password: "Votre mot de passe %{value}"
   mail_body_lost_password: 'Pour changer votre mot de passe, cliquez sur le lien suivant :'

test/functional/issues_controller_test.rb

@@ -4609,7 +4609,7 @@ class IssuesControllerTest < Redmine::ControllerTest
     assert_response :success
   end
 
-  def test_destroy_issue_with_no_time_entries
+  def test_destroy_issue_with_no_time_entries_should_delete_the_issues
     assert_nil TimeEntry.find_by_issue_id(2)
     @request.session[:user_id] = 2
 
@@ -4620,7 +4620,7 @@ class IssuesControllerTest < Redmine::ControllerTest
     assert_nil Issue.find_by_id(2)
   end
 
-  def test_destroy_issues_with_time_entries
+  def test_destroy_issues_with_time_entries_should_show_the_reassign_form
     @request.session[:user_id] = 2
 
     assert_no_difference 'Issue.count' do
@@ -4633,6 +4633,20 @@ class IssuesControllerTest < Redmine::ControllerTest
     end
   end
 
+  def test_destroy_issues_with_time_entries_should_show_hours_on_issues_and_descendants
+    parent = Issue.generate_with_child!
+    TimeEntry.generate!(:issue => parent)
+    TimeEntry.generate!(:issue => parent.children.first)
+    leaf = Issue.generate!
+    TimeEntry.generate!(:issue => leaf)
+    @request.session[:user_id] = 2
+
+    delete :destroy, :ids => [parent.id, leaf.id]
+    assert_response :success
+
+    assert_select 'p', :text => /3\.00 hours were reported/
+  end
+
   def test_destroy_issues_and_destroy_time_entries
     @request.session[:user_id] = 2
 
@@ -4674,6 +4688,24 @@ class IssuesControllerTest < Redmine::ControllerTest
     assert_equal 2, TimeEntry.find(2).issue_id
   end
 
+  def test_destroy_issues_with_time_entries_should_reassign_time_entries_of_issues_and_descendants
+    parent = Issue.generate_with_child!
+    TimeEntry.generate!(:issue => parent)
+    TimeEntry.generate!(:issue => parent.children.first)
+    leaf = Issue.generate!
+    TimeEntry.generate!(:issue => leaf)
+    target = Issue.generate!
+    @request.session[:user_id] = 2
+
+    assert_difference 'Issue.count', -3 do
+      assert_no_difference 'TimeEntry.count' do
+        delete :destroy, :ids => [parent.id, leaf.id], :todo => 'reassign', :reassign_to_id => target.id
+        assert_response 302
+      end
+    end
+    assert_equal 3, target.time_entries.count
+  end
+
   def test_destroy_issues_and_reassign_time_entries_to_an_invalid_issue_should_fail
     @request.session[:user_id] = 2
 
@@ -4686,6 +4718,18 @@ class IssuesControllerTest < Redmine::ControllerTest
     assert_response :success
   end
 
+  def test_destroy_issues_and_reassign_time_entries_to_an_issue_to_delete_should_fail
+    @request.session[:user_id] = 2
+
+    assert_no_difference 'Issue.count' do
+      assert_no_difference 'TimeEntry.count' do
+        delete :destroy, :ids => [1, 3], :todo => 'reassign', :reassign_to_id => 3
+      end
+    end
+    assert_response :success
+    assert_select '#flash_error', :text => I18n.t(:error_cannot_reassign_time_entries_to_an_issue_about_to_be_deleted)
+  end
+
   def test_destroy_issues_from_different_projects
     @request.session[:user_id] = 2