diff --git a/RELEASE-NOTES.txt b/RELEASE-NOTES.txt index 53167ba9778b..bb24650e58ca 100644 --- a/RELEASE-NOTES.txt +++ b/RELEASE-NOTES.txt @@ -3,6 +3,7 @@ 27.0 ----- * [*] You can now browse Google Photos (albums, collections, and search) when adding photos or videos from your device. +* [*] Stats now refresh when you return to the screen, so your latest data appears without a manual pull-to-refresh. [https://github.com/wordpress-mobile/WordPress-Android/pull/23112] 26.9 diff --git a/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/StatsListFragment.kt b/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/StatsListFragment.kt index 4fbbadfbf166..a649b3209b79 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/StatsListFragment.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/StatsListFragment.kt @@ -212,6 +212,14 @@ class StatsListFragment : ViewPagerFragment(R.layout.stats_list_fragment), PullT @Suppress("DEPRECATION") setHasOptionsMenu(statsSection == StatsSection.INSIGHTS) (parentFragment as? StatsPullToRefreshListener.PullToRefreshReceiverListener)?.setPullToRefreshReceiver(this) + // Re-fetch when the user returns to the screen so newly-available (or previously stale) stats + // appear without a manual pull-to-refresh. The stats use cases are process-lifetime singletons + // that otherwise keep serving their last in-memory result. This uses the non-forced refresh + // path, so StatsRequestSqlUtils.STALE_PERIOD throttles it to at most one network request per + // 5 minutes; resumes within that window are served from cache. + if (::viewModel.isInitialized) { + viewModel.onRefresh() + } } override fun onDestroyView() { diff --git a/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/UiModelMapper.kt b/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/UiModelMapper.kt index 6c2169d85328..1343f466fec3 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/UiModelMapper.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/UiModelMapper.kt @@ -31,7 +31,9 @@ class UiModelMapper ) LOADING -> StatsBlock.Loading( useCaseModel.type, - useCaseModel.stateData ?: useCaseModel.data ?: listOf() + // Keep already-loaded rows visible during a refresh; fall back to the + // loading placeholder only on first load (no data yet). + useCaseModel.data ?: useCaseModel.stateData ?: listOf() ) EMPTY -> StatsBlock.EmptyBlock( useCaseModel.type, @@ -70,9 +72,12 @@ class UiModelMapper } else if (!allFailing && !allFailingWithoutData) { val data = useCaseModels.mapNotNull { useCaseModel -> when (useCaseModel.state) { - LOADING -> useCaseModel.stateData?.let { - StatsBlock.Loading(useCaseModel.type, useCaseModel.stateData) - } + // Keep already-loaded rows visible during a refresh; fall back to the loading + // placeholder only on first load (no data yet). + LOADING -> StatsBlock.Loading( + useCaseModel.type, + useCaseModel.data ?: useCaseModel.stateData ?: listOf() + ) SUCCESS -> StatsBlock.Success(useCaseModel.type, useCaseModel.data ?: listOf()) ERROR -> useCaseModel.stateData?.let { @@ -114,9 +119,12 @@ class UiModelMapper UiModel.Success( useCaseModels.mapNotNull { useCaseModel -> when { - useCaseModel.state == LOADING -> useCaseModel.stateData?.let { - StatsBlock.Loading(useCaseModel.type, useCaseModel.stateData) - } + // Keep already-loaded rows visible during a refresh; fall back to the + // loading placeholder only on first load (no data yet). + useCaseModel.state == LOADING -> StatsBlock.Loading( + useCaseModel.type, + useCaseModel.data ?: useCaseModel.stateData ?: listOf() + ) useCaseModel.type == overViewType && useCaseModel.data != null -> StatsBlock.Success( useCaseModel.type, diff --git a/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCase.kt b/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCase.kt index 8708bfd4862e..243dbccb1c2e 100644 --- a/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCase.kt +++ b/WordPress/src/main/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCase.kt @@ -2,6 +2,7 @@ package org.wordpress.android.ui.stats.refresh.lists.sections import androidx.lifecycle.LiveData import androidx.lifecycle.MutableLiveData +import java.util.concurrent.atomic.AtomicBoolean import kotlinx.coroutines.CoroutineDispatcher import kotlinx.coroutines.CoroutineScope import kotlinx.coroutines.Job @@ -44,6 +45,7 @@ abstract class BaseStatsUseCase( private var domainModel: DOMAIN_MODEL? = null private var uiState: UI_STATE = defaultUiState private var updateJob: Job? = null + private val fetchInProgress = AtomicBoolean(false) private val _liveData = MutableLiveData() val liveData: LiveData = _liveData @@ -72,9 +74,22 @@ abstract class BaseStatsUseCase( } } if (refresh || domainState != SUCCESS || emptyDb) { + // Guard against duplicate concurrent loads — e.g. the initial start() load racing the + // onResume refresh. A forced refresh (pull-to-refresh) still proceeds so it can bypass + // the STALE_PERIOD cache. + val startedFetch = fetchInProgress.compareAndSet(false, true) + if (!startedFetch && !forced) { + return + } updateUseCaseState(LOADING) - val state = fetchRemoteData(forced) - evaluateState(state) + try { + val state = fetchRemoteData(forced) + evaluateState(state) + } finally { + if (startedFetch) { + fetchInProgress.set(false) + } + } } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/stats/refresh/lists/UiModelMapperTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/stats/refresh/lists/UiModelMapperTest.kt index 951ca90a5335..e48e56ed78c2 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/stats/refresh/lists/UiModelMapperTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/stats/refresh/lists/UiModelMapperTest.kt @@ -9,9 +9,12 @@ import org.wordpress.android.BaseUnitTest import org.wordpress.android.R import org.wordpress.android.fluxc.store.StatsStore.InsightType.TOTAL_FOLLOWERS import org.wordpress.android.fluxc.store.StatsStore.ManagementType +import org.wordpress.android.fluxc.store.StatsStore.SubscriberType.EMAILS import org.wordpress.android.ui.stats.refresh.lists.StatsListViewModel.UiModel import org.wordpress.android.ui.stats.refresh.lists.sections.BaseStatsUseCase.UseCaseModel +import org.wordpress.android.ui.stats.refresh.lists.sections.BaseStatsUseCase.UseCaseModel.UseCaseState.LOADING import org.wordpress.android.ui.stats.refresh.lists.sections.BaseStatsUseCase.UseCaseModel.UseCaseState.SUCCESS +import org.wordpress.android.ui.stats.refresh.lists.sections.BlockListItem import org.wordpress.android.util.NetworkUtilsWrapper @ExperimentalCoroutinesApi @@ -66,4 +69,34 @@ import org.wordpress.android.util.NetworkUtilsWrapper assertThat(model.showButton).isTrue() assertThat(error).isNull() } + + @Test + fun `mapSubscribers keeps loaded rows visible while a block is refreshing`() { + val dataRows = listOf(BlockListItem.Divider) + val loadingPlaceholder = listOf(BlockListItem.Divider) + + val uiModel = mapper.mapSubscribers( + listOf(UseCaseModel(EMAILS, data = dataRows, stateData = loadingPlaceholder, state = LOADING)) + ) {} + + val model = uiModel as UiModel.Success + assertThat(model.data).hasSize(1) + assertThat(model.data[0].type).isEqualTo(StatsBlock.Type.LOADING) + // The already-loaded rows stay on screen during the refresh, not the loading placeholder. + assertThat(model.data[0].data).isSameAs(dataRows) + } + + @Test + fun `mapSubscribers shows the loading placeholder on first load when there is no data yet`() { + val loadingPlaceholder = listOf(BlockListItem.Divider) + + val uiModel = mapper.mapSubscribers( + listOf(UseCaseModel(EMAILS, data = null, stateData = loadingPlaceholder, state = LOADING)) + ) {} + + val model = uiModel as UiModel.Success + assertThat(model.data).hasSize(1) + assertThat(model.data[0].type).isEqualTo(StatsBlock.Type.LOADING) + assertThat(model.data[0].data).isSameAs(loadingPlaceholder) + } } diff --git a/WordPress/src/test/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCaseTest.kt b/WordPress/src/test/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCaseTest.kt index ba91233d9637..fe3cd56d75ea 100644 --- a/WordPress/src/test/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCaseTest.kt +++ b/WordPress/src/test/java/org/wordpress/android/ui/stats/refresh/lists/sections/BaseStatsUseCaseTest.kt @@ -1,6 +1,8 @@ package org.wordpress.android.ui.stats.refresh.lists.sections +import kotlinx.coroutines.CompletableDeferred import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.launch import kotlinx.coroutines.test.UnconfinedTestDispatcher import kotlinx.coroutines.test.advanceUntilIdle import org.assertj.core.api.Assertions.assertThat @@ -8,6 +10,8 @@ import org.junit.After import org.junit.Before import org.junit.Test import org.mockito.Mock +import org.mockito.kotlin.times +import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import org.wordpress.android.BaseUnitTest import org.wordpress.android.R @@ -95,6 +99,40 @@ class BaseStatsUseCaseTest : BaseUnitTest() { assertThat(block.liveData.value?.state).isEqualTo(UseCaseState.LOADING) } + @Test + fun `concurrent non-forced fetches only trigger one remote load`() = test { + whenever(localDataProvider.get()).thenReturn(null) + val gate = CompletableDeferred() + val gatedBlock = TestUseCase(localDataProvider, remoteDataProvider, loadingData, gate) + + launch { gatedBlock.fetch(false, false) } // acquires the in-flight guard, suspends at the gate + advanceUntilIdle() + launch { gatedBlock.fetch(true, false) } // a load is already in flight -> should be skipped + advanceUntilIdle() + gate.complete(Unit) + advanceUntilIdle() + + verify(remoteDataProvider, times(1)).get() + gatedBlock.clear() + } + + @Test + fun `a forced fetch is not skipped while a load is in flight`() = test { + whenever(localDataProvider.get()).thenReturn(null) + val gate = CompletableDeferred() + val gatedBlock = TestUseCase(localDataProvider, remoteDataProvider, loadingData, gate) + + launch { gatedBlock.fetch(false, false) } // non-forced load in flight + advanceUntilIdle() + launch { gatedBlock.fetch(true, true) } // forced (pull-to-refresh) must still hit remote + advanceUntilIdle() + gate.complete(Unit) + advanceUntilIdle() + + verify(remoteDataProvider, times(2)).get() + gatedBlock.clear() + } + @After fun tearDown() { block.clear() @@ -109,7 +147,8 @@ class BaseStatsUseCaseTest : BaseUnitTest() { class TestUseCase( private val localDataProvider: Provider, private val remoteDataProvider: Provider, - private val loadingItems: List + private val loadingItems: List, + private val remoteGate: CompletableDeferred? = null ) : BaseStatsUseCase( ALL_TIME_STATS, UnconfinedTestDispatcher(), @@ -130,6 +169,7 @@ class BaseStatsUseCaseTest : BaseUnitTest() { } override suspend fun fetchRemoteData(forced: Boolean): State { + remoteGate?.await() val domainModel = remoteDataProvider.get() return if (domainModel != null) { State.Data(domainModel)