-
Notifications
You must be signed in to change notification settings - Fork 290
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
feat: Adjust browser notifications to new states (WEBAPP-5562) #4803
Conversation
const shouldNotify = isEventToNotify && !messageEntity.isEdited() && !messageEntity.isLinkPreview() && !isMuted; | ||
const ignoreAllNotifications = conversationEntity && conversationEntity.showNotificationsNothing(); | ||
|
||
const showNotificationsOnlyMentions = conversationEntity && conversationEntity.showNotificationsOnlyMentions(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For that kind of heavy logic code, I would probably create a new class that handles filtering the incoming notifications.
The API could be very simple (for the moment at least) with just a pure function that given a message, a connection, a conversation and a user and a notification level returns true if the notification should fire.
It will be easier in the future to bootstrap tests for that class without having to initiate all the dependencies of the NotificationRepository.
Could be done in a follow up PR
d4a5584
to
5598f44
Compare
5598f44
to
50fc591
Compare
b5e9e0a
to
820239f
Compare
* @param {string} userId - The user id to check mentions for. | ||
* @returns {boolean} If the conversation should send a notification. | ||
*/ | ||
static _shouldNotify(conversationEntity, messageEntity, userId) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need for an underscore in the name. This is staic and available for everyone.
const isEventToNotify = | ||
NotificationRepository.EVENTS_TO_NOTIFY.includes(messageEntity.super_type) && | ||
!messageEntity.isEdited() && | ||
!messageEntity.isLinkPreview(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we pull this apart for a little improved readability?
const isEventTypeToNotify = NotificationRepository.EVENTS_TO_NOTIFY.includes(messageEntity.super_type);
const isEventToNotify = isEventTypeToNotify && !messageEntity.isEdited() && !messageEntity.isLinkPreview();
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
No description provided.