Merged r19676 to r19678 to 4.1-stable (#32774).

git-svn-id: http://svn.redmine.org/redmine/branches/4.1-stable@19679 e93f8b46-1217-0410-a6f0-8f06a7374b81

7 files changed, 59 insertions, 24 deletions

diff --git a/app/controllers/issues_controller.rb b/app/controllers/issues_controller.rb
index f5103a821..65caee650 100644
--- a/app/controllers/issues_controller.rb
+++ b/app/controllers/issues_controller.rb
@@ -577,6 +577,7 @@ class IssuesController < ApplicationController
         time_entry = @time_entry || TimeEntry.new
         time_entry.project = @issue.project
         time_entry.issue = @issue
+        time_entry.author = User.current
         time_entry.user = User.current
         time_entry.spent_on = User.current.today
         time_entry.safe_attributes = params[:time_entry]
diff --git a/app/controllers/timelog_controller.rb b/app/controllers/timelog_controller.rb
index 4c4e7df79..1d144b983 100644
--- a/app/controllers/timelog_controller.rb
+++ b/app/controllers/timelog_controller.rb
@@ -28,8 +28,6 @@ class TimelogController < ApplicationController
   before_action :find_optional_issue, :only => [:new, :create]
   before_action :find_optional_project, :only => [:index, :report]
 
-  before_action :authorize_logging_time_for_other_users, :only => [:create, :update]
-
   accept_rss_auth :index
   accept_api_auth :index, :show, :create, :update, :destroy
 
@@ -258,13 +256,6 @@ class TimelogController < ApplicationController
     end
   end
 
-  def authorize_logging_time_for_other_users
-    if !User.current.allowed_to?(:log_time_for_other_users, @project) && params['time_entry'].present? && params['time_entry']['user_id'].present? && params['time_entry']['user_id'].to_i != User.current.id
-      render_error :message => l(:error_not_allowed_to_log_time_for_other_users), :status => 403
-      return false
-    end
-  end
-
   def find_time_entries
     @time_entries = TimeEntry.where(:id => params[:id] || params[:ids]).
       preload(:project => :time_entry_activities).
diff --git a/app/models/time_entry.rb b/app/models/time_entry.rb
index 943613a82..1f3b0a7bf 100644
--- a/app/models/time_entry.rb
+++ b/app/models/time_entry.rb
@@ -50,6 +50,7 @@ class TimeEntry < ActiveRecord::Base
   validates_length_of :comments, :maximum => 1024, :allow_nil => true
   validates :spent_on, :date => true
   before_validation :set_project_if_nil
+  #TODO: remove this, author should be always explicitly set
   before_validation :set_author_if_nil
   validate :validate_time_entry
 
@@ -116,6 +117,11 @@ class TimeEntry < ActiveRecord::Base
           @invalid_issue_id = issue_id
         end
       end
+      if user_id_changed? && user_id != author_id && !user.allowed_to?(:log_time_for_other_users, project)
+        @invalid_user_id = user_id
+      else
+        @invalid_user_id = nil
+      end
     end
     attrs
   end
@@ -146,7 +152,7 @@ class TimeEntry < ActiveRecord::Base
       end
     end
     errors.add :project_id, :invalid if project.nil?
-    if user_id_changed? && user_id != author_id && !self.assignable_users.map(&:id).include?(user_id)
+    if @invalid_user_id || (user_id_changed? && user_id != author_id && !self.assignable_users.map(&:id).include?(user_id))
       errors.add :user_id, :invalid
     end
     errors.add :issue_id, :invalid if (issue_id && !issue) || (issue && project!=issue.project) || @invalid_issue_id
diff --git a/app/models/time_entry_import.rb b/app/models/time_entry_import.rb
index 0ac4429f4..07d2c2276 100644
--- a/app/models/time_entry_import.rb
+++ b/app/models/time_entry_import.rb
@@ -85,7 +85,7 @@ class TimeEntryImport < Import
     end
 
     user_id = nil
-    if User.current.allowed_to?(:log_time_for_other_users, project)
+    if user.allowed_to?(:log_time_for_other_users, project)
       if user_value
         user_id = user_value
       elsif user_name = row_value(row, 'user_id')
@@ -98,6 +98,7 @@ class TimeEntryImport < Import
     attributes = {
       :project_id  => project.id,
       :activity_id => activity_id,
+      :author_id   => user.id,
       :user_id     => user_id,
 
       :issue_id    => row_value(row, 'issue_id'),
diff --git a/test/functional/timelog_controller_test.rb b/test/functional/timelog_controller_test.rb
index 43c53a576..3b17c563b 100644
--- a/test/functional/timelog_controller_test.rb
+++ b/test/functional/timelog_controller_test.rb
@@ -383,7 +383,7 @@ class TimelogControllerTest < Redmine::ControllerTest
     assert_equal 2, t.author_id
   end
 
-  def test_create_for_other_user_should_deny_for_user_without_permission
+  def test_create_for_other_user_should_fail_without_permission
     Role.find_by_name('Manager').remove_permission! :log_time_for_other_users
     @request.session[:user_id] = 2
 
@@ -399,8 +399,8 @@ class TimelogControllerTest < Redmine::ControllerTest
       }
     }
 
