Clean-up workflows controller (#33337).

Patch by Vincent Robert and Marius BALTEANU.


git-svn-id: http://svn.redmine.org/redmine/trunk@20941 e93f8b46-1217-0410-a6f0-8f06a7374b81

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,21 +31,6 @@ class WorkflowsController < ApplicationController
   end
 
   def edit
-    find_trackers_roles_and_statuses_for_edit
-
-    if request.post? && @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|
-          transition_by_rule.reject! {|rule, transition| transition == 'no_change'}
-        end
-      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)).
@@ -56,20 +42,21 @@ class WorkflowsController < ApplicationController
     end
   end
 
-  def permissions
+  def update
-    find_trackers_roles_and_statuses_for_edit
+    if @roles && @trackers && params[:transitions]
-
+      transitions = params[:transitions].deep_dup
-    if request.post? && @roles && @trackers && params[:permissions]
+      transitions.each do |old_status_id, transitions_by_new_status|
-      permissions = params[:permissions].deep_dup
+        transitions_by_new_status.each do |new_status_id, transition_by_rule|
-      permissions.each do |field, rule_by_status_id|
+          transition_by_rule.reject! {|rule, transition| transition == 'no_change'}
-        rule_by_status_id.reject! {|status_id, rule| rule == 'no_change'}
         end
-      WorkflowPermission.replace_permissions(@trackers, @roles, permissions)
+      end
+      WorkflowTransition.replace_transitions(@trackers, @roles, transitions)
       flash[:notice] = l(:notice_successful_update)
-      redirect_to_referer_or workflows_permissions_path
+    end
-      return
+    redirect_to_referer_or edit_workflows_path
   end
 
+  def permissions
     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
@@ -78,10 +65,46 @@ class WorkflowsController < ApplicationController
     end
   end
 
+  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)
+    end
+    redirect_to_referer_or permissions_workflows_path
+  end
+
   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
+
+  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,24 +127,7 @@ 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

app/views/issue_statuses/index.html.erb

@@ -26,7 +26,7 @@
   <td>
     <% unless WorkflowTransition.where('old_status_id = ? OR new_status_id = ?', status.id, status.id).exists? %>
       <span class="icon icon-warning">
-        <%= 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) %>)
       </span>
     <% end %>
   </td>

app/views/roles/index.html.erb

@@ -18,7 +18,7 @@
   <td>
     <% unless role.builtin? || role.workflow_rules.exists? %>
       <span class="icon icon-warning">
-        <%= 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) %>)
       </span>
     <% end %>
   </td>

app/views/trackers/index.html.erb

@@ -22,7 +22,7 @@
   <td>
     <% unless tracker.workflow_rules.exists? %>
       <span class="icon icon-warning">
-        <%= 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) %>)
       </span>
     <% end %>
   </td>

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 %>
 <fieldset class="tabular box">
 <legend><%= l(:label_copy_source) %></legend>
 <p>

app/views/workflows/edit.html.erb

@@ -4,8 +4,8 @@
 
 <div class="tabs">
   <ul>
-    <li><%= link_to l(:label_status_transitions), workflows_edit_path(:role_id => @roles, :tracker_id => @trackers), :class => 'selected' %></li>
+    <li><%= link_to l(:label_status_transitions), edit_workflows_path(:role_id => @roles, :tracker_id => @trackers), :class => 'selected' %></li>
-    <li><%= link_to l(:label_fields_permissions), workflows_permissions_path(:role_id => @roles, :tracker_id => @trackers) %></li>
+    <li><%= link_to l(:label_fields_permissions), permissions_workflows_path(:role_id => @roles, :tracker_id => @trackers) %></li>
   </ul>
 </div>
 
@@ -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 %>

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? %>
 <p class="nodata"><%= l(:label_no_data) %></p>

app/views/workflows/permissions.html.erb

@@ -4,8 +4,8 @@
 
 <div class="tabs">
   <ul>
-    <li><%= link_to l(:label_status_transitions), workflows_edit_path(:role_id => @roles, :tracker_id => @trackers) %></li>
+    <li><%= link_to l(:label_status_transitions), edit_workflows_path(:role_id => @roles, :tracker_id => @trackers) %></li>
-    <li><%= link_to l(:label_fields_permissions), workflows_permissions_path(:role_id => @roles, :tracker_id => @trackers), :class => 'selected' %></li>
+    <li><%= link_to l(:label_fields_permissions), permissions_workflows_path(:role_id => @roles, :tracker_id => @trackers), :class => 'selected' %></li>
   </ul>
 </div>
 
@@ -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 %>

config/routes.rb

@@ -372,10 +372,17 @@ Rails.application.routes.draw do
     end
   end
 
-  match 'workflows', :controller => 'workflows', :action => 'index', :via => :get
+  resources :workflows, only: [:index] do
-  match 'workflows/edit', :controller => 'workflows', :action => 'edit', :via => [:get, :post]
+    collection do
-  match 'workflows/permissions', :controller => 'workflows', :action => 'permissions', :via => [:get, :post]
+      get 'edit'
-  match 'workflows/copy', :controller => 'workflows', :action => 'copy', :via => [:get, :post]
+      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'

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']
       }

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