Let the new time_entry form be submitted without project (#17954).

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

7 files changed, 124 insertions, 103 deletions

diff --git a/app/controllers/timelog_controller.rb b/app/controllers/timelog_controller.rb
index e97a596f0..13923b1e5 100644
--- a/app/controllers/timelog_controller.rb
+++ b/app/controllers/timelog_controller.rb
@@ -18,14 +18,12 @@
 class TimelogController < ApplicationController
   menu_item :issues
 
-  before_filter :find_project_for_new_time_entry, :only => [:create]
   before_filter :find_time_entry, :only => [:show, :edit, :update]
   before_filter :find_time_entries, :only => [:bulk_edit, :bulk_update, :destroy]
-  before_filter :authorize, :except => [:new, :index, :report]
+  before_filter :authorize, :only => [:show, :edit, :update, :bulk_edit, :bulk_update, :destroy]
 
-  before_filter :find_optional_project, :only => [:index, :report]
-  before_filter :find_optional_project_for_new_time_entry, :only => [:new]
-  before_filter :authorize_global, :only => [:new, :index, :report]
+  before_filter :find_optional_project, :only => [:new, :create, :index, :report]
+  before_filter :authorize_global, :only => [:new, :create, :index, :report]
 
   accept_rss_auth :index
   accept_api_auth :index, :show, :create, :update, :destroy
@@ -104,6 +102,10 @@ class TimelogController < ApplicationController
   def create
     @time_entry ||= TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => User.current.today)
     @time_entry.safe_attributes = params[:time_entry]
+    if @time_entry.project && !User.current.allowed_to?(:log_time, @time_entry.project)
+      render_403
+      return
+    end
 
     call_hook(:controller_timelog_edit_before_save, { :params => params, :time_entry => @time_entry })
 
@@ -112,21 +114,19 @@ class TimelogController < ApplicationController
         format.html {
           flash[:notice] = l(:notice_successful_create)
           if params[:continue]
-            if params[:project_id]
-              options = {
-                :time_entry => {:issue_id => @time_entry.issue_id, :activity_id => @time_entry.activity_id},
-                :back_url => params[:back_url]
-              }
-              if @time_entry.issue
-                redirect_to new_project_issue_time_entry_path(@time_entry.project, @time_entry.issue, options)
-              else
-                redirect_to new_project_time_entry_path(@time_entry.project, options)
-              end
+            options = {
+              :time_entry => {
+                :project_id => params[:time_entry][:project_id],
+                :issue_id => @time_entry.issue_id,
+                :activity_id => @time_entry.activity_id
+              },
+              :back_url => params[:back_url]
+            }
+            if params[:project_id] && @time_entry.project
+              redirect_to new_project_time_entry_path(@time_entry.project, options)
+            elsif params[:issue_id] && @time_entry.issue
+              redirect_to new_issue_time_entry_path(@time_entry.issue, options)
             else
-              options = {
-                :time_entry => {:project_id => @time_entry.project_id, :issue_id => @time_entry.issue_id, :activity_id => @time_entry.activity_id},
-                :back_url => params[:back_url]
-              }
               redirect_to new_time_entry_path(options)
             end
           else
@@ -251,32 +251,15 @@ private
     end
   end
 
-  def find_optional_project_for_new_time_entry
-    if (project_id = (params[:project_id] || params[:time_entry] && params[:time_entry][:project_id])).present?
-      @project = Project.find(project_id)
-    end
-    if (issue_id = (params[:issue_id] || params[:time_entry] && params[:time_entry][:issue_id])).present?
-      @issue = Issue.find(issue_id)
-      @project ||= @issue.project
-    end
-  rescue ActiveRecord::RecordNotFound
-    render_404
-  end
-
-  def find_project_for_new_time_entry
-    find_optional_project_for_new_time_entry
-    if @project.nil?
-      render_404
-    end
-  end
-
   def find_optional_project
-    if !params[:issue_id].blank?
+    if params[:issue_id].present?
       @issue = Issue.find(params[:issue_id])
       @project = @issue.project
-    elsif !params[:project_id].blank?
+    elsif params[:project_id].present?
       @project = Project.find(params[:project_id])
     end
+  rescue ActiveRecord::RecordNotFound
+    render_404
   end
 
   # Returns the TimeEntry scope for index and report actions
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 22981cf14..e9cbf6dd4 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -349,7 +349,10 @@ module ApplicationHelper
   end
 
   def project_tree_options_for_select(projects, options = {})
-    s = ''
+    s = ''.html_safe
+    if options[:include_blank]
+      s << content_tag('option', '&nbsp;'.html_safe, :value => '')
+    end
     project_tree(projects) do |project, level|
       name_prefix = (level > 0 ? '&nbsp;' * 2 * level + '&#187; ' : '').html_safe
       tag_options = {:value => project.id}
diff --git a/app/models/time_entry.rb b/app/models/time_entry.rb
index 103d5ae62..662070840 100644
--- a/app/models/time_entry.rb
+++ b/app/models/time_entry.rb
@@ -24,7 +24,7 @@ class TimeEntry < ActiveRecord::Base
   belongs_to :user
   belongs_to :activity, :class_name => 'TimeEntryActivity', :foreign_key => 'activity_id'
 
-  attr_protected :project_id, :user_id, :tyear, :tmonth, :tweek
+  attr_protected :user_id, :tyear, :tmonth, :tweek
 
   acts_as_customizable
   acts_as_event :title => Proc.new {|o| "#{l_hours(o.hours)} (#{(o.issue || o.project).event_title})"},
@@ -65,7 +65,7 @@ class TimeEntry < ActiveRecord::Base
     end
   }
 
