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

Adding dav resource for avatars #26872

Merged
merged 4 commits into from Mar 23, 2017

Conversation

Projects
None yet
9 participants
@DeepDiver1975
Member

DeepDiver1975 commented Dec 23, 2016

Description

There is one root resource called /avatars/ - within this collection there is a list of all users (only enumerable in debug mode as usual).

Within this user node avatars can be loaded following the pattern
$size.$ext

where size is less then 1024 and ext is wither jppg or png.

A default child named 96.jpeg is listed.

To Decide:

  • allow any size or support only some like 32, 64, 96, 128, ...
    • any
    • list

ToDo:

  • unit tests
  • integration tests
  • think about unauthenticated requests - interesting for public share links

Motivation and Context

@dragotin wants to have some fun with the desktop client during the holidays and I happily support him

How Has This Been Tested?

Browser with sabre's browser plugin - see screeshot

Screenshots (if appropriate):

bildschirmfoto von 2016-12-23 11-54-16

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@DeepDiver1975 DeepDiver1975 added this to the 10.0 milestone Dec 23, 2016

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Dec 23, 2016

@DeepDiver1975, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bartv2, @PVince81, @tanghus and @DeepDiver1975 to be potential reviewers.

@DeepDiver1975, thanks for your PR! By analyzing the history of the files in this pull request, we identified @bartv2, @PVince81, @tanghus and @DeepDiver1975 to be potential reviewers.

@DeepDiver1975 DeepDiver1975 requested a review from dragotin Dec 23, 2016

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Dec 23, 2016

Member

Link to the spec for easier overview ?

Member

PVince81 commented Dec 23, 2016

Link to the spec for easier overview ?

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG Dec 23, 2016

Member

This will be useful for mobile clients as well.

Member

SergioBertolinSG commented Dec 23, 2016

This will be useful for mobile clients as well.

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG Dec 23, 2016

Member

Using cadaver I can download the file correctly, but when listing collection there is an error:

dav:/remote.php/dav/avatars/tomas/> ls
Listing collection `/remote.php/dav/avatars/tomas/': succeeded.
Error: 96.jpeg 404 Not Found

Member

SergioBertolinSG commented Dec 23, 2016

Using cadaver I can download the file correctly, but when listing collection there is an error:

dav:/remote.php/dav/avatars/tomas/> ls
Listing collection `/remote.php/dav/avatars/tomas/': succeeded.
Error: 96.jpeg 404 Not Found

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Dec 23, 2016

Member

Using cadaver I can download the file correctly, but when listing collection there is an error:

indeed - I'm having a look

Member

DeepDiver1975 commented Dec 23, 2016

Using cadaver I can download the file correctly, but when listing collection there is an error:

indeed - I'm having a look

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Dec 23, 2016

Member

Using cadaver I can download the file correctly, but when listing collection there is an error:

fixed

Member

DeepDiver1975 commented Dec 23, 2016

Using cadaver I can download the file correctly, but when listing collection there is an error:

fixed

@davivel

This comment has been minimized.

Show comment
Hide comment
@davivel

davivel Dec 23, 2016

Member

A wonderful 'if (server.version >= ... )' is coming to Android...

Member

davivel commented Dec 23, 2016

A wonderful 'if (server.version >= ... )' is coming to Android...

@SergioBertolinSG

This comment has been minimized.

Show comment
Hide comment
@SergioBertolinSG

SergioBertolinSG Dec 23, 2016

Member

Works 👍

Member

SergioBertolinSG commented Dec 23, 2016

Works 👍

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Dec 23, 2016

Member

A wonderful 'if (server.version >= ... )' is coming to Android...

I would not do this based on the server version - with caldav and carddav a user's calendar/addressbook home is detected via a property on the current principal.

I did the same here

$ curl --request PROPFIND  --user admin:admin --header "Content-Type: text/xml"  --data "<D:propfind xmlns:D='DAV:' xmlns:oc='http://owncloud.org/ns'><D:prop><D:displayname/><oc:avatar-home/></D:prop></D:propfind>" http://localhost:8080/remote.php/dav/principals/users/admin
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:cal="urn:ietf:params:xml:ns:caldav" xmlns:cs="http://calendarserver.org/ns/" xmlns:card="urn:ietf:params:xml:ns:carddav" xmlns:oc="http://owncloud.org/ns">
 <d:response>
  <d:href>/remote.php/dav/principals/users/admin/</d:href>
  <d:propstat>
   <d:prop>
    <d:displayname>Mr. Admin</d:displayname>
    <oc:avatar-home>
     <d:href>/remote.php/dav/avatars/admin</d:href>
    </oc:avatar-home>
   </d:prop>
   <d:status>HTTP/1.1 200 OK</d:status>
  </d:propstat>
 </d:response>
</d:multistatus>
Member

DeepDiver1975 commented Dec 23, 2016

A wonderful 'if (server.version >= ... )' is coming to Android...

I would not do this based on the server version - with caldav and carddav a user's calendar/addressbook home is detected via a property on the current principal.

I did the same here

$ curl --request PROPFIND  --user admin:admin --header "Content-Type: text/xml"  --data "<D:propfind xmlns:D='DAV:' xmlns:oc='http://owncloud.org/ns'><D:prop><D:displayname/><oc:avatar-home/></D:prop></D:propfind>" http://localhost:8080/remote.php/dav/principals/users/admin
<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:cal="urn:ietf:params:xml:ns:caldav" xmlns:cs="http://calendarserver.org/ns/" xmlns:card="urn:ietf:params:xml:ns:carddav" xmlns:oc="http://owncloud.org/ns">
 <d:response>
  <d:href>/remote.php/dav/principals/users/admin/</d:href>
  <d:propstat>
   <d:prop>
    <d:displayname>Mr. Admin</d:displayname>
    <oc:avatar-home>
     <d:href>/remote.php/dav/avatars/admin</d:href>
    </oc:avatar-home>
   </d:prop>
   <d:status>HTTP/1.1 200 OK</d:status>
  </d:propstat>
 </d:response>
</d:multistatus>
@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Dec 23, 2016

Member

.... or we add a capability

@dragotin

Member

DeepDiver1975 commented Dec 23, 2016

.... or we add a capability

@dragotin

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
Member

DeepDiver1975 commented Jan 16, 2017

ping @dragotin

@dragotin

This comment has been minimized.

Show comment
Hide comment
@dragotin

dragotin Jan 21, 2017

Contributor

Just out of curiosity: If I do a request like

curl --request PROPFIND  --user kf:xxxxx --header "Content-Type: text/xml"  --data "<D:propfind xmlns:D='DAV:' xmlns:oc='http://owncloud.org/ns'><D:allprop/></D:propfind>" http://localhost/ocm/remote.php/dav/principals/users/kf

to get all the available properties of the resource, the result is

<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:cal="urn:ietf:params:xml:ns:caldav" xmlns:cs="http://calendarserver.org/ns/" xmlns:card="urn:ietf:params:xml:ns:carddav" xmlns:oc="http://owncloud.org/ns">
 <d:response>
  <d:href>/ocm/remote.php/dav/principals/users/kf/</d:href>
  <d:propstat>
   <d:prop>
    <d:resourcetype>
     <d:collection/>
     <d:principal/>
    </d:resourcetype>
   </d:prop>
   <d:status>HTTP/1.1 200 OK</d:status>
  </d:propstat>
 </d:response>
</d:multistatus>

Is that correct? http://www.restpatterns.org/HTTP_Methods/PROPFIND describes different.

@DeepDiver1975 I am trying to find out which other props I can query from the principal.

Contributor

dragotin commented Jan 21, 2017

Just out of curiosity: If I do a request like

curl --request PROPFIND  --user kf:xxxxx --header "Content-Type: text/xml"  --data "<D:propfind xmlns:D='DAV:' xmlns:oc='http://owncloud.org/ns'><D:allprop/></D:propfind>" http://localhost/ocm/remote.php/dav/principals/users/kf

to get all the available properties of the resource, the result is

<?xml version="1.0"?>
<d:multistatus xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns" xmlns:cal="urn:ietf:params:xml:ns:caldav" xmlns:cs="http://calendarserver.org/ns/" xmlns:card="urn:ietf:params:xml:ns:carddav" xmlns:oc="http://owncloud.org/ns">
 <d:response>
  <d:href>/ocm/remote.php/dav/principals/users/kf/</d:href>
  <d:propstat>
   <d:prop>
    <d:resourcetype>
     <d:collection/>
     <d:principal/>
    </d:resourcetype>
   </d:prop>
   <d:status>HTTP/1.1 200 OK</d:status>
  </d:propstat>
 </d:response>
</d:multistatus>

Is that correct? http://www.restpatterns.org/HTTP_Methods/PROPFIND describes different.

@DeepDiver1975 I am trying to find out which other props I can query from the principal.

dragotin added a commit to owncloud/client that referenced this pull request Jan 22, 2017

SettingsDialog: Display the user avatar as action icon if available.
The avatar image is fetched from the server async, thus connect a signal
from the account if the avatar changes.

Server feature owncloud/core#26872 is needed.
@dragotin

Works for me to implement owncloud/client#5482 yet I can not comment too much on the php code :-)

Thanks!

@dragotin

This comment has been minimized.

Show comment
Hide comment
@dragotin

dragotin Jan 24, 2017

Contributor

@DeepDiver1975 btw - I would appreciate if a client would not have to query the path to the avatar from the principals properties but if it just could always get principals/users/<username>/<size>.png and that either returns 200 with content or not.

The reason is if we use that API also to get avatars for the users in the activity stream, we would have to query first for the path, and than for the image itself, basically doubling the amount of queries... (even if the client caches the avatars for the users it saw...)

Contributor

dragotin commented Jan 24, 2017

@DeepDiver1975 btw - I would appreciate if a client would not have to query the path to the avatar from the principals properties but if it just could always get principals/users/<username>/<size>.png and that either returns 200 with content or not.

The reason is if we use that API also to get avatars for the users in the activity stream, we would have to query first for the path, and than for the image itself, basically doubling the amount of queries... (even if the client caches the avatars for the users it saw...)

@ogoffart

This comment has been minimized.

Show comment
Hide comment
@ogoffart

ogoffart Jan 26, 2017

A capability would be nice.

A capability would be nice.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jan 26, 2017

Member

A capability would be nice.

for?

Member

DeepDiver1975 commented Jan 26, 2017

A capability would be nice.

for?

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jan 26, 2017

Member

@DeepDiver1975 btw - I would appreciate if a client would not have to query the path to the avatar from the principals properties but if it just could always get principals/users//.png and that either returns 200 with content or not.

querying the prop only works for the own user - one has no access to the principal of other users as far as I have been retesting this .....

Well - just use the url as is.

Member

DeepDiver1975 commented Jan 26, 2017

@DeepDiver1975 btw - I would appreciate if a client would not have to query the path to the avatar from the principals properties but if it just could always get principals/users//.png and that either returns 200 with content or not.

querying the prop only works for the own user - one has no access to the principal of other users as far as I have been retesting this .....

Well - just use the url as is.

@dragotin

This comment has been minimized.

Show comment
Hide comment
@dragotin

dragotin Jan 26, 2017

Contributor

Yes, ok. So if we want to extend usage of the avatar image it is important that

  • the avatar url stays stable
  • and that user a can read the avatar picture of user b

About the capability: In general it would be enough to know if the avatar image can be queried yes/no. However since there will be changes in time, maybe a capability that tells the 'capability api version' would be best.

Contributor

dragotin commented Jan 26, 2017

Yes, ok. So if we want to extend usage of the avatar image it is important that

  • the avatar url stays stable
  • and that user a can read the avatar picture of user b

About the capability: In general it would be enough to know if the avatar image can be queried yes/no. However since there will be changes in time, maybe a capability that tells the 'capability api version' would be best.

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Jan 26, 2017

Member

the avatar url stays stable

yes

and that user a can read the avatar picture of user b

already possible

Member

DeepDiver1975 commented Jan 26, 2017

the avatar url stays stable

yes

and that user a can read the avatar picture of user b

already possible

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Mar 15, 2017

Contributor

Client status: We have the PR for client but we'd like for the server to have this merged first
owncloud/client#5482

Contributor

guruz commented Mar 15, 2017

Client status: We have the PR for client but we'd like for the server to have this merged first
owncloud/client#5482

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 15, 2017

Member

Client status: We have the PR for client but we'd like for the server to have this merged first

what us missing from your point of view - do we need a capability?
I for sure will remove the dav property - it only works for the currently logged in user - pointless

Member

DeepDiver1975 commented Mar 15, 2017

Client status: We have the PR for client but we'd like for the server to have this merged first

what us missing from your point of view - do we need a capability?
I for sure will remove the dav property - it only works for the currently logged in user - pointless

@guruz

This comment has been minimized.

Show comment
Hide comment
@guruz

guruz Mar 16, 2017

Contributor

do we need a capability?

