feat: Notification observation improvement [WPB-8776]

[borichellow]

WPB-8776 [Android] Enhance Notification Handling and Resource Efficiency

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 and MessageTextContent 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:

1. Update EphemeralEventsNotificationManager (renamed into NotificationEventsManager) 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-check
2. Update insertOrIgnoreMessage in MessageDAO to return InsertMessageResult which is enum of:
   • INSERTED_NEED_TO_NOTIFY_USER - message was inserted and the conversation is not muted so user could be notified about it
   • INSERTED_INTO_MUTED_CONVERSATION - message was inserted and the conversation is muted so user shouldn't be notified about it
3. Update PersistMessageUseCase 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 by NotificationEventsManager
4. In GetNotificationsUseCase instead of just observing messageRepository.getNotificationMessage() observe NotificationEventsManager.observeRegularNotificationsChecking() and re-check notifications only when it emits.

[github-actions]

Test Results

3 038 tests  ±0   2 934 ✔️ ±0   3m 43s ⏱️ +7s
   528 suites ±0      104 💤 ±0 
   528 files   ±0          0 ❌ ±0 

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.

com.wire.kalium.logic.feature.message.GetNotificationsUseCaseTest ‑ givenNoNewNotifications_thenShouldNotEmitany[jvm]

com.wire.kalium.logic.feature.message.GetNotificationsUseCaseTest ‑ givenNoNewNotifications_thenShouldNotEmitAny[jvm]

♻️ This comment has been updated with latest results.

[vitorhugods]

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.

[vitorhugods]

🥇

[datadog-wireapp]

Datadog Report

Branch report: feat/notification_observation_improvement
Commit report: ac1c2c0
Test service: kalium-jvm

✅ 0 Failed, 2934 Passed, 104 Skipped, 15.07s Total Time

[alexandreferris]

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

[borichellow]

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.

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 :)

[borichellow]

Please do not merge it yet, i'd like @MohamadJaara to check it first, as he's inspirer of this changes :)

[MohamadJaara]

LGTM