feat(sharebymail): improve share email format

Signed-off-by: skjnldsv <skjnldsv@protonmail.com>

2 files changed, 210 insertions, 43 deletions

diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php
index ab9433f51c8..b21f6440c64 100644
--- a/apps/sharebymail/lib/ShareByMailProvider.php
+++ b/apps/sharebymail/lib/ShareByMailProvider.php
@@ -328,21 +328,36 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider
 			'note' => $note
 		]);
 
-		$emailTemplate->setSubject($this->l->t('%1$s shared »%2$s« with you', [$initiatorDisplayName, $filename]));
+		$emailTemplate->setSubject($this->l->t('%1$s shared %2$s with you', [$initiatorDisplayName, $filename]));
 		$emailTemplate->addHeader();
-		$emailTemplate->addHeading($this->l->t('%1$s shared »%2$s« with you', [$initiatorDisplayName, $filename]), false);
-		$text = $this->l->t('%1$s shared »%2$s« with you.', [$initiatorDisplayName, $filename]);
+		$emailTemplate->addHeading($this->l->t('%1$s shared %2$s with you', [$initiatorDisplayName, $filename]), false);
+		$text = $this->l->t('%1$s shared %2$s with you.', [$initiatorDisplayName, $filename]);
 
 		if ($note !== '') {
-			$emailTemplate->addBodyText(htmlspecialchars($note), $note);
+			$emailTemplate->addBodyListItem(
+				htmlspecialchars($note),
+				$this->l->t('Note:'),
+				$this->getAbsoluteImagePath('caldav/description.png'),
+				$note
+			);
+		}
+
+		if ($expiration !== null) {
+			$dateString = (string)$this->l->l('date', $expiration, ['width' => 'medium']);
+			$emailTemplate->addBodyListItem(
+				$this->l->t('This share is valid until %s at midnight', [$dateString]),
+				$this->l->t('Expiration:'),
+				$this->getAbsoluteImagePath('caldav/time.png'),
+			);
 		}
 
 		$emailTemplate->addBodyText(
 			htmlspecialchars($text . ' ' . $this->l->t('Click the button below to open it.')),
 			$text
 		);
+
 		$emailTemplate->addBodyButton(
-			$this->l->t('Open »%s«', [$filename]),
+			$this->l->t('Open %s', [$filename]),
 			$link
 		);
 
@@ -415,8 +430,8 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider
 		$initiatorDisplayName = ($initiatorUser instanceof IUser) ? $initiatorUser->getDisplayName() : $initiator;
 		$initiatorEmailAddress = ($initiatorUser instanceof IUser) ? $initiatorUser->getEMailAddress() : null;
 
-		$plainBodyPart = $this->l->t("%1\$s shared »%2\$s« with you.\nYou should have already received a separate mail with a link to access it.\n", [$initiatorDisplayName, $filename]);
-		$htmlBodyPart = $this->l->t('%1$s shared »%2$s« with you. You should have already received a separate mail with a link to access it.', [$initiatorDisplayName, $filename]);
+		$plainBodyPart = $this->l->t("%1\$s shared %2\$s with you.\nYou should have already received a separate mail with a link to access it.\n", [$initiatorDisplayName, $filename]);
+		$htmlBodyPart = $this->l->t('%1$s shared %2$s with you. You should have already received a separate mail with a link to access it.', [$initiatorDisplayName, $filename]);
 
 		$message = $this->mailer->createMessage();
 
@@ -428,9 +443,9 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider
 			'shareWith' => $shareWith,
 		]);
 
-		$emailTemplate->setSubject($this->l->t('Password to access »%1$s« shared to you by %2$s', [$filename, $initiatorDisplayName]));
+		$emailTemplate->setSubject($this->l->t('Password to access %1$s shared to you by %2$s', [$filename, $initiatorDisplayName]));
 		$emailTemplate->addHeader();
