Rest api multiselect attribute bc break between 2.1.8 and 2.3.0 #21901

Valant13 · 2019-03-22T17:33:10Z

Description (*)

Correct conditions for multiselect attribute validation have been added.
Unit test for app/code/Magento/Eav/Model/TypeLocator/SimpleType.php has been created.

Fixed Issues (if relevant)

REST API multiselect attribute BC break between 2.1.8 and 2.3.0 #21364: Rest api multiselect attribute bc break between 2.1.8 and 2.3.0

Manual testing scenarios (*)

Create multiple select product attribute
Create product attribute set
Create simple product and assign it to created attribute set
Update product using PUT method of catalogProductRepositoryV1 API

Sample JSON body (options as string with ids separated by comma):

{
  "product": {
    "sku": "Test Simple Product",
    "custom_attributes": [
      {
        "attribute_code": "test_multiselect",
        "value": "4,5"
      }
    ]
  },
  "saveOptions": true
}

Sample JSON body (options as array of ids in string):

{
  "product": {
    "sku": "Test Simple Product",
    "custom_attributes": [
      {
        "attribute_code": "test_multiselect",
        "value": ["4", "5"]
      }
    ]
  },
  "saveOptions": true
}

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-cicd2 · 2019-03-22T17:33:13Z

All committers have signed the CLA.

magento-engcom-team · 2019-03-22T17:33:15Z

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

orlangur

Thanks for contribution @Valant13!

Please check my comments, simplify implementation and unit test.

Also, please write an API test covering initial case and after all builds are green, squash changes into a single commit.

orlangur · 2019-03-23T14:42:32Z

app/code/Magento/Eav/Model/TypeLocator/SimpleType.php

