Patch #32196: Allow import time entries for other users - Redmine

Custom queries

Documentation issues
Open defects
Open features
Patch queue
Plugin issues
Translation patches

Actions

Copy link

Patch #32196

closed

Allow import time entries for other users

Added by Marius BĂLTEANU over 4 years ago. Updated over 4 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Jean-Philippe Lang

Category:

Time tracking

Target version:

Start date:

Due date:

% Done:

Estimated time:

Description

#28234 allows importing time entries only for the current user. I think we should support importing for other users as well and I see two options:

1. We allow based on the new permission "Log spent time for other users" added in #3848.
2. We add a new permission "Import time entries" as we already have for issues.

On the long term, I think option 2 it's better because:
- it's consistent with the behaviour from issues
- we can allow importing time entries on multiple projects once.

Any feedback is welcome.

Files

Download all files

select_user_id.png (296 KB) select_user_id.png		Marius BĂLTEANU, 2019-10-27 22:35
0001-Add-permission-to-import-time-entries.patch (4 KB) 0001-Add-permission-to-import-time-entries.patch		Marius BĂLTEANU, 2019-10-28 09:06
0001-Allow-import-time-entries-for-other-users.patch (9.72 KB) 0001-Allow-import-time-entries-for-other-users.patch		Marius BĂLTEANU, 2019-11-01 23:03
import_new.png (55.1 KB) import_new.png		Marius BĂLTEANU, 2019-11-02 14:48

Related issues

Related to Redmine - Feature #28234: Add CSV Import for Time Entries

Closed

Go MAEDA

Actions

Related to Redmine - Feature #3848: Permission to log time for another user

Closed

Jean-Philippe Lang

2009-09-11

Actions

Issue # Delay: days Cancel

History
Notes
Property changes
Associated revisions

Actions

Copy link

Updated by Marius BĂLTEANU over 4 years ago

Related to Feature #28234: Add CSV Import for Time Entries added

Actions

Copy link

Updated by Kevin Fischer over 4 years ago

+1
Sounds good.

I also agree that option 2 would be better.

Actions

Copy link

Updated by Marius BĂLTEANU over 4 years ago

Assignee set to Marius BĂLTEANU
Target version set to Candidate for next major release

Actions

Copy link

Updated by Bernhard Rohloff over 4 years ago

Category set to Time tracking

+1
I think option 2 is the better one, too.

Actions

Copy link

Updated by Marius BĂLTEANU over 4 years ago

Assignee deleted (~~Marius BĂLTEANU~~)
Target version changed from Candidate for next major release to 4.1.0

Actions

Copy link

Updated by Marius BĂLTEANU over 4 years ago

File 0002-Allow-import-time-entries-for-other-users-if-current.patch added
File 0001-Add-permission-to-import-time-entries.patch added

I'm attaching two patches:

1. 0001-Add-permission-to-import-time-entries.patch which adds the missing "Import time entries" permission. I think it is better to have this permission in order to be consistent with import issues from CSV. Also, some Redmine administrators don't want to allow importing time entries from CSV at all.
2. 0002-Allow-import-time-entries-for-other-users-if-current.patch which allows users with "log_time_for_other_users" permission to import time entries for other users as well.

Actions

Copy link

Updated by Marius BĂLTEANU over 4 years ago

File deleted (~~0002-Allow-import-time-entries-for-other-users-if-current.patch~~)

Actions

Copy link

Updated by Marius BĂLTEANU over 4 years ago

File 0002-Allow-import-time-entries-for-other-users-if-current.patch added

Actions

Copy link

Updated by Marius BĂLTEANU over 4 years ago

File deleted (~~0002-Allow-import-time-entries-for-other-users-if-current.patch~~)

Actions

Copy link

#10

Updated by Marius BĂLTEANU over 4 years ago

File 0002-Allow-import-time-entries-for-other-users-if-current.patch added

Actions

Copy link

#11

Updated by Marius BĂLTEANU over 4 years ago

File deleted (~~0001-Add-permission-to-import-time-entries.patch~~)

Actions

Copy link

#12

Updated by Marius BĂLTEANU over 4 years ago

File 0001-Add-permission-to-import-time-entries.patch added

Actions

Copy link

#13

Updated by Marius BĂLTEANU over 4 years ago

File 0002-Allow-import-time-entries-for-other-users-if-current.patch added
File select_user_id.png select_user_id.png added

Made some minor changes to attachment:0002-Allow-import-time-entries-for-other-users-if-current.patch in order to allow selecting the user from column mapping and not only from CSV.

