Improve API, to avoid ignoring errors

appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt

@@ -36,6 +36,7 @@ import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatu
 import io.element.android.libraries.push.api.PushService
 import io.element.android.services.analytics.api.AnalyticsService
 import kotlinx.coroutines.flow.map
+import timber.log.Timber
 import javax.inject.Inject
 
 class LoggedInPresenter @Inject constructor(
@@ -56,7 +57,7 @@ class LoggedInPresenter @Inject constructor(
             if (isVerified) {
                 // Ensure pusher is registered
                 val currentPushProvider = pushService.getCurrentPushProvider()
-                if (currentPushProvider == null) {
+                val result = if (currentPushProvider == null) {
                     // Register with the first available push provider
                     val pushProvider = pushService.getAvailablePushProviders().firstOrNull() ?: return@LaunchedEffect
                     val distributor = pushProvider.getDistributors().firstOrNull() ?: return@LaunchedEffect
@@ -72,6 +73,9 @@ class LoggedInPresenter @Inject constructor(
                         pushService.registerWith(matrixClient, currentPushProvider, currentPushDistributor)
                     }
                 }
+                result.onFailure {
+                    Timber.e(it, "Failed to register pusher")
+                }
             }
         }
 

libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt

@@ -40,7 +40,11 @@ interface PushService {
      *
      * The method has effect only if the [PushProvider] is different than the current one.
      */
-    suspend fun registerWith(matrixClient: MatrixClient, pushProvider: PushProvider, distributor: Distributor)
+    suspend fun registerWith(
+        matrixClient: MatrixClient,
+        pushProvider: PushProvider,
+        distributor: Distributor,
+    ): Result<Unit>
 
     /**
      * Return false in case of early error.

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

@@ -25,6 +25,7 @@ import io.element.android.libraries.push.impl.notifications.DefaultNotificationD
 import io.element.android.libraries.pushproviders.api.Distributor
 import io.element.android.libraries.pushproviders.api.PushProvider
 import io.element.android.libraries.pushstore.api.UserPushStoreFactory
+import timber.log.Timber
 import javax.inject.Inject
 
 @ContributesBinding(AppScope::class)
@@ -53,17 +54,26 @@ class DefaultPushService @Inject constructor(
     /**
      * Get current push provider, compare with provided one, then unregister and register if different, and store change.
      */
-    override suspend fun registerWith(matrixClient: MatrixClient, pushProvider: PushProvider, distributor: Distributor) {
+    override suspend fun registerWith(
+        matrixClient: MatrixClient,
+        pushProvider: PushProvider,
+        distributor: Distributor,
+    ): Result<Unit> {
         val userPushStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId)
         val currentPushProviderName = userPushStore.getPushProviderName()
         val currentDistributorValue = pushProvider.getCurrentDistributor(matrixClient)?.value
         if (currentPushProviderName != pushProvider.name || currentDistributorValue != distributor.value) {
             // Unregister previous one if any
             pushProviders.find { it.name == currentPushProviderName }?.unregister(matrixClient)
+                ?.onFailure {
+                    Timber.w(it, "Failed to unregister previous push provider")
+                }
         }
-        pushProvider.registerWith(matrixClient, distributor)
-        // Store new value
-        userPushStore.setPushProviderName(pushProvider.name)
+        return pushProvider.registerWith(matrixClient, distributor)
+            .onSuccess {
+                // Store new value
+                userPushStore.setPushProviderName(pushProvider.name)
+            }
     }
 
     override suspend fun testPush(): Boolean {

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

@@ -63,22 +63,26 @@ class PushersManager @Inject constructor(
     /**
      * Register a pusher to the server if not done yet.
      */
-    override suspend fun registerPusher(matrixClient: MatrixClient, pushKey: String, gateway: String) {
+    override suspend fun registerPusher(
+        matrixClient: MatrixClient,
+        pushKey: String,
+        gateway: String,
+    ): Result<Unit> {
         val userDataStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId)
         if (userDataStore.getCurrentRegisteredPushKey() == pushKey) {
             Timber.tag(loggerTag.value)
                 .d("Unnecessary to register again the same pusher, but do it in case the pusher has been removed from the server")
         }
-        matrixClient.pushersService().setHttpPusher(
-            createHttpPusher(pushKey, gateway, matrixClient.sessionId)
-        ).fold(
-            {
+        return matrixClient.pushersService()
+            .setHttpPusher(
+                createHttpPusher(pushKey, gateway, matrixClient.sessionId)
+            )
+            .onSuccess {
                 userDataStore.setCurrentRegisteredPushKey(pushKey)
-            },
-            { throwable ->
+            }
+            .onFailure { throwable ->
                 Timber.tag(loggerTag.value).e(throwable, "Unable to register the pusher")
             }
-        )
     }
 
     private suspend fun createHttpPusher(
@@ -107,13 +111,25 @@ class PushersManager @Inject constructor(
         return "{\"cs\":\"$secretForUser\"}"
     }
 
-    override suspend fun unregisterPusher(matrixClient: MatrixClient, pushKey: String, gateway: String) {
-        matrixClient.pushersService().unsetHttpPusher(
-            unsetHttpPusherData = UnsetHttpPusherData(
-                pushKey = pushKey,
-                appId = PushConfig.PUSHER_APP_ID
+    override suspend fun unregisterPusher(
+        matrixClient: MatrixClient,
+        pushKey: String,
+        gateway: String,
+    ): Result<Unit> {
+        val userDataStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId)
+        return matrixClient.pushersService()
+            .unsetHttpPusher(
+                unsetHttpPusherData = UnsetHttpPusherData(
+                    pushKey = pushKey,
+                    appId = PushConfig.PUSHER_APP_ID
+                )
             )
-        )
+            .onSuccess {
+                userDataStore.setCurrentRegisteredPushKey(null)
+            }
+            .onFailure { throwable ->
+                Timber.tag(loggerTag.value).e(throwable, "Unable to unregister the pusher")
+            }
     }
 
     companion object {

libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt

@@ -32,7 +32,12 @@ class FakePushService(
         return emptyList()
     }
 
-    override suspend fun registerWith(matrixClient: MatrixClient, pushProvider: PushProvider, distributor: Distributor) {
+    override suspend fun registerWith(
+        matrixClient: MatrixClient,
+        pushProvider: PushProvider,
+        distributor: Distributor,
+    ): Result<Unit> {
+        return Result.success(Unit)
     }
 
     override suspend fun testPush(): Boolean = simulateLongTask {

libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushProvider.kt

@@ -42,7 +42,7 @@ interface PushProvider {
     /**
      * Register the pusher to the homeserver.
      */
-    suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor)
+    suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor): Result<Unit>
 
     /**
      * Return the current distributor, or null if none.
@@ -52,7 +52,7 @@ interface PushProvider {
     /**
      * Unregister the pusher.
      */
-    suspend fun unregister(matrixClient: MatrixClient)
+    suspend fun unregister(matrixClient: MatrixClient): Result<Unit>
 
     suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig?
 }

libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PusherSubscriber.kt

@@ -19,6 +19,6 @@ package io.element.android.libraries.pushproviders.api
 import io.element.android.libraries.matrix.api.MatrixClient
 
 interface PusherSubscriber {
-    suspend fun registerPusher(matrixClient: MatrixClient, pushKey: String, gateway: String)
-    suspend fun unregisterPusher(matrixClient: MatrixClient, pushKey: String, gateway: String)
+    suspend fun registerPusher(matrixClient: MatrixClient, pushKey: String, gateway: String): Result<Unit>
+    suspend fun unregisterPusher(matrixClient: MatrixClient, pushKey: String, gateway: String): Result<Unit>
 }

libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebaseNewTokenHandler.kt

@@ -16,6 +16,7 @@
 
 package io.element.android.libraries.pushproviders.firebase
 
+import io.element.android.libraries.core.extensions.flatMap
 import io.element.android.libraries.core.log.logger.LoggerTag
 import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService
 import io.element.android.libraries.matrix.api.core.SessionId
@@ -46,9 +47,17 @@ class FirebaseNewTokenHandler @Inject constructor(
             .forEach { userId ->
                 val userDataStore = userPushStoreFactory.getOrCreate(userId)
                 if (userDataStore.getPushProviderName() == FirebaseConfig.NAME) {
-                    matrixAuthenticationService.restoreSession(userId).getOrNull()?.use { client ->
-                        pusherSubscriber.registerPusher(client, firebaseToken, FirebaseConfig.PUSHER_HTTP_URL)
-                    }
+                    matrixAuthenticationService
+                        .restoreSession(userId)
+                        .onFailure {
+                            Timber.tag(loggerTag.value).e(it, "Failed to restore session $userId")
+                        }
+                        .flatMap { client ->
+                            pusherSubscriber.registerPusher(client, firebaseToken, FirebaseConfig.PUSHER_HTTP_URL)
+                        }
+                        .onFailure {
+                            Timber.tag(loggerTag.value).e(it, "Failed to register pusher for session $userId")
+                        }
                 } else {
                     Timber.tag(loggerTag.value).d("This session is not using Firebase pusher")
                 }

libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt

@@ -46,20 +46,28 @@ class FirebasePushProvider @Inject constructor(
         return listOf(firebaseDistributor)
     }
 
-    override suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor) {
-        val pushKey = firebaseStore.getFcmToken() ?: return Unit.also {
+    override suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor): Result<Unit> {
+        val pushKey = firebaseStore.getFcmToken() ?: return Result.failure<Unit>(
+            IllegalStateException(
+                "Unable to register pusher, Firebase token is not known."
+            )
+        ).also {
             Timber.tag(loggerTag.value).w("Unable to register pusher, Firebase token is not known.")
         }
-        pusherSubscriber.registerPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL)
+        return pusherSubscriber.registerPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL)
     }
 
     override suspend fun getCurrentDistributor(matrixClient: MatrixClient) = firebaseDistributor
 
-    override suspend fun unregister(matrixClient: MatrixClient) {
-        val pushKey = firebaseStore.getFcmToken() ?: return Unit.also {
+    override suspend fun unregister(matrixClient: MatrixClient): Result<Unit> {
+        val pushKey = firebaseStore.getFcmToken() ?: return Result.failure<Unit>(
+            IllegalStateException(
+                "Unable to unregister pusher, Firebase token is not known."
+            )
+        ).also {
             Timber.tag(loggerTag.value).w("Unable to unregister pusher, Firebase token is not known.")
         }
-        pusherSubscriber.unregisterPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL)
+        return pusherSubscriber.unregisterPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL)
     }
 
     override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {

libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt

@@ -31,12 +31,12 @@ class FakePushProvider(
 
     override fun getDistributors(): List<Distributor> = distributors
 
-    override suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor) {
-        // No-op
+    override suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor): Result<Unit> {
+        return Result.success(Unit)
     }
 
-    override suspend fun unregister(matrixClient: MatrixClient) {
-        // No-op
+    override suspend fun unregister(matrixClient: MatrixClient): Result<Unit> {
+        return Result.success(Unit)
     }
 
     override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {

libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/RegisterUnifiedPushUseCase.kt

@@ -18,55 +18,18 @@ package io.element.android.libraries.pushproviders.unifiedpush
 
 import android.content.Context
 import io.element.android.libraries.di.ApplicationContext
-import io.element.android.libraries.matrix.api.MatrixClient
 import io.element.android.libraries.pushproviders.api.Distributor
-import io.element.android.libraries.pushproviders.api.PusherSubscriber
 import org.unifiedpush.android.connector.UnifiedPush
 import javax.inject.Inject
 
 class RegisterUnifiedPushUseCase @Inject constructor(
     @ApplicationContext private val context: Context,
-    private val pusherSubscriber: PusherSubscriber,
-    private val unifiedPushStore: UnifiedPushStore,
 ) {
-    sealed interface RegisterUnifiedPushResult {
-        data object Success : RegisterUnifiedPushResult
-        data object NeedToAskUserForDistributor : RegisterUnifiedPushResult
-        data object Error : RegisterUnifiedPushResult
-    }
-
-    suspend fun execute(matrixClient: MatrixClient, distributor: Distributor, clientSecret: String): RegisterUnifiedPushResult {
-        val distributorValue = distributor.value
-        if (distributorValue.isNotEmpty()) {
-            saveAndRegisterApp(distributorValue, clientSecret)
-            val endpoint = unifiedPushStore.getEndpoint(clientSecret) ?: return RegisterUnifiedPushResult.Error
-            val gateway = unifiedPushStore.getPushGateway(clientSecret) ?: return RegisterUnifiedPushResult.Error
-            pusherSubscriber.registerPusher(matrixClient, endpoint, gateway)
-            return RegisterUnifiedPushResult.Success
-        }
-
-        // TODO Below should never happen?
-        if (UnifiedPush.getDistributor(context).isNotEmpty()) {
-            registerApp(clientSecret)
-            return RegisterUnifiedPushResult.Success
-        }
-
-        val distributors = UnifiedPush.getDistributors(context)
-
-        return if (distributors.size == 1) {
-            saveAndRegisterApp(distributors.first(), clientSecret)
-            RegisterUnifiedPushResult.Success
-        } else {
-            RegisterUnifiedPushResult.NeedToAskUserForDistributor
-        }
-    }
-
-    private fun saveAndRegisterApp(distributor: String, clientSecret: String) {
-        UnifiedPush.saveDistributor(context, distributor)
-        registerApp(clientSecret)
-    }
-
-    private fun registerApp(clientSecret: String) {
+    fun execute(distributor: Distributor, clientSecret: String): Result<Unit> {
+        UnifiedPush.saveDistributor(context, distributor.value)
+        // This will trigger the callback
+        // VectorUnifiedPushMessagingReceiver.onNewEndpoint
         UnifiedPush.registerApp(context = context, instance = clientSecret)
+        return Result.success(Unit)
     }
 }

libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushNewGatewayHandler.kt

@@ -16,6 +16,7 @@
 
 package io.element.android.libraries.pushproviders.unifiedpush
 
+import io.element.android.libraries.core.extensions.flatMap
 import io.element.android.libraries.core.log.logger.LoggerTag
 import io.element.android.libraries.matrix.api.auth.MatrixAuthenticationService
 import io.element.android.libraries.pushproviders.api.PusherSubscriber
@@ -35,18 +36,25 @@ class UnifiedPushNewGatewayHandler @Inject constructor(
     private val pushClientSecret: PushClientSecret,
     private val matrixAuthenticationService: MatrixAuthenticationService,
 ) {
-    suspend fun handle(endpoint: String, pushGateway: String, clientSecret: String) {
+    suspend fun handle(endpoint: String, pushGateway: String, clientSecret: String): Result<Unit> {
         // Register the pusher for the session with this client secret, if is it using UnifiedPush.
-        val userId = pushClientSecret.getUserIdFromSecret(clientSecret) ?: return Unit.also {
+        val userId = pushClientSecret.getUserIdFromSecret(clientSecret) ?: return Result.failure<Unit>(
+            IllegalStateException("Unable to retrieve session")
+        ).also {
             Timber.w("Unable to retrieve session")
         }
         val userDataStore = userPushStoreFactory.getOrCreate(userId)
-        if (userDataStore.getPushProviderName() == UnifiedPushConfig.NAME) {
-            matrixAuthenticationService.restoreSession(userId).getOrNull()?.use { client ->
-                pusherSubscriber.registerPusher(client, endpoint, pushGateway)
-            }
+        return if (userDataStore.getPushProviderName() == UnifiedPushConfig.NAME) {
+            matrixAuthenticationService
+                .restoreSession(userId)
+                .flatMap { client ->
+                    pusherSubscriber.registerPusher(client, endpoint, pushGateway)
+                }
         } else {
             Timber.tag(loggerTag.value).d("This session is not using UnifiedPush pusher")
+            Result.failure(
+                IllegalStateException("This session is not using UnifiedPush pusher")
+            )
         }
     }
 }

libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt

@@ -58,10 +58,12 @@ class UnifiedPushProvider @Inject constructor(
         return unifiedPushDistributorProvider.getDistributors()
     }
 
-    override suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor) {
+    override suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor): Result<Unit> {
         val clientSecret = pushClientSecret.getSecretForUser(matrixClient.sessionId)
-        registerUnifiedPushUseCase.execute(matrixClient, distributor, clientSecret)
-        unifiedPushStore.setDistributorValue(matrixClient.sessionId, distributor.value)
+        return registerUnifiedPushUseCase.execute(distributor, clientSecret)
+            .onSuccess {
+                unifiedPushStore.setDistributorValue(matrixClient.sessionId, distributor.value)
+            }
     }
 
     override suspend fun getCurrentDistributor(matrixClient: MatrixClient): Distributor? {
@@ -69,9 +71,9 @@ class UnifiedPushProvider @Inject constructor(
         return getDistributors().find { it.value == distributorValue }
     }
 
-    override suspend fun unregister(matrixClient: MatrixClient) {
+    override suspend fun unregister(matrixClient: MatrixClient): Result<Unit> {
         val clientSecret = pushClientSecret.getSecretForUser(matrixClient.sessionId)
-        unRegisterUnifiedPushUseCase.execute(matrixClient, clientSecret)
+        return unRegisterUnifiedPushUseCase.execute(matrixClient, clientSecret)
     }
 
     override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {

libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnregisterUnifiedPushUseCase.kt

@@ -21,7 +21,6 @@ import io.element.android.libraries.di.ApplicationContext
 import io.element.android.libraries.matrix.api.MatrixClient
 import io.element.android.libraries.pushproviders.api.PusherSubscriber
 import org.unifiedpush.android.connector.UnifiedPush
-import timber.log.Timber
 import javax.inject.Inject
 
 class UnregisterUnifiedPushUseCase @Inject constructor(
@@ -29,18 +28,17 @@ class UnregisterUnifiedPushUseCase @Inject constructor(
     private val unifiedPushStore: UnifiedPushStore,
     private val pusherSubscriber: PusherSubscriber,
 ) {
-    suspend fun execute(matrixClient: MatrixClient, clientSecret: String) {
+    suspend fun execute(matrixClient: MatrixClient, clientSecret: String): Result<Unit> {
         val endpoint = unifiedPushStore.getEndpoint(clientSecret)
         val gateway = unifiedPushStore.getPushGateway(clientSecret)
-        if (endpoint != null && gateway != null) {
-            try {
-                pusherSubscriber.unregisterPusher(matrixClient, endpoint, gateway)
-            } catch (e: Exception) {
-                Timber.d(e, "Probably unregistering a non existing pusher")
-            }
+        if (endpoint == null || gateway == null) {
+            return Result.failure(IllegalStateException("No endpoint or gateway found for client secret"))
         }
-        unifiedPushStore.storeUpEndpoint(null, clientSecret)
-        unifiedPushStore.storePushGateway(null, clientSecret)
-        UnifiedPush.unregisterApp(context)
+        return pusherSubscriber.unregisterPusher(matrixClient, endpoint, gateway)
+            .onSuccess {
+                unifiedPushStore.storeUpEndpoint(null, clientSecret)
+                unifiedPushStore.storePushGateway(null, clientSecret)
+                UnifiedPush.unregisterApp(context)
+            }
     }
 }

libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/VectorUnifiedPushMessagingReceiver.kt

@@ -73,12 +73,17 @@ class VectorUnifiedPushMessagingReceiver : MessagingReceiver() {
         // If the endpoint has changed
         // or the gateway has changed
         if (unifiedPushStore.getEndpoint(instance) != endpoint) {
-            unifiedPushStore.storeUpEndpoint(endpoint, instance)
             coroutineScope.launch {
                 val gateway = unifiedPushGatewayResolver.getGateway(endpoint)
                 unifiedPushStore.storePushGateway(gateway, instance)
                 gateway?.let { pushGateway ->
                     newGatewayHandler.handle(endpoint, pushGateway, instance)
+                        .onFailure {
+                            Timber.tag(loggerTag.value).e("Failed to handle new gateway")
+                        }
+                        .onSuccess {
+                            unifiedPushStore.storeUpEndpoint(endpoint, instance)
+                        }
                 }
             }
         } else {

libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/UserPushStore.kt

@@ -24,7 +24,7 @@ interface UserPushStore {
     suspend fun getPushProviderName(): String?
     suspend fun setPushProviderName(value: String)
     suspend fun getCurrentRegisteredPushKey(): String?
-    suspend fun setCurrentRegisteredPushKey(value: String)
+    suspend fun setCurrentRegisteredPushKey(value: String?)
 
     fun getNotificationEnabledForDevice(): Flow<Boolean>
     suspend fun setNotificationEnabledForDevice(enabled: Boolean)

libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt

@@ -76,9 +76,13 @@ class UserPushStoreDataStore(
         return context.dataStore.data.first()[currentPushKey]
     }
 
-    override suspend fun setCurrentRegisteredPushKey(value: String) {
+    override suspend fun setCurrentRegisteredPushKey(value: String?) {
         context.dataStore.edit {
-            it[currentPushKey] = value
+            if (value == null) {
+                it.remove(currentPushKey)
+            } else {
+                it[currentPushKey] = value
+            }
         }
     }
 

libraries/pushstore/test/src/main/kotlin/com/element/android/libraries/pushstore/test/userpushstore/FakeUserPushStore.kt

@@ -36,7 +36,7 @@ class FakeUserPushStore : UserPushStore {
         return currentRegisteredPushKey
     }
 
-    override suspend fun setCurrentRegisteredPushKey(value: String) {
+    override suspend fun setCurrentRegisteredPushKey(value: String?) {
         currentRegisteredPushKey = value
     }