Skip to content

Update catalog custom options with same SKU placed one-after-one. #17622

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

Closed
wants to merge 2 commits into from
Closed

Update catalog custom options with same SKU placed one-after-one. #17622

wants to merge 2 commits into from

Conversation

arendarenko
Copy link
Contributor

@arendarenko arendarenko commented Aug 15, 2018

When working with Catalog Custom Options I found that if you're need to have two already existing custom options values (it's important, because it's not reproducing when you're adding new option values) placed one after one with the same SKU, you can't do this, because when you saving product, Magento update only first option value properly, second option value didn't have those changes after save.
I investigated and found that it happens because all custom options values are saved thru only one object, and as saving goes via cycle, Magento everytime compares option value that we currently save in this iteration with previously saved object data, which placed in storedData property.

Fixed issues

#5067

Manual testing scenarios

  1. Go to catalog and create product with custom field that have two options (save product before going to step 2), set them different SKU's, for example: "11111" and "22222"
  2. Try to set the same SKU for those options (for example: "12345") and save product
    1

Expected result:
Changed will have the same SKU

Actual result:
We see changes in only first option, second option SKU is not updated.

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)

- Add ValueFactory class to save custom options in more proper way
@magento-engcom-team
Copy link
Contributor

Hi @alexeya-ven. 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 {$VERSION} instance - deploy vanilla Magento instance

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

- Fix model variable name
@ihor-sviziev ihor-sviziev self-assigned this Aug 15, 2018
@ihor-sviziev
Copy link
Contributor

ihor-sviziev commented Aug 15, 2018

It looks like there was trial to fix this issue in #13569, but it fixed not in optimal way.
I think it's better to combine changes from this PR + partly (code style fixes it's better to keep) revert changes from #13569.

I'll discuss it with maintainers.

@ihor-sviziev
Copy link
Contributor

@alexeya-ven As we discussed in Slack - looks like implementation from #13569 is better because it's not creating "value" inside "value" class and actually doing the same thing.
I'm closing this PR.

@arendarenko arendarenko deleted the fix-custom-options-updating branch August 17, 2018 12:06
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