Merge pull request #2901 from element-hq/feature/bma/fixUnregisteringPusherLocalError

Fix unregistering pusher local error

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

@@ -51,13 +51,16 @@ class DefaultPushService @Inject constructor(
         pushProvider: PushProvider,
         distributor: Distributor,
     ): Result<Unit> {
+        Timber.d("Registering with ${pushProvider.name}/${distributor.name}}")
         val userPushStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId)
         val currentPushProviderName = userPushStore.getPushProviderName()
         val currentPushProvider = pushProviders.find { it.name == currentPushProviderName }
         val currentDistributorValue = currentPushProvider?.getCurrentDistributor(matrixClient)?.value
         if (currentPushProviderName != pushProvider.name || currentDistributorValue != distributor.value) {
             // Unregister previous one if any
-            currentPushProvider?.unregister(matrixClient)
+            currentPushProvider
+                ?.also { Timber.d("Unregistering previous push provider $currentPushProviderName/$currentDistributorValue") }
+                ?.unregister(matrixClient)
                 ?.onFailure {
                     Timber.w(it, "Failed to unregister previous push provider")
                     return Result.failure(it)

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

@@ -64,14 +64,13 @@ class FirebasePushProvider @Inject constructor(
     override suspend fun getCurrentDistributor(matrixClient: MatrixClient) = firebaseDistributor
 
     override suspend fun unregister(matrixClient: MatrixClient): Result<Unit> {
-        val pushKey = firebaseStore.getFcmToken() ?: return Result.failure<Unit>(
+        val pushKey = firebaseStore.getFcmToken()
-            IllegalStateException(
+        return if (pushKey == null) {
-                "Unable to unregister pusher, Firebase token is not known."
-            )
-        ).also {
             Timber.tag(loggerTag.value).w("Unable to unregister pusher, Firebase token is not known.")
+            Result.success(Unit)
+        } else {
+            pusherSubscriber.unregisterPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL)
         }
-        return pusherSubscriber.unregisterPusher(matrixClient, pushKey, FirebaseConfig.PUSHER_HTTP_URL)
     }
 
     override suspend fun getCurrentUserPushConfig(): CurrentUserPushConfig? {

libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt

@@ -134,17 +134,14 @@ class FirebasePushProviderTest {
     }
 
     @Test
-    fun `unregister ko no token`() = runTest {
+    fun `unregister no token - in this case, the error is ignored`() = runTest {
         val firebasePushProvider = createFirebasePushProvider(
             firebaseStore = InMemoryFirebaseStore(
                 token = null
             ),
-            pusherSubscriber = FakePusherSubscriber(
-                unregisterPusherResult = { _, _, _ -> Result.success(Unit) }
-            )
         )
         val result = firebasePushProvider.unregister(FakeMatrixClient())
-        assertThat(result.isFailure).isTrue()
+        assertThat(result.isSuccess).isTrue()
     }
 
     @Test

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

@@ -23,6 +23,7 @@ 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
 
 interface UnregisterUnifiedPushUseCase {
@@ -39,7 +40,11 @@ class DefaultUnregisterUnifiedPushUseCase @Inject constructor(
         val endpoint = unifiedPushStore.getEndpoint(clientSecret)
         val gateway = unifiedPushStore.getPushGateway(clientSecret)
         if (endpoint == null || gateway == null) {
-            return Result.failure(IllegalStateException("No endpoint or gateway found for client secret"))
+            Timber.w("No endpoint or gateway found for client secret")
+            // Ensure we don't have any remaining data, but ignore this error
+            unifiedPushStore.storeUpEndpoint(clientSecret, null)
+            unifiedPushStore.storePushGateway(clientSecret, null)
+            return Result.success(Unit)
         }
         return pusherSubscriber.unregisterPusher(matrixClient, endpoint, gateway)
             .onSuccess {

libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/DefaultUnregisterUnifiedPushUseCaseTest.kt

@@ -64,29 +64,49 @@ class DefaultUnregisterUnifiedPushUseCaseTest {
     }
 
     @Test
-    fun `test un registration error - no endpoint`() = runTest {
+    fun `test un registration error - no endpoint - will not unregister but return success`() = runTest {
         val matrixClient = FakeMatrixClient()
+        val storeUpEndpointResult = lambdaRecorder { _: String, _: String? -> }
+        val storePushGatewayResult = lambdaRecorder { _: String, _: String? -> }
         val useCase = createDefaultUnregisterUnifiedPushUseCase(
             unifiedPushStore = FakeUnifiedPushStore(
                 getEndpointResult = { null },
                 getPushGatewayResult = { "aGateway" },
+                storeUpEndpointResult = storeUpEndpointResult,
+                storePushGatewayResult = storePushGatewayResult,
             ),
         )
         val result = useCase.execute(matrixClient, A_SECRET)
-        assertThat(result.isFailure).isTrue()
+        assertThat(result.isSuccess).isTrue()
+        storeUpEndpointResult.assertions()
+            .isCalledOnce()
+            .with(value(A_SECRET), value(null))
+        storePushGatewayResult.assertions()
+            .isCalledOnce()
+            .with(value(A_SECRET), value(null))
     }
 
     @Test
-    fun `test un registration error - no gateway`() = runTest {
+    fun `test un registration error - no gateway - will not unregister but return success`() = runTest {
         val matrixClient = FakeMatrixClient()
+        val storeUpEndpointResult = lambdaRecorder { _: String, _: String? -> }
+        val storePushGatewayResult = lambdaRecorder { _: String, _: String? -> }
         val useCase = createDefaultUnregisterUnifiedPushUseCase(
             unifiedPushStore = FakeUnifiedPushStore(
                 getEndpointResult = { "aEndpoint" },
                 getPushGatewayResult = { null },
+                storeUpEndpointResult = storeUpEndpointResult,
+                storePushGatewayResult = storePushGatewayResult,
             ),
         )
         val result = useCase.execute(matrixClient, A_SECRET)
-        assertThat(result.isFailure).isTrue()
+        assertThat(result.isSuccess).isTrue()
+        storeUpEndpointResult.assertions()
+            .isCalledOnce()
+            .with(value(A_SECRET), value(null))
+        storePushGatewayResult.assertions()
+            .isCalledOnce()
+            .with(value(A_SECRET), value(null))
     }
 
     @Test