From a32822477170bf57a482365b8c6c47bd617147e3 Mon Sep 17 00:00:00 2001 From: Jean-Philippe Lang Date: Fri, 13 Feb 2015 20:00:22 +0000 Subject: [PATCH] Removed IssuesController#update_form action, use #new and #edit instead. git-svn-id: http://svn.redmine.org/redmine/trunk@13997 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/issues_controller.rb | 50 ++++++++----------- app/helpers/issues_helper.rb | 10 ++++ app/views/issues/_attributes.html.erb | 2 +- app/views/issues/_form.html.erb | 4 +- .../{update_form.js.erb => edit.js.erb} | 0 app/views/issues/new.js.erb | 1 + config/routes.rb | 8 ++- lib/redmine.rb | 8 +-- test/functional/issues_controller_test.rb | 23 ++++----- test/integration/routing/issues_test.rb | 4 +- 10 files changed, 57 insertions(+), 53 deletions(-) rename app/views/issues/{update_form.js.erb => edit.js.erb} (100%) create mode 100644 app/views/issues/new.js.erb diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index f5d3fcd2e..a2e241cb8 100644 --- a/app/controllers/issues_controller.rb +++ b/app/controllers/issues_controller.rb @@ -21,10 +21,10 @@ class IssuesController < ApplicationController before_filter :find_issue, :only => [:show, :edit, :update] before_filter :find_issues, :only => [:bulk_edit, :bulk_update, :destroy] - before_filter :find_project, :only => [:new, :create, :update_form] + before_filter :find_project, :only => [:new, :create] before_filter :authorize, :except => [:index] before_filter :find_optional_project, :only => [:index] - before_filter :build_new_issue_from_params, :only => [:new, :create, :update_form] + before_filter :build_new_issue_from_params, :only => [:new, :create] accept_rss_auth :index, :show accept_api_auth :index, :show, :create, :update, :destroy @@ -138,6 +138,7 @@ class IssuesController < ApplicationController def new respond_to do |format| format.html { render :action => 'new', :layout => !request.xhr? } + format.js end end @@ -176,6 +177,7 @@ class IssuesController < ApplicationController respond_to do |format| format.html { } + format.js format.xml { } end end @@ -210,11 +212,6 @@ class IssuesController < ApplicationController end end - # Updates the issue form when changing the project, status or tracker - # on issue creation/update - def update_form - end - # Bulk edit/copy a set of issues def bulk_edit @issues.sort! @@ -419,32 +416,27 @@ class IssuesController < ApplicationController end # TODO: Refactor, lots of extra code in here - # TODO: Changing tracker on an existing issue should not trigger this def build_new_issue_from_params - if params[:id].blank? - @issue = Issue.new - if params[:copy_from] - begin - @issue.init_journal(User.current) - @copy_from = Issue.visible.find(params[:copy_from]) - unless User.current.allowed_to?(:copy_issues, @copy_from.project) - raise ::Unauthorized - end - @link_copy = link_copy?(params[:link_copy]) || request.get? - @copy_attachments = params[:copy_attachments].present? || request.get? - @copy_subtasks = params[:copy_subtasks].present? || request.get? - @issue.copy_from(@copy_from, :attachments => @copy_attachments, :subtasks => @copy_subtasks, :link => @link_copy) - rescue ActiveRecord::RecordNotFound - render_404 - return + @issue = Issue.new + if params[:copy_from] + begin + @issue.init_journal(User.current) + @copy_from = Issue.visible.find(params[:copy_from]) + unless User.current.allowed_to?(:copy_issues, @copy_from.project) + raise ::Unauthorized end + @link_copy = link_copy?(params[:link_copy]) || request.get? + @copy_attachments = params[:copy_attachments].present? || request.get? + @copy_subtasks = params[:copy_subtasks].present? || request.get? + @issue.copy_from(@copy_from, :attachments => @copy_attachments, :subtasks => @copy_subtasks, :link => @link_copy) + rescue ActiveRecord::RecordNotFound + render_404 + return end - @issue.project = @project - @issue.author ||= User.current - @issue.start_date ||= Date.today if Setting.default_issue_start_date_to_creation_date? - else - @issue = @project.issues.visible.find(params[:id]) end + @issue.project = @project + @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 params[:was_default_status] == attrs[:status_id] diff --git a/app/helpers/issues_helper.rb b/app/helpers/issues_helper.rb index 885b1c5b1..e34039d76 100644 --- a/app/helpers/issues_helper.rb +++ b/app/helpers/issues_helper.rb @@ -199,6 +199,16 @@ module IssuesHelper s.html_safe end + # Returns the path for updating the issue form + # with project as the current project + def update_issue_form_path(project, issue) + if issue.new_record? + new_project_issue_path(project, :format => 'js') + else + edit_issue_path(issue, :format => 'js') + end + end + # Returns the number of descendants for an array of issues def issues_descendant_count(issues) ids = issues.reject(&:leaf?).map {|issue| issue.descendants.ids}.flatten.uniq diff --git a/app/views/issues/_attributes.html.erb b/app/views/issues/_attributes.html.erb index 01d262e57..61ffa8ae3 100644 --- a/app/views/issues/_attributes.html.erb +++ b/app/views/issues/_attributes.html.erb @@ -4,7 +4,7 @@
<% if @issue.safe_attribute?('status_id') && @allowed_statuses.present? %>

