Skip to content

Allow user to import contacts into custom addressbook#123

Merged
skjnldsv merged 38 commits intomasterfrom
import-into-custom-ab
Aug 15, 2017
Merged

Allow user to import contacts into custom addressbook#123
skjnldsv merged 38 commits intomasterfrom
import-into-custom-ab

Conversation

@skjnldsv
Copy link
Member

@skjnldsv skjnldsv commented Feb 7, 2017

fix #55, fix #89

  • Ui-select
  • Design
  • Controller edit
  • Directive fix
  • Disabled addressbook input autotomplete, autocorrect and spellcheck
  • Settings closing when clicking on select

Vcard test example of 500 contacts: 500.txt

capture d ecran_2017-02-07_15-07-43

@nextcloud/contacts please review

@skjnldsv skjnldsv added 2. developing Work in progress enhancement New feature or request labels Feb 7, 2017
@skjnldsv skjnldsv added this to the 1.5.4 milestone Feb 7, 2017
@skjnldsv skjnldsv self-assigned this Feb 7, 2017
@codecov-io
Copy link

codecov-io commented Feb 7, 2017

Codecov Report

Merging #123 into master will decrease coverage by 0.43%.
The diff coverage is 4.54%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage    15.3%   14.87%   -0.44%     
==========================================
  Files          52       55       +3     
  Lines        1163     1217      +54     
==========================================
+ Hits          178      181       +3     
- Misses        985     1036      +51
Impacted Files Coverage Δ
js/components/avatar/avatar_directive.js 7.14% <ø> (+0.89%) ⬆️
...mponents/contactImport/contactImport_controller.js 6.25% <0%> (-27.09%) ⬇️
...omponents/contactImport/contactImport_directive.js 2.94% <0%> (-2.95%) ⬇️
js/services/contact_service.js 0.67% <0%> (-0.04%) ⬇️
js/components/groupList/groupList_controller.js 6.66% <0%> (ø) ⬆️
...s/components/contactList/contactList_controller.js 1.72% <0%> (-0.05%) ⬇️
...components/importScreen/importScreen_controller.js 12.5% <12.5%> (ø)
js/services/import_service.js 16.66% <16.66%> (ø)
.../components/importScreen/importScreen_directive.js 50% <50%> (ø)
... and 3 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20455cd...cbd0f98. Read the comment docs.

@skjnldsv skjnldsv force-pushed the import-into-custom-ab branch 3 times, most recently from d702330 to e6d1110 Compare February 7, 2017 15:02
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Feb 7, 2017
@tcitworld
Copy link
Member

tcitworld commented Feb 7, 2017

IMHO, the UX would be much better if a popup opened after selecting the file to choose the addressbook in which to import it, just like the calendar app does.

@skjnldsv
Copy link
Member Author

skjnldsv commented Feb 7, 2017

And implementing #89 ?
Could be great, but require far much work ^^

@skjnldsv
Copy link
Member Author

@tcitworld I disagree with you. The popup on calendar just add a useless step on choosing where you want to import. Having everything on the same location is much more easier UX wise that having multiple windows that open forcing the user to look away and move his mouse somewhere else.

@tcitworld
Copy link
Member

You have a point, but it doesn't feel practical with if the list of addressbooks is too long. Let's say most people won't have many addressbooks.

@skjnldsv
Copy link
Member Author

Yes most of them. But you just made a nice point too, I need to enable the search in case of too many addressbooks 😝

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Feb 20, 2017
@Tost69
Copy link

Tost69 commented Apr 28, 2017

May be most of the people are using one addressbook only, but what if it's not enough?
I have already two: one for private contacts, one for work. Additionally I have two more shared addressbooks in my account. More than 400 entries totally and for now there is no way to view only work contacts, I have to fiddle them out of the big list one by one. And if I want to export some of them there is only the possibility to export the single contacts one after another or I can choose to export the whole selected addressbook (pretty much ignoring that I only need some members of a contact group to be exported).
How about to bring back the addressbook selection checkboxes next to the download/link/share/delete buttons in the config section like in the Owncloud contacts app and a checkbox in the contact item panel for further actions like export or adding to a group? That would be really nice (and yes, I know it's not a oneliner).
Nevertheless thanks for your great work, kind regards, Tom

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 14, 2017

This is what it looks on mobile
kazam_screencast_00001

  • Auto hide sidebar if mobile
  • Fix default selected ab after a delete

@xh3n1
Copy link
Member

xh3n1 commented Aug 14, 2017

It looks really good 👍

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@jancborchardt
Copy link
Member

Nice! As said I would just add an icon above the text like in the usual emptycontent views. :) icon-upload

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv force-pushed the import-into-custom-ab branch from a9c56bd to 686b7c9 Compare August 15, 2017 06:25
@skjnldsv
Copy link
Member Author

