Create the backup user status in 1 query instead of 3

Signed-off-by: Joas Schilling <coding@schilljs.com>

Author: nickvergessen

apps/user_status/lib/Db/UserStatusMapper.php

@@ -184,6 +184,23 @@ public function deleteByIds(array $ids): void {
 		$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)

apps/user_status/lib/Service/StatusService.php

@@ -35,6 +35,7 @@
 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;
@@ -435,30 +436,18 @@ private function addDefaultMessage(UserStatus $status): void {
 	}
 
 	/**
-	 * @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 {

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

@@ -26,6 +26,8 @@
  */
 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\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,53 +726,36 @@ public function testCleanStatusCleanedAlready(): void {
 		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('findByUserId')
-			->with('john', true)
-			->willReturn($status);
+			->method('createBackupStatus')
+			->with('john')
+			->willThrowException($e);
 
-		$this->service->backupCurrentStatus('john');
+		$this->assertFalse($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');
-
-		$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);
+	public function testBackupThrowsOther(): void {
+		$e = new Exception('', Exception::REASON_CONNECTION_LOST);
+		$this->mapper->expects($this->once())
+			->method('createBackupStatus')
+			->with('john')
+			->willThrowException($e);
 
-		$newBackupStatus = new UserStatus();
-		$newBackupStatus->setStatus(IUserStatus::ONLINE);
-		$newBackupStatus->setStatusTimestamp(1337);
-		$newBackupStatus->setIsUserDefined(true);
-		$newBackupStatus->setMessageId('meeting');
-		$newBackupStatus->setUserId('_john');
-		$newBackupStatus->setIsBackup(true);
+		$this->expectException(Exception::class);
+		$this->service->backupCurrentStatus('john');
+	}
 
+	public function testBackup(): void {
+		$e = new Exception('', Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION);
 		$this->mapper->expects($this->once())
-			->method('update')
-			->with($newBackupStatus);
+			->method('createBackupStatus')
+			->with('john')
+			->willReturn(true);
 
-		$this->service->backupCurrentStatus('john');
+		$this->assertTrue($this->service->backupCurrentStatus('john'));
 	}
 
 	public function testRevertMultipleUserStatus(): void {