From 2bc5b60f9de7c533b33348213f6ddac37dc9cdb3 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sun, 26 Jul 2015 08:30:19 +0000 Subject: [PATCH] Makes new issue initial status settable in workflow (#5816). git-svn-id: http://svn.redmine.org/redmine/trunk@14458 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/issues_controller.rb | 12 +++--- app/controllers/workflows_controller.rb | 4 +- app/helpers/workflows_helper.rb | 6 +-- app/models/issue.rb | 25 ++++++----- app/models/issue_status.rb | 22 +++------- app/models/workflow_permission.rb | 1 + app/models/workflow_rule.rb | 2 +- app/views/workflows/_form.html.erb | 9 ++-- ..._insert_allowed_statuses_for_new_issues.rb | 23 ++++++++++ test/fixtures/workflows.yml | 42 +++++++++++++++++++ test/functional/issues_controller_test.rb | 37 ++++++++++++++-- test/functional/workflows_controller_test.rb | 24 +++++++++++ test/unit/role_test.rb | 5 ++- test/unit/tracker_test.rb | 5 ++- 14 files changed, 168 insertions(+), 49 deletions(-) create mode 100644 db/migrate/20150725112753_insert_allowed_statuses_for_new_issues.rb diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index 80f02409a..4238215eb 100644 --- a/app/controllers/issues_controller.rb +++ b/app/controllers/issues_controller.rb @@ -427,12 +427,12 @@ class IssuesController < ApplicationController @issue.author ||= User.current @issue.start_date ||= Date.today if Setting.default_issue_start_date_to_creation_date? - if attrs = params[:issue].deep_dup - if action_name == 'new' && params[:was_default_status] == attrs[:status_id] - attrs.delete(:status_id) - end - @issue.safe_attributes = attrs + attrs = (params[:issue] || {}).deep_dup + if action_name == 'new' && params[:was_default_status] == attrs[:status_id] + attrs.delete(:status_id) end + @issue.safe_attributes = attrs + if @issue.project @issue.tracker ||= @issue.project.trackers.first if @issue.tracker.nil? @@ -446,7 +446,7 @@ class IssuesController < ApplicationController end @priorities = IssuePriority.active - @allowed_statuses = @issue.new_statuses_allowed_to(User.current, @issue.new_record?) + @allowed_statuses = @issue.new_statuses_allowed_to(User.current) end def parse_params_for_bulk_issue_attributes(params) diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb index af8c2750b..6f8a65f9c 100644 --- a/app/controllers/workflows_controller.rb +++ b/app/controllers/workflows_controller.rb @@ -43,7 +43,9 @@ class WorkflowsController < ApplicationController end if @trackers && @roles && @statuses.any? - workflows = WorkflowTransition.where(:role_id => @roles.map(&:id), :tracker_id => @trackers.map(&:id)) + workflows = WorkflowTransition. + where(:role_id => @roles.map(&:id), :tracker_id => @trackers.map(&:id)). + preload(:old_status, :new_status) @workflows = {} @workflows['always'] = workflows.select {|w| !w.author && !w.assignee} @workflows['author'] = workflows.select {|w| w.author} diff --git a/app/helpers/workflows_helper.rb b/app/helpers/workflows_helper.rb index 4adaa5067..cfd7a3669 100644 --- a/app/helpers/workflows_helper.rb +++ b/app/helpers/workflows_helper.rb @@ -75,14 +75,14 @@ module WorkflowsHelper end def transition_tag(workflows, old_status, new_status, name) - w = workflows.select {|w| w.old_status_id == old_status.id && w.new_status_id == new_status.id}.size + w = workflows.select {|w| w.old_status == old_status && w.new_status == new_status}.size - tag_name = "transitions[#{ old_status.id }][#{new_status.id}][#{name}]" + tag_name = "transitions[#{ old_status.try(:id) || 0 }][#{new_status.id}][#{name}]" if w == 0 || w == @roles.size * @trackers.size hidden_field_tag(tag_name, "0", :id => nil) + check_box_tag(tag_name, "1", w != 0, - :class => "old-status-#{old_status.id} new-status-#{new_status.id}") + :class => "old-status-#{old_status.try(:id) || 0} new-status-#{new_status.id}") else select_tag tag_name, options_for_select([ diff --git a/app/models/issue.rb b/app/models/issue.rb index e0f93faef..a4a7614d4 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -465,11 +465,15 @@ class Issue < ActiveRecord::Base self.tracker ||= project.trackers.first end + statuses_allowed = new_statuses_allowed_to(user) if (s = attrs.delete('status_id')) && safe_attribute?('status_id') - if new_statuses_allowed_to(user).collect(&:id).include?(s.to_i) + if statuses_allowed.collect(&:id).include?(s.to_i) self.status_id = s end end + if new_record? && !statuses_allowed.include?(status) + self.status = statuses_allowed.first || default_status + end attrs = delete_unsafe_attributes(attrs, user) return if attrs.empty? @@ -825,7 +829,7 @@ class Issue < ActiveRecord::Base else initial_status = nil if new_record? - initial_status = default_status + # nop elsif tracker_id_changed? if Tracker.where(:id => tracker_id_was, :default_status_id => status_id_was).any? initial_status = default_status @@ -843,16 +847,15 @@ class Issue < ActiveRecord::Base (user.id == initial_assigned_to_id || user.group_ids.include?(initial_assigned_to_id)) statuses = [] - if initial_status - statuses += initial_status.find_new_statuses_allowed_to( - user.admin ? Role.all.to_a : user.roles_for_project(project), - tracker, - author == user, - assignee_transitions_allowed - ) - end + statuses += IssueStatus.new_statuses_allowed( + initial_status, + user.admin ? Role.all.to_a : user.roles_for_project(project), + tracker, + author == user, + assignee_transitions_allowed + ) statuses << initial_status unless statuses.empty? - statuses << default_status if include_default + statuses << default_status if include_default || (new_record? && statuses.empty?) statuses = statuses.compact.uniq.sort if blocked? statuses.reject!(&:is_closed?) diff --git a/app/models/issue_status.rb b/app/models/issue_status.rb index 4e9b2ce87..e39d69a76 100644 --- a/app/models/issue_status.rb +++ b/app/models/issue_status.rb @@ -45,28 +45,18 @@ class IssueStatus < ActiveRecord::Base end # Returns an array of all statuses the given role can switch to - # Uses association cache when called more than one time def new_statuses_allowed_to(roles, tracker, author=false, assignee=false) - if roles && tracker - role_ids = roles.collect(&:id) - transitions = workflows.select do |w| - role_ids.include?(w.role_id) && - w.tracker_id == tracker.id && - ((!w.author && !w.assignee) || (author && w.author) || (assignee && w.assignee)) - end - transitions.map(&:new_status).compact.sort - else - [] - end + self.class.new_statuses_allowed(self, roles, tracker, author, assignee) end + alias :find_new_statuses_allowed_to :new_statuses_allowed_to - # Same thing as above but uses a database query - # More efficient than the previous method if called just once - def find_new_statuses_allowed_to(roles, tracker, author=false, assignee=false) + def self.new_statuses_allowed(status, roles, tracker, author=false, assignee=false) if roles.present? && tracker + status_id = status.try(:id) || 0 + scope = IssueStatus. joins(:workflow_transitions_as_new_status). - where(:workflows => {:old_status_id => id, :role_id => roles.map(&:id), :tracker_id => tracker.id}) + where(:workflows => {:old_status_id => status_id, :role_id => roles.map(&:id), :tracker_id => tracker.id}) unless author && assignee if author || assignee diff --git a/app/models/workflow_permission.rb b/app/models/workflow_permission.rb index 6a451fd38..095d75b1c 100644 --- a/app/models/workflow_permission.rb +++ b/app/models/workflow_permission.rb @@ -17,6 +17,7 @@ class WorkflowPermission < WorkflowRule validates_inclusion_of :rule, :in => %w(readonly required) + validates_presence_of :old_status validate :validate_field_name # Returns the workflow permissions for the given trackers and roles diff --git a/app/models/workflow_rule.rb b/app/models/workflow_rule.rb index 25f76d7b2..976713a16 100644 --- a/app/models/workflow_rule.rb +++ b/app/models/workflow_rule.rb @@ -23,7 +23,7 @@ class WorkflowRule < ActiveRecord::Base belongs_to :old_status, :class_name => 'IssueStatus' belongs_to :new_status, :class_name => 'IssueStatus' - validates_presence_of :role, :tracker, :old_status + validates_presence_of :role, :tracker attr_protected :id # Copies workflows from source to targets diff --git a/app/views/workflows/_form.html.erb b/app/views/workflows/_form.html.erb index a788a7a68..76f6fefe3 100644 --- a/app/views/workflows/_form.html.erb +++ b/app/views/workflows/_form.html.erb @@ -20,16 +20,17 @@ - <% for old_status in @statuses %> + <% for old_status in [nil] + @statuses %> + <% next if old_status.nil? && name != 'always' %> "> - <%= link_to_function(image_tag('toggle_check.png'), "toggleCheckboxesBySelector('table.transitions-#{name} input.old-status-#{old_status.id}')", + <%= link_to_function(image_tag('toggle_check.png'), "toggleCheckboxesBySelector('table.transitions-#{name} input.old-status-#{old_status.try(:id) || 0}')", :title => "#{l(:button_check_all)}/#{l(:button_uncheck_all)}") %> - <%= old_status.name %> + <%= old_status ? old_status.name : content_tag('em', l(:label_issue_new)) %> <% for new_status in @statuses -%> - <% checked = workflows.detect {|w| w.old_status_id == old_status.id && w.new_status_id == new_status.id} %> + <% checked = workflows.detect {|w| w.old_status == old_status && w.new_status == new_status} %> <%= transition_tag workflows, old_status, new_status, name %> diff --git a/db/migrate/20150725112753_insert_allowed_statuses_for_new_issues.rb b/db/migrate/20150725112753_insert_allowed_statuses_for_new_issues.rb new file mode 100644 index 000000000..dec3bdda1 --- /dev/null +++ b/db/migrate/20150725112753_insert_allowed_statuses_for_new_issues.rb @@ -0,0 +1,23 @@ +class InsertAllowedStatusesForNewIssues < ActiveRecord::Migration + def self.up + # Adds the default status for all trackers and roles + sql = "INSERT INTO #{WorkflowTransition.table_name} (tracker_id, old_status_id, new_status_id, role_id, type)" + + " SELECT t.id, 0, t.default_status_id, r.id, 'WorkflowTransition'" + + " FROM #{Tracker.table_name} t, #{Role.table_name} r" + WorkflowTransition.connection.execute(sql) + + # Adds other statuses that are reachable with one transition + # to preserve previous behaviour as default + sql = "INSERT INTO #{WorkflowTransition.table_name} (tracker_id, old_status_id, new_status_id, role_id, type)" + + " SELECT t.id, 0, w.new_status_id, w.role_id, 'WorkflowTransition'" + + " FROM #{Tracker.table_name} t" + + " JOIN #{IssueStatus.table_name} s on s.id = t.default_status_id" + + " JOIN #{WorkflowTransition.table_name} w on w.tracker_id = t.id and w.old_status_id = s.id and w.type = 'WorkflowTransition'" + + " WHERE w.new_status_id <> t.default_status_id" + WorkflowTransition.connection.execute(sql) + end + + def self.down + WorkflowTransition.where(:old_status_id => 0).delete_all + end +end diff --git a/test/fixtures/workflows.yml b/test/fixtures/workflows.yml index d544545b3..7eb5481bb 100644 --- a/test/fixtures/workflows.yml +++ b/test/fixtures/workflows.yml @@ -1882,3 +1882,45 @@ WorkflowTransitions_188: id: 188 tracker_id: 3 type: WorkflowTransition +WorkflowTransitions_271: + new_status_id: 3 + role_id: 1 + old_status_id: 0 + id: 271 + tracker_id: 2 + type: WorkflowTransition +WorkflowTransitions_272: + new_status_id: 3 + role_id: 2 + old_status_id: 0 + id: 272 + tracker_id: 1 + type: WorkflowTransition +WorkflowTransitions_273: + new_status_id: 2 + role_id: 1 + old_status_id: 0 + id: 273 + tracker_id: 3 + type: WorkflowTransition +WorkflowTransitions_274: + new_status_id: 2 + role_id: 1 + old_status_id: 0 + id: 274 + tracker_id: 1 + type: WorkflowTransition +WorkflowTransitions_275: + new_status_id: 1 + role_id: 1 + old_status_id: 0 + id: 275 + tracker_id: 1 + type: WorkflowTransition +WorkflowTransitions_276: + new_status_id: 1 + role_id: 1 + old_status_id: 0 + id: 276 + tracker_id: 2 + type: WorkflowTransition diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index f642d582d..4aa89c64e 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -1602,6 +1602,37 @@ class IssuesControllerTest < ActionController::TestCase assert_select 'input[name=was_default_status][value="1"]' end + def test_new_should_propose_allowed_statuses + WorkflowTransition.delete_all + WorkflowTransition.create!(:tracker_id => 1, :role_id => 1, :old_status_id => 0, :new_status_id => 1) + WorkflowTransition.create!(:tracker_id => 1, :role_id => 1, :old_status_id => 0, :new_status_id => 3) + @request.session[:user_id] = 2 + + get :new, :project_id => 1 + assert_response :success + assert_select 'select[name=?]', 'issue[status_id]' do + assert_select 'option[value="1"]' + assert_select 'option[value="3"]' + assert_select 'option', 2 + assert_select 'option[value="1"][selected=selected]' + end + end + + def test_new_should_propose_allowed_statuses_without_default_status_allowed + WorkflowTransition.delete_all + WorkflowTransition.create!(:tracker_id => 1, :role_id => 1, :old_status_id => 0, :new_status_id => 2) + assert_equal 1, Tracker.find(1).default_status_id + @request.session[:user_id] = 2 + + get :new, :project_id => 1 + assert_response :success + assert_select 'select[name=?]', 'issue[status_id]' do + assert_select 'option[value="2"]' + assert_select 'option', 1 + assert_select 'option[value="2"][selected=selected]' + end + end + def test_get_new_with_list_custom_field @request.session[:user_id] = 2 get :new, :project_id => 1, :tracker_id => 1 @@ -1827,8 +1858,8 @@ class IssuesControllerTest < ActionController::TestCase def test_update_form_for_new_issue_should_propose_transitions_based_on_initial_status @request.session[:user_id] = 2 WorkflowTransition.delete_all - WorkflowTransition.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 2) - WorkflowTransition.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 5) + WorkflowTransition.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 0, :new_status_id => 2) + WorkflowTransition.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 0, :new_status_id => 5) WorkflowTransition.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 5, :new_status_id => 4) xhr :post, :new, :project_id => 1, @@ -1837,7 +1868,7 @@ class IssuesControllerTest < ActionController::TestCase :subject => 'This is an issue'} assert_equal 5, assigns(:issue).status_id - assert_equal [1,2,5], assigns(:allowed_statuses).map(&:id).sort + assert_equal [2,5], assigns(:allowed_statuses).map(&:id).sort end def test_update_form_with_default_status_should_ignore_submitted_status_id_if_equals diff --git a/test/functional/workflows_controller_test.rb b/test/functional/workflows_controller_test.rb index d59614aea..2f455c499 100644 --- a/test/functional/workflows_controller_test.rb +++ b/test/functional/workflows_controller_test.rb @@ -61,6 +61,16 @@ class WorkflowsControllerTest < ActionController::TestCase assert_select 'input[type=checkbox][name=?]', 'transitions[1][1][always]', 0 end + def test_get_edit_should_include_allowed_statuses_for_new_issues + WorkflowTransition.delete_all + WorkflowTransition.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 0, :new_status_id => 1) + + get :edit, :role_id => 1, :tracker_id => 1 + assert_response :success + assert_select 'td', 'New issue' + assert_select 'input[type=checkbox][name=?][value="1"][checked=checked]', 'transitions[0][1][always]' + end + def test_get_edit_with_all_roles_and_all_trackers get :edit, :role_id => 'all', :tracker_id => 'all' assert_response :success @@ -96,6 +106,20 @@ class WorkflowsControllerTest < ActionController::TestCase assert_nil WorkflowTransition.where(:role_id => 2, :tracker_id => 1, :old_status_id => 5, :new_status_id => 4).first end + def test_post_edit_with_allowed_statuses_for_new_issues + WorkflowTransition.delete_all + + post :edit, :role_id => 2, :tracker_id => 1, + :transitions => { + '0' => {'1' => {'always' => '1'}, '2' => {'always' => '1'}} + } + assert_response 302 + + assert WorkflowTransition.where(:role_id => 2, :tracker_id => 1, :old_status_id => 0, :new_status_id => 1).any? + assert WorkflowTransition.where(:role_id => 2, :tracker_id => 1, :old_status_id => 0, :new_status_id => 2).any? + assert_equal 2, WorkflowTransition.where(:tracker_id => 1, :role_id => 2).count + end + def test_post_edit_with_additional_transitions WorkflowTransition.delete_all diff --git a/test/unit/role_test.rb b/test/unit/role_test.rb index f2107ad23..2dc99864a 100644 --- a/test/unit/role_test.rb +++ b/test/unit/role_test.rb @@ -47,13 +47,14 @@ class RoleTest < ActiveSupport::TestCase def test_copy_workflows source = Role.find(1) - assert_equal 90, source.workflow_rules.size + rule_count = source.workflow_rules.count + assert rule_count > 0 target = Role.new(:name => 'Target') assert target.save target.workflow_rules.copy(source) target.reload - assert_equal 90, target.workflow_rules.size + assert_equal rule_count, target.workflow_rules.size end def test_permissions_should_be_unserialized_with_its_coder diff --git a/test/unit/tracker_test.rb b/test/unit/tracker_test.rb index d9a445133..f766a3ab4 100644 --- a/test/unit/tracker_test.rb +++ b/test/unit/tracker_test.rb @@ -30,13 +30,14 @@ class TrackerTest < ActiveSupport::TestCase def test_copy_workflows source = Tracker.find(1) - assert_equal 89, source.workflow_rules.size + rules_count = source.workflow_rules.count + assert rules_count > 0 target = Tracker.new(:name => 'Target', :default_status_id => 1) assert target.save target.workflow_rules.copy(source) target.reload - assert_equal 89, target.workflow_rules.size + assert_equal rules_count, target.workflow_rules.size end def test_issue_statuses -- 2.39.5