Adds a permission to log time for another user (#3848).

Patch by Marius BALTEANU.

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

app/controllers/timelog_controller.rb

@@ -26,6 +26,8 @@ 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
 
@@ -90,12 +92,12 @@ class TimelogController < ApplicationController
   end
 
   def new
-    @time_entry ||= TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => User.current.today)
+    @time_entry ||= TimeEntry.new(:project => @project, :issue => @issue, :author => User.current, :spent_on => User.current.today)
     @time_entry.safe_attributes = params[:time_entry]
   end
 
   def create
-    @time_entry ||= TimeEntry.new(:project => @project, :issue => @issue, :user => User.current, :spent_on => User.current.today)
+    @time_entry ||= TimeEntry.new(:project => @project, :issue => @issue, :author => User.current, :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
@@ -145,7 +147,6 @@ class TimelogController < ApplicationController
 
   def update
     @time_entry.safe_attributes = params[:time_entry]
-
     call_hook(:controller_timelog_edit_before_save, { :params => params, :time_entry => @time_entry })
 
     if @time_entry.save
@@ -254,6 +255,13 @@ private
     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).

app/helpers/timelog_helper.rb

@@ -42,6 +42,11 @@ module TimelogHelper
     collection
   end
 
+  def user_collection_for_select_options(time_entry)
+    collection = time_entry.assignable_users
+    principals_options_for_select(collection, time_entry.user_id)
+  end
+
   def select_hours(data, criteria, value)
     if value.to_s.empty?
       data.select {|row| row[criteria].blank? }

app/models/time_entry.rb

@@ -22,6 +22,7 @@ class TimeEntry < ActiveRecord::Base
   belongs_to :project
   belongs_to :issue
   belongs_to :user
+  belongs_to :author, :class_name => 'User'
   belongs_to :activity, :class_name => 'TimeEntryActivity'
 
   acts_as_customizable
@@ -39,13 +40,14 @@ class TimeEntry < ActiveRecord::Base
                             :author_key => :user_id,
                             :scope => joins(:project).preload(:project)
 
-  validates_presence_of :user_id, :activity_id, :project_id, :hours, :spent_on
+  validates_presence_of :author_id, :user_id, :activity_id, :project_id, :hours, :spent_on
   validates_presence_of :issue_id, :if => lambda { Setting.timelog_required_fields.include?('issue_id') }
   validates_presence_of :comments, :if => lambda { Setting.timelog_required_fields.include?('comments') }
   validates_numericality_of :hours, :allow_nil => true, :message => :invalid
   validates_length_of :comments, :maximum => 1024, :allow_nil => true
   validates :spent_on, :date => true
   before_validation :set_project_if_nil
+  before_validation :set_author_if_nil
   validate :validate_time_entry
 
   scope :visible, lambda {|*args|
@@ -60,7 +62,7 @@ class TimeEntry < ActiveRecord::Base
     where("#{Issue.table_name}.root_id = #{issue.root_id} AND #{Issue.table_name}.lft >= #{issue.lft} AND #{Issue.table_name}.rgt <= #{issue.rgt}")
   }
 
-  safe_attributes 'hours', 'comments', 'project_id', 'issue_id', 'activity_id', 'spent_on', 'custom_field_values', 'custom_fields'
+  safe_attributes 'user_id', 'hours', 'comments', 'project_id', 'issue_id', 'activity_id', 'spent_on', 'custom_field_values', 'custom_fields'
 
   # Returns a SQL conditions string used to find all time entries visible by the specified user
   def self.visible_condition(user, options={})
@@ -119,6 +121,10 @@ class TimeEntry < ActiveRecord::Base
     self.project = issue.project if issue && project.nil?
   end
 
+  def set_author_if_nil
+    self.author = User.current if author.nil?
+  end
+
   def validate_time_entry
     if hours
       errors.add :hours, :invalid if hours < 0
@@ -134,6 +140,7 @@ class TimeEntry < ActiveRecord::Base
       end
     end
     errors.add :project_id, :invalid if project.nil?
