enh(user_status): set OoO status and message #41461

miaulalala · 2023-11-14T14:19:33Z

Resolves: Out-of-office: Update user status #41378

Summary

Set the user status to DND and the custom message to \OCA\DAV\Db\Absence::getStatus when an absence is in effect.

TODO

determine if an absence is in effect

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
~~Screenshots before/after for front-end changes~~
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)

apps/user_status/lib/Service/StatusService.php

ChristophWurst

Neat

ChristophWurst · 2023-11-14T16:26:24Z

Going into DND works if I clear my test user's status and backup status from oc_user_status. Edit: but status is saved as user defined?! This means absence is never checked again, so DND will not end automatically.

Does not do anything if there is an existing backup status. Does this conflict with another automation?

ChristophWurst

Does not fully work as expected yet

apps/dav/lib/Service/AbsenceService.php

apps/dav/lib/Db/Absence.php

apps/dav/lib/Service/AbsenceService.php

lib/private/User/AvailabilityCoordinator.php

apps/user_status/lib/Service/StatusService.php

apps/dav/lib/Service/AbsenceService.php

apps/user_status/lib/Service/StatusService.php

miaulalala · 2023-11-14T21:12:08Z

Going into DND works if I clear my test user's status and backup status from oc_user_status. Edit: but status is saved as user defined?! This means absence is never checked again, so DND will not end automatically.

Does not do anything if there is an existing backup status. Does this conflict with another automation?

This is a general problem with the current code - there is no reverting a DND status if I use setUserStatus; but setStatus does not allow for custom messages.

This needs to be fixed in the follow up ticket #41441 which will add a method to allow to also set a custom message for the different automations we have. This will also help with the inflationary use of setUserStatus as that set isUserDefined to true whether that is correct or not.

apps/dav/lib/Db/Absence.php

ChristophWurst

Getting into OOO status works. Getting out doesn't

Like #41461 (comment) says

lib/private/User/AvailabilityCoordinator.php

apps/user_status/lib/Service/StatusService.php

ChristophWurst · 2023-11-16T15:54:47Z

What's missing here? :)

ChristophWurst · 2023-11-16T16:17:28Z

BUG: Could not resolve OCA\\DAV\\Listener\\UserPreferenceListener! Class \"OCA\\DAV\\Listener\\UserPreferenceListener\" does not exist when I try to update my availability

Signed-off-by: Anna Larch <[email protected]>

ChristophWurst · 2023-11-16T16:20:41Z

To keep things reviewable, please stay with fixup commits. I can't follow your changes if I have to review the full diff with every push 🙏

ChristophWurst · 2023-11-16T16:21:30Z

BUG (at least with previous HEAD commit): Online status and online status change to OOO and its message. When OOO ends, DND ends, but status stays at OOO message.

Edit: happens with latest state as well

ChristophWurst · 2023-11-16T16:22:52Z

BUG: online status is still always lost. I fully reset my status and am at Away after the next page reload

ChristophWurst · 2023-11-16T16:36:23Z

@miaulalala could you please drop a few lines describing how status automation now works conceptually? I see the old availability automation is gone. OOO changes are also reflected instantly, so I assume there is no caching yet?

ChristophWurst · 2023-11-16T16:48:00Z

apps/user_status/lib/Db/UserStatusMapper.php

 		$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('message_id', $qb->createNamedParameter($messageId)))


ChristophWurst · 2023-11-16T16:48:55Z

apps/user_status/lib/Listener/UserLiveStatusListener.php

-				$this->mapper->update($userStatus);
-			}
-		}
+		// The "online vs away" stuff will need to be reimplemented


does this leave the live status in any worse state than current master?

ChristophWurst · 2023-11-16T16:49:37Z

apps/user_status/lib/Listener/UserLiveStatusListener.php

-			}
-		}
+		// The "online vs away" stuff will need to be reimplemented
+		// the Status Listerner should only update the status, but not touch the messages


this is a change I don't think we can do for 28. this also needs new public APIs and likely breaking of the existing

ChristophWurst · 2023-11-16T16:51:06Z

apps/user_status/lib/Service/StatusService.php

+		// calendar status.
+		// If there is, we have a more up- to- date userDefined status which should take precedence.
+		$this->backupCurrentStatus($userId);
+		// @todo - when a user has the option set to set the status to DND outside of office hours, we should respect that


todo in this pr?

Certainly can do

do it here if this is a required change

skip if it's optional or not needed. I just want to make sure this isn't forgotten if required

ChristophWurst · 2023-11-16T16:52:30Z

apps/user_status/src/store/userStatus.js

@@ -92,13 +94,14 @@ const mutations = {
 	 * @param {string} data.icon The icon
 	 * @param {number} data.clearAt When to automatically clear the status
 	 */
-	setCustomMessage(state, { message, icon, clearAt }) {
+	setCustomMessage(state, { message, icon, clearAt, isUserDefined = false }) {


the frontend always sends false, so we could even remove the option

Even more confusing as everything the user can click in the frontend actually IS "user defined"

ChristophWurst · 2023-11-16T16:53:49Z

apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php

@@ -49,81 +52,89 @@ protected function setUp(): void {
 		$db = Server::get(IDBConnection::class);
 		$qb = $db->getQueryBuilder();
 		$qb->delete('user_status')->executeStatement();
+		$qb->delete('users')->executeStatement();


dangerous if someone runs this test without a separate testing environment. this happens occasionally.

make it target the specific user then?

sounds good!

ChristophWurst · 2023-11-16T16:53:58Z

apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php

+//			IUserStatus::ONLINE,
+//			null,
+//			false,
+//		);


ChristophWurst · 2023-11-16T16:54:06Z

apps/user_status/tests/Unit/Listener/UserLiveStatusListenerTest.php

+//					->method('insert');
+//				$this->mapper->expects($this->once())
+//					->method('update')
+//					->with($this->callback(function ($userStatus) use ($eventStatus, $eventTimestamp) {


ChristophWurst · 2023-11-16T17:02:08Z

BUG still can't save availability settings. now with

{
	"ocs": {
		"meta": {
			"status": "failure",
			"statuscode": 400,
			"message": ""
		},
		"data": []
	}
}

no logs

provokateurin

As far is I can see you are changing the behavior of isUserDefined which is a bad idea for backwards compatibility.

The OCS endpoint should not have the possibility to set it because a custom message is always set by the user.

CC @nickvergessen you know more of the internal logic here

miaulalala · 2023-11-16T17:14:30Z

As far is I can see you are changing the behavior of isUserDefined which is a bad idea for backwards compatibility.

The OCS endpoint should not have the possibility to set it because a custom message is always set by the user.

CC @nickvergessen you know more of the internal logic here

But that is not true. The "In a call" automation sets a status that pretends to be set by the user when it is in fact not for example.

provokateurin · 2023-11-16T17:20:21Z

But that is not true. The "In a call" automation sets a status that pretends to be set by the user when it is in fact not for example.

Ok that is good to hear. I wasn't really sure what is going on just looking at the code alone.

But I think the OCS endpoint still shouldn't have the option and always set it to true, do you agree?

ChristophWurst · 2023-11-16T17:25:55Z

But I think the OCS endpoint still shouldn't have the option

as far as I can tell our frontend doesn't make use of the option either. ref #41461 (comment). we should be able to revert this change.

miaulalala · 2023-11-16T18:42:33Z

But I think the OCS endpoint still shouldn't have the option

as far as I can tell our frontend doesn't make use of the option either. ref #41461 (comment). we should be able to revert this change.

the frontend now does, at least if you set the status as a user in the status modal.

miaulalala · 2023-11-16T18:43:49Z

@miaulalala could you please drop a few lines describing how status automation now works conceptually? I see the old availability automation is gone. OOO changes are also reflected instantly, so I assume there is no caching yet?

the status itself i snot cached but the OOO data is. The cache is reset when an absence is created, updated or deleted as otherwise the change would never propagate.

Will drop a few lines after I've made the logic changes.

ChristophWurst · 2023-11-16T19:10:10Z

Careful with using caches as consistent storage. Let's just let the cache expire a bit earlier because it is a local one. On a clustered setup you can only clear the cache of one instance. For the other instances it has to expire on its own. You could switch to a distributed cache but they are expensive and create a new bottleneck.

provokateurin · 2023-11-16T19:19:40Z

the frontend now does, at least if you set the status as a user in the status modal.

but it sets the isUserChanged to true, right? then you still don't need to expose this as setCustomMessage was already setting it to true.

nickvergessen · 2023-11-17T07:30:01Z

The "In a call" automation sets a status that pretends to be set by the user when it is in fact not for example.

Well just because there is no option to specify this via public OCP (not S) API. But the $createBackup set to true basically always means it's not set by the user?

nickvergessen · 2023-11-17T07:31:11Z

apps/user_status/lib/Controller/UserStatusController.php

@@ -192,7 +194,7 @@ public function clearMessage(): DataResponse {
 	 * 200: Status reverted
 	 */
 	public function revertStatus(string $messageId): DataResponse {
-		$backupStatus = $this->service->revertUserStatus($this->userId, $messageId, true);


Oo this means after ending a call your user status will always be killed?

nickvergessen · 2023-11-17T07:33:14Z

apps/user_status/lib/Service/StatusService.php

@@ -241,7 +258,7 @@ public function setPredefinedMessage(string $userId,
 			$userStatus->setUserId($userId);
 			$userStatus->setStatus(IUserStatus::OFFLINE);
 			$userStatus->setStatusTimestamp(0);
-			$userStatus->setIsUserDefined(false);
+			$userStatus->setIsUserDefined(true); // all predefined messages are by default set by the user


But offline is not settable by users, only invisible which is shown to others as "offline".
If you make Offline setIsUserDefined(true) coming online and interacting will no longer set any status?

nickvergessen · 2023-11-17T07:37:36Z

lib/public/UserStatus/IUserStatus.php

+	public const HOLIDAY_ICON = '🌴';
+	public const MEETING_ICON = '📅';


Add @since

nickvergessen · 2023-11-17T07:40:24Z

apps/user_status/lib/Service/StatusService.php

@@ -91,7 +94,8 @@ public function __construct(private UserStatusMapper $mapper,
 								private IEmojiHelper $emojiHelper,
 								private IConfig $config,
 								private IUserManager $userManager,
-								private CalendarStatusService $calendarStatusService) {
+								private CalendarStatusService $calendarStatusService,
+								public IAvailabilityCoordinator $coordinator) {


Suggested change

public IAvailabilityCoordinator $coordinator) {

private IAvailabilityCoordinator $coordinator) {

ChristophWurst · 2023-11-24T09:06:18Z

#41714

miaulalala added enhancement 3. to review Waiting for reviews feature: status labels Nov 14, 2023

miaulalala added this to the Nextcloud 28 milestone Nov 14, 2023

miaulalala requested review from ChristophWurst and st3iny November 14, 2023 14:19

miaulalala self-assigned this Nov 14, 2023

github-advanced-security bot found potential problems Nov 14, 2023

View reviewed changes

apps/user_status/lib/Service/StatusService.php Fixed Show resolved Hide resolved

ChristophWurst approved these changes Nov 14, 2023

View reviewed changes

ChristophWurst requested changes Nov 14, 2023

View reviewed changes

ChristophWurst mentioned this pull request Nov 14, 2023

28.0.0 beta 3 #41379

Merged

miaulalala force-pushed the enh/set-ooo-status branch from e294c05 to 4895e65 Compare November 14, 2023 19:13

github-advanced-security bot found potential problems Nov 14, 2023

View reviewed changes

apps/user_status/lib/Service/StatusService.php Fixed Show fixed Hide fixed

blizzz mentioned this pull request Nov 14, 2023

28.0.0 beta 4 #41477

Merged

tcitworld reviewed Nov 15, 2023

View reviewed changes

apps/dav/lib/Db/Absence.php Outdated Show resolved Hide resolved

ChristophWurst reviewed Nov 15, 2023

View reviewed changes

miaulalala force-pushed the enh/set-ooo-status branch from a7bed65 to 90cdd5c Compare November 15, 2023 20:08

miaulalala requested a review from nickvergessen as a code owner November 15, 2023 20:08

github-advanced-security bot found potential problems Nov 15, 2023

View reviewed changes

lib/private/User/AvailabilityCoordinator.php Fixed Show fixed Hide fixed

github-advanced-security bot found potential problems Nov 15, 2023

View reviewed changes

apps/user_status/lib/Service/StatusService.php Fixed Show fixed Hide fixed

miaulalala force-pushed the enh/set-ooo-status branch from 22c9e73 to 96e0218 Compare November 16, 2023 16:04

miaulalala requested a review from provokateurin as a code owner November 16, 2023 16:04

enh(user_status): set OoO status and message

14b18cc

Signed-off-by: Anna Larch <[email protected]>

miaulalala force-pushed the enh/set-ooo-status branch from 96e0218 to 14b18cc Compare November 16, 2023 16:19

ChristophWurst reviewed Nov 16, 2023

View reviewed changes

provokateurin requested changes Nov 16, 2023

View reviewed changes

fixup! enh(user_status): set OoO status and message

8a3b257

nickvergessen reviewed Nov 17, 2023

View reviewed changes

blizzz mentioned this pull request Nov 20, 2023

28.0.0 RC1 #41621

Merged

5 tasks

ChristophWurst mentioned this pull request Nov 23, 2023

feat(dav): dispatch out-of-office started and ended events #41440

Merged

6 tasks

blizzz modified the milestones: Nextcloud 28, Nextcloud 29 Nov 23, 2023

miaulalala closed this Nov 24, 2023

ChristophWurst deleted the enh/set-ooo-status branch November 24, 2023 11:18

skjnldsv removed this from the Nextcloud 29 milestone Feb 23, 2024

		public const HOLIDAY_ICON = '🌴';
		public const MEETING_ICON = '📅';

	public IAvailabilityCoordinator $coordinator) {
	private IAvailabilityCoordinator $coordinator) {

enh(user_status): set OoO status and message #41461

enh(user_status): set OoO status and message #41461

Conversation

miaulalala commented Nov 14, 2023 • edited by ChristophWurst Loading

Summary

TODO

Checklist

ChristophWurst left a comment

Choose a reason for hiding this comment

ChristophWurst commented Nov 14, 2023 • edited Loading

ChristophWurst left a comment

Choose a reason for hiding this comment

miaulalala commented Nov 14, 2023

ChristophWurst left a comment • edited Loading

Choose a reason for hiding this comment

ChristophWurst commented Nov 16, 2023

ChristophWurst commented Nov 16, 2023 • edited by miaulalala Loading

ChristophWurst commented Nov 16, 2023

ChristophWurst commented Nov 16, 2023 • edited Loading

ChristophWurst commented Nov 16, 2023

ChristophWurst commented Nov 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst commented Nov 16, 2023

provokateurin left a comment

Choose a reason for hiding this comment

miaulalala commented Nov 16, 2023

provokateurin commented Nov 16, 2023

ChristophWurst commented Nov 16, 2023

miaulalala commented Nov 16, 2023 • edited Loading

miaulalala commented Nov 16, 2023 • edited Loading

ChristophWurst commented Nov 16, 2023

provokateurin commented Nov 16, 2023

nickvergessen commented Nov 17, 2023

Choose a reason for hiding this comment

nickvergessen Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ChristophWurst commented Nov 24, 2023

miaulalala commented Nov 14, 2023 •

edited by ChristophWurst

Loading

ChristophWurst commented Nov 14, 2023 •

edited

Loading

ChristophWurst left a comment •

edited

Loading

ChristophWurst commented Nov 16, 2023 •

edited by miaulalala

Loading

ChristophWurst commented Nov 16, 2023 •

edited

Loading

miaulalala commented Nov 16, 2023 •

edited

Loading

miaulalala commented Nov 16, 2023 •

edited

Loading

nickvergessen Nov 17, 2023 •

edited

Loading