Avatars in share dialog #13866

rullzer · 2015-02-03T18:31:30Z

As mentioned in #5506 and better illustrated in #3224 this PR add user avatars to the share dialog. Not int the auto complete list but of the users we have shared with. To add it to the autocompletelist probably requires some rework.

If a user does not have an avatar the placeholder image is shown so that formatting stays consistent.

schiessle · 2015-02-06T15:19:30Z

Looks great. Let's merge this after the 8.0 release.

I wonder why you also added a avatar for the group shares. Up to now you can't set a avatar for a group.

If you share a file from user1 to user2 and then open the share drop-down for user2 on the shared file you see something like "Shared with you by user user1", would be nice to have the avatar there too.

rullzer · 2015-02-06T15:22:03Z

@schiesbn I added the avatar also for the groups to make sure that the layout stayed the same. It can be trivially removed again of course. But in my opinion it looks better if they all indent the same amount.

I'll look into the extra avatar. Should not be to hard.

schiessle · 2015-02-06T15:47:10Z

@rullzer makes sense, but then we also need a avatar for remote shares (aka server-to-server shares). With ownCloud 8 you will be able to share a file/folder to a user on another ownCloud server by adding something like <user>@<owncloud_server_url> to the share dialog. This share will be listed along the normal user and group shares.

Or you add a just some additional spacing for group and remote shares. I leave this decision to you, I think both is acceptable. Maybe @jancborchardt also has a opinion on this 😉

rullzer · 2015-02-06T15:50:12Z

@schiesbn ah true I did not really play with remote shares that much. But indeed then that is required there as well. Shich would be rather easy to add. Lets see what @jancborchardt has to say. And then I can add it.

Of course the remote avatar would then just be the same as for the groups. Unless we have a fancy way to retrieve remote avatars.

schiessle · 2015-02-06T15:51:32Z

Of course the remote avatar would then just be the same as for the groups. Unless we have a fancy way to retrieve remote avatars.

I think this would be OK as a first step. Of course it would be really cool if we could also get the avatar from the remote server.

rullzer · 2015-02-06T16:08:28Z

We now also have remote avatars. Altough I could not fully test them since I do not have 2 ownlcoud 8 setups (yet).

schiessle · 2015-02-06T16:12:59Z

We now also have remote avatars. Altough I could not fully test them since I do not have 2 ownlcoud 8 setups (yet).

Great! You should also be able to create a remote share from user1 to user2 with both on the same server. It doesn't make sense but it is useful for testing purpose.

schiessle · 2015-02-09T12:11:22Z

If I have a server-to-server share in the list I get following syntax error in the JavaScript console (remote user us "schiesbn@vm/server1"):

Error: Syntax error, unrecognized expression: #avatar-remote-schiesbn-vm/server1

rullzer · 2015-02-09T12:55:18Z

Aah mmm seems more filtering is needed. I'll have a look at it.

jancborchardt · 2015-02-09T13:16:37Z

@rullzer nice! Just some remarks:

The avatars are quite small. As the row needs to have a certain height anyway, you can increase the size to 32*32px
Groups shouldn’t have an avatar, they don’t have that anywhere else either
But the names of groups and remote shares need to be indented as you mention above so that the text is properly aligned. So when there’s no avatar, shift them to the right via CSS.

Thanks! :)

rullzer · 2015-02-10T09:49:53Z

@jancborchardt

I increased the avatar size. Looks better indeed.
Removed group avatars
Indented group shares

Currently we still have the placeholder for remote shares. Should that stay or go?

jancborchardt · 2015-02-10T13:33:48Z

If the placeholder is the actual avatar of the user, then yes. If the remote user has an actual avatar, we should show that.

If showing the actual avatar of the remote user is not possible, then we should not show a placeholder on one end.

Thanks! :) (Also, please squash the commits into one again so it’s nicer after we merge it.)

rullzer · 2015-02-10T17:22:44Z

@jancborchardt done.

It is currently not possible to get remote avatars. So that will be something for a future PR.

jancborchardt · 2015-02-10T17:28:27Z

Looks great! 👍 @schiesbn can you also review it again?

@rullzer can you check out the errors from Jenkins and Scrutinizer?

rullzer · 2015-02-11T20:26:17Z

So the problem with he build-bot are the unit tests. For some reason they fail... I have no clue why since as far as I know I do not touch the code responsible for the unit tests. I'll look into it in more depth soon.

schiessle · 2015-02-12T10:07:50Z

Tested, looks great!

The failing tests are a problem wit our build-bot. I created a new dummy-pr to get a final test. Once it passes I will merge your PR.

rullzer · 2015-02-12T10:14:52Z

@schiesbn well the unit test do also not pass locally. So I did break something.

@PVince81 as requested

PhantomJS 1.9.8 (Linux) OC.Share tests dropdown "sharesChanged" event triggers "sharesChanged" event when adding shares FAILED
        Expected false to equal true.
            at /home/roeland/owncloud/core/core/js/tests/specs/shareSpec.js:455
        TypeError: 'null' is not an object (evaluating 'handler.getCall(0).args')
            at /home/roeland/owncloud/core/core/js/tests/specs/shareSpec.js:456
