From 148177f24ee568558be83c2e7426e2f0d9f1fcf2 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 14 Jun 2024 12:04:54 +0200 Subject: [PATCH 01/29] Fix typo in log. --- .../element/android/libraries/push/impl/DefaultPushService.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt index 47e26fb920..7600b695f1 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt @@ -51,7 +51,7 @@ class DefaultPushService @Inject constructor( pushProvider: PushProvider, distributor: Distributor, ): Result { - Timber.d("Registering with ${pushProvider.name}/${distributor.name}}") + 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 } From 907fae10ac73bc5c0926ce97efe42714c33db618 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 14 Jun 2024 12:07:54 +0200 Subject: [PATCH 02/29] Pusher add more log and change comment to log. --- .../android/appnav/loggedin/LoggedInPresenter.kt | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index 6362695772..386fe2dd5c 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -55,27 +55,29 @@ class LoggedInPresenter @Inject constructor( LaunchedEffect(isVerified) { if (isVerified) { - // Ensure pusher is registered + Timber.d("Ensure pusher is registered") val currentPushProvider = pushService.getCurrentPushProvider() val result = if (currentPushProvider == null) { - // Register with the first available push provider + Timber.d("Register with the first available push provider") val pushProvider = pushService.getAvailablePushProviders().firstOrNull() ?: return@LaunchedEffect val distributor = pushProvider.getDistributors().firstOrNull() ?: return@LaunchedEffect pushService.registerWith(matrixClient, pushProvider, distributor) } else { val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient) if (currentPushDistributor == null) { - // Register with the first available distributor + Timber.d("Register with the first available distributor") val distributor = currentPushProvider.getDistributors().firstOrNull() ?: return@LaunchedEffect pushService.registerWith(matrixClient, currentPushProvider, distributor) } else { - // Re-register with the current distributor + Timber.d("Re-register with the current distributor") pushService.registerWith(matrixClient, currentPushProvider, currentPushDistributor) } } result.onFailure { Timber.e(it, "Failed to register pusher") } + } else { + Timber.w("Session is not verified, not registering pusher") } } From acde970f01e9a2553949b247d6a5529029a988dd Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 14 Jun 2024 12:19:07 +0200 Subject: [PATCH 03/29] Extract function and add more logs. --- .../appnav/loggedin/LoggedInPresenter.kt | 49 +++++++++++-------- 1 file changed, 28 insertions(+), 21 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index 386fe2dd5c..a2cb95ff6f 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -55,27 +55,7 @@ class LoggedInPresenter @Inject constructor( LaunchedEffect(isVerified) { if (isVerified) { - Timber.d("Ensure pusher is registered") - val currentPushProvider = pushService.getCurrentPushProvider() - val result = if (currentPushProvider == null) { - Timber.d("Register with the first available push provider") - val pushProvider = pushService.getAvailablePushProviders().firstOrNull() ?: return@LaunchedEffect - val distributor = pushProvider.getDistributors().firstOrNull() ?: return@LaunchedEffect - pushService.registerWith(matrixClient, pushProvider, distributor) - } else { - val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient) - if (currentPushDistributor == null) { - Timber.d("Register with the first available distributor") - val distributor = currentPushProvider.getDistributors().firstOrNull() ?: return@LaunchedEffect - pushService.registerWith(matrixClient, currentPushProvider, distributor) - } else { - Timber.d("Re-register with the current distributor") - pushService.registerWith(matrixClient, currentPushProvider, currentPushDistributor) - } - } - result.onFailure { - Timber.e(it, "Failed to register pusher") - } + ensurePusherIsRegistered() } else { Timber.w("Session is not verified, not registering pusher") } @@ -99,6 +79,33 @@ class LoggedInPresenter @Inject constructor( ) } + private suspend fun ensurePusherIsRegistered() { + Timber.d("Ensure pusher is registered") + val currentPushProvider = pushService.getCurrentPushProvider() + val result = if (currentPushProvider == null) { + Timber.d("Register with the first available push provider") + val pushProvider = pushService.getAvailablePushProviders().firstOrNull() + ?: return Unit.also { Timber.w("No push provider available") } + val distributor = pushProvider.getDistributors().firstOrNull() + ?: return Unit.also { Timber.w("No distributor available") } + pushService.registerWith(matrixClient, pushProvider, distributor) + } else { + val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient) + if (currentPushDistributor == null) { + Timber.d("Register with the first available distributor") + val distributor = currentPushProvider.getDistributors().firstOrNull() + ?: return Unit.also { Timber.w("No distributor available") } + pushService.registerWith(matrixClient, currentPushProvider, distributor) + } else { + Timber.d("Re-register with the current distributor") + pushService.registerWith(matrixClient, currentPushProvider, currentPushDistributor) + } + } + result.onFailure { + Timber.e(it, "Failed to register pusher") + } + } + private fun reportCryptoStatusToAnalytics(verificationState: SessionVerifiedStatus, recoveryState: RecoveryState) { // Update first the user property, to store the current status for that posthog user val userVerificationState = verificationState.toAnalyticsUserPropertyValue() From 101a6e6ff2227fc268edf01f5ee67870ec22160f Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 14 Jun 2024 12:30:41 +0200 Subject: [PATCH 04/29] Ensure that the code is not run twice. --- .../io/element/android/appnav/loggedin/LoggedInPresenter.kt | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index a2cb95ff6f..dec25bc065 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -53,11 +53,9 @@ class LoggedInPresenter @Inject constructor( sessionVerificationService.sessionVerifiedStatus.map { it == SessionVerifiedStatus.Verified } }.collectAsState(initial = false) - LaunchedEffect(isVerified) { - if (isVerified) { + if (isVerified) { + LaunchedEffect(Unit) { ensurePusherIsRegistered() - } else { - Timber.w("Session is not verified, not registering pusher") } } From 247b60b9ce6a2941b37c6c20ea1409954d95d89b Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 14 Jun 2024 12:33:43 +0200 Subject: [PATCH 05/29] Add Timber tag. --- .../appnav/loggedin/LoggedInPresenter.kt | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index dec25bc065..0aa6d9bf78 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -27,6 +27,7 @@ import im.vector.app.features.analytics.plan.UserProperties import io.element.android.features.networkmonitor.api.NetworkMonitor import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.libraries.architecture.Presenter +import io.element.android.libraries.core.log.logger.LoggerTag import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.encryption.EncryptionService import io.element.android.libraries.matrix.api.encryption.RecoveryState @@ -39,6 +40,8 @@ import kotlinx.coroutines.flow.map import timber.log.Timber import javax.inject.Inject +private val pusherTag = LoggerTag("Pusher", LoggerTag.PushLoggerTag) + class LoggedInPresenter @Inject constructor( private val matrixClient: MatrixClient, private val networkMonitor: NetworkMonitor, @@ -78,29 +81,29 @@ class LoggedInPresenter @Inject constructor( } private suspend fun ensurePusherIsRegistered() { - Timber.d("Ensure pusher is registered") + Timber.tag(pusherTag.value).d("Ensure pusher is registered") val currentPushProvider = pushService.getCurrentPushProvider() val result = if (currentPushProvider == null) { - Timber.d("Register with the first available push provider") + Timber.tag(pusherTag.value).d("Register with the first available push provider") val pushProvider = pushService.getAvailablePushProviders().firstOrNull() - ?: return Unit.also { Timber.w("No push provider available") } + ?: return Unit.also { Timber.tag(pusherTag.value).w("No push provider available") } val distributor = pushProvider.getDistributors().firstOrNull() - ?: return Unit.also { Timber.w("No distributor available") } + ?: return Unit.also { Timber.tag(pusherTag.value).w("No distributor available") } pushService.registerWith(matrixClient, pushProvider, distributor) } else { val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient) if (currentPushDistributor == null) { - Timber.d("Register with the first available distributor") + Timber.tag(pusherTag.value).d("Register with the first available distributor") val distributor = currentPushProvider.getDistributors().firstOrNull() - ?: return Unit.also { Timber.w("No distributor available") } + ?: return Unit.also { Timber.tag(pusherTag.value).w("No distributor available") } pushService.registerWith(matrixClient, currentPushProvider, distributor) } else { - Timber.d("Re-register with the current distributor") + Timber.tag(pusherTag.value).d("Re-register with the current distributor") pushService.registerWith(matrixClient, currentPushProvider, currentPushDistributor) } } result.onFailure { - Timber.e(it, "Failed to register pusher") + Timber.tag(pusherTag.value).e(it, "Failed to register pusher") } } From 68d2a1af8b24f7f4092963063059d4f2d1dba2ab Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 14 Jun 2024 12:39:20 +0200 Subject: [PATCH 06/29] More log. --- .../android/appnav/loggedin/LoggedInPresenter.kt | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index 0aa6d9bf78..63e1c3f9a8 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -102,9 +102,14 @@ class LoggedInPresenter @Inject constructor( pushService.registerWith(matrixClient, currentPushProvider, currentPushDistributor) } } - result.onFailure { - Timber.tag(pusherTag.value).e(it, "Failed to register pusher") - } + result.fold( + onSuccess = { + Timber.tag(pusherTag.value).d("Pusher registered") + }, + onFailure = { + Timber.tag(pusherTag.value).e(it, "Failed to register pusher") + } + ) } private fun reportCryptoStatusToAnalytics(verificationState: SessionVerifiedStatus, recoveryState: RecoveryState) { From 3d5951cbf021175ce854d4267a3a701f113014a6 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 14 Jun 2024 17:26:06 +0200 Subject: [PATCH 07/29] Add test on pusher registration --- appnav/build.gradle.kts | 1 + .../appnav/loggedin/LoggedInPresenterTest.kt | 310 +++++++++++++++++- .../libraries/push/test/FakePushService.kt | 3 +- .../pushproviders/test/FakePushProvider.kt | 3 +- 4 files changed, 311 insertions(+), 6 deletions(-) diff --git a/appnav/build.gradle.kts b/appnav/build.gradle.kts index 95fb4a8a08..aae3385fb3 100644 --- a/appnav/build.gradle.kts +++ b/appnav/build.gradle.kts @@ -67,6 +67,7 @@ dependencies { testImplementation(libs.test.turbine) testImplementation(projects.libraries.matrix.test) testImplementation(projects.libraries.push.test) + testImplementation(projects.libraries.pushproviders.test) testImplementation(projects.features.networkmonitor.test) testImplementation(projects.features.login.impl) testImplementation(projects.tests.testutils) diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index f57f648599..8d4248df27 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -24,21 +24,36 @@ import im.vector.app.features.analytics.plan.CryptoSessionStateChange import im.vector.app.features.analytics.plan.UserProperties import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.features.networkmonitor.test.FakeNetworkMonitor +import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.encryption.EncryptionService import io.element.android.libraries.matrix.api.encryption.RecoveryState import io.element.android.libraries.matrix.api.roomlist.RoomListService +import io.element.android.libraries.matrix.api.verification.SessionVerificationService import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus +import io.element.android.libraries.matrix.test.AN_EXCEPTION import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.matrix.test.encryption.FakeEncryptionService import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService import io.element.android.libraries.matrix.test.verification.FakeSessionVerificationService +import io.element.android.libraries.push.api.PushService import io.element.android.libraries.push.test.FakePushService +import io.element.android.libraries.pushproviders.api.Distributor +import io.element.android.libraries.pushproviders.api.PushProvider +import io.element.android.libraries.pushproviders.test.FakePushProvider +import io.element.android.services.analytics.api.AnalyticsService import io.element.android.services.analytics.test.FakeAnalyticsService import io.element.android.tests.testutils.WarmUpRule import io.element.android.tests.testutils.consumeItemsUntilPredicate +import io.element.android.tests.testutils.lambda.any +import io.element.android.tests.testutils.lambda.lambdaRecorder +import io.element.android.tests.testutils.lambda.value +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test +@OptIn(ExperimentalCoroutinesApi::class) class LoggedInPresenterTest { @get:Rule val warmUpRule = WarmUpRule() @@ -107,17 +122,304 @@ class LoggedInPresenterTest { } } + @Test + fun `present - ensure default pusher is not registered if session is not verified`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.success(Unit) + } + val pushService = createFakePushService(registerWithLambda = lambda) + val presenter = createLoggedInPresenter(pushService = pushService) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(1) + lambda.assertions() + .isNeverCalled() + } + } + + @Test + fun `present - ensure default pusher is registered with default provider`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.success(Unit) + } + val sessionVerificationService = FakeSessionVerificationService() + sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val pushService = createFakePushService( + registerWithLambda = lambda, + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = sessionVerificationService, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(2) + advanceUntilIdle() + lambda.assertions() + .isCalledOnce() + .with( + // MatrixClient + any(), + // PushProvider with highest priority (lower index) + value(pushService.getAvailablePushProviders()[0]), + // First distributor + value(pushService.getAvailablePushProviders()[0].getDistributors()[0]), + ) + } + } + + @Test + fun `present - ensure default pusher is registered with default provider - fail to register`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.failure(AN_EXCEPTION) + } + val sessionVerificationService = FakeSessionVerificationService() + sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val pushService = createFakePushService( + registerWithLambda = lambda, + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = sessionVerificationService, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(2) + advanceUntilIdle() + lambda.assertions() + .isCalledOnce() + .with( + // MatrixClient + any(), + // PushProvider with highest priority (lower index) + value(pushService.getAvailablePushProviders()[0]), + // First distributor + value(pushService.getAvailablePushProviders()[0].getDistributors()[0]), + ) + } + } + + @Test + fun `present - ensure current provider is registered with current distributor`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.success(Unit) + } + val sessionVerificationService = FakeSessionVerificationService() + sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val distributor = Distributor("aDistributorValue1", "aDistributorName1") + val pushProvider = FakePushProvider( + index = 1, + name = "aFakePushProvider0", + isAvailable = true, + distributors = listOf( + Distributor("aDistributorValue0", "aDistributorName0"), + distributor, + ), + currentDistributor = { distributor }, + ) + val pushService = createFakePushService( + pushProvider1 = pushProvider, + currentPushProvider = { pushProvider }, + registerWithLambda = lambda, + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = sessionVerificationService, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(2) + advanceUntilIdle() + lambda.assertions() + .isCalledOnce() + .with( + // MatrixClient + any(), + // Current push provider + value(pushProvider), + // Current distributor + value(distributor), + ) + } + } + + @Test + fun `present - if current push provider does not have current distributor, the first one is used`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.success(Unit) + } + val sessionVerificationService = FakeSessionVerificationService() + sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val pushProvider = FakePushProvider( + index = 0, + name = "aFakePushProvider0", + isAvailable = true, + distributors = listOf( + Distributor("aDistributorValue0", "aDistributorName0"), + Distributor("aDistributorValue1", "aDistributorName1"), + ), + currentDistributor = { null }, + ) + val pushService = createFakePushService( + pushProvider0 = pushProvider, + currentPushProvider = { pushProvider }, + registerWithLambda = lambda, + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = sessionVerificationService, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(2) + advanceUntilIdle() + lambda.assertions() + .isCalledOnce() + .with( + // MatrixClient + any(), + // PushProvider with highest priority (lower index) + value(pushService.getAvailablePushProviders()[0]), + // First distributor + value(pushService.getAvailablePushProviders()[0].getDistributors()[0]), + ) + } + } + + @Test + fun `present - if current push provider does not have distributors, nothing happen`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.success(Unit) + } + val sessionVerificationService = FakeSessionVerificationService() + sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val pushProvider = FakePushProvider( + index = 0, + name = "aFakePushProvider0", + isAvailable = true, + distributors = emptyList(), + ) + val pushService = createFakePushService( + pushProvider0 = pushProvider, + currentPushProvider = { pushProvider }, + registerWithLambda = lambda, + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = sessionVerificationService, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(2) + advanceUntilIdle() + lambda.assertions() + .isNeverCalled() + } + } + + @Test + fun `present - case no push provider available provider`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.success(Unit) + } + val sessionVerificationService = FakeSessionVerificationService() + sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val pushService = createFakePushService( + pushProvider0 = null, + pushProvider1 = null, + registerWithLambda = lambda, + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = sessionVerificationService, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(2) + advanceUntilIdle() + lambda.assertions() + .isNeverCalled() + } + } + + @Test + fun `present - case one push provider but no distributor available`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.success(Unit) + } + val sessionVerificationService = FakeSessionVerificationService() + sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val pushService = createFakePushService( + pushProvider0 = FakePushProvider( + index = 1, + name = "aFakePushProvider1", + isAvailable = true, + distributors = emptyList(), + ), + pushProvider1 = null, + registerWithLambda = lambda, + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = sessionVerificationService, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(2) + advanceUntilIdle() + lambda.assertions() + .isNeverCalled() + } + } + + private fun createFakePushService( + pushProvider0: PushProvider? = FakePushProvider( + index = 0, + name = "aFakePushProvider0", + isAvailable = true, + distributors = listOf(Distributor("aDistributorValue0", "aDistributorName0")), + currentDistributor = { null }, + ), + pushProvider1: PushProvider? = FakePushProvider( + index = 1, + name = "aFakePushProvider1", + isAvailable = true, + distributors = listOf(Distributor("aDistributorValue1", "aDistributorName1")), + currentDistributor = { null }, + ), + registerWithLambda: suspend (MatrixClient, PushProvider, Distributor) -> Result = { _, _, _ -> + Result.success(Unit) + }, + currentPushProvider: () -> PushProvider? = { null }, + ): PushService { + return FakePushService( + availablePushProviders = listOfNotNull(pushProvider0, pushProvider1), + registerWithLambda = registerWithLambda, + currentPushProvider = currentPushProvider, + ) + } + private fun createLoggedInPresenter( roomListService: RoomListService = FakeRoomListService(), networkStatus: NetworkStatus = NetworkStatus.Offline, - analyticsService: FakeAnalyticsService = FakeAnalyticsService(), - encryptionService: FakeEncryptionService = FakeEncryptionService(), + analyticsService: AnalyticsService = FakeAnalyticsService(), + sessionVerificationService: SessionVerificationService = FakeSessionVerificationService(), + encryptionService: EncryptionService = FakeEncryptionService(), + pushService: PushService = FakePushService(), ): LoggedInPresenter { return LoggedInPresenter( matrixClient = FakeMatrixClient(roomListService = roomListService), networkMonitor = FakeNetworkMonitor(networkStatus), - pushService = FakePushService(), - sessionVerificationService = FakeSessionVerificationService(), + pushService = pushService, + sessionVerificationService = sessionVerificationService, analyticsService = analyticsService, encryptionService = encryptionService ) diff --git a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt index 5e7d9e7ff1..0bf2da54ad 100644 --- a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt +++ b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt @@ -28,9 +28,10 @@ class FakePushService( private val registerWithLambda: suspend (MatrixClient, PushProvider, Distributor) -> Result = { _, _, _ -> Result.success(Unit) }, + private val currentPushProvider: () -> PushProvider? = { availablePushProviders.firstOrNull() }, ) : PushService { override suspend fun getCurrentPushProvider(): PushProvider? { - return registeredPushProvider ?: availablePushProviders.firstOrNull() + return registeredPushProvider ?: currentPushProvider() } override fun getAvailablePushProviders(): List { diff --git a/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt b/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt index 7b37d0d296..4f83cde911 100644 --- a/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt +++ b/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt @@ -27,6 +27,7 @@ class FakePushProvider( override val name: String = "aFakePushProvider", private val isAvailable: Boolean = true, private val distributors: List = listOf(Distributor("aDistributorValue", "aDistributorName")), + private val currentDistributor: () -> Distributor? = { distributors.firstOrNull() }, private val currentUserPushConfig: CurrentUserPushConfig? = null, private val registerWithResult: (MatrixClient, Distributor) -> Result = { _, _ -> lambdaError() }, private val unregisterWithResult: (MatrixClient) -> Result = { lambdaError() }, @@ -40,7 +41,7 @@ class FakePushProvider( } override suspend fun getCurrentDistributor(matrixClient: MatrixClient): Distributor? { - return distributors.firstOrNull() + return currentDistributor() } override suspend fun unregister(matrixClient: MatrixClient): Result { From 21ce1c40b31463e9b340e83c987850d2b1e74a8b Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Fri, 14 Jun 2024 18:40:43 +0200 Subject: [PATCH 08/29] Add pusher status in the state. It improve the tests and we may want to render errors in the View at some point. --- .../appnav/loggedin/LoggedInPresenter.kt | 29 ++++++++++++---- .../android/appnav/loggedin/LoggedInState.kt | 3 ++ .../appnav/loggedin/LoggedInStateProvider.kt | 2 ++ .../loggedin/PusherRegistrationFailure.kt | 24 +++++++++++++ .../appnav/loggedin/LoggedInPresenterTest.kt | 34 +++++++++++++------ 5 files changed, 74 insertions(+), 18 deletions(-) create mode 100644 appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index 63e1c3f9a8..737b5248ec 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -18,14 +18,17 @@ package io.element.android.appnav.loggedin import androidx.compose.runtime.Composable import androidx.compose.runtime.LaunchedEffect +import androidx.compose.runtime.MutableState import androidx.compose.runtime.collectAsState import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember import im.vector.app.features.analytics.plan.CryptoSessionStateChange import im.vector.app.features.analytics.plan.UserProperties import io.element.android.features.networkmonitor.api.NetworkMonitor import io.element.android.features.networkmonitor.api.NetworkStatus +import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.architecture.Presenter import io.element.android.libraries.core.log.logger.LoggerTag import io.element.android.libraries.matrix.api.MatrixClient @@ -55,13 +58,16 @@ class LoggedInPresenter @Inject constructor( val isVerified by remember { sessionVerificationService.sessionVerifiedStatus.map { it == SessionVerifiedStatus.Verified } }.collectAsState(initial = false) - + val pusherRegistrationState = remember>> { mutableStateOf(AsyncData.Uninitialized) } if (isVerified) { LaunchedEffect(Unit) { - ensurePusherIsRegistered() + ensurePusherIsRegistered(pusherRegistrationState) + } + } else { + LaunchedEffect(Unit) { + pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.AccountNotVerified()) } } - val syncIndicator by matrixClient.roomListService.syncIndicator.collectAsState() val networkStatus by networkMonitor.connectivity.collectAsState() val showSyncSpinner by remember { @@ -77,25 +83,32 @@ class LoggedInPresenter @Inject constructor( return LoggedInState( showSyncSpinner = showSyncSpinner, + pusherRegistrationState = pusherRegistrationState.value, ) } - private suspend fun ensurePusherIsRegistered() { + private suspend fun ensurePusherIsRegistered(pusherRegistrationState: MutableState>) { Timber.tag(pusherTag.value).d("Ensure pusher is registered") val currentPushProvider = pushService.getCurrentPushProvider() val result = if (currentPushProvider == null) { Timber.tag(pusherTag.value).d("Register with the first available push provider") val pushProvider = pushService.getAvailablePushProviders().firstOrNull() - ?: return Unit.also { Timber.tag(pusherTag.value).w("No push provider available") } + ?: return Unit + .also { Timber.tag(pusherTag.value).w("No push providers available") } + .also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoProvidersAvailable()) } val distributor = pushProvider.getDistributors().firstOrNull() - ?: return Unit.also { Timber.tag(pusherTag.value).w("No distributor available") } + ?: return Unit + .also { Timber.tag(pusherTag.value).w("No distributors available") } + .also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoDistributorsAvailable()) } pushService.registerWith(matrixClient, pushProvider, distributor) } else { val currentPushDistributor = currentPushProvider.getCurrentDistributor(matrixClient) if (currentPushDistributor == null) { Timber.tag(pusherTag.value).d("Register with the first available distributor") val distributor = currentPushProvider.getDistributors().firstOrNull() - ?: return Unit.also { Timber.tag(pusherTag.value).w("No distributor available") } + ?: return Unit + .also { Timber.tag(pusherTag.value).w("No distributors available") } + .also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoDistributorsAvailable()) } pushService.registerWith(matrixClient, currentPushProvider, distributor) } else { Timber.tag(pusherTag.value).d("Re-register with the current distributor") @@ -105,9 +118,11 @@ class LoggedInPresenter @Inject constructor( result.fold( onSuccess = { Timber.tag(pusherTag.value).d("Pusher registered") + pusherRegistrationState.value = AsyncData.Success(Unit) }, onFailure = { Timber.tag(pusherTag.value).e(it, "Failed to register pusher") + pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.RegistrationFailure(it)) } ) } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt index 4196277698..2e3db40c9a 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt @@ -16,6 +16,9 @@ package io.element.android.appnav.loggedin +import io.element.android.libraries.architecture.AsyncData + data class LoggedInState( val showSyncSpinner: Boolean, + val pusherRegistrationState: AsyncData, ) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt index 0e8fdef8d8..89f71f384a 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt @@ -17,6 +17,7 @@ package io.element.android.appnav.loggedin import androidx.compose.ui.tooling.preview.PreviewParameterProvider +import io.element.android.libraries.architecture.AsyncData open class LoggedInStateProvider : PreviewParameterProvider { override val values: Sequence @@ -31,4 +32,5 @@ fun aLoggedInState( showSyncSpinner: Boolean = true, ) = LoggedInState( showSyncSpinner = showSyncSpinner, + pusherRegistrationState = AsyncData.Uninitialized, ) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt new file mode 100644 index 0000000000..791043c6e2 --- /dev/null +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt @@ -0,0 +1,24 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.appnav.loggedin + +sealed class PusherRegistrationFailure : Exception() { + class AccountNotVerified : PusherRegistrationFailure() + class NoProvidersAvailable : PusherRegistrationFailure() + class NoDistributorsAvailable : PusherRegistrationFailure() + class RegistrationFailure(val failure: Throwable) : PusherRegistrationFailure() +} diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index 8d4248df27..27f44eaeef 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -47,13 +47,10 @@ import io.element.android.tests.testutils.consumeItemsUntilPredicate import io.element.android.tests.testutils.lambda.any import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value -import kotlinx.coroutines.ExperimentalCoroutinesApi -import kotlinx.coroutines.test.advanceUntilIdle import kotlinx.coroutines.test.runTest import org.junit.Rule import org.junit.Test -@OptIn(ExperimentalCoroutinesApi::class) class LoggedInPresenterTest { @get:Rule val warmUpRule = WarmUpRule() @@ -66,6 +63,8 @@ class LoggedInPresenterTest { }.test { val initialState = awaitItem() assertThat(initialState.showSyncSpinner).isFalse() + assertThat(initialState.pusherRegistrationState.isUninitialized()).isTrue() + skipItems(1) } } @@ -106,7 +105,7 @@ class LoggedInPresenterTest { encryptionService.emitRecoveryState(RecoveryState.INCOMPLETE) verificationService.emitVerifiedStatus(SessionVerifiedStatus.Verified) - skipItems(4) + skipItems(6) assertThat(analyticsService.capturedEvents.size).isEqualTo(1) assertThat(analyticsService.capturedEvents[0]).isInstanceOf(CryptoSessionStateChange::class.java) @@ -133,6 +132,9 @@ class LoggedInPresenterTest { presenter.present() }.test { skipItems(1) + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.errorOrNull()) + .isInstanceOf(PusherRegistrationFailure.AccountNotVerified::class.java) lambda.assertions() .isNeverCalled() } @@ -156,7 +158,8 @@ class LoggedInPresenterTest { presenter.present() }.test { skipItems(2) - advanceUntilIdle() + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.isSuccess()).isTrue() lambda.assertions() .isCalledOnce() .with( @@ -188,7 +191,8 @@ class LoggedInPresenterTest { presenter.present() }.test { skipItems(2) - advanceUntilIdle() + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.isFailure()).isTrue() lambda.assertions() .isCalledOnce() .with( @@ -233,7 +237,8 @@ class LoggedInPresenterTest { presenter.present() }.test { skipItems(2) - advanceUntilIdle() + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.isSuccess()).isTrue() lambda.assertions() .isCalledOnce() .with( @@ -277,7 +282,8 @@ class LoggedInPresenterTest { presenter.present() }.test { skipItems(2) - advanceUntilIdle() + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.isSuccess()).isTrue() lambda.assertions() .isCalledOnce() .with( @@ -317,7 +323,9 @@ class LoggedInPresenterTest { presenter.present() }.test { skipItems(2) - advanceUntilIdle() + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.errorOrNull()) + .isInstanceOf(PusherRegistrationFailure.NoDistributorsAvailable::class.java) lambda.assertions() .isNeverCalled() } @@ -343,7 +351,9 @@ class LoggedInPresenterTest { presenter.present() }.test { skipItems(2) - advanceUntilIdle() + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.errorOrNull()) + .isInstanceOf(PusherRegistrationFailure.NoProvidersAvailable::class.java) lambda.assertions() .isNeverCalled() } @@ -374,7 +384,9 @@ class LoggedInPresenterTest { presenter.present() }.test { skipItems(2) - advanceUntilIdle() + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.errorOrNull()) + .isInstanceOf(PusherRegistrationFailure.NoDistributorsAvailable::class.java) lambda.assertions() .isNeverCalled() } From 725c3838af86432c1e2236e8ca53cd999351fa1e Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 09:48:21 +0200 Subject: [PATCH 09/29] Render an error dialog in case registering a pusher fails. --- .../android/appnav/loggedin/LoggedInEvents.kt | 21 +++++++++++ .../appnav/loggedin/LoggedInPresenter.kt | 16 +++++++- .../android/appnav/loggedin/LoggedInState.kt | 1 + .../appnav/loggedin/LoggedInStateProvider.kt | 12 +++--- .../android/appnav/loggedin/LoggedInView.kt | 37 +++++++++++++++++++ .../loggedin/PusherRegistrationFailure.kt | 12 +++++- .../appnav/loggedin/LoggedInPresenterTest.kt | 4 ++ .../matrix/api/exception/ClientException.kt | 4 ++ .../matrix/impl/pushers/RustPushersService.kt | 3 ++ .../push/impl/DefaultPusherSubscriber.kt | 14 ++++++- .../pushproviders/api/PusherSubscriber.kt | 13 +++++++ .../src/main/res/values/localazy.xml | 3 ++ 12 files changed, 131 insertions(+), 9 deletions(-) create mode 100644 appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInEvents.kt diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInEvents.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInEvents.kt new file mode 100644 index 0000000000..d93e4fabfc --- /dev/null +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInEvents.kt @@ -0,0 +1,21 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.appnav.loggedin + +sealed interface LoggedInEvents { + data object CloseErrorDialog : LoggedInEvents +} diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index 737b5248ec..b28e33dc16 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -38,6 +38,7 @@ import io.element.android.libraries.matrix.api.roomlist.RoomListService import io.element.android.libraries.matrix.api.verification.SessionVerificationService import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus import io.element.android.libraries.push.api.PushService +import io.element.android.libraries.pushproviders.api.RegistrationFailure import io.element.android.services.analytics.api.AnalyticsService import kotlinx.coroutines.flow.map import timber.log.Timber @@ -81,9 +82,16 @@ class LoggedInPresenter @Inject constructor( reportCryptoStatusToAnalytics(verificationState, recoveryState) } + fun handleEvent(event: LoggedInEvents) { + when (event) { + LoggedInEvents.CloseErrorDialog -> pusherRegistrationState.value = AsyncData.Uninitialized + } + } + return LoggedInState( showSyncSpinner = showSyncSpinner, pusherRegistrationState = pusherRegistrationState.value, + eventSink = ::handleEvent ) } @@ -122,7 +130,13 @@ class LoggedInPresenter @Inject constructor( }, onFailure = { Timber.tag(pusherTag.value).e(it, "Failed to register pusher") - pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.RegistrationFailure(it)) + if (it is RegistrationFailure) { + pusherRegistrationState.value = AsyncData.Failure( + PusherRegistrationFailure.RegistrationFailure(it.clientException, it.isRegisteringAgain) + ) + } else { + pusherRegistrationState.value = AsyncData.Failure(it) + } } ) } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt index 2e3db40c9a..fabaff786e 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt @@ -21,4 +21,5 @@ import io.element.android.libraries.architecture.AsyncData data class LoggedInState( val showSyncSpinner: Boolean, val pusherRegistrationState: AsyncData, + val eventSink: (LoggedInEvents) -> Unit, ) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt index 89f71f384a..bdb0aaacf0 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt @@ -22,15 +22,17 @@ import io.element.android.libraries.architecture.AsyncData open class LoggedInStateProvider : PreviewParameterProvider { override val values: Sequence get() = sequenceOf( - aLoggedInState(false), - aLoggedInState(true), - // Add other state here + aLoggedInState(), + aLoggedInState(showSyncSpinner = true), + aLoggedInState(pusherRegistrationState = AsyncData.Failure(PusherRegistrationFailure.NoProvidersAvailable())), ) } fun aLoggedInState( - showSyncSpinner: Boolean = true, + showSyncSpinner: Boolean = false, + pusherRegistrationState: AsyncData = AsyncData.Uninitialized, ) = LoggedInState( showSyncSpinner = showSyncSpinner, - pusherRegistrationState = AsyncData.Uninitialized, + pusherRegistrationState = pusherRegistrationState, + eventSink = {}, ) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt index 74c71d387c..b997bc0b42 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt @@ -22,9 +22,14 @@ import androidx.compose.foundation.layout.systemBarsPadding import androidx.compose.runtime.Composable import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource import androidx.compose.ui.tooling.preview.PreviewParameter +import io.element.android.libraries.architecture.AsyncData +import io.element.android.libraries.designsystem.components.dialogs.ErrorDialog import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight +import io.element.android.libraries.matrix.api.exception.isNetworkError +import io.element.android.libraries.ui.strings.CommonStrings @Composable fun LoggedInView( @@ -41,6 +46,38 @@ fun LoggedInView( isVisible = state.showSyncSpinner, ) } + when (state.pusherRegistrationState) { + is AsyncData.Uninitialized, + is AsyncData.Loading, + is AsyncData.Success -> Unit + is AsyncData.Failure -> { + state.pusherRegistrationState.errorOrNull() + ?.getReason() + ?.let { reason -> + ErrorDialog( + content = stringResource(id = CommonStrings.common_error_registering_pusher_android, reason), + onDismiss = { state.eventSink(LoggedInEvents.CloseErrorDialog) }, + ) + } + } + } +} + +private fun Throwable.getReason(): String? { + return when (this) { + is PusherRegistrationFailure.RegistrationFailure -> { + if (isRegisteringAgain && clientException.isNetworkError()) { + // When registering again, ignore network error + null + } else { + clientException.message ?: "Unknown error" + } + } + is PusherRegistrationFailure.AccountNotVerified -> null + is PusherRegistrationFailure.NoDistributorsAvailable -> "No distributors available" + is PusherRegistrationFailure.NoProvidersAvailable -> "No providers available" + else -> "Other error" + } } @PreviewsDayNight diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt index 791043c6e2..1fe1009b07 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/PusherRegistrationFailure.kt @@ -16,9 +16,19 @@ package io.element.android.appnav.loggedin +import io.element.android.libraries.matrix.api.exception.ClientException + sealed class PusherRegistrationFailure : Exception() { class AccountNotVerified : PusherRegistrationFailure() class NoProvidersAvailable : PusherRegistrationFailure() class NoDistributorsAvailable : PusherRegistrationFailure() - class RegistrationFailure(val failure: Throwable) : PusherRegistrationFailure() + + /** + * @param clientException the failure that occurred. + * @param isRegisteringAgain true if the server should already have a the same pusher registered. + */ + class RegistrationFailure( + val clientException: ClientException, + val isRegisteringAgain: Boolean, + ) : PusherRegistrationFailure() } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index 27f44eaeef..aa2f720343 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -389,6 +389,10 @@ class LoggedInPresenterTest { .isInstanceOf(PusherRegistrationFailure.NoDistributorsAvailable::class.java) lambda.assertions() .isNeverCalled() + // Reset the error + finalState.eventSink(LoggedInEvents.CloseErrorDialog) + val lastState = awaitItem() + assertThat(lastState.pusherRegistrationState.isUninitialized()).isTrue() } } diff --git a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/exception/ClientException.kt b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/exception/ClientException.kt index 52dbd2eb12..afe7fca1be 100644 --- a/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/exception/ClientException.kt +++ b/libraries/matrix/api/src/main/kotlin/io/element/android/libraries/matrix/api/exception/ClientException.kt @@ -20,3 +20,7 @@ sealed class ClientException(message: String) : Exception(message) { class Generic(message: String) : ClientException(message) class Other(message: String) : ClientException(message) } + +fun ClientException.isNetworkError(): Boolean { + return this is ClientException.Generic && message?.contains("error sending request for url", ignoreCase = true) == true +} diff --git a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/pushers/RustPushersService.kt b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/pushers/RustPushersService.kt index 2686d03c6b..3274ad17ad 100644 --- a/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/pushers/RustPushersService.kt +++ b/libraries/matrix/impl/src/main/kotlin/io/element/android/libraries/matrix/impl/pushers/RustPushersService.kt @@ -17,9 +17,11 @@ package io.element.android.libraries.matrix.impl.pushers import io.element.android.libraries.core.coroutine.CoroutineDispatchers +import io.element.android.libraries.core.extensions.mapFailure import io.element.android.libraries.matrix.api.pusher.PushersService import io.element.android.libraries.matrix.api.pusher.SetHttpPusherData import io.element.android.libraries.matrix.api.pusher.UnsetHttpPusherData +import io.element.android.libraries.matrix.impl.exception.mapClientException import kotlinx.coroutines.withContext import org.matrix.rustcomponents.sdk.Client import org.matrix.rustcomponents.sdk.HttpPusherData @@ -52,6 +54,7 @@ class RustPushersService( lang = setHttpPusherData.lang ) } + .mapFailure { it.mapClientException() } } } diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPusherSubscriber.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPusherSubscriber.kt index 481081de1f..da64ebb9ec 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPusherSubscriber.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPusherSubscriber.kt @@ -18,14 +18,17 @@ package io.element.android.libraries.push.impl import com.squareup.anvil.annotations.ContributesBinding import io.element.android.appconfig.PushConfig +import io.element.android.libraries.core.extensions.mapFailure import io.element.android.libraries.core.log.logger.LoggerTag import io.element.android.libraries.core.meta.BuildMeta import io.element.android.libraries.di.AppScope import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.api.core.SessionId +import io.element.android.libraries.matrix.api.exception.ClientException import io.element.android.libraries.matrix.api.pusher.SetHttpPusherData import io.element.android.libraries.matrix.api.pusher.UnsetHttpPusherData import io.element.android.libraries.pushproviders.api.PusherSubscriber +import io.element.android.libraries.pushproviders.api.RegistrationFailure import io.element.android.libraries.pushstore.api.UserPushStoreFactory import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecret import timber.log.Timber @@ -50,7 +53,8 @@ class DefaultPusherSubscriber @Inject constructor( gateway: String, ): Result { val userDataStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId) - if (userDataStore.getCurrentRegisteredPushKey() == pushKey) { + val isRegisteringAgain = userDataStore.getCurrentRegisteredPushKey() == pushKey + if (isRegisteringAgain) { 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") } @@ -61,8 +65,14 @@ class DefaultPusherSubscriber @Inject constructor( .onSuccess { userDataStore.setCurrentRegisteredPushKey(pushKey) } - .onFailure { throwable -> + .mapFailure { throwable -> Timber.tag(loggerTag.value).e(throwable, "Unable to register the pusher") + if (throwable is ClientException) { + // It should always be the case. + RegistrationFailure(throwable, isRegisteringAgain = isRegisteringAgain) + } else { + throwable + } } } diff --git a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PusherSubscriber.kt b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PusherSubscriber.kt index d38f5dec1e..7a9e49d287 100644 --- a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PusherSubscriber.kt +++ b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PusherSubscriber.kt @@ -17,8 +17,21 @@ package io.element.android.libraries.pushproviders.api import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.exception.ClientException interface PusherSubscriber { + /** + * Register a pusher. Note that failure will be a [RegistrationFailure]. + */ suspend fun registerPusher(matrixClient: MatrixClient, pushKey: String, gateway: String): Result + + /** + * Unregister a pusher. + */ suspend fun unregisterPusher(matrixClient: MatrixClient, pushKey: String, gateway: String): Result } + +class RegistrationFailure( + val clientException: ClientException, + val isRegisteringAgain: Boolean +) : Exception(clientException) diff --git a/libraries/ui-strings/src/main/res/values/localazy.xml b/libraries/ui-strings/src/main/res/values/localazy.xml index 25c2183d0a..fd7f695c58 100644 --- a/libraries/ui-strings/src/main/res/values/localazy.xml +++ b/libraries/ui-strings/src/main/res/values/localazy.xml @@ -135,6 +135,9 @@ "Encryption enabled" "Enter your PIN" "Error" + "An error occurred, you may not receive notifications for new messages. Please troubleshoot notifications from the settings. + +Reason: %1$s." "Everyone" "Failed" "Favourite" From 675f93a5ad8ac8a6e4bb51b78ba0b5d61fb8825d Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 10:55:09 +0200 Subject: [PATCH 10/29] Create component ErrorDialogWithDoNotShowAgain --- .../dialogs/ErrorDialogWithDoNotShowAgain.kt | 86 +++++++++++++++++++ .../src/main/res/values/localazy.xml | 1 + 2 files changed, 87 insertions(+) create mode 100644 libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/dialogs/ErrorDialogWithDoNotShowAgain.kt diff --git a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/dialogs/ErrorDialogWithDoNotShowAgain.kt b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/dialogs/ErrorDialogWithDoNotShowAgain.kt new file mode 100644 index 0000000000..671e35c926 --- /dev/null +++ b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/dialogs/ErrorDialogWithDoNotShowAgain.kt @@ -0,0 +1,86 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.designsystem.components.dialogs + +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.Spacer +import androidx.compose.foundation.layout.height +import androidx.compose.material3.BasicAlertDialog +import androidx.compose.material3.ExperimentalMaterial3Api +import androidx.compose.runtime.Composable +import androidx.compose.runtime.getValue +import androidx.compose.runtime.mutableStateOf +import androidx.compose.runtime.remember +import androidx.compose.runtime.setValue +import androidx.compose.ui.Alignment +import androidx.compose.ui.Modifier +import androidx.compose.ui.res.stringResource +import androidx.compose.ui.unit.dp +import io.element.android.compound.theme.ElementTheme +import io.element.android.libraries.designsystem.preview.ElementPreview +import io.element.android.libraries.designsystem.preview.PreviewsDayNight +import io.element.android.libraries.designsystem.theme.components.Checkbox +import io.element.android.libraries.designsystem.theme.components.SimpleAlertDialogContent +import io.element.android.libraries.designsystem.theme.components.Text +import io.element.android.libraries.ui.strings.CommonStrings + +@OptIn(ExperimentalMaterial3Api::class) +@Composable +fun ErrorDialogWithDoNotShowAgain( + content: String, + onDismiss: (Boolean) -> Unit, + modifier: Modifier = Modifier, + title: String = ErrorDialogDefaults.title, + submitText: String = ErrorDialogDefaults.submitText, +) { + var doNotShowAgain by remember { mutableStateOf(false) } + BasicAlertDialog( + modifier = modifier, + onDismissRequest = { onDismiss(doNotShowAgain) } + ) { + SimpleAlertDialogContent( + title = title, + submitText = submitText, + onSubmitClick = { onDismiss(doNotShowAgain) }, + ) { + Column { + Text( + text = content, + style = ElementTheme.materialTypography.bodyMedium, + ) + Spacer(modifier = Modifier.height(8.dp)) + Row(verticalAlignment = Alignment.CenterVertically) { + Checkbox(checked = doNotShowAgain, onCheckedChange = { doNotShowAgain = it }) + Text( + text = stringResource(id = CommonStrings.common_do_not_show_this_again), + style = ElementTheme.materialTypography.bodyMedium, + ) + } + } + } + } +} + +@PreviewsDayNight +@Composable +internal fun ErrorDialogWithDoNotShowAgainPreview() = ElementPreview { + ErrorDialogWithDoNotShowAgain( + content = "Content", + onDismiss = {}, + ) +} diff --git a/libraries/ui-strings/src/main/res/values/localazy.xml b/libraries/ui-strings/src/main/res/values/localazy.xml index fd7f695c58..7c7b23cfa6 100644 --- a/libraries/ui-strings/src/main/res/values/localazy.xml +++ b/libraries/ui-strings/src/main/res/values/localazy.xml @@ -129,6 +129,7 @@ "Decryption error" "Developer options" "Direct chat" + "Do not show this again" "(edited)" "Editing" "* %1$s %2$s" From 64930e4435bcb2474683ba260773fe4188f960f1 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 11:15:47 +0200 Subject: [PATCH 11/29] Add ability to not show the pusher registration again. --- .../android/appnav/loggedin/LoggedInEvents.kt | 2 +- .../android/appnav/loggedin/LoggedInPresenter.kt | 16 +++++++++++++++- .../android/appnav/loggedin/LoggedInState.kt | 1 + .../appnav/loggedin/LoggedInStateProvider.kt | 1 + .../android/appnav/loggedin/LoggedInView.kt | 7 ++++--- .../appnav/loggedin/LoggedInPresenterTest.kt | 9 ++++++++- .../preferences/impl/tasks/ClearCacheUseCase.kt | 4 ++++ .../android/libraries/push/api/PushService.kt | 5 +++++ .../libraries/push/impl/DefaultPushService.kt | 10 ++++++++++ .../libraries/push/test/FakePushService.kt | 13 +++++++++++++ .../libraries/pushstore/api/UserPushStore.kt | 3 +++ .../pushstore/impl/UserPushStoreDataStore.kt | 12 ++++++++++++ .../test/userpushstore/FakeUserPushStore.kt | 9 +++++++++ 13 files changed, 86 insertions(+), 6 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInEvents.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInEvents.kt index d93e4fabfc..43a2c2b485 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInEvents.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInEvents.kt @@ -17,5 +17,5 @@ package io.element.android.appnav.loggedin sealed interface LoggedInEvents { - data object CloseErrorDialog : LoggedInEvents + data class CloseErrorDialog(val doNotShowAgain: Boolean) : LoggedInEvents } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index b28e33dc16..e70a44aa2b 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -24,6 +24,7 @@ import androidx.compose.runtime.derivedStateOf import androidx.compose.runtime.getValue import androidx.compose.runtime.mutableStateOf import androidx.compose.runtime.remember +import androidx.compose.runtime.rememberCoroutineScope import im.vector.app.features.analytics.plan.CryptoSessionStateChange import im.vector.app.features.analytics.plan.UserProperties import io.element.android.features.networkmonitor.api.NetworkMonitor @@ -41,6 +42,7 @@ import io.element.android.libraries.push.api.PushService import io.element.android.libraries.pushproviders.api.RegistrationFailure import io.element.android.services.analytics.api.AnalyticsService import kotlinx.coroutines.flow.map +import kotlinx.coroutines.launch import timber.log.Timber import javax.inject.Inject @@ -56,9 +58,13 @@ class LoggedInPresenter @Inject constructor( ) : Presenter { @Composable override fun present(): LoggedInState { + val coroutineScope = rememberCoroutineScope() val isVerified by remember { sessionVerificationService.sessionVerifiedStatus.map { it == SessionVerifiedStatus.Verified } }.collectAsState(initial = false) + val ignoreRegistrationError by remember { + pushService.ignoreRegistrationError(matrixClient.sessionId) + }.collectAsState(initial = false) val pusherRegistrationState = remember>> { mutableStateOf(AsyncData.Uninitialized) } if (isVerified) { LaunchedEffect(Unit) { @@ -84,13 +90,21 @@ class LoggedInPresenter @Inject constructor( fun handleEvent(event: LoggedInEvents) { when (event) { - LoggedInEvents.CloseErrorDialog -> pusherRegistrationState.value = AsyncData.Uninitialized + is LoggedInEvents.CloseErrorDialog -> { + pusherRegistrationState.value = AsyncData.Uninitialized + if (event.doNotShowAgain) { + coroutineScope.launch { + pushService.setIgnoreRegistrationError(matrixClient.sessionId, true) + } + } + } } } return LoggedInState( showSyncSpinner = showSyncSpinner, pusherRegistrationState = pusherRegistrationState.value, + ignoreRegistrationError = ignoreRegistrationError, eventSink = ::handleEvent ) } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt index fabaff786e..87f70511bc 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInState.kt @@ -21,5 +21,6 @@ import io.element.android.libraries.architecture.AsyncData data class LoggedInState( val showSyncSpinner: Boolean, val pusherRegistrationState: AsyncData, + val ignoreRegistrationError: Boolean, val eventSink: (LoggedInEvents) -> Unit, ) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt index bdb0aaacf0..d98271bef0 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt @@ -34,5 +34,6 @@ fun aLoggedInState( ) = LoggedInState( showSyncSpinner = showSyncSpinner, pusherRegistrationState = pusherRegistrationState, + ignoreRegistrationError = false, eventSink = {}, ) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt index b997bc0b42..8be46d5743 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt @@ -25,7 +25,7 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.res.stringResource import androidx.compose.ui.tooling.preview.PreviewParameter import io.element.android.libraries.architecture.AsyncData -import io.element.android.libraries.designsystem.components.dialogs.ErrorDialog +import io.element.android.libraries.designsystem.components.dialogs.ErrorDialogWithDoNotShowAgain import io.element.android.libraries.designsystem.preview.ElementPreview import io.element.android.libraries.designsystem.preview.PreviewsDayNight import io.element.android.libraries.matrix.api.exception.isNetworkError @@ -52,11 +52,12 @@ fun LoggedInView( is AsyncData.Success -> Unit is AsyncData.Failure -> { state.pusherRegistrationState.errorOrNull() + ?.takeIf { !state.ignoreRegistrationError } ?.getReason() ?.let { reason -> - ErrorDialog( + ErrorDialogWithDoNotShowAgain( content = stringResource(id = CommonStrings.common_error_registering_pusher_android, reason), - onDismiss = { state.eventSink(LoggedInEvents.CloseErrorDialog) }, + onDismiss = { state.eventSink(LoggedInEvents.CloseErrorDialog(it)) }, ) } } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index aa2f720343..191efd70ec 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -64,6 +64,7 @@ class LoggedInPresenterTest { val initialState = awaitItem() assertThat(initialState.showSyncSpinner).isFalse() assertThat(initialState.pusherRegistrationState.isUninitialized()).isTrue() + assertThat(initialState.ignoreRegistrationError).isFalse() skipItems(1) } } @@ -356,6 +357,12 @@ class LoggedInPresenterTest { .isInstanceOf(PusherRegistrationFailure.NoProvidersAvailable::class.java) lambda.assertions() .isNeverCalled() + // Reset the error and do not show again + finalState.eventSink(LoggedInEvents.CloseErrorDialog(doNotShowAgain = true)) + skipItems(1) + val lastState = awaitItem() + assertThat(lastState.pusherRegistrationState.isUninitialized()).isTrue() + assertThat(lastState.ignoreRegistrationError).isTrue() } } @@ -390,7 +397,7 @@ class LoggedInPresenterTest { lambda.assertions() .isNeverCalled() // Reset the error - finalState.eventSink(LoggedInEvents.CloseErrorDialog) + finalState.eventSink(LoggedInEvents.CloseErrorDialog(doNotShowAgain = false)) val lastState = awaitItem() assertThat(lastState.pusherRegistrationState.isUninitialized()).isTrue() } diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt index 9227eef364..effbe158d2 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt @@ -29,6 +29,7 @@ import io.element.android.libraries.core.coroutine.CoroutineDispatchers import io.element.android.libraries.di.ApplicationContext import io.element.android.libraries.di.SessionScope import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.push.api.PushService import kotlinx.coroutines.withContext import okhttp3.OkHttpClient import javax.inject.Inject @@ -47,6 +48,7 @@ class DefaultClearCacheUseCase @Inject constructor( private val okHttpClient: Provider, private val ftueService: FtueService, private val migrationScreenStore: MigrationScreenStore, + private val pushService: PushService, ) : ClearCacheUseCase { override suspend fun invoke() = withContext(coroutineDispatchers.io) { // Clear Matrix cache @@ -66,5 +68,7 @@ class DefaultClearCacheUseCase @Inject constructor( migrationScreenStore.reset() // Ensure the app is restarted defaultCacheIndexProvider.onClearedCache(matrixClient.sessionId) + // Ensure any error will be displayed again + pushService.setIgnoreRegistrationError(matrixClient.sessionId, false) } } diff --git a/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt b/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt index ce27acb7b3..7212f0534a 100644 --- a/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt +++ b/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt @@ -17,8 +17,10 @@ package io.element.android.libraries.push.api import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.pushproviders.api.Distributor import io.element.android.libraries.pushproviders.api.PushProvider +import kotlinx.coroutines.flow.Flow interface PushService { /** @@ -43,6 +45,9 @@ interface PushService { distributor: Distributor, ): Result + fun ignoreRegistrationError(sessionId: SessionId): Flow + suspend fun setIgnoreRegistrationError(sessionId: SessionId, ignore: Boolean) + /** * Return false in case of early error. */ diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt index 7600b695f1..b6efaa2c71 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt @@ -19,12 +19,14 @@ package io.element.android.libraries.push.impl import com.squareup.anvil.annotations.ContributesBinding import io.element.android.libraries.di.AppScope import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.push.api.GetCurrentPushProvider import io.element.android.libraries.push.api.PushService import io.element.android.libraries.push.impl.test.TestPush 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 kotlinx.coroutines.flow.Flow import timber.log.Timber import javax.inject.Inject @@ -72,6 +74,14 @@ class DefaultPushService @Inject constructor( return pushProvider.registerWith(matrixClient, distributor) } + override fun ignoreRegistrationError(sessionId: SessionId): Flow { + return userPushStoreFactory.getOrCreate(sessionId).ignoreRegistrationError() + } + + override suspend fun setIgnoreRegistrationError(sessionId: SessionId, ignore: Boolean) { + userPushStoreFactory.getOrCreate(sessionId).setIgnoreRegistrationError(ignore) + } + override suspend fun testPush(): Boolean { val pushProvider = getCurrentPushProvider() ?: return false val config = pushProvider.getCurrentUserPushConfig() ?: return false diff --git a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt index 0bf2da54ad..d9e5f0d2b6 100644 --- a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt +++ b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt @@ -17,10 +17,13 @@ package io.element.android.libraries.push.test import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.push.api.PushService import io.element.android.libraries.pushproviders.api.Distributor import io.element.android.libraries.pushproviders.api.PushProvider import io.element.android.tests.testutils.simulateLongTask +import kotlinx.coroutines.flow.Flow +import kotlinx.coroutines.flow.MutableStateFlow class FakePushService( private val testPushBlock: suspend () -> Boolean = { true }, @@ -53,6 +56,16 @@ class FakePushService( } } + private val ignoreRegistrationError = MutableStateFlow(false) + + override fun ignoreRegistrationError(sessionId: SessionId): Flow { + return ignoreRegistrationError + } + + override suspend fun setIgnoreRegistrationError(sessionId: SessionId, ignore: Boolean) { + ignoreRegistrationError.value = ignore + } + override suspend fun testPush(): Boolean = simulateLongTask { testPushBlock() } diff --git a/libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/UserPushStore.kt b/libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/UserPushStore.kt index d24cc5ff5e..3828ffba2b 100644 --- a/libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/UserPushStore.kt +++ b/libraries/pushstore/api/src/main/kotlin/io/element/android/libraries/pushstore/api/UserPushStore.kt @@ -29,6 +29,9 @@ interface UserPushStore { fun getNotificationEnabledForDevice(): Flow suspend fun setNotificationEnabledForDevice(enabled: Boolean) + fun ignoreRegistrationError(): Flow + suspend fun setIgnoreRegistrationError(ignore: Boolean) + /** * Return true if Pin code is disabled, or if user set the settings to see full notification content. */ diff --git a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt index cfcc9e3da4..8dc9ef9d59 100644 --- a/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt +++ b/libraries/pushstore/impl/src/main/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStore.kt @@ -25,6 +25,7 @@ import androidx.datastore.preferences.core.stringPreferencesKey import androidx.datastore.preferences.preferencesDataStore import androidx.datastore.preferences.preferencesDataStoreFile import io.element.android.libraries.androidutils.hash.hash +import io.element.android.libraries.core.bool.orFalse import io.element.android.libraries.core.bool.orTrue import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.pushstore.api.UserPushStore @@ -61,6 +62,7 @@ class UserPushStoreDataStore( private val pushProviderName = stringPreferencesKey("pushProviderName") private val currentPushKey = stringPreferencesKey("currentPushKey") private val notificationEnabled = booleanPreferencesKey("notificationEnabled") + private val ignoreRegistrationError = booleanPreferencesKey("ignoreRegistrationError") override suspend fun getPushProviderName(): String? { return context.dataStore.data.first()[pushProviderName] @@ -100,6 +102,16 @@ class UserPushStoreDataStore( return true } + override fun ignoreRegistrationError(): Flow { + return context.dataStore.data.map { it[ignoreRegistrationError].orFalse() } + } + + override suspend fun setIgnoreRegistrationError(ignore: Boolean) { + context.dataStore.edit { + it[ignoreRegistrationError] = ignore + } + } + override suspend fun reset() { context.dataStore.edit { it.clear() diff --git a/libraries/pushstore/test/src/main/kotlin/io/element/android/libraries/pushstore/test/userpushstore/FakeUserPushStore.kt b/libraries/pushstore/test/src/main/kotlin/io/element/android/libraries/pushstore/test/userpushstore/FakeUserPushStore.kt index 112a752368..35dc83040c 100644 --- a/libraries/pushstore/test/src/main/kotlin/io/element/android/libraries/pushstore/test/userpushstore/FakeUserPushStore.kt +++ b/libraries/pushstore/test/src/main/kotlin/io/element/android/libraries/pushstore/test/userpushstore/FakeUserPushStore.kt @@ -25,6 +25,7 @@ class FakeUserPushStore( ) : UserPushStore { private var currentRegisteredPushKey: String? = null private val notificationEnabledForDevice = MutableStateFlow(true) + private val ignoreRegistrationError = MutableStateFlow(false) override suspend fun getPushProviderName(): String? { return pushProviderName } @@ -53,6 +54,14 @@ class FakeUserPushStore( return true } + override fun ignoreRegistrationError(): Flow { + return ignoreRegistrationError + } + + override suspend fun setIgnoreRegistrationError(ignore: Boolean) { + ignoreRegistrationError.value = ignore + } + override suspend fun reset() { } } From 5fa3802373e51866eb7894ee64bcc80156f7efd0 Mon Sep 17 00:00:00 2001 From: ElementBot Date: Mon, 17 Jun 2024 11:22:16 +0000 Subject: [PATCH 12/29] Update screenshots --- ...InView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png | 3 +++ ...View_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png | 3 +++ ...rrorDialogWithDoNotShowAgain-Day_0_null,NEXUS_5,1.0,en].png | 3 +++ ...orDialogWithDoNotShowAgain-Night_1_null,NEXUS_5,1.0,en].png | 3 +++ 4 files changed, 12 insertions(+) create mode 100644 tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png create mode 100644 tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png create mode 100644 tests/uitests/src/test/snapshots/images/ui_S_t[l.designsystem.components.dialogs_ErrorDialogWithDoNotShowAgain_null_ErrorDialogWithDoNotShowAgain-Day_0_null,NEXUS_5,1.0,en].png create mode 100644 tests/uitests/src/test/snapshots/images/ui_S_t[l.designsystem.components.dialogs_ErrorDialogWithDoNotShowAgain_null_ErrorDialogWithDoNotShowAgain-Night_1_null,NEXUS_5,1.0,en].png diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..33aced7342 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:9761d7e583bb275c40e42c1c1051b435d7445f84e825863ab42f50922d7841b1 +size 33908 diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..d37591da85 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:8a9103631e24ed4869d3bd5c7e04851e6ef5eb327daaf00de8a5e5c9e770ccf4 +size 31829 diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[l.designsystem.components.dialogs_ErrorDialogWithDoNotShowAgain_null_ErrorDialogWithDoNotShowAgain-Day_0_null,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[l.designsystem.components.dialogs_ErrorDialogWithDoNotShowAgain_null_ErrorDialogWithDoNotShowAgain-Day_0_null,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..36a82272a0 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[l.designsystem.components.dialogs_ErrorDialogWithDoNotShowAgain_null_ErrorDialogWithDoNotShowAgain-Day_0_null,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:5aab6b3399b7826b3d1081474c6c0647bcdec933caef6bf563ea3f90d2c99bd1 +size 13688 diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[l.designsystem.components.dialogs_ErrorDialogWithDoNotShowAgain_null_ErrorDialogWithDoNotShowAgain-Night_1_null,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[l.designsystem.components.dialogs_ErrorDialogWithDoNotShowAgain_null_ErrorDialogWithDoNotShowAgain-Night_1_null,NEXUS_5,1.0,en].png new file mode 100644 index 0000000000..5cca281194 --- /dev/null +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[l.designsystem.components.dialogs_ErrorDialogWithDoNotShowAgain_null_ErrorDialogWithDoNotShowAgain-Night_1_null,NEXUS_5,1.0,en].png @@ -0,0 +1,3 @@ +version https://git-lfs.github.com/spec/v1 +oid sha256:3f232b2d478fbbb30f50333514d8678f9eec114e2119ef64e3429887a12b452b +size 12114 From abc0e7edf7657edb45a064302c0bbd78dc624562 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 14:19:28 +0200 Subject: [PATCH 13/29] Move setIgnoreRegistrationError call. --- .../features/preferences/impl/tasks/ClearCacheUseCase.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt index effbe158d2..33fbab8f39 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt @@ -66,9 +66,9 @@ class DefaultClearCacheUseCase @Inject constructor( ftueService.reset() // Clear migration screen store migrationScreenStore.reset() - // Ensure the app is restarted - defaultCacheIndexProvider.onClearedCache(matrixClient.sessionId) // Ensure any error will be displayed again pushService.setIgnoreRegistrationError(matrixClient.sessionId, false) + // Ensure the app is restarted + defaultCacheIndexProvider.onClearedCache(matrixClient.sessionId) } } From 7d1e841c6887fbaef38efab9591041cf51783bd8 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 14:20:14 +0200 Subject: [PATCH 14/29] Rename member. --- .../features/preferences/impl/tasks/ClearCacheUseCase.kt | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt index 33fbab8f39..e4edb572d1 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/tasks/ClearCacheUseCase.kt @@ -44,7 +44,7 @@ class DefaultClearCacheUseCase @Inject constructor( @ApplicationContext private val context: Context, private val matrixClient: MatrixClient, private val coroutineDispatchers: CoroutineDispatchers, - private val defaultCacheIndexProvider: DefaultCacheService, + private val defaultCacheService: DefaultCacheService, private val okHttpClient: Provider, private val ftueService: FtueService, private val migrationScreenStore: MigrationScreenStore, @@ -69,6 +69,6 @@ class DefaultClearCacheUseCase @Inject constructor( // Ensure any error will be displayed again pushService.setIgnoreRegistrationError(matrixClient.sessionId, false) // Ensure the app is restarted - defaultCacheIndexProvider.onClearedCache(matrixClient.sessionId) + defaultCacheService.onClearedCache(matrixClient.sessionId) } } From eb07ae28558000b3addf7a4cc0e9fe94d26149c2 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 14:24:55 +0200 Subject: [PATCH 15/29] Add test on `ignoreRegistrationError` and `setIgnoreRegistrationError` --- .../push/impl/DefaultPushServiceTest.kt | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/DefaultPushServiceTest.kt b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/DefaultPushServiceTest.kt index 546d69b008..f253030a74 100644 --- a/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/DefaultPushServiceTest.kt +++ b/libraries/push/impl/src/test/kotlin/io/element/android/libraries/push/impl/DefaultPushServiceTest.kt @@ -19,6 +19,7 @@ package io.element.android.libraries.push.impl import com.google.common.truth.Truth.assertThat import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.matrix.test.AN_EXCEPTION +import io.element.android.libraries.matrix.test.A_SESSION_ID import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.push.api.GetCurrentPushProvider import io.element.android.libraries.push.impl.test.FakeTestPush @@ -33,6 +34,7 @@ import io.element.android.libraries.pushstore.test.userpushstore.FakeUserPushSto import io.element.android.libraries.pushstore.test.userpushstore.FakeUserPushStoreFactory import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value +import kotlinx.coroutines.flow.first import kotlinx.coroutines.test.runTest import org.junit.Test @@ -205,6 +207,20 @@ class DefaultPushServiceTest { assertThat(result).containsExactly(aPushProvider1, aPushProvider2, aPushProvider3).inOrder() } + @Test + fun `test setIgnoreRegistrationError is sent to the store`() = runTest { + val userPushStore = FakeUserPushStore().apply { + } + val defaultPushService = createDefaultPushService( + userPushStoreFactory = FakeUserPushStoreFactory( + userPushStore = { userPushStore }, + ), + ) + assertThat(defaultPushService.ignoreRegistrationError(A_SESSION_ID).first()).isFalse() + defaultPushService.setIgnoreRegistrationError(A_SESSION_ID, true) + assertThat(defaultPushService.ignoreRegistrationError(A_SESSION_ID).first()).isTrue() + } + private fun createDefaultPushService( testPush: TestPush = FakeTestPush(), userPushStoreFactory: UserPushStoreFactory = FakeUserPushStoreFactory(), From e31fc17c9a782a641ccfd1f06d84a25a7aebb713 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 14:46:35 +0200 Subject: [PATCH 16/29] Add Unit test on UserPushStoreDataStore --- libraries/pushstore/impl/build.gradle.kts | 1 + .../impl/UserPushStoreDataStoreTest.kt | 105 ++++++++++++++++++ 2 files changed, 106 insertions(+) create mode 100644 libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStoreTest.kt diff --git a/libraries/pushstore/impl/build.gradle.kts b/libraries/pushstore/impl/build.gradle.kts index 69f0f21e55..c89c662b3b 100644 --- a/libraries/pushstore/impl/build.gradle.kts +++ b/libraries/pushstore/impl/build.gradle.kts @@ -44,6 +44,7 @@ dependencies { testImplementation(libs.test.junit) testImplementation(libs.test.mockk) + testImplementation(libs.test.robolectric) testImplementation(libs.test.truth) testImplementation(libs.test.turbine) testImplementation(libs.coroutines.test) diff --git a/libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStoreTest.kt b/libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStoreTest.kt new file mode 100644 index 0000000000..7a4cad1387 --- /dev/null +++ b/libraries/pushstore/impl/src/test/kotlin/io/element/android/libraries/pushstore/impl/UserPushStoreDataStoreTest.kt @@ -0,0 +1,105 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.pushstore.impl + +import androidx.test.platform.app.InstrumentationRegistry +import com.google.common.truth.Truth.assertThat +import io.element.android.libraries.matrix.api.core.SessionId +import io.element.android.libraries.matrix.test.A_SESSION_ID +import io.element.android.libraries.matrix.test.A_SESSION_ID_2 +import kotlinx.coroutines.flow.first +import kotlinx.coroutines.test.runTest +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +@RunWith(RobolectricTestRunner::class) +class UserPushStoreDataStoreTest { + @Test + fun `test getPushProviderName`() = runTest { + val sut = createUserPushStoreDataStore() + assertThat(sut.getPushProviderName()).isNull() + sut.setPushProviderName("name") + assertThat(sut.getPushProviderName()).isEqualTo("name") + } + + @Test + fun `test getCurrentRegisteredPushKey`() = runTest { + val sut = createUserPushStoreDataStore() + assertThat(sut.getCurrentRegisteredPushKey()).isNull() + sut.setCurrentRegisteredPushKey("aKey") + assertThat(sut.getCurrentRegisteredPushKey()).isEqualTo("aKey") + sut.setCurrentRegisteredPushKey(null) + assertThat(sut.getCurrentRegisteredPushKey()).isNull() + } + + @Test + fun `test getNotificationEnabledForDevice`() = runTest { + val sut = createUserPushStoreDataStore() + assertThat(sut.getNotificationEnabledForDevice().first()).isTrue() + sut.setNotificationEnabledForDevice(false) + assertThat(sut.getNotificationEnabledForDevice().first()).isFalse() + sut.setNotificationEnabledForDevice(true) + assertThat(sut.getNotificationEnabledForDevice().first()).isTrue() + } + + @Test + fun `test useCompleteNotificationFormat`() = runTest { + val sut = createUserPushStoreDataStore() + assertThat(sut.useCompleteNotificationFormat()).isTrue() + } + + @Test + fun `test ignoreRegistrationError`() = runTest { + val sut = createUserPushStoreDataStore() + assertThat(sut.ignoreRegistrationError().first()).isFalse() + sut.setIgnoreRegistrationError(true) + assertThat(sut.ignoreRegistrationError().first()).isTrue() + sut.setIgnoreRegistrationError(false) + assertThat(sut.ignoreRegistrationError().first()).isFalse() + } + + @Test + fun `test reset`() = runTest { + val sut = createUserPushStoreDataStore() + sut.setPushProviderName("name") + sut.setCurrentRegisteredPushKey("aKey") + sut.setNotificationEnabledForDevice(false) + sut.setIgnoreRegistrationError(true) + sut.reset() + assertThat(sut.getPushProviderName()).isNull() + assertThat(sut.getCurrentRegisteredPushKey()).isNull() + assertThat(sut.getNotificationEnabledForDevice().first()).isTrue() + assertThat(sut.ignoreRegistrationError().first()).isFalse() + } + + @Test + fun `ensure a store is created per session`() = runTest { + val sut1 = createUserPushStoreDataStore() + sut1.setPushProviderName("name") + val sut2 = createUserPushStoreDataStore(A_SESSION_ID_2) + assertThat(sut1.getPushProviderName()).isEqualTo("name") + assertThat(sut2.getPushProviderName()).isNull() + } + + private fun createUserPushStoreDataStore( + sessionId: SessionId = A_SESSION_ID, + ) = UserPushStoreDataStore( + context = InstrumentationRegistry.getInstrumentation().context, + userId = sessionId, + ) +} From 5180ce388c2bb3d424f940f3a067e8aad68c2a98 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 16:20:23 +0200 Subject: [PATCH 17/29] Add a shortcut to navigate to the notification settings in case of error. --- .../io/element/android/appnav/LoggedInFlowNode.kt | 7 ++++++- .../element/android/appnav/loggedin/LoggedInNode.kt | 12 ++++++++++++ .../element/android/appnav/loggedin/LoggedInView.kt | 13 +++++++++++-- .../preferences/api/PreferencesEntryPoint.kt | 3 +++ .../impl/DefaultPreferencesEntryPoint.kt | 1 + .../dialogs/ErrorDialogWithDoNotShowAgain.kt | 4 ++++ 6 files changed, 37 insertions(+), 3 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt index 219ec2167a..cc50e19895 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/LoggedInFlowNode.kt @@ -238,7 +238,12 @@ class LoggedInFlowNode @AssistedInject constructor( return when (navTarget) { NavTarget.Placeholder -> createNode(buildContext) NavTarget.LoggedInPermanent -> { - createNode(buildContext) + val callback = object : LoggedInNode.Callback { + override fun navigateToNotificationTroubleshoot() { + backstack.push(NavTarget.Settings(PreferencesEntryPoint.InitialTarget.NotificationTroubleshoot)) + } + } + createNode(buildContext, listOf(callback)) } NavTarget.RoomList -> { val callback = object : RoomListEntryPoint.Callback { diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInNode.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInNode.kt index a61e6b69b6..a54a189f69 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInNode.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInNode.kt @@ -21,6 +21,7 @@ import androidx.compose.ui.Modifier import com.bumble.appyx.core.modality.BuildContext import com.bumble.appyx.core.node.Node import com.bumble.appyx.core.plugin.Plugin +import com.bumble.appyx.core.plugin.plugins import dagger.assisted.Assisted import dagger.assisted.AssistedInject import io.element.android.anvilannotations.ContributesNode @@ -35,11 +36,22 @@ class LoggedInNode @AssistedInject constructor( buildContext = buildContext, plugins = plugins ) { + interface Callback : Plugin { + fun navigateToNotificationTroubleshoot() + } + + private fun navigateToNotificationTroubleshoot() { + plugins().forEach { + it.navigateToNotificationTroubleshoot() + } + } + @Composable override fun View(modifier: Modifier) { val loggedInState = loggedInPresenter.present() LoggedInView( state = loggedInState, + navigateToNotificationTroubleshoot = ::navigateToNotificationTroubleshoot, modifier = modifier ) } diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt index 8be46d5743..b619d50501 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInView.kt @@ -34,6 +34,7 @@ import io.element.android.libraries.ui.strings.CommonStrings @Composable fun LoggedInView( state: LoggedInState, + navigateToNotificationTroubleshoot: () -> Unit, modifier: Modifier = Modifier ) { Box( @@ -57,7 +58,14 @@ fun LoggedInView( ?.let { reason -> ErrorDialogWithDoNotShowAgain( content = stringResource(id = CommonStrings.common_error_registering_pusher_android, reason), - onDismiss = { state.eventSink(LoggedInEvents.CloseErrorDialog(it)) }, + cancelText = stringResource(id = CommonStrings.common_settings), + onDismiss = { + state.eventSink(LoggedInEvents.CloseErrorDialog(it)) + }, + onCancel = { + state.eventSink(LoggedInEvents.CloseErrorDialog(false)) + navigateToNotificationTroubleshoot() + } ) } } @@ -85,6 +93,7 @@ private fun Throwable.getReason(): String? { @Composable internal fun LoggedInViewPreview(@PreviewParameter(LoggedInStateProvider::class) state: LoggedInState) = ElementPreview { LoggedInView( - state = state + state = state, + navigateToNotificationTroubleshoot = {}, ) } diff --git a/features/preferences/api/src/main/kotlin/io/element/android/features/preferences/api/PreferencesEntryPoint.kt b/features/preferences/api/src/main/kotlin/io/element/android/features/preferences/api/PreferencesEntryPoint.kt index 0453fa0ce2..ef905c5173 100644 --- a/features/preferences/api/src/main/kotlin/io/element/android/features/preferences/api/PreferencesEntryPoint.kt +++ b/features/preferences/api/src/main/kotlin/io/element/android/features/preferences/api/PreferencesEntryPoint.kt @@ -32,6 +32,9 @@ interface PreferencesEntryPoint : FeatureEntryPoint { @Parcelize data object NotificationSettings : InitialTarget + + @Parcelize + data object NotificationTroubleshoot : InitialTarget } data class Params(val initialElement: InitialTarget) : NodeInputs diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/DefaultPreferencesEntryPoint.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/DefaultPreferencesEntryPoint.kt index e551d9d8dc..fd417d4934 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/DefaultPreferencesEntryPoint.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/DefaultPreferencesEntryPoint.kt @@ -51,4 +51,5 @@ class DefaultPreferencesEntryPoint @Inject constructor() : PreferencesEntryPoint internal fun PreferencesEntryPoint.InitialTarget.toNavTarget() = when (this) { is PreferencesEntryPoint.InitialTarget.Root -> PreferencesFlowNode.NavTarget.Root is PreferencesEntryPoint.InitialTarget.NotificationSettings -> PreferencesFlowNode.NavTarget.NotificationSettings + PreferencesEntryPoint.InitialTarget.NotificationTroubleshoot -> PreferencesFlowNode.NavTarget.TroubleshootNotifications } diff --git a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/dialogs/ErrorDialogWithDoNotShowAgain.kt b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/dialogs/ErrorDialogWithDoNotShowAgain.kt index 671e35c926..928124b08d 100644 --- a/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/dialogs/ErrorDialogWithDoNotShowAgain.kt +++ b/libraries/designsystem/src/main/kotlin/io/element/android/libraries/designsystem/components/dialogs/ErrorDialogWithDoNotShowAgain.kt @@ -47,6 +47,8 @@ fun ErrorDialogWithDoNotShowAgain( modifier: Modifier = Modifier, title: String = ErrorDialogDefaults.title, submitText: String = ErrorDialogDefaults.submitText, + cancelText: String? = null, + onCancel: () -> Unit = {}, ) { var doNotShowAgain by remember { mutableStateOf(false) } BasicAlertDialog( @@ -56,7 +58,9 @@ fun ErrorDialogWithDoNotShowAgain( SimpleAlertDialogContent( title = title, submitText = submitText, + cancelText = cancelText, onSubmitClick = { onDismiss(doNotShowAgain) }, + onCancelClick = onCancel, ) { Column { Text( From 1f8b5255486c7ba7f1b05088218eb140e6e5647f Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 16:36:02 +0200 Subject: [PATCH 18/29] Fix back navigation issue, when opening directly the notification troubleshoot screen. --- .../preferences/impl/PreferencesFlowNode.kt | 7 +++++- .../architecture/appyx/BackStackExt.kt | 25 +++++++++++++++++++ 2 files changed, 31 insertions(+), 1 deletion(-) create mode 100644 libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/appyx/BackStackExt.kt diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/PreferencesFlowNode.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/PreferencesFlowNode.kt index 8fef6053a4..6d3661aace 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/PreferencesFlowNode.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/PreferencesFlowNode.kt @@ -44,6 +44,7 @@ import io.element.android.features.preferences.impl.root.PreferencesRootNode import io.element.android.features.preferences.impl.user.editprofile.EditUserProfileNode import io.element.android.libraries.architecture.BackstackView import io.element.android.libraries.architecture.BaseFlowNode +import io.element.android.libraries.architecture.appyx.canPop import io.element.android.libraries.architecture.createNode import io.element.android.libraries.di.SessionScope import io.element.android.libraries.matrix.api.core.RoomId @@ -190,7 +191,11 @@ class PreferencesFlowNode @AssistedInject constructor( notificationTroubleShootEntryPoint.nodeBuilder(this, buildContext) .callback(object : NotificationTroubleShootEntryPoint.Callback { override fun onDone() { - backstack.pop() + if (backstack.canPop()) { + backstack.pop() + } else { + navigateUp() + } } }) .build() diff --git a/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/appyx/BackStackExt.kt b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/appyx/BackStackExt.kt new file mode 100644 index 0000000000..27d106eb66 --- /dev/null +++ b/libraries/architecture/src/main/kotlin/io/element/android/libraries/architecture/appyx/BackStackExt.kt @@ -0,0 +1,25 @@ +/* + * Copyright (c) 2024 New Vector Ltd + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package io.element.android.libraries.architecture.appyx + +import com.bumble.appyx.navmodel.backstack.BackStack + +fun BackStack.canPop(): Boolean { + val elements = elements.value + return elements.any { it.targetState == BackStack.State.ACTIVE } && + elements.any { it.targetState == BackStack.State.STASHED } +} From f72fc36de8c3f72647ebcb1143fac6b86f2217f3 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 17:22:11 +0200 Subject: [PATCH 19/29] Update PushProvider API, remove `isAvailable()`, but instead rely on `getDistributors()` to eventually return an empty list of Distributors. --- .../android/libraries/push/api/PushService.kt | 3 +- .../libraries/push/impl/DefaultPushService.kt | 1 - .../pushproviders/api/PushProvider.kt | 4 +-- .../firebase/FirebasePushProvider.kt | 8 ++--- .../firebase/FirebasePushProviderTest.kt | 31 ++++++++----------- .../pushproviders/test/FakePushProvider.kt | 2 -- .../unifiedpush/UnifiedPushProvider.kt | 11 ------- .../unifiedpush/UnifiedPushProviderTest.kt | 2 -- 8 files changed, 18 insertions(+), 44 deletions(-) diff --git a/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt b/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt index 7212f0534a..af21bd1168 100644 --- a/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt +++ b/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt @@ -29,8 +29,7 @@ interface PushService { suspend fun getCurrentPushProvider(): PushProvider? /** - * Return the list of push providers, available at compile time, and - * available at runtime, sorted by index. + * Return the list of push providers, available at compile time, sorted by index. */ fun getAvailablePushProviders(): List diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt index b6efaa2c71..46105a68db 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt @@ -44,7 +44,6 @@ class DefaultPushService @Inject constructor( override fun getAvailablePushProviders(): List { return pushProviders - .filter { it.isAvailable() } .sortedBy { it.index } } diff --git a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushProvider.kt b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushProvider.kt index d111dc139f..763c4fa88a 100644 --- a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushProvider.kt +++ b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/PushProvider.kt @@ -33,10 +33,8 @@ interface PushProvider { val name: String /** - * Return true if the push provider is available on this device. + * Return the list of available distributors. */ - fun isAvailable(): Boolean - fun getDistributors(): List /** diff --git a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt index 0228f8f74b..f12de0f4d2 100644 --- a/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt +++ b/libraries/pushproviders/firebase/src/main/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProvider.kt @@ -38,12 +38,10 @@ class FirebasePushProvider @Inject constructor( override val index = FirebaseConfig.INDEX override val name = FirebaseConfig.NAME - override fun isAvailable(): Boolean { - return isPlayServiceAvailable.isAvailable() - } - override fun getDistributors(): List { - return listOf(firebaseDistributor) + return listOfNotNull( + firebaseDistributor.takeIf { isPlayServiceAvailable.isAvailable() } + ) } override suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor): Result { diff --git a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt index 880be2f053..75e77d9fd9 100644 --- a/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt +++ b/libraries/pushproviders/firebase/src/test/kotlin/io/element/android/libraries/pushproviders/firebase/FirebasePushProviderTest.kt @@ -38,12 +38,23 @@ class FirebasePushProviderTest { } @Test - fun `getDistributors return the unique distributor`() { - val firebasePushProvider = createFirebasePushProvider() + fun `getDistributors return the unique distributor if available`() { + val firebasePushProvider = createFirebasePushProvider( + isPlayServiceAvailable = FakeIsPlayServiceAvailable(isAvailable = true) + ) val result = firebasePushProvider.getDistributors() assertThat(result).containsExactly(Distributor("Firebase", "Firebase")) } + @Test + fun `getDistributors return empty list if service is not available`() { + val firebasePushProvider = createFirebasePushProvider( + isPlayServiceAvailable = FakeIsPlayServiceAvailable(isAvailable = false) + ) + val result = firebasePushProvider.getDistributors() + assertThat(result).isEmpty() + } + @Test fun `getCurrentDistributor always return the unique distributor`() = runTest { val firebasePushProvider = createFirebasePushProvider() @@ -51,22 +62,6 @@ class FirebasePushProviderTest { assertThat(result).isEqualTo(Distributor("Firebase", "Firebase")) } - @Test - fun `isAvailable true`() { - val firebasePushProvider = createFirebasePushProvider( - isPlayServiceAvailable = FakeIsPlayServiceAvailable(isAvailable = true) - ) - assertThat(firebasePushProvider.isAvailable()).isTrue() - } - - @Test - fun `isAvailable false`() { - val firebasePushProvider = createFirebasePushProvider( - isPlayServiceAvailable = FakeIsPlayServiceAvailable(isAvailable = false) - ) - assertThat(firebasePushProvider.isAvailable()).isFalse() - } - @Test fun `register ok`() = runTest { val matrixClient = FakeMatrixClient() diff --git a/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt b/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt index 4f83cde911..cd68568d66 100644 --- a/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt +++ b/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt @@ -25,14 +25,12 @@ import io.element.android.tests.testutils.lambda.lambdaError class FakePushProvider( override val index: Int = 0, override val name: String = "aFakePushProvider", - private val isAvailable: Boolean = true, private val distributors: List = listOf(Distributor("aDistributorValue", "aDistributorName")), private val currentDistributor: () -> Distributor? = { distributors.firstOrNull() }, private val currentUserPushConfig: CurrentUserPushConfig? = null, private val registerWithResult: (MatrixClient, Distributor) -> Result = { _, _ -> lambdaError() }, private val unregisterWithResult: (MatrixClient) -> Result = { lambdaError() }, ) : PushProvider { - override fun isAvailable(): Boolean = isAvailable override fun getDistributors(): List = distributors diff --git a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt index 4530a4667f..f302a6016f 100644 --- a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt +++ b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt @@ -43,17 +43,6 @@ class UnifiedPushProvider @Inject constructor( override val index = UnifiedPushConfig.INDEX override val name = UnifiedPushConfig.NAME - override fun isAvailable(): Boolean { - val isAvailable = getDistributors().isNotEmpty() - return if (isAvailable) { - Timber.tag(loggerTag.value).d("UnifiedPush is available") - true - } else { - Timber.tag(loggerTag.value).w("UnifiedPush is not available") - false - } - } - override fun getDistributors(): List { return unifiedPushDistributorProvider.getDistributors() } diff --git a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProviderTest.kt b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProviderTest.kt index 826f08a1b0..0e3bf1655c 100644 --- a/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProviderTest.kt +++ b/libraries/pushproviders/unifiedpush/src/test/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProviderTest.kt @@ -58,7 +58,6 @@ class UnifiedPushProviderTest { ) val result = unifiedPushProvider.getDistributors() assertThat(result).containsExactly(Distributor("value", "Name")) - assertThat(unifiedPushProvider.isAvailable()).isTrue() } @Test @@ -70,7 +69,6 @@ class UnifiedPushProviderTest { ) val result = unifiedPushProvider.getDistributors() assertThat(result).isEmpty() - assertThat(unifiedPushProvider.isAvailable()).isFalse() } @Test From b4b407a69eb3cf2a79e56ab45d716f04bfe5be84 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 17:36:39 +0200 Subject: [PATCH 20/29] Store the first provider even if no distributor is available, else error in troubleshoot test will not be accurate. Also when registering for the first time, pick the fist available provider with at least one distributor. --- .../android/appnav/loggedin/LoggedInPresenter.kt | 11 +++++++++-- .../element/android/libraries/push/api/PushService.kt | 9 +++++++++ .../android/libraries/push/impl/DefaultPushService.kt | 9 +++++++++ .../android/libraries/push/test/FakePushService.kt | 6 ++++++ 4 files changed, 33 insertions(+), 2 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index e70a44aa2b..e780770b8b 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -113,14 +113,21 @@ class LoggedInPresenter @Inject constructor( Timber.tag(pusherTag.value).d("Ensure pusher is registered") val currentPushProvider = pushService.getCurrentPushProvider() val result = if (currentPushProvider == null) { - Timber.tag(pusherTag.value).d("Register with the first available push provider") - val pushProvider = pushService.getAvailablePushProviders().firstOrNull() + Timber.tag(pusherTag.value).d("Register with the first available push provider with at least one distributor") + val pushProvider = pushService.getAvailablePushProviders() + .firstOrNull { it.getDistributors().isNotEmpty() } + // Else fallback to the first available push provider (the list should never be empty) + ?: pushService.getAvailablePushProviders().firstOrNull() ?: return Unit .also { Timber.tag(pusherTag.value).w("No push providers available") } .also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoProvidersAvailable()) } val distributor = pushProvider.getDistributors().firstOrNull() ?: return Unit .also { Timber.tag(pusherTag.value).w("No distributors available") } + .also { + // In this case, consider the push provider is chosen. + pushService.selectPushProvider(matrixClient, pushProvider) + } .also { pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.NoDistributorsAvailable()) } pushService.registerWith(matrixClient, pushProvider, distributor) } else { diff --git a/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt b/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt index af21bd1168..e48ba81d2a 100644 --- a/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt +++ b/libraries/push/api/src/main/kotlin/io/element/android/libraries/push/api/PushService.kt @@ -44,6 +44,15 @@ interface PushService { distributor: Distributor, ): Result + /** + * Store the given push provider as the current one, but do not register. + * To be used when there is no distributor available. + */ + suspend fun selectPushProvider( + matrixClient: MatrixClient, + pushProvider: PushProvider, + ) + fun ignoreRegistrationError(sessionId: SessionId): Flow suspend fun setIgnoreRegistrationError(sessionId: SessionId, ignore: Boolean) diff --git a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt index 46105a68db..fa2f1977f4 100644 --- a/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt +++ b/libraries/push/impl/src/main/kotlin/io/element/android/libraries/push/impl/DefaultPushService.kt @@ -73,6 +73,15 @@ class DefaultPushService @Inject constructor( return pushProvider.registerWith(matrixClient, distributor) } + override suspend fun selectPushProvider( + matrixClient: MatrixClient, + pushProvider: PushProvider, + ) { + Timber.d("Select ${pushProvider.name}") + val userPushStore = userPushStoreFactory.getOrCreate(matrixClient.sessionId) + userPushStore.setPushProviderName(pushProvider.name) + } + override fun ignoreRegistrationError(sessionId: SessionId): Flow { return userPushStoreFactory.getOrCreate(sessionId).ignoreRegistrationError() } diff --git a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt index d9e5f0d2b6..8f3e45c4ce 100644 --- a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt +++ b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt @@ -21,6 +21,7 @@ import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.push.api.PushService import io.element.android.libraries.pushproviders.api.Distributor import io.element.android.libraries.pushproviders.api.PushProvider +import io.element.android.tests.testutils.lambda.lambdaError import io.element.android.tests.testutils.simulateLongTask import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow @@ -32,6 +33,7 @@ class FakePushService( Result.success(Unit) }, private val currentPushProvider: () -> PushProvider? = { availablePushProviders.firstOrNull() }, + private val selectPushProviderLambda: suspend (MatrixClient, PushProvider) -> Unit = { _, _ -> lambdaError() } ) : PushService { override suspend fun getCurrentPushProvider(): PushProvider? { return registeredPushProvider ?: currentPushProvider() @@ -56,6 +58,10 @@ class FakePushService( } } + override suspend fun selectPushProvider(matrixClient: MatrixClient, pushProvider: PushProvider) { + selectPushProviderLambda(matrixClient, pushProvider) + } + private val ignoreRegistrationError = MutableStateFlow(false) override fun ignoreRegistrationError(sessionId: SessionId): Flow { From e12f723ff7204c27a2eddae6bb511d08b12d406a Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 17:38:10 +0200 Subject: [PATCH 21/29] Fix test compilation issue. --- .../android/appnav/loggedin/LoggedInPresenterTest.kt | 6 ------ .../impl/notifications/NotificationSettingsPresenterTest.kt | 2 -- 2 files changed, 8 deletions(-) diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index 191efd70ec..4fa03fdbdd 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -218,7 +218,6 @@ class LoggedInPresenterTest { val pushProvider = FakePushProvider( index = 1, name = "aFakePushProvider0", - isAvailable = true, distributors = listOf( Distributor("aDistributorValue0", "aDistributorName0"), distributor, @@ -263,7 +262,6 @@ class LoggedInPresenterTest { val pushProvider = FakePushProvider( index = 0, name = "aFakePushProvider0", - isAvailable = true, distributors = listOf( Distributor("aDistributorValue0", "aDistributorName0"), Distributor("aDistributorValue1", "aDistributorName1"), @@ -308,7 +306,6 @@ class LoggedInPresenterTest { val pushProvider = FakePushProvider( index = 0, name = "aFakePushProvider0", - isAvailable = true, distributors = emptyList(), ) val pushService = createFakePushService( @@ -377,7 +374,6 @@ class LoggedInPresenterTest { pushProvider0 = FakePushProvider( index = 1, name = "aFakePushProvider1", - isAvailable = true, distributors = emptyList(), ), pushProvider1 = null, @@ -407,14 +403,12 @@ class LoggedInPresenterTest { pushProvider0: PushProvider? = FakePushProvider( index = 0, name = "aFakePushProvider0", - isAvailable = true, distributors = listOf(Distributor("aDistributorValue0", "aDistributorName0")), currentDistributor = { null }, ), pushProvider1: PushProvider? = FakePushProvider( index = 1, name = "aFakePushProvider1", - isAvailable = true, distributors = listOf(Distributor("aDistributorValue1", "aDistributorName1")), currentDistributor = { null }, ), diff --git a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt index 3530095ed0..7895da495b 100644 --- a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt +++ b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt @@ -302,13 +302,11 @@ class NotificationSettingsPresenterTest { val pushProvider1 = FakePushProvider( index = 0, name = "aFakePushProvider0", - isAvailable = true, distributors = listOf(Distributor("aDistributorValue0", "aDistributorName0")), ) val pushProvider2 = FakePushProvider( index = 1, name = "aFakePushProvider1", - isAvailable = true, distributors = listOf(Distributor("aDistributorValue1", "aDistributorName1")), ) return FakePushService( From 0908e9b9e436c6a57c6e6bc78d2f37d2153fcfba Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 17:49:01 +0200 Subject: [PATCH 22/29] Fix test issue. --- .../appnav/loggedin/LoggedInPresenterTest.kt | 24 +++++++++++++++---- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index 4fa03fdbdd..7141f04fd0 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -45,6 +45,7 @@ import io.element.android.services.analytics.test.FakeAnalyticsService import io.element.android.tests.testutils.WarmUpRule import io.element.android.tests.testutils.consumeItemsUntilPredicate import io.element.android.tests.testutils.lambda.any +import io.element.android.tests.testutils.lambda.lambdaError import io.element.android.tests.testutils.lambda.lambdaRecorder import io.element.android.tests.testutils.lambda.value import kotlinx.coroutines.test.runTest @@ -368,16 +369,19 @@ class LoggedInPresenterTest { val lambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } + val selectPushProviderLambda = lambdaRecorder { _, _ -> } val sessionVerificationService = FakeSessionVerificationService() sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val pushProvider = FakePushProvider( + index = 1, + name = "aFakePushProvider1", + distributors = emptyList(), + ) val pushService = createFakePushService( - pushProvider0 = FakePushProvider( - index = 1, - name = "aFakePushProvider1", - distributors = emptyList(), - ), + pushProvider0 = pushProvider, pushProvider1 = null, registerWithLambda = lambda, + selectPushProviderLambda = selectPushProviderLambda, ) val presenter = createLoggedInPresenter( pushService = pushService, @@ -392,6 +396,14 @@ class LoggedInPresenterTest { .isInstanceOf(PusherRegistrationFailure.NoDistributorsAvailable::class.java) lambda.assertions() .isNeverCalled() + selectPushProviderLambda.assertions() + .isCalledOnce() + .with( + // MatrixClient + any(), + // PushProvider + value(pushProvider), + ) // Reset the error finalState.eventSink(LoggedInEvents.CloseErrorDialog(doNotShowAgain = false)) val lastState = awaitItem() @@ -415,12 +427,14 @@ class LoggedInPresenterTest { registerWithLambda: suspend (MatrixClient, PushProvider, Distributor) -> Result = { _, _, _ -> Result.success(Unit) }, + selectPushProviderLambda: (MatrixClient, PushProvider) -> Unit = { _, _ -> lambdaError() }, currentPushProvider: () -> PushProvider? = { null }, ): PushService { return FakePushService( availablePushProviders = listOfNotNull(pushProvider0, pushProvider1), registerWithLambda = registerWithLambda, currentPushProvider = currentPushProvider, + selectPushProviderLambda = selectPushProviderLambda, ) } From 892a6d550386f34839fddd7287ab194b67c7f524 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 17:54:55 +0200 Subject: [PATCH 23/29] Add test about selecting the first provider with a distributor. --- .../appnav/loggedin/LoggedInPresenterTest.kt | 51 +++++++++++++++++-- 1 file changed, 48 insertions(+), 3 deletions(-) diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index 7141f04fd0..274ae3f04e 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -217,7 +217,7 @@ class LoggedInPresenterTest { sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) val distributor = Distributor("aDistributorValue1", "aDistributorName1") val pushProvider = FakePushProvider( - index = 1, + index = 0, name = "aFakePushProvider0", distributors = listOf( Distributor("aDistributorValue0", "aDistributorName0"), @@ -373,8 +373,8 @@ class LoggedInPresenterTest { val sessionVerificationService = FakeSessionVerificationService() sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) val pushProvider = FakePushProvider( - index = 1, - name = "aFakePushProvider1", + index = 0, + name = "aFakePushProvider", distributors = emptyList(), ) val pushService = createFakePushService( @@ -411,6 +411,51 @@ class LoggedInPresenterTest { } } + @Test + fun `present - case two push providers but first one does not have distributor - second one will be used`() = runTest { + val lambda = lambdaRecorder> { _, _, _ -> + Result.success(Unit) + } + val sessionVerificationService = FakeSessionVerificationService() + sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val pushProvider0 = FakePushProvider( + index = 0, + name = "aFakePushProvider0", + distributors = emptyList(), + ) + val distributor = Distributor("aDistributorValue1", "aDistributorName1") + val pushProvider1 = FakePushProvider( + index = 1, + name = "aFakePushProvider1", + distributors = listOf(distributor), + ) + val pushService = createFakePushService( + pushProvider0 = pushProvider0, + pushProvider1 = pushProvider1, + registerWithLambda = lambda, + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = sessionVerificationService, + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + skipItems(2) + val finalState = awaitItem() + assertThat(finalState.pusherRegistrationState.isSuccess()).isTrue() + lambda.assertions().isCalledOnce() + .with( + // MatrixClient + any(), + // PushProvider with the distributor + value(pushProvider1), + // First distributor of second push provider + value(distributor), + ) + } + } + private fun createFakePushService( pushProvider0: PushProvider? = FakePushProvider( index = 0, From d97db21d7598c8520baabdae035721a2d349461d Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 18:03:00 +0200 Subject: [PATCH 24/29] Rather use NoDistributorsAvailable, it has more chance to happen IRL. --- .../io/element/android/appnav/loggedin/LoggedInStateProvider.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt index d98271bef0..d63e96f657 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInStateProvider.kt @@ -24,7 +24,7 @@ open class LoggedInStateProvider : PreviewParameterProvider { get() = sequenceOf( aLoggedInState(), aLoggedInState(showSyncSpinner = true), - aLoggedInState(pusherRegistrationState = AsyncData.Failure(PusherRegistrationFailure.NoProvidersAvailable())), + aLoggedInState(pusherRegistrationState = AsyncData.Failure(PusherRegistrationFailure.NoDistributorsAvailable())), ) } From 46f71de5ef74f56139433b1ca57ab3f1c024b687 Mon Sep 17 00:00:00 2001 From: ElementBot Date: Mon, 17 Jun 2024 16:15:43 +0000 Subject: [PATCH 25/29] Update screenshots --- ...nView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png | 4 ++-- ...iew_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png index 33aced7342..b6d6043eec 100644 --- a/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Day-0_0_null_2,NEXUS_5,1.0,en].png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:9761d7e583bb275c40e42c1c1051b435d7445f84e825863ab42f50922d7841b1 -size 33908 +oid sha256:92c0b21d5de4540e7b3b784d18531cdf428bd0c9f13bdd445f811d6df73ef50b +size 36779 diff --git a/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png b/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png index d37591da85..b55e96be98 100644 --- a/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png +++ b/tests/uitests/src/test/snapshots/images/ui_S_t[appnav.loggedin_LoggedInView_null_LoggedInView-Night-0_1_null_2,NEXUS_5,1.0,en].png @@ -1,3 +1,3 @@ version https://git-lfs.github.com/spec/v1 -oid sha256:8a9103631e24ed4869d3bd5c7e04851e6ef5eb327daaf00de8a5e5c9e770ccf4 -size 31829 +oid sha256:9613aed2c45f172c7afec90e6205cba7f6112c6108d577c5377e5c3b07720426 +size 34664 From 85eae468b74ef6dfc77436be477cd4c19bc0b4e7 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Mon, 17 Jun 2024 18:27:47 +0200 Subject: [PATCH 26/29] Cleanup --- .../android/libraries/pushproviders/test/FakePushProvider.kt | 1 - .../pushproviders/unifiedpush/UnifiedPushProvider.kt | 4 ---- 2 files changed, 5 deletions(-) diff --git a/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt b/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt index cd68568d66..bbc1a2b60e 100644 --- a/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt +++ b/libraries/pushproviders/test/src/main/kotlin/io/element/android/libraries/pushproviders/test/FakePushProvider.kt @@ -31,7 +31,6 @@ class FakePushProvider( private val registerWithResult: (MatrixClient, Distributor) -> Result = { _, _ -> lambdaError() }, private val unregisterWithResult: (MatrixClient) -> Result = { lambdaError() }, ) : PushProvider { - override fun getDistributors(): List = distributors override suspend fun registerWith(matrixClient: MatrixClient, distributor: Distributor): Result { diff --git a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt index f302a6016f..680b264f29 100644 --- a/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt +++ b/libraries/pushproviders/unifiedpush/src/main/kotlin/io/element/android/libraries/pushproviders/unifiedpush/UnifiedPushProvider.kt @@ -17,7 +17,6 @@ package io.element.android.libraries.pushproviders.unifiedpush import com.squareup.anvil.annotations.ContributesMultibinding -import io.element.android.libraries.core.log.logger.LoggerTag import io.element.android.libraries.di.AppScope import io.element.android.libraries.matrix.api.MatrixClient import io.element.android.libraries.pushproviders.api.CurrentUserPushConfig @@ -26,11 +25,8 @@ import io.element.android.libraries.pushproviders.api.PushProvider import io.element.android.libraries.pushstore.api.clientsecret.PushClientSecret import io.element.android.services.appnavstate.api.AppNavigationStateService import io.element.android.services.appnavstate.api.currentSessionId -import timber.log.Timber import javax.inject.Inject -private val loggerTag = LoggerTag("UnifiedPushProvider", LoggerTag.PushLoggerTag) - @ContributesMultibinding(AppScope::class) class UnifiedPushProvider @Inject constructor( private val unifiedPushDistributorProvider: UnifiedPushDistributorProvider, From f09b77f72f68254b18d875d55bef2b406b966f46 Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 18 Jun 2024 10:33:07 +0200 Subject: [PATCH 27/29] Update test after merging develop. --- .../impl/tasks/DefaultClearCacheUseCaseTest.kt | 12 +++++++++++- .../android/libraries/push/test/FakePushService.kt | 4 +++- 2 files changed, 14 insertions(+), 2 deletions(-) diff --git a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/tasks/DefaultClearCacheUseCaseTest.kt b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/tasks/DefaultClearCacheUseCaseTest.kt index 4b576bf3d1..6be7775f5b 100644 --- a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/tasks/DefaultClearCacheUseCaseTest.kt +++ b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/tasks/DefaultClearCacheUseCaseTest.kt @@ -22,8 +22,11 @@ import com.google.common.truth.Truth.assertThat import io.element.android.features.ftue.test.FakeFtueService import io.element.android.features.preferences.impl.DefaultCacheService import io.element.android.features.roomlist.impl.migration.InMemoryMigrationScreenStore +import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.test.FakeMatrixClient +import io.element.android.libraries.push.test.FakePushService import io.element.android.tests.testutils.lambda.lambdaRecorder +import io.element.android.tests.testutils.lambda.value import io.element.android.tests.testutils.testCoroutineDispatchers import kotlinx.coroutines.test.runTest import okhttp3.OkHttpClient @@ -48,6 +51,10 @@ class DefaultClearCacheUseCaseTest { val migrationScreenStore = InMemoryMigrationScreenStore( resetLambda = resetMigrationLambda, ) + val setIgnoreRegistrationErrorLambda = lambdaRecorder { _, _ -> } + val pushService = FakePushService( + setIgnoreRegistrationErrorLambda = setIgnoreRegistrationErrorLambda + ) val sut = DefaultClearCacheUseCase( context = InstrumentationRegistry.getInstrumentation().context, matrixClient = matrixClient, @@ -55,13 +62,16 @@ class DefaultClearCacheUseCaseTest { defaultCacheService = defaultCacheService, okHttpClient = { OkHttpClient.Builder().build() }, ftueService = ftueService, - migrationScreenStore = migrationScreenStore + migrationScreenStore = migrationScreenStore, + pushService = pushService, ) defaultCacheService.clearedCacheEventFlow.test { sut.invoke() clearCacheLambda.assertions().isCalledOnce() resetFtueLambda.assertions().isCalledOnce() resetMigrationLambda.assertions().isCalledOnce() + setIgnoreRegistrationErrorLambda.assertions().isCalledOnce() + .with(value(matrixClient.sessionId), value(false)) assertThat(awaitItem()).isEqualTo(matrixClient.sessionId) } } diff --git a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt index 8f3e45c4ce..1736bb9ccf 100644 --- a/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt +++ b/libraries/push/test/src/main/kotlin/io/element/android/libraries/push/test/FakePushService.kt @@ -33,7 +33,8 @@ class FakePushService( Result.success(Unit) }, private val currentPushProvider: () -> PushProvider? = { availablePushProviders.firstOrNull() }, - private val selectPushProviderLambda: suspend (MatrixClient, PushProvider) -> Unit = { _, _ -> lambdaError() } + private val selectPushProviderLambda: suspend (MatrixClient, PushProvider) -> Unit = { _, _ -> lambdaError() }, + private val setIgnoreRegistrationErrorLambda: (SessionId, Boolean) -> Unit = { _, _ -> lambdaError() }, ) : PushService { override suspend fun getCurrentPushProvider(): PushProvider? { return registeredPushProvider ?: currentPushProvider() @@ -70,6 +71,7 @@ class FakePushService( override suspend fun setIgnoreRegistrationError(sessionId: SessionId, ignore: Boolean) { ignoreRegistrationError.value = ignore + setIgnoreRegistrationErrorLambda(sessionId, ignore) } override suspend fun testPush(): Boolean = simulateLongTask { From bc30aee359c834124a765fe664b548b8a765fa5d Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 18 Jun 2024 11:01:36 +0200 Subject: [PATCH 28/29] Iterate on sessionVerificationService.sessionVerifiedStatus and fix tests. --- .../appnav/loggedin/LoggedInPresenter.kt | 30 +++--- .../appnav/loggedin/LoggedInPresenterTest.kt | 100 +++++++++++------- .../FakeSessionVerificationService.kt | 6 +- 3 files changed, 80 insertions(+), 56 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index e780770b8b..82e44ba948 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -41,7 +41,8 @@ import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatu import io.element.android.libraries.push.api.PushService import io.element.android.libraries.pushproviders.api.RegistrationFailure import io.element.android.services.analytics.api.AnalyticsService -import kotlinx.coroutines.flow.map +import kotlinx.coroutines.flow.launchIn +import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch import timber.log.Timber import javax.inject.Inject @@ -59,21 +60,24 @@ class LoggedInPresenter @Inject constructor( @Composable override fun present(): LoggedInState { val coroutineScope = rememberCoroutineScope() - val isVerified by remember { - sessionVerificationService.sessionVerifiedStatus.map { it == SessionVerifiedStatus.Verified } - }.collectAsState(initial = false) val ignoreRegistrationError by remember { pushService.ignoreRegistrationError(matrixClient.sessionId) }.collectAsState(initial = false) val pusherRegistrationState = remember>> { mutableStateOf(AsyncData.Uninitialized) } - if (isVerified) { - LaunchedEffect(Unit) { - ensurePusherIsRegistered(pusherRegistrationState) - } - } else { - LaunchedEffect(Unit) { - pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.AccountNotVerified()) - } + LaunchedEffect(Unit) { + sessionVerificationService.sessionVerifiedStatus + .onEach { sessionVerifiedStatus -> + when (sessionVerifiedStatus) { + SessionVerifiedStatus.Unknown -> Unit + SessionVerifiedStatus.Verified -> { + ensurePusherIsRegistered(pusherRegistrationState) + } + SessionVerifiedStatus.NotVerified -> { + pusherRegistrationState.value = AsyncData.Failure(PusherRegistrationFailure.AccountNotVerified()) + } + } + } + .launchIn(this) } val syncIndicator by matrixClient.roomListService.syncIndicator.collectAsState() val networkStatus by networkMonitor.connectivity.collectAsState() @@ -116,7 +120,7 @@ class LoggedInPresenter @Inject constructor( Timber.tag(pusherTag.value).d("Register with the first available push provider with at least one distributor") val pushProvider = pushService.getAvailablePushProviders() .firstOrNull { it.getDistributors().isNotEmpty() } - // Else fallback to the first available push provider (the list should never be empty) + // Else fallback to the first available push provider (the list should never be empty) ?: pushService.getAvailablePushProviders().firstOrNull() ?: return Unit .also { Timber.tag(pusherTag.value).w("No push providers available") } diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index 274ae3f04e..52a27c2db1 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -18,6 +18,7 @@ package io.element.android.appnav.loggedin import app.cash.molecule.RecompositionMode import app.cash.molecule.moleculeFlow +import app.cash.turbine.ReceiveTurbine import app.cash.turbine.test import com.google.common.truth.Truth.assertThat import im.vector.app.features.analytics.plan.CryptoSessionStateChange @@ -25,12 +26,14 @@ import im.vector.app.features.analytics.plan.UserProperties import io.element.android.features.networkmonitor.api.NetworkStatus import io.element.android.features.networkmonitor.test.FakeNetworkMonitor import io.element.android.libraries.matrix.api.MatrixClient +import io.element.android.libraries.matrix.api.core.SessionId import io.element.android.libraries.matrix.api.encryption.EncryptionService import io.element.android.libraries.matrix.api.encryption.RecoveryState import io.element.android.libraries.matrix.api.roomlist.RoomListService import io.element.android.libraries.matrix.api.verification.SessionVerificationService import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatus import io.element.android.libraries.matrix.test.AN_EXCEPTION +import io.element.android.libraries.matrix.test.A_SESSION_ID import io.element.android.libraries.matrix.test.FakeMatrixClient import io.element.android.libraries.matrix.test.encryption.FakeEncryptionService import io.element.android.libraries.matrix.test.roomlist.FakeRoomListService @@ -66,7 +69,6 @@ class LoggedInPresenterTest { assertThat(initialState.showSyncSpinner).isFalse() assertThat(initialState.pusherRegistrationState.isUninitialized()).isTrue() assertThat(initialState.ignoreRegistrationError).isFalse() - skipItems(1) } } @@ -106,16 +108,12 @@ class LoggedInPresenterTest { encryptionService.emitRecoveryState(RecoveryState.UNKNOWN) encryptionService.emitRecoveryState(RecoveryState.INCOMPLETE) verificationService.emitVerifiedStatus(SessionVerifiedStatus.Verified) - - skipItems(6) - + skipItems(4) assertThat(analyticsService.capturedEvents.size).isEqualTo(1) assertThat(analyticsService.capturedEvents[0]).isInstanceOf(CryptoSessionStateChange::class.java) - assertThat(analyticsService.capturedUserProperties.size).isEqualTo(1) assertThat(analyticsService.capturedUserProperties[0].recoveryState).isEqualTo(UserProperties.RecoveryState.Incomplete) assertThat(analyticsService.capturedUserProperties[0].verificationState).isEqualTo(UserProperties.VerificationState.Verified) - // ensure a sync status change does not trigger a new capture roomListService.postSyncIndicator(RoomListService.SyncIndicator.Show) skipItems(1) @@ -129,12 +127,17 @@ class LoggedInPresenterTest { Result.success(Unit) } val pushService = createFakePushService(registerWithLambda = lambda) - val presenter = createLoggedInPresenter(pushService = pushService) + val verificationService = FakeSessionVerificationService( + initialSessionVerifiedStatus = SessionVerifiedStatus.NotVerified + ) + val presenter = createLoggedInPresenter( + pushService = pushService, + sessionVerificationService = verificationService, + ) moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(1) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.errorOrNull()) .isInstanceOf(PusherRegistrationFailure.AccountNotVerified::class.java) lambda.assertions() @@ -147,8 +150,9 @@ class LoggedInPresenterTest { val lambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } - val sessionVerificationService = FakeSessionVerificationService() - sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val sessionVerificationService = FakeSessionVerificationService( + initialSessionVerifiedStatus = SessionVerifiedStatus.Verified + ) val pushService = createFakePushService( registerWithLambda = lambda, ) @@ -159,8 +163,7 @@ class LoggedInPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(2) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.isSuccess()).isTrue() lambda.assertions() .isCalledOnce() @@ -180,8 +183,9 @@ class LoggedInPresenterTest { val lambda = lambdaRecorder> { _, _, _ -> Result.failure(AN_EXCEPTION) } - val sessionVerificationService = FakeSessionVerificationService() - sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val sessionVerificationService = FakeSessionVerificationService( + initialSessionVerifiedStatus = SessionVerifiedStatus.Verified + ) val pushService = createFakePushService( registerWithLambda = lambda, ) @@ -192,8 +196,7 @@ class LoggedInPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(2) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.isFailure()).isTrue() lambda.assertions() .isCalledOnce() @@ -213,8 +216,9 @@ class LoggedInPresenterTest { val lambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } - val sessionVerificationService = FakeSessionVerificationService() - sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val sessionVerificationService = FakeSessionVerificationService( + initialSessionVerifiedStatus = SessionVerifiedStatus.Verified + ) val distributor = Distributor("aDistributorValue1", "aDistributorName1") val pushProvider = FakePushProvider( index = 0, @@ -237,8 +241,7 @@ class LoggedInPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(2) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.isSuccess()).isTrue() lambda.assertions() .isCalledOnce() @@ -258,8 +261,9 @@ class LoggedInPresenterTest { val lambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } - val sessionVerificationService = FakeSessionVerificationService() - sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val sessionVerificationService = FakeSessionVerificationService( + initialSessionVerifiedStatus = SessionVerifiedStatus.Verified + ) val pushProvider = FakePushProvider( index = 0, name = "aFakePushProvider0", @@ -281,8 +285,7 @@ class LoggedInPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(2) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.isSuccess()).isTrue() lambda.assertions() .isCalledOnce() @@ -302,8 +305,9 @@ class LoggedInPresenterTest { val lambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } - val sessionVerificationService = FakeSessionVerificationService() - sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val sessionVerificationService = FakeSessionVerificationService( + initialSessionVerifiedStatus = SessionVerifiedStatus.Verified + ) val pushProvider = FakePushProvider( index = 0, name = "aFakePushProvider0", @@ -321,8 +325,7 @@ class LoggedInPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(2) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.errorOrNull()) .isInstanceOf(PusherRegistrationFailure.NoDistributorsAvailable::class.java) lambda.assertions() @@ -335,12 +338,13 @@ class LoggedInPresenterTest { val lambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } - val sessionVerificationService = FakeSessionVerificationService() - sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val sessionVerificationService = FakeSessionVerificationService(SessionVerifiedStatus.Verified) + val setIgnoreRegistrationErrorLambda = lambdaRecorder { _, _ -> } val pushService = createFakePushService( pushProvider0 = null, pushProvider1 = null, registerWithLambda = lambda, + setIgnoreRegistrationErrorLambda = setIgnoreRegistrationErrorLambda, ) val presenter = createLoggedInPresenter( pushService = pushService, @@ -349,8 +353,7 @@ class LoggedInPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(2) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.errorOrNull()) .isInstanceOf(PusherRegistrationFailure.NoProvidersAvailable::class.java) lambda.assertions() @@ -358,6 +361,14 @@ class LoggedInPresenterTest { // Reset the error and do not show again finalState.eventSink(LoggedInEvents.CloseErrorDialog(doNotShowAgain = true)) skipItems(1) + setIgnoreRegistrationErrorLambda.assertions() + .isCalledOnce() + .with( + // SessionId + value(A_SESSION_ID), + // Ignore + value(true), + ) val lastState = awaitItem() assertThat(lastState.pusherRegistrationState.isUninitialized()).isTrue() assertThat(lastState.ignoreRegistrationError).isTrue() @@ -370,8 +381,9 @@ class LoggedInPresenterTest { Result.success(Unit) } val selectPushProviderLambda = lambdaRecorder { _, _ -> } - val sessionVerificationService = FakeSessionVerificationService() - sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val sessionVerificationService = FakeSessionVerificationService( + initialSessionVerifiedStatus = SessionVerifiedStatus.Verified + ) val pushProvider = FakePushProvider( index = 0, name = "aFakePushProvider", @@ -390,8 +402,7 @@ class LoggedInPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(2) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.errorOrNull()) .isInstanceOf(PusherRegistrationFailure.NoDistributorsAvailable::class.java) lambda.assertions() @@ -416,8 +427,9 @@ class LoggedInPresenterTest { val lambda = lambdaRecorder> { _, _, _ -> Result.success(Unit) } - val sessionVerificationService = FakeSessionVerificationService() - sessionVerificationService.givenVerifiedStatus(SessionVerifiedStatus.Verified) + val sessionVerificationService = FakeSessionVerificationService( + initialSessionVerifiedStatus = SessionVerifiedStatus.Verified + ) val pushProvider0 = FakePushProvider( index = 0, name = "aFakePushProvider0", @@ -441,8 +453,7 @@ class LoggedInPresenterTest { moleculeFlow(RecompositionMode.Immediate) { presenter.present() }.test { - skipItems(2) - val finalState = awaitItem() + val finalState = awaitFirstItem() assertThat(finalState.pusherRegistrationState.isSuccess()).isTrue() lambda.assertions().isCalledOnce() .with( @@ -474,15 +485,22 @@ class LoggedInPresenterTest { }, selectPushProviderLambda: (MatrixClient, PushProvider) -> Unit = { _, _ -> lambdaError() }, currentPushProvider: () -> PushProvider? = { null }, + setIgnoreRegistrationErrorLambda: (SessionId, Boolean) -> Unit = { _, _ -> lambdaError() }, ): PushService { return FakePushService( availablePushProviders = listOfNotNull(pushProvider0, pushProvider1), registerWithLambda = registerWithLambda, currentPushProvider = currentPushProvider, selectPushProviderLambda = selectPushProviderLambda, + setIgnoreRegistrationErrorLambda = setIgnoreRegistrationErrorLambda, ) } + private suspend fun ReceiveTurbine.awaitFirstItem(): T { + skipItems(1) + return awaitItem() + } + private fun createLoggedInPresenter( roomListService: RoomListService = FakeRoomListService(), networkStatus: NetworkStatus = NetworkStatus.Offline, diff --git a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/verification/FakeSessionVerificationService.kt b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/verification/FakeSessionVerificationService.kt index 780758acbe..5e173dd3ac 100644 --- a/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/verification/FakeSessionVerificationService.kt +++ b/libraries/matrix/test/src/main/kotlin/io/element/android/libraries/matrix/test/verification/FakeSessionVerificationService.kt @@ -24,8 +24,10 @@ import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.flow.MutableStateFlow import kotlinx.coroutines.flow.StateFlow -class FakeSessionVerificationService : SessionVerificationService { - private val _sessionVerifiedStatus = MutableStateFlow(SessionVerifiedStatus.Unknown) +class FakeSessionVerificationService( + initialSessionVerifiedStatus: SessionVerifiedStatus = SessionVerifiedStatus.Unknown, +) : SessionVerificationService { + private val _sessionVerifiedStatus = MutableStateFlow(initialSessionVerifiedStatus) private var _verificationFlowState = MutableStateFlow(VerificationFlowState.Initial) private var _needsSessionVerification = MutableStateFlow(true) var shouldFail = false From d69a5ee1a1eb3aee87a10a83097abd90a9bed2aa Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Tue, 18 Jun 2024 11:08:25 +0200 Subject: [PATCH 29/29] Also fix same issue for analytics. --- .../android/appnav/loggedin/LoggedInPresenter.kt | 12 ++++++++---- .../android/appnav/loggedin/LoggedInPresenterTest.kt | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt index 82e44ba948..616d370859 100644 --- a/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt +++ b/appnav/src/main/kotlin/io/element/android/appnav/loggedin/LoggedInPresenter.kt @@ -41,6 +41,7 @@ import io.element.android.libraries.matrix.api.verification.SessionVerifiedStatu import io.element.android.libraries.push.api.PushService import io.element.android.libraries.pushproviders.api.RegistrationFailure import io.element.android.services.analytics.api.AnalyticsService +import kotlinx.coroutines.flow.combine import kotlinx.coroutines.flow.launchIn import kotlinx.coroutines.flow.onEach import kotlinx.coroutines.launch @@ -86,10 +87,13 @@ class LoggedInPresenter @Inject constructor( networkStatus == NetworkStatus.Online && syncIndicator == RoomListService.SyncIndicator.Show } } - val verificationState by sessionVerificationService.sessionVerifiedStatus.collectAsState() - val recoveryState by encryptionService.recoveryStateStateFlow.collectAsState() - LaunchedEffect(verificationState, recoveryState) { - reportCryptoStatusToAnalytics(verificationState, recoveryState) + LaunchedEffect(Unit) { + combine( + sessionVerificationService.sessionVerifiedStatus, + encryptionService.recoveryStateStateFlow + ) { verificationState, recoveryState -> + reportCryptoStatusToAnalytics(verificationState, recoveryState) + }.launchIn(this) } fun handleEvent(event: LoggedInEvents) { diff --git a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt index 52a27c2db1..c9f7e2bcde 100644 --- a/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt +++ b/appnav/src/test/kotlin/io/element/android/appnav/loggedin/LoggedInPresenterTest.kt @@ -108,7 +108,7 @@ class LoggedInPresenterTest { encryptionService.emitRecoveryState(RecoveryState.UNKNOWN) encryptionService.emitRecoveryState(RecoveryState.INCOMPLETE) verificationService.emitVerifiedStatus(SessionVerifiedStatus.Verified) - skipItems(4) + skipItems(2) assertThat(analyticsService.capturedEvents.size).isEqualTo(1) assertThat(analyticsService.capturedEvents[0]).isInstanceOf(CryptoSessionStateChange::class.java) assertThat(analyticsService.capturedUserProperties.size).isEqualTo(1)