From e1a9b38cc0b1231c1069d6b901e51e6b3e202b4f Mon Sep 17 00:00:00 2001 From: Marius BALTEANU Date: Fri, 1 Nov 2019 21:42:53 +0200 Subject: [PATCH 1/2] Allow import time entries for other users --- app/helpers/imports_helper.rb | 2 +- app/models/time_entry_import.rb | 26 +++++++++++- .../_time_entries_fields_mapping.html.erb | 8 +++- .../_time_entries_saved_objects.html.erb | 2 + test/fixtures/files/import_time_entries.csv | 10 ++--- test/functional/imports_controller_test.rb | 35 ++++++++++++++++ test/object_helpers.rb | 8 ++++ test/unit/time_entry_import_test.rb | 41 ++++++++++++++++++- 8 files changed, 123 insertions(+), 9 deletions(-) diff --git a/app/helpers/imports_helper.rb b/app/helpers/imports_helper.rb index ea5a6e7ee..315973fcb 100644 --- a/app/helpers/imports_helper.rb +++ b/app/helpers/imports_helper.rb @@ -33,7 +33,7 @@ module ImportsHelper tags << options_for_select(import.columns_options, import.mapping[field]) if values = options[:values] tags << content_tag('option', '--', :disabled => true) - tags << options_for_select(values.map {|text, value| [text, "value:#{value}"]}, import.mapping[field]) + tags << options_for_select(values.map {|text, value| [text, "value:#{value}"]}, import.mapping[field] || options[:default_value]) end tags end diff --git a/app/models/time_entry_import.rb b/app/models/time_entry_import.rb index 66b762f74..49c6de375 100644 --- a/app/models/time_entry_import.rb +++ b/app/models/time_entry_import.rb @@ -43,6 +43,16 @@ class TimeEntryImport < Import project.activities end + def allowed_target_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 + def project project_id = mapping['project_id'].to_i allowed_target_projects.find_by_id(project_id) || allowed_target_projects.first @@ -55,11 +65,17 @@ class TimeEntryImport < Import end end + def user_value + if mapping['user_id'].to_s =~ /\Avalue:(\d+)\z/ + $1.to_i + end + end + private def build_object(row, item) object = TimeEntry.new - object.user = user + object.author = user activity_id = nil if activity @@ -68,9 +84,17 @@ class TimeEntryImport < Import activity_id = allowed_target_activities.named(activity_name).first.try(:id) end + user_id = nil + if User.current.allowed_to?(:log_time_for_other_users, project) + user_id = user_value || row_value(row, 'user_id') + else + user_id = user.id + end + attributes = { :project_id => project.id, :activity_id => activity_id, + :user_id => user_id, :issue_id => row_value(row, 'issue_id'), :spent_on => row_date(row, 'spent_on'), diff --git a/app/views/imports/_time_entries_fields_mapping.html.erb b/app/views/imports/_time_entries_fields_mapping.html.erb index f480fae68..1b4f7a0e4 100644 --- a/app/views/imports/_time_entries_fields_mapping.html.erb +++ b/app/views/imports/_time_entries_fields_mapping.html.erb @@ -10,9 +10,15 @@ :values => @import.allowed_target_activities.sorted.map {|t| [t.name, t.id]} %>

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

+ + <%= mapping_select_tag @import, 'user_id', :required => true, + :values => @import.allowed_target_users.map {|u| [u.name, u.id]}, :default_value => "value:#{User.current.id}" %> +

+<% end %>

<%= mapping_select_tag @import, 'issue_id' %> diff --git a/app/views/imports/_time_entries_saved_objects.html.erb b/app/views/imports/_time_entries_saved_objects.html.erb index 0579536d7..002401546 100644 --- a/app/views/imports/_time_entries_saved_objects.html.erb +++ b/app/views/imports/_time_entries_saved_objects.html.erb @@ -2,6 +2,7 @@ <%= t(:field_project) %> + <%= t(:field_user) %> <%= t(:field_activity) %> <%= t(:field_issue) %> <%= t(:field_spent_on) %> @@ -13,6 +14,7 @@ <% saved_objects.each do |time_entry| %> <%= link_to_project(time_entry.project, :jump => 'time_entries') if time_entry.project %> + <%= link_to_user time_entry.user %> <%= time_entry.activity.name if time_entry.activity %> <%= link_to_issue time_entry.issue if time_entry.issue %> <%= format_date(time_entry.spent_on) %> diff --git a/test/fixtures/files/import_time_entries.csv b/test/fixtures/files/import_time_entries.csv index 1ff8075ef..0628e5203 100644 --- a/test/fixtures/files/import_time_entries.csv +++ b/test/fixtures/files/import_time_entries.csv @@ -1,5 +1,5 @@ -row;issue_id;date;hours;comment;activity;overtime -1;;2020-01-01;1;Some Design;Design;yes -2;;2020-01-02;2;Some Development;Development;yes -3;1;2020-01-03;3;Some QA;QA;no -4;2;2020-01-04;4;Some Inactivity;Inactive Activity;no +row;issue_id;date;hours;comment;activity;overtime;user_id +1;;2020-01-01;1;Some Design;Design;yes;2 +2;;2020-01-02;2;Some Development;Development;yes;2 +3;1;2020-01-03;3;Some QA;QA;no;3 +4;2;2020-01-04;4;Some Inactivity;Inactive Activity;no;2 diff --git a/test/functional/imports_controller_test.rb b/test/functional/imports_controller_test.rb index ed9024306..77bc6723f 100644 --- a/test/functional/imports_controller_test.rb +++ b/test/functional/imports_controller_test.rb @@ -187,6 +187,41 @@ class ImportsControllerTest < Redmine::ControllerTest assert_equal '0', mapping['subject'] end + def test_get_mapping_time_entry + Role.find(1).add_permission! :log_time_for_other_users + import = generate_time_entry_import + import.settings = {'separator' => ";", 'wrapper' => '"', 'encoding' => "ISO-8859-1"} + import.save! + + get :mapping, :params => { + :id => import.to_param + } + + assert_response :success + + # 'user_id' field should be available because User#2 has both + # 'import_time_entries' and 'log_time_for_other_users' permissions + assert_select 'select[name=?]', 'import_settings[mapping][user_id]' do + # Current user should be the default value + assert_select 'option[value="value:2"][selected]', :text => User.find(2).name + assert_select 'option[value="value:3"]', :text => User.find(3).name + end + end + + def test_get_mapping_time_entry_for_user_without_log_time_for_other_users_permission + import = generate_time_entry_import + import.settings = {'separator' => ";", 'wrapper' => '"', 'encoding' => "ISO-8859-1"} + import.save! + + get :mapping, :params => { + :id => import.to_param + } + + assert_response :success + + assert_select 'select[name=?]', 'import_settings[mapping][user_id]', 0 + end + def test_get_run import = generate_import_with_mapping diff --git a/test/object_helpers.rb b/test/object_helpers.rb index 4f1f4b486..e89e1bd05 100644 --- a/test/object_helpers.rb +++ b/test/object_helpers.rb @@ -259,6 +259,14 @@ module ObjectHelpers import.save! import end + + def generate_time_entry_import(fixture_name='import_time_entries.csv') + import = TimeEntryImport.new + import.user_id = 2 + import.file = uploaded_test_file(fixture_name, 'text/csv') + import.save! + import + end end module TrackerObjectHelpers diff --git a/test/unit/time_entry_import_test.rb b/test/unit/time_entry_import_test.rb index cc732f0d7..d84de22f4 100644 --- a/test/unit/time_entry_import_test.rb +++ b/test/unit/time_entry_import_test.rb @@ -36,6 +36,7 @@ class TimeEntryImportTest < ActiveSupport::TestCase def setup set_language_if_valid 'en' + User.current = nil end def test_authorized @@ -125,6 +126,43 @@ class TimeEntryImportTest < ActiveSupport::TestCase assert_equal '0', fourth.custom_field_value(overtime_cf) end + def test_maps_user_id_for_user_with_permissions + User.current = User.find(1) + import = generate_import_with_mapping + first, second, third, fourth = new_records(TimeEntry, 4) { import.run } + + assert_equal 2, first.user_id + assert_equal 2, second.user_id + assert_equal 3, third.user_id + assert_equal 2, fourth.user_id + end + + def test_maps_user_to_column_value + User.current = User.find(1) + import = generate_import_with_mapping + import.mapping.merge!('user_id' => 'value:1') + 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 + end + + def test_maps_user_id_for_user_without_permissions + # User 2 doesn't have log_time_for_other_users permission + User.current = User.find(2) + import = generate_import_with_mapping + first, second, third, fourth = new_records(TimeEntry, 4) { import.run } + + assert_equal 2, first.user_id + assert_equal 2, second.user_id + # user_id value from CSV should be ignored + assert_equal 2, third.user_id + assert_equal 2, fourth.user_id + end + protected def generate_import(fixture_name='import_time_entries.csv') @@ -146,7 +184,8 @@ class TimeEntryImportTest < ActiveSupport::TestCase 'issue_id' => '1', 'spent_on' => '2', 'hours' => '3', - 'comments' => '4' + 'comments' => '4', + 'user_id' => '7' } } import.save! -- 2.22.0