-  safe_attributes 'hours', 'comments', 'issue_id', 'activity_id', 'spent_on', 'custom_field_values', 'custom_fields'
+  safe_attributes 'hours', 'comments', 'project_id', 'issue_id', 'activity_id', 'spent_on', 'custom_field_values', 'custom_fields'
 
   def initialize(attributes=nil, *args)
     super
@@ -78,10 +78,12 @@ class TimeEntry < ActiveRecord::Base
   end
 
   def safe_attributes=(attrs, user=User.current)
-    attrs = super
-    if !new_record? && issue && issue.project_id != project_id
-      if user.allowed_to?(:log_time, issue.project)
-        self.project_id = issue.project_id
+    if attrs
+      attrs = super(attrs)
+      if issue_id_changed? && attrs[:project_id].blank? && issue && issue.project_id != project_id
+        if user.allowed_to?(:log_time, issue.project)
+          self.project_id = issue.project_id
+        end
       end
     end
     attrs
diff --git a/app/views/issues/show.html.erb b/app/views/issues/show.html.erb
index 5a855c9a5..e5a3298668 100644
--- a/app/views/issues/show.html.erb
+++ b/app/views/issues/show.html.erb
@@ -61,7 +61,7 @@
     end
   end
   if User.current.allowed_to?(:view_time_entries, @project)
-    rows.right l(:label_spent_time), (@issue.total_spent_hours > 0 ? link_to(l_hours(@issue.total_spent_hours), project_issue_time_entries_path(@project, @issue)) : "-"), :class => 'spent-time'
+    rows.right l(:label_spent_time), (@issue.total_spent_hours > 0 ? link_to(l_hours(@issue.total_spent_hours), issue_time_entries_path(@issue)) : "-"), :class => 'spent-time'
   end
 end %>
 <%= render_custom_fields_rows(@issue) %>
diff --git a/app/views/timelog/_form.html.erb b/app/views/timelog/_form.html.erb
index 3256feaac..b51f98dad 100644
--- a/app/views/timelog/_form.html.erb
+++ b/app/views/timelog/_form.html.erb
@@ -3,10 +3,12 @@
 
 <div class="box tabular">
   <% if @time_entry.new_record? %>
-    <% if params[:project_id] || @time_entry.issue %>
-      <%= f.hidden_field :project_id %>
+    <% if params[:project_id] %>
+      <%= hidden_field_tag 'project_id', params[:project_id] %>
+    <% elsif params[:issue_id] %>
+      <%= hidden_field_tag 'issue_id', params[:issue_id] %>
     <% else %>
-      <p><%= f.select :project_id, project_tree_options_for_select(Project.allowed_to(:log_time).all, :selected => @time_entry.project), :required => true %></p>
+      <p><%= f.select :project_id, project_tree_options_for_select(Project.allowed_to(:log_time).all, :selected => @time_entry.project, :include_blank => true) %></p>
     <% end %>
   <% end %>
   <p>
diff --git a/app/views/timelog/new.html.erb b/app/views/timelog/new.html.erb
index 95253e82f..84bf7dae1 100644
--- a/app/views/timelog/new.html.erb
+++ b/app/views/timelog/new.html.erb
@@ -1,7 +1,6 @@
 <h2><%= l(:label_spent_time) %></h2>
 
 <%= labelled_form_for @time_entry, :url => time_entries_path do |f| %>
-  <%= hidden_field_tag 'project_id', params[:project_id] if params[:project_id] %>
   <%= render :partial => 'form', :locals => {:f => f} %>
   <%= submit_tag l(:button_create) %>
   <%= submit_tag l(:button_create_and_continue), :name => 'continue' %>
