rip out obsolete recipientsDisplayName

also needs tests adjustements, and this also brings in natural sorting

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>

6 files changed, 31 insertions, 108 deletions

diff --git a/apps/files_sharing/js/share.js b/apps/files_sharing/js/share.js
index 9b794ca2d13..aa0803c491b 100644
--- a/apps/files_sharing/js/share.js
+++ b/apps/files_sharing/js/share.js
@@ -52,8 +52,7 @@
 						tr.attr('data-permissions', fileData.permissions | OC.PERMISSION_UPDATE);
 					}
 				}
-				if (fileData.recipientsDisplayName) {
-					tr.attr('data-share-recipients', fileData.recipientsDisplayName);
+				if (fileData.recipientData && !_.isEmpty(fileData.recipientData)) {
 					tr.attr('data-share-recipient-data', JSON.stringify(fileData.recipientData));
 				}
 				if (fileData.shareTypes) {
@@ -78,8 +77,6 @@
 					fileInfo.shares.push({expiration: expirationTimestamp});
 				}
 
-				fileInfo.recipientsDisplayName = $el.attr('data-share-recipients') || undefined;
-
 				return fileInfo;
 			};
 
@@ -219,14 +216,12 @@
 			var recipients = _.pluck(shareModel.get('shares'), 'share_with_displayname');
 			// note: we only update the data attribute because updateIcon()
 			if (recipients.length) {
-				$tr.attr('data-share-recipients', OCA.Sharing.Util.formatRecipients(recipients));
 				var recipientData = _.mapObject(shareModel.get('shares'), function (share) {
 					return {shareWith: share.share_with, shareWithDisplayName: share.share_with_displayname};
 				});
 				$tr.attr('data-share-recipient-data', JSON.stringify(recipientData));
 			}
 			else {
-				$tr.removeAttr('data-share-recipients');
 				$tr.removeAttr('data-share-recipient-data');
 			}
 		},
@@ -235,15 +230,15 @@
 		 * Update the file action share icon for the given file
 		 *
 		 * @param $tr file element of the file to update
-		 * @param {bool} hasUserShares true if a user share exists
-		 * @param {bool} hasLinkShare true if a link share exists
+		 * @param {boolean} hasUserShares true if a user share exists
+		 * @param {boolean} hasLinkShare true if a link share exists
 		 *
-		 * @return {bool} true if the icon was set, false otherwise
+		 * @return {boolean} true if the icon was set, false otherwise
 		 */
 		_updateFileActionIcon: function($tr, hasUserShares, hasLinkShare) {
 			// if the statuses are loaded already, use them for the icon
 			// (needed when scrolling to the next page)
-			if (hasUserShares || hasLinkShare || $tr.attr('data-share-recipients') || $tr.attr('data-share-owner')) {
+			if (hasUserShares || hasLinkShare || $tr.attr('data-share-recipient-data') || $tr.attr('data-share-owner')) {
 				OC.Share.markFileAsShared($tr, true, hasLinkShare);
 				return true;
 			}
@@ -251,31 +246,6 @@
 		},
 
 		/**
-		 * Formats a recipients array to be displayed.
-		 * The first four recipients will be shown and the
-		 * other ones will be shown as "+x" where "x" is the number of
-		 * remaining recipients.
-		 *
-		 * @param {Array.<String>} recipients recipients array
-		 * @param {int} count optional total recipients count (in case the array was shortened)
-		 * @return {String} formatted recipients display text
-		 */
-		formatRecipients: function(recipients, count) {
-			var maxRecipients = 4;
-			var text;
-			if (!_.isNumber(count)) {
-				count = recipients.length;
-			}
-			// TODO: use natural sort
-			recipients = _.first(recipients, maxRecipients).sort();
-			text = recipients.join(', ');
-			if (count > maxRecipients) {
-				text += ', +' + (count - maxRecipients);
-			}
-			return text;
-		},
-
-		/**
 		 * @param {Array} fileData
 		 * @returns {String}
 		 */
diff --git a/apps/files_sharing/js/sharedfilelist.js b/apps/files_sharing/js/sharedfilelist.js
index fb4909e764e..ad6f70a6f1c 100644
--- a/apps/files_sharing/js/sharedfilelist.js
+++ b/apps/files_sharing/js/sharedfilelist.js
@@ -375,11 +375,6 @@
 					// convert the recipients map to a flat
 					// array of sorted names
 					data.mountType = 'shared';
-					data.recipients = _.keys(data.recipients);
-					data.recipientsDisplayName = OCA.Sharing.Util.formatRecipients(
-						data.recipients,
-						data.recipientsCount
-					);
 					delete data.recipientsCount;
 					if (self._sharedWithUser) {
 						// only for outgoing shres
@@ -413,10 +408,19 @@
 	 * @property {int} stime share timestamp in milliseconds
 	 * @property {String} [targetDisplayName] display name of the recipient
 	 * (only when shared with others)
+	 * @property {String} [targetShareWithId] id of the recipient
 	 *
 	 */
 
 	/**
+	 * Recipient attributes
+	 *
+	 * @typedef {Object} OCA.Sharing.RecipientInfo
+	 * @property {String} shareWith the id of the recipient
+	 * @property {String} shareWithDisplayName the display name of the recipient
+	 */
+
+	/**
 	 * Shared file info attributes.
 	 *
 	 * @typedef {OCA.Files.FileInfo} OCA.Sharing.SharedFileInfo
@@ -427,7 +431,8 @@
 	 * @property {String} shareOwner name of the share owner
 	 * @property {Array.<String>} recipients name of the first 4 recipients
 	 * (this is mostly for display purposes)
-	 * @property {String} recipientsDisplayName display name
+	 * @property {Object.<OCA.Sharing.RecipientInfo>} recipientData (as object for easier
+	 * passing to HTML data attributes with jQuery)
 	 */
 
 	OCA.Sharing.FileList = FileList;
diff --git a/apps/files_sharing/tests/js/shareSpec.js b/apps/files_sharing/tests/js/shareSpec.js
index 1b628f5bb0f..893525f7566 100644
--- a/apps/files_sharing/tests/js/shareSpec.js
+++ b/apps/files_sharing/tests/js/shareSpec.js
@@ -282,8 +282,6 @@ describe('OCA.Sharing.Util tests', function() {
 				]
 			});
 
-			expect($tr.attr('data-share-recipients')).toEqual('Group One, Group Two, User One, User Two');
-
 			expect($action.text().trim()).toEqual('Shared with Group One Shared with Group Two Shared with User One Shared with User Two');
 			expect($action.find('.icon').hasClass('icon-shared')).toEqual(true);
 			expect($action.find('.icon').hasClass('icon-public')).toEqual(false);
@@ -315,8 +313,6 @@ describe('OCA.Sharing.Util tests', function() {
 				]
 			});
 
