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.

Sign up for GitHub

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

Jump to bottom

Fixed issue - Minimum Qty Allowed in Shopping Cart functionality breaks product Update Attributes functionality #21643

Closed
wants to merge 2 commits into from
Closed

Fixed issue - Minimum Qty Allowed in Shopping Cart functionality breaks product Update Attributes functionality #21643

wants to merge 2 commits into from

Conversation

hiren0241
Copy link
Contributor

Description (*)

Resolve issue of Minimum Qty Allowed in Shopping Cart" functionality breaks product "Update Attributes" functionality

Fixed Issues (if relevant)

  1. "Minimum Qty Allowed in Shopping Cart" functionality breaks product "Update Attributes" functionality #21640: "Minimum Qty Allowed in Shopping Cart" functionality breaks product "Update Attributes" functionality

Manual testing scenarios (*)

  1. Check the value of cataloginventory/item_options/min_sale_qty, the default value is 1. Here, you can set another value also.
  2. Go to: Stores -> Configuration -> Catalog -> Inventory
  3. Make sure there are multiple or no rules for 'Minimum Qty Allowed in Shopping Cart' & save your configuration.
  4. You'll now see that cataloginventory/item_options/min_sale_qty gets an array as a value.
  5. Go to: Catalog -> Products
  6. Select one or more products and click on Update Attributes in the Actions dropdown menu.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @hiren0241. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@samgranger
Copy link
Contributor

samgranger commented Mar 8, 2019 •
edited
Loading

This might seem to fix this issue, but what you are doing is retrieving a NULL value (value for cataloginventory/item_options/min_sale_qtymin_sale_qty) in this instance. The proper solution I think would be to remove the input field completely and use the ui component that is also used when editing a product, please correct me if I'm wrong!

@hiren0241
Copy link
Contributor Author

@samgranger Let me work on your suggestion.

@dmytro-ch dmytro-ch self-assigned this Mar 8, 2019
@dmytro-ch dmytro-ch self-requested a review March 8, 2019 13:37
@dmytro-ch dmytro-ch closed this Mar 10, 2019
@ghost
Copy link

ghost commented Mar 10, 2019

Hi @hiren0241, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@dmytro-ch dmytro-ch reopened this Mar 10, 2019
@ghost ghost unassigned dmytro-ch Mar 10, 2019
dmytro-ch
dmytro-ch requested changes Mar 10, 2019
View reviewed changes
Copy link
Contributor

@dmytro-ch dmytro-ch left a comment

Choose a reason for hiding this comment

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

Hi @hiren0241,
thank you for your contribution!

Unfortunately, we can't accept these changes due to the issues mentioned in code review comments.
Please take into account the recent updates in relevant issue #17592.
There is an already available solution that was reverted due to the backward compatibility issues.

Although, I agree with the recommendation from @samgranger. The most preferable approach would be replacing the existing implementation with the corresponding UI component.

The proper solution I think would be to remove the input field completely and use the ui component that is also used when editing a product, please correct me if I'm wrong!

Thank you!

app/code/Magento/Catalog/Block/Adminhtml/Product/Edit/Tab/Inventory.php
@@ -168,7 +176,7 @@ public function getConfigFieldValue($field)
*/
public function getDefaultConfigValue($field)
{
return $this->stockConfiguration->getDefaultConfigValue($field);
return $this->json->unserialize($this->stockConfiguration->getDefaultConfigValue($field));
Copy link
Contributor

@dmytro-ch dmytro-ch Mar 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

The getDefaultConfigValue method is not expected to be used for serialized values only.
I guess we need to make sure that we unserialize only serialized values etc.

app/code/Magento/Catalog/Block/Adminhtml/Product/Edit/Tab/Inventory.php
@@ -75,6 +82,7 @@ public function __construct(
$this->coreRegistry = $coreRegistry;
$this->stockRegistry = $stockRegistry;
$this->stockConfiguration = $stockConfiguration;
$this->json = $json;
Copy link
Contributor

Choose a reason for hiding this comment

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

The new constructor parameters should be optional in order to avoid backward compatibility issues.
Please check the Contribution Guide (section Adding a constructor parameter) for more details.

@ghost ghost assigned dmytro-ch Mar 10, 2019
@hiren0241
Copy link
Contributor Author

@dmytro-ch Hi,
Will you please guide me for this PR? Either I have to propose an entire new solution or I have to perform changes mentioned in comment by you?

@dmytro-ch
Copy link
Contributor

@hiren0241 you may check the solution proposed in #19095.
This solution must be finalized according to the requirements from architect.

@sidolov
Copy link
Contributor

sidolov commented Mar 28, 2019

@hiren0241 , I am closing this PR now due to inactivity.
Please reopen and update if you wish to continue.
Thank you for the collaboration!

@sidolov sidolov closed this Mar 28, 2019
@ghost
Copy link

ghost commented Mar 28, 2019

Hi @hiren0241, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@rk-mxp rk-mxp mentioned this pull request May 17, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmytro-ch dmytro-ch dmytro-ch requested changes

Assignees

@dmytro-ch dmytro-ch

Labels
Component: CatalogInventory Partner: Krish TechnoLabs partners-contribution Pull Request is created by Magento Partner Progress: needs update Release Line: 2.3
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@hiren0241 @magento-engcom-team @samgranger @dmytro-ch @sidolov