+    errors.add :user_id, :invalid if (user_id != author_id && !self.assignable_users.map(&:id).include?(user_id))
     errors.add :issue_id, :invalid if (issue_id && !issue) || (issue && project!=issue.project) || @invalid_issue_id
     errors.add :activity_id, :inclusion if activity_id_changed? && project && !project.activities.include?(activity)
     if spent_on_changed? && user
@@ -180,6 +187,16 @@ class TimeEntry < ActiveRecord::Base
     editable_custom_field_values(user).map(&:custom_field).uniq
   end
 
+  def assignable_users
+    users = []
+    if project
+      users = project.members.active.preload(:user)
+      users = users.map(&:user).select{ |u| u.allowed_to?(:log_time, project) }
+    end
+    users << User.current if User.current.logged? && !users.include?(User.current)
+    users
+  end
+
   private
 
   # Returns the hours that were logged in other time entries for the same user and the same day

app/models/time_entry_query.rb

@@ -25,6 +25,7 @@ class TimeEntryQuery < Query
     QueryColumn.new(:spent_on, :sortable => ["#{TimeEntry.table_name}.spent_on", "#{TimeEntry.table_name}.created_on"], :default_order => 'desc', :groupable => true),
     QueryColumn.new(:created_on, :sortable => "#{TimeEntry.table_name}.created_on", :default_order => 'desc'),
     QueryColumn.new(:tweek, :sortable => ["#{TimeEntry.table_name}.spent_on", "#{TimeEntry.table_name}.created_on"], :caption => :label_week),
+    QueryColumn.new(:author, :sortable => lambda {User.fields_for_order_statement}),
     QueryColumn.new(:user, :sortable => lambda {User.fields_for_order_statement}, :groupable => true),
     QueryColumn.new(:activity, :sortable => "#{TimeEntryActivity.table_name}.position", :groupable => true),
     QueryColumn.new(:issue, :sortable => "#{Issue.table_name}.id"),
@@ -75,6 +76,10 @@ class TimeEntryQuery < Query
       :type => :list_optional, :values => lambda { author_values }
     )
 
