Remove one-to-one relationship between provider and ownership

mullvad · Dec 20, 2024 · 99bd279 · 99bd279
1 parent 9246544
commit 99bd279
Show file tree

Hide file tree

Showing 25 changed files with 322 additions and 318 deletions.
diff --git a/android/CHANGELOG.md b/android/CHANGELOG.md
@@ -33,6 +33,9 @@ Line wrap the file at 100 chars.                                              Th
 - Update to DAITA v2. The main difference is that many different machines are provided by relays
   instead of a bundled list.
 
+### Fixed
+- Fix a crash that would occur because a Provider would be listed twice in the filter screen.
+
 
 ## [android/2024.9] - 2024-12-09
 ### Changed

diff --git a/android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/data/DummyRelayItems.kt b/android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/data/DummyRelayItems.kt
@@ -6,7 +6,6 @@ import net.mullvad.mullvadvpn.lib.model.CustomListName
 import net.mullvad.mullvadvpn.lib.model.GeoLocationId
 import net.mullvad.mullvadvpn.lib.model.Ownership
 import net.mullvad.mullvadvpn.lib.model.PortRange
-import net.mullvad.mullvadvpn.lib.model.Provider
 import net.mullvad.mullvadvpn.lib.model.ProviderId
 import net.mullvad.mullvadvpn.lib.model.RelayItem
 import net.mullvad.mullvadvpn.lib.model.RelayList
@@ -20,8 +19,8 @@ private val DUMMY_RELAY_1 =
                 "Relay host 1",
             ),
         active = true,
-        provider =
-            Provider(providerId = ProviderId("PROVIDER RENTED"), ownership = Ownership.Rented),
+        provider = ProviderId("PROVIDER RENTED"),
+        ownership = Ownership.Rented,
         daita = false,
     )
 private val DUMMY_RELAY_2 =
@@ -32,8 +31,8 @@ private val DUMMY_RELAY_2 =
                 "Relay host 2",
             ),
         active = true,
-        provider =
-            Provider(providerId = ProviderId("PROVIDER OWNED"), ownership = Ownership.MullvadOwned),
+        provider = ProviderId("PROVIDER OWNED"),
+        ownership = Ownership.MullvadOwned,
         daita = false,
     )
 private val DUMMY_RELAY_CITY_1 =

diff --git a/android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreenTest.kt b/android/app/src/androidTest/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreenTest.kt
@@ -11,7 +11,6 @@ import net.mullvad.mullvadvpn.compose.createEdgeToEdgeComposeExtension
 import net.mullvad.mullvadvpn.compose.setContentWithTheme
 import net.mullvad.mullvadvpn.compose.state.RelayFilterUiState
 import net.mullvad.mullvadvpn.lib.model.Ownership
-import net.mullvad.mullvadvpn.lib.model.Provider
 import net.mullvad.mullvadvpn.lib.model.ProviderId
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.extension.RegisterExtension
@@ -30,7 +29,7 @@ class FilterScreenTest {
         onApplyClick: () -> Unit = {},
         onSelectedOwnership: (ownership: Ownership?) -> Unit = {},
         onAllProviderCheckChange: (isChecked: Boolean) -> Unit = {},
-        onSelectedProvider: (checked: Boolean, provider: Provider) -> Unit = { _, _ -> },
+        onSelectedProvider: (checked: Boolean, provider: ProviderId) -> Unit = { _, _ -> },
     ) {
         setContentWithTheme {
             FilterScreen(
@@ -50,7 +49,7 @@ class FilterScreenTest {
             initScreen(
                 state =
                     RelayFilterUiState(
-                        allProviders = DUMMY_RELAY_ALL_PROVIDERS,
+                        providerToOwnerships = DUMMY_RELAY_ALL_PROVIDERS,
                         selectedOwnership = null,
                         selectedProviders = DUMMY_SELECTED_PROVIDERS,
                     )
@@ -65,7 +64,7 @@ class FilterScreenTest {
             initScreen(
                 state =
                     RelayFilterUiState(
-                        allProviders = DUMMY_RELAY_ALL_PROVIDERS,
+                        providerToOwnerships = DUMMY_RELAY_ALL_PROVIDERS,
                         selectedOwnership = null,
                         selectedProviders = DUMMY_SELECTED_PROVIDERS,
                     )
@@ -80,7 +79,7 @@ class FilterScreenTest {
             initScreen(
                 state =
                     RelayFilterUiState(
-                        allProviders = DUMMY_RELAY_ALL_PROVIDERS,
+                        providerToOwnerships = DUMMY_RELAY_ALL_PROVIDERS,
                         selectedOwnership = Ownership.MullvadOwned,
                         selectedProviders = DUMMY_SELECTED_PROVIDERS,
                     )
@@ -95,7 +94,7 @@ class FilterScreenTest {
             initScreen(
                 state =
                     RelayFilterUiState(
-                        allProviders = DUMMY_RELAY_ALL_PROVIDERS,
+                        providerToOwnerships = DUMMY_RELAY_ALL_PROVIDERS,
                         selectedOwnership = Ownership.Rented,
                         selectedProviders = DUMMY_SELECTED_PROVIDERS,
                     )
@@ -110,7 +109,7 @@ class FilterScreenTest {
             initScreen(
                 state =
                     RelayFilterUiState(
-                        allProviders = DUMMY_RELAY_ALL_PROVIDERS,
+                        providerToOwnerships = DUMMY_RELAY_ALL_PROVIDERS,
                         selectedOwnership = null,
                         selectedProviders = DUMMY_SELECTED_PROVIDERS,
                     )
@@ -128,58 +127,57 @@ class FilterScreenTest {
             initScreen(
                 state =
                     RelayFilterUiState(
-                        allProviders = listOf(),
+                        providerToOwnerships = DUMMY_RELAY_ALL_PROVIDERS,
                         selectedOwnership = null,
-                        selectedProviders =
-                            listOf(Provider(ProviderId("31173"), Ownership.MullvadOwned)),
+                        selectedProviders = listOf(ProviderId("31173")),
                     ),
                 onApplyClick = mockClickListener,
             )
             onNodeWithText("Apply").performClick()
             verify { mockClickListener() }
         }
 
+    @Test
+    fun ensureSelectedProviderIsShowEvenThoughItIsNotInAllProviders() =
+        composeExtension.use {
+            // Arrange
+            initScreen(
+                state =
+                    RelayFilterUiState(
+                        providerToOwnerships = DUMMY_RELAY_ALL_PROVIDERS,
+                        selectedOwnership = null,
+                        selectedProviders = listOf(ProviderId("1RemovedProvider")),
+                    )
+            )
+
+            // Act
+            onNodeWithText("Providers").performClick()
+            // Asset
+            onNodeWithText("1RemovedProvider (removed)").assertExists()
+        }
+
     companion object {
         private val DUMMY_RELAY_ALL_PROVIDERS =
-            listOf(
-                Provider(ProviderId("31173"), Ownership.MullvadOwned),
-                Provider(ProviderId("100TB"), Ownership.Rented),
-                Provider(ProviderId("Blix"), Ownership.MullvadOwned),
-                Provider(ProviderId("Creanova"), Ownership.MullvadOwned),
-                Provider(ProviderId("DataPacket"), Ownership.Rented),
-                Provider(ProviderId("HostRoyale"), Ownership.Rented),
-                Provider(ProviderId("hostuniversal"), Ownership.Rented),
-                Provider(ProviderId("iRegister"), Ownership.Rented),
-                Provider(ProviderId("M247"), Ownership.Rented),
-                Provider(ProviderId("Makonix"), Ownership.Rented),
-                Provider(ProviderId("PrivateLayer"), Ownership.Rented),
-                Provider(ProviderId("ptisp"), Ownership.Rented),
-                Provider(ProviderId("Qnax"), Ownership.Rented),
-                Provider(ProviderId("Quadranet"), Ownership.Rented),
-                Provider(ProviderId("techfutures"), Ownership.Rented),
-                Provider(ProviderId("Tzulo"), Ownership.Rented),
-                Provider(ProviderId("xtom"), Ownership.Rented),
+            mapOf(
+                ProviderId("31173") to setOf(Ownership.MullvadOwned),
+                ProviderId("100TB") to setOf(Ownership.Rented),
+                ProviderId("Blix") to setOf(Ownership.MullvadOwned),
+                ProviderId("Creanova") to setOf(Ownership.MullvadOwned),
+                ProviderId("DataPacket") to setOf(Ownership.Rented),
+                ProviderId("HostRoyale") to setOf(Ownership.Rented),
+                ProviderId("hostuniversal") to setOf(Ownership.Rented),
+                ProviderId("iRegister") to setOf(Ownership.Rented),
+                ProviderId("M247") to setOf(Ownership.Rented),
+                ProviderId("Makonix") to setOf(Ownership.Rented),
+                ProviderId("PrivateLayer") to setOf(Ownership.Rented),
+                ProviderId("ptisp") to setOf(Ownership.Rented),
+                ProviderId("Qnax") to setOf(Ownership.Rented),
+                ProviderId("Quadranet") to setOf(Ownership.Rented),
+                ProviderId("techfutures") to setOf(Ownership.Rented),
+                ProviderId("Tzulo") to setOf(Ownership.Rented),
+                ProviderId("xtom") to setOf(Ownership.Rented),
             )
 
-        private val DUMMY_SELECTED_PROVIDERS =
-            listOf(
-                Provider(ProviderId("31173"), Ownership.MullvadOwned),
-                Provider(ProviderId("100TB"), Ownership.Rented),
-                Provider(ProviderId("Blix"), Ownership.MullvadOwned),
-                Provider(ProviderId("Creanova"), Ownership.MullvadOwned),
-                Provider(ProviderId("DataPacket"), Ownership.Rented),
-                Provider(ProviderId("HostRoyale"), Ownership.Rented),
-                Provider(ProviderId("hostuniversal"), Ownership.Rented),
-                Provider(ProviderId("iRegister"), Ownership.Rented),
-                Provider(ProviderId("M247"), Ownership.Rented),
-                Provider(ProviderId("Makonix"), Ownership.Rented),
-                Provider(ProviderId("PrivateLayer"), Ownership.Rented),
-                Provider(ProviderId("ptisp"), Ownership.Rented),
-                Provider(ProviderId("Qnax"), Ownership.Rented),
-                Provider(ProviderId("Quadranet"), Ownership.Rented),
-                Provider(ProviderId("techfutures"), Ownership.Rented),
-                Provider(ProviderId("Tzulo"), Ownership.Rented),
-                Provider(ProviderId("xtom"), Ownership.Rented),
-            )
+        private val DUMMY_SELECTED_PROVIDERS = DUMMY_RELAY_ALL_PROVIDERS.keys.toList()
     }
 }
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/cell/CheckboxCell.kt
@@ -31,6 +31,7 @@ internal fun CheckboxCell(
     modifier: Modifier = Modifier,
     title: String,
     checked: Boolean,
+    enabled: Boolean = true,
     onCheckedChange: (Boolean) -> Unit,
     background: Color = MaterialTheme.colorScheme.surfaceContainerLow,
     startPadding: Dp = Dimens.mediumPadding,
@@ -41,7 +42,7 @@ internal fun CheckboxCell(
         verticalAlignment = Alignment.CenterVertically,
         modifier =
             modifier
-                .clickable { onCheckedChange(!checked) }
+                .clickable(enabled) { onCheckedChange(!checked) }
                 .defaultMinSize(minHeight = minHeight)
                 .fillMaxWidth()
                 .background(background)

diff --git a/...in/kotlin/net/mullvad/mullvadvpn/compose/preview/FilterUiStatePreviewParameterProvider.kt b/...in/kotlin/net/mullvad/mullvadvpn/compose/preview/FilterUiStatePreviewParameterProvider.kt
@@ -3,19 +3,17 @@ package net.mullvad.mullvadvpn.compose.preview
 import androidx.compose.ui.tooling.preview.PreviewParameterProvider
 import net.mullvad.mullvadvpn.compose.state.RelayFilterUiState
 import net.mullvad.mullvadvpn.lib.model.Ownership
-import net.mullvad.mullvadvpn.lib.model.Provider
 import net.mullvad.mullvadvpn.lib.model.ProviderId
 
-private val PROVIDER =
-    Provider(providerId = ProviderId("provider1"), ownership = Ownership.MullvadOwned)
+private val PROVIDER_TO_OWNERSHIPS = mapOf(ProviderId("provider1") to setOf(Ownership.MullvadOwned))
 
 class FilterUiStatePreviewParameterProvider : PreviewParameterProvider<RelayFilterUiState> {
     override val values =
         sequenceOf(
             RelayFilterUiState(
+                providerToOwnerships = PROVIDER_TO_OWNERSHIPS,
                 selectedOwnership = Ownership.MullvadOwned,
-                allProviders = listOf(PROVIDER),
-                selectedProviders = listOf(PROVIDER),
+                selectedProviders = PROVIDER_TO_OWNERSHIPS.keys.toList(),
             )
         )
 }
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/preview/RelayItemPreviewData.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/preview/RelayItemPreviewData.kt
@@ -2,7 +2,6 @@ package net.mullvad.mullvadvpn.compose.preview
 
 import net.mullvad.mullvadvpn.lib.model.GeoLocationId
 import net.mullvad.mullvadvpn.lib.model.Ownership
-import net.mullvad.mullvadvpn.lib.model.Provider
 import net.mullvad.mullvadvpn.lib.model.ProviderId
 import net.mullvad.mullvadvpn.lib.model.RelayItem
 
@@ -56,7 +55,8 @@ private fun generateRelayItemRelay(
     RelayItem.Location.Relay(
         id = GeoLocationId.Hostname(city = cityCode, code = hostName),
         active = active,
-        provider = Provider(ProviderId("Provider"), Ownership.MullvadOwned),
+        provider = ProviderId("Provider"),
+        ownership = Ownership.MullvadOwned,
         daita = daita,
     )
 

diff --git a/.../net/mullvad/mullvadvpn/compose/preview/SelectLocationsUiStatePreviewParameterProvider.kt b/.../net/mullvad/mullvadvpn/compose/preview/SelectLocationsUiStatePreviewParameterProvider.kt
@@ -3,8 +3,6 @@ package net.mullvad.mullvadvpn.compose.preview
 import androidx.compose.ui.tooling.preview.PreviewParameterProvider
 import net.mullvad.mullvadvpn.compose.state.RelayListType
 import net.mullvad.mullvadvpn.compose.state.SelectLocationUiState
-import net.mullvad.mullvadvpn.lib.model.Ownership
-import net.mullvad.mullvadvpn.lib.model.Provider
 import net.mullvad.mullvadvpn.usecase.FilterChip
 import net.mullvad.mullvadvpn.usecase.ModelOwnership
 

diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/FilterScreen.kt
@@ -46,7 +46,7 @@ import net.mullvad.mullvadvpn.compose.state.RelayFilterUiState
 import net.mullvad.mullvadvpn.compose.transitions.SlideInFromRightTransition
 import net.mullvad.mullvadvpn.compose.util.CollectSideEffectWithLifecycle
 import net.mullvad.mullvadvpn.lib.model.Ownership
-import net.mullvad.mullvadvpn.lib.model.Provider
+import net.mullvad.mullvadvpn.lib.model.ProviderId
 import net.mullvad.mullvadvpn.lib.theme.AppTheme
 import net.mullvad.mullvadvpn.lib.theme.Dimens
 import net.mullvad.mullvadvpn.viewmodel.FilterScreenSideEffect
@@ -98,7 +98,7 @@ fun FilterScreen(
     onApplyClick: () -> Unit,
     onSelectedOwnership: (ownership: Ownership?) -> Unit,
     onAllProviderCheckChange: (isChecked: Boolean) -> Unit,
-    onSelectedProvider: (checked: Boolean, provider: Provider) -> Unit,
+    onSelectedProvider: (checked: Boolean, provider: ProviderId) -> Unit,
 ) {
     var providerExpanded by rememberSaveable { mutableStateOf(false) }
     var ownershipExpanded by rememberSaveable { mutableStateOf(false) }
@@ -126,7 +126,7 @@ fun FilterScreen(
                 itemsWithDivider(
                     key = { it.name },
                     contentType = { ContentType.ITEM },
-                    items = state.filteredOwnershipByProviders,
+                    items = state.selectableOwnerships,
                 ) { ownership ->
                     Ownership(ownership, state, onSelectedOwnership)
                 }
@@ -139,9 +139,17 @@ fun FilterScreen(
                     AllProviders(state, onAllProviderCheckChange)
                 }
                 itemsWithDivider(
-                    key = { it.providerId.value },
+                    key = { it.value },
                     contentType = { ContentType.ITEM },
-                    items = state.filteredProvidersByOwnership,
+                    items = state.removedProviders,
+                ) { provider ->
+                    RemovedProvider(provider, state, onSelectedProvider)
+                }
+
+                itemsWithDivider(
+                    key = { it.value },
+                    contentType = { ContentType.ITEM },
+                    items = state.selectableProviders,
                 ) { provider ->
                     Provider(provider, state, onSelectedProvider)
                 }
@@ -216,14 +224,30 @@ private fun LazyItemScope.AllProviders(
 
 @Composable
 private fun LazyItemScope.Provider(
-    provider: Provider,
+    providerId: ProviderId,
+    state: RelayFilterUiState,
+    onSelectedProvider: (checked: Boolean, providerId: ProviderId) -> Unit,
+) {
+    CheckboxCell(
+        title = providerId.value,
+        checked = providerId in state.selectedProviders,
+        onCheckedChange = { checked -> onSelectedProvider(checked, providerId) },
+        modifier = Modifier.animateItem(),
+    )
+}
+
+@Composable
+private fun LazyItemScope.RemovedProvider(
+    providerId: ProviderId,
     state: RelayFilterUiState,
-    onSelectedProvider: (checked: Boolean, provider: Provider) -> Unit,
+    onSelectedProvider: (checked: Boolean, providerId: ProviderId) -> Unit,
 ) {
+    val checked = providerId in state.selectedProviders
     CheckboxCell(
-        title = provider.providerId.value,
-        checked = provider in state.selectedProviders,
-        onCheckedChange = { checked -> onSelectedProvider(checked, provider) },
+        title = stringResource(R.string.removed_provider, providerId.value),
+        checked = checked,
+        enabled = checked,
+        onCheckedChange = { checked -> onSelectedProvider(checked, providerId) },
         modifier = Modifier.animateItem(),
     )
 }

diff --git a/...oid/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/FilterConstrainExtensions.kt b/...oid/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/state/FilterConstrainExtensions.kt
@@ -2,7 +2,7 @@ package net.mullvad.mullvadvpn.compose.state
 
 import net.mullvad.mullvadvpn.lib.model.Constraint
 import net.mullvad.mullvadvpn.lib.model.Ownership
-import net.mullvad.mullvadvpn.lib.model.Provider
+import net.mullvad.mullvadvpn.lib.model.ProviderId
 import net.mullvad.mullvadvpn.lib.model.Providers
 
 fun Ownership?.toOwnershipConstraint(): Constraint<Ownership> =
@@ -11,18 +11,15 @@ fun Ownership?.toOwnershipConstraint(): Constraint<Ownership> =
         else -> Constraint.Only(this)
     }
 
-fun Constraint<Providers>.toSelectedProviders(allProviders: List<Provider>): List<Provider> =
+fun Constraint<Providers>.toSelectedProviders(allProviders: List<ProviderId>): List<ProviderId> =
     when (this) {
         Constraint.Any -> allProviders
-        is Constraint.Only ->
-            value.providers.toList().mapNotNull { provider ->
-                allProviders.firstOrNull { it.providerId == provider }
-            }
+        is Constraint.Only -> value.providers.toList()
     }
 
-fun List<Provider>.toConstraintProviders(allProviders: List<Provider>): Constraint<Providers> =
+fun List<ProviderId>.toConstraintProviders(allProviders: List<ProviderId>): Constraint<Providers> =
     if (size == allProviders.size) {
         Constraint.Any
     } else {
-        Constraint.Only(Providers(map { provider -> provider.providerId }.toHashSet()))
+        Constraint.Only(Providers(toHashSet()))
     }