Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Skip disabled users and do not break the cronjob on the first unsent email #842

Merged
merged 4 commits into from
Mar 10, 2020
Merged

Skip disabled users and do not break the cronjob on the first unsent email #842

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
408ae82
Do not send actvitiy notifications for disabled users
VicDeo Mar 9, 2020
e232024
We don't need to fix DI
VicDeo Mar 9, 2020
1298bae
Make tests for EmailNotification more DRY
VicDeo Mar 9, 2020
ec9e611
Do not break the whole batch if email was not sent
VicDeo Mar 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 24 additions & 22 deletions lib/BackgroundJob/EmailNotification.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
use OCA\Activity\MailQueueHandler;
use OCP\IConfig;
use OCP\ILogger;
use OCP\IUser;
use OCP\IUserManager;

/**
* Class EmailNotification
Expand All @@ -40,6 +42,9 @@ class EmailNotification extends TimedJob {
/** @var MailQueueHandler */
protected $mqHandler;

/** @var IUserManager */
protected $userManager;

/** @var IConfig */
protected $config;

Expand All @@ -56,29 +61,18 @@ class EmailNotification extends TimedJob {
* @param bool|null $isCLI
*/
public function __construct(MailQueueHandler $mailQueueHandler = null,
IUserManager $userManager,
IConfig $config = null,
ILogger $logger = null,
$isCLI = null) {
// Run all 15 Minutes
$this->setInterval(15 * 60);

if ($mailQueueHandler === null || $config === null || $logger === null || $isCLI === null) {
$this->fixDIForJobs();
} else {
$this->mqHandler = $mailQueueHandler;
$this->config = $config;
$this->logger = $logger;
$this->isCLI = $isCLI;
}
}

protected function fixDIForJobs() {
$application = new Application();

$this->mqHandler = $application->getContainer()->query('MailQueueHandler');
$this->config = \OC::$server->getConfig();
$this->logger = \OC::$server->getLogger();
$this->isCLI = \OC::$CLI;
$this->mqHandler = $mailQueueHandler;
$this->userManager = $userManager;
$this->config = $config;
$this->logger = $logger;
$this->isCLI = $isCLI;
}

protected function run($argument) {
Expand Down Expand Up @@ -129,7 +123,11 @@ protected function runStep($limit, $sendTime) {

foreach ($affectedUsers as $user) {
$uid = $user['uid'];
if (empty($user['email'])) {
$userObject = $this->userManager->get($uid);
if (empty($user['email'])
|| !$userObject instanceof IUser
|| $userObject->isEnabled() === false
) {
// The user did not setup an email address
// So we will not send an email but still discard the queue entries
$this->logger->debug("Couldn't send notification email to user '$uid' (email address isn't set for that user)", ['app' => 'activity']);
Expand All @@ -144,10 +142,14 @@ protected function runStep($limit, $sendTime) {
$this->mqHandler->sendEmailToUser($uid, $user['email'], $language, $timezone, $sendTime);
$sentMailForUsers[] = $uid;
} catch (\Exception $e) {
// Run cleanup before we die
$this->mqHandler->deleteSentItems($sentMailForUsers, $sendTime);
// Throw the exception again - which gets logged by core and the job is handled appropriately
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the cleanup not needed anymore?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't rethrow exception any more which means that we always reach the line 155 now
https://github.com/owncloud/activity/pull/842/files#diff-35fc1857b1cf16ed158a97c77089f80cL155

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, didn't see that,
👍

throw $e;
$this->logger->warning(
"Couldn't send notification email to user {user} ({reason}})",
[
'app' => 'activity',
'user' => $uid,
'reason' => $e->getMessage()
]
);
}
}

Expand Down
109 changes: 63 additions & 46 deletions tests/Unit/BackgroundJob/EmailNotificationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,14 @@
namespace OCA\Activity\Tests\BackgroundJob;

use OCA\Activity\BackgroundJob\EmailNotification;
use OCA\Activity\MailQueueHandler;
use OCA\Activity\Tests\Unit\TestCase;
use OCP\BackgroundJob\IJobList;
use OCP\IConfig;
use OCP\ILogger;
use OCP\IUser;
use OCP\IUserManager;
use PHPUnit\Framework\MockObject\MockObject;

/**
* Class EmailNotificationTest
Expand All @@ -31,11 +38,44 @@
* @package OCA\Activity\Tests\BackgroundJob
*/
class EmailNotificationTest extends TestCase {
/** @var MailQueueHandler | MockObject */
private $mqHandler;

/** @var IUserManager | MockObject */
private $userManager;

/** @var IConfig | MockObject */
private $config;

/** @var ILogger | MockObject */
private $logger;

protected function setUp(): void {
parent::setUp();
$this->mqHandler = $this->createMock(MailQueueHandler::class);
$this->userManager = $this->createMock(IUserManager::class);
$this->config = $this->createMock(IConfig::class);
$this->logger = $this->createMock(ILogger::class);
}

/**
* @param bool $isCLI
* @return EmailNotification
*/
protected function getEmailNotification($isCLI) {
return new EmailNotification(
$this->mqHandler,
$this->userManager,
$this->config,
$this->logger,
$isCLI
);
}

public function constructAndRunData() {
return [
[true],
[false],
[null],
[false]
];
}

Expand All @@ -45,45 +85,26 @@ public function constructAndRunData() {
* @param bool $isCLI
*/
public function testConstructAndRun($isCLI) {
$backgroundJob = new EmailNotification(
$this->getMockBuilder('OCA\Activity\MailQueueHandler')->disableOriginalConstructor()->getMock(),
$this->createMock('OCP\IConfig'),
$this->createMock('OCP\ILogger'),
$isCLI
);

$jobList = $this->createMock('\OCP\BackgroundJob\IJobList');
$backgroundJob = $this->getEmailNotification($isCLI);
$jobList = $this->createMock(IJobList::class);

/** @var \OC\BackgroundJob\JobList $jobList */
/** @var JobList $jobList */
$backgroundJob->execute($jobList);
$this->assertTrue(true);
}

public function testRunStep() {
$mailQueueHandler = $this->getMockBuilder('OCA\Activity\MailQueueHandler')
->disableOriginalConstructor()
->getMock();
$config = $this->getMockBuilder('OCP\IConfig')
->disableOriginalConstructor()
->getMock();
$backgroundJob = new EmailNotification(
$mailQueueHandler,
$config,
$this->createMock('OCP\ILogger'),
true
);

$mailQueueHandler->expects($this->any())
$this->mqHandler->expects($this->any())
->method('getAffectedUsers')
->with(2, 200)
->willReturn([
['uid' => 'test1', 'email' => 'test1@localhost'],
['uid' => 'test2', 'email' => ''],
]);
$mailQueueHandler->expects($this->once())
$this->mqHandler->expects($this->once())
->method('sendEmailToUser')
->with('test1', 'test1@localhost', 'de', \date_default_timezone_get(), $this->anything());
$config->expects($this->any())
$this->config->expects($this->any())
->method('getUserValueForUsers')
->willReturnMap([
['core', 'lang', [
Expand All @@ -94,42 +115,32 @@ public function testRunStep() {
'test2' => 'en',
]]
]);
$fakeUser = $this->createMock(IUser::class);
$fakeUser->expects($this->once())->method('isEnabled')
->willReturn(true);
$this->userManager->method('get')->willReturn($fakeUser);

$backgroundJob = $this->getEmailNotification(true);
$this->assertEquals(2, $this->invokePrivate($backgroundJob, 'runStep', [2, 200]));
}

/**
* Test run where all emails fail to send - cleanup should error
*/
public function testRunStepWhereEmailThrowsException() {
$this->expectException(\Exception::class);

$mailQueueHandler = $this->getMockBuilder('OCA\Activity\MailQueueHandler')
->disableOriginalConstructor()
->getMock();
$config = $this->getMockBuilder('OCP\IConfig')
->disableOriginalConstructor()
->getMock();
$backgroundJob = new EmailNotification(
$mailQueueHandler,
$config,
$this->createMock('OCP\ILogger'),
true
);

$mailQueueHandler->expects($this->any())
$this->mqHandler->expects($this->any())
->method('getAffectedUsers')
->with(2, 200)
->willReturn([
['uid' => 'test1', 'email' => 'test1@localhost'],
]);
$e = new \Exception();
// Sending the email will throw an exception
$mailQueueHandler->expects($this->once())
$this->mqHandler->expects($this->once())
->method('sendEmailToUser')
->with('test1', 'test1@localhost', 'de', \date_default_timezone_get(), $this->anything())
->willThrowException($e);
$config->expects($this->any())
$this->config->expects($this->any())
->method('getUserValueForUsers')
->willReturnMap([
['core', 'lang', [
Expand All @@ -138,11 +149,17 @@ public function testRunStepWhereEmailThrowsException() {
'test1' => 'de'
]]
]);
$fakeUser = $this->createMock(IUser::class);
$fakeUser->expects($this->once())->method('isEnabled')
->willReturn(true);
$this->userManager->method('get')->willReturn($fakeUser);
$this->logger->expects($this->once())
->method('warning');

// Cleanup will be performed, but should now handle having no users supplied to it
// This deals with the case that the first email in the queue throws
// an exception that we cannot handle.

$backgroundJob = $this->getEmailNotification(true);
$this->assertEquals(1, $this->invokePrivate($backgroundJob, 'runStep', [2, 200]));
}
}