Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
c38a541
Add logs to error case when failing to check Bookings tab visibility
JorgeMucientes Sep 18, 2025
c89ba2d
Add new extension function to check site is CIAB
JorgeMucientes Sep 18, 2025
84ff7b1
Check if current site is CIAB before displaying bookings tab
JorgeMucientes Sep 18, 2025
f99c836
Refactor extension function to apply only to SiteModel
JorgeMucientes Sep 19, 2025
838ea0f
Link Bookings tab visibility to Bookable published bookable products
JorgeMucientes Sep 19, 2025
aed415a
Add logs when refreshing bookable products fails
JorgeMucientes Sep 19, 2025
ccf36cd
Rename class
JorgeMucientes Sep 19, 2025
cab6f52
Add tests
JorgeMucientes Sep 19, 2025
f3b0973
Add unit tests
JorgeMucientes Sep 19, 2025
802ffd6
Ensure site is selected before observing bookings count
JorgeMucientes Sep 19, 2025
a120eed
Fix unit tests after refactor
JorgeMucientes Sep 19, 2025
eb13d9e
Move new is isCIABSite to the correct file
JorgeMucientes Sep 22, 2025
6c84c24
Use filterNotNull function
JorgeMucientes Sep 22, 2025
5610a84
Remove unneeded nullable expression and simplified boolean condition
JorgeMucientes Sep 22, 2025
2ad102a
Avoid fetching bookings over the network if site is not CIAB
JorgeMucientes Sep 22, 2025
41a3fc6
Make onStart logic async to avoid delaying the emission of first event
JorgeMucientes Sep 23, 2025
a3e6f53
Reactive implementation enables removing lifecycle observer
JorgeMucientes Sep 23, 2025
9e03aae
Fix unit tests
JorgeMucientes Sep 23, 2025
f05fba9
Remove nesting flow collection to avoid potential issues
JorgeMucientes Sep 23, 2025
7a5b2c0
Fix detekt issue
JorgeMucientes Sep 23, 2025
e1227bc
Moves logic to observe site changes to ObserveBookingsTabVisibility
JorgeMucientes Sep 24, 2025
81a856c
Fix unit tests
JorgeMucientes Sep 24, 2025
34a3bb1
Fix detekt indentation issue
JorgeMucientes Sep 24, 2025
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
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package com.woocommerce.android.ciab

import androidx.annotation.VisibleForTesting
import com.woocommerce.android.extensions.isCIABSite
import com.woocommerce.android.tools.SelectedSite
import javax.inject.Inject

Expand All @@ -18,13 +18,10 @@ class CIABSiteGateKeeper @Inject constructor(private val selectedSite: SelectedS
return !isFeatureSupported(feature)
}

private fun isCurrentSiteCIAB(): Boolean {
val site = selectedSite.getOrNull() ?: return false
return site.isGardenSite && site.gardenName == CIAB_GARDEN_NAME
}
private fun isCurrentSiteCIAB(): Boolean =
selectedSite.getOrNull()?.isCIABSite() ?: false

