Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add field mapping step to csv contact import #7045

Merged
merged 1 commit into from
Oct 11, 2020

Conversation

johndoh
Copy link
Contributor

@johndoh johndoh commented Nov 16, 2019

importing csv files into the address book can be a little tricky and there are few tickets from confused users asking about column names and things like that so I thought may be a good solution is add an intermediate step into the CSV file import process (does not effect vCards) which shows what Roundcube guessed for the field mapping and give the user the opportunity to change that mapping.

Untitled-1

I'm not sure about my addition to Elastic ui.js I think there must be a better way to get the styling of the table header right, any hints?

@johndoh johndoh force-pushed the csvimport branch 8 times, most recently from 84ba0f4 to 8170b5e Compare November 17, 2019 10:02
@alecpl
Copy link
Member

alecpl commented Nov 19, 2019

In new code we should generate HTML with bootstrap/elastic classes and make sure it works in other skins. That change in Elastic should go away completely.

@johndoh
Copy link
Contributor Author

johndoh commented Nov 19, 2019

In new code we should generate HTML with bootstrap/elastic classes

This might be really over complicated and not practical but what if there was a way for a skin to define default classes for specific HTML elements globally?

That change in Elastic should go away completely.

Agreed

@alecpl
Copy link
Member

alecpl commented Nov 20, 2019

This might be really over complicated and not practical but what if there was a way for a skin to define default classes for specific HTML elements globally?

I would rather make core to output bootstrap compatible html and make old skins understand that. What I don't like in current Elastic is a lot of code that converts old html structures (tables, forms) programmatically. We should be looking for possibility to get rid of that (and some css that is there only because bootstrap does not like forms based on tables). My point is, it would be better to not make the core php code more complicated.

I think we should spend some time in 1.5 on this. 1.4 was "look we can do this and that", but in 1.5 we should optimize Elastic.

@alecpl
Copy link
Member

alecpl commented Oct 3, 2020

@johndoh do you plan to finish this? Would be nice to have it soon-ish as I plan some refactoring that will make the merge harder.

@johndoh
Copy link
Contributor Author

johndoh commented Oct 3, 2020

I think its done apart from the change in ui.js, I don't know how to solve that. any pointers appreciated.

@johndoh
Copy link
Contributor Author

johndoh commented Oct 3, 2020

Also I added a behavioral change with c9adad9 before information inside the row of a gmail CSV file was used to decide if (e.g.) a phone number was home or work etc but with the mapping screen its by column so type attribute in the row is ignored in favour of what the user specified for the column. This was gmail specific behavior and made user specified mapping impossible and, for me, the user specified mapping is more useful.

@mandza
Copy link

mandza commented Oct 3, 2020

I think we should spend some time in 1.5 on this. 1.4 was "look we can do this and that", but in 1.5 we should optimize Elastic.

I am all in for optimization. If you point me in the right direction, where you need some optimization I am more then glad to help.

@alecpl
Copy link
Member

alecpl commented Oct 3, 2020

I was thinking about getting rid as much as possible of bootstrap_style() in ui.js, but that is not a simple task. And that is also what we should do with this change in ui.js, i.e. generate the HTML "bootstrap-style" and make it look good also in other skins.

@johndoh
Copy link
Contributor Author

johndoh commented Oct 3, 2020

@alecpl as you say, this is not simple. just in this case for example to get the correct display a class needs to be set on the thead->tr but setting classes on rows in table headers is not supported by the html_table() class.

In my latest commit I've added support for it and removed the changes from ui.js, is this anything like what you had in mind?

I have checked that the addition of these classes has no impact on the Classic or Larry skins.

Copy link
Member

@alecpl alecpl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better, and probably good enough for this PR. I'll give it a try the next weekend(s).

In general we should aim for replacing these old structures based on tables with ones based on divs, like Bootstrap does, but this is a lot of work (with BC break for plugins and skins) and I'm not going to work on that in 1.5.

skins/elastic/templates/contactimport.html Outdated Show resolved Hide resolved
@johndoh johndoh marked this pull request as ready for review October 6, 2020 08:06
@johndoh
Copy link
Contributor Author

johndoh commented Oct 6, 2020

I've rebased and squashed all the commits

@alecpl
Copy link
Member

alecpl commented Oct 11, 2020

@johndoh there's an error and a failure in tests on PHP 7.4. Could you fix this?

1) Framework_Csv2vcard::test_import_generic
count(): Parameter must be an array or an object that implements Countable

/home/alec/repos/roundcubemail/program/lib/Roundcube/rcube_csv2vcard.php:542
/home/alec/repos/roundcubemail/program/lib/Roundcube/rcube_csv2vcard.php:430
/home/alec/repos/roundcubemail/tests/Framework/Csv2vcard.php:16
/home/alec/repos/roundcubemail/vendor/phpunit/phpunit/phpunit:61

There was 1 failure:

1) Framework_Csv2vcard::test_import_gmail
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
 CATEGORIES:My Contacts,Test\n
 EMAIL;TYPE=INTERNET;TYPE=HOME:home@aaa.pl\n
 EMAIL;TYPE=INTERNET;TYPE=WORK:work@email.pl\n
-EMAIL;TYPE=INTERNET;TYPE=OTHER:unknown@email.com\n
-EMAIL;TYPE=INTERNET;TYPE=OTHER:other@email.com\n
 TEL;TYPE=pager:pager\n
 TEL;TYPE=pref:mainphone\n
 TEL;TYPE=home:homephone\n

/home/alec/repos/roundcubemail/tests/Framework/Csv2vcard.php:74
/home/alec/repos/roundcubemail/vendor/phpunit/phpunit/phpunit:61

@johndoh
Copy link
Contributor Author

johndoh commented Oct 11, 2020

failing tests fixed

@alecpl
Copy link
Member

alecpl commented Oct 11, 2020

And conflicts again, sorry. Could you resolve this?

@johndoh
Copy link
Contributor Author

johndoh commented Oct 11, 2020

done

@alecpl alecpl merged commit 7b2f135 into roundcube:master Oct 11, 2020
@johndoh johndoh deleted the csvimport branch October 11, 2020 10:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants