From 6e8aa57655cd14d02a7f0bb5d8a51efe40029e1c Mon Sep 17 00:00:00 2001 From: Imran Date: Tue, 14 Oct 2025 14:53:07 +0530 Subject: [PATCH 1/4] Fix admin auto-logout when displaying large number of product reviews --- .../Magento/Review/Block/Adminhtml/Edit.php | 1 - .../Magento/Review/Block/Adminhtml/Grid.php | 15 -- .../Magento/Review/Helper/Action/Pager.php | 127 +++--------- .../Review/Product/Collection.php | 24 --- .../Test/Unit/Helper/Action/PagerTest.php | 189 +++++++++++++----- 5 files changed, 164 insertions(+), 192 deletions(-) diff --git a/app/code/Magento/Review/Block/Adminhtml/Edit.php b/app/code/Magento/Review/Block/Adminhtml/Edit.php index 9162d293f9332..fa319261f98e3 100644 --- a/app/code/Magento/Review/Block/Adminhtml/Edit.php +++ b/app/code/Magento/Review/Block/Adminhtml/Edit.php @@ -67,7 +67,6 @@ protected function _construct() /** @var $actionPager \Magento\Review\Helper\Action\Pager */ $actionPager = $this->_reviewActionPager; - $actionPager->setStorageId('reviews'); $reviewId = $this->getRequest()->getParam('id'); $prevId = $actionPager->getPreviousItemId($reviewId); diff --git a/app/code/Magento/Review/Block/Adminhtml/Grid.php b/app/code/Magento/Review/Block/Adminhtml/Grid.php index 798d6ae7148af..5d62c28a05268 100644 --- a/app/code/Magento/Review/Block/Adminhtml/Grid.php +++ b/app/code/Magento/Review/Block/Adminhtml/Grid.php @@ -93,21 +93,6 @@ protected function _construct() $this->setDefaultSort('created_at'); } - /** - * Save search results - * - * @return \Magento\Backend\Block\Widget\Grid - */ - protected function _afterLoadCollection() - { - /** @var $actionPager \Magento\Review\Helper\Action\Pager */ - $actionPager = $this->_reviewActionPager; - $actionPager->setStorageId('reviews'); - $actionPager->setItems($this->getCollection()->getResultingIds()); - - return parent::_afterLoadCollection(); - } - /** * @inheritDoc */ diff --git a/app/code/Magento/Review/Helper/Action/Pager.php b/app/code/Magento/Review/Helper/Action/Pager.php index 5d1c5fd308e9c..e0bcc3ac78805 100644 --- a/app/code/Magento/Review/Helper/Action/Pager.php +++ b/app/code/Magento/Review/Helper/Action/Pager.php @@ -6,7 +6,8 @@ namespace Magento\Review\Helper\Action; -use Magento\Framework\Exception\LocalizedException; +use Magento\Framework\App\Helper\AbstractHelper; +use Magento\Review\Model\ResourceModel\Review\CollectionFactory; /** * Action pager helper for iterating over search results @@ -14,136 +15,68 @@ * @api * @since 100.0.2 */ -class Pager extends \Magento\Framework\App\Helper\AbstractHelper +class Pager extends AbstractHelper { - const STORAGE_PREFIX = 'search_result_ids'; - - /** - * Storage id - * - * @var int - */ - protected $_storageId = null; - /** - * Array of items + * Review collection factory * - * @var array + * @var CollectionFactory */ - protected $_items = null; + protected $reviewCollectionFactory; /** - * Backend session model + * Pager constructor. * - * @var \Magento\Backend\Model\Session - */ - protected $_backendSession; - - /** * @param \Magento\Framework\App\Helper\Context $context - * @param \Magento\Backend\Model\Session $backendSession + * @param CollectionFactory $reviewCollectionFactory */ public function __construct( \Magento\Framework\App\Helper\Context $context, - \Magento\Backend\Model\Session $backendSession + CollectionFactory $reviewCollectionFactory ) { - $this->_backendSession = $backendSession; parent::__construct($context); + $this->reviewCollectionFactory = $reviewCollectionFactory; } /** - * Set storage id - * - * @param int $storageId - * @return void - */ - public function setStorageId($storageId) - { - $this->_storageId = $storageId; - } - - /** - * Set items to storage - * - * @param array $items - * @return $this - */ - public function setItems(array $items) - { - $this->_items = $items; - $this->_backendSession->setData($this->_getStorageKey(), $this->_items); - - return $this; - } - - /** - * Load stored items - * - * @return void - */ - protected function _loadItems() - { - if ($this->_items === null) { - $this->_items = (array)$this->_backendSession->getData($this->_getStorageKey()); - } - } - - /** - * Get next item id + * Get the next review id. * * @param int $id - * @return int|bool + * @return int|false */ public function getNextItemId($id) { - $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 + * @return int|false */ public function getPreviousItemId($id) { - $position = $this->_findItemPositionByValue($id); - if ($position === false || $position == 0) { - return false; - } - - return $this->_items[$position - 1]; + return $this->getRelativeReviewId($id, 'lt', 'DESC'); } /** - * Return item position based on passed in value + * Get the review id based on comparison and order. * - * @param mixed $value - * @return int|bool + * @param int $id + * @param string $operator + * @param string $order + * @return int|false */ - protected function _findItemPositionByValue($value) + private function getRelativeReviewId($id, $operator, $order) { - $this->_loadItems(); - return array_search($value, $this->_items); - } - - /** - * Get storage key - * - * @return string - * @throws \Magento\Framework\Exception\LocalizedException - */ - protected function _getStorageKey() - { - if (!$this->_storageId) { - throw new LocalizedException(__("The storage key wasn't set. Add the storage key and try again.")); - } - - return self::STORAGE_PREFIX . $this->_storageId; + $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 900cdc1f330d4..87912df2d0dba 100644 --- a/app/code/Magento/Review/Model/ResourceModel/Review/Product/Collection.php +++ b/app/code/Magento/Review/Model/ResourceModel/Review/Product/Collection.php @@ -401,30 +401,6 @@ public function getAllIds($limit = null, $offset = null) return $this->getConnection()->fetchCol($idsSelect); } - /** - * Get result sorted ids - * - * @return array - */ - public function getResultingIds() - { - $idsSelect = clone $this->getSelect(); - $data = $this->getConnection() - ->fetchAll( - $idsSelect - ->reset(Select::LIMIT_COUNT) - ->reset(Select::LIMIT_OFFSET) - ->columns('rt.review_id') - ); - - return array_map( - function ($value) { - return $value['review_id']; - }, - $data - ); - } - /** * Render SQL for retrieve product count * 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 4529184fa0e4b..5a237427cdf90 100644 --- a/app/code/Magento/Review/Test/Unit/Helper/Action/PagerTest.php +++ b/app/code/Magento/Review/Test/Unit/Helper/Action/PagerTest.php @@ -7,93 +7,172 @@ 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() - ->setMethods( - ['setData', '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); + + $this->pager = new Pager($context, $this->collectionFactory); } /** - * Test getNextItem + * Test getting next review ID + * + * @return void */ - public function testGetNextItem() + public function testGetNextItemId() { - $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() { - $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() { - $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() { - $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)); } } From 4ab76c23d33103e399686b1bbe9643bf76f128ae Mon Sep 17 00:00:00 2001 From: Imran Date: Thu, 20 Nov 2025 18:58:09 +0530 Subject: [PATCH 2/4] Fixed failed static and integration tests --- .../Magento/Review/Helper/Action/Pager.php | 17 ++++++------- .../Review/Product/Collection.php | 25 +++++++++++++++++++ .../Test/Unit/Helper/Action/PagerTest.php | 8 +++--- 3 files changed, 37 insertions(+), 13 deletions(-) diff --git a/app/code/Magento/Review/Helper/Action/Pager.php b/app/code/Magento/Review/Helper/Action/Pager.php index 84789463a1674..f77eeecc9f3f9 100644 --- a/app/code/Magento/Review/Helper/Action/Pager.php +++ b/app/code/Magento/Review/Helper/Action/Pager.php @@ -3,7 +3,6 @@ * Copyright 2013 Adobe * All Rights Reserved. */ - namespace Magento\Review\Helper\Action; use Magento\Framework\App\Helper\AbstractHelper; @@ -18,7 +17,7 @@ class Pager extends AbstractHelper { /** - * Review collection factory + * Review collection model factory * * @var CollectionFactory */ @@ -42,9 +41,9 @@ public function __construct( * Get the next review id. * * @param int $id - * @return int|false + * @return int|false */ - public function getNextItemId($id) + public function getNextItemId($id): int|false { return $this->getRelativeReviewId($id, 'gt', 'ASC'); } @@ -55,7 +54,7 @@ public function getNextItemId($id) * @param int $id * @return int|false */ - public function getPreviousItemId($id) + public function getPreviousItemId($id): int|false { return $this->getRelativeReviewId($id, 'lt', 'DESC'); } @@ -63,12 +62,12 @@ public function getPreviousItemId($id) /** * Get the review id based on comparison and order. * - * @param int $id - * @param string $operator - * @param string $order + * @param int $id + * @param string $operator + * @param string $order * @return int|false */ - private function getRelativeReviewId($id, $operator, $order) + private function getRelativeReviewId($id, $operator, $order): int|false { $collection = $this->reviewCollectionFactory->create(); $collection->addFieldToFilter('main_table.review_id', [$operator => $id]) 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 1c4d3a7a22ad3..83efd5ef26302 100644 --- a/app/code/Magento/Review/Model/ResourceModel/Review/Product/Collection.php +++ b/app/code/Magento/Review/Model/ResourceModel/Review/Product/Collection.php @@ -401,6 +401,31 @@ public function getAllIds($limit = null, $offset = null) return $this->getConnection()->fetchCol($idsSelect); } + /** + * 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 + */ + public function getResultingIds() + { + $idsSelect = clone $this->getSelect(); + $data = $this->getConnection() + ->fetchAll( + $idsSelect + ->reset(Select::LIMIT_COUNT) + ->reset(Select::LIMIT_OFFSET) + ->columns('rt.review_id') + ); + + return array_map( + function ($value) { + return $value['review_id']; + }, + $data + ); + } + /** * Render SQL for retrieve product count * 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 181491e8e2289..0eae1137a8a1f 100644 --- a/app/code/Magento/Review/Test/Unit/Helper/Action/PagerTest.php +++ b/app/code/Magento/Review/Test/Unit/Helper/Action/PagerTest.php @@ -53,7 +53,7 @@ protected function setUp(): void * * @return void */ - public function testGetNextItemId() + public function testGetNextItemId(): void { $item = new DataObject(['id' => 10]); @@ -85,7 +85,7 @@ public function testGetNextItemId() * * @return void */ - public function testGetNextItemIdReturnsFalse() + public function testGetNextItemIdReturnsFalse(): void { $item = new DataObject([]); @@ -117,7 +117,7 @@ public function testGetNextItemIdReturnsFalse() * * @return void */ - public function testGetPreviousItemId() + public function testGetPreviousItemId(): void { $item = new DataObject(['id' => 4]); @@ -149,7 +149,7 @@ public function testGetPreviousItemId() * * @return void */ - public function testGetPreviousItemIdReturnsFalse() + public function testGetPreviousItemIdReturnsFalse(): void { $item = new DataObject([]); From ed08e6279459be03820eba0ca5dfb766d18dac79 Mon Sep 17 00:00:00 2001 From: Imran Date: Fri, 21 Nov 2025 11:21:22 +0530 Subject: [PATCH 3/4] Fixed failed static test --- .../Review/Model/ResourceModel/Review/Product/Collection.php | 2 ++ 1 file changed, 2 insertions(+) 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 83efd5ef26302..c35c58009fbaa 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 @@ -406,6 +407,7 @@ public function getAllIds($limit = null, $offset = null) * * @return array * @deprecated This method is not being used to fetch ids anymore.We use it only to support backward compatibility + * @see Pager */ public function getResultingIds() { From 73e7edccde2c31517c2b5e1137155e2ef952245c Mon Sep 17 00:00:00 2001 From: Imran Date: Fri, 21 Nov 2025 12:28:36 +0530 Subject: [PATCH 4/4] Improved doc block --- app/code/Magento/Review/Block/Adminhtml/Grid.php | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/code/Magento/Review/Block/Adminhtml/Grid.php b/app/code/Magento/Review/Block/Adminhtml/Grid.php index 16edc77800610..781bc8e7f633d 100644 --- a/app/code/Magento/Review/Block/Adminhtml/Grid.php +++ b/app/code/Magento/Review/Block/Adminhtml/Grid.php @@ -19,21 +19,21 @@ class Grid extends \Magento\Backend\Block\Widget\Grid\Extended { /** - * Review action pager + * Review action pager helper * * @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 */