-    assert_response 403
-    assert_select 'p[id=?]', 'errorExplanation', :text => I18n.t(:error_not_allowed_to_log_time_for_other_users)
+    assert_response :success
+    assert_select_error /User is invalid/
   end
 
   def test_create_and_continue_at_project_level
@@ -668,7 +668,7 @@ class TimelogControllerTest < Redmine::ControllerTest
     assert_select_error /Issue is invalid/
   end
 
-  def test_update_should_deny_changing_user_for_user_without_permission
+  def test_update_should_fail_when_changing_user_without_permission
     Role.find_by_name('Manager').remove_permission! :log_time_for_other_users
     @request.session[:user_id] = 2
 
@@ -679,8 +679,8 @@ class TimelogControllerTest < Redmine::ControllerTest
       }
     }
 
-    assert_response 403
-    assert_select 'p[id=?]', 'errorExplanation', :text => I18n.t(:error_not_allowed_to_log_time_for_other_users)
+    assert_response :success
+    assert_select_error /User is invalid/
   end
 
   def test_update_should_allow_updating_existing_entry_logged_on_a_locked_user
diff --git a/test/integration/api_test/time_entries_test.rb b/test/integration/api_test/time_entries_test.rb
index f42d0ae97..4557032be 100644
--- a/test/integration/api_test/time_entries_test.rb
+++ b/test/integration/api_test/time_entries_test.rb
@@ -144,6 +144,40 @@ class Redmine::ApiTest::TimeEntriesTest < Redmine::ApiTest::Base
     assert_select 'errors error', :text => "Hours cannot be blank"
   end
 
+  test "POST /time_entries.xml with :project_id for other user" do
+    Role.find_by_name('Manager').add_permission! :log_time_for_other_users
+
+    entry = new_record(TimeEntry) do
+      post(
+        '/time_entries.xml',
+        :params =>
+          {:time_entry =>
+            {:project_id => '1', :spent_on => '2010-12-02', :user_id => '3',
+             :hours => '3.5', :activity_id => '11'}},
+        :headers => credentials('jsmith'))
+    end
+    assert_response :created
+    assert_equal 3, entry.user_id
+    assert_equal 2, entry.author_id
+  end
+
+  test "POST /time_entries.xml with :issue_id for other user" do
+    Role.find_by_name('Manager').add_permission! :log_time_for_other_users
+
+    entry = new_record(TimeEntry) do
+      post(
+        '/time_entries.xml',
+        :params =>
+          {:time_entry =>
+            {:issue_id => '1', :spent_on => '2010-12-02', :user_id => '3',
+             :hours => '3.5', :activity_id => '11'}},
+        :headers => credentials('jsmith'))
+    end
+    assert_response :created
+    assert_equal 3, entry.user_id
+    assert_equal 2, entry.author_id
+  end
+
   test "PUT /time_entries/:id.xml with valid parameters should update time entry" do
     assert_no_difference 'TimeEntry.count' do
       put(
diff --git a/test/unit/time_entry_import_test.rb b/test/unit/time_entry_import_test.rb
index d84de22f4..440089dc9 100644
--- a/test/unit/time_entry_import_test.rb
+++ b/test/unit/time_entry_import_test.rb
@@ -127,7 +127,8 @@ class TimeEntryImportTest < ActiveSupport::TestCase
   end
 
   def test_maps_user_id_for_user_with_permissions
-    User.current = User.find(1)
+    Role.find_by_name('Manager').add_permission! :log_time_for_other_users
+
     import = generate_import_with_mapping
     first, second, third, fourth = new_records(TimeEntry, 4) { import.run }
 
@@ -138,16 +139,17 @@ class TimeEntryImportTest < ActiveSupport::TestCase
   end
 
   def test_maps_user_to_column_value
-    User.current = User.find(1)
+    Role.find_by_name('Manager').add_permission! :log_time_for_other_users
+
     import = generate_import_with_mapping
-    import.mapping.merge!('user_id' => 'value:1')
+    import.mapping.merge!('user_id' => 'value:3')
     import.save!
     first, second, third, fourth = new_records(TimeEntry, 4) { import.run }
 
-    assert_equal 1, first.user_id
-    assert_equal 1, second.user_id
-    assert_equal 1, third.user_id
-    assert_equal 1, fourth.user_id
+    assert_equal 3, first.user_id
+    assert_equal 3, second.user_id
+    assert_equal 3, third.user_id
+    assert_equal 3, fourth.user_id
   end
 
   def test_maps_user_id_for_user_without_permissions