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

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index 1d13afa0ef96660340202c7bf38f0107a840bd77..798bb5a83dfcc2d56743efc9917459fe7a93c19d 100644 (file)
--- 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 :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)
-    render :layout => !request.xhr?
+
+    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 94f86dde912b5e4b81321555567b621e6c879ba2..839286bdbb17d946f9c8099a13f117ef660199a6 100644 (file)
--- a/app/views/issues/new.rhtml
+++ b/app/views/issues/new.rhtml
@@ -1,6 +1,6 @@
 <h2><%=l(:label_issue_new)%></h2>
 
-<% 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' %>
     <div class="box">

diff --git a/config/routes.rb b/config/routes.rb
index 8c707f2e28f4642e159dc03d6ca0805a5e2b7855..2e10cd6513d1b70db5c9465629d1c07eb84a9646 100644 (file)
--- 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 f510d5d11f97d94756b2afaab43c92d61a5780fc..ce8e3211b2c48edd276dd7e272a43c7e87f16db6 100644 (file)
--- 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 a0def24232b2c0482068d4130c4eafd072920d40..97835133fbe8aad656537442fcb0d2b52d2a6f4a 100644 (file)
--- 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 e51f34642f820da4166aa6d3838536ccc34a5a5f..abb182623681b62c89473bd1e49e0890900457ba 100644 (file)
--- 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