The current code works by just guessing the URL: https://github.com/owncloud/client/pull/5482/files#diff-4301a6735ca9b7817b27e8f2f1bd5615R596

From the client developer perspective this is completely fine, but you guys are usually not happy about too many requests on the server so if you want to avoid that then better supply a capability?
(although 1 request when connecting is not so bad)

@ogoffart Feel free to override my opinion

Contributor

guruz commented Mar 16, 2017

do we need a capability?

The current code works by just guessing the URL: https://github.com/owncloud/client/pull/5482/files#diff-4301a6735ca9b7817b27e8f2f1bd5615R596

From the client developer perspective this is completely fine, but you guys are usually not happy about too many requests on the server so if you want to avoid that then better supply a capability?
(although 1 request when connecting is not so bad)

@ogoffart Feel free to override my opinion

function getChildren() {
try {
return [
$this->getChild('96.jpeg')

This comment has been minimized.

@PVince81

PVince81 Mar 22, 2017

Member

hardcoded just like that ? "jpeg" or "jpg" ?

@PVince81

PVince81 Mar 22, 2017

Member

hardcoded just like that ? "jpeg" or "jpg" ?

This comment has been minimized.

@DeepDiver1975

DeepDiver1975 Mar 22, 2017

Member

we support jpeg and png - we have to choose on extension

@DeepDiver1975

DeepDiver1975 Mar 22, 2017

Member

we support jpeg and png - we have to choose on extension

This comment has been minimized.

@PVince81

PVince81 Mar 22, 2017

Member

"jpg" then ? 😉

Are real avatars really stored as "jpeg" and not "jpg" ?

@PVince81

PVince81 Mar 22, 2017

Member

"jpg" then ? 😉

Are real avatars really stored as "jpeg" and not "jpg" ?

This comment has been minimized.

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 22, 2017

Member

Fine then, "jpeg" it is.

What else is missing ? Capability ? Integration tests ? (integration tests could be added later)

Member

PVince81 commented Mar 22, 2017

Fine then, "jpeg" it is.

What else is missing ? Capability ? Integration tests ? (integration tests could be added later)

@DeepDiver1975

What else is missing ? Capability ? Integration tests ? (integration tests could be added later)

fixing ui tests .... no capabilities required according to the client team

function getChildren() {
try {
return [
$this->getChild('96.jpeg')
@individual-it

This comment has been minimized.

Show comment
Hide comment
@individual-it

individual-it Mar 23, 2017

Member

please get commit e5d1c36 into this branch to make the UI tests pass
I've branched off and cherry picked it, to make the test pass: https://travis-ci.org/owncloud/core/jobs/214116816

Member

individual-it commented Mar 23, 2017

please get commit e5d1c36 into this branch to make the UI tests pass
I've branched off and cherry picked it, to make the test pass: https://travis-ci.org/owncloud/core/jobs/214116816

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 23, 2017

Member

please get commit e5d1c36 into this branch to make the UI tests pass

I rebased this branch and the commit is in here - but I still observe the error ... strange ...

Member

DeepDiver1975 commented Mar 23, 2017

please get commit e5d1c36 into this branch to make the UI tests pass

I rebased this branch and the commit is in here - but I still observe the error ... strange ...

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 23, 2017

Member

what - now green - are you kidding me?

@individual-it THX

Member

DeepDiver1975 commented Mar 23, 2017

what - now green - are you kidding me?

@individual-it THX

@DeepDiver1975

This comment has been minimized.

Show comment
Hide comment
@DeepDiver1975

DeepDiver1975 Mar 23, 2017

Member

@PVince81 objections in merging?

Member

DeepDiver1975 commented Mar 23, 2017

@PVince81 objections in merging?

@PVince81 PVince81 merged commit 7115494 into master Mar 23, 2017

3 of 4 checks passed

continuous-integration/jenkins/pr-head This commit is being built
Details
Scrutinizer 1 new issues, 22 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details

@PVince81 PVince81 deleted the dav-avatars branch Mar 23, 2017

@PVince81

This comment has been minimized.

Show comment
Hide comment
@PVince81

PVince81 Mar 23, 2017

Member

Please raise doc ticket with instructions how to use the endpoint.

And add this to the wiki page about new features

Member

PVince81 commented Mar 23, 2017

Please raise doc ticket with instructions how to use the endpoint.

And add this to the wiki page about new features

@guruz

This comment has been minimized.

Show comment
Hide comment
Contributor

guruz commented Apr 3, 2017

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