PhantomJS 1.9.8 (Linux) OC.Share tests dropdown "sharesChanged" event triggers "sharesChanged" event when deleting shares FAILED
        Expected false to equal true.
            at /home/roeland/owncloud/core/core/js/tests/specs/shareSpec.js:469
        TypeError: 'null' is not an object (evaluating 'handler.getCall(0).args')
            at /home/roeland/owncloud/core/core/js/tests/specs/shareSpec.js:470
PhantomJS 1.9.8 (Linux) OC.Share tests dropdown "sharesChanged" event triggers "sharesChanged" event when toggling link share FAILED
        Expected false to equal true.
            at /home/roeland/owncloud/core/core/js/tests/specs/shareSpec.js:483
        TypeError: 'null' is not an object (evaluating 'handler.getCall(0).args')
            at /home/roeland/owncloud/core/core/js/tests/specs/shareSpec.js:484

PVince81 · 2015-02-12T10:21:10Z

core/js/share.js

@@ -681,6 +692,9 @@ OC.Share={
 			html += '</div>';
 			html += '</li>';
 			html = $(html).appendTo('#shareWithList');
+			if (shareType == OC.Share.SHARE_TYPE_USER) {


use triple equals ===

PVince81 · 2015-02-12T14:12:31Z

Please do the following:

stub the function $.fn.avatar in beforeEach in shareSpec.js
add a new unit test to check if the avatar is loaded
- check if the avatar stub was called with the right arguments
- check the DOM to see if the avatar element is there

Still not correct but it is a small step

rullzer · 2015-02-12T15:53:09Z

So the unit tests pass again!
Please review.

PVince81 · 2015-02-12T15:56:19Z

core/js/tests/specs/shareSpec.js

+				expect(avatarStub.calledOnce).toEqual(true);
+				var args = avatarStub.getCall(0).args;
+
+				expect($('#avatar-user1')[0]).toEqual(jasmine.anything());


I usually do expect($('#avatar-user1').length).toEqual(1));

Ah that might look cleaner indeed

PVince81 · 2015-02-12T15:56:38Z

The code looks good, haven't tested yet.

ghost · 2015-02-12T16:09:53Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9339/

Build result: FAILURE

[...truncated 14 lines...] > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config --add remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git2703028884527562311.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse refs/remotes/origin/pr/13866/merge^{commit} # timeout=10 > git rev-parse refs/remotes/origin/origin/pr/13866/merge^{commit} # timeout=10Checking out Revision 9da1be0b58085f4a766226f38155c7fb1c1e0a3e (refs/remotes/origin/pr/13866/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f 9da1be0b58085f4a766226f38155c7fb1c1e0a3e > git rev-list 8da12e0f9ed71ad6ad9b15060f849e3179231733 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILURESetting status of ecb7d44 to FAILURE with url https://ci.owncloud.org/job/pull-request-analyser-ng-simple/9339/ and message: Merged build finished.
Test FAILed.

PVince81 · 2015-02-13T09:53:55Z

@rullzer I just remembered that there is an option in config.php to disable avatars.

Can you add this to your code as well ?
Basically: don't render the avatars if "enable_avatars" is false.
For the unit test you can simulate the config option, see the top of "shareSpec.js", it uses "oc_appconfig.core.someOption = someTestValue"

rullzer · 2015-02-13T11:58:18Z

@PVince81 I added the checks and extended the unit tests.

ghost · 2015-02-13T11:59:38Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/9390/

Build result: FAILURE

[...truncated 14 lines...] > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10 > git config --add remote.origin.fetch +refs/heads/:refs/remotes/origin/ # timeout=10 > git config remote.origin.url https://github.com/owncloud/core.git # timeout=10Fetching upstream changes from https://github.com/owncloud/core.gitusing GIT_SSH to set credentials using .gitcredentials to set credentials > git config --local credential.helper store --file=/tmp/git6036363490301203250.credentials # timeout=10 > git -c core.askpass=true fetch --tags --progress https://github.com/owncloud/core.git +refs/pull/:refs/remotes/origin/pr/ > git config --local --remove-section credential # timeout=10 > git rev-parse refs/remotes/origin/pr/13866/merge^{commit} # timeout=10 > git rev-parse refs/remotes/origin/origin/pr/13866/merge^{commit} # timeout=10Checking out Revision dccf9b71f33b336b55aa9c1580252dceb7fbd2bc (refs/remotes/origin/pr/13866/merge) > git config core.sparsecheckout # timeout=10 > git checkout -f dccf9b71f33b336b55aa9c1580252dceb7fbd2bc > git rev-list 8da12e0f9ed71ad6ad9b15060f849e3179231733 # timeout=10First time build. Skipping changelog. > git remote # timeout=10 > git submodule init # timeout=10 > git submodule sync # timeout=10 > git config --get remote.origin.url # timeout=10 > git submodule update --init --recursiveTriggering pull-request-analyser-ng-simple » SLAVEpull-request-analyser-ng-simple » SLAVE completed with result FAILURESetting status of 9a6da8e to FAILURE with url https://ci.owncloud.org/job/pull-request-analyser-ng-simple/9390/ and message: Merged build finished.
Test FAILed.

scrutinizer-notifier · 2015-02-13T12:35:44Z

The inspection completed: 8 new issues

schiessle · 2015-02-13T17:37:28Z

Tests successfully passed here: #14144

👍 from my side.

jancborchardt · 2015-02-16T10:16:01Z

Ok, so if the commits are squashed one more time into one this looks good to go! :)

PVince81 · 2015-02-16T10:17:45Z

I do wonder whether this needs more vertical spacing:

@jancborchardt what do you think ?

@rullzer sorry for delaying the merge of this nice PR a bit more 😄

rullzer · 2015-02-16T20:09:47Z

@PVince81 Hehe no problem. We have some time before 8.1 right :)

Anyway. I'm fine either way. Adding a bit more spacing is not that hard.
However I do not think extra vertical spacing is going to make it look nicer. It is probably better If all he permissions start at the same indentation. I'll try to get screen shots of both. But it will probably be later this week.

PVince81 · 2015-02-17T09:17:34Z

Anyway, I don't want to block this needlessly. The spacing wasn't that nice even before the avatars.

👍

Avatars in share dialog

jancborchardt · 2015-02-17T10:50:37Z

Yeah, it’s a different issue. It doesn’t need vertical spacing, it needs a proper dropdown for the »create, edit, delete« actions ;)

Great job @rullzer! :)

DeepDiver1975 added this to the 8.1-next milestone Feb 6, 2015

DeepDiver1975 added the enhancement - proposed label Feb 6, 2015

schiessle added the feature:sharing label Feb 9, 2015

jancborchardt added the design label Feb 10, 2015

schiessle mentioned this pull request Feb 12, 2015

[ignore] add avatars to share dialog #14144

Merged

PVince81 reviewed Feb 12, 2015
View reviewed changes

rullzer added 3 commits February 12, 2015 16:50

Display user avatar in share dialog

639a634

Added dependancy for the JS unit tests

47b96c9

Still not correct but it is a small step

Added unit test

e952687

rullzer added the 3 - To Review label Feb 12, 2015

PVince81 reviewed Feb 12, 2015
View reviewed changes

Small unit test rewrite

ecb7d44

rullzer added 2 commits February 13, 2015 12:17

Added config check

90218dc

Extended avatar unit tests

9a6da8e

PVince81 pushed a commit that referenced this pull request Feb 17, 2015

Merge pull request #13866 from rullzer/avatar_share_dialog

e8f16db

Avatars in share dialog

PVince81 merged commit e8f16db into owncloud:master Feb 17, 2015

PVince81 mentioned this pull request Feb 26, 2015

Avatars in sharing dropdown #5506

Closed

rullzer deleted the avatar_share_dialog branch May 22, 2015 11:50

lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019

Avatars in share dialog #13866

Avatars in share dialog #13866

Uh oh!

Conversation

rullzer commented Feb 3, 2015

Uh oh!

schiessle commented Feb 6, 2015

Uh oh!

rullzer commented Feb 6, 2015

Uh oh!

schiessle commented Feb 6, 2015

Uh oh!

rullzer commented Feb 6, 2015

Uh oh!

schiessle commented Feb 6, 2015

Uh oh!

rullzer commented Feb 6, 2015

Uh oh!

schiessle commented Feb 6, 2015

Uh oh!

schiessle commented Feb 9, 2015

Uh oh!

rullzer commented Feb 9, 2015

Uh oh!

jancborchardt commented Feb 9, 2015

Uh oh!

rullzer commented Feb 10, 2015

Uh oh!

jancborchardt commented Feb 10, 2015

Uh oh!

rullzer commented Feb 10, 2015

Uh oh!

jancborchardt commented Feb 10, 2015

Uh oh!

rullzer commented Feb 11, 2015

Uh oh!

schiessle commented Feb 12, 2015

Uh oh!

rullzer commented Feb 12, 2015

Uh oh!

PVince81 Feb 12, 2015

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Feb 12, 2015

Uh oh!

rullzer commented Feb 12, 2015

Uh oh!

PVince81 Feb 12, 2015

Choose a reason for hiding this comment

Uh oh!

rullzer Feb 12, 2015

Choose a reason for hiding this comment

Uh oh!

PVince81 commented Feb 12, 2015

Uh oh!

ghost commented Feb 12, 2015

Build result: FAILURE

Uh oh!

PVince81 commented Feb 13, 2015

Uh oh!

rullzer commented Feb 13, 2015

Uh oh!

ghost commented Feb 13, 2015

Build result: FAILURE

Uh oh!

scrutinizer-notifier commented Feb 13, 2015

Uh oh!

schiessle commented Feb 13, 2015

Uh oh!

jancborchardt commented Feb 16, 2015

Uh oh!

PVince81 commented Feb 16, 2015

Uh oh!

rullzer commented Feb 16, 2015

Uh oh!

PVince81 commented Feb 17, 2015

Uh oh!

jancborchardt commented Feb 17, 2015

Uh oh!

Uh oh!