Merge pull request #22548 from nextcloud/bugfix/noid/improved_status_cleanup

Improved status cleanup
This commit is contained in:
Roeland Jago Douma 2020-09-07 15:19:42 +02:00 committed by GitHub
commit 99ee00be8c
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
17 changed files with 243 additions and 59 deletions

View File

@ -5,7 +5,7 @@
<name>User status</name>
<summary>User status</summary>
<description><![CDATA[User status]]></description>
<version>0.0.2</version>
<version>1.0.0</version>
<licence>agpl</licence>
<author mail="oc.list@georgehrke.com" >Georg Ehrke</author>
<namespace>UserStatus</namespace>

View File

@ -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',

View File

@ -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',

View File

@ -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->clearOlderThan($this->time->getTime());
$now = $this->time->getTime();
$this->mapper->clearMessagesOlderThan($now);
$this->mapper->clearStatusesOlderThan($now - StatusService::INVALIDATE_STATUS_THRESHOLD, $now);
}
}

View File

@ -56,8 +56,8 @@ class UserStatus implements IUserStatus {
$this->message = $status->getCustomMessage();
$this->icon = $status->getCustomIcon();
if ($status->getStatus() === 'invisible') {
$this->status = 'offline';
if ($status->getStatus() === IUserStatus::INVISIBLE) {
$this->status = IUserStatus::OFFLINE;
}
if ($status->getClearAt() !== null) {
$this->clearAt = DateTimeImmutable::createFromFormat('U', (string)$status->getClearAt());

View File

@ -33,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 {
@ -51,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,
@ -70,7 +73,7 @@ class HeartbeatController extends Controller {
* @return JSONResponse
*/
public function heartbeat(string $status): JSONResponse {
if (!\in_array($status, ['online', 'away'])) {
if (!\in_array($status, [IUserStatus::ONLINE, IUserStatus::AWAY], true)) {
return new JSONResponse([], Http::STATUS_BAD_REQUEST);
}

View File

@ -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 === 'invisible') {
$visibleStatus = 'offline';
if ($visibleStatus === IUserStatus::INVISIBLE) {
$visibleStatus = IUserStatus::OFFLINE;
}
return [

View File

@ -32,6 +32,7 @@ use OCP\IInitialStateService;
use OCP\IL10N;
use OCP\IUserManager;
use OCP\IUserSession;
use OCP\UserStatus\IUserStatus;
/**
* Class UserStatusWidget
@ -146,7 +147,9 @@ class UserStatusWidget implements IWidget {
return [
'userId' => $status->getUserId(),
'displayName' => $displayName,
'status' => $status->getStatus() === 'invisible' ? 'offline' : $status->getStatus(),
'status' => $status->getStatus() === IUserStatus::INVISIBLE
? IUserStatus::OFFLINE
: $status->getStatus(),
'icon' => $status->getCustomIcon(),
'message' => $status->getCustomMessage(),
'timestamp' => $status->getStatusTimestamp(),

View File

@ -28,6 +28,7 @@ namespace OCA\UserStatus\Db;
use OCP\AppFramework\Db\QBMapper;
use OCP\DB\QueryBuilder\IQueryBuilder;
use OCP\IDBConnection;
use OCP\UserStatus\IUserStatus;
/**
* Class UserStatusMapper
@ -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([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'));
@ -125,12 +126,31 @@ 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(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(IUserStatus::ONLINE))
));
$qb->execute();
}
/**
* Clear all statuses older than a given timestamp
*
* @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))

View File

@ -27,11 +27,13 @@ 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;
use OCP\EventDispatcher\Event;
use OCP\User\Events\UserLiveStatusEvent;
use OCP\UserStatus\IUserStatus;
/**
* Class UserDeletedListener
@ -46,25 +48,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 +75,7 @@ class UserLiveStatusListener implements IEventListener {
} catch (DoesNotExistException $ex) {
$userStatus = new UserStatus();
$userStatus->setUserId($user->getUID());
$userStatus->setStatus('offline');
$userStatus->setStatus(IUserStatus::OFFLINE);
$userStatus->setStatusTimestamp(0);
$userStatus->setIsUserDefined(false);
}
@ -100,7 +83,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 +91,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;
}

View File

@ -0,0 +1,57 @@
<?php
declare(strict_types=1);
/**
* @copyright Copyright (c) 2020, Georg Ehrke
*
* @author Georg Ehrke <oc.list@georgehrke.com>
*
* @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 <http://www.gnu.org/licenses/>
*
*/
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;
}
}

View File

@ -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' => 'offline',
'status' => IUserStatus::OFFLINE,
'statusIsUserDefined' => false,
];
}

View File

@ -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,17 +55,32 @@ class StatusService {
/** @var EmojiService */
private $emojiService;
/** @var string[] */
private $allowedStatusTypes = [
'online',
'away',
'dnd',
'invisible',
'offline'
/**
* List of priorities ordered by their priority
*/
public const PRIORITY_ORDERED_STATUSES = [
IUserStatus::ONLINE,
IUserStatus::AWAY,
IUserStatus::DND,
IUserStatus::INVISIBLE,
IUserStatus::OFFLINE
];
/**
* List of statuses that persist the clear-up
* or UserLiveStatusEvents
*/
public const PERSISTENT_STATUSES = [
IUserStatus::AWAY,
IUserStatus::DND,
IUserStatus::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 +161,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 +195,7 @@ class StatusService {
} catch (DoesNotExistException $ex) {
$userStatus = new UserStatus();
$userStatus->setUserId($userId);
$userStatus->setStatus('offline');
$userStatus->setStatus(IUserStatus::OFFLINE);
$userStatus->setStatusTimestamp(0);
$userStatus->setIsUserDefined(false);
}
@ -224,7 +240,7 @@ class StatusService {
} catch (DoesNotExistException $ex) {
$userStatus = new UserStatus();
$userStatus->setUserId($userId);
$userStatus->setStatus('offline');
$userStatus->setStatus(IUserStatus::OFFLINE);
$userStatus->setStatusTimestamp(0);
$userStatus->setIsUserDefined(false);
}
@ -234,8 +250,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 +282,7 @@ class StatusService {
return false;
}
$userStatus->setStatus('offline');
$userStatus->setStatus(IUserStatus::OFFLINE);
$userStatus->setStatusTimestamp(0);
$userStatus->setIsUserDefined(false);
@ -320,9 +336,14 @@ class StatusService {
*/
private function processStatus(UserStatus $status): UserStatus {
$clearAt = $status->getClearAt();
if ($clearAt !== null && $clearAt < $this->timeFactory->getTime()) {
if ($status->getStatusTimestamp() < $this->timeFactory->getTime() - self::INVALIDATE_STATUS_THRESHOLD
&& (!$status->getIsUserDefined() || $status->getStatus() === IUserStatus::ONLINE)) {
$this->cleanStatus($status);
}
if ($clearAt !== null && $clearAt < $this->timeFactory->getTime()) {
$this->cleanStatusMessage($status);
}
if ($status->getMessageId() !== null) {
$this->addDefaultMessage($status);
}
@ -334,6 +355,17 @@ class StatusService {
* @param UserStatus $status
*/
private function cleanStatus(UserStatus $status): void {
$status->setStatus(IUserStatus::OFFLINE);
$status->setStatusTimestamp($this->timeFactory->getTime());
$status->setIsUserDefined(false);
$this->mapper->update($status);
}
/**
* @param UserStatus $status
*/
private function cleanStatusMessage(UserStatus $status): void {
$status->setMessageId(null);
$status->setCustomIcon(null);
$status->setCustomMessage(null);

View File

@ -52,8 +52,11 @@ class ClearOldStatusesBackgroundJobTest extends TestCase {
public function testRun() {
$this->mapper->expects($this->once())
->method('clearOlderThan')
->method('clearMessagesOlderThan')
->with(1337);
$this->mapper->expects($this->once())
->method('clearStatusesOlderThan')
->with(1037, 1337);
$this->time->method('getTime')
->willReturn(1337);

View File

@ -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());
@ -152,10 +152,60 @@ 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->clearOlderThan(55000);
$this->mapper->clearMessagesOlderThan(55000);
$allStatuses = $this->mapper->findAll();
$this->assertCount(3, $allStatuses);
@ -189,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');

View File

@ -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');

View File

@ -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
*