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

Conversation

nickvergessen
Copy link
Member

πŸ› οΈ API Checklist

🏁 Checklist

  • ⛑️ Tests (unit and/or integration) are included or not possible
  • πŸ“˜ API documentation in docs/ has been updated or is not required
  • πŸ”– Capability is added or not needed

@nickvergessen
Copy link
Member Author

/backport to stable30


$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

Comment on lines +323 to +326
if ($attendee->getNotificationLevel() === Participant::NOTIFY_DEFAULT
&& $this->getDefaultGroupNotification() !== Participant::NOTIFY_NEVER) {
$attendee->setNotificationCalls(Participant::NOTIFY_MENTION);
}
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants