[19.0][ADD] website_sale_product_multiple_qty#4165
[19.0][ADD] website_sale_product_multiple_qty#4165
Conversation
73eccbc to
79e2dc1
Compare
ivantodorovich
left a comment
There was a problem hiding this comment.
LG, thanks @yankinmax !
Would it be possible to add a test tour with a simple test case?
| # The website expects an integer value as an input | ||
| # ``website_sale::variant_mixin.js`` | ||
| # parseInt(parent.querySelector('input[name="add_qty"]').value). | ||
| "multiple_qty": int(rounded_qty), |
There was a problem hiding this comment.
nitpicking: I suggest math.ceil here, to always round-up instead of down; or round(rounded_qty, 0)
There was a problem hiding this comment.
We want to be able to round "DOWN" also.
There was a problem hiding this comment.
Go for round(rounded_qty, 0) then which would get the best outcome I guess.
There was a problem hiding this comment.
It doesn't make any sense to add float rounding here if int is expected:
When the page is refreshed/opened we have the number rounded with zero decimal.
Adding the quantity increases to an integer.
const combinationInfo = await this.waitFor(rpc('/website_sale/get_combination_info', {
'product_template_id': parseInt(parent.querySelector('.product_template_id')?.value),
'product_id': this._getProductId(parent),
'combination': combination,
'add_qty': parseInt(parent.querySelector('input[name="add_qty"]')?.value),
'uom_id': this._getUoMId(parent),
'context': this.context,
...this._getOptionalCombinationInfoParam(parent),
}));
There was a problem hiding this comment.
int(round(rounded_qty, 0)) then
int alone would result in, for example 1.999 -> 1.
Essentially takes only the integer part, ignoring all decimals.
with round, 1.999 -> 2
d479913 to
5f740b3
Compare
|
Moved to OCA/e-commerce: |
5f740b3 to
7021587
Compare
7021587 to
cdf12ac
Compare
alexey-pelykh
left a comment
There was a problem hiding this comment.
Thanks for the contribution, @yankinmax!
I see this has been moved to OCA/e-commerce (#1172 on that repo) per your comment. Should this PR be closed in favor of the e-commerce one? Reviewing here in case it's still relevant.
A few observations on the current state:
CI: Unreleased dependency
test-requirements.txt points to sale_product_multiple_qty from PR #4143, which is still open. The "Detect unreleased dependencies" check fails because of this. This is expected until #4143 is merged, but worth noting.
Code duplication in _get_sale_multiple_vals
The method _get_sale_multiple_vals is defined identically in both controllers/product_configurator.py and models/product_template.py. The model already has this logic -- the controller should call self.env['product.template']._get_sale_multiple_vals(product_or_template) (or better, move it to a mixin) instead of duplicating it. Duplicated logic will inevitably drift over time.
No tests
As @ivantodorovich already mentioned, a test tour covering at least a basic scenario (product page load with a multiple configured, +/- buttons, manual input rounding) would be valuable. For an ADD PR, some test coverage is expected per OCA guidelines.
UoM factor as the quantity step
The code passes multiple_uom.factor as the step value to the frontend. In Odoo, the UoM factor represents the conversion ratio relative to the reference UoM (e.g., for "Pack of 500 units", factor would be 0.002, not 500). If the intent is to use the packaging quantity as the step, you might want factor_inv (or relative_factor in 19.0) instead -- or verify that the sale_product_multiple_qty module configures UoMs such that factor already gives the right number. Could you confirm this works correctly end-to-end for a typical setup like "Pack of 500"?
Minor: _get_additionnal_combination_info spelling
I see "additionnal" with double 'n' -- if this is the upstream Odoo method name, then it matches correctly. Just flagging in case this is a typo in the override name.
| WebsiteSaleProductConfiguratorController | ||
| ): | ||
| def _get_sale_multiple_vals(self, product_or_template): | ||
| # Get product variant if we got a single variant template |
There was a problem hiding this comment.
This method is duplicated verbatim in models/product_template.py. Consider reusing the model method instead:
product_or_template._get_sale_multiple_vals(product_or_template)Or extract to a shared mixin to avoid drift.
There was a problem hiding this comment.
This is addressed, thx
|
|
||
| if multiple_uom := product.sale_multiple_uom_id: | ||
| return { | ||
| "is_multiple": 1, |
There was a problem hiding this comment.
Worth double-checking which UoM field gives the correct step value here. In Odoo's UoM model, factor is the ratio to the reference UoM (e.g. 0.002 for a pack of 500 units). If the intent is to get 500 as the step, you may need factor_inv / relative_factor instead. Or does the sale_product_multiple_qty module set up UoMs where factor already holds the packaging quantity?
There was a problem hiding this comment.
I've checked and it's ok to use factor if I always have the same "Units" relative UoM.
It could make sense to use relative_factor if at some point I have "Box of 10" with relative UoM as "Box of 500". So, it's a multi-level architecture, so the sales multiple is itself a reference UoM. Then factor will be 500 and relative_factor will be 10: 10 boxes of 500 boxes.
I don't cover such cases here for now.
| if (!Number.isFinite(value)) value = 0; | ||
| if (value <= 0) return 0; | ||
|
|
||
| // -1e-9 prevents rounding artifacts when value is already a multiple. |
There was a problem hiding this comment.
The - 1e-9 epsilon here (and in the other JS files) works for typical cases, but could silently eat a real fractional remainder if step is very small. A comment or named constant would make the intent clearer.
| @@ -0,0 +1 @@ | |||
| odoo-addon-sale_product_multiple_qty @ git+https://github.com/OCA/sale-workflow.git@refs/pull/4143/head#subdirectory=sale_product_multiple_qty | |||
There was a problem hiding this comment.
This references PR #4143 which is still unmerged, causing the CI Detect unreleased dependencies failure. This will resolve once sale_product_multiple_qty is merged, but it blocks merge of this PR as well.
|
Hello @alexey-pelykh |
|
hey @alexey-pelykh |
This module extends the eCommerce flow to support Sales Multiples
(packaging quantities) directly on the product page, in the cart,
and in the product configurator.
When a product (or variant) has a Sales Multiple configured,
the quantity entered by the customer on the website is automatically
rounded to a valid multiple according to the interaction type.
The rounding logic is applied dynamically when the customer: