From df77501528297a548e61f4d75881aa98afe72427 Mon Sep 17 00:00:00 2001 From: Mischa The Evil Date: Mon, 11 May 2020 07:26:54 +0200 Subject: [PATCH] Fix #33415 by re-implementing #31589. Todo: * fix (superfluous) test code * fix (any) inconsistently named variables, labels and t9n's * fix (any) inconsistent code styles and markup --- app/helpers/issues_helper.rb | 18 ++++++++++ app/models/issue.rb | 35 ++++++++++++------ app/views/issues/_attributes.html.erb | 4 +-- config/locales/en.yml | 1 + test/functional/issues_controller_test.rb | 43 ++++++++++++++++++++++- test/unit/issue_test.rb | 43 ++++++++++++++++------- 6 files changed, 118 insertions(+), 26 deletions(-) diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 6b63f7e11..e86efeea9 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -640,4 +640,22 @@ module IssuesHelper end end + # Returns the transition warning message + # Returns nil if @transition_warnings is nil + def transition_warning_message(issue) + tw = issue.transition_warnings + if !tw.nil? + if tw.include?(:not_closable_by_open_sub_tasks_and_blocking_issues) + s = l(:notice_issue_not_closable_by_open_tasks_and_blocking_issue) + elsif tw.include?(:not_closable_by_open_sub_tasks) + s = l(:notice_issue_not_closable_by_open_tasks) + elsif tw.include?(:not_closable_by_open_blocking_issues) + s = l(:notice_issue_not_closable_by_blocking_issue) + elsif tw.include?(:not_reopenable_by_closed_parent_issue) + s = l(:notice_issue_not_reopenable_by_closed_parent_issue) + end + return s + end + end + end diff --git a/app/models/issue.rb b/app/models/issue.rb index 487b1b552..ff80d8de0 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -56,7 +56,7 @@ class Issue < ActiveRecord::Base DONE_RATIO_OPTIONS = %w(issue_field issue_status) - attr_reader :transition_warning + attr_reader :transition_warnings attr_writer :deleted_attachment_ids delegate :notes, :notes=, :private_notes, :private_notes=, :to => :current_journal, :allow_nil => true @@ -971,25 +971,40 @@ class Issue < ActiveRecord::Base end # Returns true if this issue can be closed and if not, returns false and populates the reason + # This does not take the workflow transitions into account def closable? + @transition_warnings ||= [] if descendants.open.any? - @transition_warning = l(:notice_issue_not_closable_by_open_tasks) - return false + open_sub_tasks = true end if blocked? - @transition_warning = l(:notice_issue_not_closable_by_blocking_issue) - return false + open_blocking_issues = true end - return true + + if (open_sub_tasks && open_blocking_issues) && !@transition_warnings.include?(:not_closable_by_open_sub_tasks_and_blocking_issues) + @transition_warnings << :not_closable_by_open_sub_tasks_and_blocking_issues + elsif open_sub_tasks && !@transition_warnings.include?(:not_closable_by_open_sub_tasks) + @transition_warnings << :not_closable_by_open_sub_tasks + elsif open_blocking_issues && !@transition_warnings.include?(:not_closable_by_open_blocking_issues) + @transition_warnings << :not_closable_by_open_blocking_issues + end + + open_sub_tasks || open_blocking_issues ? (return false) : (return true) end - # Returns true if this issue can be reopen and if not, returns false and populates the reason + # Returns true if this issue can be reopened and if not, returns false and populates the reason + # This does not take the workflow transitions into account def reopenable? + @transition_warnings ||= [] if ancestors.open(false).any? - @transition_warning = l(:notice_issue_not_reopenable_by_closed_parent_issue) - return false + closed_parent_issue = true end - return true + + if closed_parent_issue && !@transition_warnings.include?(:not_reopenable_by_closed_parent_issue) + @transition_warnings << :not_reopenable_by_closed_parent_issue + end + + closed_parent_issue ? (return false) : (return true) end # Returns the default status of the issue based on its tracker diff --git a/app/views/issues/_attributes.html.erb b/app/views/issues/_attributes.html.erb index ee4ae109c..60cb952f4 100644 --- a/app/views/issues/_attributes.html.erb +++ b/app/views/issues/_attributes.html.erb @@ -6,8 +6,8 @@

<%= 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 %> + <% if @issue.transition_warnings %> + <%= transition_warning_message(@issue) %> <% end %>

<%= hidden_field_tag 'was_default_status', @issue.status_id, :id => nil if @issue.status == @issue.default_status %> diff --git a/config/locales/en.yml b/config/locales/en.yml index 89fd151c4..8e2b416fd 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -193,6 +193,7 @@ en: 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." + notice_issue_not_closable_by_open_tasks_and_blocking_issue: "This issue cannot be closed because it has at least one open subtask and it is blocked by at least one open issue." 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 a33605bf9..bf3f0eb86 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -5439,7 +5439,20 @@ class IssuesControllerTest < Redmine::ControllerTest end def test_get_edit_for_issue_with_transition_warning_should_show_the_warning + # Transition warning message for open issue blocked by another open issue @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 + + # Transition warning message for open issue with open subtask and blocked by another open issue + subissue1 = Issue.generate!(:project_id => 5, :author_id => 2, :tracker_id => 1, :parent_issue_id => 9, :subject => 'Open Child Issue') get( :edit, @@ -5447,9 +5460,37 @@ class IssuesControllerTest < Redmine::ControllerTest :id => 9, } ) + assert_response :success + reason = l(:notice_issue_not_closable_by_open_tasks_and_blocking_issue) + assert_select 'span.icon-warning[title=?]', reason, :text => reason + # Transition warning message for open issue with open subtask + subissue2 = Issue.generate!(:project_id => 5, :author_id => 2, :tracker_id => 1, :parent_issue_id => subissue1.id, :subject => 'Open Child Issue') + + get( + :edit, + :params => { + :id => subissue1.id, + } + ) assert_response :success - reason = l(:notice_issue_not_closable_by_blocking_issue) + reason = l(:notice_issue_not_closable_by_open_tasks) + assert_select 'span.icon-warning[title=?]', reason, :text => reason + + # Transition warning message for closed subtask with closed parent issue + subissue1.update_attribute :status_id, 5 + assert subissue1.closed? + subissue3 = Issue.generate!(:project_id => 5, :author_id => 2, :tracker_id => 1, :status_id => 5, :parent_issue_id => subissue1.id, :subject => 'Closed Child Issue') + assert subissue3.closed? + + get( + :edit, + :params => { + :id => subissue3.id, + } + ) + assert_response :success + reason = l(:notice_issue_not_reopenable_by_closed_parent_issue) assert_select 'span.icon-warning[title=?]', reason, :text => reason end diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb index d82f6e079..f7e03c33c 100644 --- a/test/unit/issue_test.rb +++ b/test/unit/issue_test.rb @@ -2105,9 +2105,6 @@ class IssueTest < ActiveSupport::TestCase 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 @@ -2118,8 +2115,6 @@ class IssueTest < ActiveSupport::TestCase 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 @@ -3296,19 +3291,41 @@ class IssueTest < ActiveSupport::TestCase def test_closable issue10 = Issue.find(10) assert issue10.closable? - assert_nil issue10.transition_warning + assert_empty(issue10.transition_warnings) - # Issue blocked by another issue + # Issue blocked by another open issue issue9 = Issue.find(9) assert !issue9.closable? - assert_equal l(:notice_issue_not_closable_by_blocking_issue), issue9.transition_warning + assert_includes(issue9.transition_warnings, :not_closable_by_open_blocking_issues) + + # Issue with an open subtask and blocked by another open issue + child = issue9.generate_child!(:status_id => 1) + issue9.reload + assert !issue9.closable? + assert_includes(issue9.transition_warnings, :not_closable_by_open_sub_tasks_and_blocking_issues) + + # Issue with an open subtask + assert child.closable? + open_parent_issue = child.generate_child! + child.reload + assert !child.closable? + assert_includes(child.transition_warnings, :not_closable_by_open_sub_tasks) + + # Closed issue should be closable without a transition warning message + closed_issue = Issue.generate!(:status_id => 5) + assert closed_issue.closable? + assert_empty(closed_issue.transition_warnings) 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 + closed_parent = Issue.generate!(:status_id => 5) + closed_child = closed_parent.generate_child!(:status_id => 5) + assert !closed_child.reopenable? + assert_includes(closed_child.transition_warnings, :not_reopenable_by_closed_parent_issue) + + # Open issue should be reopenable without a transition warning message + open_issue = Issue.generate!(:status_id => 1) + assert open_issue.reopenable? + assert_empty(open_issue.transition_warnings) end end -- 2.26.0.windows.1