<%= f.select :status_id, (@allowed_statuses.collect {|p| [p.name, p.id]}), {:required => true}, - :onchange => "updateIssueFrom('#{escape_javascript project_issue_form_path(@project, :id => @issue, :format => 'js')}')" %>

+ :onchange => "updateIssueFrom('#{escape_javascript update_issue_form_path(@project, @issue)}')" %>

<%= hidden_field_tag 'was_default_status', @issue.status_id, :id => nil if @issue.status == @issue.default_status %> <% else %>

<%= @issue.status %>

diff --git a/app/views/issues/_form.html.erb b/app/views/issues/_form.html.erb index 401789c13..44578610c 100644 --- a/app/views/issues/_form.html.erb +++ b/app/views/issues/_form.html.erb @@ -9,12 +9,12 @@ <% if @issue.safe_attribute? 'project_id' %>

<%= f.select :project_id, project_tree_options_for_select(@issue.allowed_target_projects, :selected => @issue.project), {:required => true}, - :onchange => "updateIssueFrom('#{escape_javascript project_issue_form_path(@project, :id => @issue, :format => 'js')}')" %>

+ :onchange => "updateIssueFrom('#{escape_javascript update_issue_form_path(@project, @issue)}')" %>

<% end %> <% if @issue.safe_attribute? 'tracker_id' %>

<%= f.select :tracker_id, @issue.project.trackers.collect {|t| [t.name, t.id]}, {:required => true}, - :onchange => "updateIssueFrom('#{escape_javascript project_issue_form_path(@project, :id => @issue, :format => 'js')}')" %>

+ :onchange => "updateIssueFrom('#{escape_javascript update_issue_form_path(@project, @issue)}')" %>

