Magento2 attribute option does not validate for existing records before insert 16852

[novikor]

Description (*)

See #16852 for the problem details.

New logic implemented:

1. Check if option exists.
2. If option does not exist - create new option.
3. If option exists - check the sort order of the option.
4. If option exists in DB and option in the request has other sort order than option in DB - update the sort order.
5. If option in DB exists and has the same sort order in DB - do nothing.

Fixed Issues (if relevant)

1. Magento2 Attribute option does not validate for existing records before insert #16852: Magento2 attribute option does not validate for existing records before insert

Manual testing scenarios (*)

1. Create a product eav attribute dynamically using class (Magento\Catalog\Model\ResourceModel\Eav\Attribute) and set the entity type Id to the type id of catalog_product (See sample code below) (eg: attribute code = colors)
2. generate a list of possible options and then add those options into that product EAV attribute by using (Magento\Eav\Setup\EavSetup -> AddOption()) (optionarray = 'Black, Blue, White')
3. Generate some more options (pick some of those which are already existed) and add then add them into the product attribute by following the same step describe in step 2. ('White, Green, Purple')

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]

Hi @novikor. 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

[novikor]

@swnsma , please review.

I will cover this logic by integration tests later.

[swnsma]

Hi @novikor,

Please check my notices.
Please add unit and integration tests.

[novikor]

@swnsma , please review: requested changes have been applied and integration tests provided.

[swnsma]

Generally good, but some changes is required.

[orlangur]

@novikor no objections from my side if @swnsma will finish review.

Please squash changes into a single commit so that we have perfectly clean history 😉

complete solution is not implemented here

[orlangur]

@novikor @swnsma

I have doubts regarding

If option exists in DB and option in the request has other sort order than option in DB - update the sort order.

Such behavior may lead to position update not noticed by merchant. Much cleaner approach is to show a validation error instead so that merchant may decide what to do.

[novikor]

@orlangur - I succeeded to squash only the last commits after the merge with the upstream branch and failed to process the rest.
@swnsma, we need to discuss this PR.

[orlangur]

@novikor create a new branch out of 2.3-develop and git merge --squash this old branch into it.

[sivaschenko]

@akaplya can you please approve the SVC build failure (Magento\Eav\Setup\EavSetup::__construct optional param introduced)

[swnsma]

Hi @akaplya,
Any updates?

[swnsma]

@magento run all tests

[swnsma]

@sivaschenko, it is look like re-run of tests fixed the problem :)
Can we proceed with the PR?

[magento-engcom-team]

Hi @swnsma, thank you for the review.
ENGCOM-5131 has been created to process this Pull Request
✳️ @swnsma, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[m2-assistant]

Hi @novikor, 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.