Merge pull request #4312 from element-hq/feature/bma/fixMultipleNtfy

Fix issues due to multiple ntfy applications with the same name.

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<String>>(AsyncData.Uninitialized) }
+        var currentDistributor by remember { mutableStateOf<AsyncData<Distributor>>(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

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<Unit>,
-    val currentPushDistributor: AsyncData<String>,
-    val availablePushDistributors: ImmutableList<String>,
+    val currentPushDistributor: AsyncData<Distributor>,
+    val availablePushDistributors: ImmutableList<Distributor>,
     val showChangePushProviderDialog: Boolean,
     val fullScreenIntentPermissionsState: FullScreenIntentPermissionsState,
     val eventSink: (NotificationSettingsEvents) -> Unit,

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<Notifica
             aValidNotificationSettingsState(changeNotificationSettingAction = AsyncAction.Loading),
             aValidNotificationSettingsState(changeNotificationSettingAction = AsyncAction.Failure(Throwable("error"))),
             aValidNotificationSettingsState(
-                availablePushDistributors = listOf("Firebase"),
+                availablePushDistributors = listOf(aDistributor("Firebase")),
                 changeNotificationSettingAction = AsyncAction.Failure(Throwable("error")),
             ),
-            aValidNotificationSettingsState(availablePushDistributors = listOf("Firebase")),
+            aValidNotificationSettingsState(availablePushDistributors = listOf(aDistributor("Firebase"))),
             aValidNotificationSettingsState(showChangePushProviderDialog = true),
+            aValidNotificationSettingsState(
+                availablePushDistributors = listOf(
+                    aDistributor("Firebase"),
+                    aDistributor("ntfy", "app.id1"),
+                    aDistributor("ntfy", "app.id2"),
+                ),
+                showChangePushProviderDialog = true,
+            ),
             aValidNotificationSettingsState(currentPushDistributor = AsyncData.Loading()),
             aValidNotificationSettingsState(currentPushDistributor = AsyncData.Failure(Exception("Failed to change distributor"))),
             aInvalidNotificationSettingsState(),
@@ -45,8 +54,11 @@ fun aValidNotificationSettingsState(
     inviteForMeNotificationsEnabled: Boolean = true,
     systemNotificationsEnabled: Boolean = true,
     appNotificationEnabled: Boolean = true,
-    currentPushDistributor: AsyncData<String> = AsyncData.Success("Firebase"),
-    availablePushDistributors: List<String> = listOf("Firebase", "ntfy"),
+    currentPushDistributor: AsyncData<Distributor> = AsyncData.Success(aDistributor("Firebase")),
+    availablePushDistributors: List<Distributor> = 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,
+)

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 ->

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)

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()

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)"
+}