-		$emailTemplate->addHeading($this->l->t('Password to access »%s«', [$filename]), false);
+		$emailTemplate->addHeading($this->l->t('Password to access %s', [$filename]), false);
 		$emailTemplate->addBodyText(htmlspecialchars($htmlBodyPart), $plainBodyPart);
 		$emailTemplate->addBodyText($this->l->t('It is protected with the following password:'));
 		$emailTemplate->addBodyText($password);
@@ -501,14 +516,14 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider
 		$initiatorDisplayName = ($initiatorUser instanceof IUser) ? $initiatorUser->getDisplayName() : $initiator;
 		$initiatorEmailAddress = ($initiatorUser instanceof IUser) ? $initiatorUser->getEMailAddress() : null;
 
-		$plainHeading = $this->l->t('%1$s shared »%2$s« with you and wants to add:', [$initiatorDisplayName, $filename]);
-		$htmlHeading = $this->l->t('%1$s shared »%2$s« with you and wants to add', [$initiatorDisplayName, $filename]);
+		$plainHeading = $this->l->t('%1$s shared %2$s with you and wants to add:', [$initiatorDisplayName, $filename]);
+		$htmlHeading = $this->l->t('%1$s shared %2$s with you and wants to add', [$initiatorDisplayName, $filename]);
 
 		$message = $this->mailer->createMessage();
 
 		$emailTemplate = $this->mailer->createEMailTemplate('shareByMail.sendNote');
 
-		$emailTemplate->setSubject($this->l->t('»%s« added a note to a file shared with you', [$initiatorDisplayName]));
+		$emailTemplate->setSubject($this->l->t('%s added a note to a file shared with you', [$initiatorDisplayName]));
 		$emailTemplate->addHeader();
 		$emailTemplate->addHeading(htmlspecialchars($htmlHeading), $plainHeading);
 		$emailTemplate->addBodyText(htmlspecialchars($note), $note);
@@ -516,7 +531,7 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider
 		$link = $this->urlGenerator->linkToRouteAbsolute('files_sharing.sharecontroller.showShare',
 			['token' => $share->getToken()]);
 		$emailTemplate->addBodyButton(
-			$this->l->t('Open »%s«', [$filename]),
+			$this->l->t('Open %s', [$filename]),
 			$link
 		);
 
@@ -564,7 +579,7 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider
 			);
 		}
 
-		$bodyPart = $this->l->t('You just shared »%1$s« with %2$s. The share was already sent to the recipient. Due to the security policies defined by the administrator of %3$s each share needs to be protected by password and it is not allowed to send the password directly to the recipient. Therefore you need to forward the password manually to the recipient.', [$filename, $shareWith, $this->defaults->getName()]);
+		$bodyPart = $this->l->t('You just shared %1$s with %2$s. The share was already sent to the recipient. Due to the security policies defined by the administrator of %3$s each share needs to be protected by password and it is not allowed to send the password directly to the recipient. Therefore you need to forward the password manually to the recipient.', [$filename, $shareWith, $this->defaults->getName()]);
 
 		$message = $this->mailer->createMessage();
 		$emailTemplate = $this->mailer->createEMailTemplate('sharebymail.OwnerPasswordNotification', [
@@ -575,9 +590,9 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider
 			'shareWith' => $shareWith,
 		]);
 
-		$emailTemplate->setSubject($this->l->t('Password to access »%1$s« shared by you with %2$s', [$filename, $shareWith]));
+		$emailTemplate->setSubject($this->l->t('Password to access %1$s shared by you with %2$s', [$filename, $shareWith]));
 		$emailTemplate->addHeader();
-		$emailTemplate->addHeading($this->l->t('Password to access »%s«', [$filename]), false);
+		$emailTemplate->addHeading($this->l->t('Password to access %s', [$filename]), false);
 		$emailTemplate->addBodyText($bodyPart);
 		$emailTemplate->addBodyText($this->l->t('This is the password:'));
 		$emailTemplate->addBodyText($password);
@@ -611,6 +626,12 @@ class ShareByMailProvider extends DefaultShareProvider implements IShareProvider
 		return true;
 	}
 
