Add support for more detailed location changed messages

Rawa · Pururun · commit 7224dfe9c892 · 2024-07-18T09:35:24.000+02:00
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListSuccess.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/communication/CustomListSuccess.kt
@@ -1,9 +1,11 @@
 package net.mullvad.mullvadvpn.compose.communication
 
 import android.os.Parcelable
+import kotlinx.parcelize.IgnoredOnParcel
 import kotlinx.parcelize.Parcelize
 import net.mullvad.mullvadvpn.lib.model.CustomListId
 import net.mullvad.mullvadvpn.lib.model.CustomListName
+import net.mullvad.mullvadvpn.lib.model.GeoLocationId
 
 sealed interface CustomListSuccess : Parcelable {
     val undo: CustomListAction
@@ -31,6 +33,14 @@ data class Renamed(override val undo: CustomListAction.Rename) : CustomListSucce
 
 @Parcelize
 data class LocationsChanged(
+    val id: CustomListId,
     val name: CustomListName,
+    val locations: List<GeoLocationId>,
+    val oldLocations: List<GeoLocationId>,
+) : CustomListSuccess {
     override val undo: CustomListAction.UpdateLocations
-) : CustomListSuccess
+        get() = CustomListAction.UpdateLocations(id, oldLocations)
+
+    @IgnoredOnParcel val addedLocations = locations - oldLocations
+    @IgnoredOnParcel val removedLocations = oldLocations - locations
+}
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/screen/SelectLocationScreen.kt
@@ -849,7 +849,22 @@ private fun CustomListSuccess.message(context: Context): String =
             } ?: context.getString(R.string.locations_were_changed_for, name)
         is Deleted -> context.getString(R.string.delete_custom_list_message, name)
         is Renamed -> context.getString(R.string.name_was_changed_to, name)
-        is LocationsChanged -> context.getString(R.string.locations_were_changed_for, name)
+        is LocationsChanged ->
+            when {
+                addedLocations.isNotEmpty() && removedLocations.isEmpty() ->
+                    context.getString(
+                        R.string.location_was_added_to_list,
+                        addedLocations.first(),
+                        name
+                    )
+                removedLocations.isNotEmpty() && addedLocations.isEmpty() ->
+                    context.getString(
+                        R.string.location_was_removed_from_list,
+                        removedLocations.first(),
+                        name
+                    )
+                else -> context.getString(R.string.locations_were_changed_for, name)
+            }
     }
 
 @Composable
diff --git a/android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/customlists/CustomListActionUseCase.kt b/android/app/src/main/kotlin/net/mullvad/mullvadvpn/usecase/customlists/CustomListActionUseCase.kt
@@ -107,8 +107,10 @@ class CustomListActionUseCase(
             .mapLeft(UpdateLocationsError::UpdateLocations)
             .bind()
         LocationsChanged(
+            id = action.id,
             name = customList.name,
-            undo = action.not(locations = customList.locations)
+            locations = action.locations,
+            oldLocations = customList.locations,
         )
     }
 }
