]> source.dussan.org Git - redmine.git/commitdiff
Makes new issue initial status settable in workflow (#5816).
authorJean-Philippe Lang <jp_lang@yahoo.fr>
Sun, 26 Jul 2015 08:30:19 +0000 (08:30 +0000)
committerJean-Philippe Lang <jp_lang@yahoo.fr>
Sun, 26 Jul 2015 08:30:19 +0000 (08:30 +0000)
git-svn-id: http://svn.redmine.org/redmine/trunk@14458 e93f8b46-1217-0410-a6f0-8f06a7374b81

14 files changed:
app/controllers/issues_controller.rb
app/controllers/workflows_controller.rb
app/helpers/workflows_helper.rb
app/models/issue.rb
app/models/issue_status.rb
app/models/workflow_permission.rb
app/models/workflow_rule.rb
app/views/workflows/_form.html.erb
db/migrate/20150725112753_insert_allowed_statuses_for_new_issues.rb [new file with mode: 0644]
test/fixtures/workflows.yml
test/functional/issues_controller_test.rb
test/functional/workflows_controller_test.rb
test/unit/role_test.rb
test/unit/tracker_test.rb

index 80f02409aaae032841d9c76f60e61de00dad0e37..4238215eb93b2a309d6c0ffa2bdc2c42ac501ab8 100644 (file)
@@ -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)
index af8c2750bab1cb125c52820de07934d34d3cee76..6f8a65f9ca8eef018e261142028ac9b68f7dbdd7 100644 (file)
@@ -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}
index 4adaa50675c01c0683e1009a913edf6b87d90de1..cfd7a3669648f5e96bc2fff9fc321cac8b560315 100644 (file)
@@ -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([
index e0f93faef19c3e48c57b8fe6a2765e97089e7fe9..a4a7614d4c4e8ae8b406143443dc7f8cb65e602a 100644 (file)
@@ -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?)
index 4e9b2ce87e7ef307cfe597cc64c1c02fb7e3f585..e39d69a765fdaaab984665171604635e7ee1b324 100644 (file)
@@ -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
index 6a451fd383a7610e23c512e31c9cb39d75dcbeeb..095d75b1c53ade113b90942fc329ed8d4096398f 100644 (file)
@@ -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
index 25f76d7b25e61008e7e862c942c30ba5ad25f965..976713a16dfff1ffabd1d267ff957b8652466557 100644 (file)
@@ -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
index a788a7a68e507ef609567a7136d62940631f5433..76f6fefe3722b6547c7d36b3c78a4f13019f6764 100644 (file)
   </tr>
 </thead>
 <tbody>
-  <% for old_status in @statuses %>
+  <% for old_status in [nil] + @statuses %>
+  <% next if old_status.nil? && name != 'always' %>
   <tr class="<%= cycle("odd", "even") %>">
     <td class="name">
-      <%= 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)) %>
     </td>
     <% 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} %>
     <td class="<%= checked ? 'enabled' : '' %>">
       <%= transition_tag workflows, old_status, new_status, name %>
     </td>
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 (file)
index 0000000..dec3bdd
--- /dev/null
@@ -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
index d544545b33122642826db299e3d25074e68a272c..7eb5481bb74d0b74d42c740aaff0e15e1e23e8f5 100644 (file)
@@ -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
index f642d582d4e2910e2926eadb13b8a2d04646debf..4aa89c64e3285e080e9ba3bb64db4d7f3befa87d 100644 (file)
@@ -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
index d59614aea4681392ed069cafb1c1be0ec491b4e1..2f455c4990bdec37b6ff0f8551fd19ddf45478e8 100644 (file)
@@ -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
   
index f2107ad23014d06514480f30336c1c1eb2bf0e17..2dc99864af4f19a90b155c4ec1e568c8e068cb10 100644 (file)
@@ -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
index d9a445133799d91c15ec69e14468b6deb8312ee0..f766a3ab4332faff290b3c97a1315fbace9df1c2 100644 (file)
@@ -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