Vote #79164
完了Add CSV Import for Time Entries
0%
説明
h3. 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.
h3. 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.
h3. 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.
h3. 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.
journals
--------------------------------------------------------------------------------
Thanks for sharing this, Gregor. I really like these patches (especially the first one).
I'd target this issue against version#23 – along with #28198 and #28213, but since Jan already set the target to version#33, I'll leave it as-is.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
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?
<pre>
$ 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>
--------------------------------------------------------------------------------
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@?
--------------------------------------------------------------------------------
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:
<pre>
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:
...
</pre>
--------------------------------------------------------------------------------
Attached you may find an updated version of the patch files. This time they should be fully compatible with the @patch@ command.
--------------------------------------------------------------------------------
LGTM, setting target version to 4.1.0.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
Sounds good to me.
--------------------------------------------------------------------------------
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!
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Committed the patches. Thank you for extracting the great feature from Planio.
Great news!
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
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.
--------------------------------------------------------------------------------
I'm reopening this in the light of #32196, I think it is not to late to fix at least the missing "Import time entries" permission.
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
--------------------------------------------------------------------------------
related_issues
relates,Closed,31508,Add missing frozen strings and copyrights
relates,Closed,32196,Allow import time entries for other users
relates,Closed,22913,Auto-select fields mapping in Importing
relates,Closed,33027,Fix missing closing div in _time_entries_fields_mapping.html.erb
duplicates,Closed,25578,Import spent time