Skip to content

Commit 3af8a43

Browse files
AndyScherzingerAndyScherzinger
authored
Merge pull request #61618 from nextcloud/backport/59535/stable32
[stable32] fix(user_status): stabilise the user_status
2 parents 0904c13 + 6fd5fe7 commit 3af8a43

5 files changed

Lines changed: 92 additions & 8 deletions

File tree

‎apps/user_status/lib/Db/UserStatusMapper.php‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -36,7 +36,8 @@ public function findAll(?int $limit = null, ?int $offset = null):array {
3636
$qb = $this->db->getQueryBuilder();
3737
$qb
3838
->select('*')
39-
->from($this->tableName);
39+
->from($this->tableName)
40+
->where($qb->expr()->eq('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)));
4041

4142
if ($limit !== null) {
4243
$qb->setMaxResults($limit);
@@ -68,7 +69,7 @@ public function findAllRecent(?int $limit = null, ?int $offset = null): array {
6869
$qb->expr()->isNotNull('custom_icon'),
6970
$qb->expr()->isNotNull('custom_message'),
7071
),
71-
$qb->expr()->notLike('user_id', $qb->createNamedParameter($this->db->escapeLikeParameter('_') . '%'))
72+
$qb->expr()->eq('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL))
7273
));
7374

7475
if ($limit !== null) {
@@ -125,7 +126,8 @@ public function clearStatusesOlderThan(int $olderThan, int $now): void {
125126
->andWhere($qb->expr()->orX(
126127
$qb->expr()->eq('is_user_defined', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL), IQueryBuilder::PARAM_BOOL),
127128
$qb->expr()->eq('status', $qb->createNamedParameter(IUserStatus::ONLINE))
128-
));
129+
))
130+
->andWhere($qb->expr()->eq('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)));
129131

130132
$qb->executeStatement();
131133
}
@@ -139,7 +141,8 @@ public function clearOlderThanClearAt(int $timestamp): void {
139141
$qb = $this->db->getQueryBuilder();
140142
$qb->delete($this->tableName)
141143
->where($qb->expr()->isNotNull('clear_at'))
142-
->andWhere($qb->expr()->lte('clear_at', $qb->createNamedParameter($timestamp, IQueryBuilder::PARAM_INT)));
144+
->andWhere($qb->expr()->lte('clear_at', $qb->createNamedParameter($timestamp, IQueryBuilder::PARAM_INT)))
145+
->andWhere($qb->expr()->eq('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)));
143146

144147
$qb->executeStatement();
145148
}

‎apps/user_status/lib/Listener/UserDeletedListener.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -43,5 +43,6 @@ public function handle(Event $event): void {
4343

4444
$user = $event->getUser();
4545
$this->service->removeUserStatus($user->getUID());
46+
$this->service->removeBackupUserStatus($user->getUID());
4647
}
4748
}

‎apps/user_status/lib/Service/StatusService.php‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,6 +284,7 @@ public function setUserStatus(string $userId,
284284

285285
if ($createBackup) {
286286
if ($this->backupCurrentStatus($userId) === false) {
287+
$this->logger->debug('Automated status change aborted for user ' . $userId . ': backup already exists (another automated status is active)', ['app' => 'user_status']);
287288
return null; // Already a status set automatically => abort.
288289
}
289290

@@ -516,6 +517,7 @@ public function backupCurrentStatus(string $userId): bool {
516517
return true;
517518
} catch (Exception $ex) {
518519
if ($ex->getReason() === Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
520+
$this->logger->debug('Backup status already exists for user ' . $userId . ', skipping backup creation', ['app' => 'user_status']);
519521
return false;
520522
}
521523
throw $ex;
@@ -533,7 +535,7 @@ public function revertUserStatus(string $userId, string $messageId, bool $revert
533535

534536
$deleted = $this->mapper->deleteCurrentStatusToRestoreBackup($userId, $messageId);
535537
if (!$deleted) {
536-
// Another status is set automatically or no status, do nothing
538+
$this->logger->debug('Status revert skipped for user ' . $userId . ': current status does not match messageId "' . $messageId . '" (user may have changed status manually)', ['app' => 'user_status']);
537539
return null;
538540
}
539541

@@ -589,10 +591,10 @@ protected function insertWithoutThrowingUniqueConstrain(UserStatus $userStatus):
589591
try {
590592
return $this->mapper->insert($userStatus);
591593
} catch (Exception $e) {
592-
// Ignore if a parallel request already set the status
593594
if ($e->getReason() !== Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION) {
594595
throw $e;
595596
}
597+
$this->logger->debug('Concurrent insert conflict for user ' . $userStatus->getUserId() . ': status was already set by a parallel request', ['app' => 'user_status']);
596598
}
597599
return $userStatus;
598600
}

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

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,24 @@ public function testGetFindAll(): void {
4747
$this->assertEquals('user2', $offsetResults[0]->getUserId());
4848
}
4949

50+
public function testFindAllExcludesBackups(): void {
51+
$this->insertSampleStatuses();
52+
53+
$backup = new UserStatus();
54+
$backup->setUserId('_backupuser');
55+
$backup->setStatus('dnd');
56+
$backup->setStatusTimestamp(5000);
57+
$backup->setIsUserDefined(true);
58+
$backup->setIsBackup(true);
59+
$this->mapper->insert($backup);
60+
61+
$allResults = $this->mapper->findAll();
62+
$this->assertCount(3, $allResults);
63+
64+
$userIds = array_map(fn ($s) => $s->getUserId(), $allResults);
65+
$this->assertNotContains('_backupuser', $userIds);
66+
}
67+
5068
public function testFindAllRecent(): void {
5169
$this->insertSampleStatuses();
5270

@@ -56,6 +74,26 @@ public function testFindAllRecent(): void {
5674
$this->assertEquals('user1', $allResults[1]->getUserId());
5775
}
5876

77+
public function testFindAllRecentExcludesBackups(): void {
78+
$this->insertSampleStatuses();
79+
80+
$backup = new UserStatus();
81+
$backup->setUserId('_backupuser');
82+
$backup->setStatus('dnd');
83+
$backup->setStatusTimestamp(7000);
84+
$backup->setStatusMessageTimestamp(7000);
85+
$backup->setIsUserDefined(true);
86+
$backup->setIsBackup(true);
87+
$backup->setCustomMessage('Backed up status');
88+
$this->mapper->insert($backup);
89+
90+
$allResults = $this->mapper->findAllRecent(10, 0);
91+
$this->assertCount(2, $allResults);
92+
93+
$userIds = array_map(fn ($s) => $s->getUserId(), $allResults);
94+
$this->assertNotContains('_backupuser', $userIds);
95+
}
96+
5997
public function testGetFind(): void {
6098
$this->insertSampleStatuses();
6199

@@ -190,6 +228,43 @@ public function testClearOlderThanClearAt(): void {
190228
$this->mapper->findByUserId('user1');
191229
}
192230

231+
public function testClearOlderThanClearAtPreservesBackups(): void {
232+
$backup = new UserStatus();
233+
$backup->setUserId('_user1');
234+
$backup->setStatus('dnd');
235+
$backup->setStatusTimestamp(5000);
236+
$backup->setIsUserDefined(true);
237+
$backup->setIsBackup(true);
238+
$backup->setClearAt(50000);
239+
$this->mapper->insert($backup);
240+
241+
$this->mapper->clearOlderThanClearAt(55000);
242+
243+
// Backup should survive cleanup despite having expired clear_at
244+
$backupStatus = $this->mapper->findByUserId('user1', true);
245+
$this->assertEquals('_user1', $backupStatus->getUserId());
246+
$this->assertEquals('dnd', $backupStatus->getStatus());
247+
$this->assertTrue($backupStatus->getIsBackup());
248+
}
249+
250+
public function testClearStatusesOlderThanPreservesBackups(): void {
251+
$backup = new UserStatus();
252+
$backup->setUserId('_user1');
253+
$backup->setStatus('online');
254+
$backup->setStatusTimestamp(1000);
255+
$backup->setIsUserDefined(false);
256+
$backup->setIsBackup(true);
257+
$this->mapper->insert($backup);
258+
259+
$this->mapper->clearStatusesOlderThan(5000, 8000);
260+
261+
// Backup should survive cleanup despite old timestamp
262+
$backupStatus = $this->mapper->findByUserId('user1', true);
263+
$this->assertEquals('online', $backupStatus->getStatus());
264+
$this->assertFalse($backupStatus->getIsUserDefined());
265+
$this->assertEquals(1000, $backupStatus->getStatusTimestamp());
266+
}
267+
193268
private function insertSampleStatuses(): void {
194269
$userStatus1 = new UserStatus();
195270
$userStatus1->setUserId('admin');

‎apps/user_status/tests/Unit/Listener/UserDeletedListenerTest.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -29,14 +29,17 @@ protected function setUp(): void {
2929

3030
public function testHandleWithCorrectEvent(): void {
3131
$user = $this->createMock(IUser::class);
32-
$user->expects($this->once())
33-
->method('getUID')
32+
$user->method('getUID')
3433
->willReturn('john.doe');
3534

3635
$this->service->expects($this->once())
3736
->method('removeUserStatus')
3837
->with('john.doe');
3938

39+
$this->service->expects($this->once())
40+
->method('removeBackupUserStatus')
41+
->with('john.doe');
42+
4043
$event = new UserDeletedEvent($user);
4144
$this->listener->handle($event);
4245
}

0 commit comments

Comments
 (0)