From 344c5a4faaae6836d23549f55963fde2ea0d271b Mon Sep 17 00:00:00 2001 From: Wally Hackenslacker Date: Mon, 16 Mar 2026 22:01:09 -0400 Subject: [PATCH] fix: avoid redundant card activity reload and assert ordering --- .../kanbn4droid/app/CardDetailFlowTest.kt | 13 ++++++-- .../app/carddetail/CardDetailActivity.kt | 11 ++++++- .../app/carddetail/CardDetailViewModel.kt | 30 ++++++++++++++----- app/src/main/res/values/strings.xml | 2 ++ .../app/carddetail/CardDetailViewModelTest.kt | 9 +++--- 5 files changed, 48 insertions(+), 17 deletions(-) diff --git a/app/src/androidTest/java/space/hackenslacker/kanbn4droid/app/CardDetailFlowTest.kt b/app/src/androidTest/java/space/hackenslacker/kanbn4droid/app/CardDetailFlowTest.kt index 025d091..848e304 100644 --- a/app/src/androidTest/java/space/hackenslacker/kanbn4droid/app/CardDetailFlowTest.kt +++ b/app/src/androidTest/java/space/hackenslacker/kanbn4droid/app/CardDetailFlowTest.kt @@ -180,8 +180,9 @@ class CardDetailFlowTest { onView(withText(R.string.add)).perform(click()) awaitCondition { - fakeDataSource.addCommentCalls.get() == 1 && fakeDataSource.listActivitiesCalls.get() >= 2 + fakeDataSource.addCommentCalls.get() == 1 } + assertEquals(1, fakeDataSource.listActivitiesCalls.get()) assertTrue(observedEvents.any { it is CardDetailUiEvent.ShowSnackbar }) } @@ -196,6 +197,7 @@ class CardDetailFlowTest { onView(withText(containsString("Someone commented"))).check(matches(isDisplayed())) onView(withText(containsString("Someone updated this card"))).check(matches(isDisplayed())) onView(withText(containsString("body"))).check(matches(isDisplayed())) + assertEquals(listOf("a-new", "a-old"), observedStates.last().activities.map { it.id }) } @Test @@ -360,15 +362,20 @@ class CardDetailFlowTest { return if (listActivitiesResults.isNotEmpty()) listActivitiesResults.removeFirst() else DataSourceResult.Success(activities) } - override suspend fun addComment(cardId: String, comment: String): DataSourceResult { + override suspend fun addComment(cardId: String, comment: String): DataSourceResult> { addCommentCalls.incrementAndGet() val result = if (addCommentResults.isNotEmpty()) addCommentResults.removeFirst() else DataSourceResult.Success(Unit) if (result is DataSourceResult.Success) { activities = listOf( CardActivity("comment-${addCommentCalls.get()}", "comment", comment, System.currentTimeMillis()), ) + activities + return DataSourceResult.Success(activities) + } + return when (result) { + is DataSourceResult.GenericError -> result + DataSourceResult.SessionExpired -> DataSourceResult.SessionExpired + is DataSourceResult.Success -> DataSourceResult.Success(activities) } - return result } } } diff --git a/app/src/main/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailActivity.kt b/app/src/main/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailActivity.kt index 9f9e6a3..1b5906e 100644 --- a/app/src/main/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailActivity.kt +++ b/app/src/main/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailActivity.kt @@ -97,7 +97,13 @@ class CardDetailActivity : AppCompatActivity() { val id = cardId val fakeFactory = testDataSourceFactory if (fakeFactory != null) { - CardDetailViewModel.Factory(id, fakeFactory.invoke(id)) + CardDetailViewModel.Factory( + cardId = id, + dataSource = fakeFactory.invoke(id), + titleRequiredMessage = getString(R.string.card_title_required), + commentRequiredMessage = getString(R.string.card_detail_comment_required), + commentAddedMessage = getString(R.string.card_detail_comment_added), + ) } else { val repository = CardDetailRepository( sessionStore = sessionStore, @@ -107,6 +113,9 @@ class CardDetailActivity : AppCompatActivity() { CardDetailViewModel.Factory( cardId = id, dataSource = CardDetailRepositoryDataSource(repository, repository::loadCard), + titleRequiredMessage = getString(R.string.card_title_required), + commentRequiredMessage = getString(R.string.card_detail_comment_required), + commentAddedMessage = getString(R.string.card_detail_comment_added), ) } } diff --git a/app/src/main/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailViewModel.kt b/app/src/main/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailViewModel.kt index d181fab..81858b1 100644 --- a/app/src/main/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailViewModel.kt +++ b/app/src/main/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailViewModel.kt @@ -58,7 +58,7 @@ interface CardDetailDataSource { suspend fun updateDescription(cardId: String, description: String): DataSourceResult suspend fun updateDueDate(cardId: String, dueDate: LocalDate?): DataSourceResult suspend fun listActivities(cardId: String): DataSourceResult> - suspend fun addComment(cardId: String, comment: String): DataSourceResult + suspend fun addComment(cardId: String, comment: String): DataSourceResult> } internal class CardDetailRepositoryDataSource( @@ -85,10 +85,10 @@ internal class CardDetailRepositoryDataSource( return repository.listActivities(cardId).toDataSourceResult() } - override suspend fun addComment(cardId: String, comment: String): DataSourceResult { + override suspend fun addComment(cardId: String, comment: String): DataSourceResult> { val result = repository.addComment(cardId, comment) return when (result) { - is CardDetailRepository.Result.Success -> DataSourceResult.Success(Unit) + is CardDetailRepository.Result.Success -> DataSourceResult.Success(result.value) is CardDetailRepository.Result.Failure.SessionExpired -> DataSourceResult.SessionExpired is CardDetailRepository.Result.Failure.Generic -> { DataSourceResult.GenericError(result.message) @@ -101,6 +101,9 @@ class CardDetailViewModel( private val cardId: String, private val repository: CardDetailDataSource, private val descriptionDebounceMillis: Long = 800L, + private val titleRequiredMessage: String = "Card title is required", + private val commentRequiredMessage: String = "Comment is required", + private val commentAddedMessage: String = "Comment added", ) : ViewModel() { private val _uiState = MutableStateFlow(CardDetailUiState()) val uiState: StateFlow = _uiState.asStateFlow() @@ -284,7 +287,7 @@ class CardDetailViewModel( val payload = _uiState.value.commentDraft.trim() if (payload.isBlank()) { - _uiState.update { it.copy(commentErrorMessage = "Comment is required") } + _uiState.update { it.copy(commentErrorMessage = commentRequiredMessage) } return } @@ -299,10 +302,12 @@ class CardDetailViewModel( isCommentDialogOpen = false, commentDraft = "", commentDialogMode = CommentDialogMode.EDIT, + activities = result.value, + activitiesErrorMessage = null, + isActivitiesLoading = false, ) } - _events.emit(CardDetailUiEvent.ShowSnackbar("Comment added")) - loadActivities() + _events.emit(CardDetailUiEvent.ShowSnackbar(commentAddedMessage)) } is DataSourceResult.SessionExpired -> { @@ -342,7 +347,7 @@ class CardDetailViewModel( } val normalized = rawTitle.trim() if (normalized.isBlank()) { - _uiState.update { it.copy(titleErrorMessage = "Card title is required") } + _uiState.update { it.copy(titleErrorMessage = titleRequiredMessage) } return } if (!fromRetry && normalized == persistedTitle) { @@ -567,11 +572,20 @@ class CardDetailViewModel( class Factory( private val cardId: String, private val dataSource: CardDetailDataSource, + private val titleRequiredMessage: String = "Card title is required", + private val commentRequiredMessage: String = "Comment is required", + private val commentAddedMessage: String = "Comment added", ) : ViewModelProvider.Factory { @Suppress("UNCHECKED_CAST") override fun create(modelClass: Class): T { if (modelClass.isAssignableFrom(CardDetailViewModel::class.java)) { - return CardDetailViewModel(cardId = cardId, repository = dataSource) as T + return CardDetailViewModel( + cardId = cardId, + repository = dataSource, + titleRequiredMessage = titleRequiredMessage, + commentRequiredMessage = commentRequiredMessage, + commentAddedMessage = commentAddedMessage, + ) as T } throw IllegalArgumentException("Unknown ViewModel class: ${modelClass.name}") } diff --git a/app/src/main/res/values/strings.xml b/app/src/main/res/values/strings.xml index 6a1a551..e2abd52 100644 --- a/app/src/main/res/values/strings.xml +++ b/app/src/main/res/values/strings.xml @@ -85,4 +85,6 @@ commented updated this card Add + Comment is required + Comment added diff --git a/app/src/test/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailViewModelTest.kt b/app/src/test/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailViewModelTest.kt index 22ea69c..2293ffa 100644 --- a/app/src/test/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailViewModelTest.kt +++ b/app/src/test/java/space/hackenslacker/kanbn4droid/app/carddetail/CardDetailViewModelTest.kt @@ -227,10 +227,9 @@ class CardDetailViewModelTest { listActivitiesResults = ArrayDeque( listOf( DataSourceResult.Success(emptyList()), - DataSourceResult.Success(sampleActivitiesShuffled()), ), ), - addCommentResult = DataSourceResult.Success(Unit), + addCommentResult = DataSourceResult.Success(sampleActivitiesShuffled()), ) val viewModel = loadedViewModel(this, repository) val eventDeferred = async { viewModel.events.first { it is CardDetailUiEvent.ShowSnackbar } } @@ -243,7 +242,7 @@ class CardDetailViewModelTest { assertFalse(viewModel.uiState.value.isCommentDialogOpen) assertEquals("", viewModel.uiState.value.commentDraft) assertEquals(1, repository.addCommentCalls) - assertEquals(2, repository.listActivitiesCalls) + assertEquals(1, repository.listActivitiesCalls) assertEquals(listOf("a-mid", "a-new", "a-old"), viewModel.uiState.value.activities.map { it.id }) assertTrue(eventDeferred.await() is CardDetailUiEvent.ShowSnackbar) } @@ -365,7 +364,7 @@ class CardDetailViewModelTest { var updateTitleResult: DataSourceResult = DataSourceResult.Success(Unit), var updateDescriptionResult: DataSourceResult = DataSourceResult.Success(Unit), var updateDueDateResult: DataSourceResult = DataSourceResult.Success(Unit), - var addCommentResult: DataSourceResult = DataSourceResult.Success(Unit), + var addCommentResult: DataSourceResult> = DataSourceResult.Success(emptyList()), var updateDescriptionGate: CompletableDeferred? = null, var updateDescriptionGates: ArrayDeque> = ArrayDeque(), ) : CardDetailDataSource { @@ -416,7 +415,7 @@ class CardDetailViewModelTest { return listActivitiesResult } - override suspend fun addComment(cardId: String, comment: String): DataSourceResult { + override suspend fun addComment(cardId: String, comment: String): DataSourceResult> { addCommentCalls += 1 addCommentPayloads += comment return addCommentResult