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

[issue-#39] It is now possible to download group of contacts. #667

Merged

Conversation

charismatic-claire
Copy link
Contributor

I everyone. I implemented a way to solve issue #39 .
Please review and let me know if I need to change this in order to make it really work.

Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Hello!
Thanks for your pr! Unfortunately such mechanism is too expensive on the browser. We won't add such thing . The better way is to create a carddav plugin on the server to allow the retrieval of a set of vcard directly instead of a one by one. :/

I'll leave this pr opened for discussion but I am blocking this as this is not the optimal way to add this feature :)

@skjnldsv
Copy link
Member

If you want some example on how to do that, I added some infos on this comment: #39 (comment)

I will gladely help you if you want to create this plugin (check with @brjhaverkamp as well)
Thanks a lot for your attention on this issue! Tell me if you have any questions or need help (I'm on vacation though, so my answer delay can be a few days ;) )

@charismatic-claire
Copy link
Contributor Author

charismatic-claire commented Oct 16, 2018

Hi,
I was expecting that kind of review, that's why I asked. If I get it right you say that the way I made the "Download" symbol appear is alright. Also the assigned "downloadGroup(group)" is not a problem. But the way the actual result is constructed is not suitable.

We would need a "CardDAV" plugin handling the request, based on an URL as an argument, right? And this plugin would be a part of the "Contacts" project and the router of this project would trigger the execution of the plugin, resulting in the creation of the requested *.vcf file. Right?

That way, the result would be computed on the server side and not on the client side by the browser evaluating the JavaScript expressions. Do I get it? By the way: For the use case I have in mind, it is important that more than one address book can be used. Say five address books contain users from ten different groups and I want to export all members of a certain group, regardless of the address book they are stored in.

@skjnldsv
Copy link
Member

skjnldsv commented Oct 16, 2018

Hi,
I was expecting that kind of review, that's why I asked. If I get it right you say that the way I made the "Download" symbol appear is alright. Also the assigned "downloadGroup(group)" is not a problem. But the way the actual result is constructed is not suitable.

I did not looked as I'm on vacation (shame on me for checking github ;) ) But looking at the code it should look ok yes :)

We would need a "CardDAV" plugin handling the request, based on an URL as an argument, right? And this plugin would be a part of the "Calendar" project and the router of this project would trigger the execution of the plugin, resulting in the creation of the requested *.vcf file. Right?

Yes, basically (Not really sure) it would be a request with params (array of urls?, multidimensional array of addressbooks and vcards? pick one or find another alternative 😉 ) that is sent to the server. Not really related to the 'calendar' project like you said, but I'll assume you meant carddav/caldav/webdav? If so yes! :)

That way, the result would be computed on the server side and not on the client side by the browser evaluating the JavaScript expressions. Do I get it? By the way: For the use case I have in mind, it is important that more than one address book can be used. Say five address books contain users from ten different groups and I want to export all members of a certain group, regardless of the address book they are stored in.

Exactly that yes! The js computation will be too heavy. We need to delegate the work to the server for that (should be easy for it since this is basically a simple string concat) :)

@skjnldsv
Copy link
Member

If you need anything regarding the server part or the way our plugins work, please come on irc on #nextcloud-dev, and ask your question! It's nice seeing more people on board 🤗
You can also shoot me an email on [email protected] if you have any question! :)

@charismatic-claire
Copy link
Contributor Author

I wrote "Calendar" and meant "Contacts". I'm sorry for that. I just wanted to know if I need to touch another App for that. That's why I asked. Sounds like no, I don't.

Well I don't know when I will find the time to implement this. But at least I have a better picture of what needs to be done. Maybe that also helps @brjhaverkamp if he is also working on it...

@charismatic-claire
Copy link
Contributor Author

Alright. Now the *.vcf files to be exported get indeed created on the server side. Unfortunately I wasn't able to do it using a "CardDAV" plugin. Instead I added a new route, added a method to the PageController and created a GroupExportService to construct my result. This service is the worst part: I used simple database queries to fetch the data and combined it into a string...

