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

Bundle quantity rules support #2883

Conversation

atorresveiga
Copy link
Contributor

Why

As part of the support for extensions milestone 2, we are working on support bundle products in the order creation flow.

Description

This PR brings support to the bundle quantity rules in the App. It fetches the bundle min and max quantity rules and saves them as part of the product metadata.

Description

It is better to test these changes in the Woo PR

Comment on lines 178 to 196
val hasBundleMinQuantityRule = response.bundle_min_size.isNullOrEmpty().not()
val hasBundleMaxQuantityRule = response.bundle_max_size.isNullOrEmpty().not()
val hasBundleQuantityRules = hasBundleMinQuantityRule || hasBundleMaxQuantityRule

metadata = if (
isBundledProduct
&& response.metadata != null
&& hasBundleQuantityRules
) {
response.bundle_max_size?.let { value ->
WCMetaData.addAsMetadata(response.metadata, BUNDLE_MAX_SIZE, value)
}
response.bundle_min_size?.let { value ->
WCMetaData.addAsMetadata(response.metadata, BUNDLE_MIN_SIZE, value)
}
response.metadata.toString()
} else {
response.metadata?.toString() ?: ""
}
Copy link
Contributor

Choose a reason for hiding this comment

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

np: It seems we're adding multiple checks over the bundle rules. I also see one at the top of the asProductModel call. Maybe we could benefit from a code extraction to a function in this response class to make it easier to concentrate on all the bundle behaviors.

Copy link
Contributor

@ThomazFB ThomazFB left a comment

Choose a reason for hiding this comment

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

Tested alongside the changes of #2882. Looks good! I only left a comment over code organization as the asProductModel is getting bigger and more complex.

Base automatically changed from issue/9541-float-deserializer to issue/9541-configuration-key November 10, 2023 03:33
@atorresveiga
Copy link
Contributor Author

Maybe we could benefit from a code extraction to a function in this response class to make it easier to concentrate on all the bundle behaviors.

Fixed here 7c508d2 @ThomazFB. Please let me know WDYT?

Base automatically changed from issue/9541-configuration-key to issue/9541-flexible-list-items November 14, 2023 22:36
@atorresveiga atorresveiga merged commit 8b108e7 into issue/9541-flexible-list-items Nov 14, 2023
2 checks passed
@atorresveiga atorresveiga deleted the issue/9541-bundle-quantity-rules-support branch November 14, 2023 22:58
return applyBundledProductChanges(model)
}

private fun applyBundledProductChanges(model: WCProductModel): WCProductModel {
Copy link
Member

Choose a reason for hiding this comment

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

@atorresveiga I'm working on some refactorings to allow extracting the product metadata to a separate table, and while doing so I stumbled upon this logic that you added to add the properties regarding bundle to the meta_data Json instead of exposing them as regular properties in the WCProductModel.

Can I ask what was the reason for choosing this path? This was causing the WCMetaData object to be returned with null ids, Gson allowed this as it doesn't support Kotlin's nullability, but it's still problematic.

With the upcoming PR, I'm intending to remove the meta_data column completely from the WCProductModel, and I will need to make changes to this logic too, is it OK if I make these keys as regular properties of the model instead of the current logic?

Copy link
Contributor Author

@atorresveiga atorresveiga Aug 7, 2024

Choose a reason for hiding this comment

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

Hi Hicham, thanks for the ping

Can I ask what was the reason for choosing this path?

I chose this path with the goal in mind of keeping the product model small.

is it OK if I make these keys as regular properties of the model instead of the current logic?

Yes, feel free to make those regular properties of the product model 👍

Copy link
Member

Choose a reason for hiding this comment

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

Great, thanks for the clarifications @atorresveiga, I'll make those changes.

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.

3 participants