+	private function getAbsoluteImagePath(string $path):string {
+		return $this->urlGenerator->getAbsoluteURL(
+			$this->urlGenerator->imagePath('core', $path)
+		);
+	}
+
 	/**
 	 * generate share token
 	 */
diff --git a/apps/sharebymail/tests/ShareByMailProviderTest.php b/apps/sharebymail/tests/ShareByMailProviderTest.php
index c7f41176c44..e03fba72312 100644
--- a/apps/sharebymail/tests/ShareByMailProviderTest.php
+++ b/apps/sharebymail/tests/ShareByMailProviderTest.php
@@ -5,6 +5,7 @@
  */
 namespace OCA\ShareByMail\Tests;
 
+use DateTime;
 use OC\Mail\Message;
 use OCA\ShareByMail\Settings\SettingsManager;
 use OCA\ShareByMail\ShareByMailProvider;
@@ -1298,19 +1299,19 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('addHeading')
-			->with('Mrs. Owner User shared »file.txt« with you');
+			->with('Mrs. Owner User shared file.txt with you');
 		$template
 			->expects($this->once())
 			->method('addBodyText')
 			->with(
-				'Mrs. Owner User shared »file.txt« with you. Click the button below to open it.',
-				'Mrs. Owner User shared »file.txt« with you.'
+				'Mrs. Owner User shared file.txt with you. Click the button below to open it.',
+				'Mrs. Owner User shared file.txt with you.'
 			);
 		$template
 			->expects($this->once())
 			->method('addBodyButton')
 			->with(
-				'Open »file.txt«',
+				'Open file.txt',
 				'https://example.com/file.txt'
 			);
 		$message
@@ -1346,7 +1347,7 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('setSubject')
-			->with('Mrs. Owner User shared »file.txt« with you');
+			->with('Mrs. Owner User shared file.txt with you');
 		$message
 			->expects($this->once())
 			->method('useTemplate')
@@ -1411,19 +1412,32 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('addHeading')
-			->with('Mrs. Owner User shared »file.txt« with you');
+			->with('Mrs. Owner User shared file.txt with you');
 		$template
-			->expects($this->exactly(2))
+			->expects($this->once())
 			->method('addBodyText')
-			->withConsecutive(
-				['This is a note to the recipient', 'This is a note to the recipient'],
-				['Mrs. Owner User shared »file.txt« with you. Click the button below to open it.', 'Mrs. Owner User shared »file.txt« with you.'],
+			->with('Mrs. Owner User shared file.txt with you. Click the button below to open it.', 'Mrs. Owner User shared file.txt with you.');
+
+		$this->urlGenerator->expects($this->once())->method('imagePath')
+			->with('core', 'caldav/description.png')
+			->willReturn('core/img/caldav/description.png');
+		$this->urlGenerator->expects($this->once())->method('getAbsoluteURL')
+			->with('core/img/caldav/description.png')
+			->willReturn('https://example.com/core/img/caldav/description.png');
+		$template
+			->expects($this->once())
+			->method('addBodyListItem')
+			->with(
+				'This is a note to the recipient',
+				'Note:',
+				'https://example.com/core/img/caldav/description.png',
+				'This is a note to the recipient'
 			);
 		$template
 			->expects($this->once())
 			->method('addBodyButton')
 			->with(
-				'Open »file.txt«',
+				'Open file.txt',
 				'https://example.com/file.txt'
 			);
 		$message
@@ -1459,7 +1473,7 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('setSubject')
-			->with('Mrs. Owner User shared »file.txt« with you');
+			->with('Mrs. Owner User shared file.txt with you');
 		$message
 			->expects($this->once())
 			->method('useTemplate')
@@ -1495,6 +1509,138 @@ class ShareByMailProviderTest extends TestCase {
 		);
 	}
 
