Improved /settings/notifications push toggle error handling #4062

ouchadam · 2021-09-22T16:25:59Z

Carrying on from #4052 Email notifications

Adds a separate synchronous (via suspend) api for registering pushers in order to provide instant feedback within the settings page when an error occurs. The background enqueuing version is still used for the FCM token regeneration.

Splits the logic from the WorkManager based implementation of the AddPusherWorker into a AddPusherTask which the worker now delegates to.
Adds unit tests around the extracted AddPusherTask which in turn meant extracting our a RequestExecutor along with other test fakes
Updates the settings page to use the suspending versions of the pusher registration calls

Email notification toggles whilst offline

BEFORE	AFTER

Session notification toggles whilst offline

BEFORE	AFTER ENABLE	AFTER DISABLE

github-actions · 2021-09-22T16:48:48Z

Unit Test Results

  44 files +2   44 suites +2 46s ⏱️ -6s
  87 tests +4   87 ✔️ +4 0 💤 ±0 0 ❌ ±0
228 runs +8 228 ✔️ +8 0 💤 ±0 0 ❌ ±0

Results for commit e82de2b. ± Comparison against base commit 1dd2d41.

♻️ This comment has been updated with latest results.

ouchadam · 2021-09-23T07:41:22Z

matrix-sdk-android/build.gradle

