Skip to content

[Dynamic Dashboard] Fix issue of not updating the orders count in some scenarios #11530

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 17, 2024
Merged

[Dynamic Dashboard] Fix issue of not updating the orders count in some scenarios #11530

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
8a141f5
A minor improvement to the logic
hichamboushaba May 16, 2024
aabdf6d
Always use order status options to infer the number of processing orders
hichamboushaba May 16, 2024
f7af83e
Update logic of ObserveSiteOrderState to observe status options changes
hichamboushaba May 16, 2024
cf38b03
Remove redundant fetch
hichamboushaba May 16, 2024
2e8a72f
Improve FakeDispatcher to allow executing a callback when dispatching
hichamboushaba May 16, 2024
4be19ab
Fix unit tests
hichamboushaba May 16, 2024
bdac596
A minor improvement to avoid redundant API call
hichamboushaba May 17, 2024
73e7b31
Remove usage of `observeOrdersCount` for detecting if store has orders
hichamboushaba May 17, 2024
53d9227
Merge branch 'trunk' into dashboard/improve-logic-of-has-orders
hichamboushaba May 17, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 22 additions & 10 deletions ...merce/src/main/kotlin/com/woocommerce/android/ui/dashboard/data/ObserveSiteOrdersState.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,22 +4,27 @@ import com.woocommerce.android.R
import com.woocommerce.android.model.DashboardWidget
import com.woocommerce.android.tools.SelectedSite
import com.woocommerce.android.util.CoroutineDispatchers
import com.woocommerce.android.util.observeEvents
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.emitAll
import kotlinx.coroutines.flow.filterNotNull
import kotlinx.coroutines.flow.flatMapLatest
import kotlinx.coroutines.flow.flowOn
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.transformLatest
import kotlinx.coroutines.withContext
import org.wordpress.android.fluxc.Dispatcher
import org.wordpress.android.fluxc.store.WCOrderStore
import org.wordpress.android.fluxc.store.WCOrderStore.HasOrdersResult
import org.wordpress.android.fluxc.store.WCOrderStore.OnOrderStatusOptionsChanged
import javax.inject.Inject

