From ae5dfae57e62170de4668812454ad57fe8ff8b26 Mon Sep 17 00:00:00 2001 From: MaxNovik Date: Thu, 22 Nov 2018 14:34:25 +0200 Subject: [PATCH 1/2] [Forwardport] Cart-Sales-Rule-with-negated-condition-over-special-price-does-not-work-for-configurable-products. Use configurable product`s children for shopping cart rules validation for cases when attribute exists for children only (E.g. special_price) --- .../Unit/Model/Rule/Condition/ProductTest.php | 1 + .../Model/Rule/Condition/Product.php | 69 ++++++ .../Model/Rule/Condition/ProductTest.php | 225 ++++++++++++++++++ .../Magento/ConfigurableProduct/etc/di.xml | 3 + .../Model/Rule/Condition/Product.php | 20 +- 5 files changed, 312 insertions(+), 6 deletions(-) create mode 100644 app/code/Magento/ConfigurableProduct/Plugin/SalesRule/Model/Rule/Condition/Product.php create mode 100644 app/code/Magento/ConfigurableProduct/Test/Unit/Plugin/SalesRule/Model/Rule/Condition/ProductTest.php diff --git a/app/code/Magento/CatalogWidget/Test/Unit/Model/Rule/Condition/ProductTest.php b/app/code/Magento/CatalogWidget/Test/Unit/Model/Rule/Condition/ProductTest.php index 219cae6829299..00550383ce26f 100644 --- a/app/code/Magento/CatalogWidget/Test/Unit/Model/Rule/Condition/ProductTest.php +++ b/app/code/Magento/CatalogWidget/Test/Unit/Model/Rule/Condition/ProductTest.php @@ -9,6 +9,7 @@ /** * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + * @SuppressWarnings(PHPMD.LongVariable) */ class ProductTest extends \PHPUnit\Framework\TestCase { diff --git a/app/code/Magento/ConfigurableProduct/Plugin/SalesRule/Model/Rule/Condition/Product.php b/app/code/Magento/ConfigurableProduct/Plugin/SalesRule/Model/Rule/Condition/Product.php new file mode 100644 index 0000000000000..1ed4432347b7a --- /dev/null +++ b/app/code/Magento/ConfigurableProduct/Plugin/SalesRule/Model/Rule/Condition/Product.php @@ -0,0 +1,69 @@ +getProductToValidate($subject, $model); + if ($model->getProduct() !== $product) { + // We need to replace product only for validation and keep original product for all other cases. + $clone = clone $model; + $clone->setProduct($product); + $model = $clone; + } + + return [$model]; + } + + /** + * Select proper product for validation. + * + * @param \Magento\SalesRule\Model\Rule\Condition\Product $subject + * @param \Magento\Framework\Model\AbstractModel $model + * + * @return \Magento\Catalog\Api\Data\ProductInterface|\Magento\Catalog\Model\Product + */ + private function getProductToValidate( + \Magento\SalesRule\Model\Rule\Condition\Product $subject, + \Magento\Framework\Model\AbstractModel $model + ) { + /** @var \Magento\Catalog\Model\Product $product */ + $product = $model->getProduct(); + + $attrCode = $subject->getAttribute(); + + /* Check for attributes which are not available for configurable products */ + if ($product->getTypeId() == Configurable::TYPE_CODE && !$product->hasData($attrCode)) { + /** @var \Magento\Catalog\Model\AbstractModel $childProduct */ + $childProduct = current($model->getChildren())->getProduct(); + if ($childProduct->hasData($attrCode)) { + $product = $childProduct; + } + } + + return $product; + } +} diff --git a/app/code/Magento/ConfigurableProduct/Test/Unit/Plugin/SalesRule/Model/Rule/Condition/ProductTest.php b/app/code/Magento/ConfigurableProduct/Test/Unit/Plugin/SalesRule/Model/Rule/Condition/ProductTest.php new file mode 100644 index 0000000000000..c9d06709defa1 --- /dev/null +++ b/app/code/Magento/ConfigurableProduct/Test/Unit/Plugin/SalesRule/Model/Rule/Condition/ProductTest.php @@ -0,0 +1,225 @@ +objectManager = new \Magento\Framework\TestFramework\Unit\Helper\ObjectManager($this); + $this->validator = $this->createValidator(); + $this->validatorPlugin = $this->objectManager->getObject(ValidatorPlugin::class); + } + + /** + * @return \Magento\SalesRule\Model\Rule\Condition\Product + */ + private function createValidator(): SalesRuleProduct + { + /** @var Context|\PHPUnit_Framework_MockObject_MockObject $contextMock */ + $contextMock = $this->getMockBuilder(Context::class) + ->disableOriginalConstructor() + ->getMock(); + /** @var Data|\PHPUnit_Framework_MockObject_MockObject $backendHelperMock */ + $backendHelperMock = $this->getMockBuilder(Data::class) + ->disableOriginalConstructor() + ->getMock(); + /** @var Config|\PHPUnit_Framework_MockObject_MockObject $configMock */ + $configMock = $this->getMockBuilder(Config::class) + ->disableOriginalConstructor() + ->getMock(); + /** @var ProductFactory|\PHPUnit_Framework_MockObject_MockObject $productFactoryMock */ + $productFactoryMock = $this->getMockBuilder(ProductFactory::class) + ->disableOriginalConstructor() + ->getMock(); + /** @var ProductRepositoryInterface|\PHPUnit_Framework_MockObject_MockObject $productRepositoryMock */ + $productRepositoryMock = $this->getMockBuilder(ProductRepositoryInterface::class) + ->getMockForAbstractClass(); + $attributeLoaderInterfaceMock = $this->getMockBuilder(AbstractEntity::class) + ->disableOriginalConstructor() + ->setMethods(['getAttributesByCode']) + ->getMock(); + $attributeLoaderInterfaceMock + ->expects($this->any()) + ->method('getAttributesByCode') + ->willReturn([]); + /** @var Product|\PHPUnit_Framework_MockObject_MockObject $productMock */ + $productMock = $this->getMockBuilder(Product::class) + ->disableOriginalConstructor() + ->setMethods(['loadAllAttributes', 'getConnection', 'getTable']) + ->getMock(); + $productMock->expects($this->any()) + ->method('loadAllAttributes') + ->willReturn($attributeLoaderInterfaceMock); + /** @var Collection|\PHPUnit_Framework_MockObject_MockObject $collectionMock */ + $collectionMock = $this->getMockBuilder(Collection::class) + ->disableOriginalConstructor() + ->getMock(); + /** @var FormatInterface|\PHPUnit_Framework_MockObject_MockObject $formatMock */ + $formatMock = new Format( + $this->getMockBuilder(ScopeResolverInterface::class)->disableOriginalConstructor()->getMock(), + $this->getMockBuilder(ResolverInterface::class)->disableOriginalConstructor()->getMock(), + $this->getMockBuilder(CurrencyFactory::class)->disableOriginalConstructor()->getMock() + ); + + return new SalesRuleProduct( + $contextMock, + $backendHelperMock, + $configMock, + $productFactoryMock, + $productRepositoryMock, + $productMock, + $collectionMock, + $formatMock + ); + } + + public function testChildIsUsedForValidation() + { + $configurableProductMock = $this->createProductMock(); + $configurableProductMock + ->expects($this->any()) + ->method('getTypeId') + ->willReturn(Configurable::TYPE_CODE); + $configurableProductMock + ->expects($this->any()) + ->method('hasData') + ->with($this->equalTo('special_price')) + ->willReturn(false); + + /* @var AbstractItem|\PHPUnit_Framework_MockObject_MockObject $item */ + $item = $this->getMockBuilder(AbstractItem::class) + ->disableOriginalConstructor() + ->setMethods(['setProduct', 'getProduct', 'getChildren']) + ->getMockForAbstractClass(); + $item->expects($this->any()) + ->method('getProduct') + ->willReturn($configurableProductMock); + + $simpleProductMock = $this->createProductMock(); + $simpleProductMock + ->expects($this->any()) + ->method('getTypeId') + ->willReturn(Type::TYPE_SIMPLE); + $simpleProductMock + ->expects($this->any()) + ->method('hasData') + ->with($this->equalTo('special_price')) + ->willReturn(true); + + $childItem = $this->getMockBuilder(AbstractItem::class) + ->disableOriginalConstructor() + ->setMethods(['getProduct']) + ->getMockForAbstractClass(); + $childItem->expects($this->any()) + ->method('getProduct') + ->willReturn($simpleProductMock); + + $item->expects($this->any()) + ->method('getChildren') + ->willReturn([$childItem]); + $item->expects($this->once()) + ->method('setProduct') + ->with($simpleProductMock); + + $this->validator->setAttribute('special_price'); + + $this->validatorPlugin->beforeValidate($this->validator, $item); + } + + /** + * @return Product|\PHPUnit_Framework_MockObject_MockObject + */ + private function createProductMock(): \PHPUnit_Framework_MockObject_MockObject + { + $productMock = $this->getMockBuilder(\Magento\Catalog\Model\Product::class) + ->disableOriginalConstructor() + ->setMethods([ + 'getAttribute', + 'getId', + 'setQuoteItemQty', + 'setQuoteItemPrice', + 'getTypeId', + 'hasData', + ]) + ->getMock(); + $productMock + ->expects($this->any()) + ->method('setQuoteItemQty') + ->willReturnSelf(); + $productMock + ->expects($this->any()) + ->method('setQuoteItemPrice') + ->willReturnSelf(); + + return $productMock; + } + + public function testChildIsNotUsedForValidation() + { + $simpleProductMock = $this->createProductMock(); + $simpleProductMock + ->expects($this->any()) + ->method('getTypeId') + ->willReturn(Type::TYPE_SIMPLE); + $simpleProductMock + ->expects($this->any()) + ->method('hasData') + ->with($this->equalTo('special_price')) + ->willReturn(true); + + /* @var AbstractItem|\PHPUnit_Framework_MockObject_MockObject $item */ + $item = $this->getMockBuilder(AbstractItem::class) + ->disableOriginalConstructor() + ->setMethods(['setProduct', 'getProduct']) + ->getMockForAbstractClass(); + $item->expects($this->any()) + ->method('getProduct') + ->willReturn($simpleProductMock); + + $this->validator->setAttribute('special_price'); + + $this->validatorPlugin->beforeValidate($this->validator, $item); + } +} diff --git a/app/code/Magento/ConfigurableProduct/etc/di.xml b/app/code/Magento/ConfigurableProduct/etc/di.xml index 1dbb0969687d5..a1cf1a4f28e8e 100644 --- a/app/code/Magento/ConfigurableProduct/etc/di.xml +++ b/app/code/Magento/ConfigurableProduct/etc/di.xml @@ -235,4 +235,7 @@ + + + diff --git a/app/code/Magento/SalesRule/Model/Rule/Condition/Product.php b/app/code/Magento/SalesRule/Model/Rule/Condition/Product.php index 9bda4793e8681..ff83bb1ee9129 100644 --- a/app/code/Magento/SalesRule/Model/Rule/Condition/Product.php +++ b/app/code/Magento/SalesRule/Model/Rule/Condition/Product.php @@ -35,12 +35,13 @@ protected function _addSpecialAttributes(array &$attributes) * * @return string */ - public function getAttribute() + public function getAttribute(): string { $attribute = $this->getData('attribute'); if (strpos($attribute, '::') !== false) { - list (, $attribute) = explode('::', $attribute); + list(, $attribute) = explode('::', $attribute); } + return $attribute; } @@ -53,6 +54,7 @@ public function getAttributeName() if ($this->getAttributeScope()) { $attribute = $this->getAttributeScope() . '::' . $attribute; } + return $this->getAttributeOption($attribute); } @@ -92,6 +94,7 @@ public function getAttributeElementHtml() { $html = parent::getAttributeElementHtml() . $this->getAttributeScopeElement()->getHtml(); + return $html; } @@ -100,7 +103,7 @@ public function getAttributeElementHtml() * * @return \Magento\Framework\Data\Form\Element\AbstractElement */ - private function getAttributeScopeElement() + private function getAttributeScopeElement(): \Magento\Framework\Data\Form\Element\AbstractElement { return $this->getForm()->addField( $this->getPrefix() . '__' . $this->getId() . '__attribute_scope', @@ -110,7 +113,7 @@ private function getAttributeScopeElement() 'value' => $this->getAttributeScope(), 'no_span' => true, 'class' => 'hidden', - 'data-form-part' => $this->getFormName() + 'data-form-part' => $this->getFormName(), ] ); } @@ -119,8 +122,9 @@ private function getAttributeScopeElement() * Set attribute value * * @param string $value + * @return void */ - public function setAttribute($value) + public function setAttribute(string $value) { if (strpos($value, '::') !== false) { list($scope, $attribute) = explode('::', $value); @@ -137,7 +141,8 @@ public function setAttribute($value) public function loadArray($arr) { parent::loadArray($arr); - $this->setAttributeScope(isset($arr['attribute_scope']) ? $arr['attribute_scope'] : null); + $this->setAttributeScope($arr['attribute_scope'] ?? null); + return $this; } @@ -148,6 +153,7 @@ public function asArray(array $arrAttributes = []) { $out = parent::asArray($arrAttributes); $out['attribute_scope'] = $this->getAttributeScope(); + return $out; } @@ -155,7 +161,9 @@ public function asArray(array $arrAttributes = []) * Validate Product Rule Condition * * @param \Magento\Framework\Model\AbstractModel $model + * * @return bool + * @throws \Magento\Framework\Exception\NoSuchEntityException */ public function validate(\Magento\Framework\Model\AbstractModel $model) { From 6754554ef57fa60d54e07dc5611d105ee286c5a9 Mon Sep 17 00:00:00 2001 From: nmalevanec Date: Wed, 12 Dec 2018 18:25:08 +0200 Subject: [PATCH 2/2] Fix static and functional tests. --- .../Test/Unit/Model/Rule/Condition/ProductTest.php | 1 - ...tingByPriceForConfigurableWithCatalogRuleAppliedTest.xml | 6 +++--- .../Plugin/SalesRule/Model/Rule/Condition/ProductTest.php | 3 ++- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/code/Magento/CatalogWidget/Test/Unit/Model/Rule/Condition/ProductTest.php b/app/code/Magento/CatalogWidget/Test/Unit/Model/Rule/Condition/ProductTest.php index 00550383ce26f..219cae6829299 100644 --- a/app/code/Magento/CatalogWidget/Test/Unit/Model/Rule/Condition/ProductTest.php +++ b/app/code/Magento/CatalogWidget/Test/Unit/Model/Rule/Condition/ProductTest.php @@ -9,7 +9,6 @@ /** * @SuppressWarnings(PHPMD.CouplingBetweenObjects) - * @SuppressWarnings(PHPMD.LongVariable) */ class ProductTest extends \PHPUnit\Framework\TestCase { diff --git a/app/code/Magento/ConfigurableProduct/Test/Mftf/Test/StorefrontSortingByPriceForConfigurableWithCatalogRuleAppliedTest.xml b/app/code/Magento/ConfigurableProduct/Test/Mftf/Test/StorefrontSortingByPriceForConfigurableWithCatalogRuleAppliedTest.xml index c93b216fc89d5..cd185ea534d0a 100644 --- a/app/code/Magento/ConfigurableProduct/Test/Mftf/Test/StorefrontSortingByPriceForConfigurableWithCatalogRuleAppliedTest.xml +++ b/app/code/Magento/ConfigurableProduct/Test/Mftf/Test/StorefrontSortingByPriceForConfigurableWithCatalogRuleAppliedTest.xml @@ -150,9 +150,9 @@ - - - + + + diff --git a/app/code/Magento/ConfigurableProduct/Test/Unit/Plugin/SalesRule/Model/Rule/Condition/ProductTest.php b/app/code/Magento/ConfigurableProduct/Test/Unit/Plugin/SalesRule/Model/Rule/Condition/ProductTest.php index c9d06709defa1..80979148c4959 100644 --- a/app/code/Magento/ConfigurableProduct/Test/Unit/Plugin/SalesRule/Model/Rule/Condition/ProductTest.php +++ b/app/code/Magento/ConfigurableProduct/Test/Unit/Plugin/SalesRule/Model/Rule/Condition/ProductTest.php @@ -4,7 +4,7 @@ * See COPYING.txt for license details. */ -namespace Magento\ConfigurableProduct\Plugin\SalesRule\Model\Rule\Condition; +namespace Magento\ConfigurableProduct\Test\Unit\Plugin\SalesRule\Model\Rule\Condition; use Magento\Backend\Helper\Data; use Magento\Catalog\Api\ProductRepositoryInterface; @@ -27,6 +27,7 @@ /** * @SuppressWarnings(PHPMD.CouplingBetweenObjects) + * @SuppressWarnings(PHPMD.LongVariable) */ class ProductTest extends \PHPUnit\Framework\TestCase {