-			expect($tr.attr('data-share-recipients')).toEqual('User One, User Three, User Two');
-
 			expect($action.text().trim()).toEqual('Shared with User One Shared with User Three Shared with User Two');
 			expect($action.find('.icon').hasClass('icon-shared')).toEqual(true);
 			expect($action.find('.icon').hasClass('icon-public')).toEqual(false);
@@ -345,7 +341,7 @@ describe('OCA.Sharing.Util tests', function() {
 				shares: []
 			});
 
-			expect($tr.attr('data-share-recipients')).not.toBeDefined();
+			expect($tr.attr('data-share-recipient-data')).not.toBeDefined();
 		});
 		it('keep share text after updating reshare', function() {
 			var $action, $tr;
@@ -372,8 +368,6 @@ describe('OCA.Sharing.Util tests', function() {
 				shares: [{share_with_displayname: 'User Two'}]
 			});
 
-			expect($tr.attr('data-share-recipients')).toEqual('User Two');
-
 			expect($action.find('>span').text().trim()).toEqual('Shared by User One');
 			expect($action.find('.icon').hasClass('icon-shared')).toEqual(true);
 			expect($action.find('.icon').hasClass('icon-public')).toEqual(false);
@@ -405,60 +399,13 @@ describe('OCA.Sharing.Util tests', function() {
 				shares: []
 			});
 
-			expect($tr.attr('data-share-recipients')).not.toBeDefined();
+			expect($tr.attr('data-share-recipient-data')).not.toBeDefined();
 
 			expect($action.find('>span').text().trim()).toEqual('Shared by User One');
 			expect($action.find('.icon').hasClass('icon-shared')).toEqual(true);
 			expect($action.find('.icon').hasClass('icon-public')).toEqual(false);
 		});
 	});
