diff --git a/app/code/Magento/Review/Block/Adminhtml/Grid.php b/app/code/Magento/Review/Block/Adminhtml/Grid.php index 61620281fd3f3..556c6f05b54e1 100644 --- a/app/code/Magento/Review/Block/Adminhtml/Grid.php +++ b/app/code/Magento/Review/Block/Adminhtml/Grid.php @@ -7,6 +7,8 @@ namespace Magento\Review\Block\Adminhtml; +use Magento\Review\Helper\Action\Pager; + /** * Adminhtml reviews grid * @@ -19,21 +21,21 @@ class Grid extends \Magento\Backend\Block\Widget\Grid\Extended { /** - * Review action pager + * Magento Review action pager * * @var \Magento\Review\Helper\Action\Pager */ protected $_reviewActionPager = null; /** - * Review data + * Review helper data * * @var \Magento\Review\Helper\Data */ protected $_reviewData = null; /** - * Core registry + * Magento framework Core registry * * @var \Magento\Framework\Registry */ @@ -94,16 +96,17 @@ protected function _construct() } /** - * Save search results + * Executes after the collection is loaded. This method is intentionally overridden to preserve compatibility * * @return \Magento\Backend\Block\Widget\Grid + * @deprecated This method is no longer in used.We use it only to support compatibility + * @see Pager::getRelativeReviewId() */ protected function _afterLoadCollection() { /** @var $actionPager \Magento\Review\Helper\Action\Pager */ $actionPager = $this->_reviewActionPager; $actionPager->setStorageId('reviews'); - $actionPager->setItems($this->getCollection()->getResultingIds()); return parent::_afterLoadCollection(); } diff --git a/app/code/Magento/Review/Helper/Action/Pager.php b/app/code/Magento/Review/Helper/Action/Pager.php index 7d371969be0ec..eec1e5d368923 100644 --- a/app/code/Magento/Review/Helper/Action/Pager.php +++ b/app/code/Magento/Review/Helper/Action/Pager.php @@ -6,20 +6,23 @@ namespace Magento\Review\Helper\Action; +use Magento\Framework\App\ObjectManager; use Magento\Framework\Exception\LocalizedException; +use Magento\Review\Model\ResourceModel\Review\CollectionFactory; /** * Action pager helper for iterating over search results * * @api * @since 100.0.2 + * @SuppressWarnings(PHPMD.CookieAndSessionMisuse) */ class Pager extends \Magento\Framework\App\Helper\AbstractHelper { - const STORAGE_PREFIX = 'search_result_ids'; + public const STORAGE_PREFIX = 'search_result_ids'; /** - * Storage id + * Key identifier for session storage id * * @var int */ @@ -39,15 +42,26 @@ class Pager extends \Magento\Framework\App\Helper\AbstractHelper */ protected $_backendSession; + /** + * Review collection model factory + * + * @var CollectionFactory|null + */ + protected $reviewCollectionFactory = null; + /** * @param \Magento\Framework\App\Helper\Context $context * @param \Magento\Backend\Model\Session $backendSession + * @param CollectionFactory|null $reviewCollectionFactory */ public function __construct( \Magento\Framework\App\Helper\Context $context, - \Magento\Backend\Model\Session $backendSession + \Magento\Backend\Model\Session $backendSession, + ?CollectionFactory $reviewCollectionFactory = null ) { $this->_backendSession = $backendSession; + $this->reviewCollectionFactory = $reviewCollectionFactory + ?: ObjectManager::getInstance()->get(CollectionFactory::class); parent::__construct($context); } @@ -56,6 +70,8 @@ public function __construct( * * @param int $storageId * @return void + * @deprecated This method is no longer used for setting storage id.We use it only to support backward compatibility + * @see self::getRelativeReviewId() */ public function setStorageId($storageId) { @@ -67,6 +83,8 @@ public function setStorageId($storageId) * * @param array $items * @return $this + * @deprecated This method is not being used.We use it only to support compatibility + * @see self::getRelativeReviewId() */ public function setItems(array $items) { @@ -80,6 +98,8 @@ public function setItems(array $items) * Load stored items * * @return void + * @deprecated This method is not being used.We use it only to support compatibility + * @see self::getRelativeReviewId() */ protected function _loadItems() { @@ -89,35 +109,25 @@ protected function _loadItems() } /** - * Get next item id + * Get the next review id. * * @param int $id * @return int|bool */ - public function getNextItemId($id) + public function getNextItemId($id): int|bool { - $position = $this->_findItemPositionByValue($id); - if ($position === false || $position == count($this->_items) - 1) { - return false; - } - - return $this->_items[$position + 1]; + return $this->getRelativeReviewId($id, 'gt', 'ASC'); } /** - * Get previous item id + * Get the previous review id. * * @param int $id * @return int|bool */ - public function getPreviousItemId($id) + public function getPreviousItemId($id): int|bool { - $position = $this->_findItemPositionByValue($id); - if ($position === false || $position == 0) { - return false; - } - - return $this->_items[$position - 1]; + return $this->getRelativeReviewId($id, 'lt', 'DESC'); } /** @@ -125,6 +135,8 @@ public function getPreviousItemId($id) * * @param mixed $value * @return int|bool + * @deprecated This method is not being used.We use it only to support compatibility + * @see self::getRelativeReviewId() */ protected function _findItemPositionByValue($value) { @@ -137,6 +149,8 @@ protected function _findItemPositionByValue($value) * * @return string * @throws \Magento\Framework\Exception\LocalizedException + * @deprecated This method is not being used.We use it only to support compatibility + * @see self::getRelativeReviewId() */ protected function _getStorageKey() { @@ -146,4 +160,24 @@ protected function _getStorageKey() return self::STORAGE_PREFIX . $this->_storageId; } + + /** + * Get the review id based on comparison and order. + * + * @param int $id + * @param string $operator + * @param string $order + * @return int|bool + */ + private function getRelativeReviewId($id, $operator, $order): int|bool + { + $collection = $this->reviewCollectionFactory->create(); + $collection->addFieldToFilter('main_table.review_id', [$operator => $id]) + ->setOrder('main_table.review_id', $order) + ->setPageSize(1) + ->setCurPage(1); + + $item = $collection->getFirstItem(); + return $item->getId() ? (int)$item->getId() : false; + } } diff --git a/app/code/Magento/Review/Model/ResourceModel/Review/Product/Collection.php b/app/code/Magento/Review/Model/ResourceModel/Review/Product/Collection.php index 3d4d76bb55477..de31099326e14 100644 --- a/app/code/Magento/Review/Model/ResourceModel/Review/Product/Collection.php +++ b/app/code/Magento/Review/Model/ResourceModel/Review/Product/Collection.php @@ -9,6 +9,7 @@ use Magento\Eav\Model\Entity\Attribute\AbstractAttribute; use Magento\Framework\DB\Select; use Magento\Framework\EntityManager\MetadataPool; +use Magento\Review\Helper\Action\Pager; /** * Review Product Collection @@ -405,6 +406,8 @@ public function getAllIds($limit = null, $offset = null) * Get result sorted ids * * @return array + * @deprecated This method is not being used to fetch ids anymore.We use it only to support backward compatibility + * @see Pager::getRelativeReviewId() */ public function getResultingIds() { diff --git a/app/code/Magento/Review/Test/Unit/Helper/Action/PagerTest.php b/app/code/Magento/Review/Test/Unit/Helper/Action/PagerTest.php index cbd23a7a22c2f..c73a2a07a8baa 100644 --- a/app/code/Magento/Review/Test/Unit/Helper/Action/PagerTest.php +++ b/app/code/Magento/Review/Test/Unit/Helper/Action/PagerTest.php @@ -7,94 +7,173 @@ namespace Magento\Review\Test\Unit\Helper\Action; -use Magento\Backend\Model\Session; use Magento\Framework\App\Helper\Context; +use Magento\Framework\DataObject; use Magento\Review\Helper\Action\Pager; +use Magento\Review\Model\ResourceModel\Review\Collection; +use Magento\Review\Model\ResourceModel\Review\CollectionFactory; +use PHPUnit\Framework\MockObject\MockObject; use PHPUnit\Framework\TestCase; +/** + * Unit test for \Magento\Review\Helper\Action\Pager + */ class PagerTest extends TestCase { /** @var Pager */ - protected $_helper = null; + private $pager; + + /** @var CollectionFactory|MockObject */ + private $collectionFactory; + + /** @var Collection|MockObject */ + private $collection; /** - * Prepare helper object + * Set up test environment + * + * @return void */ protected function setUp(): void { - $sessionMock = $this->getMockBuilder( - Session::class - )->disableOriginalConstructor() - ->addMethods(['setData']) - ->onlyMethods( - ['getData'] - )->getMock(); - $sessionMock->expects( - $this->any() - )->method( - 'setData' - )->with( - 'search_result_idsreviews', - $this->anything() - ); - $sessionMock->expects( - $this->any() - )->method( - 'getData' - )->with( - 'search_result_idsreviews' - )->willReturn( - [3, 2, 6, 5] - ); - - $contextMock = $this->createPartialMock( - Context::class, - ['getModuleManager', 'getRequest'] - ); - $this->_helper = new Pager($contextMock, $sessionMock); - $this->_helper->setStorageId('reviews'); - } + $this->collection = $this->createMock(Collection::class); - /** - * Test storage set with proper parameters - */ - public function testStorageSet() - { - $result = $this->_helper->setItems([1]); - $this->assertEquals($result, $this->_helper); + $this->collectionFactory = $this->createMock(CollectionFactory::class); + $this->collectionFactory->expects($this->any()) + ->method('create') + ->willReturn($this->collection); + + $context = $this->createMock(Context::class); + $backendSession = $this->createMock(\Magento\Backend\Model\Session::class); + + $this->pager = new Pager($context, $backendSession, $this->collectionFactory); } /** - * Test getNextItem + * Test getting next review ID + * + * @return void */ - public function testGetNextItem() + public function testGetNextItemId(): void { - $this->assertEquals(2, $this->_helper->getNextItemId(3)); + $item = new DataObject(['id' => 10]); + + $this->collection->expects($this->once()) + ->method('addFieldToFilter') + ->with('main_table.review_id', ['gt' => 5]) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setOrder') + ->with('main_table.review_id', 'ASC') + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setPageSize') + ->with(1) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setCurPage') + ->with(1) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('getFirstItem') + ->willReturn($item); + + $this->assertEquals(10, $this->pager->getNextItemId(5)); } /** - * Test getNextItem when item not found or no next item + * Test that getNextItemId returns false when no item exists + * + * @return void */ - public function testGetNextItemNotFound() + public function testGetNextItemIdReturnsFalse(): void { - $this->assertFalse($this->_helper->getNextItemId(30)); - $this->assertFalse($this->_helper->getNextItemId(5)); + $item = new DataObject([]); + + $this->collection->expects($this->once()) + ->method('addFieldToFilter') + ->with('main_table.review_id', ['gt' => 99]) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setOrder') + ->with('main_table.review_id', 'ASC') + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setPageSize') + ->with(1) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setCurPage') + ->with(1) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('getFirstItem') + ->willReturn($item); + + $this->assertFalse($this->pager->getNextItemId(99)); } /** - * Test getPreviousItemId + * Test getting previous review ID + * + * @return void */ - public function testGetPreviousItem() + public function testGetPreviousItemId(): void { - $this->assertEquals(2, $this->_helper->getPreviousItemId(6)); + $item = new DataObject(['id' => 4]); + + $this->collection->expects($this->once()) + ->method('addFieldToFilter') + ->with('main_table.review_id', ['lt' => 5]) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setOrder') + ->with('main_table.review_id', 'DESC') + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setPageSize') + ->with(1) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setCurPage') + ->with(1) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('getFirstItem') + ->willReturn($item); + + $this->assertEquals(4, $this->pager->getPreviousItemId(5)); } /** - * Test getPreviousItemId when item not found or no next item + * Test that getPreviousItemId returns false when no item exists + * + * @return void */ - public function testGetPreviousItemNotFound() + public function testGetPreviousItemIdReturnsFalse(): void { - $this->assertFalse($this->_helper->getPreviousItemId(30)); - $this->assertFalse($this->_helper->getPreviousItemId(3)); + $item = new DataObject([]); + + $this->collection->expects($this->once()) + ->method('addFieldToFilter') + ->with('main_table.review_id', ['lt' => 1]) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setOrder') + ->with('main_table.review_id', 'DESC') + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setPageSize') + ->with(1) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('setCurPage') + ->with(1) + ->willReturnSelf(); + $this->collection->expects($this->once()) + ->method('getFirstItem') + ->willReturn($item); + + $this->assertFalse($this->pager->getPreviousItemId(1)); } }