Patch #32196
closed
Allow import time entries for other users
Added by Marius BĂLTEANU about 5 years ago. Updated about 5 years ago.
0%
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
| 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
Updated by Marius BĂLTEANU about 5 years ago
- Related to Feature #28234: Add CSV Import for Time Entries added
Updated by Kevin Fischer about 5 years ago
+1
Sounds good.
I also agree that option 2 would be better.
Updated by Marius BĂLTEANU about 5 years ago
- Assignee set to Marius BĂLTEANU
- Target version set to Candidate for next major release
Updated by Bernhard Rohloff about 5 years ago
- Category set to Time tracking
+1
I think option 2 is the better one, too.
Updated by Marius BĂLTEANU about 5 years ago
- Assignee deleted (
Marius BĂLTEANU) - Target version changed from Candidate for next major release to 4.1.0
Updated by Marius BĂLTEANU about 5 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.
Updated by Marius BĂLTEANU about 5 years ago
- File deleted (
0002-Allow-import-time-entries-for-other-users-if-current.patch)
Updated by Marius BĂLTEANU about 5 years ago
- File 0002-Allow-import-time-entries-for-other-users-if-current.patch added
Updated by Marius BĂLTEANU about 5 years ago
- File deleted (
0002-Allow-import-time-entries-for-other-users-if-current.patch)
Updated by Marius BĂLTEANU about 5 years ago
- File 0002-Allow-import-time-entries-for-other-users-if-current.patch added
Updated by Marius BĂLTEANU about 5 years ago
- File deleted (
0001-Add-permission-to-import-time-entries.patch)
Updated by Marius BĂLTEANU about 5 years ago
- File 0001-Add-permission-to-import-time-entries.patch added
Updated by Marius BĂLTEANU about 5 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.
Updated by Marius BĂLTEANU about 5 years ago
- File deleted (
0002-Allow-import-time-entries-for-other-users-if-current.patch)
Updated by Go MAEDA about 5 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.
Updated by Marius BĂLTEANU about 5 years ago
- File deleted (
0001-Add-permission-to-import-time-entries.patch)
Updated by Marius BĂLTEANU about 5 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.
Updated by Go MAEDA about 5 years ago
- Related to Feature #3848: Permission to log time for another user added
Updated by Go MAEDA about 5 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.
Updated by Marius BĂLTEANU about 5 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.
Updated by Dominik Ras about 5 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 !!
Updated by Marius BĂLTEANU about 5 years ago
- File deleted (
0002-Allow-import-time-entries-for-other-users-if-current.patch)
Updated by Marius BĂLTEANU about 5 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.
Updated by Jean-Philippe Lang about 5 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?
Updated by Marius BĂLTEANU about 5 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.
Updated by Marius BĂLTEANU about 5 years ago
Anyway, I'll try to update my patch in the following 2 or 3 hours.
Updated by Jean-Philippe Lang about 5 years ago
Marius BALTEANU wrote:
Anyway, I'll try to update my patch in the following 2 or 3 hours.
No I think it's OK for now.
Updated by Marius BĂLTEANU about 5 years ago
Jean-Philippe Lang wrote:
Marius BALTEANU wrote:
Anyway, I'll try to update my patch in the following 2 or 3 hours.
No I think it's OK for now.
Ok, thanks for you quick response.
Updated by Jean-Philippe Lang about 5 years ago
- Status changed from New to Closed
- Assignee set to Jean-Philippe Lang
Committed, thanks.
Updated by Jean-Philippe Lang about 5 years ago
Marius BALTEANU wrote:
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.
I made the change in order to reference users just like we reference assignees when importing issues.
Updated by Marius BĂLTEANU about 5 years ago
- File import_new.png import_new.png added
- Status changed from Closed to Reopened
r18893 broke the translation in time_entries/imports/new:
I think we need to add both keys in the locales (as we have for issues). Sorry for not catching this.
Updated by Marius BĂLTEANU about 5 years ago
Updated by Jean-Philippe Lang about 5 years ago
- Status changed from Reopened to Closed
- Target version deleted (
4.1.0)
Marius BALTEANU wrote:
I think we need to add both keys in the locales (as we have for issues). Sorry for not catching this.
Fixed, thanks for pointing this out.