From a352a7c7f38d7b1e34caafbbd53ef1e2c28c1caa Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Wed, 2 Sep 2020 11:49:14 +0200 Subject: [PATCH 1/6] Introduce consts for statuses Signed-off-by: Georg Ehrke --- .../ClearOldStatusesBackgroundJob.php | 2 +- apps/user_status/lib/Connector/UserStatus.php | 5 +- .../lib/Controller/HeartbeatController.php | 3 +- .../lib/Controller/StatusesController.php | 4 +- .../lib/Dashboard/UserStatusWidget.php | 4 +- apps/user_status/lib/Db/UserStatusMapper.php | 2 +- .../lib/Listener/UserLiveStatusListener.php | 28 ++-------- .../user_status/lib/Service/JSDataService.php | 2 +- .../user_status/lib/Service/StatusService.php | 53 +++++++++++++------ .../ClearOldStatusesBackgroundJobTest.php | 2 +- .../tests/Unit/Db/UserStatusMapperTest.php | 2 +- 11 files changed, 57 insertions(+), 50 deletions(-) diff --git a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php index 4063904884..aa6202de43 100644 --- a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php +++ b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php @@ -58,6 +58,6 @@ class ClearOldStatusesBackgroundJob extends TimedJob { * @inheritDoc */ protected function run($argument) { - $this->mapper->clearOlderThan($this->time->getTime()); + $this->mapper->clearMessagesOlderThan($this->time->getTime()); } } diff --git a/apps/user_status/lib/Connector/UserStatus.php b/apps/user_status/lib/Connector/UserStatus.php index acbdac2007..e8ab31bb10 100644 --- a/apps/user_status/lib/Connector/UserStatus.php +++ b/apps/user_status/lib/Connector/UserStatus.php @@ -25,6 +25,7 @@ declare(strict_types=1); namespace OCA\UserStatus\Connector; use DateTimeImmutable; +use OCA\UserStatus\Service\StatusService; use OCP\UserStatus\IUserStatus; use OCA\UserStatus\Db; @@ -56,8 +57,8 @@ class UserStatus implements IUserStatus { $this->message = $status->getCustomMessage(); $this->icon = $status->getCustomIcon(); - if ($status->getStatus() === 'invisible') { - $this->status = 'offline'; + if ($status->getStatus() === StatusService::INVISIBLE) { + $this->status = StatusService::OFFLINE; } if ($status->getClearAt() !== null) { $this->clearAt = DateTimeImmutable::createFromFormat('U', (string)$status->getClearAt()); diff --git a/apps/user_status/lib/Controller/HeartbeatController.php b/apps/user_status/lib/Controller/HeartbeatController.php index fb8259a2ad..e05a12fae8 100644 --- a/apps/user_status/lib/Controller/HeartbeatController.php +++ b/apps/user_status/lib/Controller/HeartbeatController.php @@ -25,6 +25,7 @@ declare(strict_types=1); namespace OCA\UserStatus\Controller; +use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; @@ -70,7 +71,7 @@ class HeartbeatController extends Controller { * @return JSONResponse */ public function heartbeat(string $status): JSONResponse { - if (!\in_array($status, ['online', 'away'])) { + if (!\in_array($status, [StatusService::ONLINE, StatusService::AWAY], true)) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } diff --git a/apps/user_status/lib/Controller/StatusesController.php b/apps/user_status/lib/Controller/StatusesController.php index b707708f46..0391ea1492 100644 --- a/apps/user_status/lib/Controller/StatusesController.php +++ b/apps/user_status/lib/Controller/StatusesController.php @@ -92,8 +92,8 @@ class StatusesController extends OCSController { */ private function formatStatus(UserStatus $status): array { $visibleStatus = $status->getStatus(); - if ($visibleStatus === 'invisible') { - $visibleStatus = 'offline'; + if ($visibleStatus === StatusService::INVISIBLE) { + $visibleStatus = StatusService::OFFLINE; } return [ diff --git a/apps/user_status/lib/Dashboard/UserStatusWidget.php b/apps/user_status/lib/Dashboard/UserStatusWidget.php index 809aa00000..9cd29e3ddb 100644 --- a/apps/user_status/lib/Dashboard/UserStatusWidget.php +++ b/apps/user_status/lib/Dashboard/UserStatusWidget.php @@ -146,7 +146,9 @@ class UserStatusWidget implements IWidget { return [ 'userId' => $status->getUserId(), 'displayName' => $displayName, - 'status' => $status->getStatus() === 'invisible' ? 'offline' : $status->getStatus(), + 'status' => $status->getStatus() === StatusService::INVISIBLE + ? StatusService::OFFLINE + : $status->getStatus(), 'icon' => $status->getCustomIcon(), 'message' => $status->getCustomMessage(), 'timestamp' => $status->getStatusTimestamp(), diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 2bd6a5024d..0f3693a4d2 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -130,7 +130,7 @@ class UserStatusMapper extends QBMapper { * * @param int $timestamp */ - public function clearOlderThan(int $timestamp): void { + public function clearMessagesOlderThan(int $timestamp): void { $qb = $this->db->getQueryBuilder(); $qb->update($this->tableName) ->set('message_id', $qb->createNamedParameter(null)) diff --git a/apps/user_status/lib/Listener/UserLiveStatusListener.php b/apps/user_status/lib/Listener/UserLiveStatusListener.php index ce97841d9a..52e73017d4 100644 --- a/apps/user_status/lib/Listener/UserLiveStatusListener.php +++ b/apps/user_status/lib/Listener/UserLiveStatusListener.php @@ -27,6 +27,7 @@ namespace OCA\UserStatus\Listener; use OCA\UserStatus\Db\UserStatus; use OCA\UserStatus\Db\UserStatusMapper; +use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventListener; @@ -46,25 +47,6 @@ class UserLiveStatusListener implements IEventListener { /** @var ITimeFactory */ private $timeFactory; - /** @var string[] */ - private $priorityOrderedStatuses = [ - 'online', - 'away', - 'dnd', - 'invisible', - 'offline' - ]; - - /** @var string[] */ - private $persistentUserStatuses = [ - 'away', - 'dnd', - 'invisible', - ]; - - /** @var int */ - private $offlineThreshold = 300; - /** * UserLiveStatusListener constructor. * @@ -92,7 +74,7 @@ class UserLiveStatusListener implements IEventListener { } catch (DoesNotExistException $ex) { $userStatus = new UserStatus(); $userStatus->setUserId($user->getUID()); - $userStatus->setStatus('offline'); + $userStatus->setStatus(StatusService::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); } @@ -100,7 +82,7 @@ class UserLiveStatusListener implements IEventListener { // If the status is user-defined and one of the persistent statuses, we // will not override it. if ($userStatus->getIsUserDefined() && - \in_array($userStatus->getStatus(), $this->persistentUserStatuses, true)) { + \in_array($userStatus->getStatus(), StatusService::PERSISTENT_STATUSES, true)) { return; } @@ -108,13 +90,13 @@ class UserLiveStatusListener implements IEventListener { // If the current status is older than 5 minutes, // treat it as outdated and update - if ($userStatus->getStatusTimestamp() < ($this->timeFactory->getTime() - $this->offlineThreshold)) { + if ($userStatus->getStatusTimestamp() < ($this->timeFactory->getTime() - StatusService::INVALIDATE_STATUS_THRESHOLD)) { $needsUpdate = true; } // If the emitted status is more important than the current status // treat it as outdated and update - if (array_search($event->getStatus(), $this->priorityOrderedStatuses) < array_search($userStatus->getStatus(), $this->priorityOrderedStatuses)) { + if (array_search($event->getStatus(), StatusService::PRIORITY_ORDERED_STATUSES) < array_search($userStatus->getStatus(), StatusService::PRIORITY_ORDERED_STATUSES)) { $needsUpdate = true; } diff --git a/apps/user_status/lib/Service/JSDataService.php b/apps/user_status/lib/Service/JSDataService.php index ebe801cd57..334ae248d4 100644 --- a/apps/user_status/lib/Service/JSDataService.php +++ b/apps/user_status/lib/Service/JSDataService.php @@ -65,7 +65,7 @@ class JSDataService implements \JsonSerializable { 'messageIsPredefined' => false, 'icon' => null, 'clearAt' => null, - 'status' => 'offline', + 'status' => StatusService::OFFLINE, 'statusIsUserDefined' => false, ]; } diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index e36d605cee..06441f6005 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -54,17 +54,38 @@ class StatusService { /** @var EmojiService */ private $emojiService; - /** @var string[] */ - private $allowedStatusTypes = [ - 'online', - 'away', - 'dnd', - 'invisible', - 'offline' + public const ONLINE = 'online'; + public const AWAY = 'away'; + public const DND = 'dnd'; + public const INVISIBLE = 'invisible'; + public const OFFLINE = 'offline'; + + /** + * List of priorities ordered by their priority + */ + public const PRIORITY_ORDERED_STATUSES = [ + self::ONLINE, + self::AWAY, + self::DND, + self::INVISIBLE, + self::OFFLINE + ]; + + /** + * List of statuses that persist the clear-up + * or UserLiveStatusEvents + */ + public const PERSISTENT_STATUSES = [ + self::AWAY, + self::DND, + self::INVISIBLE, ]; /** @var int */ - private $maximumMessageLength = 80; + public const INVALIDATE_STATUS_THRESHOLD = 5 /* minutes */ * 60 /* seconds */; + + /** @var int */ + public const MAXIMUM_MESSAGE_LENGTH = 80; /** * StatusService constructor. @@ -145,7 +166,7 @@ class StatusService { } // Check if status-type is valid - if (!\in_array($status, $this->allowedStatusTypes, true)) { + if (!\in_array($status, self::PRIORITY_ORDERED_STATUSES, true)) { throw new InvalidStatusTypeException('Status-type "' . $status . '" is not supported'); } if ($statusTimestamp === null) { @@ -179,7 +200,7 @@ class StatusService { } catch (DoesNotExistException $ex) { $userStatus = new UserStatus(); $userStatus->setUserId($userId); - $userStatus->setStatus('offline'); + $userStatus->setStatus(self::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); } @@ -224,7 +245,7 @@ class StatusService { } catch (DoesNotExistException $ex) { $userStatus = new UserStatus(); $userStatus->setUserId($userId); - $userStatus->setStatus('offline'); + $userStatus->setStatus(self::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); } @@ -234,8 +255,8 @@ class StatusService { throw new InvalidStatusIconException('Status-Icon is longer than one character'); } // Check for maximum length of custom message - if (\mb_strlen($message) > $this->maximumMessageLength) { - throw new StatusMessageTooLongException('Message is longer than supported length of ' . $this->maximumMessageLength . ' characters'); + if (\mb_strlen($message) > self::MAXIMUM_MESSAGE_LENGTH) { + throw new StatusMessageTooLongException('Message is longer than supported length of ' . self::MAXIMUM_MESSAGE_LENGTH . ' characters'); } // Check that clearAt is in the future if ($clearAt !== null && $clearAt < $this->timeFactory->getTime()) { @@ -266,7 +287,7 @@ class StatusService { return false; } - $userStatus->setStatus('offline'); + $userStatus->setStatus(self::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); @@ -321,7 +342,7 @@ class StatusService { private function processStatus(UserStatus $status): UserStatus { $clearAt = $status->getClearAt(); if ($clearAt !== null && $clearAt < $this->timeFactory->getTime()) { - $this->cleanStatus($status); + $this->cleanStatusMessage($status); } if ($status->getMessageId() !== null) { $this->addDefaultMessage($status); @@ -333,7 +354,7 @@ class StatusService { /** * @param UserStatus $status */ - private function cleanStatus(UserStatus $status): void { + private function cleanStatusMessage(UserStatus $status): void { $status->setMessageId(null); $status->setCustomIcon(null); $status->setCustomMessage(null); diff --git a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php index 6c5f15d47e..a89d0e270f 100644 --- a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php +++ b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php @@ -52,7 +52,7 @@ class ClearOldStatusesBackgroundJobTest extends TestCase { public function testRun() { $this->mapper->expects($this->once()) - ->method('clearOlderThan') + ->method('clearMessagesOlderThan') ->with(1337); $this->time->method('getTime') diff --git a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php index bf4c911c81..de05d62c22 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -155,7 +155,7 @@ class UserStatusMapperTest extends TestCase { public function testClearOlderThan(): void { $this->insertSampleStatuses(); - $this->mapper->clearOlderThan(55000); + $this->mapper->clearMessagesOlderThan(55000); $allStatuses = $this->mapper->findAll(); $this->assertCount(3, $allStatuses); From 10df7198fe5a1318afaaf6396b64627f70245070 Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Wed, 2 Sep 2020 12:03:04 +0200 Subject: [PATCH 2/6] HeartbeatController: PHPDoc fixes Signed-off-by: Georg Ehrke --- apps/user_status/lib/Controller/HeartbeatController.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/apps/user_status/lib/Controller/HeartbeatController.php b/apps/user_status/lib/Controller/HeartbeatController.php index e05a12fae8..cc45b0502c 100644 --- a/apps/user_status/lib/Controller/HeartbeatController.php +++ b/apps/user_status/lib/Controller/HeartbeatController.php @@ -52,6 +52,8 @@ class HeartbeatController extends Controller { * @param string $appName * @param IRequest $request * @param IEventDispatcher $eventDispatcher + * @param IUserSession $userSession + * @param ITimeFactory $timeFactory */ public function __construct(string $appName, IRequest $request, From 7fedd33825dc9eb2f3f9bddbc1b3f4301859206f Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Wed, 2 Sep 2020 12:25:02 +0200 Subject: [PATCH 3/6] 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 aa6202de43..3077fbec09 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 0f3693a4d2..3bae5b3780 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 06441f6005..1e1e5b1fcc 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 a89d0e270f..ccb649ecce 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 de05d62c22..44ffa75c44 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 d9d0b84750..4f47070e7c 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'); From 5be49491d31f7f4a972fa5c2b7034b688f318c94 Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Wed, 2 Sep 2020 12:25:54 +0200 Subject: [PATCH 4/6] Do not display Offline statuses in the Dashboard widget Signed-off-by: Georg Ehrke --- apps/user_status/lib/Db/UserStatusMapper.php | 2 +- .../user_status/tests/Unit/Db/UserStatusMapperTest.php | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 3bae5b3780..31156345b4 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -82,7 +82,7 @@ class UserStatusMapper extends QBMapper { ->select('*') ->from($this->tableName) ->orderBy('status_timestamp', 'DESC') - ->where($qb->expr()->notIn('status', $qb->createNamedParameter([StatusService::ONLINE, StatusService::AWAY], IQueryBuilder::PARAM_STR_ARRAY))) + ->where($qb->expr()->notIn('status', $qb->createNamedParameter([StatusService::ONLINE, StatusService::AWAY, StatusService::OFFLINE], IQueryBuilder::PARAM_STR_ARRAY))) ->orWhere($qb->expr()->isNotNull('message_id')) ->orWhere($qb->expr()->isNotNull('custom_icon')) ->orWhere($qb->expr()->isNotNull('custom_message')); diff --git a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php index 44ffa75c44..e86cee6d68 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -70,8 +70,8 @@ class UserStatusMapperTest extends TestCase { $allResults = $this->mapper->findAllRecent(2, 0); $this->assertCount(2, $allResults); - $this->assertEquals('user1', $allResults[0]->getUserId()); - $this->assertEquals('user2', $allResults[1]->getUserId()); + $this->assertEquals('user2', $allResults[0]->getUserId()); + $this->assertEquals('user1', $allResults[1]->getUserId()); } public function testGetFind(): void { @@ -98,7 +98,7 @@ class UserStatusMapperTest extends TestCase { $user2Status = $this->mapper->findByUserId('user2'); $this->assertEquals('user2', $user2Status->getUserId()); $this->assertEquals('away', $user2Status->getStatus()); - $this->assertEquals(5000, $user2Status->getStatusTimestamp()); + $this->assertEquals(6000, $user2Status->getStatusTimestamp()); $this->assertEquals(false, $user2Status->getIsUserDefined()); $this->assertEquals('🏝', $user2Status->getCustomIcon()); $this->assertEquals('On vacation', $user2Status->getCustomMessage()); @@ -123,7 +123,7 @@ class UserStatusMapperTest extends TestCase { $user2Status = $statuses[1]; $this->assertEquals('user2', $user2Status->getUserId()); $this->assertEquals('away', $user2Status->getStatus()); - $this->assertEquals(5000, $user2Status->getStatusTimestamp()); + $this->assertEquals(6000, $user2Status->getStatusTimestamp()); $this->assertEquals(false, $user2Status->getIsUserDefined()); $this->assertEquals('🏝', $user2Status->getCustomIcon()); $this->assertEquals('On vacation', $user2Status->getCustomMessage()); @@ -239,7 +239,7 @@ class UserStatusMapperTest extends TestCase { $userStatus3 = new UserStatus(); $userStatus3->setUserId('user2'); $userStatus3->setStatus('away'); - $userStatus3->setStatusTimestamp(5000); + $userStatus3->setStatusTimestamp(6000); $userStatus3->setIsUserDefined(false); $userStatus3->setCustomIcon('🏝'); $userStatus3->setCustomMessage('On vacation'); From 2146950a711fc49cf48c1ec10230106f5a59d5b9 Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Wed, 2 Sep 2020 14:33:40 +0200 Subject: [PATCH 5/6] Add MigrationStep to add better Indizes Signed-off-by: Georg Ehrke --- apps/user_status/appinfo/info.xml | 2 +- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../Version0002Date20200902144824.php | 57 +++++++++++++++++++ 4 files changed, 60 insertions(+), 1 deletion(-) create mode 100644 apps/user_status/lib/Migration/Version0002Date20200902144824.php diff --git a/apps/user_status/appinfo/info.xml b/apps/user_status/appinfo/info.xml index 46036812f3..26d8113a84 100644 --- a/apps/user_status/appinfo/info.xml +++ b/apps/user_status/appinfo/info.xml @@ -5,7 +5,7 @@ User status User status - 0.0.2 + 1.0.0 agpl Georg Ehrke UserStatus diff --git a/apps/user_status/composer/composer/autoload_classmap.php b/apps/user_status/composer/composer/autoload_classmap.php index 151d834807..ba2ed1f176 100644 --- a/apps/user_status/composer/composer/autoload_classmap.php +++ b/apps/user_status/composer/composer/autoload_classmap.php @@ -27,6 +27,7 @@ return array( 'OCA\\UserStatus\\Listener\\UserDeletedListener' => $baseDir . '/../lib/Listener/UserDeletedListener.php', 'OCA\\UserStatus\\Listener\\UserLiveStatusListener' => $baseDir . '/../lib/Listener/UserLiveStatusListener.php', 'OCA\\UserStatus\\Migration\\Version0001Date20200602134824' => $baseDir . '/../lib/Migration/Version0001Date20200602134824.php', + 'OCA\\UserStatus\\Migration\\Version0002Date20200902144824' => $baseDir . '/../lib/Migration/Version0002Date20200902144824.php', 'OCA\\UserStatus\\Service\\EmojiService' => $baseDir . '/../lib/Service/EmojiService.php', 'OCA\\UserStatus\\Service\\JSDataService' => $baseDir . '/../lib/Service/JSDataService.php', 'OCA\\UserStatus\\Service\\PredefinedStatusService' => $baseDir . '/../lib/Service/PredefinedStatusService.php', diff --git a/apps/user_status/composer/composer/autoload_static.php b/apps/user_status/composer/composer/autoload_static.php index bed8bc364b..fd05c1c162 100644 --- a/apps/user_status/composer/composer/autoload_static.php +++ b/apps/user_status/composer/composer/autoload_static.php @@ -42,6 +42,7 @@ class ComposerStaticInitUserStatus 'OCA\\UserStatus\\Listener\\UserDeletedListener' => __DIR__ . '/..' . '/../lib/Listener/UserDeletedListener.php', 'OCA\\UserStatus\\Listener\\UserLiveStatusListener' => __DIR__ . '/..' . '/../lib/Listener/UserLiveStatusListener.php', 'OCA\\UserStatus\\Migration\\Version0001Date20200602134824' => __DIR__ . '/..' . '/../lib/Migration/Version0001Date20200602134824.php', + 'OCA\\UserStatus\\Migration\\Version0002Date20200902144824' => __DIR__ . '/..' . '/../lib/Migration/Version0002Date20200902144824.php', 'OCA\\UserStatus\\Service\\EmojiService' => __DIR__ . '/..' . '/../lib/Service/EmojiService.php', 'OCA\\UserStatus\\Service\\JSDataService' => __DIR__ . '/..' . '/../lib/Service/JSDataService.php', 'OCA\\UserStatus\\Service\\PredefinedStatusService' => __DIR__ . '/..' . '/../lib/Service/PredefinedStatusService.php', diff --git a/apps/user_status/lib/Migration/Version0002Date20200902144824.php b/apps/user_status/lib/Migration/Version0002Date20200902144824.php new file mode 100644 index 0000000000..abec418058 --- /dev/null +++ b/apps/user_status/lib/Migration/Version0002Date20200902144824.php @@ -0,0 +1,57 @@ + + * + * @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 + * + */ + +namespace OCA\UserStatus\Migration; + +use OCP\DB\ISchemaWrapper; +use OCP\Migration\IOutput; +use OCP\Migration\SimpleMigrationStep; + +/** + * Class Version0001Date20200602134824 + * + * @package OCA\UserStatus\Migration + */ +class Version0002Date20200902144824 extends SimpleMigrationStep { + + /** + * @param IOutput $output + * @param \Closure $schemaClosure The `\Closure` returns a `ISchemaWrapper` + * @param array $options + * @return null|ISchemaWrapper + * @since 20.0.0 + */ + public function changeSchema(IOutput $output, \Closure $schemaClosure, array $options) { + /** @var ISchemaWrapper $schema */ + $schema = $schemaClosure(); + + $statusTable = $schema->getTable('user_status'); + + $statusTable->addIndex(['status_timestamp'], 'user_status_tstmp_ix'); + $statusTable->addIndex(['is_user_defined', 'status'], 'user_status_iud_ix'); + + return $schema; + } +} From d7ccc616e42bef9270e3d315baf381c020f747c0 Mon Sep 17 00:00:00 2001 From: Georg Ehrke Date: Thu, 3 Sep 2020 16:23:35 +0200 Subject: [PATCH 6/6] Always use IUserStatus consts Signed-off-by: Georg Ehrke --- apps/user_status/lib/Connector/UserStatus.php | 5 ++- .../lib/Controller/HeartbeatController.php | 4 +-- .../lib/Controller/StatusesController.php | 5 +-- .../lib/Dashboard/UserStatusWidget.php | 5 +-- apps/user_status/lib/Db/UserStatusMapper.php | 8 ++--- .../lib/Listener/UserLiveStatusListener.php | 3 +- .../user_status/lib/Service/JSDataService.php | 3 +- .../user_status/lib/Service/StatusService.php | 33 ++++++++----------- lib/public/UserStatus/IUserStatus.php | 6 ++++ 9 files changed, 38 insertions(+), 34 deletions(-) diff --git a/apps/user_status/lib/Connector/UserStatus.php b/apps/user_status/lib/Connector/UserStatus.php index e8ab31bb10..d6b075dcd8 100644 --- a/apps/user_status/lib/Connector/UserStatus.php +++ b/apps/user_status/lib/Connector/UserStatus.php @@ -25,7 +25,6 @@ declare(strict_types=1); namespace OCA\UserStatus\Connector; use DateTimeImmutable; -use OCA\UserStatus\Service\StatusService; use OCP\UserStatus\IUserStatus; use OCA\UserStatus\Db; @@ -57,8 +56,8 @@ class UserStatus implements IUserStatus { $this->message = $status->getCustomMessage(); $this->icon = $status->getCustomIcon(); - if ($status->getStatus() === StatusService::INVISIBLE) { - $this->status = StatusService::OFFLINE; + if ($status->getStatus() === IUserStatus::INVISIBLE) { + $this->status = IUserStatus::OFFLINE; } if ($status->getClearAt() !== null) { $this->clearAt = DateTimeImmutable::createFromFormat('U', (string)$status->getClearAt()); diff --git a/apps/user_status/lib/Controller/HeartbeatController.php b/apps/user_status/lib/Controller/HeartbeatController.php index cc45b0502c..02a0fd082c 100644 --- a/apps/user_status/lib/Controller/HeartbeatController.php +++ b/apps/user_status/lib/Controller/HeartbeatController.php @@ -25,7 +25,6 @@ declare(strict_types=1); namespace OCA\UserStatus\Controller; -use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Controller; use OCP\AppFramework\Http; use OCP\AppFramework\Http\JSONResponse; @@ -34,6 +33,7 @@ use OCP\EventDispatcher\IEventDispatcher; use OCP\IRequest; use OCP\IUserSession; use OCP\User\Events\UserLiveStatusEvent; +use OCP\UserStatus\IUserStatus; class HeartbeatController extends Controller { @@ -73,7 +73,7 @@ class HeartbeatController extends Controller { * @return JSONResponse */ public function heartbeat(string $status): JSONResponse { - if (!\in_array($status, [StatusService::ONLINE, StatusService::AWAY], true)) { + if (!\in_array($status, [IUserStatus::ONLINE, IUserStatus::AWAY], true)) { return new JSONResponse([], Http::STATUS_BAD_REQUEST); } diff --git a/apps/user_status/lib/Controller/StatusesController.php b/apps/user_status/lib/Controller/StatusesController.php index 0391ea1492..4295fb7694 100644 --- a/apps/user_status/lib/Controller/StatusesController.php +++ b/apps/user_status/lib/Controller/StatusesController.php @@ -32,6 +32,7 @@ use OCP\AppFramework\Http\DataResponse; use OCP\AppFramework\OCS\OCSNotFoundException; use OCP\AppFramework\OCSController; use OCP\IRequest; +use OCP\UserStatus\IUserStatus; class StatusesController extends OCSController { @@ -92,8 +93,8 @@ class StatusesController extends OCSController { */ private function formatStatus(UserStatus $status): array { $visibleStatus = $status->getStatus(); - if ($visibleStatus === StatusService::INVISIBLE) { - $visibleStatus = StatusService::OFFLINE; + if ($visibleStatus === IUserStatus::INVISIBLE) { + $visibleStatus = IUserStatus::OFFLINE; } return [ diff --git a/apps/user_status/lib/Dashboard/UserStatusWidget.php b/apps/user_status/lib/Dashboard/UserStatusWidget.php index 9cd29e3ddb..6ccea95acd 100644 --- a/apps/user_status/lib/Dashboard/UserStatusWidget.php +++ b/apps/user_status/lib/Dashboard/UserStatusWidget.php @@ -32,6 +32,7 @@ use OCP\IInitialStateService; use OCP\IL10N; use OCP\IUserManager; use OCP\IUserSession; +use OCP\UserStatus\IUserStatus; /** * Class UserStatusWidget @@ -146,8 +147,8 @@ class UserStatusWidget implements IWidget { return [ 'userId' => $status->getUserId(), 'displayName' => $displayName, - 'status' => $status->getStatus() === StatusService::INVISIBLE - ? StatusService::OFFLINE + 'status' => $status->getStatus() === IUserStatus::INVISIBLE + ? IUserStatus::OFFLINE : $status->getStatus(), 'icon' => $status->getCustomIcon(), 'message' => $status->getCustomMessage(), diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 31156345b4..dc54789d34 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -25,10 +25,10 @@ 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; +use OCP\UserStatus\IUserStatus; /** * Class UserStatusMapper @@ -82,7 +82,7 @@ class UserStatusMapper extends QBMapper { ->select('*') ->from($this->tableName) ->orderBy('status_timestamp', 'DESC') - ->where($qb->expr()->notIn('status', $qb->createNamedParameter([StatusService::ONLINE, StatusService::AWAY, StatusService::OFFLINE], IQueryBuilder::PARAM_STR_ARRAY))) + ->where($qb->expr()->notIn('status', $qb->createNamedParameter([IUserStatus::ONLINE, IUserStatus::AWAY, IUserStatus::OFFLINE], IQueryBuilder::PARAM_STR_ARRAY))) ->orWhere($qb->expr()->isNotNull('message_id')) ->orWhere($qb->expr()->isNotNull('custom_icon')) ->orWhere($qb->expr()->isNotNull('custom_message')); @@ -133,13 +133,13 @@ class UserStatusMapper extends QBMapper { public function clearStatusesOlderThan(int $olderThan, int $now): void { $qb = $this->db->getQueryBuilder(); $qb->update($this->tableName) - ->set('status', $qb->createNamedParameter(StatusService::OFFLINE)) + ->set('status', $qb->createNamedParameter(IUserStatus::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->expr()->eq('status', $qb->createNamedParameter(IUserStatus::ONLINE)) )); $qb->execute(); diff --git a/apps/user_status/lib/Listener/UserLiveStatusListener.php b/apps/user_status/lib/Listener/UserLiveStatusListener.php index 52e73017d4..967810367e 100644 --- a/apps/user_status/lib/Listener/UserLiveStatusListener.php +++ b/apps/user_status/lib/Listener/UserLiveStatusListener.php @@ -33,6 +33,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\EventDispatcher\IEventListener; use OCP\EventDispatcher\Event; use OCP\User\Events\UserLiveStatusEvent; +use OCP\UserStatus\IUserStatus; /** * Class UserDeletedListener @@ -74,7 +75,7 @@ class UserLiveStatusListener implements IEventListener { } catch (DoesNotExistException $ex) { $userStatus = new UserStatus(); $userStatus->setUserId($user->getUID()); - $userStatus->setStatus(StatusService::OFFLINE); + $userStatus->setStatus(IUserStatus::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); } diff --git a/apps/user_status/lib/Service/JSDataService.php b/apps/user_status/lib/Service/JSDataService.php index 334ae248d4..ca78943faf 100644 --- a/apps/user_status/lib/Service/JSDataService.php +++ b/apps/user_status/lib/Service/JSDataService.php @@ -27,6 +27,7 @@ namespace OCA\UserStatus\Service; use OCP\AppFramework\Db\DoesNotExistException; use OCP\IUserSession; +use OCP\UserStatus\IUserStatus; class JSDataService implements \JsonSerializable { @@ -65,7 +66,7 @@ class JSDataService implements \JsonSerializable { 'messageIsPredefined' => false, 'icon' => null, 'clearAt' => null, - 'status' => StatusService::OFFLINE, + 'status' => IUserStatus::OFFLINE, 'statusIsUserDefined' => false, ]; } diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 1e1e5b1fcc..85368342b2 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -34,6 +34,7 @@ use OCA\UserStatus\Exception\InvalidStatusTypeException; use OCA\UserStatus\Exception\StatusMessageTooLongException; use OCP\AppFramework\Db\DoesNotExistException; use OCP\AppFramework\Utility\ITimeFactory; +use OCP\UserStatus\IUserStatus; /** * Class StatusService @@ -54,21 +55,15 @@ class StatusService { /** @var EmojiService */ private $emojiService; - public const ONLINE = 'online'; - public const AWAY = 'away'; - public const DND = 'dnd'; - public const INVISIBLE = 'invisible'; - public const OFFLINE = 'offline'; - /** * List of priorities ordered by their priority */ public const PRIORITY_ORDERED_STATUSES = [ - self::ONLINE, - self::AWAY, - self::DND, - self::INVISIBLE, - self::OFFLINE + IUserStatus::ONLINE, + IUserStatus::AWAY, + IUserStatus::DND, + IUserStatus::INVISIBLE, + IUserStatus::OFFLINE ]; /** @@ -76,9 +71,9 @@ class StatusService { * or UserLiveStatusEvents */ public const PERSISTENT_STATUSES = [ - self::AWAY, - self::DND, - self::INVISIBLE, + IUserStatus::AWAY, + IUserStatus::DND, + IUserStatus::INVISIBLE, ]; /** @var int */ @@ -200,7 +195,7 @@ class StatusService { } catch (DoesNotExistException $ex) { $userStatus = new UserStatus(); $userStatus->setUserId($userId); - $userStatus->setStatus(self::OFFLINE); + $userStatus->setStatus(IUserStatus::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); } @@ -245,7 +240,7 @@ class StatusService { } catch (DoesNotExistException $ex) { $userStatus = new UserStatus(); $userStatus->setUserId($userId); - $userStatus->setStatus(self::OFFLINE); + $userStatus->setStatus(IUserStatus::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); } @@ -287,7 +282,7 @@ class StatusService { return false; } - $userStatus->setStatus(self::OFFLINE); + $userStatus->setStatus(IUserStatus::OFFLINE); $userStatus->setStatusTimestamp(0); $userStatus->setIsUserDefined(false); @@ -343,7 +338,7 @@ class StatusService { $clearAt = $status->getClearAt(); if ($status->getStatusTimestamp() < $this->timeFactory->getTime() - self::INVALIDATE_STATUS_THRESHOLD - && (!$status->getIsUserDefined() || $status->getStatus() === self::ONLINE)) { + && (!$status->getIsUserDefined() || $status->getStatus() === IUserStatus::ONLINE)) { $this->cleanStatus($status); } if ($clearAt !== null && $clearAt < $this->timeFactory->getTime()) { @@ -360,7 +355,7 @@ class StatusService { * @param UserStatus $status */ private function cleanStatus(UserStatus $status): void { - $status->setStatus(self::OFFLINE); + $status->setStatus(IUserStatus::OFFLINE); $status->setStatusTimestamp($this->timeFactory->getTime()); $status->setIsUserDefined(false); diff --git a/lib/public/UserStatus/IUserStatus.php b/lib/public/UserStatus/IUserStatus.php index b721ee67a7..3e606e3871 100644 --- a/lib/public/UserStatus/IUserStatus.php +++ b/lib/public/UserStatus/IUserStatus.php @@ -59,6 +59,12 @@ interface IUserStatus { */ public const OFFLINE = 'offline'; + /** + * @var string + * @since 20.0.0 + */ + public const INVISIBLE = 'invisible'; + /** * Get the user this status is connected to *