Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(archive): Improve archive preference handling #13404

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/Controller/RoomController.php
Original file line number Diff line number Diff line change
Expand Up @@ -1546,6 +1546,9 @@ public function setPassword(string $password): DataResponse {
/**
* Archive a conversation
*
* - Turns off call notifications
* - Reduces chat notifications in group and public rooms when it was "default" resulting in "always"
*
* @return DataResponse<Http::STATUS_OK, TalkRoom, array{}>
*
* 200: Conversation was archived
Expand Down
26 changes: 22 additions & 4 deletions lib/Service/ParticipantService.php
Original file line number Diff line number Diff line change
Expand Up @@ -299,18 +299,36 @@ public function updateNotificationCalls(Participant $participant, int $level): v
}

/**
* @param Participant $participant
* @return Participant::NOTIFY_*
*/
protected function getDefaultGroupNotification(): int {
$config = (int)$this->serverConfig->getAppValue('spreed', 'default_group_notification', (string)Participant::NOTIFY_MENTION);
return max(Participant::NOTIFY_DEFAULT, min($config, Participant::NOTIFY_NEVER));
}

public function archiveConversation(Participant $participant): void {
$attendee = $participant->getAttendee();
$attendee->setArchived(true);
$attendee->setLastAttendeeActivity($this->timeFactory->getTime());

$room = $participant->getRoom();
if ($room->getType() === Room::TYPE_PUBLIC || $room->getType() === Room::TYPE_GROUP) {
// Turn of call notifications by default
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Turn of call notifications by default
// Turn off call notifications by default

if ($attendee->getNotificationCalls() === Participant::NOTIFY_CALLS_ON) {
$attendee->setNotificationCalls(Participant::NOTIFY_CALLS_OFF);
}

// Reduce notifications when it was "Always" via the default
// Otherwise we respect the attendees selection
if ($attendee->getNotificationLevel() === Participant::NOTIFY_DEFAULT
&& $this->getDefaultGroupNotification() !== Participant::NOTIFY_NEVER) {
$attendee->setNotificationCalls(Participant::NOTIFY_MENTION);
}
Comment on lines +323 to +326
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to consider the default group notification level? I thought it's relevant only when creating a new conversation.

Also, I think user expects to turn off all notifications automatically (maybe except mention) when archiving a conversation. They could have set "always" for group conversation previously and when archiving, they expect it to be changed to "only mention" and not stay "always" (why would they archive it then).

Copy link
Contributor

@Antreesy Antreesy Sep 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume DEFAULT is stored in DB per each attendee, unless was redefined by that attendee. Then it makes sense to redefine default behaviour only

If they are annoyed by notifications, maybe they shouldn't have set it to ALWAYS?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume DEFAULT is stored in DB per each attendee, unless was redefined by that attendee. Then it makes sense to redefine default behaviour only

Even if it was redefined by attendee, it doesn't mean it should be applied in archive (the point of archive it to put the notifications frequency to the lowest (not in sidebar list, no call, no message notif).

If they are annoyed by notifications, maybe they shouldn't have set it to ALWAYS?)

Conversation was previously important (a deal) and now it's not (deal closed for a while now). A user can see archiving as a good shortcut to reduce the notifications at once.

}

$this->attendeeMapper->update($attendee);
}

/**
* @param Participant $participant
*/
public function unarchiveConversation(Participant $participant): void {
$attendee = $participant->getAttendee();
$attendee->setArchived(false);
Expand Down
1 change: 1 addition & 0 deletions openapi-full.json
Original file line number Diff line number Diff line change
Expand Up @@ -15814,6 +15814,7 @@
"post": {
"operationId": "room-archive-conversation",
"summary": "Archive a conversation",
"description": "- Turns off call notifications - Reduces chat notifications in group and public rooms when it was \"default\" resulting in \"always\"",
"tags": [
"room"
],
Expand Down
1 change: 1 addition & 0 deletions openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -15948,6 +15948,7 @@
"post": {
"operationId": "room-archive-conversation",
"summary": "Archive a conversation",
"description": "- Turns off call notifications - Reduces chat notifications in group and public rooms when it was \"default\" resulting in \"always\"",
"tags": [
"room"
],
Expand Down
5 changes: 4 additions & 1 deletion src/types/openapi/openapi-full.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1231,7 +1231,10 @@ export type paths = {
};
get?: never;
put?: never;
/** Archive a conversation */
/**
* Archive a conversation
* @description - Turns off call notifications - Reduces chat notifications in group and public rooms when it was "default" resulting in "always"
*/
post: operations["room-archive-conversation"];
/** Unarchive a conversation */
delete: operations["room-unarchive-conversation"];
Expand Down
5 changes: 4 additions & 1 deletion src/types/openapi/openapi.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1233,7 +1233,10 @@ export type paths = {
};
get?: never;
put?: never;
/** Archive a conversation */
/**
* Archive a conversation
* @description - Turns off call notifications - Reduces chat notifications in group and public rooms when it was "default" resulting in "always"
*/
post: operations["room-archive-conversation"];
/** Unarchive a conversation */
delete: operations["room-unarchive-conversation"];
Expand Down
Loading