Project

General

Profile

Actions

Feature #22913

closed

Auto-select fields mapping in Importing

Added by Haihan Ji almost 8 years ago. Updated almost 3 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Importers
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed

Description

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


Files


Related issues

Related to Redmine - Feature #28234: Add CSV Import for Time EntriesClosedGo MAEDA

Actions
Related to Redmine - Defect #34326: CSV import raises an exception if CSV header has empty columnsClosedGo MAEDA

Actions
Related to Redmine - Defect #35131: Issue import - allow auto mapping for Unique ID and relation type fieldsClosedGo MAEDA

Actions
Actions #1

Updated by Sebastian Paluch almost 8 years ago

+1

Actions #2

Updated by Go MAEDA almost 8 years ago

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

Actions #4

Updated by Go MAEDA almost 8 years ago

  • Target version set to Candidate for next major release
Actions #5

Updated by Go MAEDA about 7 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'
Actions #6

Updated by Yuichi HARADA over 4 years 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.

Actions #7

Updated by Joshua Jobin over 4 years 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.

Actions #8

Updated by Go MAEDA about 4 years 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.

Actions #9

Updated by Go MAEDA about 4 years 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.

Actions #10

Updated by Marius BĂLTEANU about 4 years ago

  • Assignee set to Marius BĂLTEANU

Go MAEDA wrote:

Setting the target version to 4.2.0.

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

Actions #11

Updated by Marius BĂLTEANU about 4 years ago

Actions #12

Updated by Marius BĂLTEANU about 4 years 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.

Actions #13

Updated by Yuichi HARADA about 4 years 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.

Actions #14

Updated by Marius BĂLTEANU about 4 years 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

Actions #15

Updated by Yuichi HARADA about 4 years 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
Actions #16

Updated by Marius BĂLTEANU about 4 years 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.

Actions #17

Updated by Marius BĂLTEANU about 4 years ago

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

Updated by Go MAEDA about 4 years ago

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

Actions #19

Updated by Marius BĂLTEANU about 4 years ago

  • Assignee set to Go MAEDA
Actions #20

Updated by Go MAEDA about 4 years 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.

Actions #21

Updated by Go MAEDA about 4 years ago

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

Updated by Go MAEDA over 3 years ago

  • Related to Defect #34326: CSV import raises an exception if CSV header has empty columns added
Actions #23

Updated by Go MAEDA almost 3 years ago

  • Tracker changed from Patch to Feature
  • Resolution set to Fixed
Actions #24

Updated by Marius BĂLTEANU almost 3 years ago

  • Related to Defect #35131: Issue import - allow auto mapping for Unique ID and relation type fields added
Actions

Also available in: Atom PDF