diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt index 474ef518d722..ec1427374780 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderFragment.kt @@ -13,6 +13,7 @@ import androidx.compose.runtime.livedata.observeAsState import androidx.compose.ui.platform.ViewCompositionStrategy import androidx.core.view.isVisible import androidx.fragment.app.Fragment +import androidx.lifecycle.Observer import androidx.lifecycle.ViewModelProvider import androidx.lifecycle.ViewModelStoreOwner import androidx.recyclerview.widget.RecyclerView @@ -96,6 +97,86 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView private var readerSubsActivityResultLauncher: ActivityResultLauncher? = null + private val wpMainActivityViewModel by lazy { + ViewModelProvider( + requireActivity(), + viewModelFactory + )[WPMainActivityViewModel::class.java] + } + + // region SubgroupFilterViewModel Observers + // we need a reference to the observers so they are properly handled by the lifecycle and ViewModel owners, avoiding + // duplication, and ensuring they are properly removed when the Fragment is destroyed + private val currentSubfilterObserver = Observer { subfilterListItem -> + viewModel.onSubFilterItemSelected(subfilterListItem) + } + + private val updateTagsAndSitesObserver = Observer>> { event -> + event.applyIfNotHandled { + if (NetworkUtils.isNetworkAvailable(activity)) { + ReaderUpdateServiceStarter.startService(activity, this) + } + } + } + + private val subFiltersObserver = Observer> { subFilters -> + val selectedTag = (viewModel.uiState.value as? ContentUiState)?.selectedReaderTag ?: return@Observer + viewModel.showTopBarFilterGroup( + selectedTag, + subFilters + ) + } + + private val bottomSheetUiStateObserver = Observer> { event -> + event.applyIfNotHandled { + val selectedTag = (viewModel.uiState.value as? ContentUiState)?.selectedReaderTag + ?: return@applyIfNotHandled + val viewModelKey = SubFilterViewModel.getViewModelKeyForTag(selectedTag) + + val fm = childFragmentManager + var bottomSheet = fm.findFragmentByTag(SUBFILTER_BOTTOM_SHEET_TAG) as SubfilterBottomSheetFragment? + if (isVisible && bottomSheet == null) { + val (title, category) = this as BottomSheetVisible + bottomSheet = newInstance( + viewModelKey, + category, + uiHelpers.getTextOfUiString(requireContext(), title) + ) + bottomSheet.show(childFragmentManager, SUBFILTER_BOTTOM_SHEET_TAG) + } else if (!isVisible && bottomSheet != null) { + bottomSheet.dismiss() + } + } + } + + private val bottomSheetActionObserver = Observer> { event -> + event.applyIfNotHandled { + when (this) { + is OpenSubsAtPage -> { + readerSubsActivityResultLauncher?.launch( + ReaderActivityLauncher.createIntentShowReaderSubs( + requireActivity(), + tabIndex + ) + ) + } + + is OpenLoginPage -> { + wpMainActivityViewModel.onOpenLoginPage() + } + + is OpenSearchPage -> { + ReaderActivityLauncher.showReaderSearch(requireActivity()) + } + + is OpenSuggestedTagsPage -> { + ReaderActivityLauncher.showReaderInterests(requireActivity()) + } + } + } + } + // endregion + override fun onViewCreated(view: View, savedInstanceState: Bundle?) { binding = ReaderFragmentLayoutBinding.bind(view).apply { initTopAppBar() @@ -453,104 +534,41 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView } private fun SubFilterViewModel.initSubFilterViewModel(startedTag: ReaderTag, savedInstanceState: Bundle?) { - setupSubFilterBottomSheetObservers(startedTag) + bottomSheetUiState.observe( + viewLifecycleOwner, + bottomSheetUiStateObserver + ) + + bottomSheetAction.observe( + viewLifecycleOwner, + bottomSheetActionObserver + ) currentSubFilter.observe( - viewLifecycleOwner - ) { subfilterListItem: SubfilterListItem -> - onSubfilterSelected(subfilterListItem) - viewModel.onSubFilterItemSelected(subfilterListItem) - } + viewLifecycleOwner, + currentSubfilterObserver + ) updateTagsAndSites.observe( - viewLifecycleOwner - ) { event: Event> -> - event.applyIfNotHandled { - if (NetworkUtils.isNetworkAvailable(activity)) { - ReaderUpdateServiceStarter.startService(activity, this) - } - } - } + viewLifecycleOwner, + updateTagsAndSitesObserver + ) if (startedTag.isFilterable) { subFilters.observe( - viewLifecycleOwner - ) { subFilters: List -> - viewModel.showTopBarFilterGroup( - startedTag, - subFilters - ) - } + viewLifecycleOwner, + subFiltersObserver + ) updateTagsAndSites() } else { viewModel.hideTopBarFilterGroup(startedTag) } - // thomashortadev: not sure if always passing the same tags can cause problems start(startedTag, startedTag, savedInstanceState) } - private fun SubFilterViewModel.setupSubFilterBottomSheetObservers(startedTag: ReaderTag) { - val wpMainActivityViewModel = ViewModelProvider( - requireActivity(), - viewModelFactory - )[WPMainActivityViewModel::class.java] - - val viewModelKey = SubFilterViewModel.getViewModelKeyForTag(startedTag) - - bottomSheetUiState.observe( - viewLifecycleOwner - ) { event: Event -> - event.applyIfNotHandled { - val fm = childFragmentManager - var bottomSheet = fm.findFragmentByTag(SUBFILTER_BOTTOM_SHEET_TAG) as SubfilterBottomSheetFragment? - if (isVisible && bottomSheet == null) { - loadSubFilters() - val (title, category) = this as BottomSheetVisible - bottomSheet = newInstance( - viewModelKey, - category, - uiHelpers.getTextOfUiString(requireContext(), title) - ) - bottomSheet.show(childFragmentManager, SUBFILTER_BOTTOM_SHEET_TAG) - } else if (!isVisible && bottomSheet != null) { - bottomSheet.dismiss() - } - } - } - - bottomSheetAction.observe( - viewLifecycleOwner - ) { event: Event -> - event.applyIfNotHandled { - when (this) { - is OpenSubsAtPage -> { - readerSubsActivityResultLauncher?.launch( - ReaderActivityLauncher.createIntentShowReaderSubs( - requireActivity(), - tabIndex - ) - ) - } - - is OpenLoginPage -> { - wpMainActivityViewModel.onOpenLoginPage() - } - - is OpenSearchPage -> { - ReaderActivityLauncher.showReaderSearch(requireActivity()) - } - - is OpenSuggestedTagsPage -> { - ReaderActivityLauncher.showReaderInterests(requireActivity()) - } - } - } - } - } - companion object { private const val SUBFILTER_BOTTOM_SHEET_TAG = "SUBFILTER_BOTTOM_SHEET_TAG" } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt index eefb58a67f82..8f41c16c70e7 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModel.kt @@ -9,8 +9,8 @@ import kotlinx.coroutines.withContext import org.greenrobot.eventbus.Subscribe import org.greenrobot.eventbus.ThreadMode import org.wordpress.android.analytics.AnalyticsTracker.Stat -import org.wordpress.android.datasets.ReaderBlogTable -import org.wordpress.android.datasets.ReaderTagTable +import org.wordpress.android.datasets.ReaderBlogTableWrapper +import org.wordpress.android.datasets.wrappers.ReaderTagTableWrapper import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.models.ReaderBlog import org.wordpress.android.models.ReaderTag @@ -38,7 +38,6 @@ import org.wordpress.android.util.UrlUtils import org.wordpress.android.viewmodel.Event import org.wordpress.android.viewmodel.ScopedViewModel import org.wordpress.android.viewmodel.SingleLiveEvent -import java.util.Comparator import java.util.EnumSet import javax.inject.Inject import javax.inject.Named @@ -50,7 +49,9 @@ class SubFilterViewModel @Inject constructor( private val subfilterListItemMapper: SubfilterListItemMapper, private val eventBusWrapper: EventBusWrapper, private val accountStore: AccountStore, - private val readerTracker: ReaderTracker + private val readerTracker: ReaderTracker, + private val readerTagTableWrapper: ReaderTagTableWrapper, + private val readerBlogTableWrapper: ReaderBlogTableWrapper, ) : ScopedViewModel(bgDispatcher) { private val _subFilters = MutableLiveData>() val subFilters: LiveData> = _subFilters @@ -125,6 +126,7 @@ class SubFilterViewModel @Inject constructor( "" } } + fun loadSubFilters() { launch { val filterList = ArrayList() @@ -132,19 +134,19 @@ class SubFilterViewModel @Inject constructor( if (accountStore.hasAccessToken()) { val organization = mTagFragmentStartedWith?.organization - val followedBlogs = ReaderBlogTable.getFollowedBlogs().let { blogList -> + val followedBlogs = readerBlogTableWrapper.getFollowedBlogs().let { blogList -> // Filtering out all blogs not belonging to this VM organization if valid blogList.filter { blog -> organization?.let { blog.organizationId == organization.orgId } ?: false } - }.sortedWith(Comparator { blog1, blog2 -> + }.sortedWith { blog1, blog2 -> // sort followed blogs by name/domain to match display val blogOneName = getBlogNameForComparison(blog1) val blogTwoName = getBlogNameForComparison(blog2) blogOneName.compareTo(blogTwoName, true) - }) + } filterList.addAll( followedBlogs.map { blog -> @@ -157,7 +159,7 @@ class SubFilterViewModel @Inject constructor( ) } - val tags = ReaderTagTable.getFollowedTags() + val tags = readerTagTableWrapper.getFollowedTags() for (tag in tags) { filterList.add( @@ -239,13 +241,14 @@ class SubFilterViewModel @Inject constructor( category: SubfilterCategory, ) { updateTagsAndSites() + loadSubFilters() _bottomSheetUiState.value = Event( BottomSheetVisible( UiStringRes(category.titleRes), category ) ) - val source = when(category) { + val source = when (category) { SubfilterCategory.SITES -> "blogs" SubfilterCategory.TAGS -> "tags" } @@ -323,8 +326,8 @@ class SubFilterViewModel @Inject constructor( } fun onSubfilterSelected(subfilterListItem: SubfilterListItem) { - // We should not track subfilter selected if we're clearing a filter that is currently applied. - if (!subfilterListItem.isClearingFilter) { + // We should only track the selection of a subfilter if it's a tracked item (meaning it's a valid tag or site) + if (subfilterListItem.isTrackedItem) { val filterItemType = FilterItemType.fromSubfilterListItem(subfilterListItem) if (filterItemType != null) { readerTracker.track( @@ -332,7 +335,7 @@ class SubFilterViewModel @Inject constructor( mutableMapOf(FilterItemType.trackingEntry(filterItemType)) ) } else { - readerTracker.track(Stat.READER_FILTER_SHEET_ITEM_SELECTED,) + readerTracker.track(Stat.READER_FILTER_SHEET_ITEM_SELECTED) } } changeSubfilter(subfilterListItem, true, mTagFragmentStartedWith) @@ -354,6 +357,7 @@ class SubFilterViewModel @Inject constructor( readerTracker.stop(ReaderTrackerType.SUBFILTERED_LIST) } _currentSubFilter.value = filter + onSubfilterSelected(filter) } fun onUserComesToReader() { @@ -414,10 +418,10 @@ class SubFilterViewModel @Inject constructor( loadSubFilters() } - @Suppress("unused", "UNUSED_PARAMETER") + @Suppress("unused") @Subscribe(threadMode = ThreadMode.MAIN) fun onEventMainThread(event: ReaderEvents.FollowedBlogsFetched) { - if(event.didChange()) { + if (event.didChange()) { AppLog.d(T.READER, "Subfilter bottom sheet > followed blogs changed") loadSubFilters() } @@ -449,7 +453,7 @@ class SubFilterViewModel @Inject constructor( companion object { fun fromSubfilterListItem(subfilterListItem: SubfilterListItem): FilterItemType? = - when(subfilterListItem.type) { + when (subfilterListItem.type) { SubfilterListItem.ItemType.SITE -> Blog SubfilterListItem.ItemType.TAG -> Tag else -> null diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt index bad452b02fd0..23e3a40f8b30 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/subfilter/SubFilterViewModelTest.kt @@ -17,17 +17,20 @@ import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.analytics.AnalyticsTracker +import org.wordpress.android.datasets.ReaderBlogTableWrapper +import org.wordpress.android.datasets.wrappers.ReaderTagTableWrapper import org.wordpress.android.fluxc.model.AccountModel import org.wordpress.android.fluxc.store.AccountStore import org.wordpress.android.getOrAwaitValue import org.wordpress.android.models.ReaderBlog import org.wordpress.android.models.ReaderTag +import org.wordpress.android.models.ReaderTagList import org.wordpress.android.models.ReaderTagType.BOOKMARKED import org.wordpress.android.models.ReaderTagType.TAGS +import org.wordpress.android.ui.Organization import org.wordpress.android.ui.prefs.AppPrefsWrapper import org.wordpress.android.ui.reader.ReaderSubsActivity import org.wordpress.android.ui.reader.ReaderTypes.ReaderPostListType -import org.wordpress.android.ui.reader.services.update.ReaderUpdateLogic import org.wordpress.android.ui.reader.services.update.ReaderUpdateLogic.UpdateTask import org.wordpress.android.ui.reader.subfilter.ActionType.OpenLoginPage import org.wordpress.android.ui.reader.subfilter.ActionType.OpenSubsAtPage @@ -78,7 +81,10 @@ class SubFilterViewModelTest : BaseUnitTest() { private lateinit var savedState: Bundle @Mock - private lateinit var filter: SubfilterListItem + private lateinit var readerTagTableWrapper: ReaderTagTableWrapper + + @Mock + private lateinit var readerBlogTableWrapper: ReaderBlogTableWrapper private lateinit var viewModel: SubFilterViewModel @@ -93,7 +99,9 @@ class SubFilterViewModelTest : BaseUnitTest() { subfilterListItemMapper, eventBusWrapper, accountStore, - readerTracker + readerTracker, + readerTagTableWrapper, + readerBlogTableWrapper, ) viewModel.start(initialTag, savedTag, savedState) @@ -102,6 +110,10 @@ class SubFilterViewModelTest : BaseUnitTest() { @Test fun `current subfilter is set back when we have a previous intance state`() { val json = "{\"blogId\":0,\"feedId\":0,\"tagSlug\":\"news\",\"tagType\":1,\"type\":4}" + val filter = Site( + blog = ReaderBlog(), + onClickAction = mock(), + ) whenever(savedState.getString(SubFilterViewModel.ARG_CURRENT_SUBFILTER_JSON)).thenReturn(json) whenever(subfilterListItemMapper.fromJson(eq(json), any(), any())).thenReturn(filter) @@ -113,7 +125,9 @@ class SubFilterViewModelTest : BaseUnitTest() { subfilterListItemMapper, eventBusWrapper, accountStore, - readerTracker + readerTracker, + readerTagTableWrapper, + readerBlogTableWrapper, ) viewModel.start(initialTag, savedTag, savedState) @@ -288,11 +302,11 @@ class SubFilterViewModelTest : BaseUnitTest() { @Test fun `view model updates the tags and sites and asks to show the bottom sheet when filters button is tapped`() { - var updateTasks: EnumSet? = null - var uiState: BottomSheetUiState? = null + mockReaderTableEmpty() + + var updateTasks: EnumSet? = null viewModel.updateTagsAndSites.observeForever { updateTasks = it.peekContent() } - viewModel.bottomSheetUiState.observeForever { uiState = it.peekContent() } viewModel.onSubFiltersListButtonClicked(SubfilterCategory.SITES) @@ -302,10 +316,48 @@ class SubFilterViewModelTest : BaseUnitTest() { UpdateTask.FOLLOWED_BLOGS ) ) + } + + @Test + fun `view model asks to show the bottom sheet when filters button is tapped`() { + mockReaderTableEmpty() + + var uiState: BottomSheetUiState? = null + + viewModel.bottomSheetUiState.observeForever { uiState = it.peekContent() } + + viewModel.onSubFiltersListButtonClicked(SubfilterCategory.SITES) assertThat(uiState).isInstanceOf(BottomSheetVisible::class.java) } + @Test + fun `view model updates subfilters when filters button is tapped`() { + whenever(initialTag.organization).thenReturn(Organization.NO_ORGANIZATION) + whenever(accountStore.hasAccessToken()).thenReturn(true) + whenever(readerTagTableWrapper.getFollowedTags()).thenReturn( + ReaderTagList().apply { + add(ReaderTag("a", "a", "a", "endpoint-a", TAGS)) + add(ReaderTag("b", "b", "b", "endpoint-b", TAGS)) + add(ReaderTag("c", "c", "c", "endpoint-c", TAGS)) + } + ) + + whenever(readerBlogTableWrapper.getFollowedBlogs()).thenReturn( + List(2) { + ReaderBlog().apply { organizationId = Organization.NO_ORGANIZATION.orgId } + } + ) + + var subFilters: List? = null + + viewModel.subFilters.observeForever { subFilters = it } + + viewModel.onSubFiltersListButtonClicked(SubfilterCategory.SITES) + + assertThat(subFilters).hasSize(5) + } + @Test fun `view model hides the bottom sheet when it is cancelled`() { var uiState: BottomSheetUiState? = null @@ -320,7 +372,11 @@ class SubFilterViewModelTest : BaseUnitTest() { @Test fun `bottom sheet is hidden when a filter is tapped on`() { var uiState: BottomSheetUiState? = null - val filter: SubfilterListItem = mock() + val filter: SubfilterListItem = Site( + isSelected = false, + onClickAction = mock(), + blog = ReaderBlog(), + ) viewModel.setSubfilterFromTag(savedTag) @@ -430,9 +486,8 @@ class SubFilterViewModelTest : BaseUnitTest() { } @Test - fun `Should NOT track READER_FILTER_SHEET_ITEM_SELECTED if clearing filter when onSubfilterSelected is called`() { + fun `Should NOT track READER_FILTER_SHEET_ITEM_SELECTED if SiteAll when onSubfilterSelected is called`() { val filter = SiteAll( - isClearingFilter = true, onClickAction = {}, ) viewModel.onSubfilterSelected(filter) @@ -440,13 +495,17 @@ class SubFilterViewModelTest : BaseUnitTest() { } @Test - fun `Should track READER_FILTER_SHEET_ITEM_SELECTED if NOT clearing filter when onSubfilterSelected is called`() { - val filter = SiteAll( - isClearingFilter = false, - onClickAction = {}, - ) + fun `Should NOT track READER_FILTER_SHEET_ITEM_SELECTED if Divider when onSubfilterSelected is called`() { + val filter = SubfilterListItem.Divider + viewModel.onSubfilterSelected(filter) + verify(readerTracker, times(0)).track(AnalyticsTracker.Stat.READER_FILTER_SHEET_ITEM_SELECTED) + } + + @Test + fun `Should NOT track READER_FILTER_SHEET_ITEM_SELECTED if SectionTitle when onSubfilterSelected is called`() { + val filter = SubfilterListItem.SectionTitle(UiStringText("test")) viewModel.onSubfilterSelected(filter) - verify(readerTracker).track(AnalyticsTracker.Stat.READER_FILTER_SHEET_ITEM_SELECTED) + verify(readerTracker, times(0)).track(AnalyticsTracker.Stat.READER_FILTER_SHEET_ITEM_SELECTED) } @Test @@ -484,4 +543,11 @@ class SubFilterViewModelTest : BaseUnitTest() { assertThat(viewModel.isTitleContainerVisible.getOrAwaitValue()).isEqualTo(isTitleContainerVisible) } } + + private fun mockReaderTableEmpty() { + whenever(initialTag.organization).thenReturn(Organization.NO_ORGANIZATION) + whenever(accountStore.hasAccessToken()).thenReturn(true) + whenever(readerTagTableWrapper.getFollowedTags()).thenReturn(ReaderTagList()) + whenever(readerBlogTableWrapper.getFollowedBlogs()).thenReturn(emptyList()) + } }