Merge pull request #31106 from nextcloud/techdebt/noid/improve-user-status-update-handling

Improve user status revert performance

apps/user_status/lib/Connector/UserStatusProvider.php

@@ -57,17 +57,15 @@ class UserStatusProvider implements IProvider, ISettableProvider {
 		return $userStatuses;
 	}
 
-	public function setUserStatus(string $userId, string $messageId, string $status, bool $createBackup = false): void {
-		if ($createBackup) {
-			if ($this->service->backupCurrentStatus($userId) === false) {
-				return; // Already a status set automatically => abort.
-			}
-		}
-		$this->service->setStatus($userId, $status, null, true);
-		$this->service->setPredefinedMessage($userId, $messageId, null);
+	public function setUserStatus(string $userId, string $messageId, string $status, bool $createBackup): void {
+		$this->service->setUserStatus($userId, $status, $messageId, $createBackup);
 	}
 
 	public function revertUserStatus(string $userId, string $messageId, string $status): void {
 		$this->service->revertUserStatus($userId, $messageId, $status);
 	}
+
+	public function revertMultipleUserStatus(array $userIds, string $messageId, string $status): void {
+		$this->service->revertMultipleUserStatus($userIds, $messageId, $status);
+	}
 }

apps/user_status/lib/Db/UserStatusMapper.php

@@ -101,8 +101,7 @@ class UserStatusMapper extends QBMapper {
 		$qb
 			->select('*')
 			->from($this->tableName)
-			->where($qb->expr()->eq('user_id', $qb->createNamedParameter($isBackup ? '_' . $userId : $userId, IQueryBuilder::PARAM_STR)))
-			->andWhere($qb->expr()->eq('is_backup', $qb->createNamedParameter($isBackup, IQueryBuilder::PARAM_BOOL)));
+			->where($qb->expr()->eq('user_id', $qb->createNamedParameter($isBackup ? '_' . $userId : $userId, IQueryBuilder::PARAM_STR)));
 
 		return $this->findEntity($qb);
 	}
@@ -111,7 +110,7 @@ class UserStatusMapper extends QBMapper {
 	 * @param array $userIds
 	 * @return array
 	 */
-	public function findByUserIds(array $userIds):array {
+	public function findByUserIds(array $userIds): array {
 		$qb = $this->db->getQueryBuilder();
 		$qb
 			->select('*')
@@ -158,4 +157,57 @@ class UserStatusMapper extends QBMapper {
 
 		$qb->execute();
 	}
+
+
+	/**
+	 * Deletes a user status so we can restore the backup
+	 *
+	 * @param string $userId
+	 * @param string $messageId
+	 * @param string $status
+	 * @return bool True if an entry was deleted
+	 */
+	public function deleteCurrentStatusToRestoreBackup(string $userId, string $messageId, string $status): bool {
+		$qb = $this->db->getQueryBuilder();
+		$qb->delete($this->tableName)
+			->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId)))
+			->andWhere($qb->expr()->eq('message_id', $qb->createNamedParameter($messageId)))
+			->andWhere($qb->expr()->eq('status', $qb->createNamedParameter($status)))
+			->andWhere($qb->expr()->eq('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)));
+		return $qb->executeStatement() > 0;
+	}
+
+	public function deleteByIds(array $ids): void {
+		$qb = $this->db->getQueryBuilder();
+		$qb->delete($this->tableName)
+			->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY)));
+		$qb->executeStatement();
+	}
+
+	/**
+	 * @param string $userId
+	 * @return bool
+	 * @throws \OCP\DB\Exception
+	 */
+	public function createBackupStatus(string $userId): bool {
+		// Prefix user account with an underscore because user_id is marked as unique
+		// in the table. Starting a username with an underscore is not allowed so this
+		// shouldn't create any trouble.
+		$qb = $this->db->getQueryBuilder();
+		$qb->update($this->tableName)
+			->set('is_backup', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL))
+			->set('user_id', $qb->createNamedParameter('_' . $userId))
+			->where($qb->expr()->eq('user_id', $qb->createNamedParameter($userId)));
+		return $qb->executeStatement() > 0;
+	}
+
+	public function restoreBackupStatuses(array $ids): void {
+		$qb = $this->db->getQueryBuilder();
+		$qb->update($this->tableName)
+			->set('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL))
+			->set('user_id', $qb->func()->substring('user_id', $qb->createNamedParameter(2, IQueryBuilder::PARAM_INT)))
+			->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY)));
+
+		$qb->executeStatement();
+	}
 }

