From 56d609e4d0c6dbe4a14dc71c13353be28bb1bc2f Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Wed, 10 Apr 2024 16:32:54 -0300 Subject: [PATCH 01/21] Initial Fragment and ViewModel for ReaderTagsFeed This breaks the top bar filter behavior of the tags feed for now, since that requires the SubFilterViewModel to be properly initialized and communicate with the TopBar (via ReaderViewModel), which happens ONLY inside the ReaderPostListFragment for now. --- .../android/ui/reader/ReaderFragment.kt | 13 +++--- .../ui/reader/ReaderTagsFeedFragment.kt | 41 +++++++++++++++++++ .../viewmodels/ReaderTagsFeedViewModel.kt | 7 ++++ .../reader_tag_feed_fragment_layout.xml | 17 ++++++++ 4 files changed, 72 insertions(+), 6 deletions(-) create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt create mode 100644 WordPress/src/main/res/layout/reader_tag_feed_fragment_layout.xml 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 23e1b6835c44..50f5c0071217 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 @@ -226,14 +226,15 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView } childFragmentManager.beginTransaction().apply { - val fragment = if (uiState.selectedReaderTag.isDiscover) { - ReaderDiscoverFragment() - } else { - ReaderPostListFragment.newInstanceForTag( - uiState.selectedReaderTag, + val selectedTag = uiState.selectedReaderTag + val fragment = when { + selectedTag.isDiscover -> ReaderDiscoverFragment() + selectedTag.isTags -> ReaderTagsFeedFragment() + else -> ReaderPostListFragment.newInstanceForTag( + selectedTag, ReaderTypes.ReaderPostListType.TAG_FOLLOWED, true, - uiState.selectedReaderTag.isFilterable + selectedTag.isFilterable ) } replace(R.id.container, fragment, uiState.selectedReaderTag.tagSlug) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt new file mode 100644 index 000000000000..c4ee6df6bb14 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt @@ -0,0 +1,41 @@ +package org.wordpress.android.ui.reader + +import android.os.Bundle +import android.view.View +import androidx.fragment.app.viewModels +import dagger.hilt.android.AndroidEntryPoint +import org.wordpress.android.R +import org.wordpress.android.databinding.ReaderTagFeedFragmentLayoutBinding +import org.wordpress.android.ui.ViewPagerFragment +import org.wordpress.android.ui.main.WPMainActivity +import org.wordpress.android.ui.reader.viewmodels.ReaderTagsFeedViewModel + +/** + * Initial implementation of ReaderTagFeedFragment with the idea of it containing both a ComposeView, which will host + * all Compose content related to the new Tags Feed as well as an internal ReaderPostListFragment, which will be used + * to display "filtered" content based on the currently selected tag on the top app bar filter. + * + * It might be tricky to get this working properly since a lot of places expect the ReaderPostListFragment to be the + * main content of the ReaderFragment (e.g.: initializing the SubFilterViewModel), so a few changes might be needed. + */ +@AndroidEntryPoint +class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragment_layout), + WPMainActivity.OnScrollToTopListener { + private val viewModel: ReaderTagsFeedViewModel by viewModels() + + // binding + private lateinit var binding: ReaderTagFeedFragmentLayoutBinding + + override fun onViewCreated(view: View, savedInstanceState: Bundle?) { + super.onViewCreated(view, savedInstanceState) + binding = ReaderTagFeedFragmentLayoutBinding.bind(view) + } + + override fun getScrollableViewForUniqueIdProvision(): View { + return binding.composeView + } + + override fun onScrollToTop() { + // TODO scroll current content to top + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt new file mode 100644 index 000000000000..bc38dbbb331b --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt @@ -0,0 +1,7 @@ +package org.wordpress.android.ui.reader.viewmodels + +import androidx.lifecycle.ViewModel + +class ReaderTagsFeedViewModel : ViewModel() { + // TODO implement ViewModel +} diff --git a/WordPress/src/main/res/layout/reader_tag_feed_fragment_layout.xml b/WordPress/src/main/res/layout/reader_tag_feed_fragment_layout.xml new file mode 100644 index 000000000000..c2b216966121 --- /dev/null +++ b/WordPress/src/main/res/layout/reader_tag_feed_fragment_layout.xml @@ -0,0 +1,17 @@ + + + + + + + + From d0ea2ae081d2b560cf5ba19d360d2bae0e8f58fc Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Tue, 16 Apr 2024 16:36:59 -0300 Subject: [PATCH 02/21] Create ReaderPostRepository --- .../BloggingPromptsPostTagProvider.kt | 4 +- .../reader/repository/ReaderPostRepository.kt | 289 +++++++++++++++++ .../services/post/ReaderPostJobService.java | 10 +- .../reader/services/post/ReaderPostLogic.java | 293 +----------------- .../services/post/ReaderPostLogicFactory.kt | 14 + .../services/post/ReaderPostService.java | 10 +- .../BloggingPromptsPostTagProviderTest.kt | 6 +- 7 files changed, 326 insertions(+), 300 deletions(-) create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactory.kt diff --git a/WordPress/src/main/java/org/wordpress/android/ui/bloggingprompts/BloggingPromptsPostTagProvider.kt b/WordPress/src/main/java/org/wordpress/android/ui/bloggingprompts/BloggingPromptsPostTagProvider.kt index 9bac2972def3..2fcff83c7a75 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/bloggingprompts/BloggingPromptsPostTagProvider.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/bloggingprompts/BloggingPromptsPostTagProvider.kt @@ -2,7 +2,7 @@ package org.wordpress.android.ui.bloggingprompts import org.wordpress.android.models.ReaderTag import org.wordpress.android.models.ReaderTagType -import org.wordpress.android.ui.reader.services.post.ReaderPostLogic +import org.wordpress.android.ui.reader.repository.ReaderPostRepository import org.wordpress.android.ui.reader.utils.ReaderUtilsWrapper import javax.inject.Inject @@ -23,7 +23,7 @@ class BloggingPromptsPostTagProvider @Inject constructor( promptIdTag, promptIdTag, promptIdTag, - ReaderPostLogic.formatFullEndpointForTag(promptIdTag), + ReaderPostRepository.formatFullEndpointForTag(promptIdTag), ReaderTagType.FOLLOWED, ) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt new file mode 100644 index 000000000000..755c03195ed7 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt @@ -0,0 +1,289 @@ +package org.wordpress.android.ui.reader.repository + +import android.text.TextUtils +import com.android.volley.VolleyError +import com.wordpress.rest.RestRequest +import org.json.JSONObject +import org.wordpress.android.WordPress.Companion.getRestClientUtilsV1_2 +import org.wordpress.android.datasets.ReaderPostTable +import org.wordpress.android.datasets.ReaderTagTable +import org.wordpress.android.models.ReaderPost +import org.wordpress.android.models.ReaderPostList +import org.wordpress.android.models.ReaderTag +import org.wordpress.android.models.ReaderTagType +import org.wordpress.android.ui.prefs.AppPrefs +import org.wordpress.android.ui.reader.ReaderConstants +import org.wordpress.android.ui.reader.actions.ReaderActions +import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResultListener +import org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter +import org.wordpress.android.ui.reader.utils.ReaderUtils +import org.wordpress.android.util.AppLog +import org.wordpress.android.util.LocaleManagerWrapper +import org.wordpress.android.util.UrlUtils +import java.util.Locale +import javax.inject.Inject + +class ReaderPostRepository @Inject constructor( + private val localeManagerWrapper: LocaleManagerWrapper +) { + fun requestPostsWithTag( + tag: ReaderTag, + updateAction: ReaderPostServiceStarter.UpdateAction, + resultListener: UpdateResultListener + ) { + val path = getRelativeEndpointForTag(tag) + if (path.isNullOrBlank()) { + resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED) + return + } + val sb = StringBuilder(path) + + // append #posts to retrieve + sb.append("?number=").append(ReaderConstants.READER_MAX_POSTS_TO_REQUEST) + + // return newest posts first (this is the default, but make it explicit since it's important) + sb.append("&order=DESC") + + val beforeDate: String? = when (updateAction) { + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER -> { + // request posts older than the oldest existing post with this tag + ReaderPostTable.getOldestDateWithTag(tag) + } + + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP -> { + // request posts older than the post with the gap marker for this tag + ReaderPostTable.getGapMarkerDateForTag(tag) + } + + ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER, + ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> null + } + + if (!TextUtils.isEmpty(beforeDate)) { + sb.append("&before=").append(UrlUtils.urlEncode(beforeDate)) + } + sb.append("&meta=site,likes") + sb.append("&lang=").append(localeManagerWrapper.getLanguage()) + + val listener = RestRequest.Listener { jsonObject: JSONObject? -> + // remember when this tag was updated if newer posts were requested + if (updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER || + updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH + ) { + ReaderTagTable.setTagLastUpdated(tag) + } + handleUpdatePostsResponse(tag, jsonObject, updateAction, resultListener) + } + + val errorListener = RestRequest.ErrorListener { volleyError: VolleyError? -> + AppLog.e(AppLog.T.READER, volleyError) + resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED) + } + + getRestClientUtilsV1_2().get(sb.toString(), null, null, listener, errorListener) + } + + fun requestPostsForBlog( + blogId: Long, + updateAction: ReaderPostServiceStarter.UpdateAction, + resultListener: UpdateResultListener + ) { + var path = "read/sites/$blogId/posts/?meta=site,likes" + + // append the date of the oldest cached post in this blog when requesting older posts + if (updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER) { + val dateOldest = ReaderPostTable.getOldestPubDateInBlog(blogId) + if (!TextUtils.isEmpty(dateOldest)) { + path += "&before=" + UrlUtils.urlEncode(dateOldest) + } + } + val listener = RestRequest.Listener { jsonObject -> + handleUpdatePostsResponse( + null, + jsonObject, + updateAction, + resultListener + ) + } + val errorListener = RestRequest.ErrorListener { volleyError -> + AppLog.e(AppLog.T.READER, volleyError) + resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED) + } + AppLog.d(AppLog.T.READER, "updating posts in blog $blogId") + getRestClientUtilsV1_2().getWithLocale(path, null, null, listener, errorListener) + } + + fun requestPostsForFeed( + feedId: Long, + updateAction: ReaderPostServiceStarter.UpdateAction, + resultListener: UpdateResultListener + ) { + var path = "read/feed/$feedId/posts/?meta=site,likes" + if (updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER) { + val dateOldest = ReaderPostTable.getOldestPubDateInFeed(feedId) + if (!TextUtils.isEmpty(dateOldest)) { + path += "&before=" + UrlUtils.urlEncode(dateOldest) + } + } + val listener = RestRequest.Listener { jsonObject -> + handleUpdatePostsResponse( + null, + jsonObject, + updateAction, + resultListener + ) + } + val errorListener = RestRequest.ErrorListener { volleyError -> + AppLog.e(AppLog.T.READER, volleyError) + resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED) + } + AppLog.d(AppLog.T.READER, "updating posts in feed $feedId") + getRestClientUtilsV1_2().getWithLocale(path, null, null, listener, errorListener) + } + + /* + * called after requesting posts with a specific tag or in a specific blog/feed + */ + private fun handleUpdatePostsResponse( + tag: ReaderTag?, + jsonObject: JSONObject?, + updateAction: ReaderPostServiceStarter.UpdateAction, + resultListener: UpdateResultListener + ) { + if (jsonObject == null) { + resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED) + return + } + object : Thread() { + override fun run() { + val serverPosts = ReaderPostList.fromJson(jsonObject) + val updateResult = ReaderPostTable.comparePosts(serverPosts) + if (updateResult.isNewOrChanged) { + // gap detection - only applies to posts with a specific tag + var postWithGap: ReaderPost? = null + if (tag != null) { + when (updateAction) { + ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER -> { + // if there's no overlap between server and local (ie: all server + // posts are new), assume there's a gap between server and local + // provided that local posts exist + val numServerPosts = serverPosts.size + if (numServerPosts >= 2 && ReaderPostTable.getNumPostsWithTag(tag) > 0 && + !ReaderPostTable.hasOverlap( + serverPosts, + tag + ) + ) { + // treat the second to last server post as having a gap + postWithGap = serverPosts[numServerPosts - 2] + // remove the last server post to deal with the edge case of + // there actually not being a gap between local & server + serverPosts.removeAt(numServerPosts - 1) + val gapMarker = ReaderPostTable.getGapMarkerIdsForTag(tag) + if (gapMarker != null) { + // We mustn't have two gapMarkers at the same time. Therefor we need to + // delete all posts before the current gapMarker and clear the gapMarker flag. + ReaderPostTable.deletePostsBeforeGapMarkerForTag(tag) + ReaderPostTable.removeGapMarkerForTag(tag) + } + } + } + + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP -> { + // if service was started as a request to fill a gap, delete existing posts + // before the one with the gap marker, then remove the existing gap marker + ReaderPostTable.deletePostsBeforeGapMarkerForTag(tag) + ReaderPostTable.removeGapMarkerForTag(tag) + } + + ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> ReaderPostTable.deletePostsWithTag( + tag + ) + + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER -> {} + } + } + ReaderPostTable.addOrUpdatePosts(tag, serverPosts) + if (AppPrefs.shouldUpdateBookmarkPostsPseudoIds(tag)) { + ReaderPostTable.updateBookmarkedPostPseudoId(serverPosts) + AppPrefs.setBookmarkPostsPseudoIdsUpdated() + } + + // gap marker must be set after saving server posts + if (postWithGap != null) { + ReaderPostTable.setGapMarkerForTag(postWithGap.blogId, postWithGap.postId, tag) + AppLog.d(AppLog.T.READER, "added gap marker to tag " + tag!!.tagNameForLog) + } + } else if (updateResult == ReaderActions.UpdateResult.UNCHANGED + && updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP + ) { + // edge case - request to fill gap returned nothing new, so remove the gap marker + ReaderPostTable.removeGapMarkerForTag(tag) + AppLog.w(AppLog.T.READER, "attempt to fill gap returned nothing new") + } + AppLog.d( + AppLog.T.READER, + "requested posts response = $updateResult" + ) + resultListener.onUpdateResult(updateResult) + } + }.start() + } + + /* + * returns the endpoint to use when requesting posts with the passed tag + */ + private fun getRelativeEndpointForTag(tag: ReaderTag): String? { + val endpoint = tag.endpoint?.takeIf { it.isNotBlank() } // if passed tag has an assigned endpoint, use it + ?: ReaderTagTable.getEndpointForTag(tag)?.takeIf { it.isNotBlank() } // check the db for the endpoint + + return endpoint + ?.let { getRelativeEndpoint(it) } + ?: if (tag.tagType == ReaderTagType.DEFAULT) { + // never hand craft the endpoint for default tags, since these MUST be updated using their endpoints + null + } else { + formatRelativeEndpointForTag(tag.tagSlug) + } + } + + private fun formatRelativeEndpointForTag(tagSlug: String): String { + return String.format(Locale.US, "read/tags/%s/posts", ReaderUtils.sanitizeWithDashes(tagSlug)) + } + + /* + * returns the passed endpoint without the unnecessary path - this is + * needed because as of 20-Feb-2015 the /read/menu/ call returns the + * full path but we don't want to use the full path since it may change + * between API versions (as it did when we moved from v1 to v1.1) + * + * ex: https://public-api.wordpress.com/rest/v1/read/tags/fitness/posts + * becomes just read/tags/fitness/posts + */ + @Suppress("MagicNumber") + private fun getRelativeEndpoint(endpoint: String): String { + return endpoint.takeIf { it.startsWith("http") } + ?.let { + var pos = it.indexOf("/read/") + if (pos > -1) { + return@let it.substring(pos + 1) + } + pos = it.indexOf("/v1/") + if (pos > -1) { + return@let it.substring(pos + 4) + } + return@let it + } + ?: endpoint + } + + companion object { + private fun formatRelativeEndpointForTag(tagSlug: String): String { + return String.format(Locale.US, "read/tags/%s/posts", ReaderUtils.sanitizeWithDashes(tagSlug)) + } + + fun formatFullEndpointForTag(tagSlug: String): String { + return (getRestClientUtilsV1_2().restClient.endpointURL + formatRelativeEndpointForTag(tagSlug)) + } + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostJobService.java b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostJobService.java index 70a2ed80efb8..b0cfb5d519db 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostJobService.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostJobService.java @@ -10,12 +10,9 @@ import org.wordpress.android.ui.reader.ReaderEvents; import org.wordpress.android.ui.reader.services.ServiceCompletionListener; import org.wordpress.android.util.AppLog; -import org.wordpress.android.util.LocaleManagerWrapper; import javax.inject.Inject; -import dagger.hilt.android.AndroidEntryPoint; - import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.ARG_ACTION; import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.ARG_BLOG_ID; import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.ARG_FEED_ID; @@ -26,6 +23,8 @@ import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.ARG_TAG_PARAM_TITLE; import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.UpdateAction; +import dagger.hilt.android.AndroidEntryPoint; + /** * service which updates posts with specific tags or in specific blogs/feeds - relies on * EventBus to alert of update status @@ -33,10 +32,9 @@ @AndroidEntryPoint public class ReaderPostJobService extends JobService implements ServiceCompletionListener { + @Inject ReaderPostLogicFactory mPostLogicFactory; private ReaderPostLogic mReaderPostLogic; - @Inject LocaleManagerWrapper mLocaleManagerWrapper; - @Override public boolean onStartJob(JobParameters params) { AppLog.i(AppLog.T.READER, "reader post job service > started"); UpdateAction action; @@ -74,7 +72,7 @@ public class ReaderPostJobService extends JobService implements ServiceCompletio @Override public void onCreate() { super.onCreate(); - mReaderPostLogic = new ReaderPostLogic(this, mLocaleManagerWrapper); + mReaderPostLogic = mPostLogicFactory.create(this); AppLog.i(AppLog.T.READER, "reader post job service > created"); } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogic.java b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogic.java index 212d32230fab..d3b9dd20a8dd 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogic.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogic.java @@ -1,43 +1,26 @@ package org.wordpress.android.ui.reader.services.post; -import android.text.TextUtils; - import androidx.annotation.NonNull; -import com.android.volley.VolleyError; -import com.wordpress.rest.RestRequest; - import org.greenrobot.eventbus.EventBus; -import org.json.JSONObject; -import org.wordpress.android.WordPress; -import org.wordpress.android.datasets.ReaderPostTable; -import org.wordpress.android.datasets.ReaderTagTable; -import org.wordpress.android.models.ReaderPost; -import org.wordpress.android.models.ReaderPostList; import org.wordpress.android.models.ReaderTag; -import org.wordpress.android.models.ReaderTagType; -import org.wordpress.android.ui.prefs.AppPrefs; -import org.wordpress.android.ui.reader.ReaderConstants; import org.wordpress.android.ui.reader.ReaderEvents; import org.wordpress.android.ui.reader.actions.ReaderActions; -import org.wordpress.android.ui.reader.models.ReaderBlogIdPostId; +import org.wordpress.android.ui.reader.repository.ReaderPostRepository; import org.wordpress.android.ui.reader.services.ServiceCompletionListener; import org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.UpdateAction; -import org.wordpress.android.ui.reader.utils.ReaderUtils; -import org.wordpress.android.util.AppLog; -import org.wordpress.android.util.LocaleManagerWrapper; -import org.wordpress.android.util.StringUtils; -import org.wordpress.android.util.UrlUtils; public class ReaderPostLogic { - private ServiceCompletionListener mCompletionListener; - private final LocaleManagerWrapper mLocaleManagerWrapper; + @NonNull + private final ServiceCompletionListener mCompletionListener; + @NonNull + private final ReaderPostRepository mReaderPostRepository; private Object mListenerCompanion; public ReaderPostLogic(@NonNull final ServiceCompletionListener listener, - @NonNull final LocaleManagerWrapper localeManagerWrapper) { + @NonNull final ReaderPostRepository readerPostRepository) { mCompletionListener = listener; - mLocaleManagerWrapper = localeManagerWrapper; + mReaderPostRepository = readerPostRepository; } public void performTask(Object companion, UpdateAction action, @@ -55,9 +38,8 @@ public void performTask(Object companion, UpdateAction action, } } - private void updatePostsWithTag(final ReaderTag tag, final UpdateAction action) { - requestPostsWithTag( + mReaderPostRepository.requestPostsWithTag( tag, action, new ReaderActions.UpdateResultListener() { @@ -77,7 +59,7 @@ public void onUpdateResult(ReaderActions.UpdateResult result) { mCompletionListener.onCompleted(mListenerCompanion); } }; - requestPostsForBlog(blogId, action, listener); + mReaderPostRepository.requestPostsForBlog(blogId, action, listener); } private void updatePostsInFeed(long feedId, final UpdateAction action) { @@ -88,261 +70,6 @@ public void onUpdateResult(ReaderActions.UpdateResult result) { mCompletionListener.onCompleted(mListenerCompanion); } }; - requestPostsForFeed(feedId, action, listener); - } - - private void requestPostsWithTag(final ReaderTag tag, - final UpdateAction updateAction, - final ReaderActions.UpdateResultListener resultListener) { - String path = getRelativeEndpointForTag(tag); - if (TextUtils.isEmpty(path)) { - resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED); - return; - } - - StringBuilder sb = new StringBuilder(path); - - // append #posts to retrieve - sb.append("?number=").append(ReaderConstants.READER_MAX_POSTS_TO_REQUEST); - - // return newest posts first (this is the default, but make it explicit since it's important) - sb.append("&order=DESC"); - - String beforeDate; - switch (updateAction) { - case REQUEST_OLDER: - // request posts older than the oldest existing post with this tag - beforeDate = ReaderPostTable.getOldestDateWithTag(tag); - break; - case REQUEST_OLDER_THAN_GAP: - // request posts older than the post with the gap marker for this tag - beforeDate = ReaderPostTable.getGapMarkerDateForTag(tag); - break; - case REQUEST_NEWER: - case REQUEST_REFRESH: - default: - beforeDate = null; - break; - } - if (!TextUtils.isEmpty(beforeDate)) { - sb.append("&before=").append(UrlUtils.urlEncode(beforeDate)); - } - - sb.append("&meta=site,likes"); - - sb.append("&lang=").append(mLocaleManagerWrapper.getLanguage()); - - com.wordpress.rest.RestRequest.Listener listener = jsonObject -> { - // remember when this tag was updated if newer posts were requested - if (updateAction == UpdateAction.REQUEST_NEWER || updateAction == UpdateAction.REQUEST_REFRESH) { - ReaderTagTable.setTagLastUpdated(tag); - } - handleUpdatePostsResponse(tag, jsonObject, updateAction, resultListener); - }; - RestRequest.ErrorListener errorListener = volleyError -> { - AppLog.e(AppLog.T.READER, volleyError); - resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED); - }; - - WordPress.getRestClientUtilsV1_2().get(sb.toString(), null, null, listener, errorListener); - } - - private static void requestPostsForBlog(final long blogId, - final UpdateAction updateAction, - final ReaderActions.UpdateResultListener resultListener) { - String path = "read/sites/" + blogId + "/posts/?meta=site,likes"; - - // append the date of the oldest cached post in this blog when requesting older posts - if (updateAction == UpdateAction.REQUEST_OLDER) { - String dateOldest = ReaderPostTable.getOldestPubDateInBlog(blogId); - if (!TextUtils.isEmpty(dateOldest)) { - path += "&before=" + UrlUtils.urlEncode(dateOldest); - } - } - - com.wordpress.rest.RestRequest.Listener listener = new RestRequest.Listener() { - @Override - public void onResponse(JSONObject jsonObject) { - handleUpdatePostsResponse(null, jsonObject, updateAction, resultListener); - } - }; - RestRequest.ErrorListener errorListener = new RestRequest.ErrorListener() { - @Override - public void onErrorResponse(VolleyError volleyError) { - AppLog.e(AppLog.T.READER, volleyError); - resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED); - } - }; - AppLog.d(AppLog.T.READER, "updating posts in blog " + blogId); - WordPress.getRestClientUtilsV1_2().getWithLocale(path, null, null, listener, errorListener); - } - - private static void requestPostsForFeed(final long feedId, - final UpdateAction updateAction, - final ReaderActions.UpdateResultListener resultListener) { - String path = "read/feed/" + feedId + "/posts/?meta=site,likes"; - if (updateAction == UpdateAction.REQUEST_OLDER) { - String dateOldest = ReaderPostTable.getOldestPubDateInFeed(feedId); - if (!TextUtils.isEmpty(dateOldest)) { - path += "&before=" + UrlUtils.urlEncode(dateOldest); - } - } - - com.wordpress.rest.RestRequest.Listener listener = new RestRequest.Listener() { - @Override - public void onResponse(JSONObject jsonObject) { - handleUpdatePostsResponse(null, jsonObject, updateAction, resultListener); - } - }; - RestRequest.ErrorListener errorListener = new RestRequest.ErrorListener() { - @Override - public void onErrorResponse(VolleyError volleyError) { - AppLog.e(AppLog.T.READER, volleyError); - resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED); - } - }; - - AppLog.d(AppLog.T.READER, "updating posts in feed " + feedId); - WordPress.getRestClientUtilsV1_2().getWithLocale(path, null, null, listener, errorListener); - } - - /* - * called after requesting posts with a specific tag or in a specific blog/feed - */ - private static void handleUpdatePostsResponse(final ReaderTag tag, - final JSONObject jsonObject, - final UpdateAction updateAction, - final ReaderActions.UpdateResultListener resultListener) { - if (jsonObject == null) { - resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED); - return; - } - - new Thread() { - @Override - public void run() { - ReaderPostList serverPosts = ReaderPostList.fromJson(jsonObject); - ReaderActions.UpdateResult updateResult = ReaderPostTable.comparePosts(serverPosts); - if (updateResult.isNewOrChanged()) { - // gap detection - only applies to posts with a specific tag - ReaderPost postWithGap = null; - if (tag != null) { - switch (updateAction) { - case REQUEST_NEWER: - // if there's no overlap between server and local (ie: all server - // posts are new), assume there's a gap between server and local - // provided that local posts exist - int numServerPosts = serverPosts.size(); - if (numServerPosts >= 2 - && ReaderPostTable.getNumPostsWithTag(tag) > 0 - && !ReaderPostTable.hasOverlap(serverPosts, tag)) { - // treat the second to last server post as having a gap - postWithGap = serverPosts.get(numServerPosts - 2); - // remove the last server post to deal with the edge case of - // there actually not being a gap between local & server - serverPosts.remove(numServerPosts - 1); - ReaderBlogIdPostId gapMarker = ReaderPostTable.getGapMarkerIdsForTag(tag); - if (gapMarker != null) { - // We mustn't have two gapMarkers at the same time. Therefor we need to - // delete all posts before the current gapMarker and clear the gapMarker flag. - ReaderPostTable.deletePostsBeforeGapMarkerForTag(tag); - ReaderPostTable.removeGapMarkerForTag(tag); - } - } - break; - case REQUEST_OLDER_THAN_GAP: - // if service was started as a request to fill a gap, delete existing posts - // before the one with the gap marker, then remove the existing gap marker - ReaderPostTable.deletePostsBeforeGapMarkerForTag(tag); - ReaderPostTable.removeGapMarkerForTag(tag); - break; - case REQUEST_REFRESH: - ReaderPostTable.deletePostsWithTag(tag); - break; - case REQUEST_OLDER: - // no-op - break; - } - } - ReaderPostTable.addOrUpdatePosts(tag, serverPosts); - if (AppPrefs.shouldUpdateBookmarkPostsPseudoIds(tag)) { - ReaderPostTable.updateBookmarkedPostPseudoId(serverPosts); - AppPrefs.setBookmarkPostsPseudoIdsUpdated(); - } - - // gap marker must be set after saving server posts - if (postWithGap != null) { - ReaderPostTable.setGapMarkerForTag(postWithGap.blogId, postWithGap.postId, tag); - AppLog.d(AppLog.T.READER, "added gap marker to tag " + tag.getTagNameForLog()); - } - } else if (updateResult == ReaderActions.UpdateResult.UNCHANGED - && updateAction == UpdateAction.REQUEST_OLDER_THAN_GAP) { - // edge case - request to fill gap returned nothing new, so remove the gap marker - ReaderPostTable.removeGapMarkerForTag(tag); - AppLog.w(AppLog.T.READER, "attempt to fill gap returned nothing new"); - } - AppLog.d(AppLog.T.READER, "requested posts response = " + updateResult.toString()); - resultListener.onUpdateResult(updateResult); - } - }.start(); - } - - /* - * returns the endpoint to use when requesting posts with the passed tag - */ - private static String getRelativeEndpointForTag(ReaderTag tag) { - if (tag == null) { - return null; - } - - // if passed tag has an assigned endpoint, return it and be done - if (!TextUtils.isEmpty(tag.getEndpoint())) { - return getRelativeEndpoint(tag.getEndpoint()); - } - - // check the db for the endpoint - String endpoint = ReaderTagTable.getEndpointForTag(tag); - if (!TextUtils.isEmpty(endpoint)) { - return getRelativeEndpoint(endpoint); - } - - // never hand craft the endpoint for default tags, since these MUST be updated - // using their stored endpoints - if (tag.tagType == ReaderTagType.DEFAULT) { - return null; - } - return formatRelativeEndpointForTag(tag.getTagSlug()); - } - - private static String formatRelativeEndpointForTag(@NonNull final String tagSlug) { - return String.format("read/tags/%s/posts", ReaderUtils.sanitizeWithDashes(tagSlug)); - } - - public static String formatFullEndpointForTag(@NonNull final String tagSlug) { - return WordPress.getRestClientUtilsV1_2().getRestClient().getEndpointURL() - + formatRelativeEndpointForTag(tagSlug); - } - - /* - * returns the passed endpoint without the unnecessary path - this is - * needed because as of 20-Feb-2015 the /read/menu/ call returns the - * full path but we don't want to use the full path since it may change - * between API versions (as it did when we moved from v1 to v1.1) - * - * ex: https://public-api.wordpress.com/rest/v1/read/tags/fitness/posts - * becomes just read/tags/fitness/posts - */ - private static String getRelativeEndpoint(final String endpoint) { - if (endpoint != null && endpoint.startsWith("http")) { - int pos = endpoint.indexOf("/read/"); - if (pos > -1) { - return endpoint.substring(pos + 1, endpoint.length()); - } - pos = endpoint.indexOf("/v1/"); - if (pos > -1) { - return endpoint.substring(pos + 4, endpoint.length()); - } - } - return StringUtils.notNullStr(endpoint); + mReaderPostRepository.requestPostsForFeed(feedId, action, listener); } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactory.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactory.kt new file mode 100644 index 000000000000..6d64d4b5117d --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactory.kt @@ -0,0 +1,14 @@ +package org.wordpress.android.ui.reader.services.post + +import org.wordpress.android.ui.reader.repository.ReaderPostRepository +import org.wordpress.android.ui.reader.services.ServiceCompletionListener +import javax.inject.Inject + +class ReaderPostLogicFactory @Inject constructor( + private val readerPostRepository: ReaderPostRepository, +) { + fun create(listener: ServiceCompletionListener) = ReaderPostLogic( + listener, + readerPostRepository, + ) +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostService.java b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostService.java index 5753d9ce4cb7..8312575d0124 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostService.java +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostService.java @@ -9,18 +9,17 @@ import org.wordpress.android.ui.reader.ReaderEvents; import org.wordpress.android.ui.reader.services.ServiceCompletionListener; import org.wordpress.android.util.AppLog; -import org.wordpress.android.util.LocaleManagerWrapper; import javax.inject.Inject; -import dagger.hilt.android.AndroidEntryPoint; - import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.ARG_ACTION; import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.ARG_BLOG_ID; import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.ARG_FEED_ID; import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.ARG_TAG; import static org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter.UpdateAction; +import dagger.hilt.android.AndroidEntryPoint; + /** * service which updates posts with specific tags or in specific blogs/feeds - relies on * EventBus to alert of update status @@ -28,10 +27,9 @@ @AndroidEntryPoint public class ReaderPostService extends Service implements ServiceCompletionListener { + @Inject ReaderPostLogicFactory mPostLogicFactory; private ReaderPostLogic mReaderPostLogic; - @Inject LocaleManagerWrapper mLocaleManagerWrapper; - @Override public IBinder onBind(Intent intent) { return null; @@ -40,7 +38,7 @@ public IBinder onBind(Intent intent) { @Override public void onCreate() { super.onCreate(); - mReaderPostLogic = new ReaderPostLogic(this, mLocaleManagerWrapper); + mReaderPostLogic = mPostLogicFactory.create(this); AppLog.i(AppLog.T.READER, "reader post service > created"); } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/bloggingprompts/BloggingPromptsPostTagProviderTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/bloggingprompts/BloggingPromptsPostTagProviderTest.kt index 73250ba0c7f5..5b1fc3546c74 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/bloggingprompts/BloggingPromptsPostTagProviderTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/bloggingprompts/BloggingPromptsPostTagProviderTest.kt @@ -10,7 +10,7 @@ import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.models.ReaderTag import org.wordpress.android.models.ReaderTagType -import org.wordpress.android.ui.reader.services.post.ReaderPostLogic +import org.wordpress.android.ui.reader.repository.ReaderPostRepository import org.wordpress.android.ui.reader.utils.ReaderUtilsWrapper import kotlin.test.assertEquals @@ -47,7 +47,7 @@ class BloggingPromptsPostTagProviderTest : BaseUnitTest() { BLOGGING_PROMPT_ID_TAG, BLOGGING_PROMPT_ID_TAG, BLOGGING_PROMPT_ID_TAG, - ReaderPostLogic.formatFullEndpointForTag(BLOGGING_PROMPT_ID_TAG), + ReaderPostRepository.formatFullEndpointForTag(BLOGGING_PROMPT_ID_TAG), ReaderTagType.FOLLOWED, ) val actual = tagProvider.promptSearchReaderTag("valid-url") @@ -61,7 +61,7 @@ class BloggingPromptsPostTagProviderTest : BaseUnitTest() { BLOGGING_PROMPT_TAG, BLOGGING_PROMPT_TAG, BLOGGING_PROMPT_TAG, - ReaderPostLogic.formatFullEndpointForTag(BLOGGING_PROMPT_TAG), + ReaderPostRepository.formatFullEndpointForTag(BLOGGING_PROMPT_TAG), ReaderTagType.FOLLOWED, ) val actual = tagProvider.promptSearchReaderTag("invalid-url") From 8145b0abd5093e7e8b9e14543c08ec3309a8ece5 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Tue, 16 Apr 2024 17:13:24 -0300 Subject: [PATCH 03/21] Replace StringUtils by kotlin extensions --- .../android/ui/reader/repository/ReaderPostRepository.kt | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt index 755c03195ed7..448ca5860b1e 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt @@ -1,6 +1,5 @@ package org.wordpress.android.ui.reader.repository -import android.text.TextUtils import com.android.volley.VolleyError import com.wordpress.rest.RestRequest import org.json.JSONObject @@ -59,7 +58,7 @@ class ReaderPostRepository @Inject constructor( ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> null } - if (!TextUtils.isEmpty(beforeDate)) { + if (!beforeDate.isNullOrBlank()) { sb.append("&before=").append(UrlUtils.urlEncode(beforeDate)) } sb.append("&meta=site,likes") @@ -93,7 +92,7 @@ class ReaderPostRepository @Inject constructor( // append the date of the oldest cached post in this blog when requesting older posts if (updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER) { val dateOldest = ReaderPostTable.getOldestPubDateInBlog(blogId) - if (!TextUtils.isEmpty(dateOldest)) { + if (!dateOldest.isNullOrBlank()) { path += "&before=" + UrlUtils.urlEncode(dateOldest) } } @@ -121,7 +120,7 @@ class ReaderPostRepository @Inject constructor( var path = "read/feed/$feedId/posts/?meta=site,likes" if (updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER) { val dateOldest = ReaderPostTable.getOldestPubDateInFeed(feedId) - if (!TextUtils.isEmpty(dateOldest)) { + if (!dateOldest.isNullOrBlank()) { path += "&before=" + UrlUtils.urlEncode(dateOldest) } } From e596f1cf9a73daf7a7c7c2e9633fad26f30a049c Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Wed, 17 Apr 2024 18:12:18 -0300 Subject: [PATCH 04/21] Create function to fetch newer posts for tag --- .../reader/repository/ReaderPostRepository.kt | 27 ++++++++++++++++--- 1 file changed, 24 insertions(+), 3 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt index 448ca5860b1e..7e4a98959fee 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt @@ -2,6 +2,7 @@ package org.wordpress.android.ui.reader.repository import com.android.volley.VolleyError import com.wordpress.rest.RestRequest +import kotlinx.coroutines.suspendCancellableCoroutine import org.json.JSONObject import org.wordpress.android.WordPress.Companion.getRestClientUtilsV1_2 import org.wordpress.android.datasets.ReaderPostTable @@ -21,10 +22,30 @@ import org.wordpress.android.util.LocaleManagerWrapper import org.wordpress.android.util.UrlUtils import java.util.Locale import javax.inject.Inject +import kotlin.coroutines.resume +import kotlin.coroutines.resumeWithException class ReaderPostRepository @Inject constructor( private val localeManagerWrapper: LocaleManagerWrapper ) { + /** + * Fetches and returns the most recent posts for the passed tag, respecting the maxPosts limit. + * It always fetches the most recent posts, saves them to the local DB and returns the latest from that cache. + */ + suspend fun fetchNewerPostsForTag(tag: ReaderTag, maxPosts: Int = 7): ReaderPostList { + return suspendCancellableCoroutine { cont -> + val resultListener = UpdateResultListener { result -> + if (result == ReaderActions.UpdateResult.FAILED) { + cont.resumeWithException(Exception("Failed to fetch newer posts for tag")) + } else { + val posts = ReaderPostTable.getPostsWithTag(tag, maxPosts, false) + cont.resume(posts) + } + } + requestPostsWithTag(tag, ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER, resultListener) + } + } + fun requestPostsWithTag( tag: ReaderTag, updateAction: ReaderPostServiceStarter.UpdateAction, @@ -140,7 +161,7 @@ class ReaderPostRepository @Inject constructor( getRestClientUtilsV1_2().getWithLocale(path, null, null, listener, errorListener) } - /* + /** * called after requesting posts with a specific tag or in a specific blog/feed */ private fun handleUpdatePostsResponse( @@ -229,7 +250,7 @@ class ReaderPostRepository @Inject constructor( }.start() } - /* + /** * returns the endpoint to use when requesting posts with the passed tag */ private fun getRelativeEndpointForTag(tag: ReaderTag): String? { @@ -250,7 +271,7 @@ class ReaderPostRepository @Inject constructor( return String.format(Locale.US, "read/tags/%s/posts", ReaderUtils.sanitizeWithDashes(tagSlug)) } - /* + /** * returns the passed endpoint without the unnecessary path - this is * needed because as of 20-Feb-2015 the /read/menu/ call returns the * full path but we don't want to use the full path since it may change From 23a0771a49b31d69e00fcf1fe96698cb0854f4d3 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Wed, 17 Apr 2024 18:12:40 -0300 Subject: [PATCH 05/21] Create ViewModel methods to fetch posts for Tags Feed --- .../viewmodels/ReaderTagsFeedViewModel.kt | 66 ++++++++++++++++++- 1 file changed, 63 insertions(+), 3 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt index bc38dbbb331b..0ec39b37a6fd 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt @@ -1,7 +1,67 @@ package org.wordpress.android.ui.reader.viewmodels -import androidx.lifecycle.ViewModel +import dagger.hilt.android.lifecycle.HiltViewModel +import kotlinx.coroutines.CoroutineDispatcher +import kotlinx.coroutines.flow.MutableStateFlow +import kotlinx.coroutines.flow.StateFlow +import kotlinx.coroutines.flow.update +import org.wordpress.android.models.ReaderPostList +import org.wordpress.android.models.ReaderTag +import org.wordpress.android.models.ReaderTagType +import org.wordpress.android.modules.BG_THREAD +import org.wordpress.android.ui.reader.repository.ReaderPostRepository +import org.wordpress.android.viewmodel.ScopedViewModel +import javax.inject.Inject +import javax.inject.Named -class ReaderTagsFeedViewModel : ViewModel() { - // TODO implement ViewModel +@HiltViewModel +class ReaderTagsFeedViewModel @Inject constructor( + @Named(BG_THREAD) private val bgDispatcher: CoroutineDispatcher, + private val readerPostRepository: ReaderPostRepository, +) : ScopedViewModel(bgDispatcher) { + private val _uiStateFlow = MutableStateFlow(UiState(emptyMap())) + val uiStateFlow: StateFlow = _uiStateFlow + + fun fetchAll() { + FAKE_TAGS.forEach { + fetchTag(it) + } + } + + fun fetchTag(tag: ReaderTag) { + launch { + _uiStateFlow.update { + it.copy(tagStates = it.tagStates + (tag to FetchState.Loading)) + } + + try { + val posts = readerPostRepository.fetchNewerPostsForTag(tag) + _uiStateFlow.update { + it.copy(tagStates = it.tagStates + (tag to FetchState.Success(posts))) + } + } catch (e: Exception) { + _uiStateFlow.update { + it.copy(tagStates = it.tagStates + (tag to FetchState.Error)) + } + } + } + } + + data class UiState( + val tagStates: Map, + ) + + sealed class FetchState { + data object Loading : FetchState() + data object Error : FetchState() + data class Success(val posts: ReaderPostList) : FetchState() + } + + companion object { + private val FAKE_TAGS = listOf( + ReaderTag("science", "Science", "Science", null, ReaderTagType.FOLLOWED), + ReaderTag("fiction", "Fiction", "Fiction", null, ReaderTagType.FOLLOWED), + ReaderTag("rpg", "RPG", "RPG", null, ReaderTagType.FOLLOWED), + ) + } } From d95ab6ff33364dcebf95db1190118db1cfcb75c3 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Wed, 17 Apr 2024 18:13:02 -0300 Subject: [PATCH 06/21] Create temporary UI for testing tag fetching --- .../ui/reader/ReaderTagsFeedFragment.kt | 108 ++++++++++++++++++ 1 file changed, 108 insertions(+) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt index c4ee6df6bb14..82b814782c21 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt @@ -2,11 +2,31 @@ package org.wordpress.android.ui.reader import android.os.Bundle import android.view.View +import androidx.compose.foundation.background +import androidx.compose.foundation.horizontalScroll +import androidx.compose.foundation.layout.Arrangement +import androidx.compose.foundation.layout.Column +import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxWidth +import androidx.compose.foundation.layout.padding +import androidx.compose.foundation.layout.width +import androidx.compose.foundation.rememberScrollState +import androidx.compose.foundation.shape.RoundedCornerShape +import androidx.compose.foundation.verticalScroll +import androidx.compose.material.MaterialTheme +import androidx.compose.material.Text +import androidx.compose.runtime.Composable +import androidx.compose.runtime.collectAsState +import androidx.compose.runtime.getValue +import androidx.compose.ui.Modifier +import androidx.compose.ui.text.style.TextOverflow +import androidx.compose.ui.unit.dp import androidx.fragment.app.viewModels import dagger.hilt.android.AndroidEntryPoint import org.wordpress.android.R import org.wordpress.android.databinding.ReaderTagFeedFragmentLayoutBinding import org.wordpress.android.ui.ViewPagerFragment +import org.wordpress.android.ui.compose.theme.AppThemeWithoutBackground import org.wordpress.android.ui.main.WPMainActivity import org.wordpress.android.ui.reader.viewmodels.ReaderTagsFeedViewModel @@ -29,6 +49,15 @@ class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragme override fun onViewCreated(view: View, savedInstanceState: Bundle?) { super.onViewCreated(view, savedInstanceState) binding = ReaderTagFeedFragmentLayoutBinding.bind(view) + + binding.composeView.setContent { + AppThemeWithoutBackground { + val uiState by viewModel.uiStateFlow.collectAsState() + ReaderTagsFeedScreen(uiState) + } + } + + viewModel.fetchAll() } override fun getScrollableViewForUniqueIdProvision(): View { @@ -39,3 +68,82 @@ class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragme // TODO scroll current content to top } } + +/** + * Throwaway UI code just for testing the initial Tags Feed fetching code. + */ +@Composable +private fun ReaderTagsFeedScreen( + uiState: ReaderTagsFeedViewModel.UiState, +) { + AppThemeWithoutBackground { + Column( + modifier = Modifier + .verticalScroll(rememberScrollState()) + .padding(16.dp), + verticalArrangement = Arrangement.spacedBy(8.dp), + ) { + uiState.tagStates.forEach { (tag, fetchState) -> + Column( + verticalArrangement = Arrangement.spacedBy(4.dp), + ) { + Text( + text = tag.tagTitle, + style = MaterialTheme.typography.h4, + ) + + when (fetchState) { + is ReaderTagsFeedViewModel.FetchState.Loading -> { + Text( + text = "Loading...", + style = MaterialTheme.typography.body1, + ) + } + + is ReaderTagsFeedViewModel.FetchState.Error -> { + Text( + text = "Error loading posts", + style = MaterialTheme.typography.body1, + ) + } + + is ReaderTagsFeedViewModel.FetchState.Success -> { + Row( + modifier = Modifier + .fillMaxWidth() + .horizontalScroll(rememberScrollState()), + ) { + fetchState.posts.forEach { post -> + Column( + modifier = Modifier + .width(300.dp) + .background( + MaterialTheme.colors.surface, + RoundedCornerShape(4.dp) + ) + .padding(4.dp), + verticalArrangement = Arrangement.spacedBy(4.dp), + ) { + Text( + text = post.title, + style = MaterialTheme.typography.h5, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + ) + + Text( + text = post.excerpt, + style = MaterialTheme.typography.body1, + maxLines = 4, + overflow = TextOverflow.Ellipsis, + ) + } + } + } + } + } + } + } + } + } +} From 3a8bbfd62cc4f89e6f0b34acd1a2705c1cc3886a Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 19 Apr 2024 13:57:56 -0300 Subject: [PATCH 07/21] Fetch content for the actual followed tags --- .../ui/reader/ReaderTagsFeedFragment.kt | 174 ++++++++++++------ .../viewmodels/ReaderTagsFeedViewModel.kt | 13 +- 2 files changed, 117 insertions(+), 70 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt index 82b814782c21..d4c81cde82b6 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt @@ -7,6 +7,7 @@ import androidx.compose.foundation.horizontalScroll import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column import androidx.compose.foundation.layout.Row +import androidx.compose.foundation.layout.fillMaxHeight import androidx.compose.foundation.layout.fillMaxWidth import androidx.compose.foundation.layout.padding import androidx.compose.foundation.layout.width @@ -22,13 +23,23 @@ import androidx.compose.ui.Modifier import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import androidx.fragment.app.viewModels +import androidx.lifecycle.ViewModelProvider import dagger.hilt.android.AndroidEntryPoint import org.wordpress.android.R import org.wordpress.android.databinding.ReaderTagFeedFragmentLayoutBinding +import org.wordpress.android.models.ReaderTag +import org.wordpress.android.models.ReaderTagType import org.wordpress.android.ui.ViewPagerFragment import org.wordpress.android.ui.compose.theme.AppThemeWithoutBackground import org.wordpress.android.ui.main.WPMainActivity +import org.wordpress.android.ui.reader.services.update.ReaderUpdateServiceStarter +import org.wordpress.android.ui.reader.subfilter.SubFilterViewModel +import org.wordpress.android.ui.reader.subfilter.SubFilterViewModel.Companion.getViewModelKeyForTag +import org.wordpress.android.ui.reader.subfilter.SubfilterListItem import org.wordpress.android.ui.reader.viewmodels.ReaderTagsFeedViewModel +import org.wordpress.android.ui.reader.viewmodels.ReaderViewModel +import org.wordpress.android.util.NetworkUtils +import javax.inject.Inject /** * Initial implementation of ReaderTagFeedFragment with the idea of it containing both a ComposeView, which will host @@ -41,7 +52,25 @@ import org.wordpress.android.ui.reader.viewmodels.ReaderTagsFeedViewModel @AndroidEntryPoint class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragment_layout), WPMainActivity.OnScrollToTopListener { + // TODO thomashortadev get this via Fragment arguments + private val tagsFeedTag by lazy { + ReaderTag( + "", + getString(R.string.reader_tags_display_name), + getString(R.string.reader_tags_display_name), + "", + ReaderTagType.TAGS + ) + } + + @Inject + lateinit var viewModelFactory: ViewModelProvider.Factory + private lateinit var subFilterViewModel: SubFilterViewModel + private val viewModel: ReaderTagsFeedViewModel by viewModels() + private val readerViewModel: ReaderViewModel by viewModels( + ownerProducer = { requireParentFragment() } + ) // binding private lateinit var binding: ReaderTagFeedFragmentLayoutBinding @@ -57,7 +86,34 @@ class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragme } } - viewModel.fetchAll() + initViewModels(savedInstanceState) + } + + private fun initViewModels(savedInstanceState: Bundle?) { + subFilterViewModel = ViewModelProvider(this, viewModelFactory).get( + getViewModelKeyForTag(tagsFeedTag), + SubFilterViewModel::class.java + ) + subFilterViewModel.start(tagsFeedTag, tagsFeedTag, savedInstanceState) + + subFilterViewModel.updateTagsAndSites.observe(viewLifecycleOwner) { event -> + event.applyIfNotHandled { + if (NetworkUtils.isNetworkAvailable(activity)) { + ReaderUpdateServiceStarter.startService(activity, this) + } + } + } + + subFilterViewModel.subFilters.observe(viewLifecycleOwner) { subFilters -> + readerViewModel.showTopBarFilterGroup( + tagsFeedTag, + subFilters + ) + + val tags = subFilters.filterIsInstance().map { it.tag } + viewModel.fetchAll(tags) + } + subFilterViewModel.updateTagsAndSites() } override fun getScrollableViewForUniqueIdProvision(): View { @@ -71,73 +127,73 @@ class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragme /** * Throwaway UI code just for testing the initial Tags Feed fetching code. + * TODO remove this and replace with the final Compose content. */ @Composable private fun ReaderTagsFeedScreen( uiState: ReaderTagsFeedViewModel.UiState, ) { - AppThemeWithoutBackground { - Column( - modifier = Modifier - .verticalScroll(rememberScrollState()) - .padding(16.dp), - verticalArrangement = Arrangement.spacedBy(8.dp), - ) { - uiState.tagStates.forEach { (tag, fetchState) -> - Column( - verticalArrangement = Arrangement.spacedBy(4.dp), - ) { - Text( - text = tag.tagTitle, - style = MaterialTheme.typography.h4, - ) - - when (fetchState) { - is ReaderTagsFeedViewModel.FetchState.Loading -> { - Text( - text = "Loading...", - style = MaterialTheme.typography.body1, - ) - } + Column( + modifier = Modifier + .fillMaxHeight() + .verticalScroll(rememberScrollState()) + .padding(16.dp), + verticalArrangement = Arrangement.spacedBy(8.dp), + ) { + uiState.tagStates.forEach { (tag, fetchState) -> + Column( + verticalArrangement = Arrangement.spacedBy(4.dp), + ) { + Text( + text = tag.tagTitle, + style = MaterialTheme.typography.h4, + ) - is ReaderTagsFeedViewModel.FetchState.Error -> { - Text( - text = "Error loading posts", - style = MaterialTheme.typography.body1, - ) - } + when (fetchState) { + is ReaderTagsFeedViewModel.FetchState.Loading -> { + Text( + text = "Loading...", + style = MaterialTheme.typography.body1, + ) + } - is ReaderTagsFeedViewModel.FetchState.Success -> { - Row( - modifier = Modifier - .fillMaxWidth() - .horizontalScroll(rememberScrollState()), - ) { - fetchState.posts.forEach { post -> - Column( - modifier = Modifier - .width(300.dp) - .background( - MaterialTheme.colors.surface, - RoundedCornerShape(4.dp) - ) - .padding(4.dp), - verticalArrangement = Arrangement.spacedBy(4.dp), - ) { - Text( - text = post.title, - style = MaterialTheme.typography.h5, - maxLines = 2, - overflow = TextOverflow.Ellipsis, - ) + is ReaderTagsFeedViewModel.FetchState.Error -> { + Text( + text = "Error loading posts", + style = MaterialTheme.typography.body1, + ) + } - Text( - text = post.excerpt, - style = MaterialTheme.typography.body1, - maxLines = 4, - overflow = TextOverflow.Ellipsis, + is ReaderTagsFeedViewModel.FetchState.Success -> { + Row( + modifier = Modifier + .fillMaxWidth() + .horizontalScroll(rememberScrollState()), + ) { + fetchState.posts.forEach { post -> + Column( + modifier = Modifier + .width(300.dp) + .background( + MaterialTheme.colors.surface, + RoundedCornerShape(4.dp) ) - } + .padding(4.dp), + verticalArrangement = Arrangement.spacedBy(4.dp), + ) { + Text( + text = post.title, + style = MaterialTheme.typography.h5, + maxLines = 2, + overflow = TextOverflow.Ellipsis, + ) + + Text( + text = post.excerpt, + style = MaterialTheme.typography.body1, + maxLines = 4, + overflow = TextOverflow.Ellipsis, + ) } } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt index 0ec39b37a6fd..a3a7e089008f 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt @@ -7,7 +7,6 @@ import kotlinx.coroutines.flow.StateFlow import kotlinx.coroutines.flow.update import org.wordpress.android.models.ReaderPostList import org.wordpress.android.models.ReaderTag -import org.wordpress.android.models.ReaderTagType import org.wordpress.android.modules.BG_THREAD import org.wordpress.android.ui.reader.repository.ReaderPostRepository import org.wordpress.android.viewmodel.ScopedViewModel @@ -22,8 +21,8 @@ class ReaderTagsFeedViewModel @Inject constructor( private val _uiStateFlow = MutableStateFlow(UiState(emptyMap())) val uiStateFlow: StateFlow = _uiStateFlow - fun fetchAll() { - FAKE_TAGS.forEach { + fun fetchAll(tags: List) { + tags.forEach { fetchTag(it) } } @@ -56,12 +55,4 @@ class ReaderTagsFeedViewModel @Inject constructor( data object Error : FetchState() data class Success(val posts: ReaderPostList) : FetchState() } - - companion object { - private val FAKE_TAGS = listOf( - ReaderTag("science", "Science", "Science", null, ReaderTagType.FOLLOWED), - ReaderTag("fiction", "Fiction", "Fiction", null, ReaderTagType.FOLLOWED), - ReaderTag("rpg", "RPG", "RPG", null, ReaderTagType.FOLLOWED), - ) - } } From a82d1e8b5113b88944985e5cac2cc071641d49e1 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 19 Apr 2024 14:26:27 -0300 Subject: [PATCH 08/21] Get the Tags Feed tag from Fragment's newInstance --- .../android/ui/reader/ReaderFragment.kt | 2 +- .../ui/reader/ReaderTagsFeedFragment.kt | 27 ++++++++++++------- 2 files changed, 18 insertions(+), 11 deletions(-) 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 50f5c0071217..eb8a55d29038 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 @@ -229,7 +229,7 @@ class ReaderFragment : Fragment(R.layout.reader_fragment_layout), ScrollableView val selectedTag = uiState.selectedReaderTag val fragment = when { selectedTag.isDiscover -> ReaderDiscoverFragment() - selectedTag.isTags -> ReaderTagsFeedFragment() + selectedTag.isTags -> ReaderTagsFeedFragment.newInstance(selectedTag) else -> ReaderPostListFragment.newInstanceForTag( selectedTag, ReaderTypes.ReaderPostListType.TAG_FOLLOWED, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt index d4c81cde82b6..d5e586deaa7d 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt @@ -28,7 +28,6 @@ import dagger.hilt.android.AndroidEntryPoint import org.wordpress.android.R import org.wordpress.android.databinding.ReaderTagFeedFragmentLayoutBinding import org.wordpress.android.models.ReaderTag -import org.wordpress.android.models.ReaderTagType import org.wordpress.android.ui.ViewPagerFragment import org.wordpress.android.ui.compose.theme.AppThemeWithoutBackground import org.wordpress.android.ui.main.WPMainActivity @@ -39,6 +38,7 @@ import org.wordpress.android.ui.reader.subfilter.SubfilterListItem import org.wordpress.android.ui.reader.viewmodels.ReaderTagsFeedViewModel import org.wordpress.android.ui.reader.viewmodels.ReaderViewModel import org.wordpress.android.util.NetworkUtils +import org.wordpress.android.util.extensions.getSerializableCompat import javax.inject.Inject /** @@ -52,15 +52,10 @@ import javax.inject.Inject @AndroidEntryPoint class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragment_layout), WPMainActivity.OnScrollToTopListener { - // TODO thomashortadev get this via Fragment arguments private val tagsFeedTag by lazy { - ReaderTag( - "", - getString(R.string.reader_tags_display_name), - getString(R.string.reader_tags_display_name), - "", - ReaderTagType.TAGS - ) + // TODO maybe we can just create a static function somewhere that returns the Tags Feed ReaderTag, since it's + // used in multiple places, client-side only, and always the same. + requireArguments().getSerializableCompat(ARG_TAGS_FEED_TAG)!! } @Inject @@ -90,7 +85,7 @@ class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragme } private fun initViewModels(savedInstanceState: Bundle?) { - subFilterViewModel = ViewModelProvider(this, viewModelFactory).get( + subFilterViewModel = ViewModelProvider(this, viewModelFactory).get( getViewModelKeyForTag(tagsFeedTag), SubFilterViewModel::class.java ) @@ -123,6 +118,18 @@ class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragme override fun onScrollToTop() { // TODO scroll current content to top } + + companion object { + private const val ARG_TAGS_FEED_TAG = "tags_feed_tag" + + fun newInstance( + feedTag: ReaderTag + ): ReaderTagsFeedFragment = ReaderTagsFeedFragment().apply { + arguments = Bundle().apply { + putSerializable(ARG_TAGS_FEED_TAG, feedTag) + } + } + } } /** From e29762fc988b9683ce0481dd016c9e5b19ef17f4 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 19 Apr 2024 16:30:27 -0300 Subject: [PATCH 09/21] Add some doc comments in the Tags Feed ViewModel --- .../ui/reader/viewmodels/ReaderTagsFeedViewModel.kt | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt index a3a7e089008f..a148dc0b10ec 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt @@ -21,12 +21,24 @@ class ReaderTagsFeedViewModel @Inject constructor( private val _uiStateFlow = MutableStateFlow(UiState(emptyMap())) val uiStateFlow: StateFlow = _uiStateFlow + /** + * 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 + * [FetchState]s: [FetchState.Loading], [FetchState.Success], [FetchState.Error]. + */ fun fetchAll(tags: List) { tags.forEach { fetchTag(it) } } + /** + * Fetch posts for a single tag. This method will emit a new state to [uiStateFlow] for different [FetchState]s: + * [FetchState.Loading], [FetchState.Success], [FetchState.Error], but only for the tag being fetched. + * + * Can be used for retrying a failed fetch, for instance. + */ + @Suppress("MemberVisibilityCanBePrivate") fun fetchTag(tag: ReaderTag) { launch { _uiStateFlow.update { From ca30b4715b5fdeefa5263215a04d9705abed6000 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 19 Apr 2024 17:32:59 -0300 Subject: [PATCH 10/21] Create a more specific exception for post fetching --- .../ui/reader/exception/ReaderPostFetchException.kt | 5 +++++ .../android/ui/reader/repository/ReaderPostRepository.kt | 5 ++++- .../ui/reader/viewmodels/ReaderTagsFeedViewModel.kt | 7 ++++--- 3 files changed, 13 insertions(+), 4 deletions(-) create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/reader/exception/ReaderPostFetchException.kt diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/exception/ReaderPostFetchException.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/exception/ReaderPostFetchException.kt new file mode 100644 index 000000000000..b1918ef2f372 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/exception/ReaderPostFetchException.kt @@ -0,0 +1,5 @@ +package org.wordpress.android.ui.reader.exception + +class ReaderPostFetchException( + message: String = "Failed to fetch post(s).", +) : RuntimeException(message) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt index 7e4a98959fee..e33951ac82eb 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt @@ -15,6 +15,7 @@ import org.wordpress.android.ui.prefs.AppPrefs import org.wordpress.android.ui.reader.ReaderConstants import org.wordpress.android.ui.reader.actions.ReaderActions import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResultListener +import org.wordpress.android.ui.reader.exception.ReaderPostFetchException import org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter import org.wordpress.android.ui.reader.utils.ReaderUtils import org.wordpress.android.util.AppLog @@ -36,7 +37,9 @@ class ReaderPostRepository @Inject constructor( return suspendCancellableCoroutine { cont -> val resultListener = UpdateResultListener { result -> if (result == ReaderActions.UpdateResult.FAILED) { - cont.resumeWithException(Exception("Failed to fetch newer posts for tag")) + cont.resumeWithException( + ReaderPostFetchException("Failed to fetch newer posts for tag: ${tag.tagSlug}") + ) } else { val posts = ReaderPostTable.getPostsWithTag(tag, maxPosts, false) cont.resume(posts) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt index a148dc0b10ec..fba36feca81c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt @@ -8,6 +8,7 @@ import kotlinx.coroutines.flow.update import org.wordpress.android.models.ReaderPostList import org.wordpress.android.models.ReaderTag import org.wordpress.android.modules.BG_THREAD +import org.wordpress.android.ui.reader.exception.ReaderPostFetchException import org.wordpress.android.ui.reader.repository.ReaderPostRepository import org.wordpress.android.viewmodel.ScopedViewModel import javax.inject.Inject @@ -50,9 +51,9 @@ class ReaderTagsFeedViewModel @Inject constructor( _uiStateFlow.update { it.copy(tagStates = it.tagStates + (tag to FetchState.Success(posts))) } - } catch (e: Exception) { + } catch (e: ReaderPostFetchException) { _uiStateFlow.update { - it.copy(tagStates = it.tagStates + (tag to FetchState.Error)) + it.copy(tagStates = it.tagStates + (tag to FetchState.Error(e))) } } } @@ -64,7 +65,7 @@ class ReaderTagsFeedViewModel @Inject constructor( sealed class FetchState { data object Loading : FetchState() - data object Error : FetchState() data class Success(val posts: ReaderPostList) : FetchState() + data class Error(val exception: Exception) : FetchState() } } From ad521d9fc9de057ec6ba75e71d4527c9af35a532 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Fri, 19 Apr 2024 18:09:07 -0300 Subject: [PATCH 11/21] Create ReaderPostLocalSource for extracting local DB logic --- .../ReaderPostFetchException.kt | 2 +- .../reader/repository/ReaderPostRepository.kt | 82 ++---------- .../reader/sources/ReaderPostLocalSource.kt | 121 ++++++++++++++++++ .../viewmodels/ReaderTagsFeedViewModel.kt | 2 +- 4 files changed, 133 insertions(+), 74 deletions(-) rename WordPress/src/main/java/org/wordpress/android/ui/reader/{exception => exceptions}/ReaderPostFetchException.kt (68%) create mode 100644 WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/exception/ReaderPostFetchException.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/exceptions/ReaderPostFetchException.kt similarity index 68% rename from WordPress/src/main/java/org/wordpress/android/ui/reader/exception/ReaderPostFetchException.kt rename to WordPress/src/main/java/org/wordpress/android/ui/reader/exceptions/ReaderPostFetchException.kt index b1918ef2f372..de4cc47ae7df 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/exception/ReaderPostFetchException.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/exceptions/ReaderPostFetchException.kt @@ -1,4 +1,4 @@ -package org.wordpress.android.ui.reader.exception +package org.wordpress.android.ui.reader.exceptions class ReaderPostFetchException( message: String = "Failed to fetch post(s).", diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt index e33951ac82eb..9c24b917023f 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/repository/ReaderPostRepository.kt @@ -2,21 +2,21 @@ package org.wordpress.android.ui.reader.repository import com.android.volley.VolleyError import com.wordpress.rest.RestRequest +import dagger.Reusable import kotlinx.coroutines.suspendCancellableCoroutine import org.json.JSONObject import org.wordpress.android.WordPress.Companion.getRestClientUtilsV1_2 import org.wordpress.android.datasets.ReaderPostTable import org.wordpress.android.datasets.ReaderTagTable -import org.wordpress.android.models.ReaderPost import org.wordpress.android.models.ReaderPostList import org.wordpress.android.models.ReaderTag import org.wordpress.android.models.ReaderTagType -import org.wordpress.android.ui.prefs.AppPrefs import org.wordpress.android.ui.reader.ReaderConstants import org.wordpress.android.ui.reader.actions.ReaderActions import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResultListener -import org.wordpress.android.ui.reader.exception.ReaderPostFetchException +import org.wordpress.android.ui.reader.exceptions.ReaderPostFetchException import org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter +import org.wordpress.android.ui.reader.sources.ReaderPostLocalSource import org.wordpress.android.ui.reader.utils.ReaderUtils import org.wordpress.android.util.AppLog import org.wordpress.android.util.LocaleManagerWrapper @@ -26,8 +26,10 @@ import javax.inject.Inject import kotlin.coroutines.resume import kotlin.coroutines.resumeWithException +@Reusable class ReaderPostRepository @Inject constructor( - private val localeManagerWrapper: LocaleManagerWrapper + private val localeManagerWrapper: LocaleManagerWrapper, + private val localSource: ReaderPostLocalSource, ) { /** * Fetches and returns the most recent posts for the passed tag, respecting the maxPosts limit. @@ -177,77 +179,13 @@ class ReaderPostRepository @Inject constructor( resultListener.onUpdateResult(ReaderActions.UpdateResult.FAILED) return } + + // this should ideally be done using coroutines, but this class is currently being used from Java, which makes + // it difficult to use coroutines. This should be refactored to use coroutines when possible. object : Thread() { override fun run() { val serverPosts = ReaderPostList.fromJson(jsonObject) - val updateResult = ReaderPostTable.comparePosts(serverPosts) - if (updateResult.isNewOrChanged) { - // gap detection - only applies to posts with a specific tag - var postWithGap: ReaderPost? = null - if (tag != null) { - when (updateAction) { - ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER -> { - // if there's no overlap between server and local (ie: all server - // posts are new), assume there's a gap between server and local - // provided that local posts exist - val numServerPosts = serverPosts.size - if (numServerPosts >= 2 && ReaderPostTable.getNumPostsWithTag(tag) > 0 && - !ReaderPostTable.hasOverlap( - serverPosts, - tag - ) - ) { - // treat the second to last server post as having a gap - postWithGap = serverPosts[numServerPosts - 2] - // remove the last server post to deal with the edge case of - // there actually not being a gap between local & server - serverPosts.removeAt(numServerPosts - 1) - val gapMarker = ReaderPostTable.getGapMarkerIdsForTag(tag) - if (gapMarker != null) { - // We mustn't have two gapMarkers at the same time. Therefor we need to - // delete all posts before the current gapMarker and clear the gapMarker flag. - ReaderPostTable.deletePostsBeforeGapMarkerForTag(tag) - ReaderPostTable.removeGapMarkerForTag(tag) - } - } - } - - ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP -> { - // if service was started as a request to fill a gap, delete existing posts - // before the one with the gap marker, then remove the existing gap marker - ReaderPostTable.deletePostsBeforeGapMarkerForTag(tag) - ReaderPostTable.removeGapMarkerForTag(tag) - } - - ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> ReaderPostTable.deletePostsWithTag( - tag - ) - - ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER -> {} - } - } - ReaderPostTable.addOrUpdatePosts(tag, serverPosts) - if (AppPrefs.shouldUpdateBookmarkPostsPseudoIds(tag)) { - ReaderPostTable.updateBookmarkedPostPseudoId(serverPosts) - AppPrefs.setBookmarkPostsPseudoIdsUpdated() - } - - // gap marker must be set after saving server posts - if (postWithGap != null) { - ReaderPostTable.setGapMarkerForTag(postWithGap.blogId, postWithGap.postId, tag) - AppLog.d(AppLog.T.READER, "added gap marker to tag " + tag!!.tagNameForLog) - } - } else if (updateResult == ReaderActions.UpdateResult.UNCHANGED - && updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP - ) { - // edge case - request to fill gap returned nothing new, so remove the gap marker - ReaderPostTable.removeGapMarkerForTag(tag) - AppLog.w(AppLog.T.READER, "attempt to fill gap returned nothing new") - } - AppLog.d( - AppLog.T.READER, - "requested posts response = $updateResult" - ) + val updateResult = localSource.saveUpdatedPosts(serverPosts, updateAction, tag) resultListener.onUpdateResult(updateResult) } }.start() diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt new file mode 100644 index 000000000000..d327effc5015 --- /dev/null +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt @@ -0,0 +1,121 @@ +package org.wordpress.android.ui.reader.sources + +import dagger.Reusable +import org.wordpress.android.datasets.ReaderPostTable +import org.wordpress.android.models.ReaderPost +import org.wordpress.android.models.ReaderPostList +import org.wordpress.android.models.ReaderTag +import org.wordpress.android.ui.prefs.AppPrefs +import org.wordpress.android.ui.reader.actions.ReaderActions +import org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter +import org.wordpress.android.util.AppLog +import javax.inject.Inject + +/** + * Manage the saving of posts to the local database table. + */ +@Reusable +class ReaderPostLocalSource @Inject constructor() { + /** + * Save the list of posts to the local database, and handle any gaps between local and server posts. + * + * Ideally this should be a suspend function but since it's being ultimately used by Java in some scenarios we + * are keeping it blocking for now and it's up to the caller to run it in a coroutine or different thread. + */ + fun saveUpdatedPosts( + serverPosts: ReaderPostList, + updateAction: ReaderPostServiceStarter.UpdateAction, + requestedTag: ReaderTag?, + ): ReaderActions.UpdateResult { + val updateResult = ReaderPostTable.comparePosts(serverPosts) + if (updateResult.isNewOrChanged) { + // gap detection - only applies to posts with a specific tag + var postWithGap: ReaderPost? = null + if (requestedTag != null) { + when (updateAction) { + ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER -> { + postWithGap = handleRequestNewerResult(serverPosts, requestedTag) + } + + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP -> { + handleRequestOlderThanGapResult(requestedTag) + } + + ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> ReaderPostTable.deletePostsWithTag( + requestedTag + ) + + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER -> { /* noop */ + } + } + } + + // save posts to local db + ReaderPostTable.addOrUpdatePosts(requestedTag, serverPosts) + if (AppPrefs.shouldUpdateBookmarkPostsPseudoIds(requestedTag)) { + ReaderPostTable.updateBookmarkedPostPseudoId(serverPosts) + AppPrefs.setBookmarkPostsPseudoIdsUpdated() + } + + // gap marker must be set after saving server posts + if (postWithGap != null) { + ReaderPostTable.setGapMarkerForTag(postWithGap.blogId, postWithGap.postId, requestedTag) + AppLog.d(AppLog.T.READER, "added gap marker to tag " + requestedTag!!.tagNameForLog) + } + } else if (updateResult == ReaderActions.UpdateResult.UNCHANGED + && updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP + ) { + // edge case - request to fill gap returned nothing new, so remove the gap marker + ReaderPostTable.removeGapMarkerForTag(requestedTag) + AppLog.w(AppLog.T.READER, "attempt to fill gap returned nothing new") + } + AppLog.d( + AppLog.T.READER, + "requested posts response = $updateResult" + ) + return updateResult + } + + private fun handleRequestOlderThanGapResult(requestedTag: ReaderTag?) { + // if service was started as a request to fill a gap, delete existing posts + // before the one with the gap marker, then remove the existing gap marker + ReaderPostTable.deletePostsBeforeGapMarkerForTag(requestedTag) + ReaderPostTable.removeGapMarkerForTag(requestedTag) + } + + /** + * Handle the result of a request for newer posts, which may include a gap between local and server posts. + * + * @return the post that has a gap, or null if there's no gap + */ + private fun handleRequestNewerResult( + serverPosts: ReaderPostList, + requestedTag: ReaderTag?, + ): ReaderPost? { + // if there's no overlap between server and local (ie: all server + // posts are new), assume there's a gap between server and local + // provided that local posts exist + var postWithGap: ReaderPost? = null + val numServerPosts = serverPosts.size + if (numServerPosts >= 2 && ReaderPostTable.getNumPostsWithTag(requestedTag) > 0 && + !ReaderPostTable.hasOverlap( + serverPosts, + requestedTag + ) + ) { + // treat the second to last server post as having a gap + postWithGap = serverPosts[numServerPosts - 2] + // remove the last server post to deal with the edge case of + // there actually not being a gap between local & server + serverPosts.removeAt(numServerPosts - 1) + val gapMarker = ReaderPostTable.getGapMarkerIdsForTag(requestedTag) + if (gapMarker != null) { + // We mustn't have two gapMarkers at the same time. Therefor we need to + // delete all posts before the current gapMarker and clear the gapMarker flag. + ReaderPostTable.deletePostsBeforeGapMarkerForTag(requestedTag) + ReaderPostTable.removeGapMarkerForTag(requestedTag) + } + } + return postWithGap + } +} diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt index fba36feca81c..7d25a4bfe557 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt @@ -8,7 +8,7 @@ import kotlinx.coroutines.flow.update import org.wordpress.android.models.ReaderPostList import org.wordpress.android.models.ReaderTag import org.wordpress.android.modules.BG_THREAD -import org.wordpress.android.ui.reader.exception.ReaderPostFetchException +import org.wordpress.android.ui.reader.exceptions.ReaderPostFetchException import org.wordpress.android.ui.reader.repository.ReaderPostRepository import org.wordpress.android.viewmodel.ScopedViewModel import javax.inject.Inject From 83bec26ce4bd3cbc5a223589cec2ba97ef497e04 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 22 Apr 2024 12:16:14 -0300 Subject: [PATCH 12/21] Add unit test for ReaderPostLogicFactory --- .../post/ReaderPostLogicFactoryTest.kt | 32 +++++++++++++++++++ 1 file changed, 32 insertions(+) create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactoryTest.kt diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactoryTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactoryTest.kt new file mode 100644 index 000000000000..981220e7d45f --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactoryTest.kt @@ -0,0 +1,32 @@ +package org.wordpress.android.ui.reader.services.post + +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.mockito.Mock +import org.mockito.junit.MockitoJUnitRunner +import org.wordpress.android.ui.reader.repository.ReaderPostRepository +import org.wordpress.android.ui.reader.services.ServiceCompletionListener + +@RunWith(MockitoJUnitRunner::class) +class ReaderPostLogicFactoryTest { + @Mock + lateinit var readerPostRepository: ReaderPostRepository + + private lateinit var factory: ReaderPostLogicFactory + + @Before + fun setUp() { + factory = ReaderPostLogicFactory(readerPostRepository) + } + + @Test + fun `create should return a PostLogic instance`() { + val listener = ServiceCompletionListener { + // no-op + } + val logic = factory.create(listener) + assertThat(logic).isNotNull + } +} From d06808eb5f919df1749e2feaea8a8db77662ad57 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 22 Apr 2024 17:47:56 -0300 Subject: [PATCH 13/21] Use ReaderPostTableWrapper in ReaderPostLocalSource --- .../wrappers/ReaderPostTableWrapper.kt | 21 +++++++++- .../reader/sources/ReaderPostLocalSource.kt | 42 ++++++++++--------- 2 files changed, 43 insertions(+), 20 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/datasets/wrappers/ReaderPostTableWrapper.kt b/WordPress/src/main/java/org/wordpress/android/datasets/wrappers/ReaderPostTableWrapper.kt index eeabbd6a0e0f..8b532688a530 100644 --- a/WordPress/src/main/java/org/wordpress/android/datasets/wrappers/ReaderPostTableWrapper.kt +++ b/WordPress/src/main/java/org/wordpress/android/datasets/wrappers/ReaderPostTableWrapper.kt @@ -5,6 +5,8 @@ import org.wordpress.android.datasets.ReaderPostTable import org.wordpress.android.models.ReaderPost import org.wordpress.android.models.ReaderPostList import org.wordpress.android.models.ReaderTag +import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResult +import org.wordpress.android.ui.reader.models.ReaderBlogIdPostId import javax.inject.Inject @Reusable @@ -30,6 +32,23 @@ class ReaderPostTableWrapper @Inject constructor() { fun getNumPostsWithTag(readerTag: ReaderTag): Int = ReaderPostTable.getNumPostsWithTag(readerTag) - fun addOrUpdatePosts(readerTag: ReaderTag, posts: ReaderPostList) = + fun addOrUpdatePosts(readerTag: ReaderTag?, posts: ReaderPostList) = ReaderPostTable.addOrUpdatePosts(readerTag, posts) + + fun deletePostsWithTag(tag: ReaderTag) = ReaderPostTable.deletePostsWithTag(tag) + + fun comparePosts(posts: ReaderPostList): UpdateResult = ReaderPostTable.comparePosts(posts) + + fun updateBookmarkedPostPseudoId(posts: ReaderPostList) = ReaderPostTable.updateBookmarkedPostPseudoId(posts) + + fun setGapMarkerForTag(blogId: Long, postId: Long, tag: ReaderTag) = + ReaderPostTable.setGapMarkerForTag(blogId, postId, tag) + + fun removeGapMarkerForTag(tag: ReaderTag) = ReaderPostTable.removeGapMarkerForTag(tag) + + fun deletePostsBeforeGapMarkerForTag(tag: ReaderTag) = ReaderPostTable.deletePostsBeforeGapMarkerForTag(tag) + + fun hasOverlap(posts: ReaderPostList?, tag: ReaderTag): Boolean = ReaderPostTable.hasOverlap(posts, tag) + + fun getGapMarkerIdsForTag(tag: ReaderTag): ReaderBlogIdPostId? = ReaderPostTable.getGapMarkerIdsForTag(tag) } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt index d327effc5015..dfd03752dcc1 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt @@ -1,7 +1,7 @@ package org.wordpress.android.ui.reader.sources import dagger.Reusable -import org.wordpress.android.datasets.ReaderPostTable +import org.wordpress.android.datasets.wrappers.ReaderPostTableWrapper import org.wordpress.android.models.ReaderPost import org.wordpress.android.models.ReaderPostList import org.wordpress.android.models.ReaderTag @@ -15,7 +15,9 @@ import javax.inject.Inject * Manage the saving of posts to the local database table. */ @Reusable -class ReaderPostLocalSource @Inject constructor() { +class ReaderPostLocalSource @Inject constructor( + private val readerPostTable: ReaderPostTableWrapper, +) { /** * Save the list of posts to the local database, and handle any gaps between local and server posts. * @@ -27,7 +29,7 @@ class ReaderPostLocalSource @Inject constructor() { updateAction: ReaderPostServiceStarter.UpdateAction, requestedTag: ReaderTag?, ): ReaderActions.UpdateResult { - val updateResult = ReaderPostTable.comparePosts(serverPosts) + val updateResult = readerPostTable.comparePosts(serverPosts) if (updateResult.isNewOrChanged) { // gap detection - only applies to posts with a specific tag var postWithGap: ReaderPost? = null @@ -41,7 +43,7 @@ class ReaderPostLocalSource @Inject constructor() { handleRequestOlderThanGapResult(requestedTag) } - ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> ReaderPostTable.deletePostsWithTag( + ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> readerPostTable.deletePostsWithTag( requestedTag ) @@ -51,22 +53,24 @@ class ReaderPostLocalSource @Inject constructor() { } // save posts to local db - ReaderPostTable.addOrUpdatePosts(requestedTag, serverPosts) + readerPostTable.addOrUpdatePosts(requestedTag, serverPosts) + if (AppPrefs.shouldUpdateBookmarkPostsPseudoIds(requestedTag)) { - ReaderPostTable.updateBookmarkedPostPseudoId(serverPosts) + readerPostTable.updateBookmarkedPostPseudoId(serverPosts) AppPrefs.setBookmarkPostsPseudoIdsUpdated() } // gap marker must be set after saving server posts - if (postWithGap != null) { - ReaderPostTable.setGapMarkerForTag(postWithGap.blogId, postWithGap.postId, requestedTag) - AppLog.d(AppLog.T.READER, "added gap marker to tag " + requestedTag!!.tagNameForLog) + if (postWithGap != null && requestedTag != null) { + readerPostTable.setGapMarkerForTag(postWithGap.blogId, postWithGap.postId, requestedTag) + AppLog.d(AppLog.T.READER, "added gap marker to tag " + requestedTag.tagNameForLog) } } else if (updateResult == ReaderActions.UpdateResult.UNCHANGED && updateAction == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP + && requestedTag != null ) { // edge case - request to fill gap returned nothing new, so remove the gap marker - ReaderPostTable.removeGapMarkerForTag(requestedTag) + readerPostTable.removeGapMarkerForTag(requestedTag) AppLog.w(AppLog.T.READER, "attempt to fill gap returned nothing new") } AppLog.d( @@ -76,11 +80,11 @@ class ReaderPostLocalSource @Inject constructor() { return updateResult } - private fun handleRequestOlderThanGapResult(requestedTag: ReaderTag?) { + private fun handleRequestOlderThanGapResult(requestedTag: ReaderTag) { // if service was started as a request to fill a gap, delete existing posts // before the one with the gap marker, then remove the existing gap marker - ReaderPostTable.deletePostsBeforeGapMarkerForTag(requestedTag) - ReaderPostTable.removeGapMarkerForTag(requestedTag) + readerPostTable.deletePostsBeforeGapMarkerForTag(requestedTag) + readerPostTable.removeGapMarkerForTag(requestedTag) } /** @@ -90,15 +94,15 @@ class ReaderPostLocalSource @Inject constructor() { */ private fun handleRequestNewerResult( serverPosts: ReaderPostList, - requestedTag: ReaderTag?, + requestedTag: ReaderTag, ): ReaderPost? { // if there's no overlap between server and local (ie: all server // posts are new), assume there's a gap between server and local // provided that local posts exist var postWithGap: ReaderPost? = null val numServerPosts = serverPosts.size - if (numServerPosts >= 2 && ReaderPostTable.getNumPostsWithTag(requestedTag) > 0 && - !ReaderPostTable.hasOverlap( + if (numServerPosts >= 2 && readerPostTable.getNumPostsWithTag(requestedTag) > 0 && + !readerPostTable.hasOverlap( serverPosts, requestedTag ) @@ -108,12 +112,12 @@ class ReaderPostLocalSource @Inject constructor() { // remove the last server post to deal with the edge case of // there actually not being a gap between local & server serverPosts.removeAt(numServerPosts - 1) - val gapMarker = ReaderPostTable.getGapMarkerIdsForTag(requestedTag) + val gapMarker = readerPostTable.getGapMarkerIdsForTag(requestedTag) if (gapMarker != null) { // We mustn't have two gapMarkers at the same time. Therefor we need to // delete all posts before the current gapMarker and clear the gapMarker flag. - ReaderPostTable.deletePostsBeforeGapMarkerForTag(requestedTag) - ReaderPostTable.removeGapMarkerForTag(requestedTag) + readerPostTable.deletePostsBeforeGapMarkerForTag(requestedTag) + readerPostTable.removeGapMarkerForTag(requestedTag) } } return postWithGap From 50e28c43aee830457a4a64b5481375adefafd8fc Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 22 Apr 2024 19:12:37 -0300 Subject: [PATCH 14/21] Use AppPrefsWrapper in ReaderPostLocalSource --- .../android/ui/prefs/AppPrefsWrapper.kt | 4 +++ .../reader/sources/ReaderPostLocalSource.kt | 35 ++++++++++--------- 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/prefs/AppPrefsWrapper.kt b/WordPress/src/main/java/org/wordpress/android/ui/prefs/AppPrefsWrapper.kt index ac2df764c1f4..d42aa5fa4797 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/prefs/AppPrefsWrapper.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/prefs/AppPrefsWrapper.kt @@ -448,6 +448,10 @@ class AppPrefsWrapper @Inject constructor() { fun getShouldHideDynamicCard(id: String, ): Boolean = AppPrefs.getShouldHideDynamicCard(id) + fun shouldUpdateBookmarkPostsPseudoIds(tag: ReaderTag?): Boolean = AppPrefs.shouldUpdateBookmarkPostsPseudoIds(tag) + + fun setBookmarkPostsPseudoIdsUpdated() = AppPrefs.setBookmarkPostsPseudoIdsUpdated() + fun getAllPrefs(): Map = AppPrefs.getAllPrefs() fun setString(prefKey: PrefKey, value: String) { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt index dfd03752dcc1..f1be3b98bae3 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt @@ -5,7 +5,7 @@ import org.wordpress.android.datasets.wrappers.ReaderPostTableWrapper import org.wordpress.android.models.ReaderPost import org.wordpress.android.models.ReaderPostList import org.wordpress.android.models.ReaderTag -import org.wordpress.android.ui.prefs.AppPrefs +import org.wordpress.android.ui.prefs.AppPrefsWrapper import org.wordpress.android.ui.reader.actions.ReaderActions import org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter import org.wordpress.android.util.AppLog @@ -16,7 +16,8 @@ import javax.inject.Inject */ @Reusable class ReaderPostLocalSource @Inject constructor( - private val readerPostTable: ReaderPostTableWrapper, + private val readerPostTableWrapper: ReaderPostTableWrapper, + private val appPrefsWrapper: AppPrefsWrapper, ) { /** * Save the list of posts to the local database, and handle any gaps between local and server posts. @@ -29,7 +30,7 @@ class ReaderPostLocalSource @Inject constructor( updateAction: ReaderPostServiceStarter.UpdateAction, requestedTag: ReaderTag?, ): ReaderActions.UpdateResult { - val updateResult = readerPostTable.comparePosts(serverPosts) + val updateResult = readerPostTableWrapper.comparePosts(serverPosts) if (updateResult.isNewOrChanged) { // gap detection - only applies to posts with a specific tag var postWithGap: ReaderPost? = null @@ -43,7 +44,7 @@ class ReaderPostLocalSource @Inject constructor( handleRequestOlderThanGapResult(requestedTag) } - ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> readerPostTable.deletePostsWithTag( + ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH -> readerPostTableWrapper.deletePostsWithTag( requestedTag ) @@ -53,16 +54,16 @@ class ReaderPostLocalSource @Inject constructor( } // save posts to local db - readerPostTable.addOrUpdatePosts(requestedTag, serverPosts) + readerPostTableWrapper.addOrUpdatePosts(requestedTag, serverPosts) - if (AppPrefs.shouldUpdateBookmarkPostsPseudoIds(requestedTag)) { - readerPostTable.updateBookmarkedPostPseudoId(serverPosts) - AppPrefs.setBookmarkPostsPseudoIdsUpdated() + if (appPrefsWrapper.shouldUpdateBookmarkPostsPseudoIds(requestedTag)) { + readerPostTableWrapper.updateBookmarkedPostPseudoId(serverPosts) + appPrefsWrapper.setBookmarkPostsPseudoIdsUpdated() } // gap marker must be set after saving server posts if (postWithGap != null && requestedTag != null) { - readerPostTable.setGapMarkerForTag(postWithGap.blogId, postWithGap.postId, requestedTag) + readerPostTableWrapper.setGapMarkerForTag(postWithGap.blogId, postWithGap.postId, requestedTag) AppLog.d(AppLog.T.READER, "added gap marker to tag " + requestedTag.tagNameForLog) } } else if (updateResult == ReaderActions.UpdateResult.UNCHANGED @@ -70,7 +71,7 @@ class ReaderPostLocalSource @Inject constructor( && requestedTag != null ) { // edge case - request to fill gap returned nothing new, so remove the gap marker - readerPostTable.removeGapMarkerForTag(requestedTag) + readerPostTableWrapper.removeGapMarkerForTag(requestedTag) AppLog.w(AppLog.T.READER, "attempt to fill gap returned nothing new") } AppLog.d( @@ -83,8 +84,8 @@ class ReaderPostLocalSource @Inject constructor( private fun handleRequestOlderThanGapResult(requestedTag: ReaderTag) { // if service was started as a request to fill a gap, delete existing posts // before the one with the gap marker, then remove the existing gap marker - readerPostTable.deletePostsBeforeGapMarkerForTag(requestedTag) - readerPostTable.removeGapMarkerForTag(requestedTag) + readerPostTableWrapper.deletePostsBeforeGapMarkerForTag(requestedTag) + readerPostTableWrapper.removeGapMarkerForTag(requestedTag) } /** @@ -101,8 +102,8 @@ class ReaderPostLocalSource @Inject constructor( // provided that local posts exist var postWithGap: ReaderPost? = null val numServerPosts = serverPosts.size - if (numServerPosts >= 2 && readerPostTable.getNumPostsWithTag(requestedTag) > 0 && - !readerPostTable.hasOverlap( + if (numServerPosts >= 2 && readerPostTableWrapper.getNumPostsWithTag(requestedTag) > 0 && + !readerPostTableWrapper.hasOverlap( serverPosts, requestedTag ) @@ -112,12 +113,12 @@ class ReaderPostLocalSource @Inject constructor( // remove the last server post to deal with the edge case of // there actually not being a gap between local & server serverPosts.removeAt(numServerPosts - 1) - val gapMarker = readerPostTable.getGapMarkerIdsForTag(requestedTag) + val gapMarker = readerPostTableWrapper.getGapMarkerIdsForTag(requestedTag) if (gapMarker != null) { // We mustn't have two gapMarkers at the same time. Therefor we need to // delete all posts before the current gapMarker and clear the gapMarker flag. - readerPostTable.deletePostsBeforeGapMarkerForTag(requestedTag) - readerPostTable.removeGapMarkerForTag(requestedTag) + readerPostTableWrapper.deletePostsBeforeGapMarkerForTag(requestedTag) + readerPostTableWrapper.removeGapMarkerForTag(requestedTag) } } return postWithGap From 404d22b8e4c159893576367eda390362501c0b5a Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Mon, 22 Apr 2024 19:12:52 -0300 Subject: [PATCH 15/21] Add unit tests for ReaderPostLocalSource --- .../sources/ReaderPostLocalSourceTest.kt | 327 ++++++++++++++++++ 1 file changed, 327 insertions(+) create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSourceTest.kt diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSourceTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSourceTest.kt new file mode 100644 index 000000000000..34e555cf219e --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSourceTest.kt @@ -0,0 +1,327 @@ +package org.wordpress.android.ui.reader.sources + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.kotlin.any +import org.mockito.kotlin.clearInvocations +import org.mockito.kotlin.eq +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.only +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.datasets.wrappers.ReaderPostTableWrapper +import org.wordpress.android.models.ReaderPostList +import org.wordpress.android.models.ReaderTag +import org.wordpress.android.models.ReaderTagType +import org.wordpress.android.ui.prefs.AppPrefsWrapper +import org.wordpress.android.ui.reader.actions.ReaderActions.UpdateResult +import org.wordpress.android.ui.reader.services.post.ReaderPostServiceStarter + +@OptIn(ExperimentalCoroutinesApi::class) +class ReaderPostLocalSourceTest : BaseUnitTest() { + @Mock + lateinit var readerPostTableWrapper: ReaderPostTableWrapper + + @Mock + lateinit var appPrefsWrapper: AppPrefsWrapper + + private lateinit var localSource: ReaderPostLocalSource + + @Before + fun setUp() { + localSource = ReaderPostLocalSource(readerPostTableWrapper, appPrefsWrapper) + } + + @Test + fun `given no changes and no tag provided, when saveUpdatedPosts, then do nothing`() { + // Given + val serverPosts = ReaderPostList() + val requestedTag = null + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(UpdateResult.UNCHANGED) + + // it doesn't matter which update action was used, so let's test all of them + ReaderPostServiceStarter.UpdateAction.values().forEach { updateAction -> + clearInvocations(readerPostTableWrapper) + + // When + val result = localSource.saveUpdatedPosts(serverPosts, updateAction, requestedTag) + + // Then + verify(readerPostTableWrapper, only()).comparePosts(serverPosts) // only comparePosts should be + + assertThat(result).isEqualTo(UpdateResult.UNCHANGED) + } + } + + @Test + fun `given no changes and tag provided, when saveUpdatedPosts, then do nothing`() { + // Given + val serverPosts = ReaderPostList() + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(UpdateResult.UNCHANGED) + + // if the action is any but REQUEST_OLDER_THAN_GAP we should not do anything + ReaderPostServiceStarter.UpdateAction.values() + .filterNot { it == ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP } + .forEach { updateAction -> + clearInvocations(readerPostTableWrapper) + + // When + val result = localSource.saveUpdatedPosts(serverPosts, updateAction, requestedTag) + + // Then + verify(readerPostTableWrapper, only()).comparePosts(serverPosts) // only comparePosts should be + + assertThat(result).isEqualTo(UpdateResult.UNCHANGED) + } + } + + @Test + fun `given no changes, tag provided and OLDER_THAN_GAP, when saveUpdatedPosts, then remove gap marker`() { + // Given + val serverPosts = ReaderPostList() + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(UpdateResult.UNCHANGED) + + // When + val result = localSource.saveUpdatedPosts( + serverPosts, + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP, + requestedTag, + ) + + // Then + verify(readerPostTableWrapper).removeGapMarkerForTag(requestedTag) + + assertThat(result).isEqualTo(UpdateResult.UNCHANGED) + } + + @Test + fun `given new posts and no tag provided, when saveUpdatedPosts, then save posts`() { + // Given + val serverPosts = ReaderPostList() + val requestedTag = null + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(UpdateResult.HAS_NEW) + + // it doesn't matter which update action was used, so let's test all of them + ReaderPostServiceStarter.UpdateAction.values().forEach { updateAction -> + clearInvocations(readerPostTableWrapper) + + // When + val result = localSource.saveUpdatedPosts(serverPosts, updateAction, requestedTag) + + // Then + verify(readerPostTableWrapper).addOrUpdatePosts(requestedTag, serverPosts) + + assertThat(result).isEqualTo(UpdateResult.HAS_NEW) + } + } + + @Test + fun `given posts changed, tag provided and OLDER_THAN_GAP, when saveUpdatedPosts, then remove gap marker`() { + // Given + val serverPosts = ReaderPostList() + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + listOf(UpdateResult.CHANGED, UpdateResult.HAS_NEW).forEach { updateResult -> + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(updateResult) + clearInvocations(readerPostTableWrapper) + + // When + val result = localSource.saveUpdatedPosts( + serverPosts, + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER_THAN_GAP, + requestedTag, + ) + + // Then + verify(readerPostTableWrapper).deletePostsBeforeGapMarkerForTag(requestedTag) + verify(readerPostTableWrapper).removeGapMarkerForTag(requestedTag) + verify(readerPostTableWrapper).addOrUpdatePosts(requestedTag, serverPosts) + + assertThat(result).isEqualTo(updateResult) + } + } + + @Test + fun `given posts changed, tag provided and REFRESH, when saveUpdatedPosts, then delete posts and save`() { + // Given + val serverPosts = ReaderPostList() + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + listOf(UpdateResult.CHANGED, UpdateResult.HAS_NEW).forEach { updateResult -> + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(updateResult) + clearInvocations(readerPostTableWrapper) + + // When + val result = localSource.saveUpdatedPosts( + serverPosts, + ReaderPostServiceStarter.UpdateAction.REQUEST_REFRESH, + requestedTag, + ) + + // Then + verify(readerPostTableWrapper).deletePostsWithTag(requestedTag) + verify(readerPostTableWrapper).addOrUpdatePosts(requestedTag, serverPosts) + + assertThat(result).isEqualTo(updateResult) + } + } + + @Test + fun `given posts changed, tag provided and OLDER, when saveUpdatedPosts, then save`() { + // Given + val serverPosts = ReaderPostList() + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + listOf(UpdateResult.CHANGED, UpdateResult.HAS_NEW).forEach { updateResult -> + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(updateResult) + clearInvocations(readerPostTableWrapper) + + + // When + val result = localSource.saveUpdatedPosts( + serverPosts, + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER, + requestedTag, + ) + + // Then + verify(readerPostTableWrapper).addOrUpdatePosts(requestedTag, serverPosts) + + assertThat(result).isEqualTo(updateResult) + } + } + + @Test + fun `given posts changed, tag provided and NEWER with no gap, when saveUpdatedPosts, then save`() { + // Given + val serverPosts = ReaderPostList().apply { + repeat(4) { add(mock()) } + } + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + whenever(readerPostTableWrapper.getNumPostsWithTag(requestedTag)).thenReturn(4) + whenever(readerPostTableWrapper.hasOverlap(serverPosts, requestedTag)).thenReturn(true) + + listOf(UpdateResult.CHANGED, UpdateResult.HAS_NEW).forEach { updateResult -> + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(updateResult) + clearInvocations(readerPostTableWrapper) + + // When + val result = localSource.saveUpdatedPosts( + serverPosts, + ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER, + requestedTag, + ) + + // Then + verify(readerPostTableWrapper).addOrUpdatePosts(requestedTag, serverPosts) + + assertThat(result).isEqualTo(updateResult) + } + } + + @Test + fun `given posts changed, tag provided and NEWER with gap, when saveUpdatedPosts, then save and set gap`() { + // Given + val serverPosts = ReaderPostList().apply { + repeat(4) { add(mock()) } + } + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + whenever(readerPostTableWrapper.getNumPostsWithTag(requestedTag)).thenReturn(4) + whenever(readerPostTableWrapper.hasOverlap(serverPosts, requestedTag)).thenReturn(false) + + listOf(UpdateResult.CHANGED, UpdateResult.HAS_NEW).forEach { updateResult -> + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(updateResult) + clearInvocations(readerPostTableWrapper) + + // When + val result = localSource.saveUpdatedPosts( + serverPosts, + ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER, + requestedTag, + ) + + // Then + verify(readerPostTableWrapper, never()).deletePostsBeforeGapMarkerForTag(requestedTag) + verify(readerPostTableWrapper, never()).removeGapMarkerForTag(requestedTag) + verify(readerPostTableWrapper).addOrUpdatePosts(requestedTag, serverPosts) + verify(readerPostTableWrapper).setGapMarkerForTag(any(), any(), eq(requestedTag)) + + assertThat(result).isEqualTo(updateResult) + } + } + + @Test + fun `given posts changed, tag provided and NEWER with gap, when saveUpdatedPosts, then keep 1 gap only and save`() { + // Given + val serverPosts = ReaderPostList().apply { + repeat(4) { add(mock()) } + } + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + whenever(readerPostTableWrapper.getNumPostsWithTag(requestedTag)).thenReturn(5) + whenever(readerPostTableWrapper.hasOverlap(serverPosts, requestedTag)).thenReturn(false) + whenever(readerPostTableWrapper.getGapMarkerIdsForTag(requestedTag)).thenReturn(mock()) + + listOf(UpdateResult.CHANGED, UpdateResult.HAS_NEW).forEach { updateResult -> + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(updateResult) + clearInvocations(readerPostTableWrapper) + + // When + val result = localSource.saveUpdatedPosts( + serverPosts, + ReaderPostServiceStarter.UpdateAction.REQUEST_NEWER, + requestedTag, + ) + + // Then + verify(readerPostTableWrapper).deletePostsBeforeGapMarkerForTag(requestedTag) + verify(readerPostTableWrapper).removeGapMarkerForTag(requestedTag) + verify(readerPostTableWrapper).addOrUpdatePosts(requestedTag, serverPosts) + verify(readerPostTableWrapper).setGapMarkerForTag(any(), any(), eq(requestedTag)) + + assertThat(result).isEqualTo(updateResult) + } + } + + @Test + fun `given posts changed, tag provided and update bookmark, when saveUpdatedPosts, then update bookmark`() { + // Given + val serverPosts = ReaderPostList() + val requestedTag = ReaderTag("tag", "tag", "tag", "endpoint", ReaderTagType.FOLLOWED) + + listOf(UpdateResult.CHANGED, UpdateResult.HAS_NEW).forEach { updateResult -> + whenever(readerPostTableWrapper.comparePosts(serverPosts)).thenReturn(updateResult) + whenever(appPrefsWrapper.shouldUpdateBookmarkPostsPseudoIds(requestedTag)).thenReturn(true) + + // it doesn't matter which update action was used, so let's test all of them + ReaderPostServiceStarter.UpdateAction.values().forEach { updateAction -> + clearInvocations(readerPostTableWrapper, appPrefsWrapper) + + // When + val result = localSource.saveUpdatedPosts( + serverPosts, + updateAction, + requestedTag, + ) + + // Then + verify(readerPostTableWrapper).addOrUpdatePosts(requestedTag, serverPosts) + verify(readerPostTableWrapper).updateBookmarkedPostPseudoId(serverPosts) + verify(appPrefsWrapper).setBookmarkPostsPseudoIdsUpdated() + + assertThat(result).isEqualTo(updateResult) + } + } + } +} From b414849b36f439f05a32356b25f79723b00a8f4f Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Tue, 23 Apr 2024 14:56:22 -0300 Subject: [PATCH 16/21] Add retry to temporary UI to remove suppress annotation --- .../ui/reader/ReaderTagsFeedFragment.kt | 30 +++++++++++++++---- .../viewmodels/ReaderTagsFeedViewModel.kt | 1 - 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt index d5e586deaa7d..25560fcff18a 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt @@ -3,6 +3,7 @@ package org.wordpress.android.ui.reader import android.os.Bundle import android.view.View import androidx.compose.foundation.background +import androidx.compose.foundation.clickable import androidx.compose.foundation.horizontalScroll import androidx.compose.foundation.layout.Arrangement import androidx.compose.foundation.layout.Column @@ -20,6 +21,7 @@ import androidx.compose.runtime.Composable import androidx.compose.runtime.collectAsState import androidx.compose.runtime.getValue import androidx.compose.ui.Modifier +import androidx.compose.ui.text.style.TextDecoration import androidx.compose.ui.text.style.TextOverflow import androidx.compose.ui.unit.dp import androidx.fragment.app.viewModels @@ -77,7 +79,10 @@ class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragme binding.composeView.setContent { AppThemeWithoutBackground { val uiState by viewModel.uiStateFlow.collectAsState() - ReaderTagsFeedScreen(uiState) + ReaderTagsFeedScreen( + uiState = uiState, + onRetryClicked = viewModel::fetchTag, + ) } } @@ -139,6 +144,7 @@ class ReaderTagsFeedFragment : ViewPagerFragment(R.layout.reader_tag_feed_fragme @Composable private fun ReaderTagsFeedScreen( uiState: ReaderTagsFeedViewModel.UiState, + onRetryClicked: (ReaderTag) -> Unit, ) { Column( modifier = Modifier @@ -165,10 +171,24 @@ private fun ReaderTagsFeedScreen( } is ReaderTagsFeedViewModel.FetchState.Error -> { - Text( - text = "Error loading posts", - style = MaterialTheme.typography.body1, - ) + Row( + horizontalArrangement = Arrangement.spacedBy(2.dp), + ) { + Text( + text = "Error loading posts.", + style = MaterialTheme.typography.body1, + ) + + Text( + text = "Retry", + style = MaterialTheme.typography.body1, + textDecoration = TextDecoration.Underline, + color = MaterialTheme.colors.primary, + modifier = Modifier + .padding(start = 8.dp) + .clickable { onRetryClicked(tag) }, + ) + } } is ReaderTagsFeedViewModel.FetchState.Success -> { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt index 7d25a4bfe557..6016df2cc87a 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModel.kt @@ -39,7 +39,6 @@ class ReaderTagsFeedViewModel @Inject constructor( * * Can be used for retrying a failed fetch, for instance. */ - @Suppress("MemberVisibilityCanBePrivate") fun fetchTag(tag: ReaderTag) { launch { _uiStateFlow.update { From d7f3946076f9e1de748aaaea7793fa4cdff38858 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Thu, 25 Apr 2024 15:02:54 -0300 Subject: [PATCH 17/21] Fix comment typo --- .../org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt index 25560fcff18a..c4ce39ad344c 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/ReaderTagsFeedFragment.kt @@ -44,7 +44,7 @@ import org.wordpress.android.util.extensions.getSerializableCompat import javax.inject.Inject /** - * Initial implementation of ReaderTagFeedFragment with the idea of it containing both a ComposeView, which will host + * Initial implementation of ReaderTagsFeedFragment with the idea of it containing both a ComposeView, which will host * all Compose content related to the new Tags Feed as well as an internal ReaderPostListFragment, which will be used * to display "filtered" content based on the currently selected tag on the top app bar filter. * From 05e043348d72972c968961995c6546b98c684fdd Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Thu, 25 Apr 2024 15:03:04 -0300 Subject: [PATCH 18/21] Add return type to public function --- .../android/ui/reader/services/post/ReaderPostLogicFactory.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactory.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactory.kt index 6d64d4b5117d..952ef6b31db8 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactory.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactory.kt @@ -7,7 +7,7 @@ import javax.inject.Inject class ReaderPostLogicFactory @Inject constructor( private val readerPostRepository: ReaderPostRepository, ) { - fun create(listener: ServiceCompletionListener) = ReaderPostLogic( + fun create(listener: ServiceCompletionListener): ReaderPostLogic = ReaderPostLogic( listener, readerPostRepository, ) From 6a0d1395d71ea94607623af5762ae30dad56e53c Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Thu, 25 Apr 2024 15:05:11 -0300 Subject: [PATCH 19/21] Check for returned instance type instead of null in test --- .../ui/reader/services/post/ReaderPostLogicFactoryTest.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactoryTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactoryTest.kt index 981220e7d45f..dea73350f024 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactoryTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/services/post/ReaderPostLogicFactoryTest.kt @@ -27,6 +27,6 @@ class ReaderPostLogicFactoryTest { // no-op } val logic = factory.create(listener) - assertThat(logic).isNotNull + assertThat(logic).isInstanceOf(ReaderPostLogic::class.java) } } From 4b99a1f313dc7b82b867f65a5755068deff2f7fb Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Thu, 25 Apr 2024 18:39:14 -0300 Subject: [PATCH 20/21] Add unit tests for ReaderTagsFeedViewModel --- .../viewmodels/ReaderTagsFeedViewModelTest.kt | 234 ++++++++++++++++++ 1 file changed, 234 insertions(+) create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModelTest.kt 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 new file mode 100644 index 000000000000..d4008189759b --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderTagsFeedViewModelTest.kt @@ -0,0 +1,234 @@ +package org.wordpress.android.ui.reader.viewmodels + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.delay +import kotlinx.coroutines.flow.toList +import kotlinx.coroutines.launch +import kotlinx.coroutines.test.TestScope +import org.assertj.core.api.Assertions.assertThat +import org.junit.Before +import org.junit.Test +import org.mockito.Mock +import org.mockito.kotlin.doSuspendableAnswer +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.models.ReaderPostList +import org.wordpress.android.models.ReaderTag +import org.wordpress.android.models.ReaderTagType +import org.wordpress.android.ui.reader.exceptions.ReaderPostFetchException +import org.wordpress.android.ui.reader.repository.ReaderPostRepository +import org.wordpress.android.ui.reader.viewmodels.ReaderTagsFeedViewModel.FetchState + +@OptIn(ExperimentalCoroutinesApi::class) +class ReaderTagsFeedViewModelTest : BaseUnitTest() { + @Mock + lateinit var readerPostRepository: ReaderPostRepository + + private lateinit var viewModel: ReaderTagsFeedViewModel + + private val collectedUiStates: MutableList = mutableListOf() + + @Before + fun setUp() { + viewModel = ReaderTagsFeedViewModel(testDispatcher(), readerPostRepository) + } + + @Test + fun `given valid tag, when fetchTag, then UI state should update properly`() = testCollectingUiStates { + // Given + val tag = ReaderTag( + "tag", + "tag", + "tag", + "endpoint", + ReaderTagType.FOLLOWED, + ) + val posts = ReaderPostList() + whenever(readerPostRepository.fetchNewerPostsForTag(tag)).doSuspendableAnswer { + delay(100) + posts + } + + // When + viewModel.fetchTag(tag) + advanceUntilIdle() + + // Then + assertThat(collectedUiStates).contains( + ReaderTagsFeedViewModel.UiState( + mapOf( + tag to FetchState.Loading, + ) + ), + ReaderTagsFeedViewModel.UiState( + mapOf( + tag to FetchState.Success(posts), + ) + ), + ) + } + + @Test + fun `given invalid tag, when fetchTag, then UI state should update properly`() = testCollectingUiStates { + // Given + val tag = ReaderTag( + "tag", + "tag", + "tag", + "endpoint", + ReaderTagType.FOLLOWED, + ) + val error = ReaderPostFetchException("error") + whenever(readerPostRepository.fetchNewerPostsForTag(tag)).doSuspendableAnswer { + delay(100) + throw error + } + + // When + viewModel.fetchTag(tag) + advanceUntilIdle() + + // Then + assertThat(collectedUiStates).contains( + ReaderTagsFeedViewModel.UiState( + mapOf( + tag to FetchState.Loading, + ) + ), + ReaderTagsFeedViewModel.UiState( + mapOf( + tag to FetchState.Error(error), + ) + ), + ) + } + + @Test + fun `given valid tags, when fetchAll, then UI state should update properly`() = testCollectingUiStates { + // Given + val tag1 = ReaderTag( + "tag1", + "tag1", + "tag1", + "endpoint1", + ReaderTagType.FOLLOWED, + ) + val tag2 = ReaderTag( + "tag2", + "tag2", + "tag2", + "endpoint2", + ReaderTagType.FOLLOWED, + ) + val posts1 = ReaderPostList() + val posts2 = ReaderPostList() + whenever(readerPostRepository.fetchNewerPostsForTag(tag1)).doSuspendableAnswer { + delay(100) + posts1 + } + whenever(readerPostRepository.fetchNewerPostsForTag(tag2)).doSuspendableAnswer { + delay(200) + posts2 + } + + // When + viewModel.fetchAll(listOf(tag1, tag2)) + advanceUntilIdle() + + // Then + + // tag 1 + assertThat(collectedUiStates).anyMatch { + it.tagStates[tag1] == FetchState.Loading + } + assertThat(collectedUiStates).anyMatch { + it.tagStates[tag1] == FetchState.Success(posts1) + } + + // tag 2 + assertThat(collectedUiStates).anyMatch { + it.tagStates[tag2] == FetchState.Loading + } + assertThat(collectedUiStates).anyMatch { + it.tagStates[tag2] == FetchState.Success(posts1) + } + + assertThat(collectedUiStates.last()).isEqualTo( + ReaderTagsFeedViewModel.UiState( + mapOf( + tag1 to FetchState.Success(posts1), + tag2 to FetchState.Success(posts2), + ) + ) + ) + } + + @Test + fun `given valid and invalid tags, when fetchAll, then UI state should update properly`() = testCollectingUiStates { + // Given + val tag1 = ReaderTag( + "tag1", + "tag1", + "tag1", + "endpoint1", + ReaderTagType.FOLLOWED, + ) + val tag2 = ReaderTag( + "tag2", + "tag2", + "tag2", + "endpoint2", + ReaderTagType.FOLLOWED, + ) + val posts1 = ReaderPostList() + val error2 = ReaderPostFetchException("error") + whenever(readerPostRepository.fetchNewerPostsForTag(tag1)).doSuspendableAnswer { + delay(100) + posts1 + } + whenever(readerPostRepository.fetchNewerPostsForTag(tag2)).doSuspendableAnswer { + delay(200) + throw error2 + } + + // When + viewModel.fetchAll(listOf(tag1, tag2)) + advanceUntilIdle() + + // Then + + // tag 1 + assertThat(collectedUiStates).anyMatch { + it.tagStates[tag1] == FetchState.Loading + } + assertThat(collectedUiStates).anyMatch { + it.tagStates[tag1] == FetchState.Success(posts1) + } + + // tag 2 + assertThat(collectedUiStates).anyMatch { + it.tagStates[tag2] == FetchState.Loading + } + assertThat(collectedUiStates).anyMatch { + it.tagStates[tag2] == FetchState.Error(error2) + } + + assertThat(collectedUiStates.last()).isEqualTo( + ReaderTagsFeedViewModel.UiState( + mapOf( + tag1 to FetchState.Success(posts1), + tag2 to FetchState.Error(error2), + ) + ) + ) + } + + private fun testCollectingUiStates(block: suspend TestScope.() -> Unit) = test { + val collectedUiStatesJob = launch { + collectedUiStates.clear() + viewModel.uiStateFlow.toList(collectedUiStates) + } + this.block() + collectedUiStatesJob.cancel() + } +} From 59ba75c6f3ace25198d20d2c7c56d33c8edb2e37 Mon Sep 17 00:00:00 2001 From: Thomas Horta Date: Thu, 25 Apr 2024 18:39:31 -0300 Subject: [PATCH 21/21] Fix linebreak format --- .../android/ui/reader/sources/ReaderPostLocalSource.kt | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt index f1be3b98bae3..b649d5fb55ec 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/sources/ReaderPostLocalSource.kt @@ -48,7 +48,8 @@ class ReaderPostLocalSource @Inject constructor( requestedTag ) - ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER -> { /* noop */ + ReaderPostServiceStarter.UpdateAction.REQUEST_OLDER -> { + /* noop */ } } }