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.

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

[Woo] Extract subscriptions outside of WCProductModel table #3106

Merged
merged 9 commits into from
Oct 16, 2024

Conversation

hichamboushaba
Copy link
Member

@hichamboushaba hichamboushaba commented Oct 8, 2024

⚠️ Please don't merge yet, this has some breaking changes.

This PR has two main changes:

  • It removes the METADATA column from the WCProductModel table.
  • It adds the ability to update the metadata separately when adding or updating a product.
Testing

Testing steps are provided in woocommerce/woocommerce-android#12766 and woocommerce/woocommerce-android#12767

Comment on lines +7 to +11
data class MetadataChanges(
val insertedMetadata: List<WCMetaData> = emptyList(),
val updatedMetadata: List<WCMetaData> = emptyList(),
val deletedMetadataIds: List<Long> = emptyList(),
) {
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 extracted this bit to a separate class, this class can now be used in other places when needed (now it's used in WCProductStore, but can be used for orders as well), while the UpdateMetadataRequest will still be used for the MetadataStore operations.

My goal of extracting this is to not have to deal with unneeded information for the other usages (I mean parentItemId and parentItemType).

Comment on lines +81 to +89
operator fun invoke(value: String?): WCMetaDataValue {
if (value == null) return StringValue(null)

return runCatching { JsonParser().parse(value) }
.getOrElse { JsonPrimitive(value) }
.let { fromJsonElement(it) }
}
operator fun invoke(value: Number): WCMetaDataValue = NumberValue(value)
operator fun invoke(value: Boolean): WCMetaDataValue = BooleanValue(value)
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 a nicer way to instantiate the values instead of the fromRawString version.

Copy link
Contributor

@JorgeMucientes JorgeMucientes left a comment

Choose a reason for hiding this comment

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

Well done LGTM!


204 -> migrateAddOn(ADDON_WOOCOMMERCE, version) {
// Drop the column METADATA from WCProductModel
// Since SQLite version varies accross devices, we can't rely on the support of DROP COLUMN,
Copy link
Contributor

Choose a reason for hiding this comment

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

Well spotted as most Android versions won't support alter table -> drop column operation. It seems this was added in SQLite 3.35.0 and it was in Android API 34 when SQLite was updated to a version > 3.35.0

Super minor np, typo in across word

@hichamboushaba hichamboushaba merged commit 7475ad2 into trunk Oct 16, 2024
13 checks passed
@hichamboushaba hichamboushaba deleted the woo/subscriptions-off-product branch October 16, 2024 09:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants