Merge pull request #46077 from nextcloud/bugfix/noid/user-status-auto…

…mation fix(userstatus): Fix user status automation in real-life scenario
nextcloud · Jun 25, 2024 · eed6216 · eed6216
2 parents 9496ce6 + 2c977d2
commit eed6216
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 21 deletions.
diff --git a/apps/dav/lib/BackgroundJob/UserStatusAutomation.php b/apps/dav/lib/BackgroundJob/UserStatusAutomation.php
@@ -201,9 +201,7 @@ private function processAvailability(string $property, string $userId, bool $has
 			return;
 		}
 
-		$this->logger->debug('User is currently NOT available, reverting call status if applicable and then setting DND');
-		// The DND status automation is more important than the "Away - In call" so we also restore that one if it exists.
-		$this->manager->revertUserStatus($userId, IUserStatus::MESSAGE_CALL, IUserStatus::AWAY);
+		$this->logger->debug('User is currently NOT available, reverting call and meeting status if applicable and then setting DND');
 		$this->manager->setUserStatus($userId, IUserStatus::MESSAGE_AVAILABILITY, IUserStatus::DND, true);
 		$this->logger->debug('User status automation ran');
 	}
@@ -232,10 +230,8 @@ private function processOutOfOfficeData(IUser $user, ?IOutOfOfficeData $ooo): bo
 		}
 
 		$this->logger->debug('User is currently in an OOO period, reverting other automated status and setting OOO DND status');
-		// Revert both a possible 'CALL - away' and 'office hours - DND' status
-		$this->manager->revertUserStatus($user->getUID(), IUserStatus::MESSAGE_CALL, IUserStatus::DND);
-		$this->manager->revertUserStatus($user->getUID(), IUserStatus::MESSAGE_AVAILABILITY, IUserStatus::DND);
 		$this->manager->setUserStatus($user->getUID(), IUserStatus::MESSAGE_OUT_OF_OFFICE, IUserStatus::DND, true, $ooo->getShortMessage());
+
 		// Run at the end of an ooo period to return to availability / regular user status
 		// If it's overwritten by a custom status in the meantime, there's nothing we can do about it
 		$this->setLastRunToNextToggleTime($user->getUID(), $ooo->getEndDate());

