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

Author: bmarty

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,15 +12,16 @@ 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
 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)"
+}