From 579620c22202b7c3c61985dc0639b1630505bb05 Mon Sep 17 00:00:00 2001 From: Marius BALTEANU Date: Tue, 7 Jan 2020 23:43:09 +0200 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 --- 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 efb55fafb..f43e6382c 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 332ecbf19..7fe182fdc 100644 --- a/app/views/issues/_attributes.html.erb +++ b/app/views/issues/_attributes.html.erb @@ -3,8 +3,13 @@
<% if @issue.safe_attribute?('status_id') && @allowed_statuses.present? %> -

<%= 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)" %>

+

+ <%= 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 %> + <%= @issue.transition_warning %> + <% end %> +

<%= hidden_field_tag 'was_default_status', @issue.status_id, :id => nil if @issue.status == @issue.default_status %> <% else %>

<%= @issue.status %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index 031c7ba87..5cead8cb4 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 4d0cb4ef1..a33d4a02d 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -5196,6 +5196,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 @@ -5307,6 +5308,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 3203e2a3f..713d4137c 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 @@ -3244,4 +3251,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.22.0