From 2c898d8a817ee5bd1182cf111db5f8fa1e91e870 Mon Sep 17 00:00:00 2001 From: Eric Davis Date: Thu, 22 Apr 2010 15:43:57 +0000 Subject: [PATCH] Refactor: Split IssuesController#new to #new and #create to match REST pattern. git-svn-id: svn+ssh://rubyforge.org/var/svn/redmine/trunk@3688 e93f8b46-1217-0410-a6f0-8f06a7374b81 --- app/controllers/issues_controller.rb | 77 ++++++++++++++--------- app/views/issues/new.rhtml | 2 +- config/routes.rb | 4 +- lib/redmine.rb | 2 +- test/functional/issues_controller_test.rb | 40 ++++++------ test/integration/routing_test.rb | 3 +- 6 files changed, 75 insertions(+), 53 deletions(-) diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb index 1d13afa0..798bb5a8 100644 --- a/app/controllers/issues_controller.rb +++ b/app/controllers/issues_controller.rb @@ -21,7 +21,7 @@ class IssuesController < ApplicationController before_filter :find_issue, :only => [:show, :edit, :update, :reply] before_filter :find_issues, :only => [:bulk_edit, :move, :destroy] - before_filter :find_project, :only => [:new, :update_form, :preview, :auto_complete] + before_filter :find_project, :only => [:new, :create, :update_form, :preview, :auto_complete] before_filter :authorize, :except => [:index, :changes, :gantt, :calendar, :preview, :context_menu] before_filter :find_optional_project, :only => [:index, :changes, :gantt, :calendar] accept_key_auth :index, :show, :changes @@ -51,6 +51,7 @@ class IssuesController < ApplicationController :only => :destroy, :render => { :nothing => true, :status => :method_not_allowed } + verify :method => :post, :only => :create, :render => {:nothing => true, :status => :method_not_allowed } verify :method => :put, :only => :update, :render => {:nothing => true, :status => :method_not_allowed } def index @@ -145,35 +146,55 @@ class IssuesController < ApplicationController @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project) end @issue.author = User.current - - if request.get? || request.xhr? - @issue.start_date ||= Date.today - else - call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue }) - if @issue.save - attachments = Attachment.attach_files(@issue, params[:attachments]) - render_attachment_warning_if_needed(@issue) - flash[:notice] = l(:notice_successful_create) - call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue}) - respond_to do |format| - format.html { - redirect_to(params[:continue] ? { :action => 'new', :issue => {:tracker_id => @issue.tracker, - :parent_issue_id => @issue.parent_issue_id}.reject {|k,v| v.nil?} } : - { :action => 'show', :id => @issue }) - } - format.xml { render :action => 'show', :status => :created, :location => url_for(:controller => 'issues', :action => 'show', :id => @issue) } - end - return - else - respond_to do |format| - format.html { } - format.xml { render(:xml => @issue.errors, :status => :unprocessable_entity); return } - end - end - end + @issue.start_date ||= Date.today @priorities = IssuePriority.all @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true) - render :layout => !request.xhr? + render :action => 'new', :layout => !request.xhr? + end + + def create + @issue = Issue.new + @issue.copy_from(params[:copy_from]) if params[:copy_from] + @issue.project = @project + # Tracker must be set before custom field values + @issue.tracker ||= @project.trackers.find((params[:issue] && params[:issue][:tracker_id]) || params[:tracker_id] || :first) + if @issue.tracker.nil? + render_error l(:error_no_tracker_in_project) + return + end + if @issue.status.nil? + render_error l(:error_no_default_issue_status) + return + end + if params[:issue].is_a?(Hash) + @issue.safe_attributes = params[:issue] + @issue.watcher_user_ids = params[:issue]['watcher_user_ids'] if User.current.allowed_to?(:add_issue_watchers, @project) + end + @issue.author = User.current + + @priorities = IssuePriority.all + @allowed_statuses = @issue.new_statuses_allowed_to(User.current, true) + + call_hook(:controller_issues_new_before_save, { :params => params, :issue => @issue }) + if @issue.save + attachments = Attachment.attach_files(@issue, params[:attachments]) + render_attachment_warning_if_needed(@issue) + flash[:notice] = l(:notice_successful_create) + call_hook(:controller_issues_new_after_save, { :params => params, :issue => @issue}) + respond_to do |format| + format.html { + redirect_to(params[:continue] ? { :action => 'new', :issue => {:tracker_id => @issue.tracker, :parent_issue_id => @issue.parent_issue_id}.reject {|k,v| v.nil?} } : + { :action => 'show', :id => @issue }) + } + format.xml { render :action => 'show', :status => :created, :location => url_for(:controller => 'issues', :action => 'show', :id => @issue) } + end + return + else + respond_to do |format| + format.html { render :action => 'new' } + format.xml { render(:xml => @issue.errors, :status => :unprocessable_entity); return } + end + end end # Attributes that can be updated on workflow transition (without :edit permission) diff --git a/app/views/issues/new.rhtml b/app/views/issues/new.rhtml index 94f86dde..839286bd 100644 --- a/app/views/issues/new.rhtml +++ b/app/views/issues/new.rhtml @@ -1,6 +1,6 @@

<%=l(:label_issue_new)%>

-<% labelled_tabular_form_for :issue, @issue, +<% labelled_tabular_form_for :issue, @issue, :url => {:controller => 'issues', :action => 'create', :project_id => @project}, :html => {:multipart => true, :id => 'issue-form'} do |f| %> <%= error_messages_for 'issue' %>
diff --git a/config/routes.rb b/config/routes.rb index 8c707f2e..2e10cd65 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -120,10 +120,10 @@ ActionController::Routing::Routes.draw do |map| end issues_routes.with_options :conditions => {:method => :post} do |issues_actions| issues_actions.connect 'issues', :action => 'index' - issues_actions.connect 'projects/:project_id/issues', :action => 'new' + issues_actions.connect 'projects/:project_id/issues', :action => 'create' issues_actions.connect 'issues/:id/quoted', :action => 'reply', :id => /\d+/ issues_actions.connect 'issues/:id/:action', :action => /edit|move|destroy/, :id => /\d+/ - issues_actions.connect 'issues.:format', :action => 'new', :format => /xml/ + issues_actions.connect 'issues.:format', :action => 'create', :format => /xml/ end issues_routes.with_options :conditions => {:method => :put} do |issues_actions| issues_actions.connect 'issues/:id/edit', :action => 'update', :id => /\d+/ diff --git a/lib/redmine.rb b/lib/redmine.rb index f510d5d1..ce8e3211 100644 --- a/lib/redmine.rb +++ b/lib/redmine.rb @@ -62,7 +62,7 @@ Redmine::AccessControl.map do |map| :versions => [:show, :status_by], :queries => :index, :reports => [:issue_report, :issue_report_details]} - map.permission :add_issues, {:issues => [:new, :update_form]} + map.permission :add_issues, {:issues => [:new, :create, :update_form]} map.permission :edit_issues, {:issues => [:edit, :update, :reply, :bulk_edit, :update_form]} map.permission :manage_issue_relations, {:issue_relations => [:new, :destroy]} map.permission :manage_subtasks, {} diff --git a/test/functional/issues_controller_test.rb b/test/functional/issues_controller_test.rb index a0def242..97835133 100644 --- a/test/functional/issues_controller_test.rb +++ b/test/functional/issues_controller_test.rb @@ -446,10 +446,10 @@ class IssuesControllerTest < ActionController::TestCase assert_equal 'This is the test_new issue', issue.subject end - def test_post_new + def test_post_create @request.session[:user_id] = 2 assert_difference 'Issue.count' do - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 3, :status_id => 2, :subject => 'This is the test_new issue', @@ -471,9 +471,9 @@ class IssuesControllerTest < ActionController::TestCase assert_equal 'Value for field 2', v.value end - def test_post_new_and_continue + def test_post_create_and_continue @request.session[:user_id] = 2 - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 3, :subject => 'This is first issue', :priority_id => 5}, @@ -481,10 +481,10 @@ class IssuesControllerTest < ActionController::TestCase assert_redirected_to :controller => 'issues', :action => 'new', :issue => {:tracker_id => 3} end - def test_post_new_without_custom_fields_param + def test_post_create_without_custom_fields_param @request.session[:user_id] = 2 assert_difference 'Issue.count' do - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 1, :subject => 'This is the test_new issue', :description => 'This is the description', @@ -493,12 +493,12 @@ class IssuesControllerTest < ActionController::TestCase assert_redirected_to :controller => 'issues', :action => 'show', :id => Issue.last.id end - def test_post_new_with_required_custom_field_and_without_custom_fields_param + def test_post_create_with_required_custom_field_and_without_custom_fields_param field = IssueCustomField.find_by_name('Database') field.update_attribute(:is_required, true) @request.session[:user_id] = 2 - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 1, :subject => 'This is the test_new issue', :description => 'This is the description', @@ -510,12 +510,12 @@ class IssuesControllerTest < ActionController::TestCase assert_equal I18n.translate('activerecord.errors.messages.invalid'), issue.errors.on(:custom_values) end - def test_post_new_with_watchers + def test_post_create_with_watchers @request.session[:user_id] = 2 ActionMailer::Base.deliveries.clear assert_difference 'Watcher.count', 2 do - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 1, :subject => 'This is a new issue with watchers', :description => 'This is the description', @@ -535,11 +535,11 @@ class IssuesControllerTest < ActionController::TestCase assert [mail.bcc, mail.cc].flatten.include?(User.find(3).mail) end - def test_post_new_subissue + def test_post_create_subissue @request.session[:user_id] = 2 assert_difference 'Issue.count' do - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 1, :subject => 'This is a child issue', :parent_issue_id => 2} @@ -549,11 +549,11 @@ class IssuesControllerTest < ActionController::TestCase assert_equal Issue.find(2), issue.parent end - def test_post_new_should_send_a_notification + def test_post_create_should_send_a_notification ActionMailer::Base.deliveries.clear @request.session[:user_id] = 2 assert_difference 'Issue.count' do - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 3, :subject => 'This is the test_new issue', :description => 'This is the description', @@ -566,9 +566,9 @@ class IssuesControllerTest < ActionController::TestCase assert_equal 1, ActionMailer::Base.deliveries.size end - def test_post_should_preserve_fields_values_on_validation_failure + def test_post_create_should_preserve_fields_values_on_validation_failure @request.session[:user_id] = 2 - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 1, # empty subject :subject => '', @@ -593,10 +593,10 @@ class IssuesControllerTest < ActionController::TestCase :value => 'Value for field 2'} end - def test_post_new_should_ignore_non_safe_attributes + def test_post_create_should_ignore_non_safe_attributes @request.session[:user_id] = 2 assert_nothing_raised do - post :new, :project_id => 1, :issue => { :tracker => "A param can not be a Tracker" } + post :create, :project_id => 1, :issue => { :tracker => "A param can not be a Tracker" } end end @@ -619,7 +619,7 @@ class IssuesControllerTest < ActionController::TestCase should "accept default status" do assert_difference 'Issue.count' do - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 1, :subject => 'This is an issue', :status_id => 1} @@ -630,7 +630,7 @@ class IssuesControllerTest < ActionController::TestCase should "ignore unauthorized status" do assert_difference 'Issue.count' do - post :new, :project_id => 1, + post :create, :project_id => 1, :issue => {:tracker_id => 1, :subject => 'This is an issue', :status_id => 3} diff --git a/test/integration/routing_test.rb b/test/integration/routing_test.rb index e51f3464..abb18262 100644 --- a/test/integration/routing_test.rb +++ b/test/integration/routing_test.rb @@ -70,7 +70,8 @@ class RoutingTest < ActionController::IntegrationTest should_route :get, "/issues/64.xml", :controller => 'issues', :action => 'show', :id => '64', :format => 'xml' should_route :get, "/projects/23/issues/new", :controller => 'issues', :action => 'new', :project_id => '23' - should_route :post, "/issues.xml", :controller => 'issues', :action => 'new', :format => 'xml' + should_route :post, "/projects/23/issues", :controller => 'issues', :action => 'create', :project_id => '23' + should_route :post, "/issues.xml", :controller => 'issues', :action => 'create', :format => 'xml' should_route :get, "/issues/64/edit", :controller => 'issues', :action => 'edit', :id => '64' # TODO: Should use PUT