From 1a062ae0e036b858f9126f0bd270f7b9076b13fd Mon Sep 17 00:00:00 2001 From: Annmarie Ziegler Date: Tue, 4 Jun 2024 17:17:40 -0400 Subject: [PATCH 1/4] Add an ongoingRequest hashset to track fetch requests --- .../ui/posts/PostListFeaturedImageTracker.kt | 22 +++++++++++++++++-- 1 file changed, 20 insertions(+), 2 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt index d94dd6e0a582..986402011e80 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt @@ -23,12 +23,23 @@ class PostListFeaturedImageTracker(private val dispatcher: Dispatcher, private v */ @SuppressLint("UseSparseArrays") private val featuredImageMap = HashMap() + private val ongoingRequests = HashSet() fun getFeaturedImageUrl(site: SiteModel, featuredImageId: Long): String? { if (featuredImageId == 0L) { return null } - featuredImageMap[featuredImageId]?.let { return it } + + featuredImageMap[featuredImageId]?.let { + return it + } + + // Check if a request for this image is already ongoing + if (ongoingRequests.contains(featuredImageId)) { + // If the request is ongoing, just return. The callback will be invoked upon completion. + return null + } + mediaStore.getSiteMediaWithId(site, featuredImageId)?.let { media -> // This should be a pretty rare case, but some media seems to be missing url return if (media.url.isNotBlank()) { @@ -36,7 +47,11 @@ class PostListFeaturedImageTracker(private val dispatcher: Dispatcher, private v media.url } else null } + // Media is not in the Store, we need to download it + // Mark the request as ongoing + ongoingRequests.add(featuredImageId) + val mediaToDownload = MediaModel( site.id, featuredImageId @@ -47,6 +62,9 @@ class PostListFeaturedImageTracker(private val dispatcher: Dispatcher, private v } fun invalidateFeaturedMedia(featuredImageIds: List) { - featuredImageIds.forEach { featuredImageMap.remove(it) } + featuredImageIds.forEach { + featuredImageMap.remove(it) + ongoingRequests.remove(it) + } } } From dc601e9583fdf7391bee6cf2057dbec90040b85a Mon Sep 17 00:00:00 2001 From: Annmarie Ziegler Date: Tue, 4 Jun 2024 17:18:24 -0400 Subject: [PATCH 2/4] Invoke featureMediaChanged on error and success --- .../org/wordpress/android/ui/posts/PostListEventListener.kt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListEventListener.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListEventListener.kt index 16d7e3067a53..85e775d67c66 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListEventListener.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListEventListener.kt @@ -174,8 +174,8 @@ class PostListEventListener( @Suppress("unused", "SpreadOperator") @Subscribe(threadMode = BACKGROUND) fun onMediaChanged(event: OnMediaChanged) { + featuredMediaChanged(*event.mediaList.map { it.mediaId }.toLongArray()) if (!event.isError) { - featuredMediaChanged(*event.mediaList.map { it.mediaId }.toLongArray()) uploadStatusChanged(*event.mediaList.map { it.localPostId }.toIntArray()) } } From bc7d8c5e45d1de4fe4fdd3a746cb37136cec5391 Mon Sep 17 00:00:00 2001 From: Annmarie Ziegler Date: Tue, 4 Jun 2024 18:12:36 -0400 Subject: [PATCH 3/4] Mark the mapSet and hashMap as internal and set visible for testing --- .../android/ui/posts/PostListFeaturedImageTracker.kt | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt b/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt index 986402011e80..32a9a63b2518 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/posts/PostListFeaturedImageTracker.kt @@ -1,6 +1,7 @@ package org.wordpress.android.ui.posts import android.annotation.SuppressLint +import androidx.annotation.VisibleForTesting import org.wordpress.android.fluxc.Dispatcher import org.wordpress.android.fluxc.generated.MediaActionBuilder import org.wordpress.android.fluxc.model.MediaModel @@ -22,8 +23,11 @@ class PostListFeaturedImageTracker(private val dispatcher: Dispatcher, private v https://github.com/wordpress-mobile/WordPress-Android/issues/11487 */ @SuppressLint("UseSparseArrays") - private val featuredImageMap = HashMap() - private val ongoingRequests = HashSet() + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + internal val featuredImageMap = HashMap() + + @VisibleForTesting(otherwise = VisibleForTesting.PRIVATE) + internal val ongoingRequests = HashSet() fun getFeaturedImageUrl(site: SiteModel, featuredImageId: Long): String? { if (featuredImageId == 0L) { From 0637eec7fd2af624a0396fd396aa89bceac0233b Mon Sep 17 00:00:00 2001 From: Annmarie Ziegler Date: Tue, 4 Jun 2024 18:12:58 -0400 Subject: [PATCH 4/4] Add unit tests for PostListFeaturedImageTracker --- .../posts/PostListFeaturedImageTrackerTest.kt | 111 ++++++++++++++++++ 1 file changed, 111 insertions(+) create mode 100644 WordPress/src/test/java/org/wordpress/android/ui/posts/PostListFeaturedImageTrackerTest.kt diff --git a/WordPress/src/test/java/org/wordpress/android/ui/posts/PostListFeaturedImageTrackerTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/posts/PostListFeaturedImageTrackerTest.kt new file mode 100644 index 000000000000..a9616c3fd5b1 --- /dev/null +++ b/WordPress/src/test/java/org/wordpress/android/ui/posts/PostListFeaturedImageTrackerTest.kt @@ -0,0 +1,111 @@ +package org.wordpress.android.ui.posts + +import kotlinx.coroutines.ExperimentalCoroutinesApi +import org.junit.Assert.assertEquals +import org.junit.Assert.assertNull +import org.junit.Before +import org.mockito.kotlin.any +import org.mockito.kotlin.mock +import org.mockito.kotlin.never +import org.mockito.kotlin.verify +import org.mockito.kotlin.whenever +import org.wordpress.android.BaseUnitTest +import org.wordpress.android.fluxc.Dispatcher +import org.wordpress.android.fluxc.model.MediaModel +import org.wordpress.android.fluxc.model.SiteModel +import org.wordpress.android.fluxc.store.MediaStore +import kotlin.test.Test + +@Suppress("UNCHECKED_CAST") +@ExperimentalCoroutinesApi +class PostListFeaturedImageTrackerTest : BaseUnitTest() { + private val dispatcher: Dispatcher = mock() + private val mediaStore: MediaStore = mock() + + private lateinit var tracker: PostListFeaturedImageTracker + + private val site = SiteModel().apply { id = 123 } + + @Before + fun setup() { + tracker = PostListFeaturedImageTracker(dispatcher, mediaStore) + } + + @Test + fun `given id exists in map, when getFeaturedImageUrl invoked, then return url`() { + val imageId = 123L + val imageUrl = "https://example.com/image.jpg" + tracker.featuredImageMap[imageId] = imageUrl + + val result = tracker.getFeaturedImageUrl(site, imageId) + + assertEquals(imageUrl, result) + } + + @Test + fun `given id is 0, when getFeaturedImageUrl invoked, then return null`() { + val result = tracker.getFeaturedImageUrl(site, 0L) + + assertNull(result) + } + + @Test + fun `given id not in map and exists in store, when invoked, then return url from media store`() { + val imageId = 456L + val imageUrl = "https://example.com/image.jpg" + val mediaModel = MediaModel(site.id, imageId).apply { + url = imageUrl + } + + whenever(mediaStore.getSiteMediaWithId(site, imageId)).thenReturn(mediaModel) + + val result = tracker.getFeaturedImageUrl(site, imageId) + + assertEquals(imageUrl, result) + assertEquals(imageUrl, tracker.featuredImageMap[imageId]) + } + + @Test + fun `given id not in map or store, when invoked, then return null and dispatch fetch request`() { + val imageId = 123L + + whenever(mediaStore.getSiteMediaWithId(site, imageId)).thenReturn(null) + + val result = tracker.getFeaturedImageUrl(site, imageId) + + assertNull(result) + verify(dispatcher).dispatch(any()) + assert(tracker.ongoingRequests.contains(imageId)) + } + + @Test + fun `given request ongoing for id, when invoked, should return null`() { + val imageId = 123L + + tracker.ongoingRequests.add(imageId) + + val result = tracker.getFeaturedImageUrl(site, imageId) + + assertNull(result) + verify(mediaStore, never()).getSiteMediaWithId(site, imageId) + verify(dispatcher, never()).dispatch(any()) + } + + @Test + fun `given id in map and ongoingRequests, when invalidate, then remove id from map and ongoingRequests`() { + val imageId1 = 123L + val imageId2 = 456L + + tracker.featuredImageMap[imageId1] = "https://example.com/image1.jpg" + tracker.featuredImageMap[imageId2] = "https://example.com/image2.jpg" + tracker.ongoingRequests.add(imageId1) + tracker.ongoingRequests.add(imageId2) + + tracker.invalidateFeaturedMedia(listOf(imageId1, imageId2)) + + assertNull(tracker.featuredImageMap[imageId1]) + assertNull(tracker.featuredImageMap[imageId2]) + assert(!tracker.ongoingRequests.contains(imageId1)) + assert(!tracker.ongoingRequests.contains(imageId2)) + } +}