From e3ad99d2523e8614b6d84765fc16104b86e9730a Mon Sep 17 00:00:00 2001 From: Lukas Reschke Date: Fri, 10 Apr 2015 17:21:52 +0200 Subject: [PATCH] Add "Reply-To" support to sharing mails and refactor code --- core/ajax/share.php | 18 +- lib/private/share/mailnotifications.php | 158 +++++++++------ tests/lib/mail/message.php | 17 ++ tests/lib/share/MailNotificationsTest.php | 237 ++++++++++++++++++++++ 4 files changed, 369 insertions(+), 61 deletions(-) create mode 100644 tests/lib/share/MailNotificationsTest.php diff --git a/core/ajax/share.php b/core/ajax/share.php index bc83c41642..d9bf97d646 100644 --- a/core/ajax/share.php +++ b/core/ajax/share.php @@ -119,7 +119,14 @@ if (isset($_POST['action']) && isset($_POST['itemType']) && isset($_POST['itemSo // don't send a mail to the user who shared the file $recipientList = array_diff($recipientList, array(\OCP\User::getUser())); - $mailNotification = new OC\Share\MailNotifications(); + $mailNotification = new \OC\Share\MailNotifications( + \OC::$server->getUserSession()->getUser()->getUID(), + \OC::$server->getConfig(), + \OC::$server->getL10N('lib'), + \OC::$server->getMailer(), + \OC::$server->getLogger(), + $defaults + ); $result = $mailNotification->sendInternalShareMail($recipientList, $itemSource, $itemType); \OCP\Share::setSendMailStatus($itemType, $itemSource, $shareType, $recipient, true); @@ -151,7 +158,14 @@ if (isset($_POST['action']) && isset($_POST['itemType']) && isset($_POST['itemSo $file = (string)$_POST['file']; $to_address = (string)$_POST['toaddress']; - $mailNotification = new \OC\Share\MailNotifications(); + $mailNotification = new \OC\Share\MailNotifications( + \OC::$server->getUserSession()->getUser()->getUID(), + \OC::$server->getConfig(), + \OC::$server->getL10N('lib'), + \OC::$server->getMailer(), + \OC::$server->getLogger(), + $defaults + ); $expiration = null; if (isset($_POST['expiration']) && $_POST['expiration'] !== '') { diff --git a/lib/private/share/mailnotifications.php b/lib/private/share/mailnotifications.php index 68f2e66a09..1c15b6e3e1 100644 --- a/lib/private/share/mailnotifications.php +++ b/lib/private/share/mailnotifications.php @@ -6,6 +6,7 @@ * @author Robin McCorkell * @author Thomas Müller * @author Tom Needham + * @author Lukas Reschke * * @copyright Copyright (c) 2015, ownCloud, Inc. * @license AGPL-3.0 @@ -27,47 +28,59 @@ namespace OC\Share; use DateTime; +use OCP\IConfig; +use OCP\IL10N; +use OCP\Mail\IMailer; +use OCP\ILogger; +use OCP\Defaults; +/** + * Class MailNotifications + * + * @package OC\Share + */ class MailNotifications { - /** - * sender userId - * @var null|string - */ - private $senderId; - - /** - * sender email address - * @var string - */ - private $from; - - /** - * @var string - */ + /** @var string sender userId */ + private $userId; + /** @var string sender email address */ + private $replyTo; + /** @var string */ private $senderDisplayName; - - /** - * @var \OC_L10N - */ + /** @var IL10N */ private $l; + /** @var IConfig */ + private $config; + /** @var IMailer */ + private $mailer; + /** @var Defaults */ + private $defaults; + /** @var ILogger */ + private $logger; /** - * - * @param string $sender user id (if nothing is set we use the currently logged-in user) + * @param string $uid user id + * @param IConfig $config + * @param IL10N $l10n + * @param IMailer $mailer + * @param ILogger $logger + * @param Defaults $defaults */ - public function __construct($sender = null) { - $this->l = \OC::$server->getL10N('lib'); + public function __construct($uid, + IConfig $config, + IL10N $l10n, + IMailer $mailer, + ILogger $logger, + Defaults $defaults) { + $this->l = $l10n; + $this->userId = $uid; + $this->config = $config; + $this->mailer = $mailer; + $this->logger = $logger; + $this->defaults = $defaults; - $this->senderId = $sender; - - $this->from = \OCP\Util::getDefaultEmailAddress('sharing-noreply'); - if ($this->senderId) { - $this->from = \OCP\Config::getUserValue($this->senderId, 'settings', 'email', $this->from); - $this->senderDisplayName = \OCP\User::getDisplayName($this->senderId); - } else { - $this->senderDisplayName = \OCP\User::getDisplayName(); - } + $this->replyTo = $this->config->getUserValue($this->userId, 'settings', 'email', null); + $this->senderDisplayName = \OCP\User::getDisplayName($this->userId); } /** @@ -79,12 +92,11 @@ class MailNotifications { * @return array list of user to whom the mail send operation failed */ public function sendInternalShareMail($recipientList, $itemSource, $itemType) { - - $noMail = array(); + $noMail = []; foreach ($recipientList as $recipient) { $recipientDisplayName = \OCP\User::getDisplayName($recipient); - $to = \OC::$server->getConfig()->getUserValue($recipient, 'settings', 'email', ''); + $to = $this->config->getUserValue($recipient, 'settings', 'email', ''); if ($to === '') { $noMail[] = $recipientDisplayName; @@ -100,7 +112,7 @@ class MailNotifications { $date = new DateTime($items[0]['expiration']); $expiration = $date->getTimestamp(); } catch (\Exception $e) { - \OCP\Util::writeLog('sharing', "Couldn't read date: " . $e->getMessage(), \OCP\Util::ERROR); + $this->logger->error("Couldn't read date: ".$e->getMessage(), ['app' => 'sharing']); } } @@ -119,13 +131,29 @@ class MailNotifications { $link = \OCP\Util::linkToAbsolute('files', 'index.php', $args); - list($htmlMail, $alttextMail) = $this->createMailBody($filename, $link, $expiration); + list($htmlBody, $textBody) = $this->createMailBody($filename, $link, $expiration); // send it out now try { - \OCP\Util::sendMail($to, $recipientDisplayName, $subject, $htmlMail, $this->from, $this->senderDisplayName, 1, $alttextMail); + $message = $this->mailer->createMessage(); + $message->setSubject($subject); + $message->setTo([$to => $recipientDisplayName]); + $message->setHtmlBody($htmlBody); + $message->setPlainBody($textBody); + $message->setFrom([ + \OCP\Util::getDefaultEmailAddress('sharing-noreply') => + (string)$this->l->t('%s via %s', [ + $this->senderDisplayName, + $this->defaults->getName() + ]), + ]); + if(!is_null($this->replyTo)) { + $message->setReplyTo([$this->replyTo]); + } + + $this->mailer->send($message); } catch (\Exception $e) { - \OCP\Util::writeLog('sharing', "Can't send mail to inform the user about an internal share: " . $e->getMessage() , \OCP\Util::ERROR); + $this->logger->error("Can't send mail to inform the user about an internal share: ".$e->getMessage(), ['app' => 'sharing']); $noMail[] = $recipientDisplayName; } } @@ -144,19 +172,31 @@ class MailNotifications { * @return array $result of failed recipients */ public function sendLinkShareMail($recipient, $filename, $link, $expiration) { - $subject = (string)$this->l->t('%s shared »%s« with you', array($this->senderDisplayName, $filename)); - list($htmlMail, $alttextMail) = $this->createMailBody($filename, $link, $expiration); - $rs = explode(' ', $recipient); - $failed = array(); - foreach ($rs as $r) { - try { - \OCP\Util::sendMail($r, $r, $subject, $htmlMail, $this->from, $this->senderDisplayName, 1, $alttextMail); - } catch (\Exception $e) { - \OCP\Util::writeLog('sharing', "Can't send mail with public link to $r: " . $e->getMessage(), \OCP\Util::ERROR); - $failed[] = $r; + $subject = (string)$this->l->t('%s shared »%s« with you', [$this->senderDisplayName, $filename]); + list($htmlBody, $textBody) = $this->createMailBody($filename, $link, $expiration); + + try { + $message = $this->mailer->createMessage(); + $message->setSubject($subject); + $message->setTo([$recipient]); + $message->setHtmlBody($htmlBody); + $message->setPlainBody($textBody); + $message->setFrom([ + \OCP\Util::getDefaultEmailAddress('sharing-noreply') => + (string)$this->l->t('%s via %s', [ + $this->senderDisplayName, + $this->defaults->getName() + ]), + ]); + if(!is_null($this->replyTo)) { + $message->setReplyTo([$this->replyTo]); } + + return $this->mailer->send($message); + } catch (\Exception $e) { + $this->logger->error("Can't send mail with public link to $recipient: ".$e->getMessage(), ['app' => 'sharing']); + return [$recipient]; } - return $failed; } /** @@ -169,23 +209,23 @@ class MailNotifications { */ private function createMailBody($filename, $link, $expiration) { - $formatedDate = $expiration ? $this->l->l('date', $expiration) : null; + $formattedDate = $expiration ? $this->l->l('date', $expiration) : null; $html = new \OC_Template("core", "mail", ""); $html->assign ('link', $link); $html->assign ('user_displayname', $this->senderDisplayName); $html->assign ('filename', $filename); - $html->assign('expiration', $formatedDate); + $html->assign('expiration', $formattedDate); $htmlMail = $html->fetchPage(); - $alttext = new \OC_Template("core", "altmail", ""); - $alttext->assign ('link', $link); - $alttext->assign ('user_displayname', $this->senderDisplayName); - $alttext->assign ('filename', $filename); - $alttext->assign('expiration', $formatedDate); - $alttextMail = $alttext->fetchPage(); + $plainText = new \OC_Template("core", "altmail", ""); + $plainText->assign ('link', $link); + $plainText->assign ('user_displayname', $this->senderDisplayName); + $plainText->assign ('filename', $filename); + $plainText->assign('expiration', $formattedDate); + $plainTextMail = $plainText->fetchPage(); - return array($htmlMail, $alttextMail); + return [$htmlMail, $plainTextMail]; } } diff --git a/tests/lib/mail/message.php b/tests/lib/mail/message.php index 0db2017d81..c75cc18b65 100644 --- a/tests/lib/mail/message.php +++ b/tests/lib/mail/message.php @@ -62,6 +62,23 @@ class MessageTest extends TestCase { $this->assertSame(array('lukas@owncloud.com'), $this->message->getFrom()); } + public function testSetReplyTo() { + $this->swiftMessage + ->expects($this->once()) + ->method('setReplyTo') + ->with(['lukas@owncloud.com']); + $this->message->setReplyTo(['lukas@owncloud.com']); + } + + public function testGetReplyTo() { + $this->swiftMessage + ->expects($this->once()) + ->method('getReplyTo') + ->will($this->returnValue(['lukas@owncloud.com'])); + + $this->assertSame(['lukas@owncloud.com'], $this->message->getReplyTo()); + } + public function testSetTo() { $this->swiftMessage ->expects($this->once()) diff --git a/tests/lib/share/MailNotificationsTest.php b/tests/lib/share/MailNotificationsTest.php new file mode 100644 index 0000000000..c74fe406db --- /dev/null +++ b/tests/lib/share/MailNotificationsTest.php @@ -0,0 +1,237 @@ + + * + * @copyright Copyright (c) 2015, ownCloud, Inc. + * @license AGPL-3.0 + * + * This code is free software: you can redistribute it and/or modify + * it under the terms of the GNU Affero General Public License, version 3, + * as published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU Affero General Public License for more details. + * + * You should have received a copy of the GNU Affero General Public License, version 3, + * along with this program. If not, see + * + */ + +use OC\Share\MailNotifications; +use OCP\IConfig; +use OCP\IL10N; +use OCP\Mail\IMailer; +use OCP\ILogger; +use OCP\Defaults; + +/** + * Class MailNotificationsTest + */ +class MailNotificationsTest extends \Test\TestCase { + /** @var IConfig */ + private $config; + /** @var IL10N */ + private $l10n; + /** @var IMailer */ + private $mailer; + /** @var ILogger */ + private $logger; + /** @var Defaults */ + private $defaults; + + + public function setUp() { + parent::setUp(); + + $this->config = $this->getMockBuilder('\OCP\IConfig') + ->disableOriginalConstructor()->getMock(); + $this->l10n = $this->getMockBuilder('\OCP\IL10N') + ->disableOriginalConstructor()->getMock(); + $this->mailer = $this->getMockBuilder('\OCP\Mail\IMailer') + ->disableOriginalConstructor()->getMock(); + $this->logger = $this->getMockBuilder('\OCP\ILogger') + ->disableOriginalConstructor()->getMock(); + $this->defaults = $this->getMockBuilder('\OCP\Defaults') + ->disableOriginalConstructor()->getMock(); + + $this->l10n->expects($this->any()) + ->method('t') + ->will($this->returnCallback(function($text, $parameters = array()) { + return vsprintf($text, $parameters); + })); + } + + public function testSendLinkShareMailWithoutReplyTo() { + $message = $this->getMockBuilder('\OC\Mail\Message') + ->disableOriginalConstructor()->getMock(); + + $message + ->expects($this->once()) + ->method('setSubject') + ->with('TestUser shared »MyFile« with you'); + $message + ->expects($this->once()) + ->method('setTo') + ->with(['lukas@owncloud.com']); + $message + ->expects($this->once()) + ->method('setHtmlBody'); + $message + ->expects($this->once()) + ->method('setPlainBody'); + $message + ->expects($this->once()) + ->method('setFrom') + ->with([\OCP\Util::getDefaultEmailAddress('sharing-noreply') => 'TestUser via UnitTestCloud']); + + $this->mailer + ->expects($this->once()) + ->method('createMessage') + ->will($this->returnValue($message)); + $this->mailer + ->expects($this->once()) + ->method('send') + ->with($message) + ->will($this->returnValue([])); + + $this->defaults + ->expects($this->once()) + ->method('getName') + ->will($this->returnValue('UnitTestCloud')); + + $this->config + ->expects($this->at(0)) + ->method('getUserValue') + ->with('TestUser', 'settings', 'email', null) + ->will($this->returnValue('sharer@owncloud.com')); + + $mailNotifications = new MailNotifications( + 'TestUser', + $this->config, + $this->l10n, + $this->mailer, + $this->logger, + $this->defaults + ); + + $this->assertSame([], $mailNotifications->sendLinkShareMail('lukas@owncloud.com', 'MyFile', 'https://owncloud.com/file/?foo=bar', 3600)); + } + + public function testSendLinkShareMailWithReplyTo() { + $message = $this->getMockBuilder('\OC\Mail\Message') + ->disableOriginalConstructor()->getMock(); + + $message + ->expects($this->once()) + ->method('setSubject') + ->with('TestUser shared »MyFile« with you'); + $message + ->expects($this->once()) + ->method('setTo') + ->with(['lukas@owncloud.com']); + $message + ->expects($this->once()) + ->method('setHtmlBody'); + $message + ->expects($this->once()) + ->method('setPlainBody'); + $message + ->expects($this->once()) + ->method('setFrom') + ->with([\OCP\Util::getDefaultEmailAddress('sharing-noreply') => 'TestUser via UnitTestCloud']); + $message + ->expects($this->once()) + ->method('setReplyTo') + ->with(['sharer@owncloud.com']); + + $this->mailer + ->expects($this->once()) + ->method('createMessage') + ->will($this->returnValue($message)); + $this->mailer + ->expects($this->once()) + ->method('send') + ->with($message) + ->will($this->returnValue([])); + + $this->defaults + ->expects($this->once()) + ->method('getName') + ->will($this->returnValue('UnitTestCloud')); + + $this->config + ->expects($this->at(0)) + ->method('getUserValue') + ->with('TestUser', 'settings', 'email', null) + ->will($this->returnValue('sharer@owncloud.com')); + + $mailNotifications = new MailNotifications( + 'TestUser', + $this->config, + $this->l10n, + $this->mailer, + $this->logger, + $this->defaults + ); + $this->assertSame([], $mailNotifications->sendLinkShareMail('lukas@owncloud.com', 'MyFile', 'https://owncloud.com/file/?foo=bar', 3600)); + } + + public function testSendLinkShareMailException() { + $message = $this->getMockBuilder('\OC\Mail\Message') + ->disableOriginalConstructor()->getMock(); + + $message + ->expects($this->once()) + ->method('setSubject') + ->with('TestUser shared »MyFile« with you'); + $message + ->expects($this->once()) + ->method('setTo') + ->with(['lukas@owncloud.com']); + $message + ->expects($this->once()) + ->method('setHtmlBody'); + $message + ->expects($this->once()) + ->method('setPlainBody'); + $message + ->expects($this->once()) + ->method('setFrom') + ->with([\OCP\Util::getDefaultEmailAddress('sharing-noreply') => 'TestUser via UnitTestCloud']); + + $this->mailer + ->expects($this->once()) + ->method('createMessage') + ->will($this->returnValue($message)); + $this->mailer + ->expects($this->once()) + ->method('send') + ->with($message) + ->will($this->throwException(new Exception('Some Exception Message'))); + + $this->defaults + ->expects($this->once()) + ->method('getName') + ->will($this->returnValue('UnitTestCloud')); + + $this->config + ->expects($this->at(0)) + ->method('getUserValue') + ->with('TestUser', 'settings', 'email', null) + ->will($this->returnValue('sharer@owncloud.com')); + + $mailNotifications = new MailNotifications( + 'TestUser', + $this->config, + $this->l10n, + $this->mailer, + $this->logger, + $this->defaults + ); + + $this->assertSame(['lukas@owncloud.com'], $mailNotifications->sendLinkShareMail('lukas@owncloud.com', 'MyFile', 'https://owncloud.com/file/?foo=bar', 3600)); + } + +}