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.

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

Add swatch option: Prevent loosing data and default value if data is not populated via adminhtml #12036

Merged
merged 4 commits into from
Nov 25, 2017

Conversation

gomencal
Copy link
Contributor

@gomencal gomencal commented Nov 5, 2017

Add swatch option: Prevent loosing data and default value if data is not populated via adminhtml.

Description

Adapt \Magento\Swatches\Model\Plugin\EavAttribute::setProperOptionsArray to make possible that the plugins afterSave in the same module doesn't delete data if they don't found this data with all the options data, as sent via adminhtml form.

Fixed Issues (if relevant)

  1. Create attribute option via API for swatch attribute fails #10707: Create attribute option via API for swatch attribute fails
  2. Can't import attribute option over API if option is 'visual swatch' #10737: Can't import attribute option over API if option is 'visual swatch'
  3. Unable to add new options to swatch attribute #11032: Unable to add new options to swatch attribute
  4. Impossible to add swatch options via Service Contracts if there is no existing swatch option for attribute #9410: Impossible to add swatch options via Service Contracts if there is no existing swatch option for attribute

Manual testing scenarios

  1. Create a select attribute
  2. Make this attribute use visual or text swatch
  3. Try adding attribute options through the API via POST /V1/products/attributes/{attributeCode}/options

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)

@dmanners
Copy link
Contributor

dmanners commented Nov 5, 2017

@gomencal just wondering if this will also change/fix the issue being solved with #11628

@dmanners dmanners added the mm17es label Nov 5, 2017
@gomencal
Copy link
Contributor Author

gomencal commented Nov 5, 2017

I didn't see that issue :( , but it appears to be working in that scenario too

@vkublytskyi vkublytskyi self-assigned this Nov 5, 2017
@vkublytskyi vkublytskyi added this to the November 2017 milestone Nov 5, 2017
@vkublytskyi vkublytskyi added Release Line: 2.2 2.2.x bug report Component: Catalog Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog labels Nov 5, 2017
@vkublytskyi
Copy link

@gomencal Thank you for such precise fix. As this is a fix of API it would be good to cover it with an integration or api-functional test. Feel free to ask any help to implement them.

@vkublytskyi vkublytskyi self-requested a review November 6, 2017 12:43
@magento-engcom-team magento-engcom-team added bugfix Issue: Cannot Reproduce Cannot reproduce the issue on the latest `2.4-develop` branch labels Nov 7, 2017
);

$this->assertTrue($response);
$updatedData = $this->getAttributeOptions($testAttributeCode);

Choose a reason for hiding this comment

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

getAttributeOptions Method is not defined.

const RESOURCE_PATH = '/V1/products/attributes';

/**
* @magentoApiDataFixture Magento/Catalog/Model/Product/Attribute/_files/swatch_attribute.php

Choose a reason for hiding this comment

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

No need to introduce new fixture. Use Magento/Swatches/_files/swatch_attribute.php

@@ -0,0 +1,48 @@
<?php

Choose a reason for hiding this comment

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

This fixture is not correct.

  1. Rollback is required. Otherwise, an exception Exception occurred when running the "\/magento2\/magento2ce\/dev\/tests\/integration\/testsuite\/Magento\/Catalog\/Model\/Product\/Attribute\/_files\/swatch_attribute.php" fixture: Magento\Framework\Exception\AlreadyExistsException: Attribute with the same code already exists. is thrown:
<?php
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */

/** @var $attribute \Magento\Catalog\Model\ResourceModel\Eav\Attribute */
$attribute = \Magento\TestFramework\Helper\Bootstrap::getObjectManager()->create(
    \Magento\Catalog\Model\ResourceModel\Eav\Attribute::class
);
$attribute->loadByCode(4, 'swatch_attribute');
if ($attribute->getId()) {
    $attribute->delete();
}
  1. Attribute is not configured well and tests failed in all data sets with message
{"message":"Attribute %1 doesn't work with options","parameters":["swatch_attribute"],"trace":"#0 \/magento2\/magento2ce\/app\/code\/Magento\/Catalog\/Model\/Product\/Attribute\/OptionManagement.php(57): Magento\\Eav\\Model\\Entity\\Attribute\\OptionManagement->add('catalog_product', 'swatch_attribut...', Object(Magento\\Eav\\Model\\Entity\\Attribute\\Option))\n#1 [internal function]: Magento\\Catalog\\Model\\Product\\Attribute\\OptionManagement->add('swatch_attribut...', Object(Magento\\Eav\\Model\\Entity\\Attribute\\Option))\n#2 \/magento2\/magento2ce\/app\/code\/Magento\/Webapi\/Controller\/Rest.php(330): call_user_func_array(Array, Array)\n#3 \/magento2\/magento2ce\/app\/code\/Magento\/Webapi\/Controller\/Rest.php(239): Magento\\Webapi\\Controller\\Rest->processApiRequest()\n#4 \/magento2\/magento2ce\/lib\/internal\/Magento\/Framework\/Interception\/Interceptor.php(58): Magento\\Webapi\\Controller\\Rest->dispatch(Object(Magento\\Framework\\App\\Request\\Http))\n#5 \/magento2\/magento2ce\/lib\/internal\/Magento\/Framework\/Interception\/Interceptor.php(138): Magento\\Webapi\\Controller\\Rest\\Interceptor->___callParent('dispatch', Array)\n#6 \/magento2\/magento2ce\/lib\/internal\/Magento\/Framework\/Interception\/Interceptor.php(153): Magento\\Webapi\\Controller\\Rest\\Interceptor->Magento\\Framework\\Interception\\{closure}(Object(Magento\\Framework\\App\\Request\\Http))\n#7 \/magento2\/magento2ce\/generated\/code\/Magento\/Webapi\/Controller\/Rest\/Interceptor.php(39): Magento\\Webapi\\Controller\\Rest\\Interceptor->___callPlugins('dispatch', Array, Array)\n#8 \/magento2\/magento2ce\/lib\/internal\/Magento\/Framework\/App\/Http.php(135): Magento\\Webapi\\Controller\\Rest\\Interceptor->dispatch(Object(Magento\\Framework\\App\\Request\\Http))\n#9 \/magento2\/magento2ce\/generated\/code\/Magento\/Framework\/App\/Http\/Interceptor.php(24): Magento\\Framework\\App\\Http->launch()\n#10 \/magento2\/magento2ce\/lib\/internal\/Magento\/Framework\/App\/Bootstrap.php(256): Magento\\Framework\\App\\Http\\Interceptor->launch()\n#11 \/magento2\/magento2ce\/index.php(39): Magento\\Framework\\App\\Bootstrap->run(Object(Magento\\Framework\\App\\Http\\Interceptor))\n#12 {main}"}

Seems insted of introduction new fixture and resolve these issues fixture dev/tests/integration/testsuite/Magento/Swatches/_files/swatch_attribute.php may be reused.

@gomencal
Copy link
Contributor Author

Hi @vkublytskyi ! thanks for your feedback! If you see ok the changes I'll apply them to 2.3 and 2.1.

@magento-team magento-team merged commit 243dccb into magento:2.2-develop Nov 25, 2017
magento-team pushed a commit that referenced this pull request Nov 25, 2017
magento-team pushed a commit that referenced this pull request Nov 25, 2017
[EngCom] Public Pull Requests - 2.2-develop
 - MAGETWO-83552: save invoice ID on credit memo when using API method salesRefundInvoiceV1 #11670
 - MAGETWO-82577: [Backport 2.2] Translate order getCreatedAtFormatted() to store locale #11422
 - MAGETWO-84474: 10128: New Orders not being saved to order grid #12241
 - MAGETWO-83783: Shipping method fixtures not compatible with getShippingMethod(true) in OrderCreateTest #12227
 - MAGETWO-83290: Add swatch option: Prevent loosing data and default value if data is not populated via adminhtml #12036
 - MAGETWO-83741: 11740: Sending emails from Admin in Multi-Store Environment defaults to Primary Store #11992
 - MAGETWO-83399: Fix for remove 'product_list_toolbar' block from layout in XML #9413 #11473
dan-lgtm pushed a commit to Space48/magento2-patches that referenced this pull request May 23, 2018
dan-lgtm pushed a commit to Space48/magento2-patches that referenced this pull request May 23, 2018
dan-lgtm pushed a commit to Space48/magento2-patches that referenced this pull request May 23, 2018
dan-lgtm pushed a commit to Space48/magento2-patches that referenced this pull request May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Award: test coverage bug report bugfix Component: Catalog Component: Framework/Webapi USE ONLY for FRAMEWORK RELATED BUG! E.g If bug related to Catalog WEB API use just Catalog Issue: Cannot Reproduce Cannot reproduce the issue on the latest `2.4-develop` branch Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants