From 15677b59b6e2eab58c0c907bf669345fefdc2448 Mon Sep 17 00:00:00 2001 From: Mugurell Date: Fri, 29 Apr 2022 13:35:36 +0300 Subject: [PATCH] [fenix] For https://github.com/mozilla-mobile/fenix/issues/25025 - Hardcode showing at max 8 Pocket stories The default was already 8. This is now being moved closer to the source from where the stories to be shown are emitted. With the addition of sponsored stories at fixed positions having to support a variable number of stories being returned from AppState#getFilteredStories means increased complexity with no benefit. --- .../components/appstate/AppStoreReducer.kt | 17 +--- .../java/org/mozilla/fenix/ext/AppState.kt | 16 ++-- .../home/pocket/PocketStoriesViewHolder.kt | 5 +- .../mozilla/fenix/components/AppStoreTest.kt | 21 +++-- .../org/mozilla/fenix/ext/AppStateTest.kt | 80 ++++++++----------- 5 files changed, 57 insertions(+), 82 deletions(-) diff --git a/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt b/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt index a55fe11d3b..2ec3cae6f6 100644 --- a/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt +++ b/app/src/main/java/org/mozilla/fenix/components/appstate/AppStoreReducer.kt @@ -9,7 +9,6 @@ import org.mozilla.fenix.components.AppStore import org.mozilla.fenix.ext.filterOutTab import org.mozilla.fenix.ext.getFilteredStories import org.mozilla.fenix.ext.recentSearchGroup -import org.mozilla.fenix.home.pocket.POCKET_STORIES_TO_SHOW_COUNT import org.mozilla.fenix.home.pocket.PocketRecommendedStoriesSelectedCategory import org.mozilla.fenix.home.recenttabs.RecentTab import org.mozilla.fenix.home.recentvisits.RecentlyVisitedItem @@ -110,9 +109,7 @@ internal object AppStoreReducer { // Selecting a category means the stories to be displayed needs to also be changed. updatedCategoriesState.copy( - pocketStories = updatedCategoriesState.getFilteredStories( - POCKET_STORIES_TO_SHOW_COUNT - ) + pocketStories = updatedCategoriesState.getFilteredStories() ) } is AppAction.DeselectPocketStoriesCategory -> { @@ -124,18 +121,14 @@ internal object AppStoreReducer { // Deselecting a category means the stories to be displayed needs to also be changed. updatedCategoriesState.copy( - pocketStories = updatedCategoriesState.getFilteredStories( - POCKET_STORIES_TO_SHOW_COUNT - ) + pocketStories = updatedCategoriesState.getFilteredStories() ) } is AppAction.PocketStoriesCategoriesChange -> { val updatedCategoriesState = state.copy(pocketStoriesCategories = action.storiesCategories) // Whenever categories change stories to be displayed needs to also be changed. updatedCategoriesState.copy( - pocketStories = updatedCategoriesState.getFilteredStories( - POCKET_STORIES_TO_SHOW_COUNT - ) + pocketStories = updatedCategoriesState.getFilteredStories() ) } is AppAction.PocketStoriesCategoriesSelectionsChange -> { @@ -145,9 +138,7 @@ internal object AppStoreReducer { ) // Whenever categories change stories to be displayed needs to also be changed. updatedCategoriesState.copy( - pocketStories = updatedCategoriesState.getFilteredStories( - POCKET_STORIES_TO_SHOW_COUNT - ) + pocketStories = updatedCategoriesState.getFilteredStories() ) } is AppAction.PocketStoriesChange -> state.copy(pocketStories = action.pocketStories) diff --git a/app/src/main/java/org/mozilla/fenix/ext/AppState.kt b/app/src/main/java/org/mozilla/fenix/ext/AppState.kt index ea8a65e2a2..73b132c1ec 100644 --- a/app/src/main/java/org/mozilla/fenix/ext/AppState.kt +++ b/app/src/main/java/org/mozilla/fenix/ext/AppState.kt @@ -12,24 +12,22 @@ import org.mozilla.fenix.home.pocket.POCKET_STORIES_DEFAULT_CATEGORY_NAME import org.mozilla.fenix.home.pocket.PocketRecommendedStoriesCategory import org.mozilla.fenix.home.recenttabs.RecentTab.SearchGroup +@VisibleForTesting +internal const val POCKET_STORIES_TO_SHOW_COUNT = 8 + /** * Get the list of stories to be displayed based on the user selected categories. * - * @param neededStoriesCount how many stories are intended to be displayed. - * This only impacts filtered results guaranteeing an even spread of stories from each category. - * * @return a list of [PocketRecommendedStory]es from the currently selected categories. */ -fun AppState.getFilteredStories( - neededStoriesCount: Int -): List { +fun AppState.getFilteredStories(): List { if (pocketStoriesCategoriesSelections.isEmpty()) { return pocketStoriesCategories .find { it.name == POCKET_STORIES_DEFAULT_CATEGORY_NAME }?.stories ?.sortedBy { it.timesShown } - ?.take(neededStoriesCount) ?: emptyList() + ?.take(POCKET_STORIES_TO_SHOW_COUNT) ?: emptyList() } val oldestSortedCategories = pocketStoriesCategoriesSelections @@ -41,13 +39,13 @@ fun AppState.getFilteredStories( } val filteredStoriesCount = getFilteredStoriesCount( - oldestSortedCategories, neededStoriesCount + oldestSortedCategories, POCKET_STORIES_TO_SHOW_COUNT ) return oldestSortedCategories .flatMap { category -> category.stories.sortedBy { it.timesShown }.take(filteredStoriesCount[category.name]!!) - }.take(neededStoriesCount) + }.take(POCKET_STORIES_TO_SHOW_COUNT) } /** diff --git a/app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesViewHolder.kt b/app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesViewHolder.kt index c5d13eb5c3..585c20af93 100644 --- a/app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesViewHolder.kt +++ b/app/src/main/java/org/mozilla/fenix/home/pocket/PocketStoriesViewHolder.kt @@ -32,8 +32,6 @@ import org.mozilla.fenix.compose.SectionHeader import org.mozilla.fenix.theme.FirefoxTheme import org.mozilla.fenix.theme.Theme -internal const val POCKET_STORIES_TO_SHOW_COUNT = 8 - /** * [RecyclerView.ViewHolder] for displaying the list of [PocketRecommendedStory]s from [AppStore]. * @@ -102,8 +100,9 @@ fun PocketStoriesViewHolderPreview() { Spacer(Modifier.height(16.dp)) + @Suppress("MagicNumber") PocketStories( - stories = getFakePocketStories(POCKET_STORIES_TO_SHOW_COUNT), + stories = getFakePocketStories(8), contentPadding = 0.dp, onStoryClicked = { _, _ -> }, onDiscoverMoreClicked = {} diff --git a/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt b/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt index c447f0bf73..9c98b6c06c 100644 --- a/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt +++ b/app/src/test/java/org/mozilla/fenix/components/AppStoreTest.kt @@ -33,7 +33,6 @@ import org.mozilla.fenix.ext.components import org.mozilla.fenix.ext.getFilteredStories import org.mozilla.fenix.home.CurrentMode import org.mozilla.fenix.home.Mode -import org.mozilla.fenix.home.pocket.POCKET_STORIES_TO_SHOW_COUNT import org.mozilla.fenix.home.pocket.PocketRecommendedStoriesCategory import org.mozilla.fenix.home.pocket.PocketRecommendedStoriesSelectedCategory import org.mozilla.fenix.home.recentbookmarks.RecentBookmark @@ -300,11 +299,11 @@ class AppStoreTest { ) mockkStatic("org.mozilla.fenix.ext.AppStateKt") { - every { any().getFilteredStories(any()) } returns filteredStories + every { any().getFilteredStories() } returns filteredStories appStore.dispatch(AppAction.SelectPocketStoriesCategory("another")).join() - verify { any().getFilteredStories(POCKET_STORIES_TO_SHOW_COUNT) } + verify { any().getFilteredStories() } } val selectedCategories = appStore.state.pocketStoriesCategoriesSelections @@ -329,11 +328,11 @@ class AppStoreTest { ) mockkStatic("org.mozilla.fenix.ext.AppStateKt") { - every { any().getFilteredStories(any()) } returns filteredStories + every { any().getFilteredStories() } returns filteredStories appStore.dispatch(AppAction.DeselectPocketStoriesCategory("other")).join() - verify { any().getFilteredStories(POCKET_STORIES_TO_SHOW_COUNT) } + verify { any().getFilteredStories() } } val selectedCategories = appStore.state.pocketStoriesCategoriesSelections @@ -365,12 +364,12 @@ class AppStoreTest { mockkStatic("org.mozilla.fenix.ext.AppStateKt") { val firstFilteredStories = listOf(mockk()) - every { any().getFilteredStories(any()) } returns firstFilteredStories + every { any().getFilteredStories() } returns firstFilteredStories appStore.dispatch( AppAction.PocketStoriesCategoriesChange(listOf(otherStoriesCategory, anotherStoriesCategory)) ).join() - verify { any().getFilteredStories(POCKET_STORIES_TO_SHOW_COUNT) } + verify { any().getFilteredStories() } assertTrue( appStore.state.pocketStoriesCategories.containsAll( listOf(otherStoriesCategory, anotherStoriesCategory) @@ -380,13 +379,13 @@ class AppStoreTest { val updatedCategories = listOf(PocketRecommendedStoriesCategory("yetAnother")) val secondFilteredStories = listOf(mockk()) - every { any().getFilteredStories(any()) } returns secondFilteredStories + every { any().getFilteredStories() } returns secondFilteredStories appStore.dispatch( AppAction.PocketStoriesCategoriesChange( updatedCategories ) ).join() - verify(exactly = 2) { any().getFilteredStories(POCKET_STORIES_TO_SHOW_COUNT) } + verify(exactly = 2) { any().getFilteredStories() } assertTrue(updatedCategories.containsAll(appStore.state.pocketStoriesCategories)) assertSame(secondFilteredStories, appStore.state.pocketStories) } @@ -401,7 +400,7 @@ class AppStoreTest { mockkStatic("org.mozilla.fenix.ext.AppStateKt") { val firstFilteredStories = listOf(mockk()) - every { any().getFilteredStories(any()) } returns firstFilteredStories + every { any().getFilteredStories() } returns firstFilteredStories appStore.dispatch( AppAction.PocketStoriesCategoriesSelectionsChange( @@ -409,7 +408,7 @@ class AppStoreTest { categoriesSelected = listOf(selectedCategory) ) ).join() - verify { any().getFilteredStories(POCKET_STORIES_TO_SHOW_COUNT) } + verify { any().getFilteredStories() } assertTrue( appStore.state.pocketStoriesCategories.containsAll( listOf(otherStoriesCategory, anotherStoriesCategory) diff --git a/app/src/test/java/org/mozilla/fenix/ext/AppStateTest.kt b/app/src/test/java/org/mozilla/fenix/ext/AppStateTest.kt index 723ab9e07c..7a911623ea 100644 --- a/app/src/test/java/org/mozilla/fenix/ext/AppStateTest.kt +++ b/app/src/test/java/org/mozilla/fenix/ext/AppStateTest.kt @@ -36,48 +36,61 @@ class AppStateTest { ) ) - var result = state.getFilteredStories(2) - assertNull(result.firstOrNull { it.category != POCKET_STORIES_DEFAULT_CATEGORY_NAME }) + val result = state.getFilteredStories() - result = state.getFilteredStories(5) assertNull(result.firstOrNull { it.category != POCKET_STORIES_DEFAULT_CATEGORY_NAME }) } @Test - fun `GIVEN no category is selected WHEN getFilteredStories is called THEN no more than the indicated number of stories are returned`() { + fun `GIVEN no category is selected WHEN getFilteredStories is called THEN no more than the default stories number are returned from the default category`() { + val defaultStoriesCategoryWithManyStories = PocketRecommendedStoriesCategory( + POCKET_STORIES_DEFAULT_CATEGORY_NAME, + getFakePocketStories(POCKET_STORIES_TO_SHOW_COUNT + 2) + ) val state = AppState( pocketStoriesCategories = listOf( - otherStoriesCategory, anotherStoriesCategory, defaultStoriesCategory + otherStoriesCategory, anotherStoriesCategory, defaultStoriesCategoryWithManyStories ) ) - // Asking for fewer than available - var result = state.getFilteredStories(2) - assertEquals(2, result.size) + val result = state.getFilteredStories() - // Asking for more than available - result = state.getFilteredStories(5) - assertEquals(3, result.size) + assertEquals(POCKET_STORIES_TO_SHOW_COUNT, result.size) } @Test - fun `GIVEN a category is selected WHEN getFilteredStories is called for fewer than in the category THEN only stories from that category are returned`() { + fun `GIVEN a category is selected WHEN getFilteredStories is called THEN only stories from that category are returned`() { val state = AppState( pocketStoriesCategories = listOf(otherStoriesCategory, anotherStoriesCategory, defaultStoriesCategory), pocketStoriesCategoriesSelections = listOf(PocketRecommendedStoriesSelectedCategory(otherStoriesCategory.name)) ) - var result = state.getFilteredStories(2) - assertEquals(2, result.size) - assertNull(result.firstOrNull { it.category != otherStoriesCategory.name }) + val result = state.getFilteredStories() - result = state.getFilteredStories(3) - assertEquals(3, result.size) assertNull(result.firstOrNull { it.category != otherStoriesCategory.name }) } @Test - fun `GIVEN two categories are selected WHEN getFilteredStories is called for fewer than in both THEN only stories from those categories are returned`() { + fun `GIVEN a category is selected WHEN getFilteredStories is called THEN no more than the default stories number are returned from the selected category`() { + val otherStoriesCategoryWithManyStories = + PocketRecommendedStoriesCategory( + "other", + getFakePocketStories(POCKET_STORIES_TO_SHOW_COUNT + 2, "other") + ) + val state = AppState( + pocketStoriesCategories = + listOf(otherStoriesCategoryWithManyStories, anotherStoriesCategory, defaultStoriesCategory), + pocketStoriesCategoriesSelections = + listOf(PocketRecommendedStoriesSelectedCategory(otherStoriesCategoryWithManyStories.name)) + ) + + val result = state.getFilteredStories() + + assertEquals(POCKET_STORIES_TO_SHOW_COUNT, result.size) + } + + @Test + fun `GIVEN two categories are selected WHEN getFilteredStories is called THEN only stories from those categories are returned`() { val state = AppState( pocketStoriesCategories = listOf(otherStoriesCategory, anotherStoriesCategory, defaultStoriesCategory), pocketStoriesCategoriesSelections = listOf( @@ -86,15 +99,7 @@ class AppStateTest { ) ) - var result = state.getFilteredStories(2) - assertEquals(2, result.size) - assertNull( - result.firstOrNull { - it.category != otherStoriesCategory.name && it.category != anotherStoriesCategory.name - } - ) - - result = state.getFilteredStories(6) + val result = state.getFilteredStories() assertEquals(6, result.size) assertNull( result.firstOrNull { @@ -103,23 +108,6 @@ class AppStateTest { ) } - @Test - fun `GIVEN two categories are selected WHEN getFilteredStories is called for an odd number of stories THEN there are more by one stories from the newest category`() { - val state = AppState( - pocketStoriesCategories = listOf(otherStoriesCategory, anotherStoriesCategory, defaultStoriesCategory), - pocketStoriesCategoriesSelections = listOf( - PocketRecommendedStoriesSelectedCategory(otherStoriesCategory.name, selectionTimestamp = 0), - PocketRecommendedStoriesSelectedCategory(anotherStoriesCategory.name, selectionTimestamp = 1) - ) - ) - - val result = state.getFilteredStories(5) - - assertEquals(5, result.size) - assertEquals(2, result.filter { it.category == otherStoriesCategory.name }.size) - assertEquals(3, result.filter { it.category == anotherStoriesCategory.name }.size) - } - @Test fun `GIVEN no category is selected WHEN getFilteredStoriesCount is called THEN return an empty result`() { val result = getFilteredStoriesCount(emptyList(), 1) @@ -250,7 +238,7 @@ class AppStateTest { ) ) - val result = state.getFilteredStories(6) + val result = state.getFilteredStories() assertEquals(6, result.size) assertSame(secondCategory.stories[2], result.first()) @@ -271,7 +259,7 @@ class AppStateTest { ) ) - val result = state.getFilteredStories(6) + val result = state.getFilteredStories() assertEquals(3, result.size) assertNull(result.firstOrNull { it.category != anotherStoriesCategory.name })