Contact import improvements #9431
Conversation
…ort UI, remove hard coded $local_map
…ist_fields as it relies on rcmail_action_contacts
|
I moved the Also I completed the second part of my suggestion to remove the hard coded mapping information entirely and use 1 to define the source and destination. |
| @@ -201,7 +201,7 @@ class rcmail_action_contacts_index extends rcmail_action | |||
| 'size' => 40, | |||
| 'maxlength' => 128, | |||
| 'label' => 'instantmessenger', | |||
| 'subtypes' => ['aim', 'icq', 'msn', 'yahoo', 'jabber', 'skype', 'other'], | |||
| 'subtypes' => ['aim', 'icq', 'msn', 'yahoo', 'jabber', 'skype'], | |||
There was a problem hiding this comment.
Because it's not supported by the rest of the code. It's not included in the vCard, so once you hit save the value is lost.
| $map['work_state'] = "Work State"; | ||
| $map['work_title'] = "Work Title"; | ||
| $map['work_zip'] = "Work Zip"; | ||
| $map['anniversary'][] = "Jahrestag"; |
There was a problem hiding this comment.
I don't like this change. We should keep this file syntax simple.
There was a problem hiding this comment.
What is not simple about the syntax? This way, the mapping process is clearer. There is only one map of CSV field to vCard field rather than the CSV to internal ID, then a hard coded map of internl ID to vCard used currebtly. It also adds support variations of field names in the CSV file.
There was a problem hiding this comment.
$map['anniversary'][] is not simple ;)
There was a problem hiding this comment.
The reason for the nested array is to allow multiple labels to map to the same vCard field, e.g. currently the CSV labels first and given_name both map to vCard field firstname. Currently this is done via the hard coded rcube_csv2vcard::$csv2vcard_map. For me this is not simple. Adding a new field into the map means a change in 2 files, you have to know the hard coded map is there, there is no info about it in csv2vcard.inc. Using a nested array may not be simple but for me it is simpler than the current approach as it means there is only 1 place to change when adding a new field.
There was a problem hiding this comment.
I understand your approach but also understand Alec's concern and share the latter here. To me, csv2vcard.inc is a config file, and so the priority should be readability for humans. Understanding the syntax is one thing (which might be hard for non-PHP-developers), another thing I don't understand without intimate knowledge of the code is why e.g. email:other gets three values:
$map['email:other'][] = "E-mail Address";
$map['email:other'][] = "E-mail 2 Address";
$map['email:other'][] = "E-mail 3 Address";
Can we find a way to specify the mapping while keeping your approach (which I do like!) to de-duplicate and strip indirection?
You're removing this variable in this PR already and to me it looks like information gets lost there, because there's no mapping e.g. of |
|
I should have updated my comment when I changed my approach. You are totally right that this PR now removes that variable. It happens in this commit.
The $map['email:other'][] = "E-mail Address";
$map['email:other'][] = "E-mail 2 Address";
$map['email:other'][] = "E-mail 3 Address"; |
For #9419
Some of the mappings in the current code work (pass the tests) but the result is different to what you get if you create the same contact manually through the UI and then export it as a vCard, for example Roundcube uses
phone:mobilenotphone:celland there is noemail:preffield. I have changed the mapping so it matches the fields in the UI. I added some more aliasesrcube_vcardto show existing vCards with mismatched fields nicely.I have removed the subtype
otherfrom the IM field because that does not exist inrcube_vcardwhich means if when you hit save in the UI the value simply disappears, it’s not saved.I have removed the variable
rcube_csv2vcard::$label_mapbecause it’s a duplicate of contents of/program/localization/en_US/csv2vcard.incand just a bit confusing currently/program/localization/en_US/csv2vcard.incis not used by the code and this PR changes that.I have another reason for wanting to use
/program/localization/en_US/csv2vcard.incoverrcube_csv2vcard::$label_mapand if this PR gets merged then I will propose 1 additional change which will remove thercube_csv2vcard::$csv2vcard_mapvariable and simplify the mapping process by storing both the source and destination for the map in/program/localization/en_US/csv2vcard.incand allowing multiple sources (including multiple languages) for the same destination. This is a slightly larger and may be more controversial change hence the separate PR.