-	describe('formatRecipients', function() {
-		it('returns a single recipient when one passed', function() {
-			expect(OCA.Sharing.Util.formatRecipients(['User one']))
-				.toEqual('User one');
-		});
-		it('returns two recipients when two passed', function() {
-			expect(OCA.Sharing.Util.formatRecipients(['User one', 'User two']))
-				.toEqual('User one, User two');
-		});
-		it('returns four recipients with plus when five passed', function() {
-			var recipients = [
-				'User one',
-				'User two',
-				'User three',
-				'User four',
-				'User five'
-			];
-			expect(OCA.Sharing.Util.formatRecipients(recipients))
-				.toEqual('User four, User one, User three, User two, +1');
-		});
-		it('returns four recipients with plus when ten passed', function() {
-			var recipients = [
-				'User one',
-				'User two',
-				'User three',
-				'User four',
-				'User five',
-				'User six',
-				'User seven',
-				'User eight',
-				'User nine',
-				'User ten'
-			];
-			expect(OCA.Sharing.Util.formatRecipients(recipients))
-				.toEqual('User four, User one, User three, User two, +6');
-		});
-		it('returns four recipients with plus when four passed with counter', function() {
-			var recipients = [
-				'User one',
-				'User two',
-				'User three',
-				'User four'
-			];
-			expect(OCA.Sharing.Util.formatRecipients(recipients, 10))
-				.toEqual('User four, User one, User three, User two, +6');
-		});
-	});
 	describe('Excluded lists', function() {
 		function createListThenAttach(listId) {
 			var fileActions = new OCA.Files.FileActions();
diff --git a/apps/files_sharing/tests/js/sharedfilelistSpec.js b/apps/files_sharing/tests/js/sharedfilelistSpec.js
index 3efbb8fcea3..903234947bd 100644
--- a/apps/files_sharing/tests/js/sharedfilelistSpec.js
+++ b/apps/files_sharing/tests/js/sharedfilelistSpec.js
@@ -628,7 +628,7 @@ describe('OCA.Sharing.FileList tests', function() {
 			expect($tr.attr('data-permissions')).toEqual('31'); // read and delete
 			expect($tr.attr('data-mime')).toEqual('text/plain');
 			expect($tr.attr('data-mtime')).toEqual('11111000');
-			expect($tr.attr('data-share-recipients')).not.toBeDefined();
+			expect($tr.attr('data-share-recipient-data')).not.toBeDefined();
 			expect($tr.attr('data-share-owner')).not.toBeDefined();
 			expect($tr.attr('data-share-id')).toEqual('7');
 			expect($tr.attr('data-favorite')).toEqual('true');
@@ -681,7 +681,7 @@ describe('OCA.Sharing.FileList tests', function() {
 			expect($tr.attr('data-permissions')).toEqual('31'); // read and delete
 			expect($tr.attr('data-mime')).toEqual('text/plain');
 			expect($tr.attr('data-mtime')).toEqual('11111000');
-			expect($tr.attr('data-share-recipients')).not.toBeDefined();
+			expect($tr.attr('data-share-recipient-data')).not.toBeDefined();
 			expect($tr.attr('data-share-owner')).not.toBeDefined();
 			expect($tr.attr('data-share-id')).toEqual('7');
 			expect($tr.attr('data-favorite')).toEqual('true');
diff --git a/core/js/share.js b/core/js/share.js
index 381c42c5de2..a2a3635e145 100644
--- a/core/js/share.js
+++ b/core/js/share.js
@@ -245,7 +245,10 @@ OC.Share = _.extend(OC.Share || {}, {
 	 */
 	_formatShareList: function(recipients) {
 		var _parent = this;
-		recipients = _.sortBy(_.toArray(recipients), 'shareWithDisplayName');
+		recipients = _.toArray(recipients);
+		recipients.sort(function(a, b) {
+			return a.shareWithDisplayName.localeCompare(b.shareWithDisplayName);
+		});
 		return $.map(recipients, function(recipient) {
 			return _parent._formatRemoteShare(recipient.shareWith, recipient.shareWithDisplayName, t('core', 'Shared with'));
 		});
diff --git a/core/js/tests/specs/shareSpec.js b/core/js/tests/specs/shareSpec.js
index 8688705b8e8..399911c3d54 100644
--- a/core/js/tests/specs/shareSpec.js
+++ b/core/js/tests/specs/shareSpec.js
@@ -157,8 +157,6 @@ describe('OC.Share tests', function() {
 			function checkRecipients(input, output, title) {
 				var $action;
 
-				var concatenated = _.values(input).join(', ');
-				$file.attr('data-share-recipients', concatenated);
 				$file.attr('data-share-recipient-data', JSON.stringify(input));
 				OC.Share.markFileAsShared($file, true);
 
@@ -270,8 +268,8 @@ describe('OC.Share tests', function() {
 						shareWithDisplayName: 'One@someserver.com'
 					},
 					1: {
-						shareWith: 'two@someserver.com',
-						shareWithDisplayName: 'two@someserver.com'
+						shareWith: 'two@otherserver.com',
+						shareWithDisplayName: 'two@otherserver.com'
 					}
 				};
 				checkRecipients(
@@ -320,8 +318,8 @@ describe('OC.Share tests', function() {
 							shareWithDisplayName: 'One'
 						},
 						1: {
-							shareWith: 'two@someserver.com',
-							shareWithDisplayName: 'two@someserver.com'
+							shareWith: 'two@otherserver.com',
+							shareWithDisplayName: 'two@otherserver.com'
 						}
 					},
 					'Shared with One two@…',
@@ -336,15 +334,15 @@ describe('OC.Share tests', function() {
 							shareWithDisplayName: 'Yoko Ono'
 						},
 						1: {
-							shareWith: 'two@someserver.com',
-							shareWithDisplayName: 'two@someserver.com'
+							shareWith: 'two@otherserver.com',
+							shareWithDisplayName: 'two@othererver.com'
 						},
 						2: {
 							shareWith: 'Three',
 							shareWithDisplayName: 'Green, Mina'
 						}
 					},
-					'Shared with Yoko Ono two@… Shared with Green, Mina',
+					'Shared with Green, Mina two@… Shared with Yoko Ono',
 					['Shared with two@otherserver.com']
 				);
 			});