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

Actions

Copy link

Feature #28234

closed

Add CSV Import for Time Entries

Added by Gregor Schmidt over 6 years ago. Updated about 5 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Go MAEDA

Category:

Importers

Target version:

4.1.0

Start date:

Due date:

% Done:

Estimated time:

Resolution:

Fixed

Description

Preface¶

The imports controller was obviously built to be used for more than just issue imports. But since Redmine currently only supports issue imports, there are now some places, where assumptions about the issue import are hard-coded.

There are some plugins out there, which add other kinds of CSV imports to Redmine, but none of them seems to use the imports controller. I assume, it's because of the shortcomings mentioned above. I wanted to improve this situation, so I decided to build a time entry importer and - along the way - generalize the imports controller so that it's able to handle more than just issues.

Preparations¶

The first patch attached extracts the issue related view parts into partials which are determined by convention. They are all prefixed with the imported model name, i.e. _issues.

The issue related model parts are moved to the import model. Those are, e.g. the proper menu item, the authorization or the layout. Strictly speaking they should be located on the controller level, but I did not want to build sub classes of the imports controller for each and every kind of import.

Adding the time entry import¶

The second patch attached adds the time entry import. It's using the newly introduced extension points to fill in the blanks. To provide a way for the user to find the new import, I've added the timelog/_sidebar partial, similar to the one for issues. This new sidebar partial is now used in the timelog views and it contains a link to the import.

Note: within said partial, I've mis/re-used an i18n key label_time_entries_visibility_all since it was providing the desired text in German and English. I did not want to introduce a new key based on the existing one, since that would have probably caused major merge problems. If desired, I may of course do that anyway.

Closing notes¶

The second patch may of course also become a plugin. The main point of this patch (series) was it, to allow plugins to reuse the existing plugin infrastructure. Without the first patch though, a plugin author would need copy most of the views and functionality of the imports class, which is undesirable (at least for the plugin author).

Thank you for considering these patches.

Files

Download all files

0002-Import-time-entries.patch (16.5 KB) 0002-Import-time-entries.patch	updated version (v2)	Gregor Schmidt, 2018-03-05 13:09
0001-Generalize-issues-imports.patch (16.4 KB) 0001-Generalize-issues-imports.patch	updated version (v2)	Gregor Schmidt, 2018-03-05 13:09
0001-Generalize-issues-imports.patch (22.6 KB) 0001-Generalize-issues-imports.patch	updated version (v3)	Gregor Schmidt, 2018-03-12 09:21
0002-Import-time-entries.patch (16.5 KB) 0002-Import-time-entries.patch	updated version (v3)	Gregor Schmidt, 2018-03-12 09:21

Related issues

Actions

Copy link

Updated by Jan from Planio www.plan.io over 6 years ago

Target version set to Candidate for next minor release

Actions

Copy link

Updated by Mischa The Evil over 6 years ago

Thanks for sharing this, Gregor. I really like these patches (especially the first one).
I'd target this issue against Unplanned backlogs – along with #28198 and #28213, but since Jan already set the target to Candidate for next minor release, I'll leave it as-is.

Actions

Copy link

Updated by Marius BĂLTEANU over 6 years ago

Has duplicate Feature #25578: Import spent time added

Actions

Copy link

Updated by Gregor Schmidt over 6 years ago

File 0001-Generalize-issues-imports.patch 0001-Generalize-issues-imports.patch added
File 0002-Import-time-entries.patch 0002-Import-time-entries.patch added

In my initial version, I've missed the JavaScript-based updates of the form. This is fixed in the new patch files, which are attached to this note. The initial ones are now obsolete.

Actions

Copy link

Updated by Go MAEDA over 6 years ago

File deleted (~~0002-Import-time-entries.patch~~)

Actions

Copy link

Updated by Go MAEDA over 6 years ago

File deleted (~~0001-Generalize-issues-imports.patch~~)

Actions

Copy link

Updated by Go MAEDA over 6 years ago

Status changed from New to Needs feedback
Assignee set to Gregor Schmidt

I tried out the import time entries feature and it works well. Thank you for the patch.

But it seems that the issue import feature is broken after applying the patch. Maybe the patch missing some view partials. Could you check the patch?

$ ruby test/functional/imports_controller_test.rb
Run options: --seed 16739

# Running:

F