Done
capture d ecran_2017-08-15_08-44-01

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@jancborchardt
Copy link
Member

👍 all good from my side now, looks very good! :)

@jancborchardt
Copy link
Member

Another review please @nextcloud/javascript? :)

@skjnldsv
Copy link
Member Author

@jancborchardt Still waiting for the sidebar block to avoid user interaction too :)

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Aug 15, 2017
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 15, 2017

Okay, I need to fix the addressbook deletion update, But since this is a bigger work, I will do it in another pr.

@jancborchardt, @ChristophWurst, @MorrisJobke, @Henni, @irgendwie, @jonatoni, @xh3n1
Please review!! 😄 ❤️

Test: Import this big file: 500vcards.txt

  • See the loading animation with the spinner
  • See the percentage of completion increasing in the main screen
  • See the imported user switch under the progress bar
  • See no change on the url nor the contacts while the import is in background
  • See the load of the first contact/group once everything is done
  • Try the search (if more than 5 addressbooks)
  • See the auto-hide of the sidebar on mobile when the import starts

Test package: contacts.tar.gz

Signed-off-by: John Molakvoæ (skjnldsv) <skjnldsv@protonmail.com>
Copy link
Member

@jonatoni jonatoni left a comment

Choose a reason for hiding this comment

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

Good job @skjnldsv 👍

@nursoda
Copy link

nursoda commented Aug 15, 2017

Works :-)

I spotted one glitch:

  • have one addressbook 'first'
  • create another ab 'second'
  • 'import into' shows dropdown, one may select 'first' and 'second'
  • switch that dopdown to 'second'
  • delete ab 'second' through it's context menu (three dots)
  • 'import into' still shows 'second'(!)

Nice to have:

  • an 'abort import' button, especially for big imports

Related:

  • If one imports the 500 and deletes the test ab, then all test groups (and contacts) remain visible although they are gone. Can we trigger a refresh after deleting an ab?!

@skjnldsv
Copy link
Member Author

skjnldsv commented Aug 15, 2017

@nursoda all the bug you found are known, I created a pr for the contacts/groups list update after deletion, and will also fix the selection at the same time!
See #288
Thank you very much for your review, it's perfect!

@jonatoni & @xh3n1 thanks!! 😉

@skjnldsv skjnldsv merged commit 2bebe03 into master Aug 15, 2017
@skjnldsv skjnldsv deleted the import-into-custom-ab branch August 15, 2017 13:47
@devillemereuil
Copy link

devillemereuil commented Aug 15, 2017

I just tested this PR on Nextcloud 12. I've found two issues:

  • When imported the first time, the contacts cannot be displayed. I get "No Contacts Here". If I quit Contacts (i.e. change to another Nextcloud App) and come back again, then they are displayed.
  • Once they are displayed, the first contact to be displayed is assigned to the correct AddressBook (field at the bottom), but all the others are assigned to the first, pre-existing addressbook. Doesn't seem that they actually are misassigned though, because removing the addressbook does remove all the imported contacts. So, probably just a display issue.

I tested the 500 contacts provided by @skjnldsv and another one of mine waiting for this awesome feature to be imported! ;)

@skjnldsv
Copy link
Member Author

@devillemereuil It's possible that your server is having trouble loading the contact you just clicked since it has received a lot of requests after the import.
I can't reproduce here :/

@devillemereuil
Copy link

Possible. The contacts do not seem to be sync with CardDAV either... Weird. I'll let it settle a bit and report back if it stays the same...

@devillemereuil
Copy link

A few hours after uploading my VCFs, during which I even rebooted the server, I'm still having the same issues as described above... Also the contacts are not visible through CardDAV (the address books are synced as empty...). Weird. Might be something wrong in my install...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews enhancement New feature or request high High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Importing large contacts file is very heavy Allow vCard importing into custom AddressBook

9 participants