companion object Companion {
@VisibleForTesting
const val CIAB_GARDEN_NAME = "commerce"
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.woocommerce.android.extensions

import android.text.TextUtils
import com.woocommerce.android.ciab.CIABSiteGateKeeper.Companion.CIAB_GARDEN_NAME
import com.woocommerce.android.ui.plans.domain.FREE_TRIAL_PLAN_ID
import com.woocommerce.android.util.WooLog
import org.wordpress.android.fluxc.model.SiteModel
Expand Down Expand Up @@ -78,3 +79,5 @@ val SiteModel?.isSitePublic: Boolean

val SiteModel.isEligibleForAI: Boolean
get() = isWPComAtomic || planActiveFeatures.orEmpty().contains("ai-assistant")

fun SiteModel.isCIABSite() = isGardenSite && gardenName == CIAB_GARDEN_NAME
Original file line number Diff line number Diff line change
@@ -1,7 +1,5 @@
package com.woocommerce.android.ui.bookings.tab

import androidx.lifecycle.DefaultLifecycleObserver
import androidx.lifecycle.LifecycleOwner
import androidx.lifecycle.lifecycleScope
import com.woocommerce.android.R
import com.woocommerce.android.databinding.ActivityMainBinding
Expand All @@ -10,8 +8,8 @@ import kotlinx.coroutines.launch
import javax.inject.Inject

class BookingsTabController @Inject constructor(
private val showBookingsTab: ShowBookingsTab
) : DefaultLifecycleObserver {
private val observeBookingsTabVisibility: ObserveBookingsTabVisibility
) {
private lateinit var activity: MainActivity
private lateinit var binding: ActivityMainBinding

Expand All @@ -21,25 +19,14 @@ class BookingsTabController @Inject constructor(
) {
this.activity = activity
this.binding = binding
activity.lifecycle.addObserver(this)
}

override fun onResume(owner: LifecycleOwner) {
checkBookingsTabVisibility()
}

override fun onDestroy(owner: LifecycleOwner) {
owner.lifecycle.removeObserver(this)
}

private fun checkBookingsTabVisibility() {
Copy link
Member

Choose a reason for hiding this comment

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

Not directly related to this PR, but just sharing an observation here, I think that given we use activity.lifecycleScope.launch in this function, it's safe to call it directly from init without making this class a LifecycleObserver, lifecycleScope already takes care of this, and won't start the Coroutine until the Activity is started.

activity.lifecycleScope.launch {
showBookingsTab()
.onSuccess {
binding.bottomNav.menu.findItem(R.id.bookings)?.isVisible = it
}
.onFailure {
// TODO log error or track errors?
observeBookingsTabVisibility()
.collect { isVisible ->
binding.bottomNav.menu.findItem(R.id.bookings)?.isVisible = isVisible
}
}
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
package com.woocommerce.android.ui.bookings.tab

import androidx.annotation.VisibleForTesting
import com.woocommerce.android.di.AppCoroutineScope
import com.woocommerce.android.extensions.isCIABSite
import com.woocommerce.android.tools.SelectedSite
import com.woocommerce.android.ui.products.ProductStatus
import com.woocommerce.android.ui.products.list.ProductListRepository
import com.woocommerce.android.util.FeatureFlag
import com.woocommerce.android.util.WooLog
import kotlinx.coroutines.CoroutineScope
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.distinctUntilChanged
import kotlinx.coroutines.flow.emitAll
import kotlinx.coroutines.flow.filterNotNull
import kotlinx.coroutines.flow.flatMapLatest
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.map
import kotlinx.coroutines.flow.onStart
import kotlinx.coroutines.launch
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.fluxc.store.WCProductStore.ProductFilterOption
import javax.inject.Inject

class ObserveBookingsTabVisibility @Inject constructor(
private val productListRepository: ProductListRepository,
private val selectedSite: SelectedSite,
@AppCoroutineScope private val appCoroutineScope: CoroutineScope,
) {

@OptIn(ExperimentalCoroutinesApi::class)
operator fun invoke(): Flow<Boolean> =
selectedSite.observe()
.filterNotNull()
.flatMapLatest { siteModel ->
observeBookingTabVisibility(siteModel)
}

private fun observeBookingTabVisibility(siteModel: SiteModel): Flow<Boolean> = flow {
val isCIABSite = FeatureFlag.BOOKINGS_MVP.isEnabled() && siteModel.isCIABSite()
if (!isCIABSite) {
emit(false)
} else {
emitAll(
productListRepository.observeProductsCount(
filterOptions = mapOf(
ProductFilterOption.STATUS to ProductStatus.PUBLISH.value,
ProductFilterOption.TYPE to BOOKING_PRODUCT_TYPE
),
excludeSampleProducts = true
)
.map { count -> count > 0 }
.onStart {
appCoroutineScope.launch {
productListRepository.fetchProductList(
productFilterOptions = mapOf(ProductFilterOption.TYPE to BOOKING_PRODUCT_TYPE)
).onFailure {
WooLog.w(WooLog.T.BOOKINGS, "Failed to fetch bookable products")
}
}
}.distinctUntilChanged()
)
}
}

companion object {
@VisibleForTesting
const val BOOKING_PRODUCT_TYPE = "booking"
}
}

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ object WooLog {
GOOGLE_ADS,
POS,
CUSTOM_FIELDS,
SHIPPING_LABELS
SHIPPING_LABELS,
BOOKINGS
}

// Breaking convention to be consistent with org.wordpress.android.util.AppLog
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,174 @@
package com.woocommerce.android.ui.bookings.tab

import app.cash.turbine.test
import com.woocommerce.android.tools.SelectedSite
import com.woocommerce.android.ui.bookings.tab.ObserveBookingsTabVisibility.Companion.BOOKING_PRODUCT_TYPE
import com.woocommerce.android.ui.products.ProductStatus
import com.woocommerce.android.ui.products.list.ProductListRepository
import com.woocommerce.android.viewmodel.BaseUnitTest
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.MutableStateFlow
import kotlinx.coroutines.test.TestScope
import org.junit.Assert.assertFalse
import org.junit.Assert.assertTrue
import org.junit.Test
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.eq
import org.mockito.kotlin.mock
import org.mockito.kotlin.verify
import org.mockito.kotlin.whenever
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.fluxc.store.WCProductStore.ProductFilterOption

@OptIn(ExperimentalCoroutinesApi::class)
class ObserveBookingsTabVisibilityTest : BaseUnitTest() {
private val testScope = TestScope(coroutinesTestRule.testDispatcher)
private val selectedSite: SelectedSite = mock()
private val selectedSiteFlow = MutableStateFlow<SiteModel?>(null)
private val bookableProdsFilterOptions = mapOf(
ProductFilterOption.STATUS to ProductStatus.PUBLISH.value,
ProductFilterOption.TYPE to BOOKING_PRODUCT_TYPE
)
private val bookableProdsCountFlow = MutableStateFlow(0L)

private val productListRepository: ProductListRepository = mock {
on {
observeProductsCount(
filterOptions = bookableProdsFilterOptions,
excludeSampleProducts = true
)
}.thenReturn(bookableProdsCountFlow)
}

private lateinit var sut: ObserveBookingsTabVisibility

suspend fun setup(prepareMocks: suspend () -> Unit = {}) {
prepareMocks()
whenever(selectedSite.observe()).thenReturn(selectedSiteFlow)
sut = ObserveBookingsTabVisibility(
productListRepository = productListRepository,
selectedSite = selectedSite,
appCoroutineScope = testScope,
)
}

@Test
fun `given site is CIAB, when invoke is called, then bookable products are fetched`() = testBlocking {
bookableProdsCountFlow.value = 2
selectedSiteFlow.value = ciabSite()
setup()

sut().test {
verify(productListRepository).fetchProductList(
loadMore = any(),
productFilterOptions = eq(mapOf(ProductFilterOption.TYPE to BOOKING_PRODUCT_TYPE)),
excludedProductIds = any(),
sortType = anyOrNull()
)
cancelAndIgnoreRemainingEvents()
}
}

@Test
fun `given CIAB site and bookings products published, when invoke, then emits true`() = testBlocking {
bookableProdsCountFlow.value = 1
selectedSiteFlow.value = ciabSite()
setup()

sut().test {
val showBookingTabValue = awaitItem()
assertTrue(showBookingTabValue)
cancelAndIgnoreRemainingEvents()
}
}

@Test
fun `given CIAB site with zero booking products, when invoke, then emits false`() = testBlocking {
bookableProdsCountFlow.value = 0
selectedSiteFlow.value = ciabSite()
setup()

sut().test {
val showBookingTabValue = awaitItem()
assertFalse(showBookingTabValue)
cancelAndIgnoreRemainingEvents()
}
}

@Test
fun `given non-CIAB site, when invoke, then emits false`() = testBlocking {
bookableProdsCountFlow.value = 10
selectedSiteFlow.value = nonCiabSite()
setup()

sut().test {
val showBookingTabValue = awaitItem()
assertFalse(showBookingTabValue)
cancelAndIgnoreRemainingEvents()
}
}

@Test
fun `given non-Commerce Garden CIAB site, when invoke, then emits false`() = testBlocking {
bookableProdsCountFlow.value = 10
selectedSiteFlow.value = nonCommerceGardenSite()
setup()

sut().test {
val showBookingTabValue = awaitItem()
assertFalse(showBookingTabValue)
cancelAndIgnoreRemainingEvents()
}
}

@Test
fun `given same inputs produce same result, when values update, then only emits once`() = testBlocking {
bookableProdsCountFlow.value = 2
selectedSiteFlow.value = ciabSite()
setup()

sut().test {
val firstEmission = awaitItem()
assertTrue(firstEmission)
// When inputs change but computed value remains true
bookableProdsCountFlow.value = 3 // still true
// Then no new emissions due to distinctUntilChanged
expectNoEvents()

// Now change to false
bookableProdsCountFlow.value = 0
val secondEmission = awaitItem()
assertFalse(secondEmission)
cancelAndIgnoreRemainingEvents()
}
}

@Test
fun `given bookable products fetch fails onStart, when invoke, then emits based on persisted values`() =
testBlocking {
bookableProdsCountFlow.value = 1
selectedSiteFlow.value = ciabSite()
setup()

sut().test {
assert(awaitItem())
cancelAndIgnoreRemainingEvents()
}
}

private fun ciabSite(): SiteModel = SiteModel().apply {
setIsGardenSite(true)
gardenName = "commerce"
}

private fun nonCiabSite(): SiteModel = SiteModel().apply {
setIsGardenSite(false)
gardenName = "commerce"
}

private fun nonCommerceGardenSite(): SiteModel = SiteModel().apply {
setIsGardenSite(true)
gardenName = "other"
}
}