Skip to content

Commit

Permalink
MC-36048: Unexpected behavior of sorting in the Magento Admin Panel
Browse files Browse the repository at this point in the history
  • Loading branch information
OlgaVasyltsun committed Aug 7, 2020
1 parent 0d7cf24 commit 663f023
Show file tree
Hide file tree
Showing 7 changed files with 112 additions and 10 deletions.
8 changes: 5 additions & 3 deletions app/code/Magento/Theme/Plugin/Data/Collection.php
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

namespace Magento\Theme\Plugin\Data;

use Magento\Framework\Data\Collection as DataCollection;

/**
* Plugin to return last page if current page greater then collection size.
*/
Expand All @@ -15,14 +17,14 @@ class Collection
/**
* Return last page if current page greater then last page.
*
* @param \Magento\Framework\Data\Collection $subject
* @param DataCollection $subject
* @param int $result
* @return int
*/
public function afterGetCurPage(\Magento\Framework\Data\Collection $subject, int $result)
public function afterGetCurPage(DataCollection $subject, int $result): int
{
if ($result > $subject->getLastPageNumber()) {
$result = $subject->getLastPageNumber();
$result = 1;
}

return $result;
Expand Down
3 changes: 0 additions & 3 deletions app/code/Magento/Theme/etc/adminhtml/di.xml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,4 @@
</argument>
</arguments>
</type>
<type name="Magento\Framework\Data\Collection">
<plugin name="currentPageDetection" type="Magento\Theme\Plugin\Data\Collection" />
</type>
</config>
3 changes: 3 additions & 0 deletions app/code/Magento/Theme/etc/di.xml
Original file line number Diff line number Diff line change
Expand Up @@ -326,4 +326,7 @@
<argument name="filesystemDriver" xsi:type="object">Magento\Framework\Filesystem\Driver\File</argument>
</arguments>
</type>
<type name="Magento\Framework\Data\Collection">
<plugin name="currentPageDetection" type="Magento\Theme\Plugin\Data\Collection" />
</type>
</config>
4 changes: 0 additions & 4 deletions app/code/Magento/Theme/etc/frontend/di.xml
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,4 @@
<argument name="filePath" xsi:type="string">css/critical.css</argument>
</arguments>
</type>

<type name="Magento\Framework\Data\Collection">
<plugin name="currentPageDetection" type="Magento\Theme\Plugin\Data\Collection" />
</type>
</config>
12 changes: 12 additions & 0 deletions app/code/Magento/Theme/etc/webapi_rest/di.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?xml version="1.0"?>
<!--
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
-->
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
<type name="Magento\Framework\Data\Collection">
<plugin name="currentPageDetection" disabled="true"/>
</type>
</config>
12 changes: 12 additions & 0 deletions app/code/Magento/Theme/etc/webapi_soap/di.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?xml version="1.0"?>
<!--
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
-->
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:ObjectManager/etc/config.xsd">
<type name="Magento\Framework\Data\Collection">
<plugin name="currentPageDetection" disabled="true"/>
</type>
</config>
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
<?php
/**
* Copyright © Magento, Inc. All rights reserved.
* See COPYING.txt for license details.
*/
declare(strict_types=1);

namespace Magento\Catalog\Model\ResourceModel\Attribute;

use Magento\Catalog\Model\ResourceModel\Product\Attribute\CollectionFactory;
use Magento\TestFramework\Helper\Bootstrap;
use PHPUnit\Framework\TestCase;

/**
* Tests \Magento\Catalog\Model\ResourceModel\Product\Attribute\Collection
*/
class CollectionTest extends TestCase
{
/**
* @var CollectionFactory .
*/
private $attributesCollectionFactory;

/**
* @inheritDoc
*/
protected function setUp(): void
{
$objectManager = Bootstrap::getObjectManager();
$this->attributesCollectionFactory = $objectManager->get(CollectionFactory::class);
}

/**
* @magentoAppArea adminhtml
* @dataProvider attributesCollectionGetCurrentPageDataProvider
*
* @param array|null $condition
* @param int $currentPage
* @param int $expectedCurrentPage
* @return void
*/
public function testAttributesCollectionGetCurrentPage(
?array $condition,
int $currentPage,
int $expectedCurrentPage
): void {
$attributeCollection = $this->attributesCollectionFactory->create();
$attributeCollection->setCurPage($currentPage)->setPageSize(20);

if ($condition !== null) {
$attributeCollection->addFieldToFilter('is_global', $condition);
}

$this->assertEquals($expectedCurrentPage, (int)$attributeCollection->getCurPage());
}

/**
* @return array[]
*/
public function attributesCollectionGetCurrentPageDataProvider(): array
{
return [
[
'condition' => null,
'currentPage' => 1,
'expectedCurrentPage' => 1,
],
[
'condition' => ['eq' => 0],
'currentPage' => 1,
'expectedCurrentPage' => 1,
],
[
'condition' => ['eq' => 0],
'currentPage' => 15,
'expectedCurrentPage' => 1,
],
];
}
}

4 comments on commit 663f023

@mrtuvn
Copy link
Contributor

@mrtuvn mrtuvn commented on 663f023 May 5, 2021

Choose a reason for hiding this comment

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

@magento-engcom-team Could some one give description from MC-36048: Unexpected behavior of sorting in the Magento Admin Panel
This PR looks correct but it is also reverting changes from previously #26988
I'm not sure this is correct fix the root cause or workarounds solution with some approach disabled = true

cc: @gabrieldagama @sidolov

@andrewdaluz
Copy link

Choose a reason for hiding this comment

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

This change is giving me issues with commands attached to bin/magento. We have a custom reindex where the pagination is going into an infinite loop because it's always returning the FIRST page results.

@aurmil
Copy link

@aurmil aurmil commented on 663f023 Aug 9, 2022

Choose a reason for hiding this comment

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

I also don't understand this change and have the same issue as @andrewdaluz

@mrtuvn
Copy link
Contributor

@mrtuvn mrtuvn commented on 663f023 Aug 10, 2022

Choose a reason for hiding this comment

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

I believe newer code already updated in 2.4.5. But not sure how effective and solve problem!

Please sign in to comment.