-
Notifications
You must be signed in to change notification settings - Fork 37
Woo/meta data refactor 3 #3074
Woo/meta data refactor 3 #3074
Conversation
val roomDb = Room.inMemoryDatabaseBuilder(appContext, WCAndroidDatabase::class.java) | ||
.allowMainThreadQueries() | ||
.build() |
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 added this initialization because it's needed for the MetaData table.
object AddOnsMetadataKeys { | ||
const val ADDONS_METADATA_KEY = "_product_addons" | ||
} |
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 moved this object here instead of having it on WCProductModel.
|
||
/** | ||
* This holds just the subscription keys, for the rest of product's metadata please check [ProductWithMetaData] | ||
*/ | ||
@Column var metadata = "" |
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.
Important: My goal is still to get rid of this completely, but given the impact on product subscriptions and the big changes it would require, I decided to keep it for now, I'll prioritize the project work first, and if after finishing I still have time, I'll come back to this part to remove the column completely.
fun fromMetaDataValue(value: WCMetaDataValue): List<RemoteAddonDto>? { | ||
return try { | ||
Gson().run { | ||
fromJson(value.jsonValue, Array<RemoteAddonDto>::class.java) | ||
}.toList() | ||
} catch (ex: Exception) { | ||
null | ||
} | ||
} |
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.
This is a replication of the logic that we had on WCProductModel
above.
private val stripProductMetaData: StripProductMetaData | ||
) { | ||
@Suppress("LongMethod", "ComplexMethod") | ||
fun mapToModel(localSiteId: LocalOrRemoteId.LocalId, dto: ProductDto): ProductWithMetaData { |
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.
This function is almost the same as ProductApiResponse#asProductModel
that it replaced, but with some small changes that I'll mention below.
metadata = metaData.filter { it.key in WCProductModel.SubscriptionMetadataKeys.ALL_KEYS } | ||
.let { gson.toJson(it) } |
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.
Now we save to the metadata
column only the subscription keys.
bundleMaxSize = dto.bundle_max_size?.toFloatOrNull() | ||
bundleMinSize = dto.bundle_min_size?.toFloatOrNull() |
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.
these are new columns on the WCProductModel, and we set them directly now instead of using the metadata (as mentioned in the PR description).
Also removes the logic to save the sizes as metadata
a62356b
to
4638073
Compare
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.
Excellent refactor @hichamboushaba 👏🏼 .
I've tested:
- Product Addons feature following steps from this PR and everything worked as expected
- Tested subscriptions and same, everything worked well
- Smoked tested the product section from sample app and everything worked well.
Everything worked well on that part
To double check
While testing, I noticed that the meta data that is not directly tied to the subscription product metadata, is missing. If I open View custom fields
from order details, I can only see is_vat_exmpt
field. However, my product contains several other custom fields that are missing and are not displayed. Is that expected at this point and I missed something? Because from what I see these changes were merged . So, I'd expect custom fields to show like in that PR.
Thanks for the review @JorgeMucientes 🙏
Currently, we don't offer any screen to show the metadata linked to a given product, the "View custom fields" button in the app lists only the metadata linked to the given order, so what you faced is expected. |
Hey @hichamboushaba
I see. I mixed up adding custom fields to products and to orders. I thought that an order that contained products with custom fields would reflect the custom fields for the product (not only for the order itself). Thanks for clarifying! Good to go |
First of all, sorry for the size of the PR, fixing the tests caused more changes than I anticipated, and it's not easy to split this work further.
This PR adds the third part of the metadata refactor for Woo parts, the main change of the PR is to handle saving the product's metadata in the separate table
MetaData
, and this happens at every product persist call.This required some other changes:
WCProductModel
table with two columns to store the bundle min/max sizes, these values were saved as virtual meta (by virtual I mean it's not real metadata coming from the API) data previously, and this was getting on our way to get rid of themetadata
column.addons
from the WCProductModel, and moves it to the addons DTO itself.I'll add additional comments below for places where I consider it necessary for understanding.
Testing