<% end %> <% if @issue.safe_attribute? 'subject' %> diff --git a/app/views/issues/update_form.js.erb b/app/views/issues/edit.js.erb similarity index 100% rename from app/views/issues/update_form.js.erb rename to app/views/issues/edit.js.erb diff --git a/app/views/issues/new.js.erb b/app/views/issues/new.js.erb new file mode 100644 index 000000000..a0c9ad052 --- /dev/null +++ b/app/views/issues/new.js.erb @@ -0,0 +1 @@ +replaceIssueFormWith('<%= escape_javascript(render :partial => 'form') %>'); diff --git a/config/routes.rb b/config/routes.rb index 4b9cc77aa..a32ec76ef 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -112,8 +112,8 @@ Rails.application.routes.draw do get 'issues/:copy_from/copy', :to => 'issues#new', :as => 'copy_issue' resources :issues, :only => [:index, :new, :create] - # issue form update - match 'issues/update_form', :controller => 'issues', :action => 'update_form', :via => [:put, :patch, :post], :as => 'issue_form' + # Used when updating the form of a new issue + post 'issues/new', :to => 'issues#new' resources :files, :only => [:index, :new, :create] @@ -168,6 +168,10 @@ Rails.application.routes.draw do end resources :issues do + member do + # Used when updating the form of an existing issue + patch 'edit', :to => 'issues#edit' + end collection do match 'bulk_edit', :via => [:get, :post] post 'bulk_update' diff --git a/lib/redmine.rb b/lib/redmine.rb index fc05a142d..f041082a8 100644 --- a/lib/redmine.rb +++ b/lib/redmine.rb @@ -98,14 +98,14 @@ Redmine::AccessControl.map do |map| :queries => :index, :reports => [:issue_report, :issue_report_details]}, :read => true - map.permission :add_issues, {:issues => [:new, :create, :update_form], :attachments => :upload} - map.permission :edit_issues, {:issues => [:edit, :update, :bulk_edit, :bulk_update, :update_form], :journals => [:new], :attachments => :upload} - map.permission :copy_issues, {:issues => [:new, :create, :bulk_edit, :bulk_update, :update_form], :attachments => :upload} + map.permission :add_issues, {:issues => [:new, :create], :attachments => :upload} + map.permission :edit_issues, {:issues => [:edit, :update, :bulk_edit, :bulk_update], :journals => [:new], :attachments => :upload} + map.permission :copy_issues, {:issues => [:new, :create, :bulk_edit, :bulk_update], :attachments => :upload} map.permission :manage_issue_relations, {:issue_relations => [:index, :show, :create, :destroy]} map.permission :manage_subtasks, {} map.permission :set_issues_private, {} map.permission :set_own_issues_private, {}, :require => :loggedin - map.permission :add_issue_notes, {:issues => [:edit, :update, :update_form], :journals => [:new], :attachments => :upload} + map.permission :add_issue_notes, {:issues => [:edit, :update], :journals => [:new], :attachments => :upload} map.permission :edit_issue_notes, {:journals => :edit}, :require => :loggedin map.permission :edit_own_issue_notes, {:journals => :edit}, :require => :loggedin map.permission :view_private_notes, {}, :read => true, :require => :member diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index 7a58d5e92..5e8762e08 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -1757,13 +1757,13 @@ class IssuesControllerTest < ActionController::TestCase def test_update_form_for_new_issue @request.session[:user_id] = 2 - xhr :post, :update_form, :project_id => 1, + xhr :post, :new, :project_id => 1, :issue => {:tracker_id => 2, :subject => 'This is the test_new issue', :description => 'This is the description', :priority_id => 5} assert_response :success - assert_template 'update_form' + assert_template 'new' assert_template :partial => '_form' assert_equal 'text/javascript', response.content_type @@ -1781,7 +1781,7 @@ class IssuesControllerTest < ActionController::TestCase 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 => 5, :new_status_id => 4) - xhr :post, :update_form, :project_id => 1, + xhr :post, :new, :project_id => 1, :issue => {:tracker_id => 1, :status_id => 5, :subject => 'This is an issue'} @@ -1796,7 +1796,7 @@ class IssuesControllerTest < ActionController::TestCase tracker.update! :default_status_id => 2 tracker.generate_transitions! 2, 1, :clear => true - xhr :post, :update_form, :project_id => 1, + xhr :post, :new, :project_id => 1, :issue => {:tracker_id => 2, :status_id => 1}, :was_default_status => 1 @@ -2776,15 +2776,14 @@ class IssuesControllerTest < ActionController::TestCase def test_update_form_for_existing_issue @request.session[:user_id] = 2 - xhr :put, :update_form, :project_id => 1, - :id => 1, + xhr :patch, :edit, :id => 1, :issue => {:tracker_id => 2, :subject => 'This is the test_new issue', :description => 'This is the description', :priority_id => 5} assert_response :success assert_equal 'text/javascript', response.content_type - assert_template 'update_form' + assert_template 'edit' assert_template :partial => '_form' issue = assigns(:issue) @@ -2797,7 +2796,7 @@ class IssuesControllerTest < ActionController::TestCase def test_update_form_for_existing_issue_should_keep_issue_author @request.session[:user_id] = 3 - xhr :put, :update_form, :project_id => 1, :id => 1, :issue => {:subject => 'Changed'} + xhr :patch, :edit, :id => 1, :issue => {:subject => 'Changed'} assert_response :success assert_equal 'text/javascript', response.content_type @@ -2814,8 +2813,7 @@ class IssuesControllerTest < ActionController::TestCase WorkflowTransition.create!(:role_id => 1, :tracker_id => 2, :old_status_id => 2, :new_status_id => 5) WorkflowTransition.create!(:role_id => 1, :tracker_id => 2, :old_status_id => 5, :new_status_id => 4) - xhr :put, :update_form, :project_id => 1, - :id => 2, + xhr :patch, :edit, :id => 2, :issue => {:tracker_id => 2, :status_id => 5, :subject => 'This is an issue'} @@ -2826,8 +2824,7 @@ class IssuesControllerTest < ActionController::TestCase def test_update_form_for_existing_issue_with_project_change @request.session[:user_id] = 2 - xhr :put, :update_form, :project_id => 1, - :id => 1, + xhr :patch, :edit, :id => 1, :issue => {:project_id => 2, :tracker_id => 2, :subject => 'This is the test_new issue', @@ -2849,7 +2846,7 @@ class IssuesControllerTest < ActionController::TestCase WorkflowTransition.delete_all WorkflowTransition.create!(:role_id => 1, :tracker_id => 2, :old_status_id => 2, :new_status_id => 3) - xhr :put, :update_form, :project_id => 1, :id => 2 + xhr :patch, :edit, :id => 2 assert_response :success assert_equal [2,3], assigns(:allowed_statuses).map(&:id).sort end diff --git a/test/integration/routing/issues_test.rb b/test/integration/routing/issues_test.rb index ded631795..1b59f3214 100644 --- a/test/integration/routing/issues_test.rb +++ b/test/integration/routing/issues_test.rb @@ -50,7 +50,7 @@ class RoutingIssuesTest < Redmine::RoutingTest end def test_issues_form_update - should_route 'POST /projects/23/issues/update_form' => 'issues#update_form', :project_id => '23' - should_route 'PUT /projects/23/issues/update_form' => 'issues#update_form', :project_id => '23' + should_route 'POST /projects/23/issues/new' => 'issues#new', :project_id => '23' + should_route 'PATCH /issues/23/edit' => 'issues#edit', :id => '23' end end -- 2.39.5