From 7fedd33825dc9eb2f3f9bddbc1b3f4301859206f Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Wed, 2 Sep 2020 12:25:02 +0200 Subject: [PATCH] Better cleanup routine for statuses Signed-off-by: Georg Ehrke --- .../ClearOldStatusesBackgroundJob.php | 6 ++- apps/user_status/lib/Db/UserStatusMapper.php | 22 +++++++- .../user_status/lib/Service/StatusService.php | 16 ++++++ .../ClearOldStatusesBackgroundJobTest.php | 3 ++ .../tests/Unit/Db/UserStatusMapperTest.php | 52 ++++++++++++++++++- .../tests/Unit/Service/StatusServiceTest.php | 23 +++++++- 6 files changed, 117 insertions(+), 5 deletions(-) diff --git a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php index aa6202de43a..3077fbec099 100644 --- a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php +++ b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php @@ -26,6 +26,7 @@ declare(strict_types=1); namespace OCA\UserStatus\BackgroundJob; use OCA\UserStatus\Db\UserStatusMapper; +use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; @@ -58,6 +59,9 @@ class ClearOldStatusesBackgroundJob extends TimedJob { * @inheritDoc */ protected function run($argument) { - $this->mapper->clearMessagesOlderThan($this->time->getTime()); + $now = $this->time->getTime(); + + $this->mapper->clearMessagesOlderThan($now); + $this->mapper->clearStatusesOlderThan($now - StatusService::INVALIDATE_STATUS_THRESHOLD, $now); } } diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 0f3693a4d21..3bae5b37807 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -25,6 +25,7 @@ declare(strict_types=1); namespace OCA\UserStatus\Db; +use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Db\QBMapper; use OCP\DB\QueryBuilder\IQueryBuilder; use OCP\IDBConnection; @@ -81,7 +82,7 @@ class UserStatusMapper extends QBMapper { ->select('*') ->from($this->tableName) ->orderBy('status_timestamp', 'DESC') - ->where($qb->expr()->notIn('status', $qb->createNamedParameter(['online', 'away'], IQueryBuilder::PARAM_STR_ARRAY))) + ->where($qb->expr()->notIn('status', $qb->createNamedParameter([StatusService::ONLINE, StatusService::AWAY], IQueryBuilder::PARAM_STR_ARRAY))) ->orWhere($qb->expr()->isNotNull('message_id')) ->orWhere($qb->expr()->isNotNull('custom_icon')) ->orWhere($qb->expr()->isNotNull('custom_message')); @@ -125,6 +126,25 @@ class UserStatusMapper extends QBMapper { return $this->findEntities($qb); } + /** + * @param int $olderThan + * @param int $now + */ + public function clearStatusesOlderThan(int $olderThan, int $now): void { + $qb = $this->db->getQueryBuilder(); + $qb->update($this->tableName) + ->set('status', $qb->createNamedParameter(StatusService::OFFLINE)) + ->set('is_user_defined', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)) + ->set('status_timestamp', $qb->createNamedParameter($now, IQueryBuilder::PARAM_INT)) + ->where($qb->expr()->lte('status_timestamp', $qb->createNamedParameter($olderThan, IQueryBuilder::PARAM_INT))) + ->andWhere($qb->expr()->orX( + $qb->expr()->eq('is_user_defined', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL), IQueryBuilder::PARAM_BOOL), + $qb->expr()->eq('status', $qb->createNamedParameter(StatusService::ONLINE)) + )); + + $qb->execute(); + } + /** * Clear all statuses older than a given timestamp * diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 06441f60059..1e1e5b1fcc5 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -341,6 +341,11 @@ class StatusService { */ private function processStatus(UserStatus $status): UserStatus { $clearAt = $status->getClearAt(); + + if ($status->getStatusTimestamp() < $this->timeFactory->getTime() - self::INVALIDATE_STATUS_THRESHOLD + && (!$status->getIsUserDefined() || $status->getStatus() === self::ONLINE)) { + $this->cleanStatus($status); + } if ($clearAt !== null && $clearAt < $this->timeFactory->getTime()) { $this->cleanStatusMessage($status); } @@ -351,6 +356,17 @@ class StatusService { return $status; } + /** + * @param UserStatus $status + */ + private function cleanStatus(UserStatus $status): void { + $status->setStatus(self::OFFLINE); + $status->setStatusTimestamp($this->timeFactory->getTime()); + $status->setIsUserDefined(false); + + $this->mapper->update($status); + } + /** * @param UserStatus $status */ diff --git a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php index a89d0e270fa..ccb649ecce3 100644 --- a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php +++ b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php @@ -54,6 +54,9 @@ class ClearOldStatusesBackgroundJobTest extends TestCase { $this->mapper->expects($this->once()) ->method('clearMessagesOlderThan') ->with(1337); + $this->mapper->expects($this->once()) + ->method('clearStatusesOlderThan') + ->with(1037, 1337); $this->time->method('getTime') ->willReturn(1337); diff --git a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php index de05d62c227..44ffa75c440 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -152,7 +152,57 @@ class UserStatusMapperTest extends TestCase { $this->mapper->insert($userStatus2); } - public function testClearOlderThan(): void { + /** + * @param string $status + * @param bool $isUserDefined + * @param int $timestamp + * @param bool $expectsClean + * + * @dataProvider clearStatusesOlderThanDataProvider + */ + public function testClearStatusesOlderThan(string $status, bool $isUserDefined, int $timestamp, bool $expectsClean): void { + $oldStatus = UserStatus::fromParams([ + 'userId' => 'john.doe', + 'status' => $status, + 'isUserDefined' => $isUserDefined, + 'statusTimestamp' => $timestamp, + ]); + + $this->mapper->insert($oldStatus); + + $this->mapper->clearStatusesOlderThan(5000, 8000); + + $updatedStatus = $this->mapper->findAll()[0]; + + if ($expectsClean) { + $this->assertEquals('offline', $updatedStatus->getStatus()); + $this->assertFalse($updatedStatus->getIsUserDefined()); + $this->assertEquals(8000, $updatedStatus->getStatusTimestamp()); + } else { + $this->assertEquals($status, $updatedStatus->getStatus()); + $this->assertEquals($isUserDefined, $updatedStatus->getIsUserDefined()); + $this->assertEquals($timestamp, $updatedStatus->getStatusTimestamp()); + } + } + + public function clearStatusesOlderThanDataProvider(): array { + return [ + ['online', true, 6000, false], + ['online', true, 4000, true], + ['online', false, 6000, false], + ['online', false, 4000, true], + ['away', true, 6000, false], + ['away', true, 4000, false], + ['away', false, 6000, false], + ['away', false, 4000, true], + ['dnd', true, 6000, false], + ['dnd', true, 4000, false], + ['invisible', true, 6000, false], + ['invisible', true, 4000, false], + ]; + } + + public function testClearMessagesOlderThan(): void { $this->insertSampleStatuses(); $this->mapper->clearMessagesOlderThan(55000); diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php index d9d0b84750f..4f47070e7c1 100644 --- a/apps/user_status/tests/Unit/Service/StatusServiceTest.php +++ b/apps/user_status/tests/Unit/Service/StatusServiceTest.php @@ -146,12 +146,31 @@ class StatusServiceTest extends TestCase { } public function testFindAllClearStatus(): void { + $status = new UserStatus(); + $status->setStatus('online'); + $status->setStatusTimestamp(1000); + $status->setIsUserDefined(true); + + $this->timeFactory->method('getTime') + ->willReturn(1400); + $this->mapper->expects($this->once()) + ->method('findByUserId') + ->with('john.doe') + ->willReturn($status); + + $this->assertEquals($status, $this->service->findByUserId('john.doe')); + $this->assertEquals('offline', $status->getStatus()); + $this->assertEquals(1400, $status->getStatusTimestamp()); + $this->assertFalse($status->getIsUserDefined()); + } + + public function testFindAllClearMessage(): void { $status = new UserStatus(); $status->setClearAt(50); $status->setMessageId('commuting'); + $status->setStatusTimestamp(60); - $this->timeFactory->expects($this->once()) - ->method('getTime') + $this->timeFactory->method('getTime') ->willReturn(60); $this->predefinedStatusService->expects($this->never()) ->method('getDefaultStatusById'); -- 2.39.5