Skip to content

Commit 4328d05

Browse files
adalpariadalpari
authored
Fix/stats cards and networking (#22586)
* Using the new WPComLanguage * Add authors card * Title fix * Ading tests * detekt * Extracting common code * More code extraction * Renaming * Reverting rollback change by mistake * Updating version * Checking token * Division by zero fix * Fixing the max authors issue * Fix tests * detekt * Using the proper endpoint * Update RS * Stop duplicating cards calls * Only fetching visible cards * Fixing loading initial value * Fixing tests * detekt * Preventing loadDataIfNeeded race condition * Some Pr suggestions * Adding tests * Claude suggestions: load data state and country code check * Adding missing tests * Fixing the double endpoint call to top posts and referrers
1 parent 2155ca5 commit 4328d05

File tree

13 files changed

+592
-133
lines changed

13 files changed

+592
-133
lines changed

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/NewStatsActivity.kt‎

Lines changed: 84 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -244,25 +244,53 @@ private fun TrafficTabContent(
244244
val selectedPeriod by viewsStatsViewModel.selectedPeriod.collectAsState()
245245
val isTodaysStatsRefreshing by todaysStatsViewModel.isRefreshing.collectAsState()
246246
val isViewsStatsRefreshing by viewsStatsViewModel.isRefreshing.collectAsState()
247-
val isMostViewedRefreshing by mostViewedViewModel.isRefreshing.collectAsState()
247+
val isMostViewedPostsRefreshing by mostViewedViewModel
248+
.isPostsRefreshing.collectAsState()
249+
val isMostViewedReferrersRefreshing by mostViewedViewModel
250+
.isReferrersRefreshing.collectAsState()
248251
val isCountriesRefreshing by countriesViewModel.isRefreshing.collectAsState()
249252
val isAuthorsRefreshing by authorsViewModel.isRefreshing.collectAsState()
250-
val isRefreshing = isTodaysStatsRefreshing || isViewsStatsRefreshing ||
251-
isMostViewedRefreshing || isCountriesRefreshing || isAuthorsRefreshing
253+
val isRefreshing = listOf(
254+
isTodaysStatsRefreshing, isViewsStatsRefreshing,
255+
isMostViewedPostsRefreshing, isMostViewedReferrersRefreshing,
256+
isCountriesRefreshing, isAuthorsRefreshing
257+
).any { it }
252258
val pullToRefreshState = rememberPullToRefreshState()
253259

254260
// Card configuration state
255261
val visibleCards by newStatsViewModel.visibleCards.collectAsState()
256262
val hiddenCards by newStatsViewModel.hiddenCards.collectAsState()
257263
val isNetworkAvailable by newStatsViewModel.isNetworkAvailable.collectAsState()
264+
val cardsToLoad by newStatsViewModel.cardsToLoad.collectAsState()
265+
val isPeriodInitialized by viewsStatsViewModel.isPeriodInitialized.collectAsState()
258266
var showAddCardSheet by remember { mutableStateOf(false) }
259267
val addCardSheetState = rememberModalBottomSheetState()
260268

261-
// Propagate period changes to the MostViewedViewModel, CountriesViewModel, and AuthorsViewModel
262-
LaunchedEffect(selectedPeriod) {
263-
mostViewedViewModel.onPeriodChanged(selectedPeriod)
264-
countriesViewModel.onPeriodChanged(selectedPeriod)
265-
authorsViewModel.onPeriodChanged(selectedPeriod)
269+
// Propagate period changes only to visible card ViewModels.
270+
// cardsToLoad is empty until the configuration is loaded from the repository,
271+
// preventing data fetches for the default card set before the real config is known.
272+
// isPeriodInitialized prevents fetching with a stale default period before the
273+
// persisted period is restored from preferences.
274+
LaunchedEffect(selectedPeriod, cardsToLoad, isPeriodInitialized) {
275+
if (!isPeriodInitialized) return@LaunchedEffect
276+
cardsToLoad.dispatchToVisibleCards(
277+
onTodaysStats = { todaysStatsViewModel.loadDataIfNeeded() },
278+
onViewsStats = { viewsStatsViewModel.loadDataIfNeeded() },
279+
onMostViewedPosts = {
280+
mostViewedViewModel.onPeriodChangedPosts(selectedPeriod)
281+
},
282+
onMostViewedReferrers = {
283+
mostViewedViewModel.onPeriodChangedReferrers(
284+
selectedPeriod
285+
)
286+
},
287+
onCountries = {
288+
countriesViewModel.onPeriodChanged(selectedPeriod)
289+
},
290+
onAuthors = {
291+
authorsViewModel.onPeriodChanged(selectedPeriod)
292+
}
293+
)
266294
}
267295

268296
if (showAddCardSheet) {
@@ -280,18 +308,25 @@ private fun TrafficTabContent(
280308
// Once user retries or network becomes available, show cards instead
281309
var showNoConnectionScreen by remember { mutableStateOf(!isNetworkAvailable) }
282310

311+
val loadVisibleCards = {
312+
visibleCards.dispatchToVisibleCards(
313+
onTodaysStats = { todaysStatsViewModel.loadData() },
314+
onViewsStats = { viewsStatsViewModel.loadData() },
315+
onMostViewedPosts = { mostViewedViewModel.loadPosts() },
316+
onMostViewedReferrers = {
317+
mostViewedViewModel.loadReferrers()
318+
},
319+
onCountries = { countriesViewModel.loadData() },
320+
onAuthors = { authorsViewModel.loadData() }
321+
)
322+
}
323+
283324
// React to network availability changes
284325
LaunchedEffect(isNetworkAvailable) {
285326
if (isNetworkAvailable && showNoConnectionScreen) {
286-
// Network became available while on no-connection screen - auto-load
287327
showNoConnectionScreen = false
288-
todaysStatsViewModel.loadData()
289-
viewsStatsViewModel.loadData()
290-
mostViewedViewModel.loadData()
291-
countriesViewModel.loadData()
292-
authorsViewModel.loadData()
328+
loadVisibleCards()
293329
} else if (!isNetworkAvailable && !showNoConnectionScreen) {
294-
// Network became unavailable while viewing cards - show no-connection screen
295330
showNoConnectionScreen = true
296331
}
297332
}
@@ -303,11 +338,7 @@ private fun TrafficTabContent(
303338
val isAvailable = newStatsViewModel.checkNetworkStatus()
304339
if (isAvailable) {
305340
showNoConnectionScreen = false
306-
todaysStatsViewModel.loadData()
307-
viewsStatsViewModel.loadData()
308-
mostViewedViewModel.loadData()
309-
countriesViewModel.loadData()
310-
authorsViewModel.loadData()
341+
loadVisibleCards()
311342
}
312343
}
313344
)
@@ -320,11 +351,18 @@ private fun TrafficTabContent(
320351
state = pullToRefreshState,
321352
onRefresh = {
322353
newStatsViewModel.checkNetworkStatus()
323-
todaysStatsViewModel.refresh()
324-
viewsStatsViewModel.refresh()
325-
mostViewedViewModel.refresh()
326-
countriesViewModel.refresh()
327-
authorsViewModel.refresh()
354+
visibleCards.dispatchToVisibleCards(
355+
onTodaysStats = { todaysStatsViewModel.refresh() },
356+
onViewsStats = { viewsStatsViewModel.refresh() },
357+
onMostViewedPosts = {
358+
mostViewedViewModel.refreshPosts()
359+
},
360+
onMostViewedReferrers = {
361+
mostViewedViewModel.refreshReferrers()
362+
},
363+
onCountries = { countriesViewModel.refresh() },
364+
onAuthors = { authorsViewModel.refresh() }
365+
)
328366
},
329367
indicator = {
330368
PullToRefreshDefaults.Indicator(
@@ -508,6 +546,27 @@ private fun AddCardButton(
508546
}
509547
}
510548

549+
@Suppress("LongParameterList")
550+
private fun List<StatsCardType>.dispatchToVisibleCards(
551+
onTodaysStats: () -> Unit,
552+
onViewsStats: () -> Unit,
553+
onMostViewedPosts: () -> Unit,
554+
onMostViewedReferrers: () -> Unit,
555+
onCountries: () -> Unit,
556+
onAuthors: () -> Unit
557+
) {
558+
if (StatsCardType.TODAYS_STATS in this) onTodaysStats()
559+
if (StatsCardType.VIEWS_STATS in this) onViewsStats()
560+
if (StatsCardType.MOST_VIEWED_POSTS_AND_PAGES in this) {
561+
onMostViewedPosts()
562+
}
563+
if (StatsCardType.MOST_VIEWED_REFERRERS in this) {
564+
onMostViewedReferrers()
565+
}
566+
if (StatsCardType.COUNTRIES in this) onCountries()
567+
if (StatsCardType.AUTHORS in this) onAuthors()
568+
}
569+
511570
@Composable
512571
private fun NoConnectionContent(
513572
onRetry: () -> Unit

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/NewStatsViewModel.kt‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,14 @@ class NewStatsViewModel @Inject constructor(
3131
private val _isNetworkAvailable = MutableStateFlow(true)
3232
val isNetworkAvailable: StateFlow<Boolean> = _isNetworkAvailable.asStateFlow()
3333

34+
/**
35+
* Cards whose data should be loaded. Empty until configuration is loaded from the
36+
* repository to prevent fetching data for default cards that may not be visible.
37+
* After configuration loads, mirrors [visibleCards].
38+
*/
39+
private val _cardsToLoad = MutableStateFlow<List<StatsCardType>>(emptyList())
40+
val cardsToLoad: StateFlow<List<StatsCardType>> = _cardsToLoad.asStateFlow()
41+
3442
private val siteId: Long
3543
get() = selectedSiteRepository.getSelectedSite()?.siteId ?: 0L
3644

@@ -58,7 +66,8 @@ class NewStatsViewModel @Inject constructor(
5866
private fun observeConfigurationChanges() {
5967
viewModelScope.launch {
6068
cardConfigurationRepository.configurationFlow.collect { pair ->
61-
if (pair != null && pair.first == siteId) {
69+
val currentSiteId = siteId
70+
if (pair != null && pair.first == currentSiteId) {
6271
updateFromConfiguration(pair.second)
6372
}
6473
}
@@ -68,6 +77,7 @@ class NewStatsViewModel @Inject constructor(
6877
private fun updateFromConfiguration(config: StatsCardsConfiguration) {
6978
_visibleCards.value = config.visibleCards
7079
_hiddenCards.value = config.hiddenCards()
80+
_cardsToLoad.value = config.visibleCards
7181
}
7282

7383
fun removeCard(cardType: StatsCardType) {

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/authors/AuthorsViewModel.kt‎

Lines changed: 20 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -37,29 +37,33 @@ class AuthorsViewModel @Inject constructor(
3737
val isRefreshing: StateFlow<Boolean> = _isRefreshing.asStateFlow()
3838

3939
private var currentPeriod: StatsPeriod = StatsPeriod.Last7Days
40+
private var loadingPeriod: StatsPeriod? = null
41+
private var loadedPeriod: StatsPeriod? = null
4042

4143
private var allAuthors: List<AuthorUiItem> = emptyList()
4244
private var cachedTotalViews: Long = 0L
4345
private var cachedTotalViewsChange: Long = 0L
4446
private var cachedTotalViewsChangePercent: Double = 0.0
4547

46-
init {
47-
loadData()
48-
}
49-
5048
fun loadData() {
5149
val site = selectedSiteRepository.getSelectedSite()
5250
if (site == null) {
51+
loadingPeriod = null
5352
_uiState.value = AuthorsCardUiState.Error(
54-
resourceProvider.getString(R.string.stats_todays_stats_no_site_selected)
53+
resourceProvider.getString(
54+
R.string.stats_todays_stats_no_site_selected
55+
)
5556
)
5657
return
5758
}
5859

5960
val accessToken = accountStore.accessToken
6061
if (accessToken.isNullOrEmpty()) {
62+
loadingPeriod = null
6163
_uiState.value = AuthorsCardUiState.Error(
62-
resourceProvider.getString(R.string.stats_todays_stats_failed_to_load)
64+
resourceProvider.getString(
65+
R.string.stats_todays_stats_failed_to_load
66+
)
6367
)
6468
return
6569
}
@@ -68,7 +72,11 @@ class AuthorsViewModel @Inject constructor(
6872
_uiState.value = AuthorsCardUiState.Loading
6973

7074
viewModelScope.launch {
71-
fetchTopAuthors(site)
75+
try {
76+
fetchTopAuthors(site)
77+
} finally {
78+
loadingPeriod = null
79+
}
7280
}
7381
}
7482

@@ -90,10 +98,10 @@ class AuthorsViewModel @Inject constructor(
9098
}
9199

92100
fun onPeriodChanged(period: StatsPeriod) {
93-
if (currentPeriod != period) {
94-
currentPeriod = period
95-
loadData()
96-
}
101+
if (loadedPeriod == period || loadingPeriod == period) return
102+
loadingPeriod = period
103+
currentPeriod = period
104+
loadData()
97105
}
98106

99107
fun getDetailData(): AuthorsDetailData {
@@ -111,6 +119,7 @@ class AuthorsViewModel @Inject constructor(
111119

112120
when (val result = statsRepository.fetchTopAuthors(siteId, currentPeriod)) {
113121
is TopAuthorsResult.Success -> {
122+
loadedPeriod = currentPeriod
114123
cachedTotalViews = result.totalViews
115124
cachedTotalViewsChange = result.totalViewsChange
116125
cachedTotalViewsChangePercent = result.totalViewsChangePercent

‎WordPress/src/main/java/org/wordpress/android/ui/newstats/countries/CountriesViewModel.kt‎

Lines changed: 25 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ import javax.inject.Inject
2121
import kotlin.math.abs
2222

2323
private const val CARD_MAX_ITEMS = 10
24+
private const val MAX_COUNTRY_CODE_LENGTH = 2
2425

2526
@HiltViewModel
2627
class CountriesViewModel @Inject constructor(
@@ -36,6 +37,8 @@ class CountriesViewModel @Inject constructor(
3637
val isRefreshing: StateFlow<Boolean> = _isRefreshing.asStateFlow()
3738

3839
private var currentPeriod: StatsPeriod = StatsPeriod.Last7Days
40+
private var loadingPeriod: StatsPeriod? = null
41+
private var loadedPeriod: StatsPeriod? = null
3942

4043
private var allCountries: List<CountryItem> = emptyList()
4144
private var cachedMapData: String = ""
@@ -45,23 +48,25 @@ class CountriesViewModel @Inject constructor(
4548
private var cachedTotalViewsChange: Long = 0L
4649
private var cachedTotalViewsChangePercent: Double = 0.0
4750

48-
init {
49-
loadData()
50-
}
51-
5251
fun loadData() {
5352
val site = selectedSiteRepository.getSelectedSite()
5453
if (site == null) {
54+
loadingPeriod = null
5555
_uiState.value = CountriesCardUiState.Error(
56-
resourceProvider.getString(R.string.stats_todays_stats_no_site_selected)
56+
resourceProvider.getString(
57+
R.string.stats_todays_stats_no_site_selected
58+
)
5759
)
5860
return
5961
}
6062

6163
val accessToken = accountStore.accessToken
6264
if (accessToken.isNullOrEmpty()) {
65+
loadingPeriod = null
6366
_uiState.value = CountriesCardUiState.Error(
64-
resourceProvider.getString(R.string.stats_todays_stats_failed_to_load)
67+
resourceProvider.getString(
68+
R.string.stats_todays_stats_failed_to_load
69+
)
6570
)
6671
return
6772
}
@@ -70,7 +75,11 @@ class CountriesViewModel @Inject constructor(
7075
_uiState.value = CountriesCardUiState.Loading
7176

7277
viewModelScope.launch {
73-
fetchCountryViews(site)
78+
try {
79+
fetchCountryViews(site)
80+
} finally {
81+
loadingPeriod = null
82+
}
7483
}
7584
}
7685

@@ -92,10 +101,10 @@ class CountriesViewModel @Inject constructor(
92101
}
93102

94103
fun onPeriodChanged(period: StatsPeriod) {
95-
if (currentPeriod != period) {
96-
currentPeriod = period
97-
loadData()
98-
}
104+
if (loadedPeriod == period || loadingPeriod == period) return
105+
loadingPeriod = period
106+
currentPeriod = period
107+
loadData()
99108
}
100109

101110
fun getDetailData(): CountriesDetailData {
@@ -116,6 +125,7 @@ class CountriesViewModel @Inject constructor(
116125

117126
when (val result = statsRepository.fetchCountryViews(siteId, currentPeriod)) {
118127
is CountryViewsResult.Success -> {
128+
loadedPeriod = currentPeriod
119129
cachedTotalViews = result.totalViews
120130
cachedTotalViewsChange = result.totalViewsChange
121131
cachedTotalViewsChangePercent = result.totalViewsChangePercent
@@ -189,7 +199,10 @@ class CountriesViewModel @Inject constructor(
189199
*/
190200
private fun buildMapData(countries: List<CountryItem>): String {
191201
return countries.joinToString(",") { country ->
192-
"['${country.countryCode}',${country.views}]"
202+
val safeCode = country.countryCode
203+
.filter { it.isLetter() }
204+
.take(MAX_COUNTRY_CODE_LENGTH)
205+
"['$safeCode',${country.views}]"
193206
}
194207
}
195208
}

0 commit comments

Comments
 (0)
⚡