@@ -130,6 +130,8 @@ dependencies {

    // Database
    implementation 'com.github.Zhuinden:realm-monarchy:0.7.1'
+    testImplementation libs.rx.rxKotlin


in order to mock the realm instance we need to include a test dependency on rx as realm internally uses it and monarchy doesn't include rx in its transitive dependencies

Can you add the comment in the code? Also maybe move this line next to the other testImplementation items (I have no strong opinion on this last remark)?

ouchadam · 2021-09-23T07:42:50Z

...sdk-android/src/main/java/org/matrix/android/sdk/internal/session/pushers/AddPusherWorker.kt

-    @Inject lateinit var pushersAPI: PushersAPI
-    @Inject @SessionDatabase lateinit var monarchy: Monarchy
-    @Inject lateinit var globalErrorReceiver: GlobalErrorReceiver
+    @Inject lateinit var addPusherTask: AddPusherTask


The worker now delegates to the extracted AddPusherTask

ouchadam · 2021-09-23T07:45:21Z

...sdk-android/src/main/java/org/matrix/android/sdk/internal/session/pushers/AddPusherWorker.kt

            Result.success()
        } catch (exception: Throwable) {
            when (exception) {
                is Failure.NetworkConnection -> Result.retry()
-                else                         -> {


due to extracting the logic to a task there's now a slight change in behaviour, the AddPusherTask will always update any existing pusher state on failure instead of exception != NetworkConnection as the task can be used in non worker contexts (such as the settings page)

ouchadam · 2021-09-23T07:47:00Z

...droid/src/main/java/org/matrix/android/sdk/internal/session/pushers/DefaultPushersService.kt

@@ -107,13 +103,6 @@ internal class DefaultPushersService @Inject constructor(
        return request.id
    }

-    private fun JsonPusher.validateParameters() {


as we're always validating the payload I've lifted the logic as close of possible to the fields by doing it in the given class's init

ouchadam · 2021-09-23T07:59:30Z

...id/src/test/java/org/matrix/android/sdk/internal/session/pushers/DefaultAddPusherTaskTest.kt

+    private val realm = mockk<Realm>(relaxed = true)
+
+    init {
+        mockkStatic("org.matrix.android.sdk.internal.util.MonarchyKt")


in its current form the transactions call chain isn't very test friendly so I've opted to mock the top level awaitTransaction extension

ouchadam · 2021-09-23T08:10:06Z

...id/src/test/java/org/matrix/android/sdk/internal/session/pushers/DefaultAddPusherTaskTest.kt

+    private var error: Throwable? = null
+
+    override suspend fun getPushers(): GetPushersResponse {
+        TODO("Not yet implemented")


the current tests don't need this yet, hence the throwing TODO, ideally this would be implemented when a test around fetching pushers is created

ouchadam · 2021-09-23T08:25:43Z

...id/src/test/java/org/matrix/android/sdk/internal/session/pushers/DefaultAddPusherTaskTest.kt

+    }
+}
+
+internal class FakePushersAPI : PushersAPI {


this PR introduces the concept of fakes (here's a nice refresher!), the main purpose in this context is to avoid hitting the real api and database and to provide some reusable assertion/verification helpers

I've left the implementations within the test file for easier discussion/reviewing, will extract out to their own package if the team is happy with them 🤞

ouchadam · 2021-09-23T08:27:12Z

...id/src/test/java/org/matrix/android/sdk/internal/session/pushers/DefaultAddPusherTaskTest.kt

+
+        pushersAPI.verifySetPusher(A_JSON_PUSHER)
+        monarchy.verifyInsertOrUpdate<PusherEntity> {
+            withArg { actual ->


unfortunately the base entity models don't include any equality implementations so we have to manually check the fields

ouchadam · 2021-09-24T10:41:25Z

...x-sdk-android/src/main/java/org/matrix/android/sdk/internal/session/pushers/AddPusherTask.kt

+    }
+}
+
+internal interface RequestExecutor {


this should be extracted to its own file, which package would be best?

…task from the worker

… toggle - uses the synchronous token registering which also means we get error handling

…request (without hitting the network)

…) tranisitive depends on rx but doesn't propagate it as an API dependency - without an explicit declaration we can't mock the realm instance

- introduces the concepts of Fakes for handling the dependencies, unforuntately realm/monarchy aren't very testable in their current state so we'll need to use mocks

…exists

- also creates a dedicated RequestModule instead of providing the executor via the pushers module

bmarty

Thanks a lot for improving the test framework.
Some minor remarks

bmarty · 2021-10-05T11:44:00Z

matrix-sdk-android/build.gradle

@@ -130,6 +130,8 @@ dependencies {

    // Database
    implementation 'com.github.Zhuinden:realm-monarchy:0.7.1'
+    testImplementation libs.rx.rxKotlin


Can you add the comment in the code? Also maybe move this line next to the other testImplementation items (I have no strong opinion on this last remark)?

bmarty · 2021-10-05T11:48:10Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/api/session/pushers/PushersService.kt

+             * this is the routing or destination address information for the notification,
+             * for example, the APNS token for APNS or the Registration ID for GCM. If your
+             * notification client has no such concept, use any unique identifier. Max length, 512 chars.
+             * If the kind is "email", this is the email address to send notifications to.


I think with this Service API change, the kind should not be "email", so maybe remove this line?

bmarty · 2021-10-05T11:49:06Z

matrix-sdk-android/src/main/java/org/matrix/android/sdk/internal/network/RequestExecutor.kt

+internal interface RequestExecutor {
+    suspend fun <DATA> executeRequest(globalErrorReceiver: GlobalErrorReceiver?,
+                                      canRetry: Boolean = false,
+                                      maxDelayBeforeRetry: Long = 32_000L,


Why this default value 32s ?

it comes from here

The request executor is an injectable wrapper of the request extension, open to other ideas for making this testable!

bmarty · 2021-10-05T11:56:10Z

vector/src/main/java/im/vector/app/core/pushers/PushersManager.kt

    }

-    fun registerEmailForPush(email: String) {
+    private fun httpPusher(pushKey: String) = PushersService.HttpPusher(


rename to createHttpPusher? functions should start with a verb

bmarty · 2021-10-05T11:58:34Z

...m/vector/app/features/settings/notifications/VectorSettingsNotificationPreferenceFragment.kt

+            it.setTransactionalSwitchChangeListener(lifecycleScope) { isChecked ->
+                if (isChecked) {
+                    FcmHelper.getFcmToken(requireContext())?.let {
+                        pushManager.registerPusherWithFcmKey(it)


it's weird that in this case, we do not call session.refreshPushers() (I see this is not a change from this PR)

should we move the refresh after change to the PusherManager? 🤔 (and add to the registerPusherWithFcmKey)

ouchadam · 2021-10-08T08:14:43Z

@bmarty whilst I'm in the area I'll also address #4106

ouchadam commented Sep 23, 2021

View reviewed changes

ouchadam force-pushed the feature/adm/suspending_add_pusher branch from 36c42f3 to 12df746 Compare September 23, 2021 09:37

ouchadam force-pushed the feature/adm/email_notification_toggle branch from 5033ea5 to 8316728 Compare September 23, 2021 10:51

ouchadam force-pushed the feature/adm/suspending_add_pusher branch 2 times, most recently from 8627a08 to a0d322f Compare September 24, 2021 10:32

ouchadam commented Sep 24, 2021

View reviewed changes

Base automatically changed from feature/adm/email_notification_toggle to develop September 24, 2021 18:42

ouchadam force-pushed the feature/adm/suspending_add_pusher branch from a0d322f to 56bbeae Compare September 28, 2021 07:53

ouchadam marked this pull request as ready for review September 28, 2021 07:54

ouchadam force-pushed the feature/adm/suspending_add_pusher branch 3 times, most recently from d701974 to 71fea25 Compare October 1, 2021 18:04

ouchadam added 10 commits October 1, 2021 19:05

extracting the add pusher logic for the worker and delegating to the …

d74dedb

…task from the worker

reusing the transactional logic for the current session notifications…

2d572cb

… toggle - uses the synchronous token registering which also means we get error handling

running lint

82c0c19

removing comment which doesn't add additional context/information

eb8d6a5

creating an injectable request executor to enable unit tests network …

b7262c9

…request (without hitting the network)

including rx java dependency for the sdk tests because real (monarchy…

d3bce76

…) tranisitive depends on rx but doesn't propagate it as an API dependency - without an explicit declaration we can't mock the realm instance

adding test for the add pusher task happy flow

0bb81ed

- introduces the concepts of Fakes for handling the dependencies, unforuntately realm/monarchy aren't very testable in their current state so we'll need to use mocks

inverting if to favour positive ordering

01a1653

adding test cases for when adding a pusher fails and when it already …

317a817

…exists

removing unused import

87f0f3d

ouchadam added 2 commits October 1, 2021 19:05

extracting the test fakes to their own package

2693a34

lifting the request executor to its own file in the network package

e82de2b

- also creates a dedicated RequestModule instead of providing the executor via the pushers module

ouchadam force-pushed the feature/adm/suspending_add_pusher branch from 71fea25 to e82de2b Compare October 1, 2021 18:05

bmarty self-requested a review October 5, 2021 11:42

bmarty reviewed Oct 5, 2021

View reviewed changes

ouchadam closed this Oct 8, 2021

ouchadam mentioned this pull request Oct 12, 2021

Improved /settings/notifications push toggle error handling #4228

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improved /settings/notifications push toggle error handling #4062

Improved /settings/notifications push toggle error handling #4062

ouchadam commented Sep 22, 2021

github-actions bot commented Sep 22, 2021 •

edited

ouchadam Sep 23, 2021

bmarty Oct 5, 2021

ouchadam Sep 23, 2021

ouchadam Sep 23, 2021

ouchadam Sep 23, 2021

ouchadam Sep 23, 2021

ouchadam Sep 23, 2021 •

edited

ouchadam Sep 23, 2021

ouchadam Sep 23, 2021

ouchadam Sep 24, 2021

bmarty left a comment

bmarty Oct 5, 2021

bmarty Oct 5, 2021

bmarty Oct 5, 2021

ouchadam Oct 8, 2021

bmarty Oct 12, 2021

bmarty Oct 5, 2021

bmarty Oct 5, 2021

ouchadam Oct 8, 2021

ouchadam commented Oct 8, 2021

Improved /settings/notifications push toggle error handling #4062

Improved /settings/notifications push toggle error handling #4062

Conversation

ouchadam commented Sep 22, 2021

github-actions bot commented Sep 22, 2021 • edited

Unit Test Results

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

ouchadam Sep 23, 2021 • edited

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

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

ouchadam commented Oct 8, 2021

github-actions bot commented Sep 22, 2021 •

edited

ouchadam Sep 23, 2021 •

edited