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
This commit is contained in:
Benoit Marty
2025-02-26 10:55:33 +01:00
parent aca34c0dec
commit 68a76f83a0
7 changed files with 95 additions and 40 deletions

View File

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

View File

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

View File

@@ -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,
)

View File

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

View File

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

View File

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

View File

@@ -18,4 +18,6 @@ package io.element.android.libraries.pushproviders.api
data class Distributor(
val value: String,
val name: String,
)
) {
val fullName = "$name ($value)"
}