String validation for filename added and updated unit test case #13937.

magento · Jan 4, 2019 · 55cf2dc · 55cf2dc
1 parent cf69967
commit 55cf2dc
Show file tree

Hide file tree

Showing 3 changed files with 211 additions and 55 deletions.
diff --git 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
@@ -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
@@ -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')