Failure:
ImportsControllerTest#test_get_mapping_should_display_mapping_form [test/functional/imports_controller_test.rb:154]:
Expected response to be a <2XX: success>, but was a <404: Not Found>
<?pre>

Actions

Copy link

Updated by Gregor Schmidt over 6 years ago

Thank you very much for taking a look at these changes.

I'm afraid, I cannot reproduce the problem. I've used git am to re-apply the patches to my local git clone of https://github.com/redmine/redmine and the tests work fine.

I can see three potential causes

git am does different things than patch -p0. Therefore maybe the rename of one of the files, wasn't applied properly. Do you have a file called app/views/imports/_issues_fields_mapping.html.erb? If you have this file, then this is not the problem.
The functional test may be missing some fixture definition. Do the test work, when you run them all together?
I don't have all the optional Redmine dependencies installed (e.g. ImageMagick, ldap things and so on) and I'm running the tests with sqlite. Maybe one of the optional dependencies causes the problem?

I think, the first is the most likely one. If you can confirm this, I'll need to figure out how to create fully patch-compatible diff files.

Do you maybe have additional information in you log/test.log?

Actions

Copy link

Updated by Go MAEDA over 6 years ago

Status changed from Needs feedback to New
Assignee deleted (~~Gregor Schmidt~~)

Gregor Schmidt wrote:

git am does different things than patch -p0. Therefore maybe the rename of one of the files, wasn't applied properly. Do you have a file called app/views/imports/_issues_fields_mapping.html.erb? If you have this file, then this is not the problem.

You are right. I applied the patch with patch -p1. Applying the patch with git am solved the problem. I am sorry for bothering you and thank you so much for kindly advising me about it.

With patch -p1, I got the following error:

  Rendering imports/mapping.html.erb within layouts/base
  Rendered imports/_issues_mapping.html.erb (4.4ms)
  Rendered imports/mapping.html.erb within layouts/base (8.0ms)
Missing template, responding with 404: Missing partial imports/_issues_fields_mapping, application/_issues_fields_mapping with {:locale=>[:en], :formats=>[:html], :variants=>[], :handlers=>[:raw, :erb, :html, :builder, :ruby, :rsb]}. Searched in:
...

Actions

Copy link

#10

Updated by Gregor Schmidt over 6 years ago

File 0001-Generalize-issues-imports.patch 0001-Generalize-issues-imports.patch added
File 0002-Import-time-entries.patch 0002-Import-time-entries.patch added

Attached you may find an updated version of the patch files. This time they should be fully compatible with the patch command.

Actions

Copy link

#11

Updated by Go MAEDA over 6 years ago

Target version changed from Candidate for next minor release to 4.1.0

LGTM, setting target version to 4.1.0.

Actions

Copy link

#12

Updated by Go MAEDA about 6 years ago

Assignee set to Jean-Philippe Lang

Actions

Copy link

#13

Updated by Go MAEDA over 5 years ago

Assignee changed from Jean-Philippe Lang to Go MAEDA

I think there is no reason not to deliver this change in 4.1.0. According to the article https://support.plan.io/news/141, Planio deployed this change a year ago. This code has been tested in a production environment for a year and maybe has not encountered any major problems.

I will commit the patches in a few days.

Actions

Copy link

#14

Updated by Jean-Philippe Lang over 5 years ago

Sounds good to me.

Actions

Copy link

#15

Updated by Go MAEDA over 5 years ago

Status changed from New to Closed
Resolution set to Fixed

Committed the patches. Thank you for extracting the great feature from Planio.

Jean-Philippe Lang wrote:

Sounds good to me.

Thank you for your feedback!

Actions

Copy link

#16

Updated by Marius BĂLTEANU over 5 years ago

Go MAEDA wrote:

Committed the patches. Thank you for extracting the great feature from Planio.

Great news!

Actions

Copy link

#17

Updated by Go MAEDA over 5 years ago

Related to Defect #31508: Add missing frozen strings and copyrights added

Actions

Copy link

#18

Updated by Marius BĂLTEANU about 5 years ago

Related to Patch #32196: Allow import time entries for other users added

Actions

Copy link

#19

Updated by Marius BĂLTEANU about 5 years ago

I've added a new ticket (#32196) where we should discuss how to allow importing time entries for other users because the current implementation sticks to the current user only.

Actions

Copy link

#20