Patch #22913

Auto-select fields mapping in Importing

Added by Haihan Ji over 4 years ago. Updated 8 months ago.

Status:ClosedStart date:
Priority:NormalDue date:
Assignee:Go MAEDA% Done:

0%

Category:Importers
Target version:4.2.0

Description

I hate to select many times in import-mapping.
This patch can auto select by label before you select it.

auto_select_fields.patch Magnifier (1.14 KB) Haihan Ji, 2016-05-27 15:55

20160606_auto_select_fields_test.patch Magnifier (1.37 KB) Haihan Ji, 2016-06-06 12:28

22913_auto_mapping.patch Magnifier (4.47 KB) Yuichi HARADA, 2019-11-26 06:59

demo_auto_map_fields.patch Magnifier (2.35 KB) Marius BALTEANU, 2020-01-27 00:44

22913_auto_mapping_v2.patch Magnifier (6.36 KB) Yuichi HARADA, 2020-02-13 02:33

0001-Use-user-as-internal-field-instead-of-user_id-becaus.patch Magnifier (4.4 KB) Marius BALTEANU, 2020-02-16 11:35

0002-Auto-select-fields-mapping-in-import-based-on-the-in.patch Magnifier (10 KB) Marius BALTEANU, 2020-02-18 18:00


Related issues

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

Associated revisions

Revision 19523
Added by Go MAEDA 8 months ago

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.

Revision 19524
Added by Go MAEDA 8 months ago

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 over 4 years ago

+1

#2 Updated by Go MAEDA over 4 years ago

Very interesting feature.
Could you add tests? Tests are required to be merged into the core.

#3 Updated by Haihan Ji over 4 years ago

Append Unit Test Code.

#4 Updated by Go MAEDA over 4 years ago

  • Target version set to Candidate for next major release

#5 Updated by Go MAEDA almost 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 11 months ago

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 11 months 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 10 months 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 10 months 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 10 months 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 9 months ago

#12 Updated by Marius BALTEANU 9 months ago

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 8 months ago

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 22913_auto_mapping.patch based on the provided sample code.

#14 Updated by Marius BALTEANU 8 months ago

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 8 months 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 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.

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 8 months 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 8 months ago

  • File deleted (0002-Auto-select-fields-mapping-in-import-based-on-the-in.patch)

#18 Updated by Go MAEDA 8 months ago

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

#19 Updated by Marius BALTEANU 8 months ago

  • Assignee set to Go MAEDA

#20 Updated by Go MAEDA 8 months 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 8 months ago

  • Subject changed from Auto-select fields mapping in Importing a CSV file to Auto-select fields mapping in Importing

Also available in: Atom PDF