diff --git a/test/functional/timelog_controller_test.rb b/test/functional/timelog_controller_test.rb
index a64775039..4760b288d 100644
--- a/test/functional/timelog_controller_test.rb
+++ b/test/functional/timelog_controller_test.rb
@@ -28,31 +28,37 @@ class TimelogControllerTest < ActionController::TestCase
 
   include Redmine::I18n
 
-  def test_new_with_project_id
+  def test_new
     @request.session[:user_id] = 3
-    get :new, :project_id => 1
+    get :new
     assert_response :success
     assert_template 'new'
-    assert_select 'select[name=?]', 'time_entry[project_id]', 0
-    assert_select 'input[name=?][value=1][type=hidden]', 'time_entry[project_id]'
+    assert_select 'input[name=?][type=hidden]', 'project_id', 0
+    assert_select 'input[name=?][type=hidden]', 'issue_id', 0
+    assert_select 'select[name=?]', 'time_entry[project_id]' do
+      # blank option for project
+      assert_select 'option[value=]'
+    end
   end
 
-  def test_new_with_issue_id
+  def test_new_with_project_id
     @request.session[:user_id] = 3
-    get :new, :issue_id => 2
+    get :new, :project_id => 1
     assert_response :success
     assert_template 'new'
+    assert_select 'input[name=?][type=hidden]', 'project_id'
+    assert_select 'input[name=?][type=hidden]', 'issue_id', 0
     assert_select 'select[name=?]', 'time_entry[project_id]', 0
-    assert_select 'input[name=?][value=1][type=hidden]', 'time_entry[project_id]'
   end
 
-  def test_new_without_project
+  def test_new_with_issue_id
     @request.session[:user_id] = 3
-    get :new
+    get :new, :issue_id => 2
     assert_response :success
     assert_template 'new'
-    assert_select 'select[name=?]', 'time_entry[project_id]'
-    assert_select 'input[name=?]', 'time_entry[project_id]', 0
+    assert_select 'input[name=?][type=hidden]', 'project_id', 0
+    assert_select 'input[name=?][type=hidden]', 'issue_id'
+    assert_select 'select[name=?]', 'time_entry[project_id]', 0
   end
 
   def test_new_without_project_should_prefill_the_form
@@ -63,7 +69,6 @@ class TimelogControllerTest < ActionController::TestCase
     assert_select 'select[name=?]', 'time_entry[project_id]' do
       assert_select 'option[value=1][selected=selected]'
     end
-    assert_select 'input[name=?]', 'time_entry[project_id]', 0
   end
 
   def test_new_without_project_should_deny_without_permission
@@ -113,80 +118,101 @@ class TimelogControllerTest < ActionController::TestCase
   end
 
   def test_post_create
-    # TODO: should POST to issues’ time log instead of project. change form
-    # and routing
     @request.session[:user_id] = 3
-    post :create, :project_id => 1,
+    assert_difference 'TimeEntry.count' do
+      post :create, :project_id => 1,
                 :time_entry => {:comments => 'Some work on TimelogControllerTest',
                                 # Not the default activity
                                 :activity_id => '11',
                                 :spent_on => '2008-03-14',
                                 :issue_id => '1',
                                 :hours => '7.3'}
-    assert_redirected_to :action => 'index', :project_id => 'ecookbook'
+      assert_redirected_to '/projects/ecookbook/time_entries'
+    end
 
-    i = Issue.find(1)
-    t = TimeEntry.find_by_comments('Some work on TimelogControllerTest')
+    t = TimeEntry.order('id DESC').first
     assert_not_nil t
+    assert_equal 'Some work on TimelogControllerTest', t.comments
+    assert_equal 1, t.project_id
+    assert_equal 1, t.issue_id
     assert_equal 11, t.activity_id
     assert_equal 7.3, t.hours
     assert_equal 3, t.user_id
-    assert_equal i, t.issue
-    assert_equal i.project, t.project
   end
 
   def test_post_create_with_blank_issue
-    # TODO: should POST to issues’ time log instead of project. change form
-    # and routing
     @request.session[:user_id] = 3
-    post :create, :project_id => 1,
+    assert_difference 'TimeEntry.count' do
+      post :create, :project_id => 1,
                 :time_entry => {:comments => 'Some work on TimelogControllerTest',
                                 # Not the default activity
                                 :activity_id => '11',
                                 :issue_id => '',
                                 :spent_on => '2008-03-14',
                                 :hours => '7.3'}
-    assert_redirected_to :action => 'index', :project_id => 'ecookbook'
+      assert_redirected_to '/projects/ecookbook/time_entries'
+    end
 
-    t = TimeEntry.find_by_comments('Some work on TimelogControllerTest')
+    t = TimeEntry.order('id DESC').first
     assert_not_nil t
+    assert_equal 'Some work on TimelogControllerTest', t.comments
+    assert_equal 1, t.project_id
+    assert_nil t.issue_id
     assert_equal 11, t.activity_id
     assert_equal 7.3, t.hours
     assert_equal 3, t.user_id
   end
 
-  def test_create_and_continue
+  def test_create_and_continue_at_project_level
     @request.session[:user_id] = 2
-    post :create, :project_id => 1,
-                :time_entry => {:activity_id => '11',
-                                :issue_id => '',
-                                :spent_on => '2008-03-14',
-                                :hours => '7.3'},
-                :continue => '1'
-    assert_redirected_to '/projects/ecookbook/time_entries/new?time_entry%5Bactivity_id%5D=11&time_entry%5Bissue_id%5D='
+    assert_difference 'TimeEntry.count' do
+      post :create, :time_entry => {:project_id => '1',
+                                    :activity_id => '11',
+                                    :issue_id => '',
+                                    :spent_on => '2008-03-14',
+                                    :hours => '7.3'},
+                    :continue => '1'
+      assert_redirected_to '/time_entries/new?time_entry%5Bactivity_id%5D=11&time_entry%5Bissue_id%5D=&time_entry%5Bproject_id%5D=1'
+    end
   end
 
-  def test_create_and_continue_with_issue_id
+  def test_create_and_continue_at_issue_level
     @request.session[:user_id] = 2
-    post :create, :project_id => 1,
-                :time_entry => {:activity_id => '11',
-                                :issue_id => '1',
-                                :spent_on => '2008-03-14',
-                                :hours => '7.3'},
-                :continue => '1'
-    assert_redirected_to '/projects/ecookbook/issues/1/time_entries/new?time_entry%5Bactivity_id%5D=11&time_entry%5Bissue_id%5D=1'
+    assert_difference 'TimeEntry.count' do
+      post :create, :time_entry => {:project_id => '',
+                                    :activity_id => '11',
+                                    :issue_id => '1',
+                                    :spent_on => '2008-03-14',
+                                    :hours => '7.3'},
+                    :continue => '1'
+      assert_redirected_to '/time_entries/new?time_entry%5Bactivity_id%5D=11&time_entry%5Bissue_id%5D=1&time_entry%5Bproject_id%5D='
+    end
   end
 
-  def test_create_and_continue_without_project
+  def test_create_and_continue_with_project_id
     @request.session[:user_id] = 2
-    post :create, :time_entry => {:project_id => '1',
-                                :activity_id => '11',
-                                :issue_id => '',
-                                :spent_on => '2008-03-14',
-                                :hours => '7.3'},
-                  :continue => '1'
+    assert_difference 'TimeEntry.count' do
+      post :create, :project_id => 1,
+                    :time_entry => {:activity_id => '11',
+                                    :issue_id => '',
+                                    :spent_on => '2008-03-14',
+                                    :hours => '7.3'},
+                    :continue => '1'
+      assert_redirected_to '/projects/ecookbook/time_entries/new?time_entry%5Bactivity_id%5D=11&time_entry%5Bissue_id%5D=&time_entry%5Bproject_id%5D='
+    end
+  end
 
-    assert_redirected_to '/time_entries/new?time_entry%5Bactivity_id%5D=11&time_entry%5Bissue_id%5D=&time_entry%5Bproject_id%5D=1'
+  def test_create_and_continue_with_issue_id
+    @request.session[:user_id] = 2
+    assert_difference 'TimeEntry.count' do
+      post :create, :issue_id => 1,
+                    :time_entry => {:activity_id => '11',
+                                    :issue_id => '1',
+                                    :spent_on => '2008-03-14',
+                                    :hours => '7.3'},
+                    :continue => '1'
+      assert_redirected_to '/issues/1/time_entries/new?time_entry%5Bactivity_id%5D=11&time_entry%5Bissue_id%5D=1&time_entry%5Bproject_id%5D='
+    end
   end
 
   def test_create_without_log_time_permission_should_be_denied
@@ -201,6 +227,14 @@ class TimelogControllerTest < ActionController::TestCase
     assert_response 403
   end
 
+  def test_create_without_project_and_issue_should_fail
+    @request.session[:user_id] = 2
+    post :create, :time_entry => {:issue_id => ''}
+
+    assert_response :success
+    assert_template 'new'
+  end
+
   def test_create_with_failure
     @request.session[:user_id] = 2
     post :create, :project_id => 1,
@@ -546,8 +580,6 @@ class TimelogControllerTest < ActionController::TestCase
     # display all time
     assert_nil assigns(:from)
     assert_nil assigns(:to)
-    # TODO: remove /projects/:project_id/issues/:issue_id/time_entries routes
-    # to use /issues/:issue_id/time_entries
     assert_tag :form,
       :attributes => {:action => "/projects/ecookbook/issues/1/time_entries", :id => 'query_form'}
   end