From 68a76f83a0cbbbbf7cd4b0c06a8b980ebe3b9cff Mon Sep 17 00:00:00 2001 From: Benoit Marty Date: Wed, 26 Feb 2025 10:55:33 +0100 Subject: [PATCH] Do not rely only on distributor name but consider value (appId) as well. This will fix issue when multiple UnifiedPush distributor with the same friendly name are available on the phone. Fixes #4306 --- .../NotificationSettingsPresenter.kt | 51 +++++++++---------- .../NotificationSettingsState.kt | 5 +- .../NotificationSettingsStateProvider.kt | 28 ++++++++-- .../notifications/NotificationSettingsView.kt | 12 +++-- .../NotificationSettingsPresenterTest.kt | 33 ++++++++++-- .../NotificationSettingsViewTest.kt | 2 +- .../pushproviders/api/Distributor.kt | 4 +- 7 files changed, 95 insertions(+), 40 deletions(-) diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt index a78b94c866..317c6e796e 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt @@ -83,19 +83,19 @@ class NotificationSettingsPresenter @Inject constructor( } } } - // List of Distributor names - val distributorNames = remember { - distributors.map { it.second.name }.toImmutableList() + // List of Distributors + val availableDistributors = remember { + distributors.map { it.second }.toImmutableList() } - var currentDistributorName by remember { mutableStateOf>(AsyncData.Uninitialized) } + var currentDistributor by remember { mutableStateOf>(AsyncData.Uninitialized) } var refreshPushProvider by remember { mutableIntStateOf(0) } LaunchedEffect(refreshPushProvider) { val p = pushService.getCurrentPushProvider() - val name = p?.getCurrentDistributor(matrixClient.sessionId)?.name - currentDistributorName = if (name != null) { - AsyncData.Success(name) + val distributor = p?.getCurrentDistributor(matrixClient.sessionId) + currentDistributor = if (distributor != null) { + AsyncData.Success(distributor) } else { AsyncData.Failure(Exception("Failed to get current push provider")) } @@ -108,24 +108,23 @@ class NotificationSettingsPresenter @Inject constructor( ) = launch { showChangePushProviderDialog = false data ?: return@launch - // No op if the value is the same. - if (data.second.name == currentDistributorName.dataOrNull()) return@launch - currentDistributorName = AsyncData.Loading(currentDistributorName.dataOrNull()) - data.let { (pushProvider, distributor) -> - pushService.registerWith( - matrixClient = matrixClient, - pushProvider = pushProvider, - distributor = distributor + val (pushProvider, distributor) = data + // No op if the distributor is the same. + if (distributor == currentDistributor.dataOrNull()) return@launch + currentDistributor = AsyncData.Loading(currentDistributor.dataOrNull()) + pushService.registerWith( + matrixClient = matrixClient, + pushProvider = pushProvider, + distributor = distributor + ) + .fold( + { + refreshPushProvider++ + }, + { + currentDistributor = AsyncData.Failure(it) + } ) - .fold( - { - refreshPushProvider++ - }, - { - currentDistributorName = AsyncData.Failure(it) - } - ) - } } fun handleEvents(event: NotificationSettingsEvents) { @@ -162,8 +161,8 @@ class NotificationSettingsPresenter @Inject constructor( appNotificationsEnabled = appNotificationsEnabled.value ), changeNotificationSettingAction = changeNotificationSettingAction.value, - currentPushDistributor = currentDistributorName, - availablePushDistributors = distributorNames, + currentPushDistributor = currentDistributor, + availablePushDistributors = availableDistributors, showChangePushProviderDialog = showChangePushProviderDialog, fullScreenIntentPermissionsState = key(refreshFullScreenIntentSettings) { fullScreenIntentPermissionsPresenter.present() }, eventSink = ::handleEvents diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsState.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsState.kt index 5f3b00a174..a7a1bf9192 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsState.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsState.kt @@ -12,6 +12,7 @@ import io.element.android.libraries.architecture.AsyncAction import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.fullscreenintent.api.FullScreenIntentPermissionsState import io.element.android.libraries.matrix.api.room.RoomNotificationMode +import io.element.android.libraries.pushproviders.api.Distributor import kotlinx.collections.immutable.ImmutableList @Immutable @@ -19,8 +20,8 @@ data class NotificationSettingsState( val matrixSettings: MatrixSettings, val appSettings: AppSettings, val changeNotificationSettingAction: AsyncAction, - val currentPushDistributor: AsyncData, - val availablePushDistributors: ImmutableList, + val currentPushDistributor: AsyncData, + val availablePushDistributors: ImmutableList, val showChangePushProviderDialog: Boolean, val fullScreenIntentPermissionsState: FullScreenIntentPermissionsState, val eventSink: (NotificationSettingsEvents) -> Unit, diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsStateProvider.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsStateProvider.kt index 01765d2399..c0193bd6a2 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsStateProvider.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsStateProvider.kt @@ -13,6 +13,7 @@ import io.element.android.libraries.architecture.AsyncData import io.element.android.libraries.fullscreenintent.api.FullScreenIntentPermissionsState import io.element.android.libraries.fullscreenintent.api.aFullScreenIntentPermissionsState import io.element.android.libraries.matrix.api.room.RoomNotificationMode +import io.element.android.libraries.pushproviders.api.Distributor import kotlinx.collections.immutable.persistentListOf import kotlinx.collections.immutable.toImmutableList @@ -24,11 +25,19 @@ open class NotificationSettingsStateProvider : PreviewParameterProvider = AsyncData.Success("Firebase"), - availablePushDistributors: List = listOf("Firebase", "ntfy"), + currentPushDistributor: AsyncData = AsyncData.Success(aDistributor("Firebase")), + availablePushDistributors: List = listOf( + aDistributor("Firebase"), + aDistributor("ntfy"), + ), showChangePushProviderDialog: Boolean = false, fullScreenIntentPermissionsState: FullScreenIntentPermissionsState = aFullScreenIntentPermissionsState(), eventSink: (NotificationSettingsEvents) -> Unit = {}, @@ -88,3 +100,11 @@ fun aInvalidNotificationSettingsState( fullScreenIntentPermissionsState = aFullScreenIntentPermissionsState(), eventSink = eventSink, ) + +fun aDistributor( + name: String = "Name", + value: String = "$name Value", +) = Distributor( + value = value, + name = name, +) diff --git a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsView.kt b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsView.kt index d74ed8581c..a1bdf7921c 100644 --- a/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsView.kt +++ b/features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsView.kt @@ -206,7 +206,7 @@ private fun NotificationSettingsContentView( stringResource(id = CommonStrings.common_error) ) is AsyncData.Success -> ListItemContent.Text( - state.currentPushDistributor.dataOrNull() ?: "" + state.currentPushDistributor.dataOrNull()?.name ?: "" ) }, onClick = { @@ -219,8 +219,14 @@ private fun NotificationSettingsContentView( if (state.showChangePushProviderDialog) { SingleSelectionDialog( title = stringResource(id = R.string.screen_advanced_settings_choose_distributor_dialog_title_android), - options = state.availablePushDistributors.map { - ListOption(title = it) + options = state.availablePushDistributors.map { distributor -> + // If there are several distributors with the same name, use the full name + val title = if (state.availablePushDistributors.count { it.name == distributor.name } > 1) { + distributor.fullName + } else { + distributor.name + } + ListOption(title = title) }.toImmutableList(), initialSelection = state.availablePushDistributors.indexOf(state.currentPushDistributor.dataOrNull()), onSelectOption = { index -> 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 57179955a8..fcefd5c43d 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 @@ -240,8 +240,11 @@ class NotificationSettingsPresenterTest { presenter.present() }.test { val initialState = awaitLastSequentialItem() - assertThat(initialState.currentPushDistributor).isEqualTo(AsyncData.Success("aDistributorName0")) - assertThat(initialState.availablePushDistributors).containsExactly("aDistributorName0", "aDistributorName1") + assertThat(initialState.currentPushDistributor).isEqualTo(AsyncData.Success(Distributor(value = "aDistributorValue0", name = "aDistributorName0"))) + assertThat(initialState.availablePushDistributors).containsExactly( + Distributor(value = "aDistributorValue0", name = "aDistributorName0"), + Distributor(value = "aDistributorValue1", name = "aDistributorName1"), + ) initialState.eventSink.invoke(NotificationSettingsEvents.ChangePushProvider) val withDialog = awaitItem() assertThat(withDialog.showChangePushProviderDialog).isTrue() @@ -257,11 +260,35 @@ class NotificationSettingsPresenterTest { assertThat(withNewProvider.currentPushDistributor).isInstanceOf(AsyncData.Loading::class.java) skipItems(1) val lastItem = awaitItem() - assertThat(lastItem.currentPushDistributor).isEqualTo(AsyncData.Success("aDistributorName1")) + assertThat(lastItem.currentPushDistributor).isEqualTo(AsyncData.Success(Distributor(value = "aDistributorValue1", name = "aDistributorName1"))) cancelAndIgnoreRemainingEvents() } } + @Test + fun `present - change push provider to the same value is no op`() = runTest { + val presenter = createNotificationSettingsPresenter( + pushService = createFakePushService(), + ) + moleculeFlow(RecompositionMode.Immediate) { + presenter.present() + }.test { + val initialState = awaitLastSequentialItem() + assertThat(initialState.currentPushDistributor).isEqualTo(AsyncData.Success(Distributor(value = "aDistributorValue0", name = "aDistributorName0"))) + assertThat(initialState.availablePushDistributors).containsExactly( + Distributor(value = "aDistributorValue0", name = "aDistributorName0"), + Distributor(value = "aDistributorValue1", name = "aDistributorName1"), + ) + initialState.eventSink.invoke(NotificationSettingsEvents.ChangePushProvider) + assertThat(awaitItem().showChangePushProviderDialog).isTrue() + // Choose the same value (index 0) + initialState.eventSink(NotificationSettingsEvents.SetPushProvider(0)) + val withNewProvider = awaitItem() + assertThat(withNewProvider.showChangePushProviderDialog).isFalse() + expectNoEvents() + } + } + @Test fun `present - RefreshSystemNotificationsEnabled also refreshes fullScreenIntentState`() = runTest { var lambdaResult = aFullScreenIntentPermissionsState(permissionGranted = false) diff --git a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsViewTest.kt b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsViewTest.kt index 01cc817941..2aa485a192 100644 --- a/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsViewTest.kt +++ b/features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsViewTest.kt @@ -267,7 +267,7 @@ class NotificationSettingsViewTest { state = aValidNotificationSettingsState( eventSink = eventsRecorder, showChangePushProviderDialog = true, - availablePushDistributors = listOf("P1", "P2") + availablePushDistributors = listOf(aDistributor("P1"), aDistributor("P2")) ), ) rule.onNodeWithText("P2").performClick() diff --git a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/Distributor.kt b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/Distributor.kt index c99e5072b7..d623740444 100644 --- a/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/Distributor.kt +++ b/libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/Distributor.kt @@ -18,4 +18,6 @@ package io.element.android.libraries.pushproviders.api data class Distributor( val value: String, val name: String, -) +) { + val fullName = "$name ($value)" +}