diff --git a/apps/user_status/appinfo/info.xml b/apps/user_status/appinfo/info.xml index 7702671a50248..4a9d95be21a7e 100644 --- a/apps/user_status/appinfo/info.xml +++ b/apps/user_status/appinfo/info.xml @@ -29,6 +29,9 @@ OCA\UserStatus\BackgroundJob\ClearOldStatusesBackgroundJob + + OCA\UserStatus\Command\Repair + OCA\UserStatus\ContactsMenu\StatusProvider diff --git a/apps/user_status/composer/composer/autoload_classmap.php b/apps/user_status/composer/composer/autoload_classmap.php index b57df813bc9c4..4a1927147e7d3 100644 --- a/apps/user_status/composer/composer/autoload_classmap.php +++ b/apps/user_status/composer/composer/autoload_classmap.php @@ -10,6 +10,7 @@ 'OCA\\UserStatus\\AppInfo\\Application' => $baseDir . '/../lib/AppInfo/Application.php', 'OCA\\UserStatus\\BackgroundJob\\ClearOldStatusesBackgroundJob' => $baseDir . '/../lib/BackgroundJob/ClearOldStatusesBackgroundJob.php', 'OCA\\UserStatus\\Capabilities' => $baseDir . '/../lib/Capabilities.php', + 'OCA\\UserStatus\\Command\\Repair' => $baseDir . '/../lib/Command/Repair.php', 'OCA\\UserStatus\\Connector\\UserStatus' => $baseDir . '/../lib/Connector/UserStatus.php', 'OCA\\UserStatus\\Connector\\UserStatusProvider' => $baseDir . '/../lib/Connector/UserStatusProvider.php', 'OCA\\UserStatus\\ContactsMenu\\StatusProvider' => $baseDir . '/../lib/ContactsMenu/StatusProvider.php', diff --git a/apps/user_status/composer/composer/autoload_static.php b/apps/user_status/composer/composer/autoload_static.php index 17f45ab9bbf58..1d7a57bfe81d4 100644 --- a/apps/user_status/composer/composer/autoload_static.php +++ b/apps/user_status/composer/composer/autoload_static.php @@ -25,6 +25,7 @@ class ComposerStaticInitUserStatus 'OCA\\UserStatus\\AppInfo\\Application' => __DIR__ . '/..' . '/../lib/AppInfo/Application.php', 'OCA\\UserStatus\\BackgroundJob\\ClearOldStatusesBackgroundJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/ClearOldStatusesBackgroundJob.php', 'OCA\\UserStatus\\Capabilities' => __DIR__ . '/..' . '/../lib/Capabilities.php', + 'OCA\\UserStatus\\Command\\Repair' => __DIR__ . '/..' . '/../lib/Command/Repair.php', 'OCA\\UserStatus\\Connector\\UserStatus' => __DIR__ . '/..' . '/../lib/Connector/UserStatus.php', 'OCA\\UserStatus\\Connector\\UserStatusProvider' => __DIR__ . '/..' . '/../lib/Connector/UserStatusProvider.php', 'OCA\\UserStatus\\ContactsMenu\\StatusProvider' => __DIR__ . '/..' . '/../lib/ContactsMenu/StatusProvider.php', diff --git a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php index 2bce800c069ae..2fbb62c39063e 100644 --- a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php +++ b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php @@ -45,5 +45,6 @@ protected function run($argument) { $this->mapper->clearOlderThanClearAt($now); $this->mapper->clearStatusesOlderThan($now - StatusService::INVALIDATE_STATUS_THRESHOLD, $now); + $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); } } diff --git a/apps/user_status/lib/Command/Repair.php b/apps/user_status/lib/Command/Repair.php new file mode 100644 index 0000000000000..796fd96bcd03a --- /dev/null +++ b/apps/user_status/lib/Command/Repair.php @@ -0,0 +1,129 @@ +setName('user-status:repair') + ->setDescription('Repair user statuses left behind by an interrupted automated status') + ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Only report what would be repaired'); + } + + #[\Override] + public function execute(InputInterface $input, OutputInterface $output): int { + $dryRun = (bool)$input->getOption('dry-run'); + if ($dryRun) { + $output->writeln('Dry run, no changes will be written.'); + $output->writeln(''); + } + + $this->repairMissingBackupFlags($output, $dryRun); + $this->repairOrphanedStatuses($output, $dryRun); + $this->repairStrandedBackups($output, $dryRun); + + return self::SUCCESS; + } + + /** + * Rows written before is_backup had a default are invisible to every query + * comparing it against false, so other users see them as offline and the + * cleanup job skips them. + */ + private function repairMissingBackupFlags(OutputInterface $output, bool $dryRun): void { + $ids = $this->mapper->findStatusesWithoutBackupFlagIds(); + if ($ids === []) { + $output->writeln('No statuses with a missing backup flag.'); + return; + } + + $count = count($ids); + if ($dryRun) { + $output->writeln("Would set the backup flag on $count status(es)."); + $this->listIds($output, $ids); + return; + } + + $fixed = $this->mapper->normalizeBackupFlagByIds($ids); + $output->writeln("Set the backup flag on $fixed status(es)."); + } + + /** + * A live status on an automated message id with no backup row can never be + * reverted by the automation that set it, and the heartbeat refuses to + * overwrite it, so the user is stuck. Removing the row lets the next + * heartbeat recreate a normal status. + */ + private function repairOrphanedStatuses(OutputInterface $output, bool $dryRun): void { + $ids = $this->mapper->findOrphanedAutomatedStatusIds(StatusService::AUTOMATED_MESSAGE_IDS); + if ($ids === []) { + $output->writeln('No users stuck on an automated status.'); + return; + } + + if ($dryRun) { + $output->writeln('Would clear ' . count($ids) . ' status(es) stuck on an automated status.'); + $this->listIds($output, $ids); + return; + } + + $deleted = $this->mapper->deleteByIds($ids); + $output->writeln("Cleared $deleted status(es) stuck on an automated status."); + } + + /** + * A backup that can no longer be matched blocks every future automated + * status change for that user, because createBackupStatus() keeps hitting + * the unique constraint on user_id. + */ + private function repairStrandedBackups(OutputInterface $output, bool $dryRun): void { + $ids = $this->mapper->findStrandedBackupIds(StatusService::AUTOMATED_MESSAGE_IDS); + if ($ids === []) { + $output->writeln('No stranded backup statuses.'); + return; + } + + if ($dryRun) { + $output->writeln('Would remove ' . count($ids) . ' stranded backup status(es).'); + $this->listIds($output, $ids); + return; + } + + $deleted = $this->mapper->deleteByIds($ids); + $output->writeln("Removed $deleted stranded backup status(es)."); + } + + /** + * The ids are what an administrator needs to look the rows up themselves, + * but there can be a lot of them, so only spell them out when asked. + * + * @param list $ids + */ + private function listIds(OutputInterface $output, array $ids): void { + if ($output->getVerbosity() >= OutputInterface::VERBOSITY_VERBOSE) { + $output->writeln(' ids: ' . implode(', ', $ids)); + } + } +} diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index ea3b76c8e564d..9ed29b39fe3e8 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -20,6 +20,12 @@ */ class UserStatusMapper extends QBMapper { + /** + * Oracle rejects an IN list with more than 1000 expressions, so anything + * built from an unbounded set of ids has to be split into chunks. + */ + private const MAX_IN_CHUNK = 1000; + /** * @param IDBConnection $db */ @@ -163,11 +169,152 @@ public function deleteCurrentStatusToRestoreBackup(string $userId, string $messa return $qb->executeStatement() > 0; } - public function deleteByIds(array $ids): void { + /** + * Deletes backup rows that can never be restored, because the matching live + * status is gone or is no longer on one of the automated statuses that would + * revert into it. + * + * Such a row is not just clutter: while it exists, createBackupStatus() keeps + * hitting the unique constraint on user_id, which makes setUserStatus() + * silently abort every automated status change for that user. + * + * @param list $automatedMessageIds Message ids that own a backup + * @return int Number of deleted backup rows + */ + public function deleteStrandedBackups(array $automatedMessageIds): int { + return $this->deleteByIds($this->findStrandedBackupIds($automatedMessageIds)); + } + + /** + * Ids of backup rows that can never be restored. See deleteStrandedBackups(). + * + * A backup is reachable exactly when the live row it belongs to still carries + * one of the automated message ids, because that is what revertUserStatus() + * matches on. The live row is the one whose user id is the backup's user id + * without the underscore prefix, so the two are matched with a self join. + * + * @param list $automatedMessageIds + * @return list + */ + public function findStrandedBackupIds(array $automatedMessageIds): array { $qb = $this->db->getQueryBuilder(); - $qb->delete($this->tableName) - ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); - $qb->executeStatement(); + $qb->select('b.id') + ->from($this->tableName, 'b') + ->where($qb->expr()->eq('b.is_backup', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL))); + + if ($automatedMessageIds === []) { + // No automated status can own a backup, so none of them is reachable. + return $this->fetchIds($qb); + } + + // Not filtering the live side on is_backup is deliberate: a row whose + // is_backup is NULL is still treated as a live row, so unexpected data + // errs towards keeping the backup. + $qb->leftJoin('b', $this->tableName, 'l', $qb->expr()->andX( + $qb->expr()->eq('l.user_id', $qb->func()->substring('b.user_id', $qb->createNamedParameter(2, IQueryBuilder::PARAM_INT))), + $qb->expr()->in('l.message_id', $qb->createNamedParameter($automatedMessageIds, IQueryBuilder::PARAM_STR_ARRAY)), + )) + ->andWhere($qb->expr()->isNull('l.id')); + + return $this->fetchIds($qb); + } + + /** + * Ids of live rows that sit on an automated status with no backup row to + * revert into. Those can never be reverted by the automation that set them, + * so the user is stuck on that status until it is cleared. + * + * @param list $automatedMessageIds + * @return list + */ + public function findOrphanedAutomatedStatusIds(array $automatedMessageIds): array { + if ($automatedMessageIds === []) { + return []; + } + + $qb = $this->db->getQueryBuilder(); + // The backup of a live row carries the same user id with an underscore + // prefix, so the two are matched with a self join on the concatenation. + $qb->select('l.id') + ->from($this->tableName, 'l') + ->leftJoin('l', $this->tableName, 'b', $qb->expr()->eq( + 'b.user_id', + $qb->func()->concat($qb->createNamedParameter('_'), 'l.user_id'), + )) + ->where($qb->expr()->in('l.message_id', $qb->createNamedParameter($automatedMessageIds, IQueryBuilder::PARAM_STR_ARRAY))) + ->andWhere($qb->expr()->isNull('b.id')) + // Skip backup rows on the live side. Testing the prefix rather than + // is_backup keeps this correct for rows where is_backup is NULL, and + // a substring comparison avoids having to escape the underscore for + // a LIKE pattern. + ->andWhere($qb->expr()->neq( + $qb->func()->substring('l.user_id', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT), $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)), + $qb->createNamedParameter('_'), + )); + + return $this->fetchIds($qb); + } + + /** + * @return list + */ + private function fetchIds(IQueryBuilder $qb): array { + $result = $qb->executeQuery(); + $ids = []; + while ($row = $result->fetch()) { + $ids[] = (int)$row['id']; + } + $result->closeCursor(); + + return $ids; + } + + /** + * Ids of rows where is_backup is NULL. Those predate the column default and + * are invisible to every query that compares is_backup against false. + * + * @return list + */ + public function findStatusesWithoutBackupFlagIds(): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('id') + ->from($this->tableName) + ->where($qb->expr()->isNull('is_backup')); + + return $this->fetchIds($qb); + } + + /** + * @param list $ids + * @return int Number of rows that were given an explicit is_backup value + */ + public function normalizeBackupFlagByIds(array $ids): int { + $updated = 0; + foreach (array_chunk($ids, self::MAX_IN_CHUNK) as $chunk) { + $qb = $this->db->getQueryBuilder(); + $qb->update($this->tableName) + ->set('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); + $updated += $qb->executeStatement(); + } + + return $updated; + } + + /** + * @param list $ids + * @return int Number of deleted rows + */ + public function deleteByIds(array $ids): int { + $deleted = 0; + foreach (array_chunk($ids, self::MAX_IN_CHUNK) as $chunk) { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->tableName) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); + $deleted += $qb->executeStatement(); + } + + return $deleted; } /** diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 6c1805963c881..c08539c43c43c 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -59,6 +59,20 @@ class StatusService { IUserStatus::INVISIBLE, ]; + /** + * Message ids that are only ever set by an automation (calendar, call, + * availability, out-of-office). A status carrying one of these owns the + * backup of whatever the user had set before, and is expected to be + * reverted once the automation stops applying. + */ + public const AUTOMATED_MESSAGE_IDS = [ + IUserStatus::MESSAGE_CALENDAR_BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY_TENTATIVE, + IUserStatus::MESSAGE_CALL, + IUserStatus::MESSAGE_AVAILABILITY, + IUserStatus::MESSAGE_OUT_OF_OFFICE, + ]; + /** @var int */ public const INVALIDATE_STATUS_THRESHOLD = 15 /* minutes */ * 60 /* seconds */; @@ -530,7 +544,13 @@ public function revertUserStatus(string $userId, string $messageId, bool $revert /** @var UserStatus $userStatus */ $backupUserStatus = $this->mapper->findByUserId($userId, true); } catch (DoesNotExistException $ex) { - // No user status to revert, do nothing + // There is no backup to restore. The automated status still has to + // go, otherwise the user is stuck on it forever: UserLiveStatusListener + // refuses to overwrite an automated status, so no heartbeat can ever + // bring them back online. + if ($this->mapper->deleteCurrentStatusToRestoreBackup($userId, $messageId)) { + $this->logger->debug('Cleared automated status "' . $messageId . '" for user ' . $userId . ': there was no backup to restore', ['app' => 'user_status']); + } return null; } @@ -540,14 +560,17 @@ public function revertUserStatus(string $userId, string $messageId, bool $revert return null; } - if ($revertedManually) { - if ($backupUserStatus->getStatus() === IUserStatus::OFFLINE) { - // When the user reverts the status manually they are online - $backupUserStatus->setStatus(IUserStatus::ONLINE); - } - $backupUserStatus->setStatusTimestamp($this->timeFactory->getTime()); + if ($revertedManually && $backupUserStatus->getStatus() === IUserStatus::OFFLINE) { + // When the user reverts the status manually they are online + $backupUserStatus->setStatus(IUserStatus::ONLINE); } + // The restored status becomes the current one now. Keeping the timestamp + // from before the automation would make it instantly stale for anything + // longer than INVALIDATE_STATUS_THRESHOLD, so the next read would clean + // the user straight to offline. + $backupUserStatus->setStatusTimestamp($this->timeFactory->getTime()); + $backupUserStatus->setIsBackup(false); // Remove the underscore prefix added when creating the backup $backupUserStatus->setUserId(substr($backupUserStatus->getUserId(), 1)); diff --git a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php index a2acfe4458b7d..92f8fc5dc0eb9 100644 --- a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php +++ b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php @@ -9,6 +9,8 @@ namespace OCA\UserStatus\Tests\Integration\Service; +use OCA\UserStatus\Db\UserStatus; +use OCA\UserStatus\Db\UserStatusMapper; use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\IDBConnection; @@ -22,17 +24,40 @@ class StatusServiceIntegrationTest extends TestCase { private StatusService $service; + private UserStatusMapper $mapper; + private IDBConnection $db; protected function setUp(): void { parent::setUp(); $this->service = Server::get(StatusService::class); + $this->mapper = Server::get(UserStatusMapper::class); - $db = Server::get(IDBConnection::class); - $qb = $db->getQueryBuilder(); + $this->db = Server::get(IDBConnection::class); + $qb = $this->db->getQueryBuilder(); $qb->delete('user_status')->executeStatement(); } + /** + * Reads a row without going through StatusService::processStatus(), which + * would rewrite a stale status before the assertion can see it. + */ + private function readRaw(string $userId): ?UserStatus { + try { + return $this->mapper->findByUserId($userId); + } catch (DoesNotExistException) { + return null; + } + } + + /** Simulates elapsed time by ageing the stored timestamps backwards. */ + private function age(string $userId, int $seconds): void { + $this->db->executeStatement( + 'UPDATE `*PREFIX*user_status` SET `status_timestamp` = `status_timestamp` - ? WHERE `user_id` IN (?, ?)', + [$seconds, $userId, '_' . $userId], + ); + } + public function testNoStatusYet(): void { $this->expectException(DoesNotExistException::class); @@ -191,4 +216,281 @@ public function testOtherAutomationsDoNotOverwriteEachOther(): void { $this->service->findByUserId('test123')->getMessageId(), ); } + + /* + * Orphaned automated statuses: a live row sits on an automated status but + * there is no backup row to revert into, so revertUserStatus() has nothing + * to restore. It must still clear the automated status, otherwise the user + * is stuck on it forever and the heartbeat can never bring them back + * online. + */ + + public function testRevertWithoutBackupClearsAutomatedStatus(): void { + // No backup taken, so nothing can ever be restored for this user. + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + false, + ); + self::assertSame( + IUserStatus::MESSAGE_CALENDAR_BUSY, + $this->readRaw('test123')?->getMessageId(), + ); + + $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull($reverted, 'Nothing can be restored without a backup'); + self::assertNull( + $this->readRaw('test123'), + 'The unreachable automated status must be cleared, not left behind', + ); + } + + public function testRevertWithoutBackupKeepsStatusTheUserChangedThemselves(): void { + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + false, + ); + // The user replaces the automated message with their own. + $this->service->setCustomMessage('test123', '🍕', 'Lunch', null); + + $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull($reverted); + $status = $this->readRaw('test123'); + self::assertNotNull($status, 'A status the user set themselves must not be deleted'); + self::assertSame('Lunch', $status->getCustomMessage()); + self::assertNull($status->getMessageId()); + } + + public function testRevertWithoutBackupKeepsOtherAutomatedStatus(): void { + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALL, + false, + ); + + // The meeting automation reverts, but the live status belongs to a call. + $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull($reverted); + self::assertSame( + IUserStatus::MESSAGE_CALL, + $this->readRaw('test123')?->getMessageId(), + 'An unrelated automated status must be left alone', + ); + } + + public function testFreshUserAutomatedStatusIsClearedOnRevert(): void { + // A user who has never had a status row: there is nothing to back up, + // so the automated status is applied without a backup. + $applied = $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + self::assertNotNull($applied, 'A user without a previous status should still get the meeting status'); + self::assertNull($this->readRaw('_test123'), 'There was no status to back up'); + + $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull( + $this->readRaw('test123'), + 'The meeting status must be cleared when the meeting ends', + ); + } + + public function testRevertAfterLongMeetingRefreshesTimestamp(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + // A 90 minute meeting, well past INVALIDATE_STATUS_THRESHOLD. + $this->age('test123', 90 * 60); + + $before = time(); + $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNotNull($reverted); + self::assertGreaterThanOrEqual( + $before, + $this->readRaw('test123')?->getStatusTimestamp(), + 'A restored status must not carry the stale timestamp from before the meeting', + ); + } + + public function testRevertAfterLongMeetingDoesNotFallBackToOffline(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + $this->age('test123', 90 * 60); + + $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + // findByUserId() runs processStatus(), which cleans stale statuses. + self::assertSame( + IUserStatus::ONLINE, + $this->service->findByUserId('test123')->getStatus(), + 'The user was online before the meeting and must not be flipped to offline by reading the status', + ); + } + + /* + * Stranded backups: a backup row exists but the live row is no longer on + * the automated status that would restore it, so revertUserStatus() can + * never match. Nothing else removes it, and while it exists + * backupCurrentStatus() keeps failing, which silently aborts every future + * automated status change for that user. + */ + + public function testStrandedBackupIsCleanedUp(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + // The user clears the status message, so the meeting revert can no + // longer find a matching row. + $this->service->clearMessage('test123'); + self::assertNotNull($this->readRaw('_test123'), 'Precondition: the backup is stranded'); + + $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame(1, $deleted); + self::assertNull($this->readRaw('_test123'), 'The stranded backup must be removed'); + self::assertNotNull($this->readRaw('test123'), 'The live status must be untouched'); + } + + public function testBackupOfAnOngoingMeetingSurvivesCleanup(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame(0, $deleted); + self::assertNotNull( + $this->readRaw('_test123'), + 'The backup for a meeting that is still running must survive', + ); + } + + public function testLongOutOfOfficeBackupSurvivesCleanup(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::DND, + IUserStatus::MESSAGE_OUT_OF_OFFICE, + true, + ); + // Out of office can last for weeks; age well beyond any threshold. + $this->age('test123', 86400 * 30); + + $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame(0, $deleted); + self::assertNotNull( + $this->readRaw('_test123'), + 'A long running out-of-office backup must not be treated as stranded', + ); + } + + public function testAutomatedStatusWorksAgainAfterStrandedBackupCleanup(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + $this->service->clearMessage('test123'); + + // While the stranded backup exists, automated statuses are aborted. + self::assertNull( + $this->service->setUserStatus('test123', IUserStatus::BUSY, IUserStatus::MESSAGE_CALL, true), + 'Precondition: the stranded backup blocks automated statuses', + ); + + $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertNotNull( + $this->service->setUserStatus('test123', IUserStatus::BUSY, IUserStatus::MESSAGE_CALL, true), + 'Automated statuses must work again once the stranded backup is gone', + ); + } + + public function testCleanupLeavesUsersWithoutBackupsAlone(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setCustomMessage('test123', '🍕', 'Lunch', null); + + $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame(0, $deleted); + self::assertSame('Lunch', $this->readRaw('test123')?->getCustomMessage()); + } + + /** + * The lookup matches a live row against its backup by concatenating the + * underscore prefix in SQL, so it has to be exercised on a real database + * rather than only through the mapper unit tests. + */ + public function testFindsOrphanedAutomatedStatusOnARealDatabase(): void { + // A user with no status row at all gets no backup, so the meeting + // status it is given can never be reverted. + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + self::assertNull($this->readRaw('_test123'), 'Precondition: there is no backup'); + + // A second user on the same automated status, but with a backup, must + // not be reported. + $this->service->setStatus('test456', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test456', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + $orphaned = $this->mapper->findOrphanedAutomatedStatusIds(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame([$this->readRaw('test123')?->getId()], $orphaned); + } + + public function testFindsStatusesWithoutBackupFlagOnARealDatabase(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->db->executeStatement( + 'UPDATE `*PREFIX*user_status` SET `is_backup` = NULL WHERE `user_id` = ?', + ['test123'], + ); + + $ids = $this->mapper->findStatusesWithoutBackupFlagIds(); + + self::assertSame([$this->readRaw('test123')?->getId()], $ids); + self::assertSame(1, $this->mapper->normalizeBackupFlagByIds($ids)); + self::assertSame([], $this->mapper->findStatusesWithoutBackupFlagIds()); + } } diff --git a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php index 6d19d19e2702a..57308fdf6cb75 100644 --- a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php +++ b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php @@ -11,6 +11,7 @@ use OCA\UserStatus\BackgroundJob\ClearOldStatusesBackgroundJob; use OCA\UserStatus\Db\UserStatusMapper; +use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Utility\ITimeFactory; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -36,6 +37,9 @@ public function testRun(): void { $this->mapper->expects($this->once()) ->method('clearStatusesOlderThan') ->with(437, 1337); + $this->mapper->expects($this->once()) + ->method('deleteStrandedBackups') + ->with(StatusService::AUTOMATED_MESSAGE_IDS); $this->time->method('getTime') ->willReturn(1337); diff --git a/apps/user_status/tests/Unit/Command/RepairTest.php b/apps/user_status/tests/Unit/Command/RepairTest.php new file mode 100644 index 0000000000000..e4a7eb1d8e2c2 --- /dev/null +++ b/apps/user_status/tests/Unit/Command/RepairTest.php @@ -0,0 +1,85 @@ +mapper = $this->createMock(UserStatusMapper::class); + $this->tester = new CommandTester(new Repair($this->mapper)); + } + + public function testRepairsEverything(): void { + $this->mapper->expects($this->once()) + ->method('findStatusesWithoutBackupFlagIds') + ->willReturn([1, 2]); + $this->mapper->expects($this->once()) + ->method('normalizeBackupFlagByIds') + ->with([1, 2]) + ->willReturn(2); + + $this->mapper->expects($this->once()) + ->method('findOrphanedAutomatedStatusIds') + ->with(StatusService::AUTOMATED_MESSAGE_IDS) + ->willReturn([7, 8, 9]); + $this->mapper->expects($this->once()) + ->method('findStrandedBackupIds') + ->with(StatusService::AUTOMATED_MESSAGE_IDS) + ->willReturn([11, 12, 13, 14]); + $this->mapper->expects($this->exactly(2)) + ->method('deleteByIds') + ->willReturnCallback(static fn (array $ids): int => count($ids)); + + self::assertSame(Command::SUCCESS, $this->tester->execute([])); + + $display = $this->tester->getDisplay(); + self::assertStringContainsString('2', $display); + self::assertStringContainsString('3', $display); + self::assertStringContainsString('4', $display); + } + + public function testDryRunChangesNothing(): void { + $this->mapper->method('findStatusesWithoutBackupFlagIds')->willReturn([1, 2]); + $this->mapper->method('findOrphanedAutomatedStatusIds')->willReturn([7, 8, 9]); + $this->mapper->method('findStrandedBackupIds')->willReturn([11, 12, 13, 14]); + + $this->mapper->expects($this->never())->method('normalizeBackupFlagByIds'); + $this->mapper->expects($this->never())->method('deleteByIds'); + $this->mapper->expects($this->never())->method('deleteStrandedBackups'); + + self::assertSame(Command::SUCCESS, $this->tester->execute(['--dry-run' => true])); + + self::assertStringContainsString('dry run', strtolower($this->tester->getDisplay())); + } + + public function testNothingToRepair(): void { + $this->mapper->method('findStatusesWithoutBackupFlagIds')->willReturn([]); + $this->mapper->method('findOrphanedAutomatedStatusIds')->willReturn([]); + $this->mapper->method('findStrandedBackupIds')->willReturn([]); + + // Nothing to normalise and nothing to delete. + $this->mapper->expects($this->never())->method('normalizeBackupFlagByIds'); + $this->mapper->expects($this->never())->method('deleteByIds'); + + self::assertSame(Command::SUCCESS, $this->tester->execute([])); + } +} diff --git a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php index a697571bd5709..4b03fa0ab9ba6 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -405,4 +405,216 @@ public function testRestoreBackupStatuses(): void { $this->assertEquals(true, $user3Status->getIsBackup()); $this->assertEquals('Vacationing', $user3Status->getCustomMessage()); } + + /** + * @param string[] $liveMessageIds keyed by user id; null means no live row + */ + private function insertBackupWithLiveStatus(string $userId, ?string $liveMessageId): void { + $backup = new UserStatus(); + $backup->setUserId('_' . $userId); + $backup->setStatus('online'); + $backup->setStatusTimestamp(5000); + $backup->setIsUserDefined(false); + $backup->setIsBackup(true); + $this->mapper->insert($backup); + + if ($liveMessageId === null) { + return; + } + + $live = new UserStatus(); + $live->setUserId($userId); + $live->setStatus('busy'); + $live->setStatusTimestamp(6000); + $live->setIsUserDefined(true); + $live->setIsBackup(false); + $live->setMessageId($liveMessageId === '' ? null : $liveMessageId); + $this->mapper->insert($live); + } + + public function testDeleteStrandedBackupsWithNoBackups(): void { + $this->insertSampleStatuses(); + + $this->assertSame(0, $this->mapper->deleteStrandedBackups(['meeting', 'call'])); + $this->assertCount(3, $this->mapper->findAll()); + } + + public function testDeleteStrandedBackupsKeepsBackupsOfAutomatedStatuses(): void { + $this->insertBackupWithLiveStatus('user1', 'meeting'); + $this->insertBackupWithLiveStatus('user2', 'call'); + $this->insertBackupWithLiveStatus('user3', 'availability'); + $this->insertBackupWithLiveStatus('user4', 'out-of-office'); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call', 'availability', 'out-of-office']); + + $this->assertSame(0, $deleted); + foreach (['user1', 'user2', 'user3', 'user4'] as $userId) { + $this->assertEquals('_' . $userId, $this->mapper->findByUserId($userId, true)->getUserId()); + } + } + + public function testDeleteStrandedBackupsRemovesBackupWithoutLiveStatus(): void { + $this->insertBackupWithLiveStatus('user1', null); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(1, $deleted); + $this->expectException(DoesNotExistException::class); + $this->mapper->findByUserId('user1', true); + } + + public function testDeleteStrandedBackupsRemovesBackupWhenLiveStatusHasNoMessageId(): void { + $this->insertBackupWithLiveStatus('user1', ''); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(1, $deleted); + // The live status must survive. + $this->assertEquals('user1', $this->mapper->findByUserId('user1')->getUserId()); + } + + public function testDeleteStrandedBackupsRemovesBackupWhenLiveStatusIsUserDefinedMessage(): void { + $this->insertBackupWithLiveStatus('user1', 'vacationing'); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(1, $deleted); + $this->assertEquals('vacationing', $this->mapper->findByUserId('user1')->getMessageId()); + } + + public function testDeleteStrandedBackupsOnlyRemovesTheStrandedOnes(): void { + $this->insertBackupWithLiveStatus('keepme', 'meeting'); + $this->insertBackupWithLiveStatus('stranded1', 'vacationing'); + $this->insertBackupWithLiveStatus('stranded2', null); + $this->insertBackupWithLiveStatus('keepme2', 'call'); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(2, $deleted); + $this->assertEquals('_keepme', $this->mapper->findByUserId('keepme', true)->getUserId()); + $this->assertEquals('_keepme2', $this->mapper->findByUserId('keepme2', true)->getUserId()); + foreach (['stranded1', 'stranded2'] as $userId) { + try { + $this->mapper->findByUserId($userId, true); + $this->fail("Backup for $userId should have been deleted"); + } catch (DoesNotExistException) { + } + } + } + + public function testDeleteStrandedBackupsDoesNotConfuseUsersWithSimilarNames(): void { + // '_user1' as a backup of 'user1', plus a real user literally named + // 'user1x' whose backup must be judged on its own live row. + $this->insertBackupWithLiveStatus('user1', 'meeting'); + $this->insertBackupWithLiveStatus('user1x', 'vacationing'); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(1, $deleted); + $this->assertEquals('_user1', $this->mapper->findByUserId('user1', true)->getUserId()); + $this->expectException(DoesNotExistException::class); + $this->mapper->findByUserId('user1x', true); + } + + public function testFindStrandedBackupIds(): void { + $this->insertBackupWithLiveStatus('keepme', 'meeting'); + $this->insertBackupWithLiveStatus('stranded', 'vacationing'); + + $ids = $this->mapper->findStrandedBackupIds(['meeting', 'call']); + + $this->assertCount(1, $ids); + $this->assertSame( + $this->mapper->findByUserId('stranded', true)->getId(), + $ids[0], + ); + } + + public function testFindStrandedBackupIdsDoesNotDelete(): void { + $this->insertBackupWithLiveStatus('stranded', 'vacationing'); + + $this->mapper->findStrandedBackupIds(['meeting']); + + $this->assertEquals('_stranded', $this->mapper->findByUserId('stranded', true)->getUserId()); + } + + public function testFindOrphanedAutomatedStatusIds(): void { + // Live automated status with no backup to revert into: orphaned. + $orphan = new UserStatus(); + $orphan->setUserId('orphan'); + $orphan->setStatus('busy'); + $orphan->setStatusTimestamp(5000); + $orphan->setIsUserDefined(true); + $orphan->setIsBackup(false); + $orphan->setMessageId('meeting'); + $this->mapper->insert($orphan); + + // Same shape but with a backup: an ongoing meeting, must be left alone. + $this->insertBackupWithLiveStatus('inmeeting', 'meeting'); + + // A status the user set themselves: not automated, must be left alone. + $own = new UserStatus(); + $own->setUserId('ownstatus'); + $own->setStatus('dnd'); + $own->setStatusTimestamp(5000); + $own->setIsUserDefined(true); + $own->setIsBackup(false); + $own->setMessageId('vacationing'); + $this->mapper->insert($own); + + $ids = $this->mapper->findOrphanedAutomatedStatusIds(['meeting', 'call']); + + $this->assertCount(1, $ids); + $this->assertSame($this->mapper->findByUserId('orphan')->getId(), $ids[0]); + } + + public function testFindOrphanedAutomatedStatusIdsIgnoresBackupRows(): void { + // A backup row that happens to carry an automated message id must never + // be reported as an orphaned live status. + $backup = new UserStatus(); + $backup->setUserId('_someone'); + $backup->setStatus('busy'); + $backup->setStatusTimestamp(5000); + $backup->setIsUserDefined(true); + $backup->setIsBackup(true); + $backup->setMessageId('meeting'); + $this->mapper->insert($backup); + + $this->assertSame([], $this->mapper->findOrphanedAutomatedStatusIds(['meeting', 'call'])); + } + + public function testFindOrphanedAutomatedStatusIdsWithEmptyAutomatedList(): void { + $this->insertBackupWithLiveStatus('user1', 'meeting'); + + $this->assertSame([], $this->mapper->findOrphanedAutomatedStatusIds([])); + } + + public function testNormalizeBackupFlag(): void { + $this->insertSampleStatuses(); + self::$realDatabase->executeStatement( + 'UPDATE `*PREFIX*user_status` SET `is_backup` = NULL WHERE `user_id` = ?', + ['user1'], + ); + + $ids = $this->mapper->findStatusesWithoutBackupFlagIds(); + $this->assertCount(1, $ids); + $this->assertSame(1, $this->mapper->normalizeBackupFlagByIds($ids)); + $this->assertSame([], $this->mapper->findStatusesWithoutBackupFlagIds()); + // The row is visible to findAll() again. + $this->assertCount(3, $this->mapper->findAll()); + } + + public function testNormalizeBackupFlagWithNothingToDo(): void { + $this->insertSampleStatuses(); + + $this->assertSame([], $this->mapper->findStatusesWithoutBackupFlagIds()); + $this->assertSame(0, $this->mapper->normalizeBackupFlagByIds([])); + } + + public function testDeleteStrandedBackupsWithEmptyAutomatedListRemovesAll(): void { + $this->insertBackupWithLiveStatus('user1', 'meeting'); + $this->insertBackupWithLiveStatus('user2', 'call'); + + // Defensive: with nothing considered automated, every backup is stranded. + $this->assertSame(2, $this->mapper->deleteStrandedBackups([])); + } } diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php index 69128033631e0..b42370991503c 100644 --- a/apps/user_status/tests/Unit/Service/StatusServiceTest.php +++ b/apps/user_status/tests/Unit/Service/StatusServiceTest.php @@ -700,6 +700,106 @@ public function testBackup(): void { $this->assertTrue($this->service->backupCurrentStatus('john')); } + public function testBackupNothingToBackUp(): void { + // A user without a status row has nothing to move into a backup. That + // is not a conflict, so the automated status may still be applied. + $this->mapper->expects($this->once()) + ->method('createBackupStatus') + ->with('john') + ->willReturn(false); + + $this->assertTrue($this->service->backupCurrentStatus('john')); + } + + public function testRevertUserStatusWithoutBackupClearsAutomatedStatus(): void { + $this->mapper->expects($this->once()) + ->method('findByUserId') + ->with('john', true) + ->willThrowException(new DoesNotExistException('')); + + // There is nothing to restore, but the unreachable automated status + // must still be removed so the user is not stuck on it. + $this->mapper->expects($this->once()) + ->method('deleteCurrentStatusToRestoreBackup') + ->with('john', 'meeting') + ->willReturn(true); + + $this->assertNull($this->service->revertUserStatus('john', 'meeting')); + } + + public function testRevertUserStatusWithoutBackupAndNoMatchingStatus(): void { + $this->mapper->expects($this->once()) + ->method('findByUserId') + ->with('john', true) + ->willThrowException(new DoesNotExistException('')); + + // Nothing matched, so nothing was removed. Must not blow up. + $this->mapper->expects($this->once()) + ->method('deleteCurrentStatusToRestoreBackup') + ->with('john', 'meeting') + ->willReturn(false); + + $this->mapper->expects($this->never())->method('update'); + + $this->assertNull($this->service->revertUserStatus('john', 'meeting')); + } + + public function testRevertUserStatusRefreshesTimestamp(): void { + $backup = new UserStatus(); + $backup->setId(2); + $backup->setUserId('_john'); + $backup->setStatus(IUserStatus::ONLINE); + $backup->setStatusTimestamp(1000); + $backup->setIsUserDefined(false); + $backup->setIsBackup(true); + + $this->mapper->expects($this->once()) + ->method('findByUserId') + ->with('john', true) + ->willReturn($backup); + $this->mapper->expects($this->once()) + ->method('deleteCurrentStatusToRestoreBackup') + ->with('john', 'meeting') + ->willReturn(true); + $this->timeFactory->method('getTime')->willReturn(9999); + + $this->mapper->expects($this->once()) + ->method('update') + ->willReturnArgument(0); + + $reverted = $this->service->revertUserStatus('john', 'meeting'); + + self::assertNotNull($reverted); + self::assertSame('john', $reverted->getUserId()); + self::assertFalse($reverted->getIsBackup()); + self::assertSame( + 9999, + $reverted->getStatusTimestamp(), + 'A restored status must not keep the timestamp from before the automated status', + ); + } + + public function testRevertUserStatusManuallyStillPromotesOfflineToOnline(): void { + $backup = new UserStatus(); + $backup->setId(2); + $backup->setUserId('_john'); + $backup->setStatus(IUserStatus::OFFLINE); + $backup->setStatusTimestamp(1000); + $backup->setIsUserDefined(false); + $backup->setIsBackup(true); + + $this->mapper->method('findByUserId')->with('john', true)->willReturn($backup); + $this->mapper->method('deleteCurrentStatusToRestoreBackup')->willReturn(true); + $this->timeFactory->method('getTime')->willReturn(9999); + $this->mapper->expects($this->once())->method('update')->willReturnArgument(0); + + $reverted = $this->service->revertUserStatus('john', 'meeting', true); + + self::assertNotNull($reverted); + self::assertSame(IUserStatus::ONLINE, $reverted->getStatus()); + self::assertSame(9999, $reverted->getStatusTimestamp()); + } + public function testRevertMultipleUserStatus(): void { $john = new UserStatus(); $john->setId(1);