From 4b096e9a56768dd5ba15304a903939f4e5e62e22 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Sun, 20 Feb 2011 15:38:07 +0000 Subject: [PATCH] Allow additional workflow transitions for issue author and assignee (#2732). git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@4895 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/workflows_controller.rb | 17 ++++- app/models/issue.rb | 7 +- app/models/issue_status.rb | 27 +++---- app/views/workflows/_form.html.erb | 40 +++++++++++ app/views/workflows/edit.rhtml | 71 +++++++------------ ...60626_add_workflows_assignee_and_author.rb | 13 ++++ public/javascripts/application.js | 6 ++ test/fixtures/issue_statuses.yml | 26 ++++--- test/functional/workflows_controller_test.rb | 44 +++++++++--- test/unit/issue_status_test.rb | 26 ++++++- test/unit/issue_test.rb | 27 +++++++ 11 files changed, 221 insertions(+), 83 deletions(-) create mode 100644 app/views/workflows/_form.html.erb create mode 100644 db/migrate/20110220160626_add_workflows_assignee_and_author.rb diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb index ed4640129..9ca1a98c4 100644 --- a/app/controllers/workflows_controller.rb +++ b/app/controllers/workflows_controller.rb @@ -32,14 +32,17 @@ class WorkflowsController < ApplicationController if request.post? Workflow.destroy_all( ["role_id=? and tracker_id=?", @role.id, @tracker.id]) - (params[:issue_status] || []).each { |old, news| - news.each { |new| - @role.workflows.build(:tracker_id => @tracker.id, :old_status_id => old, :new_status_id => new) + (params[:issue_status] || []).each { |status_id, transitions| + transitions.each { |new_status_id, options| + author = options.is_a?(Array) && options.include?('author') && !options.include?('always') + assignee = options.is_a?(Array) && options.include?('assignee') && !options.include?('always') + @role.workflows.build(:tracker_id => @tracker.id, :old_status_id => status_id, :new_status_id => new_status_id, :author => author, :assignee => assignee) } } if @role.save flash[:notice] = l(:notice_successful_update) redirect_to :action => 'edit', :role_id => @role, :tracker_id => @tracker + return end end @@ -48,6 +51,14 @@ class WorkflowsController < ApplicationController @statuses = @tracker.issue_statuses end @statuses ||= IssueStatus.find(:all, :order => 'position') + + if @tracker && @role && @statuses.any? + workflows = Workflow.all(:conditions => {:role_id => @role.id, :tracker_id => @tracker.id}) + @workflows = {} + @workflows['always'] = workflows.select {|w| !w.author && !w.assignee} + @workflows['author'] = workflows.select {|w| w.author} + @workflows['assignee'] = workflows.select {|w| w.assignee} + end end def copy diff --git a/app/models/issue.rb b/app/models/issue.rb index 40b64f3e7..2810138a4 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -422,7 +422,12 @@ class Issue < ActiveRecord::Base # Returns an array of status that user is able to apply def new_statuses_allowed_to(user, include_default=false) - statuses = status.find_new_statuses_allowed_to(user.roles_for_project(project), tracker) + statuses = status.find_new_statuses_allowed_to( + user.roles_for_project(project), + tracker, + author == user, + assigned_to_id_changed? ? assigned_to_id_was == user.id : assigned_to_id == user.id + ) statuses << status unless statuses.empty? statuses << IssueStatus.default if include_default statuses = statuses.uniq.sort diff --git a/app/models/issue_status.rb b/app/models/issue_status.rb index 8171cdb79..5527e2c9a 100644 --- a/app/models/issue_status.rb +++ b/app/models/issue_status.rb @@ -50,10 +50,16 @@ class IssueStatus < ActiveRecord::Base # 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) + def new_statuses_allowed_to(roles, tracker, author=false, assignee=false) if roles && tracker role_ids = roles.collect(&:id) - new_statuses = workflows.select {|w| role_ids.include?(w.role_id) && w.tracker_id == tracker.id}.collect{|w| w.new_status}.compact.sort + transitions = workflows.select do |w| + role_ids.include?(w.role_id) && + w.tracker_id == tracker.id && + (author || !w.author) && + (assignee || !w.assignee) + end + transitions.collect{|w| w.new_status}.compact.sort else [] end @@ -61,24 +67,19 @@ class IssueStatus < ActiveRecord::Base # 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) + def find_new_statuses_allowed_to(roles, tracker, author=false, assignee=false) if roles && tracker + conditions = {:role_id => roles.collect(&:id), :tracker_id => tracker.id} + conditions[:author] = false unless author + conditions[:assignee] = false unless assignee + workflows.find(:all, :include => :new_status, - :conditions => { :role_id => roles.collect(&:id), - :tracker_id => tracker.id}).collect{ |w| w.new_status }.compact.sort + :conditions => conditions).collect{|w| w.new_status}.compact.sort else [] end end - - def new_status_allowed_to?(status, roles, tracker) - if status && roles && tracker - !workflows.find(:first, :conditions => {:new_status_id => status.id, :role_id => roles.collect(&:id), :tracker_id => tracker.id}).nil? - else - false - end - end def <=>(status) position <=> status.position diff --git a/app/views/workflows/_form.html.erb b/app/views/workflows/_form.html.erb new file mode 100644 index 000000000..e5b54c074 --- /dev/null +++ b/app/views/workflows/_form.html.erb @@ -0,0 +1,40 @@ + + + + + + + + + <% for new_status in @statuses %> + + <% end %> + + + + <% for old_status in @statuses %> + "> + + <% for new_status in @statuses -%> + + <% end -%> + + <% end %> + +
+ <%= link_to_function(image_tag('toggle_check.png'), "toggleCheckboxesBySelector('table.transitions-#{name} input')", + :title => "#{l(:button_check_all)}/#{l(:button_uncheck_all)}") %> + <%=l(:label_current_status)%> + <%=l(:label_new_statuses_allowed)%>
+ <%= link_to_function(image_tag('toggle_check.png'), "toggleCheckboxesBySelector('table.transitions-#{name} input.new-status-#{new_status.id}')", + :title => "#{l(:button_check_all)}/#{l(:button_uncheck_all)}") %> + <%=h new_status.name %> +
+ <%= link_to_function(image_tag('toggle_check.png'), "toggleCheckboxesBySelector('table.transitions-#{name} input.old-status-#{old_status.id}')", + :title => "#{l(:button_check_all)}/#{l(:button_uncheck_all)}") %> + + <%=h old_status.name %> + + <%= check_box_tag "issue_status[#{ old_status.id }][#{new_status.id}][]", name, workflows.detect {|w| w.old_status_id == old_status.id && w.new_status_id == new_status.id}, + :class => "old-status-#{old_status.id} new-status-#{new_status.id}" %> +
\ No newline at end of file diff --git a/app/views/workflows/edit.rhtml b/app/views/workflows/edit.rhtml index 26f2cf96c..2a4b8be5b 100644 --- a/app/views/workflows/edit.rhtml +++ b/app/views/workflows/edit.rhtml @@ -20,54 +20,31 @@

<% end %> - <% if @tracker && @role && @statuses.any? %> -<% form_tag({}, :id => 'workflow_form' ) do %> -<%= hidden_field_tag 'tracker_id', @tracker.id %> -<%= hidden_field_tag 'role_id', @role.id %> -
- - - - - - - - - <% for new_status in @statuses %> - - <% end %> - - - - <% for old_status in @statuses %> - "> - - <% new_status_ids_allowed = old_status.find_new_statuses_allowed_to([@role], @tracker).collect(&:id) -%> - <% for new_status in @statuses -%> - - <% end -%> - - <% end %> - -
<%=l(:label_current_status)%><%=l(:label_new_statuses_allowed)%>
- <%= link_to_function(image_tag('toggle_check.png'), "toggleCheckboxesBySelector('input.new-status-#{new_status.id}')", - :title => "#{l(:button_check_all)}/#{l(:button_uncheck_all)}") %> - <%= new_status.name %> -
- <%= link_to_function(image_tag('toggle_check.png'), "toggleCheckboxesBySelector('input.old-status-#{old_status.id}')", - :title => "#{l(:button_check_all)}/#{l(:button_uncheck_all)}") %> - - <%= old_status.name %> - - <%= check_box_tag "issue_status[#{ old_status.id }][]", new_status.id, new_status_ids_allowed.include?(new_status.id), - :class => "old-status-#{old_status.id} new-status-#{new_status.id}" %> -
-
-

<%= check_all_links 'workflow_form' %>

- -<%= submit_tag l(:button_save) %> -<% end %> + <% form_tag({}, :id => 'workflow_form' ) do %> + <%= hidden_field_tag 'tracker_id', @tracker.id %> + <%= hidden_field_tag 'role_id', @role.id %> +
+ <%= render :partial => 'form', :locals => {:name => 'always', :workflows => @workflows['always']} %> + +
+ Autorisations supplémentaires lorsque l'utilisateur a créé la demande +
+ <%= render :partial => 'form', :locals => {:name => 'author', :workflows => @workflows['author']} %> +
+
+ <%= javascript_tag "hideFieldset($('author_workflows'))" unless @workflows['author'].present? %> + +
+ Autorisations supplémentaires lorsque la demande est assignée à l'utilisateur +
+ <%= render :partial => 'form', :locals => {:name => 'assignee', :workflows => @workflows['assignee']} %> +
+
+ <%= javascript_tag "hideFieldset($('assignee_workflows'))" unless @workflows['assignee'].present? %> +
+ <%= submit_tag l(:button_save) %> + <% end %> <% end %> <% html_title(l(:label_workflow)) -%> diff --git a/db/migrate/20110220160626_add_workflows_assignee_and_author.rb b/db/migrate/20110220160626_add_workflows_assignee_and_author.rb new file mode 100644 index 000000000..73ccb4e0a --- /dev/null +++ b/db/migrate/20110220160626_add_workflows_assignee_and_author.rb @@ -0,0 +1,13 @@ +class AddWorkflowsAssigneeAndAuthor < ActiveRecord::Migration + def self.up + add_column :workflows, :assignee, :boolean, :null => false, :default => false + add_column :workflows, :author, :boolean, :null => false, :default => false + Workflow.update_all("assignee = #{Workflow.connection.quoted_false}") + Workflow.update_all("author = #{Workflow.connection.quoted_false}") + end + + def self.down + remove_column :workflows, :assignee + remove_column :workflows, :author + end +end diff --git a/public/javascripts/application.js b/public/javascripts/application.js index 1679d83d9..6cea8f3dd 100644 --- a/public/javascripts/application.js +++ b/public/javascripts/application.js @@ -46,6 +46,12 @@ function toggleFieldset(el) { Effect.toggle(fieldset.down('div'), 'slide', {duration:0.2}); } +function hideFieldset(el) { + var fieldset = Element.up(el, 'fieldset'); + fieldset.toggleClassName('collapsed'); + fieldset.down('div').hide(); +} + var fileFieldCount = 1; function addFileField() { diff --git a/test/fixtures/issue_statuses.yml b/test/fixtures/issue_statuses.yml index 098ac9619..ff40b1c50 100644 --- a/test/fixtures/issue_statuses.yml +++ b/test/fixtures/issue_statuses.yml @@ -1,31 +1,37 @@ --- -issue_statuses_006: - name: Rejected - is_default: false - is_closed: true - id: 6 issue_statuses_001: + id: 1 name: New is_default: true is_closed: false - id: 1 + position: 1 issue_statuses_002: + id: 2 name: Assigned is_default: false is_closed: false - id: 2 + position: 2 issue_statuses_003: + id: 3 name: Resolved is_default: false is_closed: false - id: 3 + position: 3 issue_statuses_004: name: Feedback + id: 4 is_default: false is_closed: false - id: 4 + position: 4 issue_statuses_005: + id: 5 name: Closed is_default: false is_closed: true - id: 5 + position: 5 +issue_statuses_006: + id: 6 + name: Rejected + is_default: false + is_closed: true + position: 6 diff --git a/test/functional/workflows_controller_test.rb b/test/functional/workflows_controller_test.rb index 64d606a8a..c868e019e 100644 --- a/test/functional/workflows_controller_test.rb +++ b/test/functional/workflows_controller_test.rb @@ -65,17 +65,17 @@ class WorkflowsControllerTest < ActionController::TestCase # allowed transitions assert_tag :tag => 'input', :attributes => { :type => 'checkbox', - :name => 'issue_status[3][]', - :value => '5', + :name => 'issue_status[3][5][]', + :value => 'always', :checked => 'checked' } # not allowed assert_tag :tag => 'input', :attributes => { :type => 'checkbox', - :name => 'issue_status[3][]', - :value => '2', + :name => 'issue_status[3][2][]', + :value => 'always', :checked => nil } # unused assert_no_tag :tag => 'input', :attributes => { :type => 'checkbox', - :name => 'issue_status[4][]' } + :name => 'issue_status[1][1][]' } end def test_get_edit_with_role_and_tracker_and_all_statuses @@ -89,13 +89,17 @@ class WorkflowsControllerTest < ActionController::TestCase assert_equal IssueStatus.count, assigns(:statuses).size assert_tag :tag => 'input', :attributes => { :type => 'checkbox', - :name => 'issue_status[1][]', - :value => '1', + :name => 'issue_status[1][1][]', + :value => 'always', :checked => nil } end def test_post_edit - post :edit, :role_id => 2, :tracker_id => 1, :issue_status => {'4' => ['5'], '3' => ['1', '2']} + post :edit, :role_id => 2, :tracker_id => 1, + :issue_status => { + '4' => {'5' => ['always']}, + '3' => {'1' => ['always'], '2' => ['always']} + } assert_redirected_to '/workflows/edit?role_id=2&tracker_id=1' assert_equal 3, Workflow.count(:conditions => {:tracker_id => 1, :role_id => 2}) @@ -103,6 +107,30 @@ class WorkflowsControllerTest < ActionController::TestCase assert_nil Workflow.find(:first, :conditions => {:role_id => 2, :tracker_id => 1, :old_status_id => 5, :new_status_id => 4}) end + def test_post_edit_with_additional_transitions + post :edit, :role_id => 2, :tracker_id => 1, + :issue_status => { + '4' => {'5' => ['always']}, + '3' => {'1' => ['author'], '2' => ['assignee'], '4' => ['author', 'assignee']} + } + assert_redirected_to '/workflows/edit?role_id=2&tracker_id=1' + + assert_equal 4, Workflow.count(:conditions => {:tracker_id => 1, :role_id => 2}) + + w = Workflow.find(:first, :conditions => {:role_id => 2, :tracker_id => 1, :old_status_id => 4, :new_status_id => 5}) + assert ! w.author + assert ! w.assignee + w = Workflow.find(:first, :conditions => {:role_id => 2, :tracker_id => 1, :old_status_id => 3, :new_status_id => 1}) + assert w.author + assert ! w.assignee + w = Workflow.find(:first, :conditions => {:role_id => 2, :tracker_id => 1, :old_status_id => 3, :new_status_id => 2}) + assert ! w.author + assert w.assignee + w = Workflow.find(:first, :conditions => {:role_id => 2, :tracker_id => 1, :old_status_id => 3, :new_status_id => 4}) + assert w.author + assert w.assignee + end + def test_clear_workflow assert Workflow.count(:conditions => {:tracker_id => 1, :role_id => 2}) > 0 diff --git a/test/unit/issue_status_test.rb b/test/unit/issue_status_test.rb index 4fc6de1e2..bc6535edc 100644 --- a/test/unit/issue_status_test.rb +++ b/test/unit/issue_status_test.rb @@ -18,7 +18,7 @@ require File.expand_path('../../test_helper', __FILE__) class IssueStatusTest < ActiveSupport::TestCase - fixtures :issue_statuses, :issues + fixtures :issue_statuses, :issues, :roles, :trackers def test_create status = IssueStatus.new :name => "Assigned" @@ -68,6 +68,30 @@ class IssueStatusTest < ActiveSupport::TestCase status.reload assert status.is_default? end + + def test_new_statuses_allowed_to + Workflow.delete_all + + Workflow.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 2, :author => false, :assignee => false) + Workflow.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 3, :author => true, :assignee => false) + Workflow.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 4, :author => false, :assignee => true) + Workflow.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 5, :author => true, :assignee => true) + status = IssueStatus.find(1) + role = Role.find(1) + tracker = Tracker.find(1) + + assert_equal [2], status.new_statuses_allowed_to([role], tracker, false, false).map(&:id) + assert_equal [2], status.find_new_statuses_allowed_to([role], tracker, false, false).map(&:id) + + assert_equal [2, 3], status.new_statuses_allowed_to([role], tracker, true, false).map(&:id) + assert_equal [2, 3], status.find_new_statuses_allowed_to([role], tracker, true, false).map(&:id) + + assert_equal [2, 4], status.new_statuses_allowed_to([role], tracker, false, true).map(&:id) + assert_equal [2, 4], status.find_new_statuses_allowed_to([role], tracker, false, true).map(&:id) + + assert_equal [2, 3, 4, 5], status.new_statuses_allowed_to([role], tracker, true, true).map(&:id) + assert_equal [2, 3, 4, 5], status.find_new_statuses_allowed_to([role], tracker, true, true).map(&:id) + end context "#update_done_ratios" do setup do diff --git a/test/unit/issue_test.rb b/test/unit/issue_test.rb index d284c7469..0ecfa2e43 100644 --- a/test/unit/issue_test.rb +++ b/test/unit/issue_test.rb @@ -210,6 +210,33 @@ class IssueTest < ActiveSupport::TestCase assert_equal IssueCategory.find(1).assigned_to, issue.assigned_to end + + + def test_new_statuses_allowed_to + Workflow.delete_all + + Workflow.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 2, :author => false, :assignee => false) + Workflow.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 3, :author => true, :assignee => false) + Workflow.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 4, :author => false, :assignee => true) + Workflow.create!(:role_id => 1, :tracker_id => 1, :old_status_id => 1, :new_status_id => 5, :author => true, :assignee => true) + status = IssueStatus.find(1) + role = Role.find(1) + tracker = Tracker.find(1) + user = User.find(2) + + issue = Issue.generate!(:tracker => tracker, :status => status, :project_id => 1) + assert_equal [1, 2], issue.new_statuses_allowed_to(user).map(&:id) + + issue = Issue.generate!(:tracker => tracker, :status => status, :project_id => 1, :author => user) + assert_equal [1, 2, 3], issue.new_statuses_allowed_to(user).map(&:id) + + issue = Issue.generate!(:tracker => tracker, :status => status, :project_id => 1, :assigned_to => user) + assert_equal [1, 2, 4], issue.new_statuses_allowed_to(user).map(&:id) + + issue = Issue.generate!(:tracker => tracker, :status => status, :project_id => 1, :author => user, :assigned_to => user) + assert_equal [1, 2, 3, 4, 5], issue.new_statuses_allowed_to(user).map(&:id) + end + def test_copy issue = Issue.new.copy_from(1) assert issue.save