apps/user_status/lib/Service/StatusService.php

@@ -35,7 +35,9 @@ use OCA\UserStatus\Exception\InvalidStatusTypeException;
 use OCA\UserStatus\Exception\StatusMessageTooLongException;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\DB\Exception;
 use OCP\IConfig;
+use OCP\IUser;
 use OCP\UserStatus\IUserStatus;
 
 /**
@@ -229,6 +231,7 @@ class StatusService {
 			$userStatus->setStatus(IUserStatus::OFFLINE);
 			$userStatus->setStatusTimestamp(0);
 			$userStatus->setIsUserDefined(false);
+			$userStatus->setIsBackup(false);
 		}
 
 		if (!$this->predefinedStatusService->isValidId($messageId)) {
@@ -252,6 +255,60 @@ class StatusService {
 		return $this->mapper->update($userStatus);
 	}
 
+	/**
+	 * @param string $userId
+	 * @param string $status
+	 * @param string $messageId
+	 * @param bool $createBackup
+	 * @throws InvalidStatusTypeException
+	 * @throws InvalidMessageIdException
+	 */
+	public function setUserStatus(string $userId,
+										 string $status,
+										 string $messageId,
+										 bool $createBackup): void {
+		// Check if status-type is valid
+		if (!\in_array($status, self::PRIORITY_ORDERED_STATUSES, true)) {
+			throw new InvalidStatusTypeException('Status-type "' . $status . '" is not supported');
+		}
+
+		if (!$this->predefinedStatusService->isValidId($messageId)) {
+			throw new InvalidMessageIdException('Message-Id "' . $messageId . '" is not supported');
+		}
+
+		if ($createBackup) {
+			if ($this->backupCurrentStatus($userId) === false) {
+				return; // Already a status set automatically => abort.
+			}
+
+			// If we just created the backup
+			$userStatus = new UserStatus();
+			$userStatus->setUserId($userId);
+		} else {
+			try {
+				$userStatus = $this->mapper->findByUserId($userId);
+			} catch (DoesNotExistException $ex) {
+				$userStatus = new UserStatus();
+				$userStatus->setUserId($userId);
+			}
+		}
+
+		$userStatus->setStatus($status);
+		$userStatus->setStatusTimestamp($this->timeFactory->getTime());
+		$userStatus->setIsUserDefined(false);
+		$userStatus->setIsBackup(false);
+		$userStatus->setMessageId($messageId);
+		$userStatus->setCustomIcon(null);
+		$userStatus->setCustomMessage(null);
+		$userStatus->setClearAt(null);
+
+		if ($userStatus->getId() !== null) {
+			$this->mapper->update($userStatus);
+			return;
+		}
+		$this->mapper->insert($userStatus);
+	}
+
 	/**
 	 * @param string $userId
 	 * @param string|null $statusIcon
@@ -434,33 +491,21 @@ class StatusService {
 	}
 
 	/**
-	 * @return bool false iff there is already a backup. In this case abort the procedure.
+	 * @return bool false if there is already a backup. In this case abort the procedure.
 	 */
 	public function backupCurrentStatus(string $userId): bool {
 		try {
-			$this->mapper->findByUserId($userId, true);
-			return false;
-		} catch (DoesNotExistException $ex) {
-			// No backup already existing => Good
-		}
-
-		try {
-			$userStatus = $this->mapper->findByUserId($userId);
-		} catch (DoesNotExistException $ex) {
-			// if there is no status to backup, just return
+			$this->mapper->createBackupStatus($userId);
 			return true;
+		} catch (Exception $ex) {
+			if ($ex->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
+				return false;
+			}
+			throw $ex;
 		}
-
-		$userStatus->setIsBackup(true);
-		// Prefix user account with an underscore because user_id is marked as unique
-		// in the table. Starting an username with an underscore is not allowed so this
-		// shouldn't create any trouble.
-		$userStatus->setUserId('_' . $userStatus->getUserId());
-		$this->mapper->update($userStatus);
-		return true;
 	}
 