@OptIn(ExperimentalCoroutinesApi::class)
class ObserveSiteOrdersState @Inject constructor(
private val selectedSite: SelectedSite,
private val orderStore: WCOrderStore,
private val coroutineDispatchers: CoroutineDispatchers
private val coroutineDispatchers: CoroutineDispatchers,
private val dispatcher: Dispatcher
) {
operator fun invoke() = selectedSite.observe()
.filterNotNull()
Expand All @@ -30,7 +35,12 @@ class ObserveSiteOrdersState @Inject constructor(
if (!hasOrders) {
// This means either the store doesn't have orders, or no orders are cached yet
// Use other approaches to determine if the store has orders
emit(getHasOrdersFromOrderStatusOptions() ?: fetchHasOrdersFromApi())

emitAll(
observeValueFromOrdersStatusOptions().map {
it ?: fetchHasOrdersFromApi()
}
)
} else {
emit(true)
}
Expand All @@ -44,13 +54,15 @@ class ObserveSiteOrdersState @Inject constructor(
}
}

private suspend fun getHasOrdersFromOrderStatusOptions() = withContext(coroutineDispatchers.io) {
orderStore.getOrderStatusOptionsForSite(selectedSite.get())
.filter { it.statusKey != "checkout-draft" }
.takeIf { it.isNotEmpty() }
?.sumOf { it.statusCount }
?.let { it != 0 }
}
private fun observeValueFromOrdersStatusOptions() = dispatcher.observeEvents<OnOrderStatusOptionsChanged>()
.map {
orderStore.getOrderStatusOptionsForSite(selectedSite.get())
.filter { it.statusKey != "checkout-draft" }
.takeIf { it.isNotEmpty() }
?.any { it.statusCount > 0 }
}
.distinctUntilChanged()
.flowOn(coroutineDispatchers.io)

private suspend fun fetchHasOrdersFromApi(): Boolean {
return orderStore.fetchHasOrders(selectedSite.get(), null).let {
Expand Down
81 changes: 47 additions & 34 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/main/ObserveProcessingOrdersCount.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,38 +1,51 @@
package com.woocommerce.android.ui.main

import com.woocommerce.android.analytics.AnalyticsEvent
import com.woocommerce.android.analytics.AnalyticsTracker
import com.woocommerce.android.OnChangedException
import com.woocommerce.android.extensions.NotificationReceivedEvent
import com.woocommerce.android.notifications.NotificationChannelType
import com.woocommerce.android.tools.SelectedSite
import com.woocommerce.android.util.CoroutineDispatchers
import com.woocommerce.android.util.WooLog
import com.woocommerce.android.util.dispatchAndAwait
import com.woocommerce.android.util.observeEvents
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.FlowPreview
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.debounce
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.emitAll
import kotlinx.coroutines.flow.filter
import kotlinx.coroutines.flow.launchIn
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.merge
import kotlinx.coroutines.flow.onEach
import kotlinx.coroutines.flow.transformLatest
import kotlinx.coroutines.withContext
import org.greenrobot.eventbus.EventBus
import org.wordpress.android.fluxc.Dispatcher
import org.wordpress.android.fluxc.action.WCOrderAction
import org.wordpress.android.fluxc.generated.WCOrderActionBuilder
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.fluxc.network.rest.wpcom.wc.order.CoreOrderStatus
import org.wordpress.android.fluxc.store.WCOrderStore
import org.wordpress.android.fluxc.store.WCOrderStore.FetchOrderStatusOptionsPayload
import org.wordpress.android.fluxc.store.WCOrderStore.OnOrderChanged
import org.wordpress.android.fluxc.store.WCOrderStore.OnOrderStatusOptionsChanged
import org.wordpress.android.fluxc.store.WCOrderStore.OrdersCountResult.Failure
import org.wordpress.android.fluxc.store.WCOrderStore.OrdersCountResult.Success
import javax.inject.Inject

class ObserveProcessingOrdersCount @Inject constructor(
private val dispatcher: Dispatcher,
private val wcOrderStore: WCOrderStore,
private val selectedSite: SelectedSite
private val selectedSite: SelectedSite,
private val dispatchers: CoroutineDispatchers
) {
@OptIn(ExperimentalCoroutinesApi::class)
companion object {
// A debounce duration to avoid fetching the value multiple times when there are multiple simultaneous events
private const val DEBOUNCE_DURATION_MS = 200L
}

@OptIn(ExperimentalCoroutinesApi::class, FlowPreview::class)
operator fun invoke(): Flow<Int?> = selectedSite.observe().transformLatest { site ->
if (site == null) {
emit(null)
Expand All @@ -42,18 +55,11 @@ class ObserveProcessingOrdersCount @Inject constructor(
// Start with the cached value
emit(getCachedValue(site))

// emit updated value
fetchOrdersCount(site)?.let { emit(it) }
Comment on lines -45 to -46
Copy link
Member Author

@hichamboushaba hichamboushaba May 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we fetch the order status options on app launch in SiteObserver, making another fetch here is redundant, so I removed it.

Now we will start with the cached value, then observe changes and fetch when needed.


// Observe value changes
coroutineScope {
dispatcher.observeEvents<OnOrderStatusOptionsChanged>()
.onEach {
emit(getCachedValue(site))
}
.launchIn(this)

merge(
wcOrderStore.observeOrderCountForSite(site)
.distinctUntilChanged(),
dispatcher.observeEvents<OnOrderChanged>()
.filter {
@Suppress("DEPRECATION")
Expand All @@ -70,35 +76,42 @@ class ObserveProcessingOrdersCount @Inject constructor(
WooLog.d(WooLog.T.ORDERS, "New order notification received, re-check unfilled orders count")
}
)
.debounce(DEBOUNCE_DURATION_MS)
.onEach {
fetchOrdersCount(site)?.let { emit(it) }
// Fetch value from API, the value will be emitted when OnOrderStatusOptionsChanged event
// is received below
fetchOrderStatusOptions(site)
}
.launchIn(this)

emitAll(
dispatcher.observeEvents<OnOrderStatusOptionsChanged>()
.map { getCachedValue(site) }
)
}
}

private fun getCachedValue(site: SiteModel): Int? =
private suspend fun getCachedValue(site: SiteModel): Int? = withContext(dispatchers.io) {
wcOrderStore.getOrderStatusForSiteAndKey(site, CoreOrderStatus.PROCESSING.value)?.statusCount
}

private suspend fun fetchOrdersCount(site: SiteModel): Int? {
return wcOrderStore.fetchOrdersCount(site, CoreOrderStatus.PROCESSING.value).let {
when (it) {
is Success -> {
AnalyticsTracker.track(
AnalyticsEvent.UNFULFILLED_ORDERS_LOADED,
mapOf(AnalyticsTracker.KEY_HAS_UNFULFILLED_ORDERS to it.count)
)
it.count
}
private suspend fun fetchOrderStatusOptions(site: SiteModel): Result<Unit> {
val event: OnOrderStatusOptionsChanged = dispatcher.dispatchAndAwait(
WCOrderActionBuilder.newFetchOrderStatusOptionsAction(
FetchOrderStatusOptionsPayload(site)
)
)

is Failure -> {
WooLog.e(
WooLog.T.ORDERS,
"Error fetching a count of orders waiting to be fulfilled: ${it.error.message}"
)
null
}
return when {
event.isError -> {
WooLog.e(
WooLog.T.ORDERS,
"Error fetching order status options: ${event.error.message}"
)
Result.failure(OnChangedException(event.error))
}

else -> Result.success(Unit)
}
}
}
6 changes: 3 additions & 3 deletions WooCommerce/src/test/kotlin/com/woocommerce/android/FakeDispatcher.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import org.greenrobot.eventbus.Subscribe
import org.wordpress.android.fluxc.Dispatcher
import org.wordpress.android.fluxc.annotations.action.Action

class FakeDispatcher : Dispatcher() {
class FakeDispatcher(private val dispatchCallback: Dispatcher.(action: Action<*>) -> Unit = {}) : Dispatcher() {
private val listeners = mutableListOf<Any>()

@Synchronized
Expand All @@ -31,7 +31,7 @@ class FakeDispatcher : Dispatcher() {
}
}

override fun dispatch(action: Action<*>?) {
// NO-OP
override fun dispatch(action: Action<*>) {
dispatchCallback(action)
}
}
102 changes: 71 additions & 31 deletions ...merce/src/test/kotlin/com/woocommerce/android/ui/main/ObserveProcessingOrdersCountTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,12 @@ import com.woocommerce.android.viewmodel.BaseUnitTest
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.async
import kotlinx.coroutines.coroutineScope
import kotlinx.coroutines.flow.drop
import kotlinx.coroutines.delay
import kotlinx.coroutines.flow.emptyFlow
import kotlinx.coroutines.flow.first
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.test.advanceUntilIdle
import org.assertj.core.api.Assertions.assertThat
import org.greenrobot.eventbus.EventBus
import org.junit.Test
Expand All @@ -24,21 +27,29 @@ import org.wordpress.android.fluxc.network.rest.wpcom.wc.order.CoreOrderStatus
import org.wordpress.android.fluxc.store.WCOrderStore
import org.wordpress.android.fluxc.store.WCOrderStore.OnOrderChanged
import org.wordpress.android.fluxc.store.WCOrderStore.OnOrderStatusOptionsChanged
import org.wordpress.android.fluxc.store.WCOrderStore.OrdersCountResult

@OptIn(ExperimentalCoroutinesApi::class)
class ObserveProcessingOrdersCountTest : BaseUnitTest() {
private val dispatcher = FakeDispatcher()
private val orderStore: WCOrderStore = mock()
private val dispatcher = FakeDispatcher { action ->
if (action.type == WCOrderAction.FETCH_ORDER_STATUS_OPTIONS) {
emitChange(
OnOrderStatusOptionsChanged(0)
)
}
}
private val site = SiteModel()
private val orderStore: WCOrderStore = mock {
on { observeOrderCountForSite(site) } doReturn emptyFlow()
}
private val selectedSite: SelectedSite = mock {
on { observe() } doReturn flowOf(site)
}

private val sut = ObserveProcessingOrdersCount(
dispatcher = dispatcher,
wcOrderStore = orderStore,
selectedSite = selectedSite
selectedSite = selectedSite,
dispatchers = coroutinesTestRule.testDispatchers
)

@Test
Expand All @@ -52,9 +63,6 @@ class ObserveProcessingOrdersCountTest : BaseUnitTest() {

@Test
fun `when observation begins, then emit the cached count`() = testBlocking {
whenever(
orderStore.fetchOrdersCount(site, CoreOrderStatus.PROCESSING.value)
).thenReturn(OrdersCountResult.Success(2))
whenever(
orderStore.getOrderStatusForSiteAndKey(site, CoreOrderStatus.PROCESSING.value)
).thenReturn(
Expand All @@ -68,23 +76,8 @@ class ObserveProcessingOrdersCountTest : BaseUnitTest() {
assertThat(count).isEqualTo(1)
}

@Test
fun `when observation begins, then fetch orders count from API`() = testBlocking {
whenever(
orderStore.fetchOrdersCount(site, CoreOrderStatus.PROCESSING.value)
).thenReturn(OrdersCountResult.Success(1))

val count = sut.invoke().drop(1).first()

assertThat(count).isEqualTo(1)
}

@Test
fun `when order statuses are fetched, then re-emit new count`() = testBlocking {
whenever(
orderStore.fetchOrdersCount(site, CoreOrderStatus.PROCESSING.value)
).thenReturn(OrdersCountResult.Success(1))

whenever(
orderStore.getOrderStatusForSiteAndKey(site, CoreOrderStatus.PROCESSING.value)
).thenReturn(
Expand All @@ -105,10 +98,18 @@ class ObserveProcessingOrdersCountTest : BaseUnitTest() {
}

@Test
fun `when push notification is received, then re-fetch orders count`() = testBlocking {
whenever(orderStore.fetchOrdersCount(site, CoreOrderStatus.PROCESSING.value))
.thenReturn(OrdersCountResult.Success(1))
.thenReturn(OrdersCountResult.Success(2))
fun `when push notification is received, then re-fetch orders status options`() = testBlocking {
whenever(
orderStore.getOrderStatusForSiteAndKey(site, CoreOrderStatus.PROCESSING.value)
).thenReturn(
WCOrderStatusModel(0).apply {
statusCount = 1
}
).thenReturn(
WCOrderStatusModel(0).apply {
statusCount = 2
}
)

val count = runAndReturnLastValue {
EventBus.getDefault().post(
Expand All @@ -117,20 +118,59 @@ class ObserveProcessingOrdersCountTest : BaseUnitTest() {
channel = NotificationChannelType.NEW_ORDER
)
)
advanceUntilIdle()
}

assertThat(count).isEqualTo(2)
}

@Test
fun `when an order status is updated, then re-fetch orders count`() = testBlocking {
whenever(orderStore.fetchOrdersCount(site, CoreOrderStatus.PROCESSING.value))
.thenReturn(OrdersCountResult.Success(1))
.thenReturn(OrdersCountResult.Success(2))
fun `when an order status is updated, then re-fetch orders status options`() = testBlocking {
whenever(
orderStore.getOrderStatusForSiteAndKey(site, CoreOrderStatus.PROCESSING.value)
).thenReturn(
WCOrderStatusModel(0).apply {
statusCount = 1
}
).thenReturn(
WCOrderStatusModel(0).apply {
statusCount = 2
}
)

val count = runAndReturnLastValue {
@Suppress("DEPRECATION")
dispatcher.emitChange(OnOrderChanged(causeOfChange = WCOrderAction.UPDATE_ORDER_STATUS))
advanceUntilIdle()
}

assertThat(count).isEqualTo(2)
}

@Test
fun `when orders count change, then re-fetch orders status options`() = testBlocking {
whenever(orderStore.observeOrderCountForSite(site))
.thenReturn(
flow {
emit(1)
delay(1000)
emit(2)
}
)
whenever(
orderStore.getOrderStatusForSiteAndKey(site, CoreOrderStatus.PROCESSING.value)
).thenReturn(
WCOrderStatusModel(0).apply {
statusCount = 1
}
).thenReturn(
WCOrderStatusModel(0).apply {
statusCount = 2
}
)

val count = runAndReturnLastValue {
advanceUntilIdle()
}

assertThat(count).isEqualTo(2)
Expand Down
Loading