From 4d9c75ecd38780e7c9a7849c4ac154580de317e8 Mon Sep 17 00:00:00 2001 From: Jan Brinkmann Date: Fri, 26 Jan 2018 17:55:19 +0100 Subject: [PATCH 1/7] Cosmetic fix for #7333 in GitHub or internal ticket id MAGETWO-60573 --- app/code/Magento/Catalog/view/base/web/js/price-options.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/code/Magento/Catalog/view/base/web/js/price-options.js b/app/code/Magento/Catalog/view/base/web/js/price-options.js index ceeea4c878622..e86f48c93aca8 100644 --- a/app/code/Magento/Catalog/view/base/web/js/price-options.js +++ b/app/code/Magento/Catalog/view/base/web/js/price-options.js @@ -20,8 +20,10 @@ define([ optionConfig: {}, optionHandlers: {}, optionTemplate: '<%= data.label %>' + - '<% if (data.finalPrice.value) { %>' + + '<% if (data.finalPrice.value > 0) { %>' + ' +<%- data.finalPrice.formatted %>' + + '<% } else if (data.finalPrice.value) { %>' + + ' <%- data.finalPrice.formatted %>' + '<% } %>', controlContainer: 'dd' }; From 0570a7c4eb45aa3bda974a4da1248f4ea2d18e63 Mon Sep 17 00:00:00 2001 From: Jan Brinkmann Date: Fri, 26 Jan 2018 18:52:47 +0100 Subject: [PATCH 2/7] Don't prevent negative custom option prices. --- .../Catalog/Model/Product/Option/Validator/DefaultValidator.php | 2 +- .../Magento/Catalog/Model/Product/Option/Validator/Select.php | 2 +- .../Ui/DataProvider/Product/Form/Modifier/CustomOptions.php | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/code/Magento/Catalog/Model/Product/Option/Validator/DefaultValidator.php b/app/code/Magento/Catalog/Model/Product/Option/Validator/DefaultValidator.php index 1e5c7f76d829b..d1fe4c570dc75 100644 --- a/app/code/Magento/Catalog/Model/Product/Option/Validator/DefaultValidator.php +++ b/app/code/Magento/Catalog/Model/Product/Option/Validator/DefaultValidator.php @@ -132,7 +132,7 @@ protected function validateOptionType(Option $option) */ protected function validateOptionValue(Option $option) { - return $this->isInRange($option->getPriceType(), $this->priceTypes) && !$this->isNegative($option->getPrice()); + return $this->isInRange($option->getPriceType(), $this->priceTypes); } /** diff --git a/app/code/Magento/Catalog/Model/Product/Option/Validator/Select.php b/app/code/Magento/Catalog/Model/Product/Option/Validator/Select.php index f04ab497e1d4f..44756890b6ed7 100644 --- a/app/code/Magento/Catalog/Model/Product/Option/Validator/Select.php +++ b/app/code/Magento/Catalog/Model/Product/Option/Validator/Select.php @@ -83,7 +83,7 @@ protected function isValidOptionPrice($priceType, $price, $storeId) if (!$priceType && !$price) { return true; } - if (!$this->isInRange($priceType, $this->priceTypes) || $this->isNegative($price)) { + if (!$this->isInRange($priceType, $this->priceTypes)) { return false; } diff --git a/app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/CustomOptions.php b/app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/CustomOptions.php index 7995926d27de5..2b75c4926516f 100755 --- a/app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/CustomOptions.php +++ b/app/code/Magento/Catalog/Ui/DataProvider/Product/Form/Modifier/CustomOptions.php @@ -923,7 +923,7 @@ protected function getPriceFieldConfig($sortOrder) 'addbeforePool' => $this->productOptionsPrice->prefixesToOptionArray(), 'sortOrder' => $sortOrder, 'validation' => [ - 'validate-zero-or-greater' => true + 'validate-zero-or-greater' => false ], ], ], From df6e2759417409baa6828d90e716fb938b0d50d9 Mon Sep 17 00:00:00 2001 From: Jan Brinkmann Date: Sat, 27 Jan 2018 13:08:20 +0100 Subject: [PATCH 3/7] fixed unit tests --- .../Option/Validator/DefaultValidatorTest.php | 14 +++++++------- .../Model/Product/Option/Validator/FileTest.php | 6 +++--- .../Model/Product/Option/Validator/SelectTest.php | 3 +-- .../Model/Product/Option/Validator/TextTest.php | 4 ++-- 4 files changed, 13 insertions(+), 14 deletions(-) diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php index 1eb5f1a2dacd2..71cabe407b728 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php @@ -84,7 +84,7 @@ public function testIsValidTitle($title, $type, $priceType, $price, $product, $m $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->never())->method('getPrice')->will($this->returnValue($price)); $valueMock->expects($this->once())->method('getProduct')->will($this->returnValue($product)); $this->assertEquals($result, $this->validator->isValid($valueMock)); $this->assertEquals($messages, $this->validator->getMessages()); @@ -129,7 +129,7 @@ public function testIsValidFail($product) * Data provider for testValidationNegativePrice * @return array */ - public function validationNegativePriceDataProvider() + public function negativePriceIsValidDataProvider() { return [ ['option_title', 'name 1.1', 'fixed', -12, new \Magento\Framework\DataObject(['store_id' => 1])], @@ -143,22 +143,22 @@ public function validationNegativePriceDataProvider() * @param $priceType * @param $price * @param $product - * @dataProvider validationNegativePriceDataProvider + * @dataProvider negativePriceIsValidDataProvider */ - public function testValidationNegativePrice($title, $type, $priceType, $price, $product) + public function testNegativePriceIsValid($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->never())->method('getPrice')->will($this->returnValue($price)); $valueMock->expects($this->once())->method('getProduct')->will($this->returnValue($product)); $messages = [ 'option values' => 'Invalid option value', ]; - $this->assertFalse($this->validator->isValid($valueMock)); - $this->assertEquals($messages, $this->validator->getMessages()); + $this->assertTrue($this->validator->isValid($valueMock)); + $this->assertNotEquals($messages, $this->validator->getMessages()); } } diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/FileTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/FileTest.php index 3c06db0e7ce5f..557ef7ef930dc 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/FileTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/FileTest.php @@ -59,7 +59,7 @@ public function testIsValidSuccess() $this->valueMock->expects($this->once())->method('getTitle')->will($this->returnValue('option_title')); $this->valueMock->expects($this->exactly(2))->method('getType')->will($this->returnValue('name 1.1')); $this->valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue('fixed')); - $this->valueMock->expects($this->once())->method('getPrice')->will($this->returnValue(10)); + $this->valueMock->expects($this->never())->method('getPrice')->will($this->returnValue(10)); $this->valueMock->expects($this->once())->method('getImageSizeX')->will($this->returnValue(10)); $this->valueMock->expects($this->once())->method('getImageSizeY')->will($this->returnValue(15)); $this->assertEmpty($this->validator->getMessages()); @@ -71,7 +71,7 @@ public function testIsValidWithNegativeImageSize() $this->valueMock->expects($this->once())->method('getTitle')->will($this->returnValue('option_title')); $this->valueMock->expects($this->exactly(2))->method('getType')->will($this->returnValue('name 1.1')); $this->valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue('fixed')); - $this->valueMock->expects($this->once())->method('getPrice')->will($this->returnValue(10)); + $this->valueMock->expects($this->never())->method('getPrice')->will($this->returnValue(10)); $this->valueMock->expects($this->once())->method('getImageSizeX')->will($this->returnValue(-10)); $this->valueMock->expects($this->never())->method('getImageSizeY'); $messages = [ @@ -86,7 +86,7 @@ public function testIsValidWithNegativeImageSizeY() $this->valueMock->expects($this->once())->method('getTitle')->will($this->returnValue('option_title')); $this->valueMock->expects($this->exactly(2))->method('getType')->will($this->returnValue('name 1.1')); $this->valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue('fixed')); - $this->valueMock->expects($this->once())->method('getPrice')->will($this->returnValue(10)); + $this->valueMock->expects($this->never())->method('getPrice')->will($this->returnValue(10)); $this->valueMock->expects($this->once())->method('getImageSizeX')->will($this->returnValue(10)); $this->valueMock->expects($this->once())->method('getImageSizeY')->will($this->returnValue(-10)); $messages = [ diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/SelectTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/SelectTest.php index 046ee703c850e..924893bce23a4 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/SelectTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/SelectTest.php @@ -87,7 +87,7 @@ public function isValidSuccessDataProvider() ] ], [ - false, + true, [ 'title' => 'Some Title', 'price_type' => 'fixed', @@ -157,7 +157,6 @@ public function testIsValidateWithInvalidData($priceType, $price, $title) public function isValidateWithInvalidDataDataProvider() { return [ - 'invalid_price' => ['fixed', -10, 'Title'], 'invalid_price_type' => ['some_value', '10', 'Title'], 'empty_title' => ['fixed', 10, null] ]; diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/TextTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/TextTest.php index cf31d67817684..1405e4faaace5 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/TextTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/TextTest.php @@ -59,7 +59,7 @@ public function testIsValidSuccess() $this->valueMock->expects($this->once())->method('getTitle')->will($this->returnValue('option_title')); $this->valueMock->expects($this->exactly(2))->method('getType')->will($this->returnValue('name 1.1')); $this->valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue('fixed')); - $this->valueMock->expects($this->once())->method('getPrice')->will($this->returnValue(10)); + $this->valueMock->expects($this->never())->method('getPrice')->will($this->returnValue(10)); $this->valueMock->expects($this->once())->method('getMaxCharacters')->will($this->returnValue(10)); $this->assertTrue($this->validator->isValid($this->valueMock)); $this->assertEmpty($this->validator->getMessages()); @@ -70,7 +70,7 @@ public function testIsValidWithNegativeMaxCharacters() $this->valueMock->expects($this->once())->method('getTitle')->will($this->returnValue('option_title')); $this->valueMock->expects($this->exactly(2))->method('getType')->will($this->returnValue('name 1.1')); $this->valueMock->expects($this->once())->method('getPriceType')->will($this->returnValue('fixed')); - $this->valueMock->expects($this->once())->method('getPrice')->will($this->returnValue(10)); + $this->valueMock->expects($this->never())->method('getPrice')->will($this->returnValue(10)); $this->valueMock->expects($this->once())->method('getMaxCharacters')->will($this->returnValue(-10)); $messages = [ 'option values' => 'Invalid option value', From a846858ec8b31374a329c8791f0908e86f239db0 Mon Sep 17 00:00:00 2001 From: Jan Brinkmann Date: Mon, 29 Jan 2018 11:59:32 +0100 Subject: [PATCH 4/7] If the method should never be called, the will is not necessary --- .../Model/Product/Option/Validator/DefaultValidatorTest.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php index 71cabe407b728..dea36ae5ec9a6 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php @@ -84,7 +84,7 @@ public function testIsValidTitle($title, $type, $priceType, $price, $product, $m $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->never())->method('getPrice')->will($this->returnValue($price)); + $valueMock->expects($this->never())->method('getPrice'); $valueMock->expects($this->once())->method('getProduct')->will($this->returnValue($product)); $this->assertEquals($result, $this->validator->isValid($valueMock)); $this->assertEquals($messages, $this->validator->getMessages()); @@ -152,7 +152,7 @@ public function testNegativePriceIsValid($title, $type, $priceType, $price, $pro $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->never())->method('getPrice')->will($this->returnValue($price)); + $valueMock->expects($this->never())->method('getPrice'); $valueMock->expects($this->once())->method('getProduct')->will($this->returnValue($product)); $messages = [ From a553cd05c3f55f78900aca1536348c47fd381551 Mon Sep 17 00:00:00 2001 From: Jan Brinkmann Date: Mon, 29 Jan 2018 12:44:36 +0100 Subject: [PATCH 5/7] Change tests: make sure valid price types for options are valid and invalid price types fail. Also makes sure price is not relevant for validation (assert that getPrice is not called) --- .../Option/Validator/DefaultValidatorTest.php | 56 +++++++++++++++---- 1 file changed, 46 insertions(+), 10 deletions(-) diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php index dea36ae5ec9a6..bae7c68ccd740 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php @@ -126,26 +126,61 @@ public function testIsValidFail($product) } /** - * Data provider for testValidationNegativePrice + * Data provider for testValidPriceTypeIsValid * @return array */ - public function negativePriceIsValidDataProvider() + public function validPriceTypeIsValidDataProvider() { 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', new \Magento\Framework\DataObject(['store_id' => 1])], + ['option_title', 'name 1.1', 'fixed', new \Magento\Framework\DataObject(['store_id' => 0])], ]; } /** + * + * * @param $title * @param $type * @param $priceType - * @param $price * @param $product - * @dataProvider negativePriceIsValidDataProvider + * @dataProvider validPriceTypeIsValidDataProvider */ - public function testNegativePriceIsValid($title, $type, $priceType, $price, $product) + public function testValidPriceTypeIsValid($title, $type, $priceType, $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->never())->method('getPrice'); + $valueMock->expects($this->once())->method('getProduct')->will($this->returnValue($product)); + + $messages = []; + $this->assertTrue($this->validator->isValid($valueMock)); + $this->assertEquals($messages, $this->validator->getMessages()); + } + + /** + * Data provider for testInvalidPriceTypeIsFail + * @return array + */ + public function invalidPriceTypeIsFailDataProvider() + { + return [ + ['option_title', 'name 1.1', 'notexisting', new \Magento\Framework\DataObject(['store_id' => 1])], + ['option_title', 'name 1.1', 'wrongtype', new \Magento\Framework\DataObject(['store_id' => 0])], + ]; + } + + /** + * @param $title + * @param $type + * @param $priceType + * @param $product + * @dataProvider invalidPriceTypeIsFailDataProvider + */ + public function testInvalidPriceTypeIsFail($title, $type, $priceType, $product) { $methods = ['getTitle', 'getType', 'getPriceType', 'getPrice', '__wakeup', 'getProduct']; $valueMock = $this->createPartialMock(\Magento\Catalog\Model\Product\Option::class, $methods); @@ -156,9 +191,10 @@ public function testNegativePriceIsValid($title, $type, $priceType, $price, $pro $valueMock->expects($this->once())->method('getProduct')->will($this->returnValue($product)); $messages = [ - 'option values' => 'Invalid option value', + 'option values' => 'Invalid option value' ]; - $this->assertTrue($this->validator->isValid($valueMock)); - $this->assertNotEquals($messages, $this->validator->getMessages()); + $this->assertFalse($this->validator->isValid($valueMock)); + $this->assertEquals($messages, $this->validator->getMessages()); } + } From b11965fd62d5d2543376e65db7b225438c806f73 Mon Sep 17 00:00:00 2001 From: Jan Brinkmann Date: Mon, 29 Jan 2018 13:41:50 +0100 Subject: [PATCH 6/7] Remove unused parameter from test --- .../Product/Option/Validator/DefaultValidatorTest.php | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php index bae7c68ccd740..4d0806907ac5e 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php @@ -60,10 +60,10 @@ public function isValidTitleDataProvider() { $mess = ['option required fields' => 'Missed values for option required fields']; return [ - ['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], + ['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], ]; } @@ -71,13 +71,12 @@ public function isValidTitleDataProvider() * @param $title * @param $type * @param $priceType - * @param $price * @param $product * @param $messages * @param $result * @dataProvider isValidTitleDataProvider */ - public function testIsValidTitle($title, $type, $priceType, $price, $product, $messages, $result) + public function testIsValidTitle($title, $type, $priceType, $product, $messages, $result) { $methods = ['getTitle', 'getType', 'getPriceType', 'getPrice', '__wakeup', 'getProduct']; $valueMock = $this->createPartialMock(\Magento\Catalog\Model\Product\Option::class, $methods); From b51d8c0468f98fa3a8976d93369c58f119b3e4b0 Mon Sep 17 00:00:00 2001 From: Jan Brinkmann Date: Mon, 29 Jan 2018 14:53:01 +0100 Subject: [PATCH 7/7] cosmectic fix: remove empty line --- .../Unit/Model/Product/Option/Validator/DefaultValidatorTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php index 4d0806907ac5e..086af60994d8d 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/Option/Validator/DefaultValidatorTest.php @@ -195,5 +195,4 @@ public function testInvalidPriceTypeIsFail($title, $type, $priceType, $product) $this->assertFalse($this->validator->isValid($valueMock)); $this->assertEquals($messages, $this->validator->getMessages()); } - }