From 45858be8e490a9f7fa70032f75aae8902e47e11a Mon Sep 17 00:00:00 2001 From: Marius BALTEANU Date: Sat, 27 Feb 2021 21:05:43 +0200 Subject: [PATCH 1/2] Clean-up workflows controller --- app/controllers/workflows_controller.rb | 90 +++++++++++--------- app/views/issue_statuses/index.html.erb | 2 +- app/views/roles/index.html.erb | 4 +- app/views/trackers/index.html.erb | 2 +- app/views/workflows/copy.html.erb | 4 +- app/views/workflows/edit.html.erb | 6 +- app/views/workflows/index.html.erb | 2 +- app/views/workflows/permissions.html.erb | 6 +- config/routes.rb | 15 +++- test/functional/workflows_controller_test.rb | 18 ++-- test/integration/routing/workflows_test.rb | 6 +- 11 files changed, 84 insertions(+), 71 deletions(-) diff --git a/app/controllers/workflows_controller.rb b/app/controllers/workflows_controller.rb index 37b4c0f4b..5e1f86b17 100644 --- a/app/controllers/workflows_controller.rb +++ b/app/controllers/workflows_controller.rb @@ -20,6 +20,7 @@ class WorkflowsController < ApplicationController layout 'admin' self.main_menu = false + before_action :find_trackers_roles_and_statuses_for_edit, only: [:edit, :update, :permissions, :update_permissions] before_action :require_admin @@ -30,9 +31,19 @@ class WorkflowsController < ApplicationController end def edit - find_trackers_roles_and_statuses_for_edit + if @trackers && @roles && @statuses.any? + 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} + @workflows['assignee'] = workflows.select {|w| w.assignee} + end + end - if request.post? && @roles && @trackers && params[:transitions] + def update + if @roles && @trackers && params[:transitions] transitions = params[:transitions].deep_dup transitions.each do |old_status_id, transitions_by_new_status| transitions_by_new_status.each do |new_status_id, transition_by_rule| @@ -41,47 +52,59 @@ class WorkflowsController < ApplicationController end WorkflowTransition.replace_transitions(@trackers, @roles, transitions) flash[:notice] = l(:notice_successful_update) - redirect_to_referer_or workflows_edit_path - return - end - - if @trackers && @roles && @statuses.any? - 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} - @workflows['assignee'] = workflows.select {|w| w.assignee} end + redirect_to_referer_or edit_workflows_path end def permissions - find_trackers_roles_and_statuses_for_edit + if @roles && @trackers + @fields = (Tracker::CORE_FIELDS_ALL - @trackers.map(&:disabled_core_fields).reduce(:&)).map {|field| [field, l("field_"+field.sub(/_id$/, ''))]} + @custom_fields = @trackers.map(&:custom_fields).flatten.uniq.sort + @permissions = WorkflowPermission.rules_by_status_id(@trackers, @roles) + @statuses.each {|status| @permissions[status.id] ||= {}} + end + end - if request.post? && @roles && @trackers && params[:permissions] + def update_permissions + if @roles && @trackers && params[:permissions] permissions = params[:permissions].deep_dup permissions.each do |field, rule_by_status_id| rule_by_status_id.reject! {|status_id, rule| rule == 'no_change'} end WorkflowPermission.replace_permissions(@trackers, @roles, permissions) flash[:notice] = l(:notice_successful_update) - redirect_to_referer_or workflows_permissions_path - return end + redirect_to_referer_or workflows_permissions_path + end - if @roles && @trackers - @fields = (Tracker::CORE_FIELDS_ALL - @trackers.map(&:disabled_core_fields).reduce(:&)).map {|field| [field, l("field_"+field.sub(/_id$/, ''))]} - @custom_fields = @trackers.map(&:custom_fields).flatten.uniq.sort - @permissions = WorkflowPermission.rules_by_status_id(@trackers, @roles) - @statuses.each {|status| @permissions[status.id] ||= {}} + def copy + find_sources_and_targets + end + + def duplicate + find_sources_and_targets + if params[:source_tracker_id].blank? || params[:source_role_id].blank? || + (@source_tracker.nil? && @source_role.nil?) + flash.now[:error] = l(:error_workflow_copy_source) + render :copy + elsif @target_trackers.blank? || @target_roles.blank? + flash.now[:error] = l(:error_workflow_copy_target) + render :copy + else + WorkflowRule.copy(@source_tracker, @source_role, @target_trackers, @target_roles) + flash[:notice] = l(:notice_successful_update) + redirect_to copy_workflows_path( + :source_tracker_id => @source_tracker, + :source_role_id => @source_role + ) end end - def copy + private + + def find_sources_and_targets @roles = Role.sorted.select(&:consider_workflow?) @trackers = Tracker.sorted - if params[:source_tracker_id].blank? || params[:source_tracker_id] == 'any' @source_tracker = nil else @@ -104,25 +127,8 @@ class WorkflowsController < ApplicationController else Role.where(:id => params[:target_role_ids]).to_a end - if request.post? - if params[:source_tracker_id].blank? || params[:source_role_id].blank? || - (@source_tracker.nil? && @source_role.nil?) - flash.now[:error] = l(:error_workflow_copy_source) - elsif @target_trackers.blank? || @target_roles.blank? - flash.now[:error] = l(:error_workflow_copy_target) - else - WorkflowRule.copy(@source_tracker, @source_role, @target_trackers, @target_roles) - flash[:notice] = l(:notice_successful_update) - redirect_to( - workflows_copy_path(:source_tracker_id => @source_tracker, - :source_role_id => @source_role) - ) - end - end end - private - def find_trackers_roles_and_statuses_for_edit find_roles find_trackers diff --git a/app/views/issue_statuses/index.html.erb b/app/views/issue_statuses/index.html.erb index b4802c241..c9f120cbd 100644 --- a/app/views/issue_statuses/index.html.erb +++ b/app/views/issue_statuses/index.html.erb @@ -26,7 +26,7 @@ <% unless WorkflowTransition.where('old_status_id = ? OR new_status_id = ?', status.id, status.id).exists? %> - <%= l(:text_status_no_workflow) %> (<%= link_to l(:button_edit), workflows_edit_path(:used_statuses_only => 0) %>) + <%= l(:text_status_no_workflow) %> (<%= link_to l(:button_edit), edit_workflows_path(:used_statuses_only => 0) %>) <% end %> diff --git a/app/views/roles/index.html.erb b/app/views/roles/index.html.erb index bc8cc9cc7..f76e90466 100644 --- a/app/views/roles/index.html.erb +++ b/app/views/roles/index.html.erb @@ -18,7 +18,7 @@ <% unless role.builtin? || role.workflow_rules.exists? %> - <%= l(:text_role_no_workflow) %> (<%= link_to l(:button_edit), workflows_edit_path(:role_id => role) %>) + <%= l(:text_role_no_workflow) %> (<%= link_to l(:button_edit), edit_workflows_path(:role_id => role) %>) <% end %> @@ -36,4 +36,4 @@ <%= javascript_tag do %> $(function() { $("table.roles tbody").positionedItems({items: ".givable"}); }); -<% end %> \ No newline at end of file +<% end %> diff --git a/app/views/trackers/index.html.erb b/app/views/trackers/index.html.erb index 85d3249fc..84093e943 100644 --- a/app/views/trackers/index.html.erb +++ b/app/views/trackers/index.html.erb @@ -22,7 +22,7 @@ <% unless tracker.workflow_rules.exists? %> - <%= l(:text_tracker_no_workflow) %> (<%= link_to l(:button_edit), workflows_edit_path(:tracker_id => tracker) %>) + <%= l(:text_tracker_no_workflow) %> (<%= link_to l(:button_edit), edit_workflows_path(:tracker_id => tracker) %>) <% end %> diff --git a/app/views/workflows/copy.html.erb b/app/views/workflows/copy.html.erb index 78997caf5..cd270a8db 100644 --- a/app/views/workflows/copy.html.erb +++ b/app/views/workflows/copy.html.erb @@ -1,6 +1,6 @@ -<%= title [l(:label_workflow), workflows_edit_path], l(:button_copy) %> +<%= title [l(:label_workflow), edit_workflows_path], l(:button_copy) %> -<%= form_tag({}, :id => 'workflow_copy_form') do %> +<%= form_tag duplicate_workflows_path, method: :post, id: 'workflow_copy_form' do %>
<%= l(:label_copy_source) %>

diff --git a/app/views/workflows/edit.html.erb b/app/views/workflows/edit.html.erb index c247097a4..df4507be2 100644 --- a/app/views/workflows/edit.html.erb +++ b/app/views/workflows/edit.html.erb @@ -4,8 +4,8 @@

    -
  • <%= link_to l(:label_status_transitions), workflows_edit_path(:role_id => @roles, :tracker_id => @trackers), :class => 'selected' %>
    • -
  • <%= link_to l(:label_fields_permissions), workflows_permissions_path(:role_id => @roles, :tracker_id => @trackers) %>
    • +
  • <%= link_to l(:label_status_transitions), edit_workflows_path(:role_id => @roles, :tracker_id => @trackers), :class => 'selected' %>
    • +
  • <%= link_to l(:label_fields_permissions), permissions_workflows_path(:role_id => @roles, :tracker_id => @trackers) %>
@@ -32,7 +32,7 @@ <% end %> <% if @trackers && @roles && @statuses.any? %> - <%= form_tag({}, :id => 'workflow_form' ) do %> + <%= form_tag workflows_path, method: :patch, id: 'workflow_form' do %> <%= @trackers.map {|tracker| hidden_field_tag 'tracker_id[]', tracker.id, :id => nil}.join.html_safe %> <%= @roles.map {|role| hidden_field_tag 'role_id[]', role.id, :id => nil}.join.html_safe %> <%= hidden_field_tag 'used_statuses_only', params[:used_statuses_only], :id => nil %> diff --git a/app/views/workflows/index.html.erb b/app/views/workflows/index.html.erb index 66785a40c..659b55c25 100644 --- a/app/views/workflows/index.html.erb +++ b/app/views/workflows/index.html.erb @@ -1,4 +1,4 @@ -<%= title [l(:label_workflow), workflows_edit_path], l(:field_summary) %> +<%= title [l(:label_workflow), edit_workflows_path], l(:field_summary) %> <% if @roles.empty? || @trackers.empty? %>

<%= l(:label_no_data) %>

diff --git a/app/views/workflows/permissions.html.erb b/app/views/workflows/permissions.html.erb index 28a671052..806d557dc 100644 --- a/app/views/workflows/permissions.html.erb +++ b/app/views/workflows/permissions.html.erb @@ -4,8 +4,8 @@
    -
  • <%= link_to l(:label_status_transitions), workflows_edit_path(:role_id => @roles, :tracker_id => @trackers) %>
    • -
  • <%= link_to l(:label_fields_permissions), workflows_permissions_path(:role_id => @roles, :tracker_id => @trackers), :class => 'selected' %>
    • +
  • <%= link_to l(:label_status_transitions), edit_workflows_path(:role_id => @roles, :tracker_id => @trackers) %>
    • +
  • <%= link_to l(:label_fields_permissions), permissions_workflows_path(:role_id => @roles, :tracker_id => @trackers), :class => 'selected' %>
@@ -30,7 +30,7 @@ <% end %> <% if @trackers && @roles && @statuses.any? %> - <%= form_tag({}, :id => 'workflow_form' ) do %> + <%= form_tag update_permissions_workflows_path, method: :patch, id: 'workflow_form' do %> <%= @trackers.map {|tracker| hidden_field_tag 'tracker_id[]', tracker.id, :id => nil}.join.html_safe %> <%= @roles.map {|role| hidden_field_tag 'role_id[]', role.id, :id => nil}.join.html_safe %> <%= hidden_field_tag 'used_statuses_only', params[:used_statuses_only], :id => nil %> diff --git a/config/routes.rb b/config/routes.rb index a01456dba..be12e504e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -372,10 +372,17 @@ Rails.application.routes.draw do end end - match 'workflows', :controller => 'workflows', :action => 'index', :via => :get - match 'workflows/edit', :controller => 'workflows', :action => 'edit', :via => [:get, :post] - match 'workflows/permissions', :controller => 'workflows', :action => 'permissions', :via => [:get, :post] - match 'workflows/copy', :controller => 'workflows', :action => 'copy', :via => [:get, :post] + resources :workflows, only: [:index] do + collection do + get 'edit' + patch 'update' + get 'permissions' + patch 'update_permissions' + get 'copy' + post 'duplicate' + end + end + match 'settings', :controller => 'settings', :action => 'index', :via => :get match 'settings/edit', :controller => 'settings', :action => 'edit', :via => [:get, :post] match 'settings/plugin/:id', :controller => 'settings', :action => 'plugin', :via => [:get, :post], :as => 'plugin_settings' diff --git a/test/functional/workflows_controller_test.rb b/test/functional/workflows_controller_test.rb index cdb1f320f..e4bdd7084 100644 --- a/test/functional/workflows_controller_test.rb +++ b/test/functional/workflows_controller_test.rb @@ -134,7 +134,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest def test_post_edit WorkflowTransition.delete_all - post :edit, :params => { + patch :update, :params => { :role_id => 2, :tracker_id => 1, :transitions => { @@ -152,7 +152,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest def test_post_edit_with_allowed_statuses_for_new_issues WorkflowTransition.delete_all - post :edit, :params => { + patch :update, :params => { :role_id => 2, :tracker_id => 1, :transitions => { @@ -169,7 +169,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest def test_post_edit_with_additional_transitions WorkflowTransition.delete_all - post :edit, :params => { + patch :update, :params => { :role_id => 2, :tracker_id => 1, :transitions => { @@ -346,7 +346,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest def test_post_permissions WorkflowPermission.delete_all - post :permissions, :params => { + patch :update_permissions, :params => { :role_id => 1, :tracker_id => 2, :permissions => { @@ -389,7 +389,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest def test_post_copy_one_to_one source_transitions = status_transitions(:tracker_id => 1, :role_id => 2) - post :copy, :params => { + post :duplicate, :params => { :source_tracker_id => '1', :source_role_id => '2', :target_tracker_ids => ['3'], :target_role_ids => ['1'] } @@ -400,7 +400,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest def test_post_copy_one_to_many source_transitions = status_transitions(:tracker_id => 1, :role_id => 2) - post :copy, :params => { + post :duplicate, :params => { :source_tracker_id => '1', :source_role_id => '2', :target_tracker_ids => ['2', '3'], :target_role_ids => ['1', '3'] } @@ -415,7 +415,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest source_t2 = status_transitions(:tracker_id => 2, :role_id => 2) source_t3 = status_transitions(:tracker_id => 3, :role_id => 2) - post :copy, :params => { + post :duplicate, :params => { :source_tracker_id => 'any', :source_role_id => '2', :target_tracker_ids => ['2', '3'], :target_role_ids => ['1', '3'] } @@ -428,7 +428,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest def test_post_copy_with_incomplete_source_specification_should_fail assert_no_difference 'WorkflowRule.count' do - post :copy, :params => { + post :duplicate, :params => { :source_tracker_id => '', :source_role_id => '2', :target_tracker_ids => ['2', '3'], :target_role_ids => ['1', '3'] } @@ -439,7 +439,7 @@ class WorkflowsControllerTest < Redmine::ControllerTest def test_post_copy_with_incomplete_target_specification_should_fail assert_no_difference 'WorkflowRule.count' do - post :copy, :params => { + post :duplicate, :params => { :source_tracker_id => '1', :source_role_id => '2', :target_tracker_ids => ['2', '3'] } diff --git a/test/integration/routing/workflows_test.rb b/test/integration/routing/workflows_test.rb index ca6b16e48..de0e7d2a6 100644 --- a/test/integration/routing/workflows_test.rb +++ b/test/integration/routing/workflows_test.rb @@ -23,12 +23,12 @@ class RoutingWorkflowsTest < Redmine::RoutingTest def test_workflows should_route 'GET /workflows' => 'workflows#index' should_route 'GET /workflows/edit' => 'workflows#edit' - should_route 'POST /workflows/edit' => 'workflows#edit' + should_route 'PATCH /workflows/update' => 'workflows#update' should_route 'GET /workflows/permissions' => 'workflows#permissions' - should_route 'POST /workflows/permissions' => 'workflows#permissions' + should_route 'PATCH /workflows/update_permissions' => 'workflows#update_permissions' should_route 'GET /workflows/copy' => 'workflows#copy' - should_route 'POST /workflows/copy' => 'workflows#copy' + should_route 'POST /workflows/duplicate' => 'workflows#duplicate' end end -- 2.22.0