+
+        $arrayFrontendInputs = ['multiselect'];
+        $frontendInput = $attribute->getFrontendInput();
+        if (in_array($frontendInput, $arrayFrontendInputs)) {


Suggested change

if (in_array($frontendInput, $arrayFrontendInputs)) {

if ($attribute->getFrontendInput() === 'multiselect') {

Hi, thank you for the review.
Do you mean integration tests?

Nope, api-functional: https://github.com/magento/magento2/tree/2.3-develop/dev/tests/api-functional/testsuite/Magento/Eav/Api

orlangur · 2019-03-23T14:44:16Z

app/code/Magento/Eav/Test/Unit/Model/TypeLocator/SimpleTypeTest.php

+        /**
+         * @var Attribute|\PHPUnit_Framework_MockObject_MockObject
+         */
+        $attributeMock = $this->getMockBuilder(Attribute::class)


Please fix formatting and use createMock instead to make mock declarations more compact.

orlangur · 2019-03-23T14:44:44Z

app/code/Magento/Eav/Test/Unit/Model/TypeLocator/SimpleTypeTest.php

+        /**
+         * @var SimpleType|\PHPUnit_Framework_MockObject_MockObject
+         */
+        $simpleType = ($this->objectManger)->getObject(


Get rid of ObjectManager and setUp method.

Hi @orlangur. I use ObjectManager to create testable class instance. What should I use instead ?

I meant just get rid of property and setUp method but here it also makes sense to replace it with new operator.

orlangur · 2019-03-23T14:45:58Z

app/code/Magento/Eav/Test/Unit/Model/TypeLocator/SimpleTypeTest.php

+     * @return array
+     * @SuppressWarnings(PHPMD.ExcessiveMethodLength)
+     */
+    public function getTypeDataProvider()


Such test does not actually verify anything. It's just a method logic written in a different form.

Try splitting into proper test cases.

sidolov · 2019-04-08T15:11:16Z

Hi @Valant13 , looks like you made some commits with email different than in your GitHub profile, please, add email from commits to your profile!
Thank you!

orlangur · 2019-04-17T08:58:48Z

app/code/Magento/Eav/Test/Unit/Model/TypeLocator/SimpleTypeTest.php

+        /**
+         * @var SimpleType|\PHPUnit_Framework_MockObject_MockObject
+         */
+        $simpleType = ($this->objectManger)->getObject(


I meant just get rid of property and setUp method but here it also makes sense to replace it with new operator.

orlangur

@Valant13 perfect! 👍

LGTM, please squash changes into a single commit so that we have perfectly clean history 😉

magento-engcom-team · 2019-04-30T18:39:47Z

Hi @sidolov, thank you for the review.
ENGCOM-4920 has been created to process this Pull Request

orlangur

Changes are still not squashed. @Valant13 please do it.

Valant13 · 2019-05-03T15:03:04Z

@orlangur, sorry for delay. I will squash changes on the next week

Add conditions for multiselect attribute validation. REST API multiselect attribute BC break between 2.1.8 and 2.3.0. Create unit test for app/code/Magento/Eav/Model/TypeLocator/SimpleType.php. REST API multiselect attribute BC break between 2.1.8 and 2.3.0. Code refactoring. REST API multiselect attribute BC break between 2.1.8 and 2.3.0. Code refactoring. REST API multiselect attribute BC break between 2.1.8 and 2.3.0. Change unit test functionality for app/code/Magento/Eav/Model/TypeLocator/SimpleType.php. REST API multiselect attribute BC break between 2.1.8 and 2.3.0. Create api-functional test for the following case REST API multiselect attribute BC break between 2.1.8 and 2.3.0. Remove ObjectManager and setUp method.

Valant13 · 2019-05-10T13:53:29Z

@orlangur, I have squashed all changes to the single commit

magento-engcom-team · 2019-05-11T10:42:38Z

Hi @orlangur, thank you for the review.
ENGCOM-4920 has been created to process this Pull Request

soleksii · 2019-05-13T08:23:25Z

✔️ QA Passed

Before:

After:

Nazar65 · 2019-05-14T08:04:26Z

Hi @Valant13 thank you for collaboration, yours changes doesn't work for SOAP client, and test testUpdateMultiselectAttributesWithArrayValue() failed with following result

SoapFault : Notice: Array to string conversion in /home/nazar/sites/magento/publicPr/22/app/code/Magento/Eav/Model/Entity/Attribute/Backend/ArrayBackend.php on line 47
 /home/nazar/sites/magento/publicPr/22/vendor/zendframework/zend-soap/src/Client.php:1166
 /home/nazar/sites/magento/publicPr/22/dev/tests/api-functional/framework/Magento/TestFramework/TestCase/Webapi/Adapter/Soap.php:55
 /home/nazar/sites/magento/publicPr/22/dev/tests/api-functional/framework/Magento/TestFramework/TestCase/WebapiAbstract.php:176
 /home/nazar/sites/magento/publicPr/22/dev/tests/api-functional/testsuite/Magento/Catalog/Api/ProductRepositoryInterfaceTest.php:1236
 /home/nazar/sites/magento/publicPr/22/dev/tests/api-functional/testsuite/Magento/Catalog/Api/ProductRepositoryInterfaceTest.php:1620

VladimirZaets · 2019-05-28T11:13:01Z

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

m2-assistant · 2019-05-28T11:13:06Z

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

magento-engcom-team added Component: Eav Release Line: 2.3 Partner: ISM eCompany Pull Request is created by partner ISM eCompany partners-contribution Pull Request is created by Magento Partner labels Mar 22, 2019

kandy requested a review from paliarush March 22, 2019 18:44

orlangur self-assigned this Mar 23, 2019

orlangur requested changes Mar 23, 2019

View reviewed changes

ghost added the Progress: needs update label Mar 23, 2019

Valant13 force-pushed the REST-API-multiselect-attribute-BC-break-between-2.1.8-and-2.3.0 branch from 963f3b7 to c0370a7 Compare March 25, 2019 07:11

Valant13 requested a review from orlangur April 17, 2019 08:22

orlangur requested changes Apr 17, 2019

View reviewed changes

Valant13 requested a review from orlangur April 17, 2019 09:54

orlangur requested changes Apr 17, 2019

View reviewed changes

sidolov approved these changes Apr 30, 2019

View reviewed changes

ghost assigned sidolov Apr 30, 2019

ghost added Progress: ready for testing and removed Progress: needs update labels Apr 30, 2019

orlangur requested changes Apr 30, 2019

View reviewed changes

ghost added Progress: needs update and removed Progress: ready for testing labels Apr 30, 2019

Valant13 force-pushed the REST-API-multiselect-attribute-BC-break-between-2.1.8-and-2.3.0 branch from 984c09d to a8bcd7d Compare May 10, 2019 13:51

orlangur approved these changes May 11, 2019

View reviewed changes

ghost removed the Progress: needs update label May 11, 2019

ghost added the Progress: ready for testing label May 11, 2019

soleksii self-assigned this May 13, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels May 13, 2019

Nazar65 self-assigned this May 13, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels May 13, 2019

Nazar65 force-pushed the REST-API-multiselect-attribute-BC-break-between-2.1.8-and-2.3.0 branch from b3f0fc5 to b44e33d Compare May 13, 2019 09:43

magento#21901 static-test-fix

5162d50

Nazar65 force-pushed the REST-API-multiselect-attribute-BC-break-between-2.1.8-and-2.3.0 branch from b44e33d to 5162d50 Compare May 13, 2019 11:08

ghost added Progress: needs update and removed Progress: extended testing labels May 14, 2019

VladimirZaets closed this May 28, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rest api multiselect attribute bc break between 2.1.8 and 2.3.0 #21901

Rest api multiselect attribute bc break between 2.1.8 and 2.3.0 #21901

Valant13 commented Mar 22, 2019

magento-cicd2 commented Mar 22, 2019 •

edited

Loading

magento-engcom-team commented Mar 22, 2019

orlangur left a comment

orlangur Mar 23, 2019

Valant13 Mar 23, 2019

orlangur Mar 23, 2019

orlangur Mar 23, 2019

orlangur Mar 23, 2019

Valant13 Apr 17, 2019

orlangur Apr 17, 2019

orlangur Mar 23, 2019

sidolov commented Apr 8, 2019

orlangur Apr 17, 2019

orlangur left a comment

magento-engcom-team commented Apr 30, 2019

orlangur left a comment

Valant13 commented May 3, 2019 •

edited

Loading

Valant13 commented May 10, 2019

magento-engcom-team commented May 11, 2019

soleksii commented May 13, 2019

Nazar65 commented May 14, 2019

VladimirZaets commented May 28, 2019

m2-assistant bot commented May 28, 2019

	if (in_array($frontendInput, $arrayFrontendInputs)) {
	if ($attribute->getFrontendInput() === 'multiselect') {

Rest api multiselect attribute bc break between 2.1.8 and 2.3.0 #21901

Rest api multiselect attribute bc break between 2.1.8 and 2.3.0 #21901

Conversation

Valant13 commented Mar 22, 2019

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

magento-cicd2 commented Mar 22, 2019 • edited Loading

magento-engcom-team commented Mar 22, 2019

orlangur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidolov commented Apr 8, 2019

Choose a reason for hiding this comment

orlangur left a comment

Choose a reason for hiding this comment

magento-engcom-team commented Apr 30, 2019

orlangur left a comment

Choose a reason for hiding this comment

Valant13 commented May 3, 2019 • edited Loading

Valant13 commented May 10, 2019

magento-engcom-team commented May 11, 2019

soleksii commented May 13, 2019

Nazar65 commented May 14, 2019

VladimirZaets commented May 28, 2019

m2-assistant bot commented May 28, 2019

magento-cicd2 commented Mar 22, 2019 •

edited

Loading

Valant13 commented May 3, 2019 •

edited

Loading