From 96b2d57ca45cc2f7207bd50da7e939848b518954 Mon Sep 17 00:00:00 2001 From: skjnldsv Date: Fri, 2 Aug 2024 15:28:09 +0200 Subject: [PATCH] feat(sharebymail): improve share email format Signed-off-by: skjnldsv --- apps/sharebymail/lib/ShareByMailProvider.php | 53 +++-- .../tests/ShareByMailProviderTest.php | 200 +++++++++++++++--- 2 files changed, 210 insertions(+), 43 deletions(-) diff --git a/apps/sharebymail/lib/ShareByMailProvider.php b/apps/sharebymail/lib/ShareByMailProvider.php index ab9433f51c830..b21f6440c648f 100644 --- a/apps/sharebymail/lib/ShareByMailProvider.php +++ b/apps/sharebymail/lib/ShareByMailProvider.php @@ -328,21 +328,36 @@ protected function sendEmail(IShare $share, array $emails): void { '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 @@ protected function sendPassword(IShare $share, string $password, array $emails): $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 @@ protected function sendPassword(IShare $share, string $password, array $emails): '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 @@ protected function sendNote(IShare $share): void { $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 @@ protected function sendNote(IShare $share): void { $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 @@ protected function sendPasswordToOwner(IShare $share, string $password): bool { ); } - $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 @@ protected function sendPasswordToOwner(IShare $share, string $password): bool { '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 @@ protected function sendPasswordToOwner(IShare $share, string $password): bool { 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 c7f41176c4413..e03fba723128e 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 @@ public function testSendMailNotificationWithSameUserAndUserEmail() { $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 @@ public function testSendMailNotificationWithSameUserAndUserEmail() { $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 @@ public function testSendMailNotificationWithSameUserAndUserEmailAndNote() { $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 @@ public function testSendMailNotificationWithSameUserAndUserEmailAndNote() { $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 @@ public function testSendMailNotificationWithSameUserAndUserEmailAndNote() { ); } + 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 @@ public function testSendMailNotificationWithDifferentUserAndNoUserEmail() { $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 @@ public function testSendMailNotificationWithDifferentUserAndNoUserEmail() { $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 @@ public function testSendMailNotificationWithSameUserAndUserEmailAndReplyToDesact $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 @@ public function testSendMailNotificationWithSameUserAndUserEmailAndReplyToDesact $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 @@ public function testSendMailNotificationWithDifferentUserAndNoUserEmailAndReplyT $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 @@ public function testSendMailNotificationWithDifferentUserAndNoUserEmailAndReplyT $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')