-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
Dev/alfmob-81 add to bag #5
base: main
Are you sure you want to change the base?
Conversation
import au.com.alfie.ecomm.repository.product.model.Price | ||
import au.com.alfie.ecomm.repository.product.model.PriceRange | ||
|
||
fun mapPrice( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know this was moved from place, but if possible can we improve the mapping to be less reliant on the properties of the price?
as far as I know when mapping the domain model Price
on the data layer mapper, there is a PriceInfo
as well as a MoneyInfo
that we can map and improve the information that is being passed along. specially the PriceInfo
that contains a ProductPriceDisplayType
which can then be mapped also to a type that can then be used here and have specific mappings per type, which will be less error prone once we have more price types and conditions
in theory we would only need the domain price to look something like this:
data class Price(
val displayType: PriceDisplayType,
val amount: Money,
val was: Money?
)
feature/src/main/java/au/com/alfie/ecomm/feature/mappers/MapImage.kt
Outdated
Show resolved
Hide resolved
feature/bag/src/test/java/au/com/alfie/ecomm/feature/bag/BagViewModelTest.kt
Outdated
Show resolved
Hide resolved
feature/bag/src/test/java/au/com/alfie/ecomm/feature/bag/BagViewModelTest.kt
Outdated
Show resolved
Hide resolved
@@ -382,7 +382,7 @@ private fun ProductDetailsBagSection( | |||
text = text, | |||
isEnabled = state.details.isSelectionSoldOut.not(), | |||
onClick = { | |||
val event = ProductDetailsEvent.OnAddToBagClick | |||
val event = ProductDetailsEvent.OnAddToBagClick(state.details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might be able to not pass this state information back to the view model, since it should already be accessible there.
I'm aware there might be a few different cases where the product has only one variant or only a color or one size, but all these cases we can either infer the selected variant (by lack of options) or the user needs to select them beforehand (updating the state in the view model), so all the information should be available before we attempt to add an item to the bag
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, i don't understand the suggestion, are you suggestion to pass only the id as a parameter on the click event?
feature/bag/src/main/java/au/com/alfie/ecomm/feature/bag/BagScreen.kt
Outdated
Show resolved
Hide resolved
feature/bag/src/main/java/au/com/alfie/ecomm/feature/bag/BagUiState.kt
Outdated
Show resolved
Hide resolved
feature/bag/src/main/java/au/com/alfie/ecomm/feature/bag/BagUiState.kt
Outdated
Show resolved
Hide resolved
feature/bag/src/main/java/au/com/alfie/ecomm/feature/bag/models/BagProductUi.kt
Show resolved
Hide resolved
feature/bag/src/main/java/au/com/alfie/ecomm/feature/bag/BagViewModel.kt
Outdated
Show resolved
Hide resolved
feature/bag/src/main/java/au/com/alfie/ecomm/feature/bag/BagUiState.kt
Outdated
Show resolved
Hide resolved
// TODO: complete flow | ||
private fun onAddToBag(item: ProductDetailsUI) { | ||
viewModelScope.launch { | ||
addToBagUseCase(item.id) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the product structure we have in place it by having a Product
ID that represents all the Variant
s of said product. The Variant
then represents a Color
and a Size
making it a unique item. was expecting the AddToBagUseCase
to receive a Variant
ID and not a Product
ID, which I believe is what is being passed as an argument or did I miss something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know what are the diferences between a variant and a product, also variant don't have id, it has sku, but i'm confused, maybe a call to explain better these diferences will be necessary!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually the hierarchy is (from more generic to unique):
- Product: has an identifier (usually ID) and has generic information about a certain item. we can't add this to a back for instance, maybe to a Wishlist sometimes
- Colour: has an identifier (also usually an ID) and there can be a multitude of colours in a Product or it can be a single colour for instance
- Size: has an identifier (can be ID or SKU or something like that) and once you have the Size it is a unique item that can then be added to the bag for instance
so what I was trying to say is that in order for us to add something to the bag we need a unique identifier which represents a specific product with a specific colour on a certain size
from what I see in the API schemas all of this can be represented by a Variant
which will have all the combinations possible for sizes and colours alike and also contain the identifier we need to add to the bag ( we can't use the product, colour or size to perform the query, but we can use it to get the right Variant
.
in summary, don't think we can use the addToBagUseCase(item.id)
which seems to use the Product ID
and something unique like a Variant ID/SKU
let me know if it clear enough or is something else I can help with
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rsousamindera
Yes makes complete sense. So, what i ll be doing is that, when user adds something to bag, then i will be saving 2 things
- Product Id
- Variant SKU
Using this information, whenever user opens the bag, we will be able to show the correct product with the correct variant.
Is there something that i am missing or this makes sense to you as well?
domain/src/test/java/au/com/alfie/ecomm/domain/bag/AddToBagUseCaseTest.kt
Outdated
Show resolved
Hide resolved
feature/bag/src/main/java/au/com/alfie/ecomm/feature/bag/BagUiFactory.kt
Outdated
Show resolved
Hide resolved
feature/bag/src/main/java/au/com/alfie/ecomm/feature/bag/BagUiFactory.kt
Outdated
Show resolved
Hide resolved
) : UseCaseInteractor { | ||
|
||
suspend operator fun invoke(productId: String) = | ||
run(productRepository.getProduct(productId = productId)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you explain a little more the idea behind getting the product from the repository and then add it in case the response is successful? couldn't we just add the item right away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand your question, are you asking why im getting the product from the repository?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry maybe I wasn't clear enough: was asking to clarify the reason why we are getting a product (productRepository.getProduct(productId = productId)
) and then add it (bagRepository.addToBag(it)
)
I understand this is a temporary approach so we can display items in the Bag
, so maybe in the future we only need to provide the arguments for a query and only change the Use Case (which is pretty good). just wanted to be sure this was the case
maybe it would help having a small comment explaining the reasoning since it might not be clear right away without some context
domain/repository/src/main/java/au/com/alfie/ecomm/repository/bag/BagRepository.kt
Outdated
Show resolved
Hide resolved
# Conflicts: # feature/plp/src/main/java/au/com/alfie/ecomm/feature/plp/factory/ProductListEntryUIFactory.kt
Add product to bag using the button add to bag on product detail
This is implemented using only an im memory cache, basically it is matching what IOS have.
Some TODO were added to change it latter to use DB or api to proper save the items.
Also the list for the bag scree was implemented, again it was implemented to match IOS and have feature parity.
![image](https://private-user-images.githubusercontent.com/7034241/407424258-0760d770-d0f2-4edb-8e20-a20b432a8785.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3Mzk0ODQ4MjMsIm5iZiI6MTczOTQ4NDUyMywicGF0aCI6Ii83MDM0MjQxLzQwNzQyNDI1OC0wNzYwZDc3MC1kMGYyLTRlZGItOGUyMC1hMjBiNDMyYTg3ODUucG5nP1gtQW16LUFsZ29yaXRobT1BV1M0LUhNQUMtU0hBMjU2JlgtQW16LUNyZWRlbnRpYWw9QUtJQVZDT0RZTFNBNTNQUUs0WkElMkYyMDI1MDIxMyUyRnVzLWVhc3QtMSUyRnMzJTJGYXdzNF9yZXF1ZXN0JlgtQW16LURhdGU9MjAyNTAyMTNUMjIwODQzWiZYLUFtei1FeHBpcmVzPTMwMCZYLUFtei1TaWduYXR1cmU9N2M5MWI4MzM2MzYxZjMwODk5ZGRmMjg4YWQ2NzU3NmJlMDg3N2YzNGNlNTY1ZmVhNjY0NjIyOThmYTQyZDY2YiZYLUFtei1TaWduZWRIZWFkZXJzPWhvc3QifQ.pOkrxxSgA3BCBqC-2ztXDUDIh8StakLsQhKhZo7xlUA)