-	public function revertUserStatus(string $userId, ?string $messageId, string $status): void {
+	public function revertUserStatus(string $userId, string $messageId, string $status): void {
 		try {
 			/** @var UserStatus $userStatus */
 			$backupUserStatus = $this->mapper->findByUserId($userId, true);
@@ -468,19 +513,49 @@ class StatusService {
 			// No user status to revert, do nothing
 			return;
 		}
-		try {
-			$userStatus = $this->mapper->findByUserId($userId);
-			if ($userStatus->getMessageId() !== $messageId || $userStatus->getStatus() !== $status) {
-				// Another status is set automatically, do nothing
-				return;
-			}
-			$this->removeUserStatus($userId);
-		} catch (DoesNotExistException $ex) {
-			// No current status => nothing to delete
+
+		$deleted = $this->mapper->deleteCurrentStatusToRestoreBackup($userId, $messageId, $status);
+		if (!$deleted) {
+			// Another status is set automatically or no status, do nothing
+			return;
 		}
+
 		$backupUserStatus->setIsBackup(false);
 		// Remove the underscore prefix added when creating the backup
 		$backupUserStatus->setUserId(substr($backupUserStatus->getUserId(), 1));
 		$this->mapper->update($backupUserStatus);
 	}
+
+	public function revertMultipleUserStatus(array $userIds, string $messageId, string $status): void {
+		// Get all user statuses and the backups
+		$findById = $userIds;
+		foreach ($userIds as $userId) {
+			$findById[] = '_' . $userId;
+		}
+		$userStatuses = $this->mapper->findByUserIds($findById);
+
+		$backups = $restoreIds = $statuesToDelete = [];
+		foreach ($userStatuses as $userStatus) {
+			if (!$userStatus->getIsBackup()
+				&& $userStatus->getMessageId() === $messageId
+				&& $userStatus->getStatus() === $status) {
+				$statuesToDelete[$userStatus->getUserId()] = $userStatus->getId();
+			} else if ($userStatus->getIsBackup()) {
+				$backups[$userStatus->getUserId()] = $userStatus->getId();
+			}
+		}
+
+		// For users with both (normal and backup) delete the status when matching
+		foreach ($statuesToDelete as $userId => $statusId) {
+			$backupUserId = '_' . $userId;
+			if (isset($backups[$backupUserId])) {
+				$restoreIds[] = $backups[$backupUserId];
+			}
+		}
+
+		$this->mapper->deleteByIds(array_values($statuesToDelete));
+
+		// For users that matched restore the previous status
+		$this->mapper->restoreBackupStatuses($restoreIds);
+	}
 }

apps/user_status/tests/Unit/Db/UserStatusMapperTest.php

@@ -251,4 +251,117 @@ class UserStatusMapperTest extends TestCase {
 		$this->mapper->insert($userStatus2);
 		$this->mapper->insert($userStatus3);
 	}
+
+	public function dataCreateBackupStatus(): array {
+		return [
+			[false, false, false],
+			[true, false, true],
+			[false, true, false],
+			[true, true, false],
+		];
+	}
+
+	/**
+	 * @dataProvider dataCreateBackupStatus
+	 * @param bool $hasStatus
+	 * @param bool $hasBackup
+	 * @param bool $backupCreated
+	 */
+	public function testCreateBackupStatus(bool $hasStatus, bool $hasBackup, bool $backupCreated): void {
+		if ($hasStatus) {
+			$userStatus1 = new UserStatus();
+			$userStatus1->setUserId('user1');
+			$userStatus1->setStatus('online');
+			$userStatus1->setStatusTimestamp(5000);
+			$userStatus1->setIsUserDefined(true);
+			$userStatus1->setIsBackup(false);
+			$userStatus1->setCustomIcon('🚀');
+			$userStatus1->setCustomMessage('Current');
+			$userStatus1->setClearAt(50000);
+			$this->mapper->insert($userStatus1);
+		}
+
+		if ($hasBackup) {
+			$userStatus1 = new UserStatus();
+			$userStatus1->setUserId('_user1');
+			$userStatus1->setStatus('online');
+			$userStatus1->setStatusTimestamp(5000);
+			$userStatus1->setIsUserDefined(true);
+			$userStatus1->setIsBackup(true);
+			$userStatus1->setCustomIcon('🚀');
+			$userStatus1->setCustomMessage('Backup');
+			$userStatus1->setClearAt(50000);
+			$this->mapper->insert($userStatus1);
+		}
+
+		if ($hasStatus && $hasBackup) {
+			$this->expectException(Exception::class);
+		}
+
+		self::assertSame($backupCreated, $this->mapper->createBackupStatus('user1'));
+
+		if ($backupCreated) {
+			$user1Status = $this->mapper->findByUserId('user1', true);
+			$this->assertEquals('_user1', $user1Status->getUserId());
+			$this->assertEquals(true, $user1Status->getIsBackup());
+			$this->assertEquals('Current', $user1Status->getCustomMessage());
+		} else if ($hasBackup) {
+			$user1Status = $this->mapper->findByUserId('user1', true);
+			$this->assertEquals('_user1', $user1Status->getUserId());
+			$this->assertEquals(true, $user1Status->getIsBackup());
+			$this->assertEquals('Backup', $user1Status->getCustomMessage());
+		}
+	}
+
+	public function testRestoreBackupStatuses(): void {
+		$userStatus1 = new UserStatus();
+		$userStatus1->setUserId('_user1');
+		$userStatus1->setStatus('online');
+		$userStatus1->setStatusTimestamp(5000);
+		$userStatus1->setIsUserDefined(true);
+		$userStatus1->setIsBackup(true);
+		$userStatus1->setCustomIcon('🚀');
+		$userStatus1->setCustomMessage('Releasing');
+		$userStatus1->setClearAt(50000);
+		$userStatus1 = $this->mapper->insert($userStatus1);
+
+		$userStatus2 = new UserStatus();
+		$userStatus2->setUserId('_user2');
+		$userStatus2->setStatus('away');
+		$userStatus2->setStatusTimestamp(5000);
+		$userStatus2->setIsUserDefined(true);
+		$userStatus2->setIsBackup(true);
+		$userStatus2->setCustomIcon('💩');
+		$userStatus2->setCustomMessage('Do not disturb');
+		$userStatus2->setClearAt(50000);
+		$userStatus2 = $this->mapper->insert($userStatus2);
+
+		$userStatus3 = new UserStatus();
+		$userStatus3->setUserId('_user3');
+		$userStatus3->setStatus('away');
+		$userStatus3->setStatusTimestamp(5000);
+		$userStatus3->setIsUserDefined(true);
+		$userStatus3->setIsBackup(true);
+		$userStatus3->setCustomIcon('🏝️');
+		$userStatus3->setCustomMessage('Vacationing');
+		$userStatus3->setClearAt(50000);
+		$this->mapper->insert($userStatus3);
+
+		$this->mapper->restoreBackupStatuses([$userStatus1->getId(), $userStatus2->getId()]);
+
+		$user1Status = $this->mapper->findByUserId('user1', false);
+		$this->assertEquals('user1', $user1Status->getUserId());
+		$this->assertEquals(false, $user1Status->getIsBackup());
+		$this->assertEquals('Releasing', $user1Status->getCustomMessage());
+
+		$user2Status = $this->mapper->findByUserId('user2', false);
+		$this->assertEquals('user2', $user2Status->getUserId());
+		$this->assertEquals(false, $user2Status->getIsBackup());
+		$this->assertEquals('Do not disturb', $user2Status->getCustomMessage());
+
+		$user3Status = $this->mapper->findByUserId('user3', true);
+		$this->assertEquals('_user3', $user3Status->getUserId());
+		$this->assertEquals(true, $user3Status->getIsBackup());
+		$this->assertEquals('Vacationing', $user3Status->getCustomMessage());
+	}
 }

apps/user_status/tests/Unit/Service/StatusServiceTest.php

@@ -26,6 +26,8 @@ declare(strict_types=1);
  */
 namespace OCA\UserStatus\Tests\Service;
 
+use Doctrine\DBAL\Exception\UniqueConstraintViolationException;
+use OC\DB\Exceptions\DbalException;
 use OCA\UserStatus\Db\UserStatus;
 use OCA\UserStatus\Db\UserStatusMapper;
 use OCA\UserStatus\Exception\InvalidClearAtException;
@@ -38,6 +40,7 @@ use OCA\UserStatus\Service\PredefinedStatusService;
 use OCA\UserStatus\Service\StatusService;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Utility\ITimeFactory;
+use OCP\DB\Exception;
 use OCP\IConfig;
 use OCP\UserStatus\IUserStatus;
 use Test\TestCase;
@@ -723,52 +726,93 @@ class StatusServiceTest extends TestCase {
 		parent::invokePrivate($this->service, 'cleanStatus', [$status]);
 	}
 
-	public function testBackupWorkingHasBackupAlready() {
-		$status = new UserStatus();
-		$status->setStatus(IUserStatus::ONLINE);
-		$status->setStatusTimestamp(1337);
-		$status->setIsUserDefined(true);
-		$status->setMessageId('meeting');
-		$status->setUserId('john');
-		$status->setIsBackup(true);
+	public function testBackupWorkingHasBackupAlready(): void {
+		$p = $this->createMock(UniqueConstraintViolationException::class);
+		$e = DbalException::wrap($p);
+		$this->mapper->expects($this->once())
+			->method('createBackupStatus')
+			->with('john')
+			->willThrowException($e);
 
+		$this->assertFalse($this->service->backupCurrentStatus('john'));
+	}
+
+	public function testBackupThrowsOther(): void {
+		$e = new Exception('', Exception::REASON_CONNECTION_LOST);
 		$this->mapper->expects($this->once())
-			->method('findByUserId')
-			->with('john', true)
-			->willReturn($status);
+			->method('createBackupStatus')
+			->with('john')
+			->willThrowException($e);
 
+		$this->expectException(Exception::class);
 		$this->service->backupCurrentStatus('john');
 	}
 
-	public function testBackup() {
-		$currentStatus = new UserStatus();
-		$currentStatus->setStatus(IUserStatus::ONLINE);
-		$currentStatus->setStatusTimestamp(1337);
-		$currentStatus->setIsUserDefined(true);
-		$currentStatus->setMessageId('meeting');
-		$currentStatus->setUserId('john');
+	public function testBackup(): void {
+		$e = new Exception('', Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION);
+		$this->mapper->expects($this->once())
+			->method('createBackupStatus')
+			->with('john')
+			->willReturn(true);
 
-		$this->mapper->expects($this->at(0))
-			->method('findByUserId')
-			->with('john', true)
-			->willThrowException(new DoesNotExistException(''));
-		$this->mapper->expects($this->at(1))
-			->method('findByUserId')
-			->with('john', false)
-			->willReturn($currentStatus);
+		$this->assertTrue($this->service->backupCurrentStatus('john'));
+	}
 
-		$newBackupStatus = new UserStatus();
-		$newBackupStatus->setStatus(IUserStatus::ONLINE);
-		$newBackupStatus->setStatusTimestamp(1337);
-		$newBackupStatus->setIsUserDefined(true);
-		$newBackupStatus->setMessageId('meeting');
-		$newBackupStatus->setUserId('_john');
-		$newBackupStatus->setIsBackup(true);
+	public function testRevertMultipleUserStatus(): void {
+		$john = new UserStatus();
+		$john->setId(1);
+		$john->setStatus(IUserStatus::AWAY);
+		$john->setStatusTimestamp(1337);
+		$john->setIsUserDefined(false);
+		$john->setMessageId('call');
+		$john->setUserId('john');
+		$john->setIsBackup(false);
+
+		$johnBackup = new UserStatus();
+		$johnBackup->setId(2);
+		$johnBackup->setStatus(IUserStatus::ONLINE);
+		$johnBackup->setStatusTimestamp(1337);
+		$johnBackup->setIsUserDefined(true);
+		$johnBackup->setMessageId('hello');
+		$johnBackup->setUserId('_john');
+		$johnBackup->setIsBackup(true);
+
+		$noBackup = new UserStatus();
+		$noBackup->setId(3);
+		$noBackup->setStatus(IUserStatus::AWAY);
+		$noBackup->setStatusTimestamp(1337);
+		$noBackup->setIsUserDefined(false);
+		$noBackup->setMessageId('call');
+		$noBackup->setUserId('nobackup');
+		$noBackup->setIsBackup(false);
+
+		$backupOnly = new UserStatus();
+		$backupOnly->setId(4);
+		$backupOnly->setStatus(IUserStatus::ONLINE);
+		$backupOnly->setStatusTimestamp(1337);
+		$backupOnly->setIsUserDefined(true);
+		$backupOnly->setMessageId('hello');
+		$backupOnly->setUserId('_backuponly');
+		$backupOnly->setIsBackup(true);
 
 		$this->mapper->expects($this->once())
-			->method('update')
-			->with($newBackupStatus);
+			->method('findByUserIds')
+			->with(['john', 'nobackup', 'backuponly', '_john', '_nobackup', '_backuponly'])
+			->willReturn([
+				$john,
+				$johnBackup,
+				$noBackup,
+				$backupOnly,
+			]);
 
-		$this->service->backupCurrentStatus('john');
+		$this->mapper->expects($this->once())
+			->method('deleteByIds')
+			->with([1, 3]);
+
+		$this->mapper->expects($this->once())
+			->method('restoreBackupStatuses')
+			->with([2]);
+
+		$this->service->revertMultipleUserStatus(['john', 'nobackup', 'backuponly'], 'call', IUserStatus::AWAY);
 	}
 }

lib/private/UserStatus/ISettableProvider.php

@@ -52,4 +52,15 @@ interface ISettableProvider extends IProvider {
 	 * @param string $status The expected current status.
 	 */
 	public function revertUserStatus(string $userId, string $messageId, string $status): void;
+
+	/**
+	 * Revert an automatically set user status. For example after leaving a call,
+	 * change back to the previously set status. If the user has already updated
+	 * their status, this method does nothing.
+	 *
+	 * @param string[] $userIds The users for which we want to update the status.
+	 * @param string $messageId The expected current messageId.
+	 * @param string $status The expected current status.
+	 */
+	public function revertMultipleUserStatus(array $userIds, string $messageId, string $status): void;
 }

lib/private/UserStatus/Manager.php

@@ -121,4 +121,12 @@ class Manager implements IManager {
 		}
 		$this->provider->revertUserStatus($userId, $messageId, $status);
 	}
+
+	public function revertMultipleUserStatus(array $userIds, string $messageId, string $status): void {
+		$this->setupProvider();
+		if (!$this->provider || !($this->provider instanceof ISettableProvider)) {
+			return;
+		}
+		$this->provider->revertMultipleUserStatus($userIds, $messageId, $status);
+	}
 }

lib/public/UserStatus/IManager.php

@@ -39,7 +39,7 @@ interface IManager {
 	 * @return IUserStatus[]
 	 * @since 20.0.0
 	 */
-	public function getUserStatuses(array $userIds):array;
+	public function getUserStatuses(array $userIds): array;
 
 
 	/**
@@ -47,6 +47,7 @@ interface IManager {
 	 *
 	 * @param string $userId The user for which we want to update the status.
 	 * @param string $messageId The id of the predefined message.
+	 * @param string $status The status to assign