feat: show user an error message if they try to create a list cycle (f07da04a) · Commits · e / os / Murena Maps

cardinal-android/app/src/main/java/earth/maps/cardinal/ui/saved/ManagePlacesScreen.kt

+20 −2

Original line number	Diff line number	Diff line
		@@ -50,6 +50,8 @@ import androidx.compose.material3.IconButton
		import androidx.compose.material3.MaterialTheme
		import androidx.compose.material3.OutlinedTextField
		import androidx.compose.material3.Scaffold
		import androidx.compose.material3.SnackbarHost
		import androidx.compose.material3.SnackbarHostState
		import androidx.compose.material3.Text
		import androidx.compose.material3.ToggleFloatingActionButton
		import androidx.compose.material3.ToggleFloatingActionButtonDefaults.animateIcon
		@@ -103,11 +105,13 @@ fun ManagePlacesScreen(
		val clipboard by viewModel.clipboard.collectAsState(emptySet())
		val selectedItems by viewModel.selectedItems.collectAsState()
		val isAllSelected by viewModel.isAllSelected.collectAsState(initial = false)
		val errorMessage by viewModel.errorMessage.collectAsState()
		var showDeleteConfirmation by remember { mutableStateOf(false) }
		var showCreateListDialog by remember { mutableStateOf(false) }
		var showEditDialog by remember { mutableStateOf(false) }
		var editingItem by remember { mutableStateOf<ListContent?>(null) }
		var fabMenuExpanded by remember { mutableStateOf(false) }
		val snackbarHostState = remember { SnackbarHostState() }

		// Initialize the view model with the listId if provided
		LaunchedEffect(listId) {
		@@ -115,13 +119,28 @@ fun ManagePlacesScreen(
		}

		Scaffold(
		contentWindowInsets = WindowInsets.safeDrawing, topBar = {
		contentWindowInsets = WindowInsets.safeDrawing,
		snackbarHost = { SnackbarHost(snackbarHostState, modifier = Modifier.padding(bottom = TOOLBAR_HEIGHT_DP)) },
		topBar = {
		ManagePlacesTopBar(
		navController = navController,
		title = currentListName ?: stringResource(string.saved_places_title_case),
		breadcrumbNames = parents.plus(currentListName ?: ""),
		)
		}) { paddingValues ->

		val coroutineScope = rememberCoroutineScope()

		// Show error message in snackbar when it changes
		LaunchedEffect(errorMessage) {
		errorMessage?.let { message ->
		coroutineScope.launch {
		snackbarHostState.showSnackbar(message)
		}
		viewModel.clearErrorMessage()
		}
		}

		if (currentListContent?.isEmpty() == true) {
		Column(
		modifier = Modifier.padding(paddingValues)
		@@ -130,7 +149,6 @@ fun ManagePlacesScreen(
		}
		}

		val coroutineScope = rememberCoroutineScope()
		val currentListContent = currentListContent
		Box(modifier = Modifier.padding(paddingValues)) {
		AnimatedVisibility(

cardinal-android/app/src/main/java/earth/maps/cardinal/ui/saved/ManagePlacesViewModel.kt

+14 −0

Original line number	Diff line number	Diff line
		@@ -18,9 +18,12 @@

		package earth.maps.cardinal.ui.saved

		import android.content.Context
		import androidx.lifecycle.ViewModel
		import androidx.lifecycle.viewModelScope
		import dagger.hilt.android.lifecycle.HiltViewModel
		import dagger.hilt.android.qualifiers.ApplicationContext
		import earth.maps.cardinal.R
		import earth.maps.cardinal.data.ClipboardItem
		import earth.maps.cardinal.data.CutPasteRepository
		import earth.maps.cardinal.data.Place
		@@ -43,6 +46,7 @@ import javax.inject.Inject

		@HiltViewModel
		class ManagePlacesViewModel @Inject constructor(
		@param:ApplicationContext private val context: Context,
		private val savedPlaceRepository: SavedPlaceRepository,
		private val savedListRepository: SavedListRepository,
		private val listItemDao: ListItemDao,
		@@ -95,6 +99,10 @@ class ManagePlacesViewModel @Inject constructor(

		val clipboard: Flow<Set<ClipboardItem>> = cutPasteRepository.clipboard

		// Error messages for UI display
		private val _errorMessage = MutableStateFlow<String?>(null)
		val errorMessage: StateFlow<String?> = _errorMessage

		suspend fun setInitialList(listId: String?) {
		if (listId != null) {
		navigateToList(listId)
		@@ -206,6 +214,8 @@ class ManagePlacesViewModel @Inject constructor(

		if (wouldCreateCycle) {
		// Don't paste if it would create a cycle
		_errorMessage.value =
		context.getString(R.string.cannot_paste_a_list_into_itself_or_one_of_its_sublists)
		return@launch
		}

		@@ -218,6 +228,10 @@ class ManagePlacesViewModel @Inject constructor(
		}
		}

		fun clearErrorMessage() {
		_errorMessage.value = null
		}

		fun updatePlace(
		id: String,
		customName: String?,

cardinal-android/app/src/main/res/values/strings.xml

+1 −0

Original line number	Diff line number	Diff line
		@@ -264,4 +264,5 @@
		<string name="category_transportation">Transportation</string>
		<string name="category_entertainment">Entertainment</string>
		<string name="category_nightlife">Nightlife</string>
		<string name="cannot_paste_a_list_into_itself_or_one_of_its_sublists">Cannot paste a list into itself or one of its sublists</string>
		</resources>

cardinal-android/app/src/test/java/earth/maps/cardinal/ui/saved/ManagePlacesViewModelTest.kt

+290 −121

Original line number	Diff line number	Diff line
		package earth.maps.cardinal.ui.saved

		import android.content.Context
		import earth.maps.cardinal.MainCoroutineRule
		import earth.maps.cardinal.R.string
		import earth.maps.cardinal.data.ClipboardItem
		import earth.maps.cardinal.data.CutPasteRepository
		import earth.maps.cardinal.data.Place
		@@ -37,6 +39,7 @@ class ManagePlacesViewModelTest {
		val mainCoroutineRule = MainCoroutineRule()

		// Mock dependencies
		private val mockContext = mockk<Context>()
		private val mockSavedListRepository = mockk<SavedListRepository>(relaxed = false)
		private val mockSavedPlaceRepository = mockk<SavedPlaceRepository>(relaxed = false)
		private val mockListItemDao = mockk<ListItemDao>(relaxed = false)
		@@ -109,21 +112,41 @@ class ManagePlacesViewModelTest {

		@Before
		fun setup() {
		// Mock context methods
		every { mockContext.getString(string.cannot_paste_a_list_into_itself_or_one_of_its_sublists) } returns "error"
		// Mock repository methods
		coEvery { mockSavedListRepository.getRootList() } returns Result.success(rootList)
		coEvery { mockSavedListRepository.getListById(any()) } returns Result.success(null)
		coEvery { mockSavedListRepository.getListById(testList.id) } returns Result.success(testList)
		coEvery { mockSavedListRepository.getListById(nestedList.id) } returns Result.success(nestedList)
		coEvery { mockSavedListRepository.getListById(nestedList.id) } returns Result.success(
		nestedList
		)
		coEvery { mockSavedListRepository.getItemsInList(any()) } returns Result.success(emptyList())
		coEvery { mockSavedListRepository.getItemsInList(testList.id) } returns Result.success(listOf(listItem1))
		coEvery { mockSavedListRepository.getItemsInList(testList.id) } returns Result.success(
		listOf(listItem1)
		)
		coEvery { mockSavedListRepository.getItemIdsInListAsFlow(any()) } returns flowOf(emptySet())
		coEvery { mockSavedListRepository.getItemIdsInListAsFlow(testList.id) } returns flowOf(setOf(testPlace.id))
		coEvery { mockSavedListRepository.getItemIdsInListAsFlow(testList.id) } returns flowOf(
		setOf(
		testPlace.id
		)
		)
		coEvery { mockSavedListRepository.getListContent(any()) } returns flowOf(emptyList())

		// Mock additional repository methods needed for tests
		coEvery { mockSavedListRepository.deleteList(any()) } returns Result.success(Unit)
		coEvery { mockSavedListRepository.updateList(any(), any(), any()) } returns Result.success(Unit)
		coEvery { mockSavedListRepository.createList(any(), any(), any(), any(), any()) } returns Result.success("new-list-id")
		coEvery { mockSavedListRepository.updateList(any(), any(), any()) } returns Result.success(
		Unit
		)
		coEvery {
		mockSavedListRepository.createList(
		any(),
		any(),
		any(),
		any(),
		any()
		)
		} returns Result.success("new-list-id")

		// Mock DAO methods
		coEvery { mockListItemDao.getItemsInList(any()) } returns emptyList()
		@@ -132,7 +155,9 @@ class ManagePlacesViewModelTest {

		// Mock place repository
		coEvery { mockSavedPlaceRepository.getPlaceById(any()) } returns Result.success(null)
		coEvery { mockSavedPlaceRepository.getPlaceById(testPlace.id) } returns Result.success(testPlace)
		coEvery { mockSavedPlaceRepository.getPlaceById(testPlace.id) } returns Result.success(
		testPlace
		)
		coEvery { mockSavedPlaceRepository.toPlace(any()) } returns Place(
		id = testPlace.id,
		name = testPlace.name,
		@@ -144,7 +169,14 @@ class ManagePlacesViewModelTest {
		transitStopId = testPlace.transitStopId
		)
		coEvery { mockSavedPlaceRepository.deletePlace(any()) } returns Result.success(Unit)
		coEvery { mockSavedPlaceRepository.updatePlace(any(), any(), any(), any()) } returns Result.success(Unit)
		coEvery {
		mockSavedPlaceRepository.updatePlace(
		any(),
		any(),
		any(),
		any()
		)
		} returns Result.success(Unit)

		// Mock cut/paste repository
		every { mockCutPasteRepository.clipboard } returns MutableStateFlow(emptySet())
		@@ -153,6 +185,7 @@ class ManagePlacesViewModelTest {
		coEvery { mockSavedListRepository.wouldCreateCycle(any(), any()) } returns false

		viewModel = ManagePlacesViewModel(
		context = mockContext,
		savedPlaceRepository = mockSavedPlaceRepository,
		savedListRepository = mockSavedListRepository,
		listItemDao = mockListItemDao,
		@@ -207,6 +240,7 @@ class ManagePlacesViewModelTest {
		fun `currentListName should return root list name when no current list`() = runTest {
		// Given - ViewModel initialized with no current list
		val freshViewModel = ManagePlacesViewModel(
		context = mockContext,
		savedPlaceRepository = mockSavedPlaceRepository,
		savedListRepository = mockSavedListRepository,
		listItemDao = mockListItemDao,
		@@ -327,7 +361,9 @@ class ManagePlacesViewModelTest {
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockSavedListRepository.getItemsInList(testList.id) } returns Result.success(listOf(listItem1, listItemWithNestedList))
		coEvery { mockSavedListRepository.getItemsInList(testList.id) } returns Result.success(
		listOf(listItem1, listItemWithNestedList)
		)

		viewModel.setInitialList(testList.id)
		advanceUntilIdle()
		@@ -346,7 +382,15 @@ class ManagePlacesViewModelTest {
		fun `createNewListWithSelected should create new list with selected items`() = runTest {
		// Given
		val newListId = "new-list-id"
		coEvery { mockSavedListRepository.createList(any(), any(), any(), any(), any()) } returns Result.success(newListId)
		coEvery {
		mockSavedListRepository.createList(
		any(),
		any(),
		any(),
		any(),
		any()
		)
		} returns Result.success(newListId)
		coEvery { mockListItemDao.getItemsInList(newListId) } returns emptyList()

		viewModel.setInitialList(testList.id)
		@@ -409,6 +453,7 @@ class ManagePlacesViewModelTest {

		// When
		val freshViewModel = ManagePlacesViewModel(
		context = mockContext,
		savedPlaceRepository = mockSavedPlaceRepository,
		savedListRepository = mockSavedListRepository,
		listItemDao = mockListItemDao,
		@@ -504,7 +549,15 @@ class ManagePlacesViewModelTest {
		fun `createNewListWithSelected should clear selection after creating list`() = runTest {
		// Given
		val newListId = "new-list-id"
		coEvery { mockSavedListRepository.createList(any(), any(), any(), any(), any()) } returns Result.success(newListId)
		coEvery {
		mockSavedListRepository.createList(
		any(),
		any(),
		any(),
		any(),
		any()
		)
		} returns Result.success(newListId)
		coEvery { mockListItemDao.getItemsInList(newListId) } returns emptyList()

		viewModel.setInitialList(testList.id)
		@@ -608,7 +661,15 @@ class ManagePlacesViewModelTest {
		@Test
		fun `createNewListWithSelected should handle creation failures`() = runTest {
		// Given
		coEvery { mockSavedListRepository.createList(any(), any(), any(), any(), any()) } returns Result.failure(Exception("Creation failed"))
		coEvery {
		mockSavedListRepository.createList(
		any(),
		any(),
		any(),
		any(),
		any()
		)
		} returns Result.failure(Exception("Creation failed"))

		viewModel.setInitialList(testList.id)
		advanceUntilIdle()
		@@ -626,7 +687,9 @@ class ManagePlacesViewModelTest {
		@Test
		fun `selectAll should handle empty list gracefully`() = runTest {
		// Given - empty list
		coEvery { mockSavedListRepository.getItemsInList(testList.id) } returns Result.success(emptyList())
		coEvery { mockSavedListRepository.getItemsInList(testList.id) } returns Result.success(
		emptyList()
		)

		viewModel.setInitialList(testList.id)
		advanceUntilIdle()
		@@ -695,9 +758,15 @@ class ManagePlacesViewModelTest {
		addedAt = System.currentTimeMillis()
		)

		coEvery { mockSavedListRepository.getListById(parentList.id) } returns Result.success(parentList)
		coEvery { mockSavedListRepository.getListById(childList.id) } returns Result.success(childList)
		coEvery { mockSavedListRepository.getItemsInList(parentList.id) } returns Result.success(listOf(parentToChildListItem))
		coEvery { mockSavedListRepository.getListById(parentList.id) } returns Result.success(
		parentList
		)
		coEvery { mockSavedListRepository.getListById(childList.id) } returns Result.success(
		childList
		)
		coEvery { mockSavedListRepository.getItemsInList(parentList.id) } returns Result.success(
		listOf(parentToChildListItem)
		)

		// Mock wouldCreateCycle to return true when trying to paste parent into child
		coEvery {
		@@ -757,8 +826,12 @@ class ManagePlacesViewModelTest {
		updatedAt = System.currentTimeMillis()
		)

		coEvery { mockSavedListRepository.getListById(targetList.id) } returns Result.success(targetList)
		coEvery { mockSavedListRepository.getItemsInList(targetList.id) } returns Result.success(emptyList())
		coEvery { mockSavedListRepository.getListById(targetList.id) } returns Result.success(
		targetList
		)
		coEvery { mockSavedListRepository.getItemsInList(targetList.id) } returns Result.success(
		emptyList()
		)

		// Mock wouldCreateCycle to return false when no cycle would be created
		coEvery {
		@@ -832,7 +905,10 @@ class ManagePlacesViewModelTest {

		// Mock wouldCreateCycle to return true because testList would create a cycle
		coEvery {
		mockSavedListRepository.wouldCreateCycle(testList.id, setOf(testList.id, anotherList.id))
		mockSavedListRepository.wouldCreateCycle(
		testList.id,
		setOf(testList.id, anotherList.id)
		)
		} returns true

		viewModel.setInitialList(testList.id)
		@@ -846,4 +922,97 @@ class ManagePlacesViewModelTest {
		coVerify(exactly = 0) { mockListItemDao.moveItem(any(), any(), any()) }
		assertEquals(clipboardItems, clipboardFlow.value)
		}

		@Test
		fun `pasteSelected should set error message when cycle detected`() = runTest {
		// Given
		val clipboardItems = setOf(ClipboardItem(testList.id, ItemType.LIST))
		val clipboardFlow = MutableStateFlow(clipboardItems)
		every { mockCutPasteRepository.clipboard } returns clipboardFlow

		// Mock wouldCreateCycle to return true
		coEvery {
		mockSavedListRepository.wouldCreateCycle(testList.id, setOf(testList.id))
		} returns true

		viewModel.setInitialList(testList.id)
		advanceUntilIdle()

		// When
		viewModel.pasteSelected()
		advanceUntilIdle()

		// Then - should set error message
		val errorMessage = viewModel.errorMessage.value
		assertEquals("error", errorMessage)
		}

		@Test
		fun `pasteSelected should not set error message when no cycle detected`() = runTest {
		// Given
		val clipboardItems = setOf(ClipboardItem(testPlace.id, ItemType.PLACE))
		val clipboardFlow = MutableStateFlow(clipboardItems)
		every { mockCutPasteRepository.clipboard } returns clipboardFlow
		every { mockContext.getString(string.cannot_paste_a_list_into_itself_or_one_of_its_sublists) } returns "error"

		// Mock wouldCreateCycle to return false
		coEvery {
		mockSavedListRepository.wouldCreateCycle(testList.id, emptySet())
		} returns false

		viewModel.setInitialList(testList.id)
		advanceUntilIdle()

		// When
		viewModel.pasteSelected()
		advanceUntilIdle()

		// Then - should not set error message
		val errorMessage = viewModel.errorMessage.value
		assertEquals(null, errorMessage)
		}

		@Test
		fun `clearErrorMessage should clear the error message`() = runTest {
		// Given - set an error message first
		val clipboardItems = setOf(ClipboardItem(testList.id, ItemType.LIST))
		val clipboardFlow = MutableStateFlow(clipboardItems)
		every { mockCutPasteRepository.clipboard } returns clipboardFlow

		coEvery {
		mockSavedListRepository.wouldCreateCycle(testList.id, setOf(testList.id))
		} returns true

		viewModel.setInitialList(testList.id)
		advanceUntilIdle()
		viewModel.pasteSelected()
		advanceUntilIdle()

		// Verify error message is set
		assertEquals("error", viewModel.errorMessage.value)

		// When
		viewModel.clearErrorMessage()

		// Then - error message should be cleared
		assertEquals(null, viewModel.errorMessage.value)
		}

		@Test
		fun `errorMessage should be null initially`() = runTest {
		// Given - fresh ViewModel
		val freshViewModel = ManagePlacesViewModel(
		context = mockContext,
		savedPlaceRepository = mockSavedPlaceRepository,
		savedListRepository = mockSavedListRepository,
		listItemDao = mockListItemDao,
		cutPasteRepository = mockCutPasteRepository
		)

		// When
		val errorMessage = freshViewModel.errorMessage.value

		// Then - should be null initially
		assertEquals(null, errorMessage)
		}
		}
		No newline at end of file