Actions

Copy link

#14

Updated by Marius BĂLTEANU over 4 years ago

File deleted (~~0002-Allow-import-time-entries-for-other-users-if-current.patch~~)

Actions

Copy link

#15

Updated by Go MAEDA over 4 years ago

Marius BALTEANU wrote:

1. 0001-Add-permission-to-import-time-entries.patch which adds the missing "Import time entries" permission. I think it is better to have this permission in order to be consistent with import issues from CSV. Also, some Redmine administrators don't want to allow importing time entries from CSV at all.

I agree. CSV import feature is a dangerous feature. A malicious or ignorant user can easily ruin the spent time page by bulk-importing junk data. so, the permission is useful to prevent such incidents.

Actions

Copy link

#16

Updated by Marius BĂLTEANU over 4 years ago

File deleted (~~0001-Add-permission-to-import-time-entries.patch~~)

Actions

Copy link

#17

Updated by Marius BĂLTEANU over 4 years ago

File 0001-Add-permission-to-import-time-entries.patch 0001-Add-permission-to-import-time-entries.patch added

Go MAEDA wrote:

Marius BALTEANU wrote:

1. 0001-Add-permission-to-import-time-entries.patch which adds the missing "Import time entries" permission. I think it is better to have this permission in order to be consistent with import issues from CSV. Also, some Redmine administrators don't want to allow importing time entries from CSV at all.

I agree. CSV import feature is a dangerous feature. A malicious or ignorant user can easily ruin the spent time page by bulk-importing junk data. so, the permission is useful to prevent such incidents.

Great, thanks. I'm attaching a new version of the patch which checks for the proper permission in /app/views/timelog/index.html.erb.

I think now both patches are ready for commit as part of #28234.

Actions

Copy link

#18

Updated by Go MAEDA over 4 years ago

Related to Feature #3848: Permission to log time for another user added

Actions

Copy link

#19

Updated by Go MAEDA over 4 years ago

Regarding the patch 0002-Allow-import-time-entries-for-other-users-if-current.patch, I think the field mapping for the User field should default to the current user for the consistency with the behavior when the current user does not have "Import time entries" permission.

When a user does not have "Import time entries" permission, the user cannot (and does not necessary to) to set field mapping for the User field and data are always imported as their own time entries.

But when a user has the permission, the same operation results in an error saying that "User cannot be blank. User is invalid".

This can be resolved if the User field defaults to the current user.

Actions

Copy link

#20

Updated by Marius BĂLTEANU over 4 years ago

Go MAEDA wrote:

Regarding the patch 0002-Allow-import-time-entries-for-other-users-if-current.patch, I think the field mapping for the User field should default to the current user for the consistency with the behavior when the current user does not have "Import time entries" permission.

When a user does not have "Import time entries" permission, the user cannot (and does not necessary to) to set field mapping for the User field and data are always imported as their own time entries.

But when a user has the permission, the same operation results in an error saying that "User cannot be blank. User is invalid".

This can be resolved if the User field defaults to the current user.

Agree with you, I'll post the updated patch these days.

Actions

Copy link

#21

Updated by Dominik Ras over 4 years ago

+1 for option 2

+100 for the whole concept of adding time for other users (with sufficient permissions!!). That will save several man-days a month. You guys ROCK !!

Actions

Copy link

#22

Updated by Marius BĂLTEANU over 4 years ago

File deleted (~~0002-Allow-import-time-entries-for-other-users-if-current.patch~~)

Actions

Copy link

#23

Updated by Marius BĂLTEANU over 4 years ago

File 0001-Allow-import-time-entries-for-other-users.patch 0001-Allow-import-time-entries-for-other-users.patch added

Updated the second patch.

Actions

Copy link

#24

Updated by Jean-Philippe Lang over 4 years ago

Marius BALTEANU wrote:

Updated the second patch.

Thanks Marius. With this implementation, we can reference users with their numeric ids only. That can be a pain to fill/review in the CSV file. Is it OK or should we allow to use usernames or emails?

Actions

Copy link

#25

Updated by Marius BĂLTEANU over 4 years ago

Jean-Philippe Lang wrote:

Thanks Marius. With this implementation, we can reference users with their numeric ids only. That can be a pain to fill/review in the CSV file. Is it OK or should we allow to use usernames or emails?

I think it's good enough to allow user ids in a first phase and then add support for usernames or emails in 4.1.1.

Actions

Copy link

#26