I think I need to understand the "dav" project and especially the idea of the ImageExportPlugin to avoid this mess. Anyway. It works. If you got some advice for me, go ahead. Oh and please enjoy your vacation first. No need to hurry.

@skjnldsv
Copy link
Member

skjnldsv commented Oct 19, 2018

Alright. Now the *.vcf files to be exported get indeed created on the server side. Unfortunately I wasn't able to do it using a "CardDAV" plugin. Instead I added a new route, added a method to the PageController and created a GroupExportService to construct my result. This service is the worst part: I used simple database queries to fetch the data and combined it into a string...

This is looking close to what we need to do :)
Unfortunately (sorry 🙈), we really need to do that on https://github.com/nextcloud/server/ as a webdav plugin because this feature go further that being able to download contacts groups. Soon, we will drop the CATEGORIES support and start using vcard4 groups (#213), thus your implementation will no longer cover this feature. Implementing this as a carddav plugin will also allow us to add a checkbox support with selection and allow us to download any kind of contacts selection (groups, manual selection, search results...)
We also need to keep this app as simple as we can, therefore, all kind of php computation should be send off to server. So that it become globally available and not only when the contacts app is enabled :)

I think I need to understand the "dav" project and especially the idea of the ImageExportPlugin to avoid this mess. Anyway. It works. If you got some advice for me, go ahead. Oh and please enjoy your vacation first. No need to hurry.

No need to rush for this feature yes. If you're okay with that, I can open a pull request with you on /server and we can start working this feature a bit? So that we could implement it on the next releases? :)
Ideally this should be shipped with nextcloud 15! ;)

@charismatic-claire
Copy link
Contributor Author

Ok, so we got a little misunderstanding here. When I asked:

We would need a "CardDAV" plugin handling the request, based on an URL as an argument, right? And this plugin would be a part of the "Contacts" project and the router of this project would trigger the execution of the plugin, resulting in the creation of the requested *.vcf file. Right?

I specifically meant: Do I need to touch other apps than this one to achieve what I want? And I thought the answer was No, but the answer seems to be Yes. So... I think we should open a forum thread or something on this and discuss the requirements first. Only than it will be possible to implement exactly what we need, won't it?

Thanks for your quick reply again. I am learning a lot about Nextcloud here. Never contributed to it before, so... It's fun.

@skjnldsv
Copy link
Member

@charismatic-claire I'm so sorry for the misunderstood 😝
Well, we can discuss it on #39 if you want :)

@charismatic-claire
Copy link
Contributor Author

Did it.

@charismatic-claire
Copy link
Contributor Author

Alright. Let me know if this is doable. Promises still confuse me, but I think I got it (almost) right...

src/views/Contacts.vue Outdated Show resolved Hide resolved
Copy link
Member

@skjnldsv skjnldsv left a comment

Choose a reason for hiding this comment

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

Hello Claire! :)
Awesome work! Though a bit of changes are needed because you fetch data we already have.
If you don't have this already, I'll strongly suggest you install the vue devtools: https://github.com/vuejs/vue-devtools
You'll be able to see the store data and the current data/props of the views of the app (if you do not compile in production the app) :)

The changes requests are separated for readability, so it might looks a lot, but don't worry! If you have any other way to do something similarly, ignore my code comments! 😉

👐

lib/Controller/PageController.php Outdated Show resolved Hide resolved
package-lock.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/views/Contacts.vue Outdated Show resolved Hide resolved
src/views/Contacts.vue Outdated Show resolved Hide resolved
src/views/Contacts.vue Outdated Show resolved Hide resolved
src/views/Contacts.vue Outdated Show resolved Hide resolved
src/views/Contacts.vue Outdated Show resolved Hide resolved
@skjnldsv skjnldsv added enhancement New feature or request 2. developing Work in progress medium Medium priority labels Nov 15, 2018
@skjnldsv skjnldsv added this to the 3.1.0 milestone Nov 15, 2018
src/views/Contacts.vue Outdated Show resolved Hide resolved
@skjnldsv skjnldsv force-pushed the download-contact-group-#39 branch from 6b0a889 to 94f50c7 Compare November 16, 2018 09:19
…ownload-contact-group-nextcloud#39