diff --git a/apps/dav/tests/unit/BackgroundJob/UserStatusAutomationTest.php b/apps/dav/tests/unit/BackgroundJob/UserStatusAutomationTest.php
@@ -108,8 +108,6 @@ public function testRunNoOOO(string $ruleDay, string $currentTime, bool $isAvail
 			->willReturn(new \DateTime($currentTime, new \DateTimeZone('UTC')));
 		$this->logger->expects(self::exactly(4))
 			->method('debug');
-		$this->statusManager->expects(self::exactly(2))
-			->method('revertUserStatus');
 		if (!$isAvailable) {
 			$this->statusManager->expects(self::once())
 				->method('setUserStatus')
@@ -172,8 +170,6 @@ public function testRunNoAvailabilityNoOOO(): void {
 			->willReturn('yes');
 		$this->time->method('getDateTime')
 			->willReturn(new \DateTime('2023-02-24 13:58:24.479357', new \DateTimeZone('UTC')));
-		$this->statusManager->expects($this->exactly(3))
-			->method('revertUserStatus');
 		$this->jobList->expects($this->once())
 			->method('remove')
 			->with(UserStatusAutomation::class, ['userId' => 'user']);
@@ -207,8 +203,6 @@ public function testRunNoAvailabilityWithOOO(): void {
 		$this->coordinator->expects(self::once())
 			->method('isInEffect')
 			->willReturn(true);
-		$this->statusManager->expects($this->exactly(2))
-			->method('revertUserStatus');
 		$this->statusManager->expects(self::once())
 			->method('setUserStatus')
 			->with('user', IUserStatus::MESSAGE_OUT_OF_OFFICE, IUserStatus::DND, true, $ooo->getShortMessage());

diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php
@@ -22,6 +22,7 @@
 use OCP\IEmojiHelper;
 use OCP\IUserManager;
 use OCP\UserStatus\IUserStatus;
+use Psr\Log\LoggerInterface;
 use function in_array;
 
 /**
@@ -63,12 +64,15 @@ class StatusService {
 	/** @var int */
 	public const MAXIMUM_MESSAGE_LENGTH = 80;
 
-	public function __construct(private UserStatusMapper $mapper,
+	public function __construct(
+		private UserStatusMapper $mapper,
 		private ITimeFactory $timeFactory,
 		private PredefinedStatusService $predefinedStatusService,
 		private IEmojiHelper $emojiHelper,
 		private IConfig $config,
-		private IUserManager $userManager) {
+		private IUserManager $userManager,
+		private LoggerInterface $logger,
+	) {
 		$this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes';
 		$this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes';
 		$this->shareeEnumerationPhone = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_phone', 'no') === 'yes';
@@ -244,8 +248,28 @@ public function setUserStatus(string $userId,
 			$userStatus->setUserId($userId);
 		}
 
-		// CALL trumps CALENDAR status, but we don't need to do anything but overwrite the message
-		if ($userStatus->getMessageId() === IUserStatus::MESSAGE_CALENDAR_BUSY && $messageId === IUserStatus::MESSAGE_CALL) {
+		$updateStatus = false;
+		if ($messageId === IUserStatus::MESSAGE_OUT_OF_OFFICE) {
+			// OUT_OF_OFFICE trumps AVAILABILITY, CALL and CALENDAR status
+			$updateStatus = $userStatus->getMessageId() === IUserStatus::MESSAGE_AVAILABILITY || $userStatus->getMessageId() === IUserStatus::MESSAGE_CALL || $userStatus->getMessageId() === IUserStatus::MESSAGE_CALENDAR_BUSY;
+		} elseif ($messageId === IUserStatus::MESSAGE_AVAILABILITY) {
+			// AVAILABILITY trumps CALL and CALENDAR status
+			$updateStatus = $userStatus->getMessageId() === IUserStatus::MESSAGE_CALL || $userStatus->getMessageId() === IUserStatus::MESSAGE_CALENDAR_BUSY;
+		} elseif ($messageId === IUserStatus::MESSAGE_CALL) {
+			// CALL trumps CALENDAR status
+			$updateStatus = $userStatus->getMessageId() === IUserStatus::MESSAGE_CALENDAR_BUSY;
+		}
+
+		if ($messageId === IUserStatus::MESSAGE_OUT_OF_OFFICE || $messageId === IUserStatus::MESSAGE_AVAILABILITY || $messageId === IUserStatus::MESSAGE_CALL || $messageId === IUserStatus::MESSAGE_CALENDAR_BUSY) {
+			if ($updateStatus) {
+				$this->logger->debug('User ' . $userId . ' is currently NOT available, overwriting status [status: ' . $userStatus->getStatus() . ', messageId: ' . json_encode($userStatus->getMessageId()) . ']', ['app' => 'dav']);
+			} else {
+				$this->logger->debug('User ' . $userId . ' is currently NOT available, but we are NOT overwriting status [status: ' . $userStatus->getStatus() . ', messageId: ' . json_encode($userStatus->getMessageId()) . ']', ['app' => 'dav']);
+			}
+		}
+
+		// There should be a backup already or none is needed. So we take a shortcut.
+		if ($updateStatus) {
 			$userStatus->setStatus($status);
 			$userStatus->setStatusTimestamp($this->timeFactory->getTime());
 			$userStatus->setIsUserDefined(true);
@@ -265,7 +289,7 @@ public function setUserStatus(string $userId,
 
 			// If we just created the backup
 			// we need to create a new status to insert
-			// Unfortunatley there's no way to unset the DB ID on an Entity
+			// Unfortunately there's no way to unset the DB ID on an Entity
 			$userStatus = new UserStatus();
 			$userStatus->setUserId($userId);
 		}

diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php
@@ -27,6 +27,7 @@
 use OCP\IUserManager;
 use OCP\UserStatus\IUserStatus;
 use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 class StatusServiceTest extends TestCase {
@@ -49,6 +50,9 @@ class StatusServiceTest extends TestCase {
 	/** @var IUserManager|MockObject  */
 	private $userManager;
 
+	/** @var LoggerInterface|MockObject  */
+	private $logger;
+
 	private StatusService $service;
 
 	protected function setUp(): void {
@@ -60,6 +64,7 @@ protected function setUp(): void {
 		$this->emojiHelper = $this->createMock(IEmojiHelper::class);
 		$this->userManager = $this->createMock(IUserManager::class);
 		$this->config = $this->createMock(IConfig::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
 
 		$this->config->method('getAppValue')
 			->willReturnMap([
@@ -72,7 +77,8 @@ protected function setUp(): void {
 			$this->predefinedStatusService,
 			$this->emojiHelper,
 			$this->config,
-			$this->userManager
+			$this->userManager,
+			$this->logger,
 		);
 	}
 
@@ -128,7 +134,8 @@ public function testFindAllRecentStatusChangesNoEnumeration(): void {
 			$this->predefinedStatusService,
 			$this->emojiHelper,
 			$this->config,
-			$this->userManager
+			$this->userManager,
+			$this->logger,
 		);
 
 		$this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50));
@@ -147,7 +154,8 @@ public function testFindAllRecentStatusChangesNoEnumeration(): void {
 			$this->predefinedStatusService,
 			$this->emojiHelper,
 			$this->config,
-			$this->userManager
+			$this->userManager,
+			$this->logger,
 		);
 
 		$this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50));
@@ -731,7 +739,6 @@ public function testBackupThrowsOther(): void {
 	}
 
 	public function testBackup(): void {
-		$e = new Exception('', Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION);
 		$this->mapper->expects($this->once())
 			->method('createBackupStatus')
 			->with('john')
@@ -807,4 +814,63 @@ public function testRevertMultipleUserStatus(): void {
 
 		$this->service->revertMultipleUserStatus(['john', 'nobackup', 'backuponly', 'nobackupanddnd'], 'call');
 	}
+
+	public function dataSetUserStatus(): array {
+		return [
+			[IUserStatus::MESSAGE_CALENDAR_BUSY, '', false],
+
+			// Call > Meeting
+			[IUserStatus::MESSAGE_CALENDAR_BUSY, IUserStatus::MESSAGE_CALL, false],
+			[IUserStatus::MESSAGE_CALL, IUserStatus::MESSAGE_CALENDAR_BUSY, true],
+
+			// Availability > Call&Meeting
+			[IUserStatus::MESSAGE_CALENDAR_BUSY, IUserStatus::MESSAGE_AVAILABILITY, false],
+			[IUserStatus::MESSAGE_CALL, IUserStatus::MESSAGE_AVAILABILITY, false],
+			[IUserStatus::MESSAGE_AVAILABILITY, IUserStatus::MESSAGE_CALENDAR_BUSY, true],
+			[IUserStatus::MESSAGE_AVAILABILITY, IUserStatus::MESSAGE_CALL, true],
+
+			// Out-of-office > Availability&Call&Meeting
+			[IUserStatus::MESSAGE_AVAILABILITY, IUserStatus::MESSAGE_OUT_OF_OFFICE, false],
+			[IUserStatus::MESSAGE_CALENDAR_BUSY, IUserStatus::MESSAGE_OUT_OF_OFFICE, false],
+			[IUserStatus::MESSAGE_CALL, IUserStatus::MESSAGE_OUT_OF_OFFICE, false],
+			[IUserStatus::MESSAGE_OUT_OF_OFFICE, IUserStatus::MESSAGE_AVAILABILITY, true],
+			[IUserStatus::MESSAGE_OUT_OF_OFFICE, IUserStatus::MESSAGE_CALENDAR_BUSY, true],
+			[IUserStatus::MESSAGE_OUT_OF_OFFICE, IUserStatus::MESSAGE_CALL, true],
+		];
+	}
+
+	/**
+	 * @dataProvider dataSetUserStatus
+	 */
+	public function testSetUserStatus(string $messageId, string $oldMessageId, bool $expectedUpdateShortcut): void {
+		$previous = new UserStatus();
+		$previous->setId(1);
+		$previous->setStatus(IUserStatus::AWAY);
+		$previous->setStatusTimestamp(1337);
+		$previous->setIsUserDefined(false);
+		$previous->setMessageId($oldMessageId);
+		$previous->setUserId('john');
+		$previous->setIsBackup(false);
+
+		$this->mapper->expects($this->once())
+			->method('findByUserId')
+			->with('john')
+			->willReturn($previous);
+
+		$e = DbalException::wrap($this->createMock(UniqueConstraintViolationException::class));
+		$this->mapper->expects($expectedUpdateShortcut ? $this->never() : $this->once())
+			->method('createBackupStatus')
+			->willThrowException($e);
+
+		$this->mapper->expects($this->any())
+			->method('update')
+			->willReturnArgument(0);
+
+		$this->predefinedStatusService->expects($this->once())
+			->method('isValidId')
+			->with($messageId)
+			->willReturn(true);
+
+		$this->service->setUserStatus('john', IUserStatus::DND, $messageId, true);
+	}
 }