Redmine

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?

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'

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.

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:

  AUTO_MAPPABLE_FIELDS = [
    'tracker' => 'label_tracker',
    'status' => 'label_status',
    ...
  ]

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 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

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.

Committed the patches.

Thank you all for working on this nice improvement.