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
Solve problem saving empty swatches in admin #13220
Solve problem saving empty swatches in admin #13220
Conversation
@serhii-balko i commited with a different email, i created this new PR to clean: #13154 |
@@ -110,6 +110,11 @@ public function execute() | |||
$options | |||
); | |||
$valueOptions = (isset($options['value']) && is_array($options['value'])) ? $options['value'] : []; | |||
foreach ($valueOptions as $key => $valueOption) { | |||
if (isset($options['delete'][$key]) && $options['delete'][$key]) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better use if(!empty($options['delete'][$key]))
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also add a new test cases with empty 'deleted' options in https://github.com/magento/magento2ce/blob/2.3-develop/app/code/Magento/Catalog/Test/Unit/Controller/Adminhtml/Product/Attribute/ValidateTest.php#L325
Hi guys, i fixed the dataprovider it had a mistake, now is solved. $isError should be "false" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the best way to test this case: add new cases in provideEmptyOption
provider like this
'empty admin scope options and deleted' => [
[
'value' => [
"option_0" => [''],
],
'delete' => [
'option_0' => '1',
],
],
(object) [
'error' => false,
],
],
'empty admin scope options and not deleted' => [
[
'value' => [
"option_0" => [''],
],
'delete' => [
'option_0' => '0',
],
],
(object) [
'error' => true,
'message' => 'The value of Admin scope can\'t be empty.',
],
],
Hi @serhii-balko should I remove the previous test case for testUniqueValidation? |
fix codestyle Fix codacy unused variable in for, and improve condition add test Fix test add testcases empty
…into FIX-ADD-EMPTY-SWATCH
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it will be better if you remove it
But that (first)case is working well, and is testing well the code. If i remove the new code (validate.php) the test fails, then i think that is useful. |
@serhii-balko did you read my last comment :) (I understand that you could be busy) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you are right, We approved this changes
Description
Solve problem saving empty swatches in admin remving html ement instead of add hidden.
Fixed Issues (if relevant)
Manual testing scenarios
Contribution checklist