From 03c427a3acf475657468caabd9fbf75541a2fe62 Mon Sep 17 00:00:00 2001 From: Marius BALTEANU Date: Tue, 1 May 2018 13:29:49 +0000 Subject: [PATCH] Allow the current user to log time for other users --- app/controllers/timelog_controller.rb | 14 ++- app/helpers/timelog_helper.rb | 5 + app/models/time_entry.rb | 21 +++- app/models/time_entry_query.rb | 5 + app/views/timelog/_form.html.erb | 3 + config/locales/en.yml | 2 + ...20180501132547_add_author_id_to_time_entries.rb | 11 ++ lib/redmine.rb | 1 + test/fixtures/time_entries.yml | 23 ++-- .../project_enumerations_controller_test.rb | 7 +- test/functional/timelog_controller_test.rb | 119 ++++++++++++++++++++- test/object_helpers.rb | 1 + .../unit/lib/redmine/export/pdf/issues_pdf_test.rb | 4 +- test/unit/time_entry_test.rb | 20 +++- 14 files changed, 214 insertions(+), 22 deletions(-) create mode 100644 db/migrate/20180501132547_add_author_id_to_time_entries.rb diff --git a/app/controllers/timelog_controller.rb b/app/controllers/timelog_controller.rb index 6bd7ab3..5d0f27c 100644 --- a/app/controllers/timelog_controller.rb +++ b/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 @@ -144,7 +146,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 @@ -243,6 +244,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). diff --git a/app/helpers/timelog_helper.rb b/app/helpers/timelog_helper.rb index 89ed996..40dd9a9 100644 --- a/app/helpers/timelog_helper.rb +++ b/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? } diff --git a/app/models/time_entry.rb b/app/models/time_entry.rb index d64c311..90c83f5 100644 --- a/app/models/time_entry.rb +++ b/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) end @@ -177,6 +184,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 diff --git a/app/models/time_entry_query.rb b/app/models/time_entry_query.rb index 1a938c5..a93c766 100644 --- a/app/models/time_entry_query.rb +++ b/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]} diff --git a/app/views/timelog/_form.html.erb b/app/views/timelog/_form.html.erb index aa62ffc..691da9e 100644 --- a/app/views/timelog/_form.html.erb +++ b/app/views/timelog/_form.html.erb @@ -16,6 +16,9 @@ <%= link_to_issue(@time_entry.issue) if @time_entry.issue.try(:visible?) %>

+ <% if User.current.allowed_to?(:log_time_for_other_users, @project) %> +

<%= f.select :user_id, user_collection_for_select_options(@time_entry), :required => true %>

+ <% end %>

<%= f.date_field :spent_on, :size => 10, :required => true %><%= calendar_for('time_entry_spent_on') %>

<%= f.hours_field :hours, :size => 6, :required => true %>

<%= f.text_field :comments, :size => 100, :maxlength => 1024, :required => Setting.timelog_required_fields.include?('comments') %>

diff --git a/config/locales/en.yml b/config/locales/en.yml index d8f0943..0f31e61 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -222,6 +222,7 @@ en: warning_fields_cleared_on_bulk_edit: "Changes will result in the automatic deletion of values from one or more fields on the selected objects" 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_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:' @@ -534,6 +535,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 diff --git a/db/migrate/20180501132547_add_author_id_to_time_entries.rb b/db/migrate/20180501132547_add_author_id_to_time_entries.rb new file mode 100644 index 0000000..3d79bf2 --- /dev/null +++ b/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 diff --git a/lib/redmine.rb b/lib/redmine.rb index 6e7d109..1116534 100644 --- a/lib/redmine.rb +++ b/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| diff --git a/test/fixtures/time_entries.yml b/test/fixtures/time_entries.yml index 1b3c9ca..4165c2a 100644 --- a/test/fixtures/time_entries.yml +++ b/test/fixtures/time_entries.yml @@ -1,5 +1,5 @@ ---- -time_entries_001: +--- +time_entries_001: created_on: 2007-03-23 12:54:18 +01:00 tweek: 12 tmonth: 3 @@ -12,8 +12,9 @@ time_entries_001: id: 1 hours: 4.25 user_id: 2 + author_id: 2 tyear: 2007 -time_entries_002: +time_entries_002: created_on: 2007-03-23 14:11:04 +01:00 tweek: 11 tmonth: 3 @@ -26,8 +27,9 @@ time_entries_002: id: 2 hours: 150.0 user_id: 1 + author_id: 1 tyear: 2007 -time_entries_003: +time_entries_003: created_on: 2007-04-21 12:20:48 +02:00 tweek: 16 tmonth: 4 @@ -40,8 +42,9 @@ time_entries_003: id: 3 hours: 1.0 user_id: 1 + author_id: 1 tyear: 2007 -time_entries_004: +time_entries_004: created_on: 2007-04-22 12:20:48 +02:00 tweek: 16 tmonth: 4 @@ -50,12 +53,13 @@ time_entries_004: updated_on: 2007-04-22 12:20:48 +02:00 activity_id: 10 spent_on: 2007-04-22 - issue_id: + issue_id: id: 4 hours: 7.65 user_id: 1 + author_id: 1 tyear: 2007 -time_entries_005: +time_entries_005: created_on: 2011-03-22 12:20:48 +02:00 tweek: 12 tmonth: 3 @@ -64,9 +68,10 @@ time_entries_005: updated_on: 2011-03-22 12:20:48 +02:00 activity_id: 10 spent_on: 2011-03-22 - issue_id: + issue_id: id: 5 hours: 7.65 user_id: 1 + author_id: 1 tyear: 2011 - + diff --git a/test/functional/project_enumerations_controller_test.rb b/test/functional/project_enumerations_controller_test.rb index 2d87b13..3fa579a 100644 --- a/test/functional/project_enumerations_controller_test.rb +++ b/test/functional/project_enumerations_controller_test.rb @@ -143,8 +143,8 @@ class ProjectEnumerationsControllerTest < Redmine::ControllerTest :enumerations => { "9"=> { "parent_id"=>"9", "custom_field_values"=> { - "7" => "1"}, "active"=>"0"} # Design, De-activate - + "7" => "1"}, "active"=>"0"} # Design, De-activate + } } assert_response :redirect @@ -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 diff --git a/test/functional/timelog_controller_test.rb b/test/functional/timelog_controller_test.rb index 652ee12..5169343 100644 --- a/test/functional/timelog_controller_test.rb +++ b/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 @@ -899,9 +991,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) - 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) - 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) + 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!(: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!(: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, @@ -1046,6 +1138,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 diff --git a/test/object_helpers.rb b/test/object_helpers.rb index b7d2eac..967acdf 100644 --- a/test/object_helpers.rb +++ b/test/object_helpers.rb @@ -142,6 +142,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 diff --git a/test/unit/lib/redmine/export/pdf/issues_pdf_test.rb b/test/unit/lib/redmine/export/pdf/issues_pdf_test.rb index c7b3ae9..14f34db 100644 --- a/test/unit/lib/redmine/export/pdf/issues_pdf_test.rb +++ b/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 diff --git a/test/unit/time_entry_test.rb b/test/unit/time_entry_test.rb index 9b3fbf7..99e489e 100644 --- a/test/unit/time_entry_test.rb +++ b/test/unit/time_entry_test.rb @@ -168,6 +168,7 @@ class TimeEntryTest < ActiveSupport::TestCase :issue => issue, :project => project, :user => anon, + :author => anon, :activity => activity) assert_equal 1, te.errors.count end @@ -206,10 +207,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 -- 2.1.4