+1
--------------------------------------------------------------------------------
Very interesting feature.
Could you add tests? Tests are required to be merged into the core.
--------------------------------------------------------------------------------
Append Unit Test Code.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
I tried this patch but got the following error while processing ImportsController#mapping. Could you test the patch on the current trunk?

<pre>
NameError (undefined local variable or method `issue' for #<ImportsController:0x007fc8be2479b0>
Did you mean? issue_url):
app/controllers/imports_controller.rb:72:in `mapping'
lib/redmine/sudo_mode.rb:63:in `sudo_mode'
</pre>
--------------------------------------------------------------------------------
I fixed attachment:auto_select_fields.patch and attachment:20160606_auto_select_fields_test.patch to work with the current trunk(r19312).
I attach a fixed patch.
--------------------------------------------------------------------------------
Yuichi HARADA wrote:
> I fixed attachment:auto_select_fields.patch and attachment:20160606_auto_select_fields_test.patch to work with the current trunk(r19312).
> I attach a fixed patch.

This is great. Thank you!
Do you find that if custom fields are absent from the default tracker, that they will not match when you load a tracker with more custom fields? I implemented this exactly and found that to be the case.
--------------------------------------------------------------------------------
Joshua Jobin wrote:
> Do you find that if custom fields are absent from the default tracker, that they will not match when you load a tracker with more custom fields? I implemented this exactly and found that to be the case.

Could you show the steps to reproduce the problem?

I didn't see the problem you pointed out with Yuichi HARADA's patch applied. What I did is disable a custom field only in the first tracker.
--------------------------------------------------------------------------------
Setting the target version to 4.2.0.
--------------------------------------------------------------------------------
Go MAEDA wrote:
> Setting the target version to 4.2.0.

The patch cannot be committed as it is, please let me review it.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Sorry for my late review on this.

My main concern with the proposed patch are the hardcoded fields from @mapping@ method (@ImportsController@). Because of them, the "auto mapping" feature it won't be easily extended by plugins or other models (it is against the generalisation added in r18145).

I'm attaching a patch with another solution, where each subclass of the import have a class constant (@AUTO_MAPPABLE_FIELDS@ in my patch) or a method that returns an array with the fields that can be auto-mapped. In @ImportsController@, the method @auto_map_fields@ uses this constant and try to auto map the fields based on some conditions. This auto mapping uses only the "internal" name of the fields, not labels or translations.

To support labels and translations as well, I see two options:
1. We modify the @AUTO_MAPPABLE_FIELDS@ to return a hash with each internal field and his label. For example:
<pre>
AUTO_MAPPABLE_FIELDS = [
'tracker' => 'label_tracker',
'status' => 'label_status',
...
]
</pre>
and then inside the @auto_map_fields@ method we use it.

2. We use Javascript where we have access to all the required information, we just need to iterate throw each select element. This solution can handle both cases.
--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> My main concern with the proposed patch are the hardcoded fields from @mapping@ method (@ImportsController@). Because of them, the "auto mapping" feature it won't be easily extended by plugins or other models (it is against the generalisation added in r18145).
>
> I'm attaching a patch with another solution, where each subclass of the import have a class constant (@AUTO_MAPPABLE_FIELDS@ in my patch) or a method that returns an array with the fields that can be auto-mapped. In @ImportsController@, the method @auto_map_fields@ uses this constant and try to auto map the fields based on some conditions. This auto mapping uses only the "internal" name of the fields, not labels or translations.
>
> To support labels and translations as well, I see two options:
> 1. We modify the @AUTO_MAPPABLE_FIELDS@ to return a hash with each internal field and his label. For example:
> [...]
> and then inside the @auto_map_fields@ method we use it.
>
> 2. We use Javascript where we have access to all the required information, we just need to iterate throw each select element. This solution can handle both cases.

Marius, thank you for pointing it out. It was very helpful because there were some specifications that I knew for the first time.
I fixed attachment:22913_auto_mapping.patch based on the provided sample code.

--------------------------------------------------------------------------------
Thanks Yuichi for updating the patch.

I tried it and I've found some issues:
- the auto mapping is case sensitive. For example: @tracker@ is not mapped to @Tracker@.
- auto mapping by internal fields name (tracker, estimared_hours, cf_6, spent_on) is not working. I find it useful to allow this as well for advanced users.
- the auto mapping works only when @import.mapping@ is empty. I don't think that this condition is necessary because you already check inside the @auto_map_fields@ method if the mapping for that field exists or not.

I decided to rework a little bit your patch in order to fix those issues, other edge cases and to reuse the existing import time entries CSV file, please let me know what do you thing. The first patch changes the internal field name for @User@ from @user_id@ to @user@ because the import accepts also logins.

Test results here: https://gitlab.com/redmine-org/redmine/pipelines/118278329

--------------------------------------------------------------------------------
Marius BALTEANU wrote:
> Thanks Yuichi for updating the patch.
>
> I tried it and I've found some issues:
> - the auto mapping is case sensitive. For example: @tracker@ is not mapped to @Tracker@.
> - auto mapping by internal fields name (tracker, estimared_hours, cf_6, spent_on) is not working. I find it useful to allow this as well for advanced users.
> - the auto mapping works only when @import.mapping@ is empty. I don't think that this condition is necessary because you already check inside the @auto_map_fields@ method if the mapping for that field exists or not.
>
> I decided to rework a little bit your patch in order to fix those issues, other edge cases and to reuse the existing import time entries CSV file, please let me know what do you thing. The first patch changes the internal field name for @User@ from @user_id@ to @user@ because the import accepts also logins.
>
> Test results here: https://gitlab.com/redmine-org/redmine/pipelines/118278329

Thanks, Marius for improving the patches. I confirmed that it works properly.
In particular, it was very good that the @test/functional/imports_controller_test.rb@ was written in detail and was easy to understand. However, I think it would be better if the following were modified.

<pre><code class="diff">
diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb
index 664d40019..96c493a31 100644
--- a/app/controllers/imports_controller.rb
+++ b/app/controllers/imports_controller.rb
@@ -186,7 +186,7 @@ class ImportsController < ApplicationController
next if mappings.include?(field_nm)
index = headers.index(field_nm) || headers.index(field.name.downcase)
if index
- mappings[field_nm] = headers.index(field_nm) || headers.index(field.name.downcase)
+ mappings[field_nm] = index
end
end
mappings
</code></pre>
--------------------------------------------------------------------------------
Yuichi HARADA wrote:
> Marius BALTEANU wrote:
> [...]
>
> Thanks, Marius for improving the patches. I confirmed that it works properly.
You're welcome.

> In particular, it was very good that the @test/functional/imports_controller_test.rb@ was written in detail and was easy to understand. However, I think it would be better if the following were modified.
Agree, I've updated the second patch. Thanks for pointing this out.

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
The patch series works fine. Thank you.
I will commit them in a few days.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------
Committed the patches.

Thank you all for working on this nice improvement.
--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

--------------------------------------------------------------------------------

relates,Closed,28234,Add CSV Import for Time Entries
relates,Closed,34326,CSV import raises an exception if CSV header has empty columns
relates,Closed,35131,Issue import - allow auto mapping for Unique ID and relation type fields