Notifications: simplify the flow by removing persistence #2924

jmartinesp · 2024-05-27T11:59:12Z

Type of change

Feature
Bugfix
Technical
Other :

Content

Remove persistence layer for notifications, which should make the flow a lot simpler.
Bump of minSdk to 24, since it's needed to be able to reuse MessagingStyle notifications and get rid of the persistence.

Motivation and context

Having this persistence layer was needed for APIs 21-23, but it means having to handle a lot more logic than needed and ended up causing issues as #609 and probably #790.

While we could spend time trying to fix those issues, it's probably better to just drop support for persistence since the number of users in those Android versions are quite low and once we've bumped the minSdk there's no need to keep supporting our custom persistence layer anymore.

Tests

Test opening and dismissing notifications.
Check notifications for room messages are removed when you visit the room.
Check summary one gets removed when you dismiss the last message/invite/whatever one.
Check the diagnostic notification can be displayed, dismissed, opened and is removed when you leave the troubleshooting notifications screen.

Any other use case that comes to mind.

Tested devices

Physical
Emulator
OS version(s): 14

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request includes a new file under ./changelog.d. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#changelog
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

…eans a bump of minSdk to `24`.

…working as expected

github-actions · 2024-05-27T13:59:40Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/tLt9yS

codecov · 2024-05-27T14:49:58Z

Codecov Report

Attention: Patch coverage is 68.45426% with 100 lines in your changes are missing coverage. Please review.

Project coverage is 75.35%. Comparing base (436a876) to head (d7e38cf).
Report is 40 commits behind head on develop.

Files	Patch %	Lines
...push/impl/notifications/NotificationDataFactory.kt	70.00%	17 Missing and 13 partials ⚠️
...mpl/notifications/NotificationBroadcastReceiver.kt	0.00%	24 Missing ⚠️
...ations/factories/action/QuickReplyActionFactory.kt	0.00%	14 Missing ⚠️
.../notifications/DefaultNotificationDrawerManager.kt	69.44%	5 Missing and 6 partials ⚠️
...mpl/notifications/factories/NotificationCreator.kt	87.93%	2 Missing and 5 partials ⚠️
...es/push/impl/notifications/NotificationRenderer.kt	86.04%	2 Missing and 4 partials ⚠️
...raries/push/impl/push/OnNotifiableEventReceived.kt	0.00%	3 Missing ⚠️
...otifications/SystemNotificationsEnabledProvider.kt	0.00%	2 Missing ⚠️
...ent/android/features/call/CallForegroundService.kt	0.00%	1 Missing ⚠️
...ement/android/libraries/push/impl/di/PushModule.kt	0.00%	1 Missing ⚠️
... and 1 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2924      +/-   ##
===========================================
+ Coverage    75.24%   75.35%   +0.10%     
===========================================
  Files         1550     1546       -4     
  Lines        36968    36790     -178     
  Branches      7156     7119      -37     
===========================================
- Hits         27817    27723      -94     
+ Misses        5414     5346      -68     
+ Partials      3737     3721      -16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

bmarty

Some first remarks. There is a regression regarding shouldIgnoreEventInRoom, please see my comment.
I keep testing the application.

bmarty · 2024-05-28T09:27:02Z

...impl/src/main/kotlin/io/element/android/features/migration/impl/migrations/AppMigration04.kt

