Skip to content
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

[Blaze] Logic for the MyStore's View when there is no campaign #9966

Merged
merged 14 commits into from
Oct 18, 2023
Merged

[Blaze] Logic for the MyStore's View when there is no campaign #9966

Show file tree
Hide file tree
Changes from 10 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
9ee26c1
Bump fluxc version
hichamboushaba Oct 16, 2023
4da4420
Logic for using the appropriate state depending on last ca
hichamboushaba Oct 16, 2023
8330553
Refactor campaign state modeling
hichamboushaba Oct 16, 2023
691a86a
Handle showing the most recent product when no active blaze campaign
hichamboushaba Oct 16, 2023
47f7c06
Fix UI of the no-campaign state
hichamboushaba Oct 16, 2023
e10c39f
Handle navigation to Blaze campaign creation
hichamboushaba Oct 16, 2023
6bf5d8c
Handle fetching products and update the no-campaign view
hichamboushaba Oct 16, 2023
a6700a5
Launch blaze campaign creation when clicking on the product too.
hichamboushaba Oct 16, 2023
db96c8c
Update logic of blaze flow when clicking on the product
hichamboushaba Oct 16, 2023
229add8
Avoid using property as default argument
hichamboushaba Oct 16, 2023
e753e9e
UI Tests: fix products endpoint matching
hichamboushaba Oct 17, 2023
4bb0b4d
UI Tests: add a mock for blaze campaigns endpoint
hichamboushaba Oct 17, 2023
c1318f3
Bump fluxc version
hichamboushaba Oct 17, 2023
89da3a7
Merge branch 'trunk' into issue/9959-blaze-no-campaign
hichamboushaba Oct 18, 2023
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
94 changes: 65 additions & 29 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/MyStoreBlazeView.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,14 @@ import androidx.compose.foundation.border
import androidx.compose.foundation.clickable
import androidx.compose.foundation.layout.Box
import androidx.compose.foundation.layout.Column
import androidx.compose.foundation.layout.PaddingValues
import androidx.compose.foundation.layout.Row
import androidx.compose.foundation.layout.fillMaxWidth
import androidx.compose.foundation.layout.padding
import androidx.compose.foundation.layout.size
import androidx.compose.foundation.layout.wrapContentHeight
import androidx.compose.foundation.shape.RoundedCornerShape
import androidx.compose.material.ButtonDefaults
import androidx.compose.material.Card
import androidx.compose.material.Divider
import androidx.compose.material.Icon
Expand All @@ -35,16 +37,14 @@ import com.woocommerce.android.R
import com.woocommerce.android.ui.blaze.MyStoreBlazeViewModel.BlazeCampaignUi
import com.woocommerce.android.ui.blaze.MyStoreBlazeViewModel.BlazeProductUi
import com.woocommerce.android.ui.blaze.MyStoreBlazeViewModel.CampaignStatusUi
import com.woocommerce.android.ui.blaze.MyStoreBlazeViewModel.MyStoreBlazeUi
import com.woocommerce.android.ui.blaze.MyStoreBlazeViewModel.MyStoreBlazeCampaignState
import com.woocommerce.android.ui.compose.component.ListItemImage
import com.woocommerce.android.ui.compose.component.WCTag
import com.woocommerce.android.ui.compose.component.WCTextButton