Signed-off-by: John Molakvoæ (skjnldsv) <[email protected]>
@skjnldsv skjnldsv force-pushed the download-contact-group-#39 branch from 94f50c7 to 5847f27 Compare November 16, 2018 09:24
@skjnldsv
Copy link
Member

Works really nicely!!!
Awesome work @charismatic-claire 👍

Thanks a lot!

@skjnldsv skjnldsv added 3. to review Waiting for reviews and removed 2. developing Work in progress labels Nov 16, 2018
@skjnldsv skjnldsv requested a review from georgehrke November 16, 2018 09:24
src/views/Contacts.vue Outdated Show resolved Hide resolved
src/views/Contacts.vue Outdated Show resolved Hide resolved
src/views/Contacts.vue Outdated Show resolved Hide resolved
@georgehrke
Copy link
Member

Works very well, will give approval when the code remarks above have been addressed! :)

@skjnldsv
Copy link
Member

@charismatic-claire Could you also add your copyright in the headers of the files you edited please? :)

@charismatic-claire
Copy link
Contributor Author

@skjnldsv Since I only changed Conacts.vue in the end, there isn't much to do, right? Do you mean I should add something similar to your @copyright Copyright (c) 2018 John Molakvoæ ... in the header of this file? And should I also add something similar to your @author John Molakvoæ ...?

@skjnldsv
Copy link
Member

Leave the copyright, since I created this file originally. But add a new @author line with whatever entity you are representing for this pull request :)

Like the team popcorn did here:

😉

@skjnldsv skjnldsv merged commit 6b6bfce into nextcloud:master Nov 20, 2018
@skjnldsv
Copy link
Member

skjnldsv commented Dec 4, 2018

@charismatic-claire By the way! We're loving new contributors like you! We're meeting for new releases of nextcloud with a lot of contributors and we help a lot for the travel expenses! The next planned one is the week of the 11/03 in Stuttgart. We'd love to have you there! 😉
Also, I don't know what you do in life, if this is a Hobby or a work, but we have a new program specifically for supporting people of underrepresented groups in open source: https://nextcloud.com/include – you can apply for travel support for an event, mentorship or a lot of different ways we can help! 👍

Feel free to reach us! 🤗
Cheers, John.

@charismatic-claire
Copy link
Contributor Author

Hi John,

sorry, it has been a long time since I checked this thread. Well, thank you! I'm kind of proud that you said that and invited me to your next meeting.

Also, I don't know what you do in life, if this is a Hobby or a work, but we have a new program specifically for supporting people of underrepresented groups in open source: https://nextcloud.com/include – you can apply for travel support for an event, mentorship or a lot of different ways we can help!

Yeah about that: Actually it is my job. I'm a programmer. I wanted to learn JavaScript and Vue.js and a friend of mine needed this feature so I decided to just do it. Actually, I really want to learn more JavaScript and more Vue.js in the future, so I might stay engaged with Nextcloud.

Thanks again for your kind way of supporting me with my first real contribution to an open source project!

@skjnldsv
Copy link
Member

skjnldsv commented Apr 4, 2019

@charismatic-claire awesome to hear! 🎉
We love people like you who believe in open source projects and contributing not only to learn new things but because it will benefit for others as well! You missed the previous contributor week, but hold tight! We have a new one incoming! 😉
24/06/2019 -> 28/06/2019

We also have the annual nextcloud conference https://nextcloud.com/conf/
Which you're of course welcome to be part of! 👍

@skjnldsv skjnldsv modified the milestones: next minor, 3.1.0, 3.0.2 Apr 8, 2019
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 medium Medium priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants