Skip to content

Commit 1bbcedf

Browse files
bmartybmarty
authored
Merge pull request #4312 from element-hq/feature/bma/fixMultipleNtfy
Fix issues due to multiple ntfy applications with the same name.
2 parents d746f59 + 516a35b commit 1bbcedf

File tree

19 files changed

+121
-60
lines changed

19 files changed

+121
-60
lines changed

‎features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenter.kt‎

Lines changed: 25 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -83,19 +83,19 @@ class NotificationSettingsPresenter @Inject constructor(
8383
}
8484
}
8585
}
86-
// List of Distributor names
87-
val distributorNames = remember {
88-
distributors.map { it.second.name }.toImmutableList()
86+
// List of Distributors
87+
val availableDistributors = remember {
88+
distributors.map { it.second }.toImmutableList()
8989
}
9090

91-
var currentDistributorName by remember { mutableStateOf<AsyncData<String>>(AsyncData.Uninitialized) }
91+
var currentDistributor by remember { mutableStateOf<AsyncData<Distributor>>(AsyncData.Uninitialized) }
9292
var refreshPushProvider by remember { mutableIntStateOf(0) }
9393

9494
LaunchedEffect(refreshPushProvider) {
9595
val p = pushService.getCurrentPushProvider()
96-
val name = p?.getCurrentDistributor(matrixClient.sessionId)?.name
97-
currentDistributorName = if (name != null) {
98-
AsyncData.Success(name)
96+
val distributor = p?.getCurrentDistributor(matrixClient.sessionId)
97+
currentDistributor = if (distributor != null) {
98+
AsyncData.Success(distributor)
9999
} else {
100100
AsyncData.Failure(Exception("Failed to get current push provider"))
101101
}
@@ -108,24 +108,23 @@ class NotificationSettingsPresenter @Inject constructor(
108108
) = launch {
109109
showChangePushProviderDialog = false
110110
data ?: return@launch
111-
// No op if the value is the same.
112-
if (data.second.name == currentDistributorName.dataOrNull()) return@launch
113-
currentDistributorName = AsyncData.Loading(currentDistributorName.dataOrNull())
114-
data.let { (pushProvider, distributor) ->
115-
pushService.registerWith(
116-
matrixClient = matrixClient,
117-
pushProvider = pushProvider,
118-
distributor = distributor
111+
val (pushProvider, distributor) = data
112+
// No op if the distributor is the same.
113+
if (distributor == currentDistributor.dataOrNull()) return@launch
114+
currentDistributor = AsyncData.Loading(currentDistributor.dataOrNull())
115+
pushService.registerWith(
116+
matrixClient = matrixClient,
117+
pushProvider = pushProvider,
118+
distributor = distributor
119+
)
120+
.fold(
121+
{
122+
refreshPushProvider++
123+
},
124+
{
125+
currentDistributor = AsyncData.Failure(it)
126+
}
119127
)
120-
.fold(
121-
{
122-
refreshPushProvider++
123-
},
124-
{
125-
currentDistributorName = AsyncData.Failure(it)
126-
}
127-
)
128-
}
129128
}
130129

131130
fun handleEvents(event: NotificationSettingsEvents) {
@@ -162,8 +161,8 @@ class NotificationSettingsPresenter @Inject constructor(
162161
appNotificationsEnabled = appNotificationsEnabled.value
163162
),
164163
changeNotificationSettingAction = changeNotificationSettingAction.value,
165-
currentPushDistributor = currentDistributorName,
166-
availablePushDistributors = distributorNames,
164+
currentPushDistributor = currentDistributor,
165+
availablePushDistributors = availableDistributors,
167166
showChangePushProviderDialog = showChangePushProviderDialog,
168167
fullScreenIntentPermissionsState = key(refreshFullScreenIntentSettings) { fullScreenIntentPermissionsPresenter.present() },
169168
eventSink = ::handleEvents

‎features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsState.kt‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,15 +12,16 @@ import io.element.android.libraries.architecture.AsyncAction
1212
import io.element.android.libraries.architecture.AsyncData
1313
import io.element.android.libraries.fullscreenintent.api.FullScreenIntentPermissionsState
1414
import io.element.android.libraries.matrix.api.room.RoomNotificationMode
15+
import io.element.android.libraries.pushproviders.api.Distributor
1516
import kotlinx.collections.immutable.ImmutableList
1617

1718
@Immutable
1819
data class NotificationSettingsState(
1920
val matrixSettings: MatrixSettings,
2021
val appSettings: AppSettings,
2122
val changeNotificationSettingAction: AsyncAction<Unit>,
22-
val currentPushDistributor: AsyncData<String>,
23-
val availablePushDistributors: ImmutableList<String>,
23+
val currentPushDistributor: AsyncData<Distributor>,
24+
val availablePushDistributors: ImmutableList<Distributor>,
2425
val showChangePushProviderDialog: Boolean,
2526
val fullScreenIntentPermissionsState: FullScreenIntentPermissionsState,
2627
val eventSink: (NotificationSettingsEvents) -> Unit,

‎features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsStateProvider.kt‎

Lines changed: 24 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import io.element.android.libraries.architecture.AsyncData
1313
import io.element.android.libraries.fullscreenintent.api.FullScreenIntentPermissionsState
1414
import io.element.android.libraries.fullscreenintent.api.aFullScreenIntentPermissionsState
1515
import io.element.android.libraries.matrix.api.room.RoomNotificationMode
16+
import io.element.android.libraries.pushproviders.api.Distributor
1617
import kotlinx.collections.immutable.persistentListOf
1718
import kotlinx.collections.immutable.toImmutableList
1819

@@ -24,11 +25,19 @@ open class NotificationSettingsStateProvider : PreviewParameterProvider<Notifica
2425
aValidNotificationSettingsState(changeNotificationSettingAction = AsyncAction.Loading),
2526
aValidNotificationSettingsState(changeNotificationSettingAction = AsyncAction.Failure(Throwable("error"))),
2627
aValidNotificationSettingsState(
27-
availablePushDistributors = listOf("Firebase"),
28+
availablePushDistributors = listOf(aDistributor("Firebase")),
2829
changeNotificationSettingAction = AsyncAction.Failure(Throwable("error")),
2930
),
30-
aValidNotificationSettingsState(availablePushDistributors = listOf("Firebase")),
31+
aValidNotificationSettingsState(availablePushDistributors = listOf(aDistributor("Firebase"))),
3132
aValidNotificationSettingsState(showChangePushProviderDialog = true),
33+
aValidNotificationSettingsState(
34+
availablePushDistributors = listOf(
35+
aDistributor("Firebase"),
36+
aDistributor("ntfy", "app.id1"),
37+
aDistributor("ntfy", "app.id2"),
38+
),
39+
showChangePushProviderDialog = true,
40+
),
3241
aValidNotificationSettingsState(currentPushDistributor = AsyncData.Loading()),
3342
aValidNotificationSettingsState(currentPushDistributor = AsyncData.Failure(Exception("Failed to change distributor"))),
3443
aInvalidNotificationSettingsState(),
@@ -45,8 +54,11 @@ fun aValidNotificationSettingsState(
4554
inviteForMeNotificationsEnabled: Boolean = true,
4655
systemNotificationsEnabled: Boolean = true,
4756
appNotificationEnabled: Boolean = true,
48-
currentPushDistributor: AsyncData<String> = AsyncData.Success("Firebase"),
49-
availablePushDistributors: List<String> = listOf("Firebase", "ntfy"),
57+
currentPushDistributor: AsyncData<Distributor> = AsyncData.Success(aDistributor("Firebase")),
58+
availablePushDistributors: List<Distributor> = listOf(
59+
aDistributor("Firebase"),
60+
aDistributor("ntfy"),
61+
),
5062
showChangePushProviderDialog: Boolean = false,
5163
fullScreenIntentPermissionsState: FullScreenIntentPermissionsState = aFullScreenIntentPermissionsState(),
5264
eventSink: (NotificationSettingsEvents) -> Unit = {},
@@ -88,3 +100,11 @@ fun aInvalidNotificationSettingsState(
88100
fullScreenIntentPermissionsState = aFullScreenIntentPermissionsState(),
89101
eventSink = eventSink,
90102
)
103+
104+
fun aDistributor(
105+
name: String = "Name",
106+
value: String = "$name Value",
107+
) = Distributor(
108+
value = value,
109+
name = name,
110+
)

‎features/preferences/impl/src/main/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsView.kt‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -206,7 +206,7 @@ private fun NotificationSettingsContentView(
206206
stringResource(id = CommonStrings.common_error)
207207
)
208208
is AsyncData.Success -> ListItemContent.Text(
209-
state.currentPushDistributor.dataOrNull() ?: ""
209+
state.currentPushDistributor.dataOrNull()?.name ?: ""
210210
)
211211
},
212212
onClick = {
@@ -219,8 +219,14 @@ private fun NotificationSettingsContentView(
219219
if (state.showChangePushProviderDialog) {
220220
SingleSelectionDialog(
221221
title = stringResource(id = R.string.screen_advanced_settings_choose_distributor_dialog_title_android),
222-
options = state.availablePushDistributors.map {
223-
ListOption(title = it)
222+
options = state.availablePushDistributors.map { distributor ->
223+
// If there are several distributors with the same name, use the full name
224+
val title = if (state.availablePushDistributors.count { it.name == distributor.name } > 1) {
225+
distributor.fullName
226+
} else {
227+
distributor.name
228+
}
229+
ListOption(title = title)
224230
}.toImmutableList(),
225231
initialSelection = state.availablePushDistributors.indexOf(state.currentPushDistributor.dataOrNull()),
226232
onSelectOption = { index ->

‎features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsPresenterTest.kt‎

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -240,8 +240,11 @@ class NotificationSettingsPresenterTest {
240240
presenter.present()
241241
}.test {
242242
val initialState = awaitLastSequentialItem()
243-
assertThat(initialState.currentPushDistributor).isEqualTo(AsyncData.Success("aDistributorName0"))
244-
assertThat(initialState.availablePushDistributors).containsExactly("aDistributorName0", "aDistributorName1")
243+
assertThat(initialState.currentPushDistributor).isEqualTo(AsyncData.Success(Distributor(value = "aDistributorValue0", name = "aDistributorName0")))
244+
assertThat(initialState.availablePushDistributors).containsExactly(
245+
Distributor(value = "aDistributorValue0", name = "aDistributorName0"),
246+
Distributor(value = "aDistributorValue1", name = "aDistributorName1"),
247+
)
245248
initialState.eventSink.invoke(NotificationSettingsEvents.ChangePushProvider)
246249
val withDialog = awaitItem()
247250
assertThat(withDialog.showChangePushProviderDialog).isTrue()
@@ -257,11 +260,35 @@ class NotificationSettingsPresenterTest {
257260
assertThat(withNewProvider.currentPushDistributor).isInstanceOf(AsyncData.Loading::class.java)
258261
skipItems(1)
259262
val lastItem = awaitItem()
260-
assertThat(lastItem.currentPushDistributor).isEqualTo(AsyncData.Success("aDistributorName1"))
263+
assertThat(lastItem.currentPushDistributor).isEqualTo(AsyncData.Success(Distributor(value = "aDistributorValue1", name = "aDistributorName1")))
261264
cancelAndIgnoreRemainingEvents()
262265
}
263266
}
264267

268+
@Test
269+
fun `present - change push provider to the same value is no op`() = runTest {
270+
val presenter = createNotificationSettingsPresenter(
271+
pushService = createFakePushService(),
272+
)
273+
moleculeFlow(RecompositionMode.Immediate) {
274+
presenter.present()
275+
}.test {
276+
val initialState = awaitLastSequentialItem()
277+
assertThat(initialState.currentPushDistributor).isEqualTo(AsyncData.Success(Distributor(value = "aDistributorValue0", name = "aDistributorName0")))
278+
assertThat(initialState.availablePushDistributors).containsExactly(
279+
Distributor(value = "aDistributorValue0", name = "aDistributorName0"),
280+
Distributor(value = "aDistributorValue1", name = "aDistributorName1"),
281+
)
282+
initialState.eventSink.invoke(NotificationSettingsEvents.ChangePushProvider)
283+
assertThat(awaitItem().showChangePushProviderDialog).isTrue()
284+
// Choose the same value (index 0)
285+
initialState.eventSink(NotificationSettingsEvents.SetPushProvider(0))
286+
val withNewProvider = awaitItem()
287+
assertThat(withNewProvider.showChangePushProviderDialog).isFalse()
288+
expectNoEvents()
289+
}
290+
}
291+
265292
@Test
266293
fun `present - RefreshSystemNotificationsEnabled also refreshes fullScreenIntentState`() = runTest {
267294
var lambdaResult = aFullScreenIntentPermissionsState(permissionGranted = false)

‎features/preferences/impl/src/test/kotlin/io/element/android/features/preferences/impl/notifications/NotificationSettingsViewTest.kt‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -267,7 +267,7 @@ class NotificationSettingsViewTest {
267267
state = aValidNotificationSettingsState(
268268
eventSink = eventsRecorder,
269269
showChangePushProviderDialog = true,
270-
availablePushDistributors = listOf("P1", "P2")
270+
availablePushDistributors = listOf(aDistributor("P1"), aDistributor("P2"))
271271
),
272272
)
273273
rule.onNodeWithText("P2").performClick()

‎libraries/pushproviders/api/src/main/kotlin/io/element/android/libraries/pushproviders/api/Distributor.kt‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,4 +18,6 @@ package io.element.android.libraries.pushproviders.api
1818
data class Distributor(
1919
val value: String,
2020
val name: String,
21-
)
21+
) {
22+
val fullName = "$name ($value)"
23+
}

‎tests/uitests/src/test/snapshots/images/features.preferences.impl.notifications_NotificationSettingsView_Day_10_en.png‎

Lines changed: 2 additions & 2 deletions
Loading

‎tests/uitests/src/test/snapshots/images/features.preferences.impl.notifications_NotificationSettingsView_Day_11_en.png‎

Lines changed: 2 additions & 2 deletions
Loading

‎tests/uitests/src/test/snapshots/images/features.preferences.impl.notifications_NotificationSettingsView_Day_12_en.png‎

Lines changed: 2 additions & 2 deletions
Loading

0 commit comments

Comments
 (0)