Merge pull request #29463 from owncloud/stable10-notify-checkbox-to-b…

…utton

[stable10] Replace notify user for local shares with button

apps/files_sharing/css/sharetabview.css

@@ -101,6 +101,12 @@
   margin-top: 20px;
 }
 
+.shareTabView .shareWithList .mailNotificationSpinner {
+	position: relative;
+	vertical-align: middle;
+	margin-right: 10px;
+}
+
 .subTabHeaders .tabHeaders {
   margin-left: 0px;
   margin-right: 0px;

core/ajax/share.php

@@ -131,7 +131,11 @@ function usersInGroup($gid, $search = '', $limit = -1, $offset = 0) {
 			);
 			$result = $mailNotification->sendInternalShareMail($recipientList, $itemSource, $itemType);
 
-			\OCP\Share::setSendMailStatus($itemType, $itemSource, $shareType, $recipient, true);
+			// if we were able to send to at least one recipient, mark as sent
+			// allowing the user to resend would spam users who already got a notification
+			if (count($result) < count($recipientList)) {
+				\OCP\Share::setSendMailStatus($itemType, $itemSource, $shareType, $recipient, true);
+			}
 
 			if (empty($result)) {
 				OCP\JSON::success();

core/js/sharedialogshareelistview.js

@@ -24,8 +24,10 @@
 			'<span class="has-tooltip username" title="{{shareWith}}">{{shareWithDisplayName}}</span>' +
 			'{{#if mailNotificationEnabled}}  {{#unless isRemoteShare}}' +
 			'<span class="shareOption">' +
-			'<input id="mail-{{cid}}-{{shareWith}}" type="checkbox" name="mailNotification" class="mailNotification checkbox" {{#if wasMailSent}}checked="checked"{{/if}} />' +
-			'<label for="mail-{{cid}}-{{shareWith}}">{{notifyByMailLabel}}</label>' +
+			'{{#unless wasMailSent}}' +
+			'<span class="mailNotificationSpinner icon-loading-small hidden"></span>' +
+			'<input id="mail-{{cid}}-{{shareWith}}" type="button" name="mailNotification" value="{{notifyByMailLabel}}" class="mailNotification checkbox" />' +
+			'{{/unless}}' +
 			'</span>' +
 			'{{/unless}} {{/if}}' +
 			'{{#if isResharingAllowed}} {{#if sharePermissionPossible}}' +
@@ -287,10 +289,26 @@
 			var shareType = $li.data('share-type');
 			var shareWith = $li.attr('data-share-with');
 
-			this.model.sendNotificationForShare(shareType, shareWith, $target.is(':checked'));
+			var $loading = $target.prev('.icon-loading-small');
+
+			$target.addClass('hidden');
+			$loading.removeClass('hidden');
+
+			this.model.sendNotificationForShare(shareType, shareWith, true).then(function(result) {
+				if (result.status === 'success') {
+					OC.Notification.showTemporary(t('core', 'Email notification was sent!'));
+					$target.remove();
+				} else {
+					// sending was successful but some users might not have any email address
+					OC.dialogs.alert(t('core', result.data.message), t('core', 'Email notification not sent'));
+				}
+
+				$target.removeClass('hidden');
+				$loading.addClass('hidden');
+			});
 		}
 	});
 
 	OC.Share.ShareDialogShareeListView = ShareDialogShareeListView;
 
-})();
+})();

core/js/shareitemmodel.js

@@ -412,12 +412,6 @@
 					shareType: shareType,
 					itemSource: itemSource,
 					itemType: itemType
-				},
-				function(result) {
-					if (result.status !== 'success') {
-						// FIXME: a model should not show dialogs
-						OC.dialogs.alert(t('core', result.data.message), t('core', 'Warning'));
-					}
 				}
 			);
 		},

core/js/tests/specs/sharedialogshareelistview.js

@@ -137,7 +137,8 @@ describe('OC.Share.ShareDialogShareeListView', function () {
 			expect(listView.$el.find('li.cruds').hasClass('hidden')).toEqual(false);
 		});
 
