From 55cf2dcb970f3f069e3d59ad7986413f80bb6cdb Mon Sep 17 00:00:00 2001 From: Rajneesh Gupta Date: Wed, 2 Jan 2019 18:25:40 +0530 Subject: [PATCH] String validation for filename added and updated unit test case magento#13937. --- .../Sitemap/Block/Adminhtml/Edit/Form.php | 3 +- .../Controller/Adminhtml/Sitemap/Save.php | 90 +++++++-- .../Controller/Adminhtml/Sitemap/SaveTest.php | 173 ++++++++++++++---- 3 files changed, 211 insertions(+), 55 deletions(-) diff --git a/app/code/Magento/Sitemap/Block/Adminhtml/Edit/Form.php b/app/code/Magento/Sitemap/Block/Adminhtml/Edit/Form.php index 5e90cf6e12f6e..2093e4d7d5736 100644 --- a/app/code/Magento/Sitemap/Block/Adminhtml/Edit/Form.php +++ b/app/code/Magento/Sitemap/Block/Adminhtml/Edit/Form.php @@ -73,7 +73,8 @@ protected function _prepareForm() 'name' => 'sitemap_filename', 'required' => true, 'note' => __('example: sitemap.xml'), - 'value' => $model->getSitemapFilename() + 'value' => $model->getSitemapFilename(), + 'class' => 'validate-length maximum-length-32' ] ); diff --git a/app/code/Magento/Sitemap/Controller/Adminhtml/Sitemap/Save.php b/app/code/Magento/Sitemap/Controller/Adminhtml/Sitemap/Save.php index 1e0d1cb248f00..23e1d2e786900 100644 --- a/app/code/Magento/Sitemap/Controller/Adminhtml/Sitemap/Save.php +++ b/app/code/Magento/Sitemap/Controller/Adminhtml/Sitemap/Save.php @@ -11,6 +11,61 @@ class Save extends \Magento\Sitemap\Controller\Adminhtml\Sitemap { + /** + * Maximum length of sitemap filename + */ + const MAX_FILENAME_LENGTH = 32; + + /** + * @var $_stringValidator + */ + public $_stringValidator; + + /** + * @var $_pathValidator + */ + public $_pathValidator; + + /** + * @var $_sitemapHelper + */ + public $_sitemapHelper; + + /** + * @var $_filesystem + */ + public $_filesystem; + + /** + * @var $_sitemapFactory + */ + public $_sitemapFactory; + + /** + * Save constructor. + * @param Action\Context $context + * @param \Magento\Framework\Validator\StringLength $stringValidator + * @param \Magento\MediaStorage\Model\File\Validator\AvailablePath $pathValidator + * @param \Magento\Sitemap\Helper\Data $sitemapHelper + * @param \Magento\Framework\Filesystem $filesystem + * @param \Magento\Sitemap\Model\SitemapFactory $sitemapFactory + */ + public function __construct( + \Magento\Backend\App\Action\Context $context, + \Magento\Framework\Validator\StringLength $stringValidator, + \Magento\MediaStorage\Model\File\Validator\AvailablePath $pathValidator, + \Magento\Sitemap\Helper\Data $sitemapHelper, + \Magento\Framework\Filesystem $filesystem, + \Magento\Sitemap\Model\SitemapFactory $sitemapFactory + ) { + parent::__construct($context); + $this->_stringValidator = $stringValidator; + $this->_pathValidator = $pathValidator; + $this->_sitemapHelper = $sitemapHelper; + $this->_filesystem = $filesystem; + $this->_sitemapFactory = $sitemapFactory; + } + /** * Validate path for generation * @@ -23,17 +78,25 @@ protected function validatePath(array $data) if (!empty($data['sitemap_filename']) && !empty($data['sitemap_path'])) { $data['sitemap_path'] = '/' . ltrim($data['sitemap_path'], '/'); $path = rtrim($data['sitemap_path'], '\\/') . '/' . $data['sitemap_filename']; - /** @var $validator \Magento\MediaStorage\Model\File\Validator\AvailablePath */ - $validator = $this->_objectManager->create(\Magento\MediaStorage\Model\File\Validator\AvailablePath::class); - /** @var $helper \Magento\Sitemap\Helper\Data */ - $helper = $this->_objectManager->get(\Magento\Sitemap\Helper\Data::class); - $validator->setPaths($helper->getValidPaths()); - if (!$validator->isValid($path)) { - foreach ($validator->getMessages() as $message) { + $this->_pathValidator->setPaths($this->_sitemapHelper->getValidPaths()); + if (!$this->_pathValidator->isValid($path)) { + foreach ($this->_pathValidator->getMessages() as $message) { + $this->messageManager->addErrorMessage($message); + } + // save data in session + $this->_session->setFormData($data); + // redirect to edit form + return false; + } + + $filename = rtrim($data['sitemap_filename']); + $this->_stringValidator->setMax(self::MAX_FILENAME_LENGTH); + if (!$this->_stringValidator->isValid($filename)) { + foreach ($this->_stringValidator->getMessages() as $message) { $this->messageManager->addErrorMessage($message); } // save data in session - $this->_objectManager->get(\Magento\Backend\Model\Session::class)->setFormData($data); + $this->_session->setFormData($data); // redirect to edit form return false; } @@ -49,9 +112,8 @@ protected function validatePath(array $data) */ protected function clearSiteMap(\Magento\Sitemap\Model\Sitemap $model) { - /** @var \Magento\Framework\Filesystem\Directory\Write $directory */ - $directory = $this->_objectManager->get(\Magento\Framework\Filesystem::class) - ->getDirectoryWrite(DirectoryList::ROOT); + /** @var \Magento\Framework\Filesystem $directory */ + $directory = $this->_filesystem->getDirectoryWrite(DirectoryList::ROOT); if ($this->getRequest()->getParam('sitemap_id')) { $model->load($this->getRequest()->getParam('sitemap_id')); @@ -74,7 +136,7 @@ protected function saveData($data) { // init model and set data /** @var \Magento\Sitemap\Model\Sitemap $model */ - $model = $this->_objectManager->create(\Magento\Sitemap\Model\Sitemap::class); + $model = $this->_sitemapFactory->create(); $this->clearSiteMap($model); $model->setData($data); @@ -85,13 +147,13 @@ protected function saveData($data) // display success message $this->messageManager->addSuccessMessage(__('You saved the sitemap.')); // clear previously saved data from session - $this->_objectManager->get(\Magento\Backend\Model\Session::class)->setFormData(false); + $this->_session->setFormData(false); return $model->getId(); } catch (\Exception $e) { // display error message $this->messageManager->addErrorMessage($e->getMessage()); // save data in session - $this->_objectManager->get(\Magento\Backend\Model\Session::class)->setFormData($data); + $this->_session->setFormData($data); } return false; } diff --git a/app/code/Magento/Sitemap/Test/Unit/Controller/Adminhtml/Sitemap/SaveTest.php b/app/code/Magento/Sitemap/Test/Unit/Controller/Adminhtml/Sitemap/SaveTest.php index f77954101df7c..0e5758a3d8f48 100644 --- a/app/code/Magento/Sitemap/Test/Unit/Controller/Adminhtml/Sitemap/SaveTest.php +++ b/app/code/Magento/Sitemap/Test/Unit/Controller/Adminhtml/Sitemap/SaveTest.php @@ -7,6 +7,7 @@ use Magento\Framework\TestFramework\Unit\Helper\ObjectManager as ObjectManagerHelper; use Magento\Framework\Controller\ResultFactory; +use Magento\Sitemap\Controller\Adminhtml\Sitemap\Save; class SaveTest extends \PHPUnit\Framework\TestCase { @@ -18,12 +19,7 @@ class SaveTest extends \PHPUnit\Framework\TestCase /** * @var \Magento\Backend\App\Action\Context */ - protected $context; - - /** - * @var \Magento\Framework\TestFramework\Unit\Helper\ObjectManager; - */ - protected $objectManagerHelper; + protected $contextMock; /** * @var \Magento\Framework\HTTP\PhpEnvironment\Request|\PHPUnit_Framework_MockObject_MockObject @@ -50,8 +46,41 @@ class SaveTest extends \PHPUnit\Framework\TestCase */ protected $messageManagerMock; + /** + * @var \Magento\Framework\Validator\StringLength|\PHPUnit_Framework_MockObject_MockObject + */ + protected $lengthValidator; + + /** + * @var \Magento\MediaStorage\Model\File\Validator\AvailablePath|\PHPUnit_Framework_MockObject_MockObject + */ + protected $pathValidator; + + /** + * @var \Magento\Sitemap\Helper\Data|\PHPUnit_Framework_MockObject_MockObject + */ + protected $helper; + + /** + * @var \Magento\Framework\Filesystem|\PHPUnit_Framework_MockObject_MockObject + */ + protected $fileSystem; + + /** + * @var \Magento\Sitemap\Model\SitemapFactory|\PHPUnit_Framework_MockObject_MockObject + */ + protected $siteMapFactory; + + /** + * @var \Magento\Backend\Model\Session|\PHPUnit_Framework_MockObject_MockObject + */ + protected $session; + protected function setUp() { + $this->contextMock = $this->getMockBuilder(\Magento\Backend\App\Action\Context::class) + ->disableOriginalConstructor() + ->getMock(); $this->requestMock = $this->getMockBuilder(\Magento\Framework\App\RequestInterface::class) ->disableOriginalConstructor() ->setMethods(['getPostValue']) @@ -66,27 +95,48 @@ protected function setUp() ->getMock(); $this->messageManagerMock = $this->getMockBuilder(\Magento\Framework\Message\ManagerInterface::class) ->getMock(); - + $this->helper = $this->getMockBuilder(\Magento\Sitemap\Helper\Data::class) + ->disableOriginalConstructor() + ->getMock(); $this->resultFactoryMock->expects($this->once()) ->method('create') ->with(ResultFactory::TYPE_REDIRECT) ->willReturn($this->resultRedirectMock); + $this->session = $this->getMockBuilder(\Magento\Backend\Model\Session::class) + ->disableOriginalConstructor() + ->setMethods(['setFormData']) + ->getMock(); - $this->objectManagerHelper = new ObjectManagerHelper($this); - $this->context = $this->objectManagerHelper->getObject( - \Magento\Backend\App\Action\Context::class, - [ - 'resultFactory' => $this->resultFactoryMock, - 'request' => $this->requestMock, - 'messageManager' => $this->messageManagerMock, - 'objectManager' => $this->objectManagerMock - ] - ); - $this->saveController = $this->objectManagerHelper->getObject( - \Magento\Sitemap\Controller\Adminhtml\Sitemap\Save::class, - [ - 'context' => $this->context - ] + $this->contextMock->expects($this->once()) + ->method('getMessageManager') + ->willReturn($this->messageManagerMock); + $this->contextMock->expects($this->once()) + ->method('getRequest') + ->willReturn($this->requestMock); + $this->contextMock->expects($this->once()) + ->method('getResultFactory') + ->willReturn($this->resultFactoryMock); + $this->contextMock->expects($this->once()) + ->method('getSession') + ->willReturn($this->session); + + $this->lengthValidator = $this->getMockBuilder(\Magento\Framework\Validator\StringLength::class) + ->disableOriginalConstructor() + ->getMock(); + $this->pathValidator = + $this->getMockBuilder(\Magento\MediaStorage\Model\File\Validator\AvailablePath::class) + ->disableOriginalConstructor() + ->getMock(); + $this->fileSystem = $this->createMock(\Magento\Framework\Filesystem::class); + $this->siteMapFactory = $this->createMock(\Magento\Sitemap\Model\SitemapFactory::class); + + $this->saveController = new Save( + $this->contextMock, + $this->lengthValidator, + $this->pathValidator, + $this->helper, + $this->fileSystem, + $this->siteMapFactory ); } @@ -105,11 +155,8 @@ public function testSaveEmptyDataShouldRedirectToDefault() public function testTryToSaveInvalidDataShouldFailWithErrors() { - $validatorClass = \Magento\MediaStorage\Model\File\Validator\AvailablePath::class; - $helperClass = \Magento\Sitemap\Helper\Data::class; $validPaths = []; $messages = ['message1', 'message2']; - $sessionClass = \Magento\Backend\Model\Session::class; $data = ['sitemap_filename' => 'sitemap_filename', 'sitemap_path' => '/sitemap_path']; $siteMapId = 1; @@ -121,37 +168,83 @@ public function testTryToSaveInvalidDataShouldFailWithErrors() ->with('sitemap_id') ->willReturn($siteMapId); - $validator = $this->createMock($validatorClass); - $validator->expects($this->once()) + $this->pathValidator->expects($this->once()) ->method('setPaths') ->with($validPaths) ->willReturnSelf(); - $validator->expects($this->once()) + $this->pathValidator->expects($this->once()) ->method('isValid') ->with('/sitemap_path/sitemap_filename') ->willReturn(false); - $validator->expects($this->once()) + $this->pathValidator->expects($this->once()) ->method('getMessages') ->willReturn($messages); - $helper = $this->createMock($helperClass); - $helper->expects($this->once()) + $this->helper->expects($this->once()) ->method('getValidPaths') ->willReturn($validPaths); - $session = $this->createPartialMock($sessionClass, ['setFormData']); - $session->expects($this->once()) + $this->session->expects($this->once()) ->method('setFormData') ->with($data) ->willReturnSelf(); - $this->objectManagerMock->expects($this->once()) - ->method('create') - ->with($validatorClass) - ->willReturn($validator); - $this->objectManagerMock->expects($this->any()) - ->method('get') - ->willReturnMap([[$helperClass, $helper], [$sessionClass, $session]]); + $this->messageManagerMock->expects($this->at(0)) + ->method('addErrorMessage') + ->withConsecutive( + [$messages[0]], + [$messages[1]] + ) + ->willReturnSelf(); + + $this->resultRedirectMock->expects($this->once()) + ->method('setPath') + ->with('adminhtml/*/edit', ['sitemap_id' => $siteMapId]) + ->willReturnSelf(); + + $this->assertSame($this->resultRedirectMock, $this->saveController->execute()); + } + + public function testTryToSaveInvalidFileNameShouldFailWithErrors() + { + $validPaths = []; + $messages = ['message1', 'message2']; + $data = ['sitemap_filename' => 'sitemap_filename', 'sitemap_path' => '/sitemap_path']; + $siteMapId = 1; + + $this->requestMock->expects($this->once()) + ->method('getPostValue') + ->willReturn($data); + $this->requestMock->expects($this->once()) + ->method('getParam') + ->with('sitemap_id') + ->willReturn($siteMapId); + + $this->lengthValidator->expects($this->once()) + ->method('isValid') + ->with('sitemap_filename') + ->willReturn(false); + $this->lengthValidator->expects($this->once()) + ->method('getMessages') + ->willReturn($messages); + + $this->pathValidator->expects($this->once()) + ->method('setPaths') + ->with($validPaths) + ->willReturnSelf(); + $this->pathValidator->expects($this->once()) + ->method('isValid') + ->with('/sitemap_path/sitemap_filename') + ->willReturn(true); + + $this->helper->expects($this->once()) + ->method('getValidPaths') + ->willReturn($validPaths); + + $this->session->expects($this->once()) + ->method('setFormData') + ->with($data) + ->willReturnSelf(); $this->messageManagerMock->expects($this->at(0)) ->method('addErrorMessage')