+    add_available_filter("author_id",
+      :type => :list_optional, :values => lambda { author_values }
+    )
+
     activities = (project ? project.activities : TimeEntryActivity.shared)
     add_available_filter("activity_id",
       :type => :list, :values => activities.map {|a| [a.name, a.id.to_s]}

app/views/timelog/_form.html.erb

@@ -16,6 +16,9 @@
       <%=  link_to_issue(@time_entry.issue) if @time_entry.issue.try(:visible?) %>
     </span>
   </p>
+  <% if User.current.allowed_to?(:log_time_for_other_users, @project) %>
+    <p><%= f.select :user_id, user_collection_for_select_options(@time_entry), :required => true %></p>
+  <% end %>
   <p><%= f.date_field :spent_on, :size => 10, :required => true %><%= calendar_for('time_entry_spent_on') %></p>
   <p><%= f.hours_field :hours, :size => 6, :required => true %></p>
   <p><%= f.text_field :comments, :size => 100, :maxlength => 1024, :required => Setting.timelog_required_fields.include?('comments') %></p>

config/locales/en.yml

@@ -224,6 +224,7 @@ en:
   error_exceeds_maximum_hours_per_day: "Cannot log more than %{max_hours} hours on the same day (%{logged_hours} hours have already been logged)"
   error_can_not_delete_auth_source: "This authentication mode is in use and cannot be deleted."
   error_spent_on_future_date: "Cannot log time on a future date"
+  error_not_allowed_to_log_time_for_other_users: "Your role is not allowed to log time for other users"
 
   mail_subject_lost_password: "Your %{value} password"
   mail_body_lost_password: 'To change your password, click on the following link:'
@@ -537,6 +538,7 @@ en:
   permission_manage_subtasks: Manage subtasks
   permission_manage_related_issues: Manage related issues
   permission_import_issues: Import issues
+  permission_log_foreign_time: Log spent time for other users
 
   project_module_issue_tracking: Issue tracking
   project_module_time_tracking: Time tracking

db/migrate/20180501132547_add_author_id_to_time_entries.rb

@@ -0,0 +1,11 @@
+class AddAuthorIdToTimeEntries < ActiveRecord::Migration[5.1]
+  def up
+    add_column :time_entries, :author_id, :integer, :default => nil, :after => :project_id
+    # Copy existing user_id to author_id
+    TimeEntry.update_all('author_id = user_id')
+  end
+
+  def down
+    remove_column :time_entries, :author_id
+  end
+end

lib/redmine.rb

@@ -126,6 +126,7 @@ Redmine::AccessControl.map do |map|
     map.permission :edit_time_entries, {:timelog => [:edit, :update, :destroy, :bulk_edit, :bulk_update]}, :require => :member
     map.permission :edit_own_time_entries, {:timelog => [:edit, :update, :destroy,:bulk_edit, :bulk_update]}, :require => :loggedin
     map.permission :manage_project_activities, {:projects => :settings, :project_enumerations => [:update, :destroy]}, :require => :member
+    map.permission :log_time_for_other_users, :require => :member
   end
 
   map.project_module :news do |map|

test/fixtures/time_entries.yml

@@ -12,6 +12,7 @@ time_entries_001:
   id: 1
   hours: 4.25
   user_id: 2
+  author_id: 2
   tyear: 2007
 time_entries_002:
   created_on: 2007-03-23 14:11:04 +01:00
@@ -26,6 +27,7 @@ time_entries_002:
   id: 2
   hours: 150.0
   user_id: 1
+  author_id: 1
   tyear: 2007
 time_entries_003:
   created_on: 2007-04-21 12:20:48 +02:00
@@ -40,6 +42,7 @@ time_entries_003:
   id: 3
   hours: 1.0
   user_id: 1
+  author_id: 1
   tyear: 2007
 time_entries_004:
   created_on: 2007-04-22 12:20:48 +02:00
@@ -54,6 +57,7 @@ time_entries_004:
   id: 4
   hours: 7.65
   user_id: 1
+  author_id: 1
   tyear: 2007
 time_entries_005:
   created_on: 2011-03-22 12:20:48 +02:00
@@ -68,5 +72,6 @@ time_entries_005:
   id: 5
   hours: 7.65
   user_id: 1
+  author_id: 1
   tyear: 2011
 

test/functional/project_enumerations_controller_test.rb

@@ -163,10 +163,11 @@ class ProjectEnumerationsControllerTest < Redmine::ControllerTest
     # TODO: Need to cause an exception on create but these tests
     # aren't setup for mocking.  Just create a record now so the
     # second one is a dupicate
+    user = User.find(1)
     parent = TimeEntryActivity.find(9)
     TimeEntryActivity.create!({:name => parent.name, :project_id => 1,
                                :position => parent.position, :active => true, :parent_id => 9})
-    TimeEntry.create!({:project_id => 1, :hours => 1.0, :user => User.find(1),
+    TimeEntry.create!({:project_id => 1, :hours => 1.0, :user => user, :author => user,
                        :issue_id => 3, :activity_id => 10, :spent_on => '2009-01-01'})
     assert_equal 3, TimeEntry.where(:activity_id => 9, :project_id => 1).count
     assert_equal 1, TimeEntry.where(:activity_id => 10, :project_id => 1).count

test/functional/timelog_controller_test.rb

@@ -102,6 +102,40 @@ class TimelogControllerTest < Redmine::ControllerTest
     assert_select 'option', :text => 'Inactive Activity', :count => 0
   end
 
+  def test_new_should_show_user_select_if_user_has_permission
+    Role.find_by_name('Manager').add_permission! :log_time_for_other_users
+    @request.session[:user_id] = 2
+
+    get :new, :params => {:project_id => 1}
+    assert_response :success
+    assert_select 'select[name=?]', 'time_entry[user_id]' do
+      assert_select 'option', 3
+      assert_select 'option[value=?]', '2', 2
+      assert_select 'option[value=?]', '3', 1
+      # locked members should not be available
+      assert_select 'option[value=?]', '4', 0
+    end
+  end
+
+  def test_new_user_select_should_include_current_user_if_is_logged
+    @request.session[:user_id] = 1
+
+    get :new, :params => {:project_id => 1}
+    assert_response :success
+    assert_select 'select[name=?]', 'time_entry[user_id]' do
+      assert_select 'option[value=?]', '1', :text => '<< me >>'
+      assert_select 'option[value=?]', '1', :text => 'Redmine Admin'
+    end
+  end
+
+  def test_new_should_not_show_user_select_if_user_does_not_have_permission
+    @request.session[:user_id] = 2
+
+    get :new, :params => {:project_id => 1}
+    assert_response :success
+    assert_select 'select[name=?]', 'time_entry[user_id]', 0
+  end
+
   def test_post_new_as_js_should_update_activity_options
     @request.session[:user_id] = 3
     post :new, :params => {:time_entry => {:project_id => 1}, :format => 'js'}
@@ -268,6 +302,49 @@ class TimelogControllerTest < Redmine::ControllerTest
     assert !response.body.include?('issue_that_is_not_visible')
   end
 
+  def test_create_for_other_user
+    Role.find_by_name('Manager').add_permission! :log_time_for_other_users
+    @request.session[:user_id] = 2
+
+    post :create, :params => {
+      :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',
+        :user_id => '3'
+      }
+    }
+
+    assert_redirected_to '/projects/ecookbook/time_entries'
+
+    t = TimeEntry.last
+    assert_equal 3, t.user_id
+    assert_equal 2, t.author_id
+  end
+
+  def test_create_for_other_user_should_deny_for_user_without_permission
+    Role.find_by_name('Manager').remove_permission! :log_time_for_other_users
+    @request.session[:user_id] = 2
+
+    post :create, :params => {
+      :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',
+        :user_id => '3'
+      }
+    }
+
+    assert_response 403
+    assert_select 'p[id=?]', 'errorExplanation', :text => 'Your role is not allowed to log time for other users'
+  end
+
   def test_create_and_continue_at_project_level
     @request.session[:user_id] = 2
     assert_difference 'TimeEntry.count' do
@@ -533,6 +610,21 @@ class TimelogControllerTest < Redmine::ControllerTest
     assert_select_error /Issue is invalid/
   end
 
+  def test_update_should_deny_changing_user_for_user_without_permission
+    Role.find_by_name('Manager').remove_permission! :log_time_for_other_users
+    @request.session[:user_id] = 2
+
+    put :update, :params => {
+      :id => 3,
+      :time_entry => {
+        :user_id => '3'
+      }
+    }
+
+    assert_response 403
+    assert_select 'p[id=?]', 'errorExplanation', :text => 'Your role is not allowed to log time for other users'
+  end
+
   def test_get_bulk_edit
     @request.session[:user_id] = 2
 
@@ -934,9 +1026,9 @@ class TimelogControllerTest < Redmine::ControllerTest
   end
 
   def test_index_should_sort_by_spent_on_and_created_on
-    t1 = TimeEntry.create!(:user => User.find(1), :project => Project.find(1), :hours => 1, :spent_on => '2012-06-16', :created_on => '2012-06-16 20:00:00', :activity_id => 10)
+    t1 = TimeEntry.create!(:author => User.find(1), :user => User.find(1), :project => Project.find(1), :hours => 1, :spent_on => '2012-06-16', :created_on => '2012-06-16 20:00:00', :activity_id => 10)
-    t2 = TimeEntry.create!(:user => User.find(1), :project => Project.find(1), :hours => 1, :spent_on => '2012-06-16', :created_on => '2012-06-16 20:05:00', :activity_id => 10)
+    t2 = TimeEntry.create!(:author => User.find(1), :user => User.find(1), :project => Project.find(1), :hours => 1, :spent_on => '2012-06-16', :created_on => '2012-06-16 20:05:00', :activity_id => 10)
-    t3 = TimeEntry.create!(:user => User.find(1), :project => Project.find(1), :hours => 1, :spent_on => '2012-06-15', :created_on => '2012-06-16 20:10:00', :activity_id => 10)
+    t3 = TimeEntry.create!(:author => User.find(1), :user => User.find(1), :project => Project.find(1), :hours => 1, :spent_on => '2012-06-15', :created_on => '2012-06-16 20:10:00', :activity_id => 10)
 
     get :index, :params => {
       :project_id => 1,
@@ -1100,6 +1192,27 @@ class TimelogControllerTest < Redmine::ControllerTest
     assert_select 'td.issue-category', :text => 'Printing'
   end
 
+  def test_index_with_author_filter
+    get :index, :params => {
+      :project_id => 'ecookbook',
+      :f => ['author_id'],
+      :op => {'author_id' => '='},
+      :v => {'author_id' => ['2']}
+    }
+    assert_response :success
+    assert_equal ['1'], css_select('input[name="ids[]"]').map {|e| e.attr('value')}
+  end
+
+  def test_index_with_author_column
+    get :index, :params => {
+      :project_id => 'ecookbook',
+      :c => %w(project spent_on issue comments hours author)
+    }
+
+    assert_response :success
+    assert_select 'td.author', :text => 'Redmine Admin'
+  end
+
   def test_index_with_issue_category_sort
     issue = Issue.find(3)
     issue.category_id = 2

test/object_helpers.rb

@@ -144,6 +144,7 @@ module ObjectHelpers
   def TimeEntry.generate(attributes={})
     entry = TimeEntry.new(attributes)
     entry.user ||= User.find(2)
+    entry.author ||= entry.user
     entry.issue ||= Issue.find(1) unless entry.project
     entry.project ||= entry.issue.project
     entry.activity ||= TimeEntryActivity.first

test/unit/lib/redmine/export/pdf/issues_pdf_test.rb

@@ -27,8 +27,10 @@ class IssuesPdfHelperTest < ActiveSupport::TestCase
     query = IssueQuery.new(:project => Project.find(1), :name => '_')
     query.column_names = [:subject, :spent_hours]
     issue = Issue.find(2)
-    TimeEntry.create(:spent_on => Date.today, :hours => 4.3432, :user => User.find(1),
+    user = User.find(1)
+    time_entry = TimeEntry.create!(:spent_on => Date.today, :hours => 4.3432, :user => user, :author => user,
                      :project_id => 1, :issue => issue, :activity => TimeEntryActivity.first)
+
     results = fetch_row_values(issue, query, 0)
     assert_equal ["2", "Add ingredients categories", "4.34"], results
   end

test/unit/time_entry_test.rb

@@ -185,6 +185,7 @@ class TimeEntryTest < ActiveSupport::TestCase
                           :issue    => issue,
                           :project  => project,
                           :user     => anon,
+                          :author     => anon,
                           :activity => activity)
     assert_equal 1, te.errors.count
   end
@@ -223,10 +224,27 @@ class TimeEntryTest < ActiveSupport::TestCase
   def test_create_with_required_issue_id_and_comment_should_be_validated
     set_language_if_valid 'en'
     with_settings :timelog_required_fields => ['issue_id' , 'comments'] do
-      entry = TimeEntry.new(:project => Project.find(1), :spent_on => Date.today, :user => User.find(1), :activity => TimeEntryActivity.first, :hours => 1)
+      entry = TimeEntry.new(:project => Project.find(1), :spent_on => Date.today, :author => User.find(1), :user => User.find(1), :activity => TimeEntryActivity.first, :hours => 1)
 
       assert !entry.save
       assert_equal ["Comment cannot be blank", "Issue cannot be blank"], entry.errors.full_messages.sort
     end
   end
+
+  def test_create_should_validate_user_id
+    entry = TimeEntry.new(:spent_on => '2010-01-01',
+                          :hours    => 10,
+                          :project_id => 1,
+                          :user_id    => 4)
+
+    assert !entry.save
+    assert_equal ["User is invalid"], entry.errors.full_messages.sort
+  end
+
+  def test_assignable_users_should_include_active_project_members_with_log_time_permission
+    Role.find(2).remove_permission! :log_time
+    time_entry = TimeEntry.find(1)
+
+    assert_equal [2], time_entry.assignable_users.map(&:id)
+  end
 end