Skip to content

Conversation

@imran9935
Copy link

@imran9935 imran9935 commented Oct 14, 2025

Fixed Issues

Fixes : Admin got auto logged out when product reviews data is more than 28,000 at Marketing > All Reviews

Description (*)

Admin gets automatically logged out when opening the 'All Reviews' section in Marketing. This occurs for products with more than 28,000 reviews, as the system cannot handle such a large volume at once.

Manual testing scenarios (*)

  1. Add products reviews with more than 28,000 reviews
  2. Log in to the magento admin panel and navigate to Marketing > User Content > All Reviews.
  3. Try to navigate or refresh the All Reviews page.
  4. Observe that the admin is automatically logged out.

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

@m2-assistant
Copy link

m2-assistant bot commented Oct 14, 2025

Hi @imran9935. Thank you for your contribution!
Here are some useful tips on how you can test your changes using Magento test environment.
❗ Automated tests can be triggered manually with an appropriate comment:

  • @magento run all tests - run or re-run all required tests against the PR changes
  • @magento run <test-build(s)> - run or re-run specific test build(s)
    For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names.

Allowed build names are:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@lbajsarowicz
Copy link
Contributor

@magento run all tests

@engcom-Hotel
Copy link
Contributor

@magento run all tests

Copy link
Contributor

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @imran9935,

Thank you for your contribution!

Please fix the the failed failed static and integration tests. and have a look into the below review comment.

* @return int|false
*/
protected function _findItemPositionByValue($value)
private function getRelativeReviewId($id, $operator, $order)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add return type for this method

*
* @return array
*/
public function getResultingIds()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this method could be backward incompatible change.

@engcom-Hotel engcom-Hotel moved this from Review in Progress to Changes Requested in Community Dashboard Oct 29, 2025
@engcom-Hotel
Copy link
Contributor

Hello @imran9935,

Gentle reminder for this PR — please let us know when it’s ready for review.

Thank you

@imran9935
Copy link
Author

imran9935 commented Nov 20, 2025

Hello @engcom-Hotel,
I have made the requested code changes, and also updated the code to resolve the failed static and integration tests.

Thank you

@imran9935
Copy link
Author

@magento run all tests

@engcom-Hotel engcom-Hotel moved this from Changes Requested to Review in Progress in Community Dashboard Nov 21, 2025
Copy link
Contributor

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @imran9935,

Thank you for your contribution!

We can see various BiC changes in this PR, please fix those. You can refer to the below documents for reference:

*
* @return \Magento\Backend\Block\Widget\Grid
*/
protected function _afterLoadCollection()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this method could backward incompatible change, third-party extensions may override this method.

public function __construct(
\Magento\Framework\App\Helper\Context $context,
\Magento\Backend\Model\Session $backendSession
CollectionFactory $reviewCollectionFactory
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use ObjectManager to initialise this, and make it null by default

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various methods has been removed from here, this could be backward incompatible change.

@imran9935
Copy link
Author

@engcom-Hotel I have added methods to maintain backward compatibility

@engcom-Hotel
Copy link
Contributor

@magento run all tests

@imran9935
Copy link
Author

@magento run all tests

Copy link
Contributor

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @imran9935,

Please refer to the below review comments.

Thank you

* @return int|false
*/
public function getNextItemId($id)
public function getNextItemId($id): int|false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend to make it bool again

* @return int|false
*/
public function getPreviousItemId($id)
public function getPreviousItemId($id): int|false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

recommend to make it bool


/** @var $actionPager \Magento\Review\Helper\Action\Pager */
$actionPager = $this->_reviewActionPager;
$actionPager->setStorageId('reviews');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing $actionPager->setStorageId('reviews') breaks the pagination API contract.

If setStorageId() is never called, then _getStorageKey() in Pager.php throws exception:

*
* @return array
* @deprecated This method is not being used to fetch ids anymore.We use it only to support backward compatibility
* @see Pager
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reference the new method in a @see tag as a recommended replacement.

Comment on lines -104 to -106
$actionPager = $this->_reviewActionPager;
$actionPager->setStorageId('reviews');
$actionPager->setItems($this->getCollection()->getResultingIds());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing these lines might breaks prev/next functionality unless replaced with alternative.

Copy link
Author

@imran9935 imran9935 Nov 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@engcom-Hotel I have made the other changes, for prev/next functionality, I have handled it in the Pager.php, please look at getPreviousItemId and getNextItemId methods.

@engcom-Hotel engcom-Hotel moved this from Review in Progress to Changes Requested in Community Dashboard Nov 24, 2025
@engcom-Hotel
Copy link
Contributor

@magento run all tests

@imran9935
Copy link
Author

@magento run all tests

Copy link
Contributor

@engcom-Hotel engcom-Hotel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @imran9935,

Please fix the failed static tests and also look into the below review comment.

Thank you

class Pager extends \Magento\Framework\App\Helper\AbstractHelper
{
const STORAGE_PREFIX = 'search_result_ids';
protected const STORAGE_PREFIX = 'search_result_ids';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason to change constant visibility from public to protected?

@ct-prd-projects-boards-automation ct-prd-projects-boards-automation bot moved this from Review in Progress to Changes Requested in Community Dashboard Nov 25, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: needs update Project: Community Picked PRs upvoted by the community

Projects

Status: Changes Requested

Development

Successfully merging this pull request may close these issues.

Admin got auto logged out when product reviews data is more than 28,000 at Marketing > All Reviews

4 participants