Patch #32196

Allow import time entries for other users

Added by Marius BALTEANU 8 months ago. Updated 7 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Jean-Philippe Lang% Done:

0%

Category:Time tracking
Target version:-

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.

select_user_id.png (296 KB) Marius BALTEANU, 2019-10-27 22:35

0001-Add-permission-to-import-time-entries.patch Magnifier (4 KB) Marius BALTEANU, 2019-10-28 09:06

0001-Allow-import-time-entries-for-other-users.patch Magnifier (9.72 KB) Marius BALTEANU, 2019-11-01 23:03

import_new.png (55.1 KB) Marius BALTEANU, 2019-11-02 14:48


Related issues

Related to Redmine - Feature #28234: Add CSV Import for Time Entries Closed
Related to Redmine - Feature #3848: Permission to log time for another user Closed 2009-09-11

Associated revisions

Revision 18878
Added by Go MAEDA 7 months ago

Add permission to import time entries (#32196, #28234).

Patch by Marius BALTEANU.

Revision 18890
Added by Jean-Philippe Lang 7 months ago

Allow import time entries for other users (#32196).

Patch by Marius BALTEANU.

Revision 18891
Added by Jean-Philippe Lang 7 months ago

Identify time entry user just like issue assignee (#32196).

Revision 18893
Added by Jean-Philippe Lang 7 months ago

Fixed i18n string for permission (#32196).

Revision 18894
Added by Jean-Philippe Lang 7 months ago

Restore label_import_time_entries i18n string (#32196).

History

#1 Updated by Marius BALTEANU 8 months ago

#2 Updated by Kevin Fischer 8 months ago

+1
Sounds good.

I also agree that option 2 would be better.

#3 Updated by Marius BALTEANU 7 months ago

  • Assignee set to Marius BALTEANU
  • Target version set to Candidate for next major release

#4 Updated by Bernhard Rohloff 7 months ago

  • Category set to Time tracking

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

#5 Updated by Marius BALTEANU 7 months ago

  • Assignee deleted (Marius BALTEANU)
  • Target version changed from Candidate for next major release to 4.1.0

#6 Updated by Marius BALTEANU 7 months 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.

#7 Updated by Marius BALTEANU 7 months ago

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

#8 Updated by Marius BALTEANU 7 months ago

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

#9 Updated by Marius BALTEANU 7 months ago

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

#10 Updated by Marius BALTEANU 7 months ago

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

#11 Updated by Marius BALTEANU 7 months ago

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

#12 Updated by Marius BALTEANU 7 months ago

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

#13 Updated by Marius BALTEANU 7 months ago

  • File 0002-Allow-import-time-entries-for-other-users-if-current.patch added
  • File 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.

#14 Updated by Marius BALTEANU 7 months ago

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

#15 Updated by Go MAEDA 7 months 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.

#16 Updated by Marius BALTEANU 7 months ago

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

#17 Updated by Marius BALTEANU 7 months ago

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.

#18 Updated by Go MAEDA 7 months ago

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

#19 Updated by Go MAEDA 7 months 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.

#20 Updated by Marius BALTEANU 7 months 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.

#21 Updated by Dominik Ras 7 months 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 !!

#22 Updated by Marius BALTEANU 7 months ago

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

#23 Updated by Marius BALTEANU 7 months ago

Updated the second patch.

#24 Updated by Jean-Philippe Lang 7 months 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?

#25 Updated by Marius BALTEANU 7 months 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.

#26 Updated by Marius BALTEANU 7 months ago

Anyway, I'll try to update my patch in the following 2 or 3 hours.

#27 Updated by Jean-Philippe Lang 7 months 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.

#28 Updated by Marius BALTEANU 7 months 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.

#29 Updated by Jean-Philippe Lang 7 months ago

  • Status changed from New to Closed
  • Assignee set to Jean-Philippe Lang

Committed, thanks.

#30 Updated by Jean-Philippe Lang 7 months 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.

#31 Updated by Marius BALTEANU 7 months ago

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.

#32 Updated by Marius BALTEANU 7 months ago

Also, I propose to remove this ticket from 4.1.0 because it will appear in the changelog as part of #3848 and #28234.

#33 Updated by Jean-Philippe Lang 7 months 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.

Also available in: Atom PDF