Skip to content

Commit 2e5de1d

Browse files
author
Allan Paiste
committed
Code simplified + test-coverage increased + fixed things that scrutinizer reported
1 parent 58c0575 commit 2e5de1d

File tree

2 files changed

+110
-42
lines changed
  • app/code/Magento/Catalog

2 files changed

+110
-42
lines changed

‎app/code/Magento/Catalog/Model/Category/Attribute/Backend/Image.php

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313

1414
class Image extends \Magento\Eav\Model\Entity\Attribute\Backend\AbstractBackend
1515
{
16-
const ADDITIONAL_DATA_SUFFIX = '_additional_data';
16+
const ADDITIONAL_DATA_PREFIX = '_additional_data_';
1717

1818
/**
1919
* @var \Magento\MediaStorage\Model\File\UploaderFactory
@@ -57,7 +57,7 @@ class Image extends \Magento\Eav\Model\Entity\Attribute\Backend\AbstractBackend
5757
* @param \Psr\Log\LoggerInterface $logger
5858
* @param \Magento\Framework\Filesystem $filesystem
5959
* @param \Magento\MediaStorage\Model\File\UploaderFactory $fileUploaderFactory
60-
* @param \Magento\Framework\ObjectManagerInterface
60+
* @param \Magento\Framework\ObjectManagerInterface $objectManager
6161
*/
6262
public function __construct(
6363
\Psr\Log\LoggerInterface $logger,
@@ -95,11 +95,11 @@ public function beforeSave($object)
9595
$attributeName = $this->getAttribute()->getName();
9696
$value = $object->getData($attributeName);
9797

98-
if ($value === false || (is_array($value) && isset($value['delete']) && $value['delete'] === true)) {
99-
$object->setData($attributeName, '');
100-
} else if ($imageName = $this->getUploadedImageName($value)) {
101-
$object->setData($attributeName . self::ADDITIONAL_DATA_SUFFIX, $value);
98+
if ($imageName = $this->getUploadedImageName($value)) {
99+
$object->setData(self::ADDITIONAL_DATA_PREFIX . $attributeName, $value);
102100
$object->setData($attributeName, $imageName);
101+
} else if (!is_string($value)) {
102+
$object->setData($attributeName, '');
103103
}
104104

105105
return parent::beforeSave($object);
@@ -127,7 +127,7 @@ private function getImageUploader()
127127
*/
128128
public function afterSave($object)
129129
{
130-
$value = $object->getData($this->getAttribute()->getName() . self::ADDITIONAL_DATA_SUFFIX);
130+
$value = $object->getData(self::ADDITIONAL_DATA_PREFIX . $this->getAttribute()->getName());
131131

132132
if ($imageName = $this->getUploadedImageName($value)) {
133133
try {

‎app/code/Magento/Catalog/Test/Unit/Model/Category/Attribute/Backend/ImageTest.php

Lines changed: 103 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,11 @@ class ImageTest extends \PHPUnit_Framework_TestCase
2424
*/
2525
protected $imageUploader;
2626

27+
/**
28+
* @var \Psr\Log\LoggerInterface
29+
*/
30+
protected $logger;
31+
2732
protected function setUp()
2833
{
2934
$this->objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this);
@@ -42,6 +47,16 @@ protected function setUp()
4247
->method('getName')
4348
->will($this->returnValue('test_attribute'));
4449

50+
$this->logger = $this->getMockForAbstractClass(
51+
\Psr\Log\LoggerInterface::class,
52+
[],
53+
'TestLogger',
54+
false,
55+
false,
56+
true,
57+
['critical']
58+
);
59+
4560
$this->imageUploader = $this->getMock(
4661
\Magento\Catalog\Model\ImageUploader::class,
4762
['moveFileFromTmp'],
@@ -51,7 +66,7 @@ protected function setUp()
5166
);
5267
}
5368

54-
public function deletionValuesProvider()
69+
public function deletionValueProvider()
5570
{
5671
return [
5772
[false],
@@ -60,7 +75,7 @@ public function deletionValuesProvider()
6075
}
6176

6277
/**
63-
* @dataProvider deletionValuesProvider
78+
* @dataProvider deletionValueProvider
6479
*
6580
* @param $value
6681
*/
@@ -78,6 +93,36 @@ public function testBeforeSaveShouldSetAttributeValueToBlankWhenImageValueRequir
7893
$this->assertEquals('', $object->getTestAttribute());
7994
}
8095

96+
public function invalidValueProvider()
97+
{
98+
return [
99+
[1234],
100+
[true],
101+
[new \stdClass()],
102+
[function() {}],
103+
[['a' => 1, 'b' => 2]]
104+
];
105+
}
106+
107+
/**
108+
* @dataProvider invalidValueProvider
109+
*
110+
* @param $value
111+
*/
112+
public function testBeforeSaveShouldSetAttributeValueToBlankWhenImageValueInvalid($value)
113+
{
114+
$model = $this->objectManager->getObject(Model::class);
115+
$model->setAttribute($this->attribute);
116+
117+
$object = new \Magento\Framework\DataObject([
118+
'test_attribute' => $value
119+
]);
120+
121+
$model->beforeSave($object);
122+
123+
$this->assertEquals('', $object->getTestAttribute());
124+
}
125+
81126
public function testBeforeSaveShouldSetAttributeValueToUploadedImageName()
82127
{
83128
$model = $this->objectManager->getObject(Model::class);
@@ -109,55 +154,35 @@ public function testBeforeSaveShouldSetAttributeUploadInformationToTemporaryAttr
109154

110155
$this->assertEquals([
111156
['name' => 'test123.jpg', 'tmp_name' => 'abc123', 'url' => 'http://www.test.com/test123.jpg']
112-
], $object->getTestAttributeAdditionalData());
157+
], $object->getData('_additional_data_test_attribute'));
113158
}
114159

115-
public function stringValueProvider()
116-
{
117-
return [
118-
['test123'],
119-
[12345],
120-
[true],
121-
['some' => 'value']
122-
];
123-
}
124-
125-
/**
126-
* @dataProvider stringValueProvider
127-
*
128-
* @param $value
129-
*/
130-
public function testBeforeSaveShouldNotModifyAttributeValueWhenNotUploadData($value)
160+
public function testBeforeSaveShouldNotModifyAttributeValueWhenStringValue()
131161
{
132162
$model = $this->objectManager->getObject(Model::class);
133163
$model->setAttribute($this->attribute);
134164

135165
$object = new \Magento\Framework\DataObject([
136-
'test_attribute' => $value
166+
'test_attribute' => 'test123.jpg'
137167
]);
138168

139169
$model->beforeSave($object);
140170

141-
$this->assertEquals($value, $object->getTestAttribute());
171+
$this->assertEquals('test123.jpg', $object->getTestAttribute());
142172
}
143173

144-
/**
145-
* @dataProvider stringValueProvider
146-
*
147-
* @param $value
148-
*/
149-
public function testBeforeSaveShouldNotSetAdditionalDataWhenNotUploadData($value)
174+
public function testBeforeSaveShouldNotSetAdditionalDataWhenStringValue()
150175
{
151176
$model = $this->objectManager->getObject(Model::class);
152177
$model->setAttribute($this->attribute);
153178

154179
$object = new \Magento\Framework\DataObject([
155-
'test_attribute' => $value
180+
'test_attribute' => 'test123.jpg'
156181
]);
157182

158183
$model->beforeSave($object);
159184

160-
$this->assertNull($object->getTestAttributeAdditionalData());
185+
$this->assertNull($object->getData('_additional_data_test_attribute'));
161186
}
162187

163188
protected function setUpModelForAfterSave()
@@ -174,7 +199,7 @@ protected function setUpModelForAfterSave()
174199

175200
$objectManagerMock->expects($this->any())
176201
->method('get')
177-
->will($this->returnCallback(function($class, $params = []) use ($imageUploaderMock) {
202+
->will($this->returnCallback(function ($class, $params = []) use ($imageUploaderMock) {
178203
if ($class == \Magento\Catalog\CategoryImageUpload::class) {
179204
return $imageUploaderMock;
180205
}
@@ -183,13 +208,29 @@ protected function setUpModelForAfterSave()
183208
}));
184209

185210
$model = $this->objectManager->getObject(Model::class, [
186-
'objectManager' => $objectManagerMock
211+
'objectManager' => $objectManagerMock,
212+
'logger' => $this->logger
187213
]);
188214

189215
return $model->setAttribute($this->attribute);
190216
}
191217

192-
public function testAfterSaveShouldUploadImageWhenAdditionalDataSet()
218+
public function attributeValidValueProvider()
219+
{
220+
return [
221+
[[['name' => 'test1234.jpg']]],
222+
['test1234.jpg'],
223+
[''],
224+
[false]
225+
];
226+
}
227+
228+
/**
229+
* @dataProvider attributeValidValueProvider
230+
*
231+
* @param $value
232+
*/
233+
public function testAfterSaveShouldUploadImageWhenAdditionalDataSet($value)
193234
{
194235
$model = $this->setUpModelForAfterSave();
195236

@@ -198,23 +239,50 @@ public function testAfterSaveShouldUploadImageWhenAdditionalDataSet()
198239
->with($this->equalTo('test1234.jpg'));
199240

200241
$object = new \Magento\Framework\DataObject([
201-
'test_attribute_additional_data' => [
242+
'test_attribute' => $value,
243+
'_additional_data_test_attribute' => [
202244
['name' => 'test1234.jpg']
203245
]
204246
]);
205247

206248
$model->afterSave($object);
207249
}
208250

209-
public function testAfterSaveShouldNotUploadImageWhenAdditionalDataNotSet()
251+
/**
252+
* @dataProvider attributeValidValueProvider
253+
*
254+
* @param $value
255+
*/
256+
public function testAfterSaveShouldNotUploadImageWhenAdditionalDataNotSet($value)
210257
{
211258
$model = $this->setUpModelForAfterSave();
212259

213260
$this->imageUploader->expects($this->never())
214261
->method('moveFileFromTmp');
215262

216263
$object = new \Magento\Framework\DataObject([
217-
'test_attribute' => [
264+
'test_attribute' => $value
265+
]);
266+
267+
$model->afterSave($object);
268+
}
269+
270+
public function testAfterSaveShouldCreateCriticalLogEntryOnUploadExceptions()
271+
{
272+
$model = $this->setUpModelForAfterSave();
273+
274+
$exception = new \Exception();
275+
276+
$this->imageUploader->expects($this->any())
277+
->method('moveFileFromTmp')
278+
->will($this->throwException($exception));
279+
280+
$this->logger->expects($this->once())
281+
->method('critical')
282+
->with($this->equalTo($exception));
283+
284+
$object = new \Magento\Framework\DataObject([
285+
'_additional_data_test_attribute' => [
218286
['name' => 'test1234.jpg']
219287
]
220288
]);

0 commit comments

Comments
 (0)