From c1ab0caf990bd9aa3b8ae6801da5e9651c349dc1 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Tue, 21 May 2024 13:14:09 -0300 Subject: [PATCH 1/3] Update Loaded state with tags in a diff-style Keep existing tags content untouched and only add new tags in initial state for new tags, so the list updates in place. Also add placement animation to the list items for a better UX. --- .../tagsfeed/ReaderTagsFeedUiStateMapper.kt | 27 +++++-- .../tagsfeed/ReaderTagsFeedViewModel.kt | 70 ++++++++++++------- .../views/compose/tagsfeed/ReaderTagsFeed.kt | 47 ++++++++----- 3 files changed, 94 insertions(+), 50 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedUiStateMapper.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedUiStateMapper.kt index cfac4b72a7b1..015a1433aec1 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedUiStateMapper.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedUiStateMapper.kt @@ -94,13 +94,10 @@ class ReaderTagsFeedUiStateMapper @Inject constructor( ): ReaderTagsFeedViewModel.UiState.Loaded = ReaderTagsFeedViewModel.UiState.Loaded( data = tags.map { tag -> - ReaderTagsFeedViewModel.TagFeedItem( - tagChip = ReaderTagsFeedViewModel.TagChip( - tag = tag, - onTagChipClick = onTagChipClick, - onMoreFromTagClick = onMoreFromTagClick, - ), - postList = ReaderTagsFeedViewModel.PostList.Initial, + mapInitialTagFeedItem( + tag = tag, + onTagChipClick = onTagChipClick, + onMoreFromTagClick = onMoreFromTagClick, onItemEnteredView = onItemEnteredView, ) }, @@ -108,6 +105,22 @@ class ReaderTagsFeedUiStateMapper @Inject constructor( onRefresh = onRefresh, ) + fun mapInitialTagFeedItem( + tag: ReaderTag, + onTagChipClick: (ReaderTag) -> Unit, + onMoreFromTagClick: (ReaderTag) -> Unit, + onItemEnteredView: (ReaderTagsFeedViewModel.TagFeedItem) -> Unit, + ): ReaderTagsFeedViewModel.TagFeedItem = + ReaderTagsFeedViewModel.TagFeedItem( + tagChip = ReaderTagsFeedViewModel.TagChip( + tag = tag, + onTagChipClick = onTagChipClick, + onMoreFromTagClick = onMoreFromTagClick, + ), + postList = ReaderTagsFeedViewModel.PostList.Initial, + onItemEnteredView = onItemEnteredView, + ) + fun mapLoadingTagFeedItem( tag: ReaderTag, onTagChipClick: (ReaderTag) -> Unit, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedViewModel.kt index 2a0344f7ef56..8ccfa74dd5fa 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedViewModel.kt @@ -88,34 +88,41 @@ class ReaderTagsFeedViewModel @Inject constructor( } } - /** - * Fetch multiple tag posts in parallel. Each tag load causes a new state to be emitted, so multiple emissions of - * [uiStateFlow] are expected when calling this method for each tag, since each can go through the following - * [UiState]s: [UiState.Initial], [UiState.Loaded], [UiState.Loading], [UiState.Empty]. - */ - fun onTagsChanged(tags: List) { - // don't fetch tags again if the tags match, unless the user requested a refresh - (_uiStateFlow.value as? UiState.Loaded)?.let { loadedState -> - if (!loadedState.isRefreshing && tags == loadedState.data.map { it.tagChip.tag }) { - return + fun onTagsChanged(tags: List) = _uiStateFlow.update { currentState -> + when { + tags.isEmpty() -> { + UiState.Empty(::onOpenTagsListClick) } - } - if (tags.isEmpty()) { - _uiStateFlow.value = UiState.Empty(::onOpenTagsListClick) - return - } + currentState is UiState.Loaded -> { + val currentTags = currentState.data.map { it.tagChip.tag } + if (currentState.isRefreshing) { + readerTagsFeedUiStateMapper.mapInitialPostsUiState( + tags, + false, + ::onTagChipClick, + ::onMoreFromTagClick, + ::onItemEnteredView, + ::onRefresh + ) + } else if (currentTags != tags) { + updateLoadedStateWithTags(currentState, tags) + } else { + currentState + } + } - // Add tags to the list with the posts loading UI - _uiStateFlow.update { - readerTagsFeedUiStateMapper.mapInitialPostsUiState( - tags, - false, - ::onTagChipClick, - ::onMoreFromTagClick, - ::onItemEnteredView, - ::onRefresh - ) + else -> { + // Add tags to the list with the posts initial/loading UI + readerTagsFeedUiStateMapper.mapInitialPostsUiState( + tags, + false, + ::onTagChipClick, + ::onMoreFromTagClick, + ::onItemEnteredView, + ::onRefresh + ) + } } } @@ -139,6 +146,19 @@ class ReaderTagsFeedViewModel @Inject constructor( } } + private fun updateLoadedStateWithTags(state: UiState.Loaded, tags: List): UiState.Loaded { + val currentTagsMap = state.data.associateBy { it.tagChip.tag.tagSlug } + val updatedData = tags.map { tag -> + currentTagsMap[tag.tagSlug] ?: readerTagsFeedUiStateMapper.mapInitialTagFeedItem( + tag = tag, + onTagChipClick = ::onTagChipClick, + onMoreFromTagClick = ::onMoreFromTagClick, + onItemEnteredView = ::onItemEnteredView, + ) + } + return state.copy(data = updatedData) + } + private fun onNoConnectionRetryClick() { _uiStateFlow.value = UiState.Loading if (networkUtilsWrapper.isNetworkAvailable()) { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/views/compose/tagsfeed/ReaderTagsFeed.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/views/compose/tagsfeed/ReaderTagsFeed.kt index 789bf55173d7..1394c2d971ce 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/views/compose/tagsfeed/ReaderTagsFeed.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/views/compose/tagsfeed/ReaderTagsFeed.kt @@ -1,6 +1,7 @@ package org.wordpress.android.ui.reader.views.compose.tagsfeed import android.content.res.Configuration +import androidx.compose.foundation.ExperimentalFoundationApi import androidx.compose.foundation.background import androidx.compose.foundation.clickable import androidx.compose.foundation.interaction.MutableInteractionSource @@ -87,7 +88,7 @@ fun ReaderTagsFeed(uiState: UiState) { } } -@OptIn(ExperimentalMaterialApi::class) +@OptIn(ExperimentalMaterialApi::class, ExperimentalFoundationApi::class) @Composable private fun Loaded(uiState: UiState.Loaded) { val pullRefreshState = rememberPullRefreshState( @@ -108,6 +109,7 @@ private fun Loaded(uiState: UiState.Loaded) { ) { items( items = uiState.data, + key = { it.tagChip.tag.tagSlug } ) { item -> val tagChip = item.tagChip val postList = item.postList @@ -121,24 +123,33 @@ private fun Loaded(uiState: UiState.Loaded) { } else { AppColor.Black.copy(alpha = 0.08F) } - Spacer(modifier = Modifier.height(Margin.Large.value)) - // Tag chip UI - ReaderFilterChip( - modifier = Modifier.padding( - start = Margin.Large.value, - ), - text = UiString.UiStringText(tagChip.tag.tagTitle), - onClick = { tagChip.onTagChipClick(tagChip.tag) }, - height = 36.dp, - ) - Spacer(modifier = Modifier.height(Margin.Large.value)) - // Posts list UI - when (postList) { - is PostList.Initial, is PostList.Loading -> PostListLoading() - is PostList.Loaded -> PostListLoaded(postList, tagChip, backgroundColor) - is PostList.Error -> PostListError(postList, tagChip, backgroundColor) + + Column( + modifier = Modifier + .animateItemPlacement() + .fillMaxWidth() + .padding( + top = Margin.Large.value, + bottom = Margin.ExtraExtraMediumLarge.value, + ) + ) { + // Tag chip UI + ReaderFilterChip( + modifier = Modifier.padding( + start = Margin.Large.value, + ), + text = UiString.UiStringText(tagChip.tag.tagTitle), + onClick = { tagChip.onTagChipClick(tagChip.tag) }, + height = 36.dp, + ) + Spacer(modifier = Modifier.height(Margin.Large.value)) + // Posts list UI + when (postList) { + is PostList.Initial, is PostList.Loading -> PostListLoading() + is PostList.Loaded -> PostListLoaded(postList, tagChip, backgroundColor) + is PostList.Error -> PostListError(postList, tagChip, backgroundColor) + } } - Spacer(modifier = Modifier.height(Margin.ExtraExtraMediumLarge.value)) } } From e67a94627bb45bf927b9da8f3d1a1f4f328d751f Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Tue, 21 May 2024 16:17:31 -0300 Subject: [PATCH 2/3] Add mapper unit tests --- .../ReaderTagsFeedUiStateMapperTest.kt | 70 ++++++++++++++++++- 1 file changed, 69 insertions(+), 1 deletion(-) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedUiStateMapperTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedUiStateMapperTest.kt index e0bfbde05d8c..54df791458d7 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedUiStateMapperTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/tagsfeed/ReaderTagsFeedUiStateMapperTest.kt @@ -256,7 +256,75 @@ class ReaderTagsFeedUiStateMapperTest : BaseUnitTest() { } @Test - fun `Should map loading posts UI state correctly`() { + fun `Should map loading TagFeedItem correctly`() { + // Given + val readerTag = ReaderTag( + "tag", + "tag", + "tag", + "endpoint", + ReaderTagType.FOLLOWED, + ) + val onTagChipClick: (ReaderTag) -> Unit = {} + val onMoreFromTagClick: (ReaderTag) -> Unit = {} + val onItemEnteredView: (ReaderTagsFeedViewModel.TagFeedItem) -> Unit = {} + // When + val actual = classToTest.mapLoadingTagFeedItem( + tag = readerTag, + onTagChipClick = onTagChipClick, + onMoreFromTagClick = onMoreFromTagClick, + onItemEnteredView = onItemEnteredView, + ) + + // Then + val expected = ReaderTagsFeedViewModel.TagFeedItem( + tagChip = ReaderTagsFeedViewModel.TagChip( + tag = readerTag, + onTagChipClick = onTagChipClick, + onMoreFromTagClick = onMoreFromTagClick, + ), + postList = ReaderTagsFeedViewModel.PostList.Loading, + onItemEnteredView = onItemEnteredView, + ) + assertEquals(expected, actual) + } + + @Test + fun `Should map initial TagFeedItem correctly`() { + // Given + val readerTag = ReaderTag( + "tag", + "tag", + "tag", + "endpoint", + ReaderTagType.FOLLOWED, + ) + val onTagChipClick: (ReaderTag) -> Unit = {} + val onMoreFromTagClick: (ReaderTag) -> Unit = {} + val onItemEnteredView: (ReaderTagsFeedViewModel.TagFeedItem) -> Unit = {} + // When + val actual = classToTest.mapInitialTagFeedItem( + tag = readerTag, + onTagChipClick = onTagChipClick, + onMoreFromTagClick = onMoreFromTagClick, + onItemEnteredView = onItemEnteredView, + ) + + // Then + val expected = ReaderTagsFeedViewModel.TagFeedItem( + tagChip = ReaderTagsFeedViewModel.TagChip( + tag = readerTag, + onTagChipClick = onTagChipClick, + onMoreFromTagClick = onMoreFromTagClick, + ), + postList = ReaderTagsFeedViewModel.PostList.Initial, + onItemEnteredView = onItemEnteredView, + ) + assertEquals(expected, actual) + } + + @Test + fun `Should map initial posts UI state correctly`() { // Given val onTagChipClick: (ReaderTag) -> Unit = {} val onMoreFromTagClick: (ReaderTag) -> Unit = {} From 13d03b121a468eb928bddfd9586e34e0c84640b5 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Tue, 21 May 2024 16:38:15 -0300 Subject: [PATCH 3/3] Update ReaderTagsFeedViewModel unit tests --- .../viewmodels/ReaderTagsFeedViewModelTest.kt | 110 ++++++++++++++---- 1 file changed, 88 insertions(+), 22 deletions(-) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModelTest.kt index 83baaa4a6c28..992017556433 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModelTest.kt @@ -182,7 +182,6 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { ) } - @Suppress("LongMethod") @Test fun `given valid tags, when loaded, then UI state should update properly`() = testCollectingUiStates { // Given @@ -225,7 +224,6 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { ) } - @Suppress("LongMethod") @Test fun `given valid and invalid tags, when loaded, then UI state should update properly`() = testCollectingUiStates { // Given @@ -342,9 +340,8 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { assertIs>(readerNavigationEvents.first()) } - @Suppress("LongMethod") @Test - fun `given tags fetched, when start again, then nothing happens`() = testCollectingUiStates { + fun `given tags fetched, when onTagsChanged again, then nothing happens`() = testCollectingUiStates { // Given val tag1 = ReaderTestUtils.createTag("tag1") val tag2 = ReaderTestUtils.createTag("tag2") @@ -384,19 +381,23 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { verifyNoInteractions(readerPostRepository) } - @Suppress("LongMethod") @Test - fun `given tags fetched, when start again refreshing, then move back to initial state`() = testCollectingUiStates { + fun `given new tags fetched, when onTagsChanged again, then state updates`() = testCollectingUiStates { // Given - val tag1 = ReaderTestUtils.createTag("tag1") - val tag2 = ReaderTestUtils.createTag("tag2") + val tag1 = ReaderTestUtils.createTag("tag1") // will be present both times + val tag2 = ReaderTestUtils.createTag("tag2") // will be present only first time + val tag3 = ReaderTestUtils.createTag("tag3") // will be present only second time + val posts1 = ReaderPostList().apply { add(ReaderPost()) } val posts2 = ReaderPostList().apply { add(ReaderPost()) } - whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + val posts3 = ReaderPostList().apply { + add(ReaderPost()) + } + whenever(readerPostRepository.fetchNewerPostsForTag(tag1)).doSuspendableAnswer { delay(100) posts1 @@ -405,9 +406,15 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { delay(200) posts2 } + whenever(readerPostRepository.fetchNewerPostsForTag(tag3)).doSuspendableAnswer { + delay(300) + posts3 + } + mockMapInitialTagFeedItems() mockMapLoadingTagFeedItems() mockMapLoadedTagFeedItems() + mockMapInitialTagFeedItem() // When viewModel.onTagsChanged(listOf(tag1, tag2)) @@ -417,25 +424,31 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { viewModel.onItemEnteredView(getInitialTagFeedItem(tag2)) advanceUntilIdle() - viewModel.onRefresh() + assertThat(collectedUiStates.last()).isEqualTo( + ReaderTagsFeedViewModel.UiState.Loaded( + data = listOf( + getLoadedTagFeedItem(tag1), + getLoadedTagFeedItem(tag2), + ) + ) + ) // Then - viewModel.onTagsChanged(listOf(tag1, tag2)) + viewModel.onTagsChanged(listOf(tag1, tag3)) advanceUntilIdle() - val loadedState = collectedUiStates.last() as ReaderTagsFeedViewModel.UiState.Loaded - assertThat(loadedState.data).isEqualTo( - listOf( - getInitialTagFeedItem(tag1), - getInitialTagFeedItem(tag2) + assertThat(collectedUiStates.last()).isEqualTo( + ReaderTagsFeedViewModel.UiState.Loaded( + data = listOf( + getLoadedTagFeedItem(tag1), // still loaded even without entering view + getInitialTagFeedItem(tag3), + ) ) ) - assertThat(loadedState.isRefreshing).isFalse() } - @Suppress("LongMethod") @Test - fun `given no tags requested, when start, then UI state should update properly`() = testCollectingUiStates { + fun `given no tags, when onTagsChanged, then UI state should update properly`() = testCollectingUiStates { // Given val tags = emptyList() @@ -447,7 +460,55 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { assertThat(collectedUiStates).last().isInstanceOf(ReaderTagsFeedViewModel.UiState.Empty::class.java) } - @Suppress("LongMethod") + @Test + fun `given tags fetched, when onTagsChanged again refreshing, then move back to initial state`() = + testCollectingUiStates { + // Given + val tag1 = ReaderTestUtils.createTag("tag1") + val tag2 = ReaderTestUtils.createTag("tag2") + val posts1 = ReaderPostList().apply { + add(ReaderPost()) + } + val posts2 = ReaderPostList().apply { + add(ReaderPost()) + } + whenever(networkUtilsWrapper.isNetworkAvailable()).thenReturn(true) + whenever(readerPostRepository.fetchNewerPostsForTag(tag1)).doSuspendableAnswer { + delay(100) + posts1 + } + whenever(readerPostRepository.fetchNewerPostsForTag(tag2)).doSuspendableAnswer { + delay(200) + posts2 + } + mockMapInitialTagFeedItems() + mockMapLoadingTagFeedItems() + mockMapLoadedTagFeedItems() + + // When + viewModel.onTagsChanged(listOf(tag1, tag2)) + advanceUntilIdle() + viewModel.onItemEnteredView(getInitialTagFeedItem(tag1)) + advanceUntilIdle() + viewModel.onItemEnteredView(getInitialTagFeedItem(tag2)) + advanceUntilIdle() + + viewModel.onRefresh() + + // Then + viewModel.onTagsChanged(listOf(tag1, tag2)) + advanceUntilIdle() + + val loadedState = collectedUiStates.last() as ReaderTagsFeedViewModel.UiState.Loaded + assertThat(loadedState.data).isEqualTo( + listOf( + getInitialTagFeedItem(tag1), + getInitialTagFeedItem(tag2) + ) + ) + assertThat(loadedState.isRefreshing).isFalse() + } + @Test fun `given tags fetched, when refreshing, then update isRefreshing status`() = testCollectingUiStates { // Given @@ -487,7 +548,6 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { assertThat(loadedState.isRefreshing).isTrue() } - @Suppress("LongMethod") @Test fun `given tags fetched, when refreshing, then RefreshTagsFeed action is posted`() = testCollectingUiStates { // Given @@ -527,7 +587,6 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { assertThat(action).isEqualTo(ActionEvent.RefreshTags) } - @Suppress("LongMethod") @Test fun `given tags fetched and no connection, when refreshing, then show error message`() = testCollectingUiStates { // Given @@ -877,6 +936,13 @@ class ReaderTagsFeedViewModelTest : BaseUnitTest() { } } + private fun mockMapInitialTagFeedItem() { + whenever(readerTagsFeedUiStateMapper.mapInitialTagFeedItem(any(), any(), any(), any())) + .thenAnswer { + getInitialTagFeedItem(it.getArgument(0)) + } + } + private fun getInitialTagFeedItem(tag: ReaderTag) = ReaderTagsFeedViewModel.TagFeedItem( ReaderTagsFeedViewModel.TagChip(tag, {}, {}), ReaderTagsFeedViewModel.PostList.Initial