diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/usecases/ReaderFetchPostUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/usecases/ReaderFetchPostUseCase.kt index aec69120a459..7047cee8bd4b 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/usecases/ReaderFetchPostUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/usecases/ReaderFetchPostUseCase.kt @@ -1,6 +1,7 @@ package org.wordpress.android.ui.reader.usecases import kotlinx.coroutines.suspendCancellableCoroutine +import kotlinx.coroutines.withTimeoutOrNull import org.wordpress.android.ui.reader.actions.ReaderActions import org.wordpress.android.ui.reader.actions.ReaderPostActionsWrapper import org.wordpress.android.ui.reader.usecases.ReaderFetchPostUseCase.FetchReaderPostState.AlreadyRunning @@ -11,50 +12,61 @@ import org.wordpress.android.ui.reader.usecases.ReaderFetchPostUseCase.FetchRead import org.wordpress.android.ui.reader.usecases.ReaderFetchPostUseCase.FetchReaderPostState.Success import org.wordpress.android.util.NetworkUtilsWrapper import java.net.HttpURLConnection +import java.util.Collections import javax.inject.Inject -import kotlin.coroutines.Continuation import kotlin.coroutines.resume class ReaderFetchPostUseCase @Inject constructor( private val networkUtilsWrapper: NetworkUtilsWrapper, private val readerPostActionsWrapper: ReaderPostActionsWrapper ) { - private val continuations: MutableMap?> = mutableMapOf() + // Tracks in-flight fetches so a second request for the same post short-circuits to + // AlreadyRunning. Synchronized because the request callbacks fire on a background thread. + private val inFlightRequests = Collections.synchronizedSet(mutableSetOf()) + @Suppress("ReturnCount") // early guard clauses (no network / already in flight) read cleaner than nesting suspend fun fetchPost(blogId: Long, postId: Long, isFeed: Boolean): FetchReaderPostState { - return if (!networkUtilsWrapper.isNetworkAvailable()) { - NoNetwork - } else { - val requestParams = FetchPostRequestParams(blogId, postId, isFeed) - // There is already an action running for this request - if (continuations[requestParams] != null) { - AlreadyRunning - } else { - when (fetchPostAndWaitForResult(requestParams)) { - HttpURLConnection.HTTP_OK -> Success - HttpURLConnection.HTTP_UNAUTHORIZED, HttpURLConnection.HTTP_FORBIDDEN -> NotAuthorised - HttpURLConnection.HTTP_NOT_FOUND -> PostNotFound - else -> RequestFailed - } - } + if (!networkUtilsWrapper.isNetworkAvailable()) { + return NoNetwork } - } - private suspend fun fetchPostAndWaitForResult(requestParams: FetchPostRequestParams): Int { - val listener = object : ReaderActions.OnRequestListener { - override fun onSuccess(result: String?) { - continuations[requestParams]?.resume(HttpURLConnection.HTTP_OK) - continuations[requestParams] = null - } + val requestParams = FetchPostRequestParams(blogId, postId, isFeed) + // add() returns false when an identical request is already in flight + if (!inFlightRequests.add(requestParams)) { + return AlreadyRunning + } - override fun onFailure(statusCode: Int) { - continuations[requestParams]?.resume(statusCode) - continuations[requestParams] = null + return try { + // Never wait forever: a request that neither succeeds nor fails (e.g. a stalled + // connection) resolves to RequestFailed so the caller can leave the loading state. + val statusCode = withTimeoutOrNull(FETCH_TIMEOUT_MS) { + fetchPostAndWaitForResult(requestParams) } + when (statusCode) { + HttpURLConnection.HTTP_OK -> Success + HttpURLConnection.HTTP_UNAUTHORIZED, HttpURLConnection.HTTP_FORBIDDEN -> NotAuthorised + HttpURLConnection.HTTP_NOT_FOUND -> PostNotFound + else -> RequestFailed + } + } finally { + // Always release the slot, including on timeout or coroutine cancellation, so a + // later retry of the same post isn't wrongly rejected as AlreadyRunning. + inFlightRequests.remove(requestParams) } + } - return suspendCancellableCoroutine { cont -> - continuations[requestParams] = cont + private suspend fun fetchPostAndWaitForResult(requestParams: FetchPostRequestParams): Int = + suspendCancellableCoroutine { cont -> + val listener = object : ReaderActions.OnRequestListener { + override fun onSuccess(result: String?) { + // Guard against resuming an already-cancelled (e.g. timed-out) request + if (cont.isActive) cont.resume(HttpURLConnection.HTTP_OK) + } + + override fun onFailure(statusCode: Int) { + if (cont.isActive) cont.resume(statusCode) + } + } if (requestParams.isFeed) { readerPostActionsWrapper.requestFeedPost( @@ -70,7 +82,6 @@ class ReaderFetchPostUseCase @Inject constructor( ) } } - } sealed class FetchReaderPostState { object Success : FetchReaderPostState() @@ -88,4 +99,8 @@ class ReaderFetchPostUseCase @Inject constructor( val postId: Long, val isFeed: Boolean ) + + companion object { + private const val FETCH_TIMEOUT_MS = 30_000L + } } diff --git a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModel.kt b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModel.kt index b8cc6d50dd03..1f5776e6675e 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModel.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModel.kt @@ -387,13 +387,14 @@ class ReaderPostDetailViewModel @Inject constructor( launch { getOrFetchReaderPost(blogId = blogId, postId = postId) } } - @Suppress("CyclomaticComplexMethod") private suspend fun getOrFetchReaderPost(blogId: Long, postId: Long) { getReaderPostFromDb(blogId = blogId, postId = postId) - // Show cached content immediately if available, otherwise show loading - val hasCachedPost = post != null - if (hasCachedPost) { + // Only render the cached copy immediately if it actually has body content. A + // header-only post (whose body hasn't been fetched yet) would otherwise leave the + // user staring at a blank article, so keep the loading state until the fetch lands. + val hasRenderableContent = post?.hasText() == true + if (hasRenderableContent) { updatePostDetailsUi() } else { _uiState.value = LoadingUiState @@ -401,45 +402,55 @@ class ReaderPostDetailViewModel @Inject constructor( // Always fetch fresh content from the server val oldPostText = post?.text - when (readerFetchPostUseCase.fetchPost(blogId = blogId, postId = postId, isFeed = isFeed)) { - FetchReaderPostState.Success -> { - getReaderPostFromDb(blogId, postId) - // Update UI if content changed, or we didn't have cached content - if (!hasCachedPost || post?.text != oldPostText) { - updatePostDetailsUi() - } - } - - FetchReaderPostState.AlreadyRunning -> { - if (!hasCachedPost) { - AppLog.i(T.READER, "reader post detail > fetch post already running") - _uiState.value = ErrorUiState(null) - } - } - - FetchReaderPostState.Failed.NoNetwork -> { - if (!hasCachedPost) { - _uiState.value = ErrorUiState(UiStringRes(R.string.no_network_message)) - } - } - - FetchReaderPostState.Failed.RequestFailed -> { - if (!hasCachedPost) { - _uiState.value = ErrorUiState(UiStringRes(R.string.reader_err_get_post_generic)) - } - } + val fetchState = readerFetchPostUseCase.fetchPost(blogId = blogId, postId = postId, isFeed = isFeed) + if (fetchState is FetchReaderPostState.Success) { + renderFetchedPost(blogId, postId, hadRenderableContent = hasRenderableContent, oldPostText = oldPostText) + } else if (!hasRenderableContent) { + // A non-success outcome only changes the UI when there's nothing on screen yet; a + // rendered post is kept rather than torn down by a failed background refresh. + showFetchOutcomeWithoutContent(fetchState) + } + } - FetchReaderPostState.Failed.NotAuthorised -> { - if (!hasCachedPost) { - trackAndUpdateNotAuthorisedErrorState() - } + private suspend fun renderFetchedPost( + blogId: Long, + postId: Long, + hadRenderableContent: Boolean, + oldPostText: String? + ) { + getReaderPostFromDb(blogId, postId) + // Apply the same renderability test as the loading gate: a fetch that "succeeds" but + // leaves us with no post, or a post whose body is still empty, must not fall through + // to a blank article — that's the CMM-2254 symptom. + if (post?.hasText() == true) { + // Render when the body changed, or we didn't already have renderable content + if (!hadRenderableContent || post?.text != oldPostText) { + updatePostDetailsUi() } + } else if (!hadRenderableContent) { + // Success but nothing renderable (a missing post or an empty body). Surface an + // error rather than a blank article or an endless spinner. + _uiState.value = ErrorUiState(UiStringRes(R.string.reader_err_get_post_generic)) + } + } - FetchReaderPostState.Failed.PostNotFound -> { - if (!hasCachedPost) { - _uiState.value = ErrorUiState(UiStringRes(R.string.reader_err_get_post_not_found)) - } + private fun showFetchOutcomeWithoutContent(fetchState: FetchReaderPostState) { + when (fetchState) { + FetchReaderPostState.AlreadyRunning -> { + // A sibling fetch for this post is already in flight (e.g. a re-entry after a + // config change); stay on loading and let it resolve the UI. + AppLog.i(T.READER, "reader post detail > fetch post already running") + _uiState.value = LoadingUiState } + FetchReaderPostState.Failed.NoNetwork -> + _uiState.value = ErrorUiState(UiStringRes(R.string.no_network_message)) + FetchReaderPostState.Failed.RequestFailed -> + _uiState.value = ErrorUiState(UiStringRes(R.string.reader_err_get_post_generic)) + FetchReaderPostState.Failed.NotAuthorised -> + trackAndUpdateNotAuthorisedErrorState() + FetchReaderPostState.Failed.PostNotFound -> + _uiState.value = ErrorUiState(UiStringRes(R.string.reader_err_get_post_not_found)) + FetchReaderPostState.Success -> Unit // handled by renderFetchedPost } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/usecases/ReaderFetchPostUseCaseTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/usecases/ReaderFetchPostUseCaseTest.kt index d03add7c7650..a64a8b36e5e3 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/usecases/ReaderFetchPostUseCaseTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/usecases/ReaderFetchPostUseCaseTest.kt @@ -1,6 +1,8 @@ package org.wordpress.android.ui.reader.usecases import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.cancelAndJoin +import kotlinx.coroutines.launch import org.assertj.core.api.Assertions.assertThat import org.junit.Before import org.junit.Test @@ -13,6 +15,7 @@ import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.ui.reader.actions.ReaderActions import org.wordpress.android.ui.reader.actions.ReaderPostActionsWrapper +import org.wordpress.android.ui.reader.usecases.ReaderFetchPostUseCase.FetchReaderPostState.AlreadyRunning import org.wordpress.android.ui.reader.usecases.ReaderFetchPostUseCase.FetchReaderPostState.Failed import org.wordpress.android.ui.reader.usecases.ReaderFetchPostUseCase.FetchReaderPostState.Success import org.wordpress.android.util.NetworkUtilsWrapper @@ -132,4 +135,23 @@ class ReaderFetchPostUseCaseTest : BaseUnitTest() { assertThat(result).isEqualTo(Failed.RequestFailed) } + + @Test + fun `given the request never responds, when reader post is fetched, then request failed is returned`() = test { + // The request mock never invokes its listener, so resolution relies on the timeout + val result = useCase.fetchPost(postId = postId, blogId = blogId, isFeed = false) + + assertThat(result).isEqualTo(Failed.RequestFailed) + } + + @Test + fun `given a request for the same post is running, when fetched again, then already running is returned`() = test { + // The first request stays in flight because its listener is never invoked + val firstRequest = launch { useCase.fetchPost(postId = postId, blogId = blogId, isFeed = false) } + + val secondResult = useCase.fetchPost(postId = postId, blogId = blogId, isFeed = false) + + assertThat(secondResult).isEqualTo(AlreadyRunning) + firstRequest.cancelAndJoin() + } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModelTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModelTest.kt index e470d5703b46..47071097af03 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModelTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModelTest.kt @@ -454,7 +454,7 @@ class ReaderPostDetailViewModelTest : BaseUnitTest() { } @Test - fun `given request already running, when post is fetched, then no error is shown`() = + fun `given request already running, when post is fetched, then loading is kept and no error is shown`() = testWithoutLocalPost { whenever(readerFetchPostUseCase.fetchPost(readerPost.blogId, readerPost.postId, viewModel.isFeed)) .thenReturn(FetchReaderPostState.Success) @@ -464,7 +464,9 @@ class ReaderPostDetailViewModelTest : BaseUnitTest() { viewModel.onShowPost(blogId = readerPost.blogId, postId = readerPost.postId) - assertThat(observers.uiStates.filterIsInstance().last().message).isNull() + // A sibling fetch is still in flight, so we stay on loading rather than showing + // a (message-less) error that would blank the screen mid-load. + assertThat(observers.uiStates.last()).isEqualTo(LoadingUiState) } @Test @@ -481,6 +483,75 @@ class ReaderPostDetailViewModelTest : BaseUnitTest() { .isEqualTo(ErrorUiState(UiStringRes(R.string.reader_err_get_post_not_found))) } + /* SHOW POST - LOAD ALWAYS TERMINATES (CMM-2254) */ + @Test + fun `given cached post has no body, when show post is triggered, then loading state is shown`() = test { + val bodylessPost = createDummyReaderPost(readerPost.postId).apply { text = null } + whenever(readerGetPostUseCase.get(anyLong(), anyLong(), anyBoolean())).thenReturn(Pair(bodylessPost, false)) + val observers = init(showPost = false) + + viewModel.onShowPost(blogId = bodylessPost.blogId, postId = bodylessPost.postId) + + assertThat(observers.uiStates.first()).isEqualTo(LoadingUiState) + } + + @Test + fun `given cached post has no body and fetch fails, when show post is triggered, then error is shown`() = test { + val bodylessPost = createDummyReaderPost(readerPost.postId).apply { text = null } + whenever(readerGetPostUseCase.get(anyLong(), anyLong(), anyBoolean())).thenReturn(Pair(bodylessPost, false)) + whenever(readerFetchPostUseCase.fetchPost(anyLong(), anyLong(), anyBoolean())) + .thenReturn(Failed.RequestFailed) + val observers = init(showPost = false) + + viewModel.onShowPost(blogId = bodylessPost.blogId, postId = bodylessPost.postId) + + assertThat(observers.uiStates.last()) + .isEqualTo(ErrorUiState(UiStringRes(R.string.reader_err_get_post_generic))) + } + + @Test + fun `given fetch succeeds but post is still missing, when show post is triggered, then error is shown`() = + testWithoutLocalPost { + whenever(readerFetchPostUseCase.fetchPost(anyLong(), anyLong(), anyBoolean())) + .thenReturn(FetchReaderPostState.Success) + val observers = init(showPost = false) + + viewModel.onShowPost(blogId = readerPost.blogId, postId = readerPost.postId) + + assertThat(observers.uiStates.last()) + .isEqualTo(ErrorUiState(UiStringRes(R.string.reader_err_get_post_generic))) + } + + @Test + fun `given fetch succeeds but body is empty, when show post is triggered, then error is shown`() = test { + val bodylessPost = createDummyReaderPost(readerPost.postId).apply { text = null } + whenever(readerGetPostUseCase.get(anyLong(), anyLong(), anyBoolean())).thenReturn(Pair(bodylessPost, false)) + whenever(readerFetchPostUseCase.fetchPost(anyLong(), anyLong(), anyBoolean())) + .thenReturn(FetchReaderPostState.Success) + val observers = init(showPost = false) + + viewModel.onShowPost(blogId = bodylessPost.blogId, postId = bodylessPost.postId) + + assertThat(observers.uiStates.last()) + .isEqualTo(ErrorUiState(UiStringRes(R.string.reader_err_get_post_generic))) + } + + @Test + fun `given rendered post and empty refetch, when show post is triggered, then content is kept`() = test { + val bodylessPost = createDummyReaderPost(readerPost.postId).apply { text = null } + whenever(readerGetPostUseCase.get(anyLong(), anyLong(), anyBoolean())) + .thenReturn(Pair(readerPost, false)) + .thenReturn(Pair(bodylessPost, false)) + whenever(readerFetchPostUseCase.fetchPost(anyLong(), anyLong(), anyBoolean())) + .thenReturn(FetchReaderPostState.Success) + val observers = init(showPost = false) + + viewModel.onShowPost(blogId = readerPost.blogId, postId = readerPost.postId) + + assertThat(observers.uiStates.last()).isInstanceOf(ReaderPostDetailsUiState::class.java) + assertThat(observers.uiStates.none { it is ErrorUiState }).isTrue + } + @Test fun `given unauthorised, when post is fetched, then error ui is shown`() = testWithoutLocalPost { whenever(readerFetchPostUseCase.fetchPost(readerPost.blogId, readerPost.postId, viewModel.isFeed)) @@ -1241,6 +1312,7 @@ class ReaderPostDetailViewModelTest : BaseUnitTest() { this.blogId = id * 100 this.feedId = id * 1000 this.title = "DummyPost" + this.text = "

Dummy post content

" this.featuredVideo = id.toString() this.featuredImage = "/featured_image/$id/url" this.isExternal = !isWpComPost