From 1f96b873283229aa6892444244475d9992c0d8f2 Mon Sep 17 00:00:00 2001 From: Dasharath Date: Sun, 2 Feb 2020 15:00:42 +0530 Subject: [PATCH 1/3] [26054-Do not duplicate SEO meta data when duplicating a product] --- app/code/Magento/Catalog/Model/Product/Copier.php | 7 +++---- .../Magento/Catalog/Test/Unit/Model/Product/CopierTest.php | 3 +++ 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/code/Magento/Catalog/Model/Product/Copier.php b/app/code/Magento/Catalog/Model/Product/Copier.php index a7f7bad1a5167..22419123d1614 100644 --- a/app/code/Magento/Catalog/Model/Product/Copier.php +++ b/app/code/Magento/Catalog/Model/Product/Copier.php @@ -78,6 +78,9 @@ public function copy(Product $product) $productData = $this->removeStockItem($productData); $duplicate->setData($productData); $duplicate->setOptions([]); + $duplicate->setMetaTitle(null); + $duplicate->setMetaKeyword(null); + $duplicate->setMetaDescription(null); $duplicate->setIsDuplicate(true); $duplicate->setOriginalLinkId($product->getData($metadata->getLinkField())); $duplicate->setStatus(\Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_DISABLED); @@ -89,10 +92,6 @@ public function copy(Product $product) $this->setDefaultUrl($product, $duplicate); $this->setStoresUrl($product, $duplicate); $this->getOptionRepository()->duplicate($product, $duplicate); - $product->getResource()->duplicate( - $product->getData($metadata->getLinkField()), - $duplicate->getData($metadata->getLinkField()) - ); return $duplicate; } diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php index 809fa0225278c..34fddc06f1c42 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php @@ -197,6 +197,9 @@ public function testCopy() $duplicateMock->expects($this->once())->method('setCreatedAt')->with(null); $duplicateMock->expects($this->once())->method('setUpdatedAt')->with(null); $duplicateMock->expects($this->once())->method('setId')->with(null); + $duplicateMock->expects($this->once())->method('setMetaTitle')->with(null); + $duplicateMock->expects($this->once())->method('setMetaKeyword')->with(null); + $duplicateMock->expects($this->once())->method('setMetaDescription')->with(null); $duplicateMock->expects($this->atLeastOnce())->method('getStoreIds')->willReturn([]); $duplicateMock->expects($this->atLeastOnce())->method('setData')->willReturn($duplicateMock); $this->copyConstructorMock->expects($this->once())->method('build')->with($this->productMock, $duplicateMock); From 89b0a9d4edd2380672ee813d812fe8fbf715eb61 Mon Sep 17 00:00:00 2001 From: Nazar Klovanych Date: Thu, 6 Feb 2020 11:04:29 +0200 Subject: [PATCH 2/3] refactoring, fix unit test --- .../Test/Unit/Model/Product/CopierTest.php | 73 ++++++++++--------- 1 file changed, 40 insertions(+), 33 deletions(-) diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php index 34fddc06f1c42..a1cbc59e56300 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php @@ -5,10 +5,18 @@ */ namespace Magento\Catalog\Test\Unit\Model\Product; +use Magento\Catalog\Api\Data\ProductExtension; use Magento\Catalog\Api\Data\ProductInterface; use Magento\Catalog\Model\Attribute\ScopeOverriddenValue; use Magento\Catalog\Model\Product; use Magento\Catalog\Model\Product\Copier; +use Magento\Catalog\Model\Product\CopyConstructorInterface; +use Magento\Catalog\Model\Product\Option\Repository; +use Magento\Catalog\Model\ProductFactory; +use Magento\CatalogInventory\Api\Data\StockItemInterface; +use Magento\Framework\EntityManager\EntityMetadata; +use Magento\Framework\EntityManager\MetadataPool; +use PHPUnit\Framework\MockObject\MockObject; /** * Test for Magento\Catalog\Model\Product\Copier class. @@ -18,62 +26,60 @@ class CopierTest extends \PHPUnit\Framework\TestCase { /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var MockObject */ - protected $optionRepositoryMock; + private $optionRepositoryMock; /** * @var Copier */ - protected $_model; + private $_model; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var MockObject */ - protected $copyConstructorMock; + private $copyConstructorMock; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var MockObject */ - protected $productFactoryMock; + private $productFactoryMock; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var MockObject */ - protected $productMock; + private $productMock; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var MockObject */ - protected $metadata; + private $metadata; /** - * @var ScopeOverriddenValue|\PHPUnit_Framework_MockObject_MockObject + * @var ScopeOverriddenValue|MockObject */ private $scopeOverriddenValue; + /** + * @ingeritdoc + */ protected function setUp() { - $this->copyConstructorMock = $this->createMock(\Magento\Catalog\Model\Product\CopyConstructorInterface::class); - $this->productFactoryMock = $this->createPartialMock( - \Magento\Catalog\Model\ProductFactory::class, - ['create'] - ); - $this->optionRepositoryMock = $this->createMock( - \Magento\Catalog\Model\Product\Option\Repository::class - ); - $this->optionRepositoryMock; + $this->metadata = $this->createMock(EntityMetadata::class); + $metadataPool = $this->createMock(MetadataPool::class); + + $this->copyConstructorMock = $this->createMock(CopyConstructorInterface::class); + $this->productFactoryMock = $this->createPartialMock(ProductFactory::class, ['create']); + $this->optionRepositoryMock = $this->createMock(Repository::class); $this->productMock = $this->createMock(Product::class); - $this->productMock->expects($this->any())->method('getEntityId')->willReturn(1); $this->scopeOverriddenValue = $this->createMock(ScopeOverriddenValue::class); - $this->metadata = $this->getMockBuilder(\Magento\Framework\EntityManager\EntityMetadata::class) - ->disableOriginalConstructor() - ->getMock(); - $metadataPool = $this->getMockBuilder(\Magento\Framework\EntityManager\MetadataPool::class) - ->disableOriginalConstructor() - ->getMock(); - $metadataPool->expects($this->any())->method('getMetadata')->willReturn($this->metadata); + $this->productMock->expects($this->any()) + ->method('getEntityId') + ->willReturn(1); + $metadataPool->expects($this->any()) + ->method('getMetadata') + ->willReturn($this->metadata); $this->_model = new Copier( $this->copyConstructorMock, @@ -95,9 +101,8 @@ protected function setUp() */ public function testCopy() { - $stockItem = $this->getMockBuilder(\Magento\CatalogInventory\Api\Data\StockItemInterface::class) - ->getMock(); - $extensionAttributes = $this->getMockBuilder(\Magento\Catalog\Api\Data\ProductExtension::class) + $stockItem = $this->createMock(StockItemInterface::class); + $extensionAttributes = $this->getMockBuilder(ProductExtension::class) ->setMethods(['getStockItem', 'setData']) ->getMock(); $extensionAttributes @@ -179,6 +184,9 @@ public function testCopy() 'setUrlKey', 'setStoreId', 'getStoreIds', + 'setMetaTitle', + 'setMetaKeyword', + 'setMetaDescription' ] ); $this->productFactoryMock->expects($this->once())->method('create')->will($this->returnValue($duplicateMock)); @@ -216,7 +224,6 @@ public function testCopy() $this->optionRepositoryMock->expects($this->once()) ->method('duplicate') ->with($this->productMock, $duplicateMock); - $resourceMock->expects($this->once())->method('duplicate')->with(1, 2); $this->assertEquals($duplicateMock, $this->_model->copy($this->productMock)); } From 2d19477ebf398f066930ac413e6e4ebec6ce1830 Mon Sep 17 00:00:00 2001 From: "vadim.malesh" Date: Tue, 11 Feb 2020 17:00:37 +0200 Subject: [PATCH 3/3] fix static, fix conflict merge --- .../Magento/Catalog/Model/Product/Copier.php | 25 ++- .../Test/Unit/Model/Product/CopierTest.php | 174 +++++++++++------- 2 files changed, 118 insertions(+), 81 deletions(-) diff --git a/app/code/Magento/Catalog/Model/Product/Copier.php b/app/code/Magento/Catalog/Model/Product/Copier.php index 645977a35af1f..b04d3da8f0223 100644 --- a/app/code/Magento/Catalog/Model/Product/Copier.php +++ b/app/code/Magento/Catalog/Model/Product/Copier.php @@ -8,10 +8,12 @@ use Magento\Catalog\Api\Data\ProductInterface; use Magento\Catalog\Model\Attribute\ScopeOverriddenValue; use Magento\Catalog\Model\Product; +use Magento\Catalog\Model\Product\Attribute\Source\Status; use Magento\Catalog\Model\Product\Option\Repository as OptionRepository; use Magento\Catalog\Model\ProductFactory; use Magento\Framework\App\ObjectManager; use Magento\Framework\EntityManager\MetadataPool; +use Magento\Store\Model\Store; use Magento\UrlRewrite\Model\Exception\UrlAlreadyExistsException; /** @@ -72,20 +74,17 @@ public function __construct( /** * Create product duplicate * - * @param \Magento\Catalog\Model\Product $product - * - * @return \Magento\Catalog\Model\Product - * - * @throws \Exception + * @param Product $product + * @return Product */ - public function copy(Product $product) + public function copy(Product $product): Product { $product->getWebsiteIds(); $product->getCategoryIds(); $metadata = $this->metadataPool->getMetadata(ProductInterface::class); - /** @var \Magento\Catalog\Model\Product $duplicate */ + /** @var Product $duplicate */ $duplicate = $this->productFactory->create(); $productData = $product->getData(); $productData = $this->removeStockItem($productData); @@ -96,11 +95,11 @@ public function copy(Product $product) $duplicate->setMetaDescription(null); $duplicate->setIsDuplicate(true); $duplicate->setOriginalLinkId($product->getData($metadata->getLinkField())); - $duplicate->setStatus(\Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_DISABLED); + $duplicate->setStatus(Status::STATUS_DISABLED); $duplicate->setCreatedAt(null); $duplicate->setUpdatedAt(null); $duplicate->setId(null); - $duplicate->setStoreId(\Magento\Store\Model\Store::DEFAULT_STORE_ID); + $duplicate->setStoreId(Store::DEFAULT_STORE_ID); $this->copyConstructor->build($product, $duplicate); $this->setDefaultUrl($product, $duplicate); $this->setStoresUrl($product, $duplicate); @@ -118,11 +117,11 @@ public function copy(Product $product) */ private function setDefaultUrl(Product $product, Product $duplicate) : void { - $duplicate->setStoreId(\Magento\Store\Model\Store::DEFAULT_STORE_ID); + $duplicate->setStoreId(Store::DEFAULT_STORE_ID); $resource = $product->getResource(); $attribute = $resource->getAttribute('url_key'); $productId = $product->getId(); - $urlKey = $resource->getAttributeRawValue($productId, 'url_key', \Magento\Store\Model\Store::DEFAULT_STORE_ID); + $urlKey = $resource->getAttributeRawValue($productId, 'url_key', Store::DEFAULT_STORE_ID); do { $urlKey = $this->modifyUrl($urlKey); $duplicate->setUrlKey($urlKey); @@ -175,7 +174,7 @@ private function setStoresUrl(Product $product, Product $duplicate) : void $productResource->saveAttribute($duplicate, 'url_path'); $productResource->saveAttribute($duplicate, 'url_key'); } - $duplicate->setStoreId(\Magento\Store\Model\Store::DEFAULT_STORE_ID); + $duplicate->setStoreId(Store::DEFAULT_STORE_ID); } /** @@ -197,7 +196,7 @@ private function modifyUrl(string $urlKey) : string * @param array $productData * @return array */ - private function removeStockItem(array $productData) + private function removeStockItem(array $productData): array { if (isset($productData[ProductInterface::EXTENSION_ATTRIBUTES_KEY])) { $extensionAttributes = $productData[ProductInterface::EXTENSION_ATTRIBUTES_KEY]; diff --git a/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php b/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php index 18179489fbd94..91441890e83b1 100644 --- a/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php +++ b/app/code/Magento/Catalog/Test/Unit/Model/Product/CopierTest.php @@ -3,53 +3,66 @@ * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ + namespace Magento\Catalog\Test\Unit\Model\Product; use Magento\Catalog\Api\Data\ProductExtension; use Magento\Catalog\Api\Data\ProductInterface; +use Magento\Catalog\Model\Attribute\ScopeOverriddenValue; use Magento\Catalog\Model\Product; +use Magento\Catalog\Model\Product\Attribute\Source\Status; use Magento\Catalog\Model\Product\Copier; +use Magento\Catalog\Model\Product\CopyConstructorInterface; +use Magento\Catalog\Model\Product\Option\Repository; +use Magento\Catalog\Model\ProductFactory; +use Magento\Catalog\Model\ResourceModel\Product as ProductResourceModel; use Magento\CatalogInventory\Api\Data\StockItemInterface; +use Magento\Eav\Model\Entity\AbstractEntity; +use Magento\Eav\Model\Entity\Attribute\AbstractAttribute; +use Magento\Framework\EntityManager\EntityMetadata; +use Magento\Framework\EntityManager\MetadataPool; +use PHPUnit\Framework\MockObject\MockObject; +use PHPUnit\Framework\TestCase; /** * Test for Magento\Catalog\Model\Product\Copier class. * * @SuppressWarnings(PHPMD.CouplingBetweenObjects) */ -class CopierTest extends \PHPUnit\Framework\TestCase +class CopierTest extends TestCase { /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var Copier */ - private $optionRepositoryMock; + private $_model; /** - * @var Copier + * @var Repository|MockObject */ - private $_model; + private $optionRepositoryMock; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var CopyConstructorInterface|MockObject */ private $copyConstructorMock; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var ProductFactory|MockObject */ private $productFactoryMock; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var ScopeOverriddenValue|MockObject */ private $scopeOverriddenValueMock; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var Product|MockObject */ private $productMock; /** - * @var \PHPUnit_Framework_MockObject_MockObject + * @var EntityMetadata|MockObject */ private $metadata; @@ -58,27 +71,23 @@ class CopierTest extends \PHPUnit\Framework\TestCase */ protected function setUp() { - $this->copyConstructorMock = $this->createMock(\Magento\Catalog\Model\Product\CopyConstructorInterface::class); - $this->productFactoryMock = $this->createPartialMock( - \Magento\Catalog\Model\ProductFactory::class, - ['create'] - ); - $this->scopeOverriddenValueMock = $this->createMock( - \Magento\Catalog\Model\Attribute\ScopeOverriddenValue::class - ); - $this->optionRepositoryMock = $this->createMock( - \Magento\Catalog\Model\Product\Option\Repository::class - ); + $this->copyConstructorMock = $this->createMock(CopyConstructorInterface::class); + $this->productFactoryMock = $this->createPartialMock(ProductFactory::class, ['create']); + $this->scopeOverriddenValueMock = $this->createMock(ScopeOverriddenValue::class); + $this->optionRepositoryMock = $this->createMock(Repository::class); $this->productMock = $this->createMock(Product::class); - $this->productMock->expects($this->any())->method('getEntityId')->willReturn(1); - $this->metadata = $this->getMockBuilder(\Magento\Framework\EntityManager\EntityMetadata::class) + $this->metadata = $this->getMockBuilder(EntityMetadata::class) ->disableOriginalConstructor() ->getMock(); - $metadataPool = $this->getMockBuilder(\Magento\Framework\EntityManager\MetadataPool::class) + + /** @var MetadataPool|MockObject $metadataPool */ + $metadataPool = $this->getMockBuilder(MetadataPool::class) ->disableOriginalConstructor() ->getMock(); - $metadataPool->expects($this->any())->method('getMetadata')->willReturn($this->metadata); + $metadataPool->expects($this->once()) + ->method('getMetadata') + ->willReturn($this->metadata); $this->_model = new Copier( $this->copyConstructorMock, $this->productFactoryMock, @@ -89,9 +98,12 @@ protected function setUp() } /** + * Test duplicate product + * + * @return void * @SuppressWarnings(PHPMD.ExcessiveMethodLength) */ - public function testCopy() + public function testCopy(): void { $stockItem = $this->createMock(StockItemInterface::class); $extensionAttributes = $this->getMockBuilder(ProductExtension::class) @@ -110,15 +122,19 @@ public function testCopy() 'product data' => ['product data'], ProductInterface::EXTENSION_ATTRIBUTES_KEY => $extensionAttributes, ]; - $this->productMock->expects($this->atLeastOnce())->method('getWebsiteIds'); - $this->productMock->expects($this->atLeastOnce())->method('getCategoryIds'); - $this->productMock->expects($this->any())->method('getData')->willReturnMap([ - ['', null, $productData], - ['linkField', null, '1'], - ]); + $this->productMock->expects($this->atLeastOnce()) + ->method('getWebsiteIds'); + $this->productMock->expects($this->atLeastOnce()) + ->method('getCategoryIds'); + $this->productMock->expects($this->exactly(2)) + ->method('getData') + ->willReturnMap([ + ['', null, $productData], + ['linkField', null, '1'], + ]); $entityMock = $this->getMockForAbstractClass( - \Magento\Eav\Model\Entity\AbstractEntity::class, + AbstractEntity::class, [], '', false, @@ -126,12 +142,12 @@ public function testCopy() true, ['checkAttributeUniqueValue'] ); - $entityMock->expects($this->any()) + $entityMock->expects($this->once()) ->method('checkAttributeUniqueValue') ->willReturn(true); $attributeMock = $this->getMockForAbstractClass( - \Magento\Eav\Model\Entity\Attribute\AbstractAttribute::class, + AbstractAttribute::class, [], '', false, @@ -139,22 +155,24 @@ public function testCopy() true, ['getEntity'] ); - $attributeMock->expects($this->any()) + $attributeMock->expects($this->once()) ->method('getEntity') ->willReturn($entityMock); - $resourceMock = $this->getMockBuilder(\Magento\Catalog\Model\ResourceModel\Product::class) + $resourceMock = $this->getMockBuilder(ProductResourceModel::class) ->disableOriginalConstructor() ->setMethods(['getAttributeRawValue', 'duplicate', 'getAttribute']) ->getMock(); - $resourceMock->expects($this->any()) + $resourceMock->expects($this->once()) ->method('getAttributeRawValue') ->willReturn('urk-key-1'); - $resourceMock->expects($this->any()) + $resourceMock->expects($this->exactly(2)) ->method('getAttribute') ->willReturn($attributeMock); - $this->productMock->expects($this->any())->method('getResource')->will($this->returnValue($resourceMock)); + $this->productMock->expects($this->exactly(2)) + ->method('getResource') + ->willReturn($resourceMock); $duplicateMock = $this->createPartialMock( Product::class, @@ -176,51 +194,71 @@ public function testCopy() 'getStoreIds', 'setMetaTitle', 'setMetaKeyword', - 'setMetaDescription' + 'setMetaDescription', ] ); - $this->productFactoryMock->expects($this->once())->method('create')->will($this->returnValue($duplicateMock)); + $this->productFactoryMock->expects($this->once()) + ->method('create') + ->willReturn($duplicateMock); $duplicateMock->expects($this->once())->method('setOptions')->with([]); $duplicateMock->expects($this->once())->method('setIsDuplicate')->with(true); $duplicateMock->expects($this->once())->method('setOriginalLinkId')->with(1); - $duplicateMock->expects( - $this->once() - )->method( - 'setStatus' - )->with( - \Magento\Catalog\Model\Product\Attribute\Source\Status::STATUS_DISABLED - ); + $duplicateMock->expects($this->once()) + ->method('setStatus') + ->with(Status::STATUS_DISABLED); $duplicateMock->expects($this->atLeastOnce())->method('setStoreId'); - $duplicateMock->expects($this->once())->method('setCreatedAt')->with(null); - $duplicateMock->expects($this->once())->method('setUpdatedAt')->with(null); - $duplicateMock->expects($this->once())->method('setId')->with(null); - $duplicateMock->expects($this->once())->method('setMetaTitle')->with(null); - $duplicateMock->expects($this->once())->method('setMetaKeyword')->with(null); - $duplicateMock->expects($this->once())->method('setMetaDescription')->with(null); - $duplicateMock->expects($this->atLeastOnce())->method('getStoreIds')->willReturn([]); - $duplicateMock->expects($this->atLeastOnce())->method('setData')->willReturn($duplicateMock); - $this->copyConstructorMock->expects($this->once())->method('build')->with($this->productMock, $duplicateMock); - $duplicateMock->expects($this->once())->method('setUrlKey')->with('urk-key-2')->willReturn($duplicateMock); - $duplicateMock->expects($this->once())->method('save'); - - $this->metadata->expects($this->any())->method('getLinkField')->willReturn('linkField'); - - $duplicateMock->expects($this->any())->method('getData')->willReturnMap([ - ['linkField', null, '2'], - ]); + $duplicateMock->expects($this->once()) + ->method('setCreatedAt') + ->with(null); + $duplicateMock->expects($this->once()) + ->method('setUpdatedAt') + ->with(null); + $duplicateMock->expects($this->once()) + ->method('setId') + ->with(null); + $duplicateMock->expects($this->once()) + ->method('setMetaTitle') + ->with(null); + $duplicateMock->expects($this->once()) + ->method('setMetaKeyword') + ->with(null); + $duplicateMock->expects($this->once()) + ->method('setMetaDescription') + ->with(null); + $duplicateMock->expects($this->atLeastOnce()) + ->method('getStoreIds')->willReturn([]); + $duplicateMock->expects($this->atLeastOnce()) + ->method('setData') + ->willReturn($duplicateMock); + $this->copyConstructorMock->expects($this->once()) + ->method('build') + ->with($this->productMock, $duplicateMock); + $duplicateMock->expects($this->once()) + ->method('setUrlKey') + ->with('urk-key-2') + ->willReturn($duplicateMock); + $duplicateMock->expects($this->once()) + ->method('save'); + $this->metadata->expects($this->once()) + ->method('getLinkField') + ->willReturn('linkField'); + $duplicateMock->expects($this->never()) + ->method('getData'); $this->optionRepositoryMock->expects($this->once()) ->method('duplicate') ->with($this->productMock, $duplicateMock); - $resourceMock->expects($this->once())->method('duplicate')->with(1, 2); $this->assertEquals($duplicateMock, $this->_model->copy($this->productMock)); } /** + * Test duplicate product with `UrlAlreadyExistsException` while copy stores url + * + * @return void * @SuppressWarnings(PHPMD.ExcessiveMethodLength) */ - public function testUrlAlreadyExistsExceptionWhileCopyStoresUrl() + public function testUrlAlreadyExistsExceptionWhileCopyStoresUrl(): void { $stockItem = $this->getMockBuilder(StockItemInterface::class) ->getMock();