-
Notifications
You must be signed in to change notification settings - Fork 6
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: Notification observation improvement [WPB-8776] #2753
feat: Notification observation improvement [WPB-8776] #2753
Conversation
Test Results3 038 tests ±0 2 934 ✔️ ±0 3m 43s ⏱️ +7s Results for commit 0c6ef01. ± Comparison against base commit eb1f80e. This pull request removes 1 and adds 1 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
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.
A great step forward.
Regarding the architecture, I think some of our DAOs are a bit too clever.
Returning if the conversation was muted or not isn't really something that insertMessage should do.
Maybe it shouldn't even update the notificationDate
either, without being told by the parameters (e.g. insertMessage(message, updateNotifications = true)
).
Whatever is inserting the message, in this case, PersistMessageUseCase
(which could be renamed to ConversationMessageHandler
or something like that), should be a centralised place where we could put all of this logic, and pass down to the DAOs and in-memory caches, instead of distributing the logic across the layers.
This is all something that I think we didn't care much about in the past and I don't think it needs to be addressed in this PR.
Overall, nice to see this.
if (!isConversationMuted && !isSelfSender && message.content.shouldNotifyUser()) { | ||
notificationEventsManager.scheduleRegularNotificationChecking() | ||
} |
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.
🥇
Datadog ReportBranch report: ✅ 0 Failed, 2934 Passed, 104 Skipped, 15.07s Total Time |
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.
Amazing work!
Just have a question that shouldn't block this PR but something we can think for the future on what other events we can show the notification for
logic/src/commonMain/kotlin/com/wire/kalium/logic/data/message/PersistMessageUseCase.kt
Show resolved
Hide resolved
I totally agree that we should avoid logic in DAOs, but the problem here is that only in DAO layer we can open the DB-transaction and do few queries. If we move that logic out of DAO - worse performance. Not sure if there is a better solution here, but definitely something to think about :) |
Please do not merge it yet, i'd like @MohamadJaara to check it first, as he's inspirer of this changes :) |
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.
LGTM
What's new in this PR?
Issues
Current implementation of observing the notifications is not perfect. GetNotification DB-query observes changes in
Message
,User
,Conversation
,MessageAssetContent
andMessageTextContent
DB-tables. So every-time something is changed there -> Flow emits a new value.Solutions
Instead of observing the notifications (using Flow in DAO), get notification once only when there are some messages that user should be notified about. For that:
EphemeralEventsNotificationManager
(renamed intoNotificationEventsManager
) by adding funs:scheduleRegularNotificationChecking
for scheduling a re-checking of notifications (called when new message that user should be notified about was persisted)observeRegularNotificationsChecking
for observing the Flow that notifications were changed - need to re-checkinsertOrIgnoreMessage
inMessageDAO
to returnInsertMessageResult
which is enum of:INSERTED_NEED_TO_NOTIFY_USER
- message was inserted and the conversation is not muted so user could be notified about itINSERTED_INTO_MUTED_CONVERSATION
- message was inserted and the conversation is muted so user shouldn't be notified about itPersistMessageUseCase
so after each succeed message persisting it checks if user should be notified about that message (if conversation is not mute, that is not "my message" and the message is "notificationable") if so -> schedule the checking byNotificationEventsManager
GetNotificationsUseCase
instead of just observingmessageRepository.getNotificationMessage()
observeNotificationEventsManager.observeRegularNotificationsChecking()
and re-check notifications only when it emits.