From 5917137d121bbca9a4a68f51beb4d59d87aa0651 Mon Sep 17 00:00:00 2001
From: Go MAEDA <maeda@farend.jp>
Date: Tue, 10 Mar 2020 03:50:47 +0000
Subject: [PATCH] Show warning and the reason when the issue cannot be closed
 or reopen because of open subtask(s), blocking issue(s) or closed parent
 issue (#31589).

Patch by Marius BALTEANU.


git-svn-id: http://svn.redmine.org/redmine/trunk@19570 e93f8b46-1217-0410-a6f0-8f06a7374b81
---
 app/models/issue.rb                       | 27 +++++++++++++++++++++--
 app/views/issues/_attributes.html.erb     |  9 ++++++--
 config/locales/en.yml                     |  3 +++
 test/functional/issues_controller_test.rb | 16 ++++++++++++++
 test/unit/issue_test.rb                   | 26 ++++++++++++++++++++++
 5 files changed, 77 insertions(+), 4 deletions(-)

diff --git a/app/models/issue.rb b/app/models/issue.rb
index f6a294ce4..487b1b552 100644
--- a/app/models/issue.rb
+++ b/app/models/issue.rb
@@ -56,6 +56,7 @@ class Issue < ActiveRecord::Base
 
   DONE_RATIO_OPTIONS = %w(issue_field issue_status)
 
+  attr_reader :transition_warning
   attr_writer :deleted_attachment_ids
   delegate :notes, :notes=, :private_notes, :private_notes=, :to => :current_journal, :allow_nil => true
 
@@ -969,6 +970,28 @@ class Issue < ActiveRecord::Base
     !relations_to.detect {|ir| ir.relation_type == 'blocks' && !ir.issue_from.closed?}.nil?
   end
 
+  # Returns true if this issue can be closed and if not, returns false and populates the reason
+  def closable?
+    if descendants.open.any?
+      @transition_warning = l(:notice_issue_not_closable_by_open_tasks)
+      return false
+    end
+    if blocked?
+      @transition_warning = l(:notice_issue_not_closable_by_blocking_issue)
+      return false
+    end
+    return true
+  end
+
+  # Returns true if this issue can be reopen and if not, returns false and populates the reason
+  def reopenable?
+    if ancestors.open(false).any?
+      @transition_warning = l(:notice_issue_not_reopenable_by_closed_parent_issue)
+      return false
+    end
+    return true
+  end
+
   # Returns the default status of the issue based on its tracker
   # Returns nil if tracker is nil
   def default_status
@@ -1008,11 +1031,11 @@ class Issue < ActiveRecord::Base
     statuses << default_status if include_default || (new_record? && statuses.empty?)
 
     statuses = statuses.compact.uniq.sort
-    if blocked? || descendants.open.any?
+    unless closable?
       # cannot close a blocked issue or a parent with open subtasks
       statuses.reject!(&:is_closed?)
     end
-    if ancestors.open(false).any?
+    unless reopenable?
       # cannot reopen a subtask of a closed parent
       statuses.select!(&:is_closed?)
     end
diff --git a/app/views/issues/_attributes.html.erb b/app/views/issues/_attributes.html.erb
index 0fc988576..ee4ae109c 100644
--- a/app/views/issues/_attributes.html.erb
+++ b/app/views/issues/_attributes.html.erb
@@ -3,8 +3,13 @@
 <div class="splitcontent">
 <div class="splitcontentleft">
 <% if @issue.safe_attribute?('status_id') && @allowed_statuses.present? %>
-<p><%= f.select :status_id, (@allowed_statuses.collect {|p| [p.name, p.id]}), {:required => true},
-                :onchange => "updateIssueFrom('#{escape_javascript(update_issue_form_path(@project, @issue))}', this)" %></p>
+<p>
+  <%= f.select :status_id, (@allowed_statuses.collect {|p| [p.name, p.id]}), {:required => true},
+                :onchange => "updateIssueFrom('#{escape_javascript(update_issue_form_path(@project, @issue))}', this)" %>
+  <% if @issue.transition_warning %>
+    <span class="icon-only icon-warning" title="<%= @issue.transition_warning %>"><%= @issue.transition_warning %></span>
+  <% end %>
+</p>
 <%= hidden_field_tag 'was_default_status', @issue.status_id, :id => nil if @issue.status == @issue.default_status %>
 <% else %>
 <p><label><%= l(:field_status) %></label> <%= @issue.status %></p>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index c4988e493..7bb506c57 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -191,6 +191,9 @@ en:
   notice_new_password_must_be_different: The new password must be different from the current password
   notice_import_finished: "%{count} items have been imported"
   notice_import_finished_with_errors: "%{count} out of %{total} items could not be imported"
+  notice_issue_not_closable_by_open_tasks: "This issue cannot be closed because it has at least one open subtask."
+  notice_issue_not_closable_by_blocking_issue: "This issue cannot be closed because it is blocked by at least one open issue."
+  notice_issue_not_reopenable_by_closed_parent_issue: "This issue cannot be reopened because its parent issue is closed."
 
   error_can_t_load_default_data: "Default configuration could not be loaded: %{value}"
   error_scm_not_found: "The entry or revision was not found in the repository."
diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb
index 3cd9d459b..cb8a6703b 100644
--- a/test/functional/issues_controller_test.rb
+++ b/test/functional/issues_controller_test.rb
@@ -5278,6 +5278,7 @@ class IssuesControllerTest < Redmine::ControllerTest
     assert_select 'select[name=?]', 'issue[priority_id]' do
       assert_select 'option[value="15"]', 0
     end
+    assert_select 'span.icon-warning', 0
   end
 
   def test_edit_should_hide_project_if_user_is_not_allowed_to_change_project
@@ -5389,6 +5390,21 @@ class IssuesControllerTest < Redmine::ControllerTest
     end
   end
 
+  def test_get_edit_for_issue_with_transition_warning_should_show_the_warning
+    @request.session[:user_id] = 2
+
+    get(
+      :edit,
+      :params => {
+        :id => 9,
+      }
+    )
+
+    assert_response :success
+    reason = l(:notice_issue_not_closable_by_blocking_issue)
+    assert_select 'span.icon-warning[title=?]', reason, :text => reason
+  end
+
   def test_update_form_for_existing_issue
     @request.session[:user_id] = 2
     patch(
diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb
index 3bd5927c0..aea76e34f 100644
--- a/test/unit/issue_test.rb
+++ b/test/unit/issue_test.rb
@@ -2104,6 +2104,10 @@ class IssueTest < ActiveSupport::TestCase
     child = Issue.generate!(:parent_issue_id => parent.id)
 
     allowed_statuses = parent.reload.new_statuses_allowed_to(users(:users_002))
+
+    assert !parent.closable?
+    assert_equal l(:notice_issue_not_closable_by_open_tasks), parent.transition_warning
+
     assert allowed_statuses.any?
     assert_equal [], allowed_statuses.select(&:is_closed?)
   end
@@ -2113,6 +2117,9 @@ class IssueTest < ActiveSupport::TestCase
     child = Issue.generate!(:parent_issue_id => parent.id, :status_id => 5)
 
     allowed_statuses = parent.reload.new_statuses_allowed_to(users(:users_002))
+
+    assert parent.closable?
+    assert_nil parent.transition_warning
     assert allowed_statuses.any?
     assert allowed_statuses.select(&:is_closed?).any?
   end
@@ -3285,4 +3292,23 @@ class IssueTest < ActiveSupport::TestCase
     User.current = user_in_asia
     assert issue.overdue?
   end
+
+  def test_closable
+    issue10 = Issue.find(10)
+    assert issue10.closable?
+    assert_nil issue10.transition_warning
+
+    # Issue blocked by another issue
+    issue9 = Issue.find(9)
+    assert !issue9.closable?
+    assert_equal l(:notice_issue_not_closable_by_blocking_issue), issue9.transition_warning
+  end
+
+  def test_reopenable
+    parent = Issue.generate!(:status_id => 5)
+    child = parent.generate_child!(:status_id => 5)
+
+    assert !child.reopenable?
+    assert_equal l(:notice_issue_not_reopenable_by_closed_parent_issue), child.transition_warning
+  end
 end
-- 
2.39.5