Merge pull request #7222 from nextcloud/fix-filerow-avatars

Fix filerow avatars

2 files changed, 160 insertions, 31 deletions

diff --git a/core/js/share.js b/core/js/share.js
index 25d59b46fb4..7662d6cffb9 100644
--- a/core/js/share.js
+++ b/core/js/share.js
@@ -161,7 +161,6 @@ OC.Share = _.extend(OC.Share || {}, {
 	updateIcon:function(itemType, itemSource) {
 		var shares = false;
 		var link = false;
-		var image = OC.imagePath('core', 'actions/share');
 		var iconClass = '';
 		$.each(OC.Share.itemShares, function(index) {
 			if (OC.Share.itemShares[index]) {
@@ -200,15 +199,17 @@ OC.Share = _.extend(OC.Share || {}, {
 	/**
 	 * Format a remote address
 	 *
-	 * @param {String} remoteAddress full remote share
+	 * @param {String} shareWith userid, full remote share, or whatever
+	 * @param {String} shareWithDisplayName
+	 * @param {String} message
 	 * @return {String} HTML code to display
 	 */
-	_formatRemoteShare: function(remoteAddress, message) {
-		var parts = this._REMOTE_OWNER_REGEXP.exec(remoteAddress);
+	_formatRemoteShare: function(shareWith, shareWithDisplayName, message) {
+		var parts = this._REMOTE_OWNER_REGEXP.exec(shareWith);
 		if (!parts) {
 			// display avatar of the user
-			var avatar = '<span class="avatar" data-userName="' + escapeHTML(remoteAddress) + '" title="' + message + " " + escapeHTML(remoteAddress) + '"></span>';
-			var hidden = '<span class="hidden-visually">' + message + ' ' + escapeHTML(remoteAddress) + '</span> ';
+			var avatar = '<span class="avatar" data-username="' + escapeHTML(shareWith) + '" title="' + message + " " + escapeHTML(shareWithDisplayName) + '"></span>';
+			var hidden = '<span class="hidden-visually">' + message + ' ' + escapeHTML(shareWithDisplayName) + '</span> ';
 			return avatar + hidden;
 		}
 
@@ -238,14 +239,17 @@ OC.Share = _.extend(OC.Share || {}, {
 	 * Loop over all recipients in the list and format them using
 	 * all kind of fancy magic.
 	 *
-	 * @param {String[]} recipients array of all the recipients
+	 * @param {Object} recipients array of all the recipients
 	 * @return {String[]} modified list of recipients
 	 */
 	_formatShareList: function(recipients) {
 		var _parent = this;
+		recipients = _.toArray(recipients);
+		recipients.sort(function(a, b) {
+			return a.shareWithDisplayName.localeCompare(b.shareWithDisplayName);
+		});
 		return $.map(recipients, function(recipient) {
-			recipient = _parent._formatRemoteShare(recipient, t('core', 'Shared with'));
-			return recipient;
+			return _parent._formatRemoteShare(recipient.shareWith, recipient.shareWithDisplayName, t('core', 'Shared with'));
 		});
 	},
 	/**
@@ -261,12 +265,13 @@ OC.Share = _.extend(OC.Share || {}, {
 		var type = $tr.data('type');
 		var icon = action.find('.icon');
 		var message, recipients, avatars;
+		var ownerId = $tr.attr('data-share-owner-id');
 		var owner = $tr.attr('data-share-owner');
 		var shareFolderIcon;
 		var iconClass = 'icon-shared';
 		action.removeClass('shared-style');
 		// update folder icon
-		if (type === 'dir' && (hasShares || hasLink || owner)) {
+		if (type === 'dir' && (hasShares || hasLink || ownerId)) {
 			if (hasLink) {
 				shareFolderIcon = OC.MimeType.getIconUrl('dir-public');
 			}
@@ -290,25 +295,26 @@ OC.Share = _.extend(OC.Share || {}, {
 			$tr.find('.filename .thumbnail').css('background-image', 'url(' + shareFolderIcon + ')');
 		}
 		// update share action text / icon
-		if (hasShares || owner) {
-			recipients = $tr.attr('data-share-recipients');
+		if (hasShares || ownerId) {
+			recipients = $tr.data('share-recipient-data');
 			action.addClass('shared-style');
 
 			avatars = '<span>' + t('core', 'Shared') + '</span>';
 			// even if reshared, only show "Shared by"
-			if (owner) {
+			if (ownerId) {
 				message = t('core', 'Shared by');
-				avatars = this._formatRemoteShare(owner, message);
+				avatars = this._formatRemoteShare(ownerId, owner, message);
 			} else if (recipients) {
-				avatars = this._formatShareList(recipients.split(', ')).join('');
+				avatars = this._formatShareList(recipients);
 			}
 			action.html(avatars).prepend(icon);
 
-			if (owner || recipients) {
+			if (ownerId || recipients) {
 				var avatarElement = action.find('.avatar');
-				avatarElement.avatar(avatarElement.data('username'), 32);
-
-				action.find('.icon-shared + span').tooltip({placement: 'top'});
+				avatarElement.each(function () {
+					$(this).avatar($(this).data('username'), 32);
+				});
+				action.find('span[title]').tooltip({placement: 'top'});
 			}
 		} else {
 			action.html('<span class="hidden-visually">' + t('core', 'Shared') + '</span>').prepend(icon);
diff --git a/core/js/tests/specs/shareSpec.js b/core/js/tests/specs/shareSpec.js
index 70c698c99a2..05057692e98 100644
--- a/core/js/tests/specs/shareSpec.js
+++ b/core/js/tests/specs/shareSpec.js
@@ -45,6 +45,7 @@ describe('OC.Share tests', function() {
 				var $action;
 
 				$file.attr('data-share-owner', input);
+				$file.attr('data-share-owner-id', input);
 				OC.Share.markFileAsShared($file);
 
 				$action = $file.find('.action-share>span').parent();
@@ -119,6 +120,7 @@ describe('OC.Share tests', function() {
 			it('shows a shared folder icon for folders shared with the current user', function() {
 				$file.attr('data-type', 'dir');
 				$file.attr('data-share-owner', 'someoneelse');
+				$file.attr('data-share-owner-id', 'someoneelse');
 				OC.Share.markFileAsShared($file);
 
 				checkIcon('filetypes/folder-shared');
@@ -155,7 +157,7 @@ describe('OC.Share tests', function() {
 			function checkRecipients(input, output, title) {
 				var $action;
 
-				$file.attr('data-share-recipients', input);
+				$file.attr('data-share-recipient-data', JSON.stringify(input));
 				OC.Share.markFileAsShared($file, true);
 
 				$action = $file.find('.action-share>span').parent();
@@ -177,66 +179,187 @@ describe('OC.Share tests', function() {
 			}
 
 			it('displays the local share owner as is', function() {
-				checkRecipients('User One', 'Shared with User One', null);
+				var input = {
+					0: {
+						shareWith: 'User One',
+						shareWithDisplayName: 'User One'
+					}
+				};
+				checkRecipients(input, 'Shared with User One', null);
 			});
 			it('displays the user name part of a remote recipient', function() {
+				var input = {
+					0: {
+						shareWith: 'User One@someserver.com',
+						shareWithDisplayName: 'User One@someserver.com'
+					}
+				};
 				checkRecipients(
-					'User One@someserver.com',
+					input,
 					'User One@…',
 					'Shared with User One@someserver.com'
 				);
+
+				input = {
+					0: {
+						shareWith: 'User One@someserver.com/',
+						shareWithDisplayName: 'User One@someserver.com/'
+					}
+				};
 				checkRecipients(
-					'User One@someserver.com/',
+					input,
 					'User One@…',
 					'Shared with User One@someserver.com'
 				);
+
+				input = {
+					0: {
+						shareWith: 'User One@someserver.com/root/of/nextcloud',
+						shareWithDisplayName: 'User One@someserver.com/root/of/nextcloud'
+					}
+				};
 				checkRecipients(
-					'User One@someserver.com/root/of/owncloud',
+					input,
 					'User One@…',
 					'Shared with User One@someserver.com'
 				);
 			});
 			it('displays the user name part with domain of a remote share owner', function() {
+				var input = {
+					0: {
+						shareWith: 'User One@example.com@someserver.com',
+						shareWithDisplayName: 'User One@example.com@someserver.com'
+					}
+				};
 				checkRecipients(
-					'User One@example.com@someserver.com',
+					input,
 					'User One@example.com',
 					'Shared with User One@example.com@someserver.com'
 				);
+
+				input = {
+					0: {
+						shareWith: 'User One@example.com@someserver.com/',
+						shareWithDisplayName: 'User One@example.com@someserver.com/'
+					}
+				};
 				checkRecipients(
-					'User One@example.com@someserver.com/',
+					input,
 					'User One@example.com',
 					'Shared with User One@example.com@someserver.com'
 				);
+
+				input = {
+					0: {
+						shareWith: 'User One@example.com@someserver.com/root/of/nextcloud',
+						shareWithDisplayName: 'User One@example.com@someserver.com/root/of/nextcloud'
+					}
+				};
 				checkRecipients(
-					'User One@example.com@someserver.com/root/of/owncloud',
+					input,
 					'User One@example.com',
 					'Shared with User One@example.com@someserver.com'
 				);
 			});
 			it('display multiple remote recipients', function() {
+				var input = {
+					0: {
+						shareWith: 'One@someserver.com',
+						shareWithDisplayName: 'One@someserver.com'
+					},
+					1: {
+						shareWith: 'two@otherserver.com',
+						shareWithDisplayName: 'two@otherserver.com'
+					}
+				};
 				checkRecipients(
-					'One@someserver.com, two@otherserver.com',
+					input,
 					'One@… two@…',
 					['Shared with One@someserver.com', 'Shared with two@otherserver.com']
 				);
+
+				input = {
+					0: {
+						shareWith: 'One@someserver.com/',
+						shareWithDisplayName: 'One@someserver.com/'
+					},
+					1: {
+						shareWith: 'two@someserver.com',
+						shareWithDisplayName: 'two@someserver.com'
+					}
+				};
 				checkRecipients(
-					'One@someserver.com/, two@otherserver.com',
+					input,
 					'One@… two@…',
 					['Shared with One@someserver.com', 'Shared with two@otherserver.com']
 				);
+
+				input = {
+					0: {
+						shareWith: 'One@someserver.com/root/of/nextcloud',
+						shareWithDisplayName: 'One@someserver.com/root/of/nextcloud'
+					},
+					1: {
+						shareWith: 'two@someserver.com',
+						shareWithDisplayName: 'two@someserver.com'
+					}
+				};
 				checkRecipients(
-					'One@someserver.com/root/of/owncloud, two@otherserver.com',
+					input,
 					'One@… two@…',
 					['Shared with One@someserver.com', 'Shared with two@otherserver.com']
 				);
 			});
 			it('display mixed recipients', function() {
 				checkRecipients(
-					'One, two@otherserver.com',
+					{
+						0: {
+							shareWith: 'One',
+							shareWithDisplayName: 'One'
+						},
+						1: {
+							shareWith: 'two@otherserver.com',
+							shareWithDisplayName: 'two@otherserver.com'
+						}
+					},
 					'Shared with One two@…',
 					['Shared with two@otherserver.com']
 				);
 			});
+			it('display multiple with divergent displaynames', function() {
+				var recipients = {
+					0: {
+						shareWith: 'One',
+						shareWithDisplayName: 'Yoko Ono',
+						_output: 'Shared with Yoko Ono'
+					},
+					1: {
+						shareWith: 'two@otherserver.com',
+						shareWithDisplayName: 'two@othererver.com',
+						_output: 'two@…'
+					},
+					2: {
+						shareWith: 'Three',
+						shareWithDisplayName: 'Green, Mina',
+						_output: 'Shared with Green, Mina'
+					}
+				};
+
+				// we cannot assume the locale, also because PhantomJS has a bug.
+				var sortArray = _.toArray(recipients)
+					.sort(function(a, b) {
+						return a.shareWithDisplayName.localeCompare(b.shareWithDisplayName);
+					});
+				var sortedOutput = _.map(sortArray, function(recipient) {
+					return recipient._output;
+				}).join(' ');
+
+				checkRecipients(
+					recipients,
+					sortedOutput,
+					['Shared with two@otherserver.com']
+				);
+			});
 		});
 	});
 });