@Composable
fun MyStoreBlazeView(
state: MyStoreBlazeUi,
onCreateCampaignClicked: () -> Unit,
onShowAllClicked: () -> Unit
state: MyStoreBlazeCampaignState
) {
Card(
modifier = Modifier
Expand All @@ -61,14 +61,14 @@ fun MyStoreBlazeView(
)
) {
BlazeCampaignHeader()
when {
state.blazeActiveCampaign != null -> BlazeCampaignItem(
campaign = state.blazeActiveCampaign,
when (state) {
is MyStoreBlazeCampaignState.Campaign -> BlazeCampaignItem(
campaign = state.campaign,
onCampaignClicked = {},
modifier = Modifier.padding(top = dimensionResource(id = R.dimen.major_100))
)

else -> {
is MyStoreBlazeCampaignState.NoCampaign -> {
Text(
modifier = Modifier.padding(
top = dimensionResource(id = R.dimen.major_100),
Expand All @@ -79,32 +79,49 @@ fun MyStoreBlazeView(
)
BlazeProductItem(
product = state.product,
onProductSelected = {},
onProductSelected = state.onProductClicked,
modifier = Modifier.padding(top = dimensionResource(id = R.dimen.major_100))
)
}

else -> error("Invalid state")
}
}
when {
state.blazeActiveCampaign != null -> ShowAllOrCreateCampaignFooter(
onShowAllClicked,
onCreateCampaignClicked
when (state) {
is MyStoreBlazeCampaignState.Campaign -> ShowAllOrCreateCampaignFooter(
onShowAllClicked = state.onViewAllCampaignsClicked,
onCreateCampaignClicked = state.onCreateCampaignClicked
)

is MyStoreBlazeCampaignState.NoCampaign -> CreateCampaignFooter(
onCreateCampaignClicked = state.onCreateCampaignClicked,
modifier = Modifier.padding(top = dimensionResource(id = R.dimen.major_100))
)

else -> CreateCampaignFooter(onCreateCampaignClicked)
else -> error("Invalid state")
}
}
}
}

@Composable
private fun CreateCampaignFooter(onCreateCampaignClicked: () -> Unit) {
Divider()
WCTextButton(
modifier = Modifier.padding(start = dimensionResource(id = R.dimen.major_75)),
onClick = onCreateCampaignClicked
Comment on lines -98 to -101
Copy link
Member Author

@hichamboushaba hichamboushaba Oct 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@JorgeMucientes I noticed that this function was emiting multiple UI elements, so it depends on the parent Composable for defining how they will be laid out, AFAIK this is not recommended, since it's hard to predict, and maintain (any change to the parent layout will impact it, but it's not easy to figure out why).
I can't find any explicit mention of this in the official guideline, but I think it can still be inferred from the fact that Component composables need to accept a Modifier argument, so they need to emit a single UI element which will consume it 🤔, and I also found this lint rule in Slack's repo which protects against such usage.

WDYT? do you agree with this opinion? and do you think it's worth adding an explicit mention of this to our guidelines?

Copy link
Member Author

Choose a reason for hiding this comment

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

Just to clarify further, I updated it because I needed adding a top padding, so it was a chance to refactor it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch! We actually do have a rule for that in the guidelines, only that I missed it. In fact, now that I see this, I remember adding fun ColumnScope.CreateCampaignFooter, but since I wasn't making any use of the scope inside the function I had to remove it.

Anyway I agree with your refactor and thanks for adding the missing Modifie param 👍🏼

) {
Text(stringResource(id = R.string.blaze_campaign_create_campaign_button))
private fun CreateCampaignFooter(
onCreateCampaignClicked: () -> Unit,
modifier: Modifier = Modifier,
) {
Column(modifier) {
Divider(Modifier.padding(start = dimensionResource(id = R.dimen.major_100)))
WCTextButton(
onClick = onCreateCampaignClicked,
contentPadding = PaddingValues(
start = dimensionResource(id = R.dimen.major_100),
end = dimensionResource(id = R.dimen.major_100),
top = ButtonDefaults.TextButtonContentPadding.calculateTopPadding(),
bottom = ButtonDefaults.TextButtonContentPadding.calculateBottomPadding(),
)
) {
Text(stringResource(id = R.string.blaze_campaign_create_campaign_button))
}
}
}

Expand Down Expand Up @@ -278,24 +295,43 @@ private fun CampaignStat(
@Preview(name = "mid screen", device = Devices.PIXEL_4)
@Preview(name = "large screen", device = Devices.NEXUS_10)
@Composable
fun MyStoreBlazeViewPreview() {
fun MyStoreBlazeViewCampaignPreview() {
val product = BlazeProductUi(
name = "Product name",
imgUrl = "",
)
MyStoreBlazeView(
state = MyStoreBlazeUi(
isVisible = true,
product = product,
blazeActiveCampaign = BlazeCampaignUi(
state = MyStoreBlazeCampaignState.Campaign(
campaign = BlazeCampaignUi(
product = product,
status = CampaignStatusUi.Active,
impressions = 100,
clicks = 10,
budget = 1000
),
),
onCreateCampaignClicked = {},
onShowAllClicked = {}
onCampaignClicked = {},
onViewAllCampaignsClicked = {},
onCreateCampaignClicked = {}
)
)
}

@ExperimentalFoundationApi
@Preview(name = "dark", uiMode = Configuration.UI_MODE_NIGHT_YES)
@Preview(name = "light", uiMode = Configuration.UI_MODE_NIGHT_NO)
@Preview(name = "small screen", device = Devices.PIXEL)
@Preview(name = "mid screen", device = Devices.PIXEL_4)
@Preview(name = "large screen", device = Devices.NEXUS_10)
@Composable
fun MyStoreBlazeViewNoCampaignPreview() {
MyStoreBlazeView(
state = MyStoreBlazeCampaignState.NoCampaign(
product = BlazeProductUi(
name = "Product name",
imgUrl = "",
),
onProductClicked = {},
onCreateCampaignClicked = {}
)
)
}
126 changes: 102 additions & 24 deletions WooCommerce/src/main/kotlin/com/woocommerce/android/ui/blaze/MyStoreBlazeViewModel.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,33 +1,84 @@
package com.woocommerce.android.ui.blaze

import android.os.Parcelable
import androidx.annotation.ColorRes
import androidx.annotation.StringRes
import androidx.lifecycle.SavedStateHandle
import androidx.lifecycle.asLiveData
import androidx.lifecycle.viewModelScope
import com.woocommerce.android.R
import com.woocommerce.android.model.Product
import com.woocommerce.android.ui.blaze.IsBlazeEnabled.BlazeFlowSource
import com.woocommerce.android.ui.products.ProductListRepository
import com.woocommerce.android.ui.products.ProductStatus
import com.woocommerce.android.util.FeatureFlag
import com.woocommerce.android.viewmodel.MultiLiveEvent
import com.woocommerce.android.viewmodel.ScopedViewModel
import com.woocommerce.android.viewmodel.getStateFlow
import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.parcelize.Parcelize
import kotlinx.coroutines.ExperimentalCoroutinesApi
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.emitAll
import kotlinx.coroutines.flow.flatMapLatest
import kotlinx.coroutines.flow.flow
import kotlinx.coroutines.flow.flowOf
import kotlinx.coroutines.flow.map
import org.wordpress.android.fluxc.model.blaze.BlazeCampaignModel
import org.wordpress.android.fluxc.store.WCProductStore.ProductFilterOption
import org.wordpress.android.fluxc.store.WCProductStore.ProductSorting
import javax.inject.Inject

@HiltViewModel
class MyStoreBlazeViewModel @Inject constructor(
savedStateHandle: SavedStateHandle,
observeMostRecentBlazeCampaign: ObserveMostRecentBlazeCampaign,
private val productListRepository: ProductListRepository,
private val isBlazeEnabled: IsBlazeEnabled
) : ScopedViewModel(savedStateHandle) {
private val _blazeCampaignState =
savedStateHandle.getStateFlow(
scope = viewModelScope,
initialValue = MyStoreBlazeUi(
isVisible = FeatureFlag.BLAZE_ITERATION_2.isEnabled(),
@OptIn(ExperimentalCoroutinesApi::class)
val blazeCampaignState = flow {
if (!FeatureFlag.BLAZE_ITERATION_2.isEnabled()) emit(MyStoreBlazeCampaignState.Hidden)
else {
emitAll(
observeMostRecentBlazeCampaign().flatMapLatest {
when (it) {
null -> prepareUiForNoCampaign()
else -> prepareUiForCampaign(it)
}
}
)
}
}.asLiveData()

private fun prepareUiForNoCampaign(): Flow<MyStoreBlazeCampaignState> {
fun launchCampaignCreation(productId: Long?) {
val url = if (productId != null) {
isBlazeEnabled.buildUrlForProduct(productId, BlazeFlowSource.MY_STORE_BANNER)
} else {
isBlazeEnabled.buildUrlForSite(BlazeFlowSource.MY_STORE_BANNER)
}
triggerEvent(LaunchBlazeCampaignCreation(url, BlazeFlowSource.MY_STORE_BANNER))
}

return getProducts().map { products ->
val product = products.firstOrNull() ?: return@map MyStoreBlazeCampaignState.Hidden
MyStoreBlazeCampaignState.NoCampaign(
product = BlazeProductUi(
name = "Product name",
imgUrl = "https://hips.hearstapps.com/hmg-prod/images/gh-082420-ghi-best-sofas-1598293488.png",
name = product.name,
imgUrl = product.firstImageUrl.orEmpty(),
),
blazeActiveCampaign = BlazeCampaignUi(
onProductClicked = {
launchCampaignCreation(product.remoteId)
},
onCreateCampaignClicked = {
launchCampaignCreation(if (products.size == 1) product.remoteId else null)
}
)
}
}

@Suppress("UNUSED_PARAMETER")
private fun prepareUiForCampaign(campaign: BlazeCampaignModel): Flow<MyStoreBlazeCampaignState> {
return flowOf(
MyStoreBlazeCampaignState.Campaign(
campaign = BlazeCampaignUi(
product = BlazeProductUi(
name = "Product name",
imgUrl = "https://hips.hearstapps.com/hmg-prod/images/gh-082420-ghi-best-sofas-1598293488.png",
Expand All @@ -36,32 +87,57 @@ class MyStoreBlazeViewModel @Inject constructor(
impressions = 100,
clicks = 10,
budget = 1000
)
),
onCampaignClicked = { /* TODO */ },
onViewAllCampaignsClicked = { /* TODO */ },
onCreateCampaignClicked = { /* TODO */ }
)
)
val blazeCampaignState = _blazeCampaignState.asLiveData()
}

@Parcelize
data class MyStoreBlazeUi(
val isVisible: Boolean,
val product: BlazeProductUi,
val blazeActiveCampaign: BlazeCampaignUi?
) : Parcelable
private fun getProducts(): Flow<List<Product>> {
fun getCachedProducts() = productListRepository.getProductList(
productFilterOptions = mapOf(ProductFilterOption.STATUS to ProductStatus.PUBLISH.value),
sortType = ProductSorting.DATE_DESC,
).filterNot { it.isSampleProduct }
return flow {
emit(getCachedProducts())
productListRepository.fetchProductList(
Copy link
Member Author

Choose a reason for hiding this comment

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

I made the function's return as Flow to allow fetching products and updating the UI, let me know what you think about the logic.

productFilterOptions = mapOf(ProductFilterOption.STATUS to ProductStatus.PUBLISH.value),
sortType = ProductSorting.DATE_DESC,
)
emit(getCachedProducts())
}
}

sealed interface MyStoreBlazeCampaignState {
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the state to a sealed interface, this IMO simplifies the logic a bit, since there is no more duplication around the product itself, and it's also simpler for dealing when there are no active products (instead of making an additional field nullable).

Please let me know if you prefer keeping it the way it was.

object Hidden : MyStoreBlazeCampaignState
data class NoCampaign(
val product: BlazeProductUi,
val onProductClicked: () -> Unit,
val onCreateCampaignClicked: () -> Unit,
) : MyStoreBlazeCampaignState

data class Campaign(
val campaign: BlazeCampaignUi,
val onCampaignClicked: () -> Unit,
val onViewAllCampaignsClicked: () -> Unit,
val onCreateCampaignClicked: () -> Unit,
) : MyStoreBlazeCampaignState
}

@Parcelize
data class BlazeProductUi(
val name: String,
val imgUrl: String
) : Parcelable
)

@Parcelize
data class BlazeCampaignUi(
val product: BlazeProductUi,
val status: CampaignStatusUi,
val impressions: Int,
val clicks: Int,
val budget: Int
) : Parcelable
)

enum class CampaignStatusUi(
@StringRes val statusDisplayText: Int,
Expand Down Expand Up @@ -89,4 +165,6 @@ class MyStoreBlazeViewModel @Inject constructor(
backgroundColor = R.color.blaze_campaign_status_completed_background
),
}

data class LaunchBlazeCampaignCreation(val url: String, val source: BlazeFlowSource) : MultiLiveEvent.Event()
}
18 changes: 18 additions & 0 deletions ...mmerce/src/main/kotlin/com/woocommerce/android/ui/blaze/ObserveMostRecentBlazeCampaign.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package com.woocommerce.android.ui.blaze

import com.woocommerce.android.tools.SelectedSite
import kotlinx.coroutines.flow.Flow
import kotlinx.coroutines.flow.onStart
import org.wordpress.android.fluxc.model.blaze.BlazeCampaignModel
import org.wordpress.android.fluxc.store.blaze.BlazeCampaignsStore
import javax.inject.Inject

class ObserveMostRecentBlazeCampaign @Inject constructor(
hichamboushaba marked this conversation as resolved.
Show resolved Hide resolved
private val selectedSite: SelectedSite,
private val blazeCampaignsStore: BlazeCampaignsStore
) {
operator fun invoke(): Flow<BlazeCampaignModel?> =
blazeCampaignsStore.observeMostRecentBlazeCampaign(selectedSite.get()).onStart {
blazeCampaignsStore.fetchBlazeCampaigns(selectedSite.get())
}
}
Loading