diff --git a/android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/CustomListActionUseCaseTest.kt b/android/app/src/test/kotlin/net/mullvad/mullvadvpn/usecase/CustomListActionUseCaseTest.kt
@@ -175,7 +175,13 @@ class CustomListActionUseCaseTest {
         val customList = CustomList(id = customListId, name = name, locations = oldLocations)
         val action = CustomListAction.UpdateLocations(id = customListId, locations = newLocations)
         val expectedResult =
-            LocationsChanged(name = name, undo = action.not(locations = oldLocations)).right()
+            LocationsChanged(
+                    id = customListId,
+                    name = name,
+                    locations = newLocations,
+                    oldLocations = oldLocations,
+                )
+                .right()
         coEvery { mockCustomListsRepository.getCustomListById(customListId) } returns
             customList.right()
 
diff --git a/android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModelTest.kt b/android/app/src/test/kotlin/net/mullvad/mullvadvpn/viewmodel/SelectLocationViewModelTest.kt
@@ -252,20 +252,37 @@ class SelectLocationViewModelTest {
     @Test
     fun `after adding a location to a list should emit location added side effect`() = runTest {
         // Arrange
-        val expectedResult: LocationsChanged = mockk()
+        val customListId = CustomListId("1")
+        val addedLocationsId = GeoLocationId.Country("se")
+        val customListName = CustomListName.fromString("custom")
         val location: RelayItem.Location.Country = mockk {
             every { id } returns GeoLocationId.Country("se")
+            every { name } returns "Sweden"
             every { descendants() } returns emptyList()
         }
         val customList =
             RelayItem.CustomList(
                 id = CustomListId("1"),
-                customListName = CustomListName.fromString("custom"),
+                customListName = customListName,
                 locations = emptyList(),
                 expanded = false
             )
+        val expectedResult =
+            LocationsChanged(
+                id = customListId,
+                name = customListName,
+                locations = listOf(addedLocationsId),
+                oldLocations = emptyList(),
+            )
+
         coEvery { mockCustomListActionUseCase(any<CustomListAction.UpdateLocations>()) } returns
-            expectedResult.right()
+            LocationsChanged(
+                    id = customListId,
+                    name = customListName,
+                    locations = listOf(addedLocationsId),
+                    oldLocations = emptyList()
+                )
+                .right()
 
         // Act, Assert
         viewModel.uiSideEffect.test {
@@ -276,6 +293,51 @@ class SelectLocationViewModelTest {
         }
     }
 
+    @Test
+    fun `after removing a location from a list should emit location removed side effect`() =
+        runTest {
+            // Arrange
+            val locationName = "Sweden"
+            val customListId = CustomListId("1")
+            val removedLocationsId = GeoLocationId.Country("se")
+            val customListName = CustomListName.fromString("custom")
+            val location: RelayItem.Location.Country = mockk {
+                every { id } returns removedLocationsId
+                every { name } returns locationName
+                every { descendants() } returns emptyList()
+            }
+            val customList =
+                RelayItem.CustomList(
+                    id = customListId,
+                    customListName = customListName,
+                    locations = emptyList(),
+                    expanded = false
+                )
+            val expectedResult =
+                LocationsChanged(
+                    id = customListId,
+                    name = customListName,
+                    locations = emptyList(),
+                    oldLocations = listOf(removedLocationsId),
+                )
+            coEvery { mockCustomListActionUseCase(any<CustomListAction.UpdateLocations>()) } returns
+                LocationsChanged(
+                        id = customListId,
+                        name = customListName,
+                        locations = emptyList(),
+                        oldLocations = listOf(removedLocationsId),
+                    )
+                    .right()
+
+            // Act, Assert
+            viewModel.uiSideEffect.test {
+                viewModel.removeLocationFromList(item = location, customList = customList)
+                val sideEffect = awaitItem()
+                assertIs<SelectLocationSideEffect.LocationRemovedFromCustomList>(sideEffect)
+                assertEquals(expectedResult, sideEffect.result)
+            }
+        }
+
     companion object {
         private const val RELAY_LIST_EXTENSIONS =
             "net.mullvad.mullvadvpn.relaylist.RelayListExtensionsKt"
diff --git a/android/lib/resource/src/main/res/values/strings.xml b/android/lib/resource/src/main/res/values/strings.xml
@@ -383,4 +383,5 @@
     <string name="delete_method_question">Delete method?</string>
     <string name="failed_to_set_current_test_error">Failed to set to current - API not reachable</string>
     <string name="failed_to_set_current_unknown_error">Failed to set to current - Unknown reason</string>
+    <string name="location_was_removed_from_list">%s was removed from \"%s\"</string>
 </resources>

Original file line number	Diff line number	Diff line change
`@@ -107,8 +107,10 @@ class CustomListActionUseCase(`
`107`	`107`	`.mapLeft(UpdateLocationsError::UpdateLocations)`
`108`	`108`	`.bind()`
`109`	`109`	`LocationsChanged(`
	`110`	`+ id = action.id,`
`110`	`111`	`name = customList.name,`
`111`		`- undo = action.not(locations = customList.locations)`
	`112`	`+ locations = action.locations,`
	`113`	`+ oldLocations = customList.locations,`
`112`	`114`	`)`
`113`	`115`	`}`
`114`	`116`	`}`