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

ownCloud users are exported as address book #9641

Merged
merged 1 commit into from
Jul 28, 2014

Conversation

DeepDiver1975
Copy link
Member

alternative approach for #8951

@LEDfan

@LEDfan
Copy link
Member

LEDfan commented Jul 15, 2014

Nice! Works as expected. The difference with #8951 is only that these contacts aren't accessible via the contacts app. Which isn't needed since they can't be edited.

@DeepDiver1975
Copy link
Member Author

The difference with #8951 is only that these contacts aren't accessible via the contacts app. Which isn't needed since they can't be edited.

They should as long as the contacts app is using the contacts manager to access all addressbook

@LEDfan
Copy link
Member

LEDfan commented Jul 15, 2014

Indeed, but this isn't the case. The contacts app runs the getContacts and getAddressbook and getContact methods on the classes defined here: ttps://github.com/owncloud/contacts/blob/master/lib/app.php#L57-L60

@DeepDiver1975
Copy link
Member Author

hehe - this actually explains why there was that much code in your PR 😉

@LEDfan
Copy link
Member

LEDfan commented Jul 15, 2014

Indeed. But I like this :)
Tested 👍

@DeepDiver1975
Copy link
Member Author

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6241/

@DeepDiver1975
Copy link
Member Author

I will add some unit tests the next days

@jancborchardt
Copy link
Member

So after some small changes in the Contacts app, this will work for the Contacts app too?

@jancborchardt
Copy link
Member

cc @enoch85 here as he was active in the preceding pull request: #8951

@LEDfan
Copy link
Member

LEDfan commented Jul 17, 2014

@jancborchardt this PR #9641 won't load the contacts in the Contacts app. I don't think it's a small adjustment in the Contacts app since it's the core function and design of the Contacts app.

@jancborchardt
Copy link
Member

Ok so the goal of this localUsers backend is to have an easy way to expose the local users to apps like Contacts and Chat. Is this achieved by this PR? Or what does this do? :)

@LEDfan
Copy link
Member

LEDfan commented Jul 17, 2014

@jancborchardt this PR is an alternative for #8951.
The Contacts can be fetched by every app on the same way as in #8951.(i.e. via the ContactsManager) The contacts app fetches the contacts by calling the getContacts() method on a Contacts backend. The thing is this PR doesn't create a Contacts backend thus the Contacts app never fetches these contacts. However #8951 creates a Contacts backend, so the contacts are fetched by the contacts app.

TL;DR this PR creates the contacts, but the Contacts app doesn't fetch them. (but the Chat app does) And #8951 creates the contacts and the Contacts is able to fetch them.

@LEDfan
Copy link
Member

LEDfan commented Jul 18, 2014

@jancborchardt which PR should be used then?

@enoch85
Copy link
Member

enoch85 commented Jul 18, 2014

👍 Nice!


/**
* @param array $properties this array if key-value-pairs defines a contact
* @return array an array representing the contact just created or updated
Copy link
Member

Choose a reason for hiding this comment

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

This returns only false.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should return an empty array to match the interface

@scrutinizer-notifier
Copy link

The inspection completed: 10 new issues, 29 updated code elements

@ghost
Copy link

ghost commented Jul 24, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6395/

@LEDfan
Copy link
Member

LEDfan commented Jul 28, 2014

Can this be merged? @DeepDiver1975 Can this be backported to 7.0.1? This way I can release a beta for the Chat app without the need for people to use the git version.

@jancborchardt
Copy link
Member

Yep @LEDfan @LukasReschke @DeepDiver1975 it would be really good if we can have this merged and in 7.0.1.

DeepDiver1975 added a commit that referenced this pull request Jul 28, 2014
ownCloud users are exported as address book
@DeepDiver1975 DeepDiver1975 merged commit a3411e3 into master Jul 28, 2014
@DeepDiver1975 DeepDiver1975 deleted the localuser-addressbook branch July 28, 2014 10:18
@DeepDiver1975
Copy link
Member Author

it would be really good if we can have this merged and in 7.0.1.

@karlitschek has to decide on this
Personal vote: go for it

@karlitschek
Copy link
Contributor

Ok. Please backport

@DeepDiver1975
Copy link
Member Author

will do later ...

@DeepDiver1975
Copy link
Member Author

stable7: 02a61c0

@DeepDiver1975
Copy link
Member Author

@LEDfan @jancborchardt go go go! 😉

@LEDfan
Copy link
Member

LEDfan commented Jul 28, 2014

Thanks @DeepDiver1975 !

@jancborchardt
Copy link
Member

Awesome!

@lock lock bot locked as resolved and limited conversation to collaborators Aug 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants