Feature #22913
Auto-select fields mapping in Importing
Status: | Closed | Start date: | ||
---|---|---|---|---|
Priority: | Normal | Due date: | ||
Assignee: | % Done: | 0% | ||
Category: | Importers | |||
Target version: | 4.2.0 | |||
Resolution: | Fixed |
Description
I hate to select many times in import-mapping.
This patch can auto select by label before you select it.
Related issues
Associated revisions
Use 'user' as internal field instead of user_id because the column accepts also user login as value, not only the id (#22913).
Patch by Marius BALTEANU.
Auto select fields mapping in import based on the internal field name (ex: estimared_hours, fixed_version, spent_on) or field label (Estimated hours, Version, Date) (#22913).
- mappings are case insensitive
- a field is auto mapped only if there is no mapping setting present for that field
- "Current user" default value for User field when the user has permission to log time for other users is override by the auto mapped column
Patch by Haihan Ji, Yuichi HARADA, and Marius BALTEANU.
History
#1
Updated by Sebastian Paluch almost 5 years ago
+1
#2
Updated by Go MAEDA almost 5 years ago
Very interesting feature.
Could you add tests? Tests are required to be merged into the core.
#3
Updated by Haihan Ji almost 5 years ago
- File 20160606_auto_select_fields_test.patch
added
Append Unit Test Code.
#4
Updated by Go MAEDA almost 5 years ago
- Target version set to Candidate for next major release
#5
Updated by Go MAEDA over 4 years ago
- Status changed from New to Needs feedback
I tried this patch but got the following error while processing ImportsController#mapping. Could you test the patch on the current trunk?
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'
#6
Updated by Yuichi HARADA over 1 year ago
- File 22913_auto_mapping.patch
added
I fixed auto_select_fields.patch and 20160606_auto_select_fields_test.patch to work with the current trunk(r19312).
I attach a fixed patch.
#7
Updated by Joshua Jobin over 1 year ago
Yuichi HARADA wrote:
I fixed auto_select_fields.patch and 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.
#8
Updated by Go MAEDA over 1 year ago
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.
#9
Updated by Go MAEDA over 1 year ago
- Status changed from Needs feedback to New
- Target version changed from Candidate for next major release to 4.2.0
Setting the target version to 4.2.0.
#10
Updated by Marius BALTEANU over 1 year ago
- Assignee set to Marius BALTEANU
Go MAEDA wrote:
Setting the target version to 4.2.0.
The patch cannot be committed as it is, please let me review it.
#11
Updated by Marius BALTEANU about 1 year ago
- Related to Feature #28234: Add CSV Import for Time Entries added
#12
Updated by Marius BALTEANU about 1 year ago
- File demo_auto_map_fields.patch
added
- Assignee deleted (
Marius BALTEANU)
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:
AUTO_MAPPABLE_FIELDS = [ 'tracker' => 'label_tracker', 'status' => 'label_status', ... ]
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.
#13
Updated by Yuichi HARADA about 1 year ago
- File 22913_auto_mapping_v2.patch
added
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. InImportsController
, the methodauto_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 theAUTO_MAPPABLE_FIELDS
to return a hash with each internal field and his label. For example:
[...]
and then inside theauto_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 22913_auto_mapping.patch based on the provided sample code.
#14
Updated by Marius BALTEANU about 1 year ago
- File 0002-Auto-select-fields-mapping-in-import-based-on-the-in.patch added
- File 0001-Use-user-as-internal-field-instead-of-user_id-becaus.patch
added
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
#15
Updated by Yuichi HARADA about 1 year ago
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 toTracker
.
- 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 whenimport.mapping
is empty. I don't think that this condition is necessary because you already check inside theauto_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
fromuser_id
touser
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.
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
#16
Updated by Marius BALTEANU about 1 year ago
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.
#17
Updated by Marius BALTEANU about 1 year ago
- File deleted (
0002-Auto-select-fields-mapping-in-import-based-on-the-in.patch)
#18
Updated by Go MAEDA about 1 year ago
The patch series works fine. Thank you.
I will commit them in a few days.
#19
Updated by Marius BALTEANU about 1 year ago
- Assignee set to Go MAEDA
#20
Updated by Go MAEDA about 1 year ago
- Subject changed from Auto-select fields mapping in Importing to Auto-select fields mapping in Importing a CSV file
- Status changed from New to Closed
Committed the patches.
Thank you all for working on this nice improvement.
#21
Updated by Go MAEDA about 1 year ago
- Subject changed from Auto-select fields mapping in Importing a CSV file to Auto-select fields mapping in Importing
#22
Updated by Go MAEDA 5 months ago
- Related to Defect #34326: CSV import raises an exception if CSV header has empty columns added