+	public function testSendMailNotificationWithSameUserAndUserEmailAndExpiration() {
+		$provider = $this->getInstance();
+		$user = $this->createMock(IUser::class);
+		$this->settingsManager->expects($this->any())->method('replyToInitiator')->willReturn(true);
+		$this->userManager
+			->expects($this->once())
+			->method('get')
+			->with('OwnerUser')
+			->willReturn($user);
+		$user
+			->expects($this->once())
+			->method('getDisplayName')
+			->willReturn('Mrs. Owner User');
+		$message = $this->createMock(Message::class);
+		$this->mailer
+			->expects($this->once())
+			->method('createMessage')
+			->willReturn($message);
+		$template = $this->createMock(IEMailTemplate::class);
+		$this->mailer
+			->expects($this->once())
+			->method('createEMailTemplate')
+			->willReturn($template);
+		$template
+			->expects($this->once())
+			->method('addHeader');
+		$template
+			->expects($this->once())
+			->method('addHeading')
+			->with('Mrs. Owner User shared file.txt with you');
+		$template
+			->expects($this->once())
+			->method('addBodyText')
+			->with('Mrs. Owner User shared file.txt with you. Click the button below to open it.', 'Mrs. Owner User shared file.txt with you.');
+
+		$expiration = new DateTime('2001-01-01');
+		$this->l->expects($this->once())
+			->method('l')
+			->with('date', $expiration, ['width' => 'medium'])
+			->willReturn('2001-01-01');
+		$this->urlGenerator->expects($this->once())->method('imagePath')
+			->with('core', 'caldav/time.png')
+			->willReturn('core/img/caldav/time.png');
+		$this->urlGenerator->expects($this->once())->method('getAbsoluteURL')
+			->with('core/img/caldav/time.png')
+			->willReturn('https://example.com/core/img/caldav/time.png');
+		$template
+			->expects($this->once())
+			->method('addBodyListItem')
+			->with(
+				'This share is valid until 2001-01-01 at midnight',
+				'Expiration:',
+				'https://example.com/core/img/caldav/time.png',
+			);
+
+		$template
+			->expects($this->once())
+			->method('addBodyButton')
+			->with(
+				'Open file.txt',
+				'https://example.com/file.txt'
+			);
+		$message
+			->expects($this->once())
+			->method('setTo')
+			->with(['john@doe.com']);
+		$this->defaults
+			->expects($this->once())
+			->method('getName')
+			->willReturn('UnitTestCloud');
+		$message
+			->expects($this->once())
+			->method('setFrom')
+			->with([
+				\OCP\Util::getDefaultEmailAddress('UnitTestCloud') => 'Mrs. Owner User via UnitTestCloud'
+			]);
+		$user
+			->expects($this->once())
+			->method('getEMailAddress')
+			->willReturn('owner@example.com');
+		$message
+			->expects($this->once())
+			->method('setReplyTo')
+			->with(['owner@example.com' => 'Mrs. Owner User']);
+		$this->defaults
+			->expects($this->exactly(2))
+			->method('getSlogan')
+			->willReturn('Testing like 1990');
+		$template
+			->expects($this->once())
+			->method('addFooter')
+			->with('UnitTestCloud - Testing like 1990');
+		$template
+			->expects($this->once())
+			->method('setSubject')
+			->with('Mrs. Owner User shared file.txt with you');
+		$message
+			->expects($this->once())
+			->method('useTemplate')
+			->with($template);
+
+		$this->mailer->expects($this->once())
+			->method('validateMailAddress')
+			->willReturn(true);
+		$this->mailer
+			->expects($this->once())
+			->method('send')
+			->with($message);
+
+		$this->urlGenerator->expects($this->once())->method('linkToRouteAbsolute')
+			->with('files_sharing.sharecontroller.showShare', ['token' => 'token'])
+			->willReturn('https://example.com/file.txt');
+
+		$node = $this->getMockBuilder(File::class)->getMock();
+		$node->expects($this->any())->method('getName')->willReturn('file.txt');
+
+		$share = $this->getMockBuilder(IShare::class)->getMock();
+		$share->expects($this->any())->method('getSharedBy')->willReturn('OwnerUser');
+		$share->expects($this->any())->method('getSharedWith')->willReturn('john@doe.com');
+		$share->expects($this->any())->method('getNode')->willReturn($node);
+		$share->expects($this->any())->method('getId')->willReturn(42);
+		$share->expects($this->any())->method('getNote')->willReturn('');
+		$share->expects($this->any())->method('getExpirationDate')->willReturn($expiration);
+		$share->expects($this->any())->method('getToken')->willReturn('token');
+		
+		self::invokePrivate(
+			$provider,
+			'sendMailNotification',
+			[$share]
+		);
+	}
+
 	public function testSendMailNotificationWithDifferentUserAndNoUserEmail() {
 		$provider = $this->getInstance();
 		$initiatorUser = $this->createMock(IUser::class);
@@ -1524,19 +1670,19 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('addHeading')
-			->with('Mr. Initiator User shared »file.txt« with you');
+			->with('Mr. Initiator User shared file.txt with you');
 		$template
 			->expects($this->once())
 			->method('addBodyText')
 			->with(
-				'Mr. Initiator User shared »file.txt« with you. Click the button below to open it.',
-				'Mr. Initiator User shared »file.txt« with you.'
+				'Mr. Initiator User shared file.txt with you. Click the button below to open it.',
+				'Mr. Initiator User shared file.txt with you.'
 			);
 		$template
 			->expects($this->once())
 			->method('addBodyButton')
 			->with(
-				'Open »file.txt«',
+				'Open file.txt',
 				'https://example.com/file.txt'
 			);
 		$message
@@ -1563,7 +1709,7 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('setSubject')
-			->with('Mr. Initiator User shared »file.txt« with you');
+			->with('Mr. Initiator User shared file.txt with you');
 		$message
 			->expects($this->once())
 			->method('useTemplate')
@@ -1628,19 +1774,19 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('addHeading')
-			->with('Mrs. Owner User shared »file.txt« with you');
+			->with('Mrs. Owner User shared file.txt with you');
 		$template
 			->expects($this->once())
 			->method('addBodyText')
 			->with(
-				'Mrs. Owner User shared »file.txt« with you. Click the button below to open it.',
-				'Mrs. Owner User shared »file.txt« with you.'
+				'Mrs. Owner User shared file.txt with you. Click the button below to open it.',
+				'Mrs. Owner User shared file.txt with you.'
 			);
 		$template
 			->expects($this->once())
 			->method('addBodyButton')
 			->with(
-				'Open »file.txt«',
+				'Open file.txt',
 				'https://example.com/file.txt'
 			);
 		$message
@@ -1671,7 +1817,7 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('setSubject')
-			->with('Mrs. Owner User shared »file.txt« with you');
+			->with('Mrs. Owner User shared file.txt with you');
 		$message
 			->expects($this->once())
 			->method('useTemplate')
@@ -1736,19 +1882,19 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('addHeading')
-			->with('Mr. Initiator User shared »file.txt« with you');
+			->with('Mr. Initiator User shared file.txt with you');
 		$template
 			->expects($this->once())
 			->method('addBodyText')
 			->with(
-				'Mr. Initiator User shared »file.txt« with you. Click the button below to open it.',
-				'Mr. Initiator User shared »file.txt« with you.'
+				'Mr. Initiator User shared file.txt with you. Click the button below to open it.',
+				'Mr. Initiator User shared file.txt with you.'
 			);
 		$template
 			->expects($this->once())
 			->method('addBodyButton')
 			->with(
-				'Open »file.txt«',
+				'Open file.txt',
 				'https://example.com/file.txt'
 			);
 		$message
@@ -1775,7 +1921,7 @@ class ShareByMailProviderTest extends TestCase {
 		$template
 			->expects($this->once())
 			->method('setSubject')
-			->with('Mr. Initiator User shared »file.txt« with you');
+			->with('Mr. Initiator User shared file.txt with you');
 		$message
 			->expects($this->once())
 			->method('useTemplate')