-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Fix mass product update with group min cart qty #19095
Fix mass product update with group min cart qty #19095
Conversation
Hi @pmclain. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
@@ -132,7 +132,7 @@ | |||
<div class="field"> | |||
<input type="text" class="input-text validate-number" id="inventory_min_sale_qty" | |||
name="<?= /* @escapeNotVerified */ $block->getFieldSuffix() ?>[min_sale_qty]" | |||
value="<?= /* @escapeNotVerified */ $block->getDefaultConfigValue('min_sale_qty') * 1 ?>" | |||
value="<?= /* @escapeNotVerified */ (int) $block->getDefaultConfigValue('min_sale_qty') * 1 ?>" |
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 get this. How original implementation produces a non-numeric value?
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.
the store config allows you to set this value at the customer group level, something that is not supported at the individual product level. something that is visible when editing the inventory config at the product level.
Product inventory using store config
Product inventory after un-checking use default
The error occurs because from multiplying json, from the store config, by 1.
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.
So after the fix some JSON string is casted to 0
?
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.
yes, should it be 1
instead? that's the value if no global config is set.
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.
@pmclain yes, I think so. And also please preserve old logic instead of casting:
is_numeric(...) ? getConfig() * 1 : <DEFAULT GLOBAL VALUE>
Resolves warning when using the product edit mass-action after configuring global group cart min qty Fixes magento#17592
a17730e
to
cf383a9
Compare
Hi @sivaschenko, thank you for the review. |
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.
Great @pmclain! I left a couple of comments but there is really no need to change anything.
$default = $this->stockConfiguration->getDefaultConfigValue('min_sale_qty'); | ||
if (!is_numeric($default)) { | ||
$default = $this->serializer->unserialize($default); | ||
$default = isset($default[GroupInterface::CUST_GROUP_ALL]) ? $default[GroupInterface::CUST_GROUP_ALL] : 1; |
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.
Get used to ??
;)
@@ -132,7 +132,7 @@ | |||
<div class="field"> | |||
<input type="text" class="input-text validate-number" id="inventory_min_sale_qty" | |||
name="<?= /* @escapeNotVerified */ $block->getFieldSuffix() ?>[min_sale_qty]" | |||
value="<?= /* @escapeNotVerified */ $block->getDefaultConfigValue('min_sale_qty') * 1 ?>" | |||
value="<?= /* @escapeNotVerified */ $block->getDefaultMinSaleQty() * 1 ?>" |
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 could use @noEscape
here.
Hi @orlangur, thank you for the review. |
$default = isset($default[GroupInterface::CUST_GROUP_ALL]) ? $default[GroupInterface::CUST_GROUP_ALL] : 1; | ||
} | ||
|
||
return (int) $default; |
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.
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.
@valdislav thanks b3a4058
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.
@valdislav Please let me know if any additional changes are requested.
Hi @sivaschenko, thank you for the review. |
Hi @pmclain, thank you for your contribution! |
Hi @pmclain. Thank you for your contribution. |
Hi @pmclain , unfortunately we had to revert changes introduced in this pull request as adding a public method Adding public method to API classes may have negative effect for extensions that have plugins applied to original public method calls (that are replaced by new method calls). I think that backward compatible approach in this case could be modifying the |
Description (*)
Resolves warning when using the product edit mass-action after configuring
global group cart min qty
Fixed Issues (if relevant)
Manual testing scenarios (*)
Contribution checklist (*)