+import javax.inject.Inject
+
+/**
+ * Remove notifications.bin file, used to store notification data locally.


Perfect, thanks!

bmarty · 2024-05-28T09:28:00Z

.../src/test/kotlin/io/element/android/features/migration/impl/migrations/AppMigration04Test.kt

+        migration.migrate()
+
+        // Check that the file has been deleted
+        assertThat(file.exists()).isFalse()


Just to ensure that the test behaves as expected, I would add

assertThat(file.exists()).isTrue()

before invoking migration.migrate()

bmarty · 2024-05-28T09:32:57Z

...kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt

+                if (NotificationConfig.SUPPORT_JOIN_DECLINE_INVITE) {
+                    addAction(rejectInvitationActionFactory.create(inviteNotifiableEvent))
+                    addAction(acceptInvitationActionFactory.create(inviteNotifiableEvent))
+                }


Thanks for un-commenting this and wrtting the code which does the actions!

bmarty · 2024-05-28T09:36:59Z

...rc/test/kotlin/io/element/android/libraries/mediaupload/impl/AndroidMediaPreProcessorTest.kt

@@ -59,16 +59,16 @@ class AndroidMediaPreProcessorTest {
        assertThat(data.file.path).endsWith("image.png")
        val info = data as MediaUploadInfo.Image
        // Computing thumbnailFile is failing with Robolectric
-        assertThat(info.thumbnailFile).isNull()
+        assertThat(info.thumbnailFile).isNotNull()


Maybe remove the comment above?

bmarty · 2024-05-28T09:40:31Z

libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/di/PushModule.kt

+    @Provides
+    fun provideNotificationCompatManager(@ApplicationContext context: Context): NotificationManagerCompat {
+        return NotificationManagerCompat.from(context)
+    }


Maybe Inject NotificationManagerCompat at some other places? You can look for NotificationManagerCompat.from to see where.

bmarty · 2024-05-28T09:50:25Z

...n/kotlin/io/element/android/libraries/push/impl/notifications/ActiveNotificationsProvider.kt

+    }
+
+    override fun count(): Int {
+        return getAllNotifications().size


Suggested change

return getAllNotifications().size

return getNotificationsForSession(sessionId).size

...kotlin/io/element/android/libraries/push/impl/notifications/factories/NotificationCreator.kt

bmarty · 2024-05-28T11:41:01Z

...main/kotlin/io/element/android/libraries/push/impl/notifications/NotifiableEventProcessor.kt

-            val type = when (it) {
-                is InviteNotifiableEvent -> ProcessedEvent.Type.KEEP
-                is NotifiableMessageEvent -> when {
-                    it.shouldIgnoreEventInRoom(appState) -> {


The call to shouldIgnoreEventInRoom was useful.
Now the notifications for new events are displayed even when the user is currently watching the room timeline.

Oh, true. I'll have to restore it, thanks!

sonarqubecloud · 2024-05-28T14:38:45Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

bmarty

Thanks for the update!

Fix small issue introduced in #2924.

jmartinesp added 6 commits May 27, 2024 13:57

Notifications: simplify the flow by removing persistence. This also m…

d9b4335

…eans a bump of minSdk to `24`.

Remove unnecessary classes

279ceb3

Fix tests and remove mockk

d6e0d1e

Fix more tests and lint issues

dd5dcbe

Fix AndroidMediaPreProcessorTest, which is now somehow seems to be …

b552e53

…working as expected

Only clear summary notification if it was the last visible notification

bd7a5c1

jmartinesp added 4 commits May 28, 2024 09:09

Add more tests, restore mockk to mock NotificationManagerCompat

b7e96d3

Remove invite- prefix in notification tag workaround

b539dd0

Add changelog

f5761c8

Add migration to remove notification.bin file

42a4297

jmartinesp marked this pull request as ready for review May 28, 2024 09:26

jmartinesp requested a review from a team as a code owner May 28, 2024 09:26

jmartinesp requested review from ganfra and removed request for a team May 28, 2024 09:26

bmarty requested review from bmarty and removed request for ganfra May 28, 2024 09:28

bmarty requested changes May 28, 2024

View reviewed changes

jmartinesp added 5 commits May 28, 2024 15:40

Restore NotifiableEvent.shouldIgnoreEventInRoom(appState)

d2d597e

Add sessionId to ActiveNotificationsProvider.count()

d098c50

Inject NotificationManagerCompat where possible

40e6bf5

Reuse getNotificationsForSession(sessionId) in other methods

f925b45

Small fixes in tests

ca7d98b

jmartinesp requested a review from bmarty May 28, 2024 13:55

Fix some docs and do some cleanup

d7e38cf

jmartinesp force-pushed the misc/jme/simplify-notification-rendering branch from e10832a to d7e38cf Compare May 28, 2024 14:28

bmarty approved these changes May 29, 2024

View reviewed changes

bmarty added the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 29, 2024

github-actions bot removed the Run-Maestro Starts a Maestro Cloud session to run integration tests label May 29, 2024

jmartinesp enabled auto-merge (squash) May 29, 2024 07:44

jmartinesp merged commit 04e5031 into develop May 29, 2024
24 of 25 checks passed

jmartinesp deleted the misc/jme/simplify-notification-rendering branch May 29, 2024 08:03

bmarty added a commit that referenced this pull request May 29, 2024

Fix small issue introduced in #2924.

8c3bb04

bmarty added a commit that referenced this pull request May 29, 2024

Merge pull request #2939 from element-hq/feature/bma/fixWrongApiUsage

994b701

Fix small issue introduced in #2924.

This was referenced Jun 6, 2024

Notifications keep persistently reappearing after being dismissed #609

Closed

Continued receiving notifications just after signing out #790

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Notifications: simplify the flow by removing persistence #2924

Notifications: simplify the flow by removing persistence #2924

jmartinesp commented May 27, 2024 •

edited

Loading

github-actions bot commented May 27, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading

bmarty left a comment

bmarty May 28, 2024

bmarty May 28, 2024

jmartinesp May 28, 2024

bmarty May 28, 2024

bmarty May 28, 2024

jmartinesp May 28, 2024

bmarty May 28, 2024

jmartinesp May 28, 2024

bmarty May 28, 2024

jmartinesp May 28, 2024

bmarty May 28, 2024

jmartinesp May 28, 2024

jmartinesp May 28, 2024

sonarqubecloud bot commented May 28, 2024

bmarty left a comment

	return getAllNotifications().size
	return getNotificationsForSession(sessionId).size

Notifications: simplify the flow by removing persistence #2924

Notifications: simplify the flow by removing persistence #2924

Conversation

jmartinesp commented May 27, 2024 • edited Loading

Type of change

Content

Motivation and context

Tests

Tested devices

Checklist

github-actions bot commented May 27, 2024 • edited Loading

codecov bot commented May 27, 2024 • edited Loading

Codecov Report

bmarty left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented May 28, 2024

Quality Gate passed

bmarty left a comment

Choose a reason for hiding this comment

jmartinesp commented May 27, 2024 •

edited

Loading

github-actions bot commented May 27, 2024 •

edited

Loading

codecov bot commented May 27, 2024 •

edited

Loading