Skip to content

[Bugfix] #7333 Unable to set negative custom option fixed price in admin view. #15267

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

Merged
merged 10 commits into from
Jan 14, 2019
Merged
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,20 @@ class DefaultValidator extends \Magento\Framework\Validator\AbstractValidator
*/
protected $priceTypes;

/**
* @var \Magento\Framework\Locale\FormatInterface
*/
private $localeFormat;

/**
* @param \Magento\Catalog\Model\ProductOptions\ConfigInterface $productOptionConfig
* @param \Magento\Catalog\Model\Config\Source\Product\Options\Price $priceConfig
* @param \Magento\Framework\Locale\FormatInterface|null $localeFormat
*/
public function __construct(
\Magento\Catalog\Model\ProductOptions\ConfigInterface $productOptionConfig,
\Magento\Catalog\Model\Config\Source\Product\Options\Price $priceConfig
\Magento\Catalog\Model\Config\Source\Product\Options\Price $priceConfig,
\Magento\Framework\Locale\FormatInterface $localeFormat = null
) {
foreach ($productOptionConfig->getAll() as $option) {
foreach ($option['types'] as $type) {
Expand All @@ -45,6 +52,9 @@ public function __construct(
foreach ($priceConfig->toOptionArray() as $item) {
$this->priceTypes[] = $item['value'];
}

$this->localeFormat = $localeFormat ?: \Magento\Framework\App\ObjectManager::getInstance()
->get(\Magento\Framework\Locale\FormatInterface::class);
}

/**
Expand Down Expand Up @@ -137,11 +147,11 @@ protected function validateOptionType(Option $option)
*/
protected function validateOptionValue(Option $option)
{
return $this->isInRange($option->getPriceType(), $this->priceTypes);
return $this->isInRange($option->getPriceType(), $this->priceTypes) && $this->isNumber($option->getPrice());
}

/**
* Check whether value is empty
* Check whether the value is empty
*
* @param mixed $value
* @return bool
Expand All @@ -152,7 +162,7 @@ protected function isEmpty($value)
}

/**
* Check whether value is in range
* Check whether the value is in range
*
* @param string $value
* @param array $range
Expand All @@ -164,13 +174,24 @@ protected function isInRange($value, array $range)
}

/**
* Check whether value is not negative
* Check whether the value is negative
*
* @param string $value
* @return bool
*/
protected function isNegative($value)
{
return (int) $value < 0;
return $this->localeFormat->getNumber($value) < 0;
}

/**
* Check whether the value is a number
*
* @param string $value
* @return bool
*/
public function isNumber($value)
{
return is_numeric($this->localeFormat->getNumber($value));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,9 @@

use Magento\Catalog\Model\Product\Option;

/**
* Select validator class
*/
class Select extends DefaultValidator
{
/**
Expand Down Expand Up @@ -83,7 +86,7 @@ protected function isValidOptionPrice($priceType, $price, $storeId)
if (!$priceType && !$price) {
return true;
}
if (!$this->isInRange($priceType, $this->priceTypes)) {
if (!$this->isInRange($priceType, $this->priceTypes) || !$this->isNumber($price)) {
return false;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ class DefaultValidatorTest extends \PHPUnit\Framework\TestCase
*/
protected $valueMock;

/**
* @var \PHPUnit_Framework_MockObject_MockObject
*/
protected $localeFormatMock;

/**
* @inheritdoc
*/
Expand All @@ -26,6 +31,8 @@ protected function setUp()
$configMock = $this->createMock(\Magento\Catalog\Model\ProductOptions\ConfigInterface::class);
$storeManagerMock = $this->createMock(\Magento\Store\Model\StoreManagerInterface::class);
$priceConfigMock = new \Magento\Catalog\Model\Config\Source\Product\Options\Price($storeManagerMock);
$this->localeFormatMock = $this->createMock(\Magento\Framework\Locale\FormatInterface::class);

$config = [
[
'label' => 'group label 1',
Expand All @@ -51,7 +58,8 @@ protected function setUp()
$configMock->expects($this->once())->method('getAll')->will($this->returnValue($config));
$this->validator = new \Magento\Catalog\Model\Product\Option\Validator\DefaultValidator(
$configMock,
$priceConfigMock
$priceConfigMock,
$this->localeFormatMock
);
}

Expand All @@ -63,10 +71,10 @@ public function isValidTitleDataProvider()
{
$mess = ['option required fields' => 'Missed values for option required fields'];
return [
['option_title', 'name 1.1', 'fixed', new \Magento\Framework\DataObject(['store_id' => 1]), [], true],
['option_title', 'name 1.1', 'fixed', new \Magento\Framework\DataObject(['store_id' => 0]), [], true],
[null, 'name 1.1', 'fixed', new \Magento\Framework\DataObject(['store_id' => 1]), [], true],
[null, 'name 1.1', 'fixed', new \Magento\Framework\DataObject(['store_id' => 0]), $mess, false],
['option_title', 'name 1.1', 'fixed', 10, new \Magento\Framework\DataObject(['store_id' => 1]), [], true],
['option_title', 'name 1.1', 'fixed', 10, new \Magento\Framework\DataObject(['store_id' => 0]), [], true],
[null, 'name 1.1', 'fixed', 10, new \Magento\Framework\DataObject(['store_id' => 1]), [], true],
[null, 'name 1.1', 'fixed', 10, new \Magento\Framework\DataObject(['store_id' => 0]), $mess, false],
];
}

Expand All @@ -79,15 +87,18 @@ public function isValidTitleDataProvider()
* @param bool $result
* @dataProvider isValidTitleDataProvider
*/
public function testIsValidTitle($title, $type, $priceType, $product, $messages, $result)
public function testIsValidTitle($title, $type, $priceType, $price, $product, $messages, $result)
{
$methods = ['getTitle', 'getType', 'getPriceType', '__wakeup', 'getProduct'];
$methods = ['getTitle', 'getType', 'getPriceType', 'getPrice', '__wakeup', 'getProduct'];
$valueMock = $this->createPartialMock(\Magento\Catalog\Model\Product\Option::class, $methods);
$valueMock->expects($this->once())->method('getTitle')->will($this->returnValue($title));
$valueMock->expects($this->any())->method('getType')->will($this->returnValue($type));
$valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue($priceType));
// $valueMock->expects($this->once())->method('getPrice')->will($this->returnValue($price));
$valueMock->expects($this->once())->method('getPrice')->will($this->returnValue($price));
$valueMock->expects($this->once())->method('getProduct')->will($this->returnValue($product));

$this->localeFormatMock->expects($this->once())->method('getNumber')->will($this->returnValue($price));

$this->assertEquals($result, $this->validator->isValid($valueMock));
$this->assertEquals($messages, $this->validator->getMessages());
}
Expand Down Expand Up @@ -126,4 +137,43 @@ public function testIsValidFail($product)
$this->assertFalse($this->validator->isValid($valueMock));
$this->assertEquals($messages, $this->validator->getMessages());
}

/**
* Data provider for testValidationNegativePrice
* @return array
*/
public function validationPriceDataProvider()
{
return [
['option_title', 'name 1.1', 'fixed', -12, new \Magento\Framework\DataObject(['store_id' => 1])],
['option_title', 'name 1.1', 'fixed', -12, new \Magento\Framework\DataObject(['store_id' => 0])],
['option_title', 'name 1.1', 'fixed', 12, new \Magento\Framework\DataObject(['store_id' => 1])],
['option_title', 'name 1.1', 'fixed', 12, new \Magento\Framework\DataObject(['store_id' => 0])]
];
}

/**
* @param $title
* @param $type
* @param $priceType
* @param $price
* @param $product
* @dataProvider validationPriceDataProvider
*/
public function testValidationPrice($title, $type, $priceType, $price, $product)
{
$methods = ['getTitle', 'getType', 'getPriceType', 'getPrice', '__wakeup', 'getProduct'];
$valueMock = $this->createPartialMock(\Magento\Catalog\Model\Product\Option::class, $methods);
$valueMock->expects($this->once())->method('getTitle')->will($this->returnValue($title));
$valueMock->expects($this->exactly(2))->method('getType')->will($this->returnValue($type));
$valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue($priceType));
$valueMock->expects($this->once())->method('getPrice')->will($this->returnValue($price));
$valueMock->expects($this->once())->method('getProduct')->will($this->returnValue($product));

$this->localeFormatMock->expects($this->once())->method('getNumber')->will($this->returnValue($price));

$messages = [];
$this->assertTrue($this->validator->isValid($valueMock));
$this->assertEquals($messages, $this->validator->getMessages());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ class FileTest extends \PHPUnit\Framework\TestCase
*/
protected $valueMock;

/**
* @var \PHPUnit_Framework_MockObject_MockObject
*/
protected $localeFormatMock;

/**
* @inheritdoc
*/
Expand All @@ -26,6 +31,8 @@ protected function setUp()
$configMock = $this->createMock(\Magento\Catalog\Model\ProductOptions\ConfigInterface::class);
$storeManagerMock = $this->createMock(\Magento\Store\Model\StoreManagerInterface::class);
$priceConfigMock = new \Magento\Catalog\Model\Config\Source\Product\Options\Price($storeManagerMock);
$this->localeFormatMock = $this->createMock(\Magento\Framework\Locale\FormatInterface::class);

$config = [
[
'label' => 'group label 1',
Expand Down Expand Up @@ -53,7 +60,8 @@ protected function setUp()
$this->valueMock = $this->createPartialMock(\Magento\Catalog\Model\Product\Option::class, $methods);
$this->validator = new \Magento\Catalog\Model\Product\Option\Validator\File(
$configMock,
$priceConfigMock
$priceConfigMock,
$this->localeFormatMock
);
}

Expand All @@ -70,6 +78,15 @@ public function testIsValidSuccess()
->willReturn(10);
$this->valueMock->expects($this->once())->method('getImageSizeX')->will($this->returnValue(10));
$this->valueMock->expects($this->once())->method('getImageSizeY')->will($this->returnValue(15));
$this->localeFormatMock->expects($this->at(0))
->method('getNumber')
->with($this->equalTo(10))
->will($this->returnValue(10));
$this->localeFormatMock
->expects($this->at(2))
->method('getNumber')
->with($this->equalTo(15))
->will($this->returnValue(15));
$this->assertEmpty($this->validator->getMessages());
$this->assertTrue($this->validator->isValid($this->valueMock));
}
Expand All @@ -87,6 +104,16 @@ public function testIsValidWithNegativeImageSize()
->willReturn(10);
$this->valueMock->expects($this->once())->method('getImageSizeX')->will($this->returnValue(-10));
$this->valueMock->expects($this->never())->method('getImageSizeY');
$this->localeFormatMock->expects($this->at(0))
->method('getNumber')
->with($this->equalTo(10))
->will($this->returnValue(10));
$this->localeFormatMock
->expects($this->at(1))
->method('getNumber')
->with($this->equalTo(-10))
->will($this->returnValue(-10));

$messages = [
'option values' => 'Invalid option value',
];
Expand All @@ -107,6 +134,15 @@ public function testIsValidWithNegativeImageSizeY()
->willReturn(10);
$this->valueMock->expects($this->once())->method('getImageSizeX')->will($this->returnValue(10));
$this->valueMock->expects($this->once())->method('getImageSizeY')->will($this->returnValue(-10));
$this->localeFormatMock->expects($this->at(0))
->method('getNumber')
->with($this->equalTo(10))
->will($this->returnValue(10));
$this->localeFormatMock
->expects($this->at(2))
->method('getNumber')
->with($this->equalTo(-10))
->will($this->returnValue(-10));
$messages = [
'option values' => 'Invalid option value',
];
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,11 @@ class SelectTest extends \PHPUnit\Framework\TestCase
*/
protected $valueMock;

/**
* @var \PHPUnit_Framework_MockObject_MockObject
*/
protected $localeFormatMock;

/**
* @inheritdoc
*/
Expand All @@ -26,6 +31,7 @@ protected function setUp()
$configMock = $this->createMock(\Magento\Catalog\Model\ProductOptions\ConfigInterface::class);
$storeManagerMock = $this->createMock(\Magento\Store\Model\StoreManagerInterface::class);
$priceConfigMock = new \Magento\Catalog\Model\Config\Source\Product\Options\Price($storeManagerMock);
$this->localeFormatMock = $this->createMock(\Magento\Framework\Locale\FormatInterface::class);
$config = [
[
'label' => 'group label 1',
Expand Down Expand Up @@ -53,7 +59,8 @@ protected function setUp()
$this->valueMock = $this->createPartialMock(\Magento\Catalog\Model\Product\Option::class, $methods, []);
$this->validator = new \Magento\Catalog\Model\Product\Option\Validator\Select(
$configMock,
$priceConfigMock
$priceConfigMock,
$this->localeFormatMock
);
}

Expand All @@ -69,6 +76,12 @@ public function testIsValidSuccess($expectedResult, array $value)
$this->valueMock->expects($this->never())->method('getPriceType');
$this->valueMock->expects($this->never())->method('getPrice');
$this->valueMock->expects($this->any())->method('getData')->with('values')->will($this->returnValue([$value]));
if (isset($value['price'])) {
$this->localeFormatMock
->expects($this->once())
->method('getNumber')
->will($this->returnValue($value['price']));
}
$this->assertEquals($expectedResult, $this->validator->isValid($this->valueMock));
}

Expand Down Expand Up @@ -117,6 +130,7 @@ public function testIsValidateWithInvalidOptionValues()
->method('getData')
->with('values')
->will($this->returnValue('invalid_data'));

$messages = [
'option values' => 'Invalid option value',
];
Expand Down Expand Up @@ -159,6 +173,7 @@ public function testIsValidateWithInvalidData($priceType, $price, $title)
$this->valueMock->expects($this->never())->method('getPriceType');
$this->valueMock->expects($this->never())->method('getPrice');
$this->valueMock->expects($this->any())->method('getData')->with('values')->will($this->returnValue([$value]));
$this->localeFormatMock->expects($this->any())->method('getNumber')->will($this->returnValue($price));
$messages = [
'option values' => 'Invalid option value',
];
Expand Down
Loading