Reader: never leave the post detail stuck on an endless spinner (CMM-2254)

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<FetchPostRequestParams, Continuation<Int>?> = 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<FetchPostRequestParams>())
 
+    @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<String> {
-            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<String> {
+                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
+    }
 }

WordPress/src/main/java/org/wordpress/android/ui/reader/viewmodels/ReaderPostDetailViewModel.kt

@@ -387,59 +387,70 @@ 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
         }
 
         // 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
         }
     }
 

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()
+    }
 }

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<ErrorUiState>().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 = "<p>Dummy post content</p>"
             this.featuredVideo = id.toString()
             this.featuredImage = "/featured_image/$id/url"
             this.isExternal = !isWpComPost