-		it('sends notification to user when checkbox clicked', function () {
+		it('sends notification to user when button clicked', function () {
+			var notifStub = sinon.stub(OC.Notification, 'show');
 			shareModel.set('shares', [{
 				id: 100,
 				item_source: 123,
@@ -147,10 +148,46 @@ describe('OC.Share.ShareDialogShareeListView', function () {
 				share_with_displayname: 'User One'
 			}]);
 			listView.render();
-			var notificationStub = sinon.stub(listView.model, 'sendNotificationForShare');
+			var deferred = new $.Deferred();
+			var notificationStub = sinon.stub(listView.model, 'sendNotificationForShare').returns(deferred.promise());
 			listView.$el.find("input[name='mailNotification']").click();
+			// spinner appears
+			expect(listView.$el.find('.mailNotificationSpinner').hasClass('hidden')).toEqual(false);
 			expect(notificationStub.called).toEqual(true);
 			notificationStub.restore();
+
+			deferred.resolve({status: 'success'});
+			expect(notifStub.calledOnce).toEqual(true);
+			notifStub.restore();
+
+			// button is removed
+			expect(listView.$el.find("input[name='mailNotification']").length).toEqual(0);
+
+		});
+		it('displays error if email notification not sent', function () {
+			var notifStub = sinon.stub(OC.dialogs, 'alert');
+			shareModel.set('shares', [{
+				id: 100,
+				item_source: 123,
+				permissions: 1,
+				share_type: OC.Share.SHARE_TYPE_USER,
+				share_with: 'user1',
+				share_with_displayname: 'User One'
+			}]);
+			listView.render();
+			var deferred = new $.Deferred();
+			var notificationStub = sinon.stub(listView.model, 'sendNotificationForShare').returns(deferred.promise());
+			listView.$el.find("input[name='mailNotification']").click();
+			expect(notificationStub.called).toEqual(true);
+			notificationStub.restore();
+
+			deferred.resolve({status: 'error', data: {message: 'message'}});
+			expect(notifStub.calledOnce).toEqual(true);
+			notifStub.restore();
+
+			// button is still there
+			expect(listView.$el.find("input[name='mailNotification']").length).toEqual(1);
+			expect(listView.$el.find("input[name='mailNotification']").hasClass('hidden')).toEqual(false);
 		});
 
 	});

lib/private/Share/MailNotifications.php

@@ -102,7 +102,7 @@ public function sendInternalShareMail($recipientList, $itemSource, $itemType) {
 			$recipientDisplayName = $recipient->getDisplayName();
 			$to = $recipient->getEMailAddress();
 
-			if ($to === '') {
+			if ($to === null || $to === '') {
 				$noMail[] = $recipientDisplayName;
 				continue;
 			}

tests/lib/Share/MailNotificationsTest.php

@@ -71,7 +71,7 @@ public function setUp() {
 			}));
 
 		$this->defaults
-				->expects($this->once())
+				->expects($this->any())
 				->method('getName')
 				->will($this->returnValue('UnitTestCloud'));
 
@@ -258,6 +258,57 @@ public function testSendInternalShareMail() {
 
 	}
 
+	public function emptinessProvider() {
+		return [
+			[null],
+			[''],
+		];
+	}
+
+	/**
+	 * @dataProvider emptinessProvider
+	 */
+	public function testSendInternalShareMailNoMail($emptiness) {
+		/** @var MailNotifications | \PHPUnit_Framework_MockObject_MockObject $mailNotifications */
+		$mailNotifications = $this->getMockBuilder('OC\Share\MailNotifications')
+			->setMethods(['getItemSharedWithUser'])
+			->setConstructorArgs([
+				$this->user,
+				$this->l10n,
+				$this->mailer,
+				$this->logger,
+				$this->defaults,
+				$this->urlGenerator
+			])
+			->getMock();
+
+		$recipient = $this->getMockBuilder('\OCP\IUser')
+				->disableOriginalConstructor()->getMock();
+		$recipient
+				->expects($this->once())
+				->method('getEMailAddress')
+				->willReturn($emptiness);
+		$recipient
+				->expects($this->once())
+				->method('getDisplayName')
+				->willReturn('No mail 1');
+		$recipient2 = $this->getMockBuilder('\OCP\IUser')
+				->disableOriginalConstructor()->getMock();
+		$recipient2
+				->expects($this->once())
+				->method('getEMailAddress')
+				->willReturn('');
+		$recipient2
+				->expects($this->once())
+				->method('getDisplayName')
+				->willReturn('No mail 2');
+
+		$recipientList = [$recipient, $recipient2];
+		$result = $mailNotifications->sendInternalShareMail($recipientList, '3', 'file');
+		$this->assertSame(['No mail 1', 'No mail 2'], $result);
+
+	}
+
 	/**
 	 * @param string $subject
 	 */