Fix issue with no results on other than first search pages when using elasticsearch

[pierzakp]

Description (*)

Pull request introduces the changes in search results applier in elastic search module which are resetting current page and page size on collection.
It is needed because elastic search applies condition on the collection with a particular set of entity ids, w/o change introduced by PR there are no results on search pages other than first one because there is a limit and offset applied on select query.

Fixed Issues (if relevant)

1. No results on other than first search pages when using elasticsearch #25378: No results on other than first search pages when using elasticsearch

Manual testing scenarios (*)

1. Need to have configured elastic search as search engine.
2. Magento 2.3.3 EE but will occur on CE as well.
3. Products which will be found using some phrase and their count is higher than the configured page size.
4. Search using this phrase, on the bottom on search page pagination will be rendered, if we click on second, third, etc no result message will appear because the query will look like the following:
   (e.entity_id IN (8357, 3524, 18671, 8360, 8390, 3971, 11213, 3527, 8387, 11393)) ORDER BY FIELD(e.entity_id,8357,3524,18671,8360,8390,3971,11213,3527,8387,11393) ASC
   LIMIT 10 OFFSET 10`

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)
• All automated tests passed successfully (all builds are green)

[m2-assistant]

Hi @pierzakp. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento give me test instance - deploy test instance based on PR changes
• @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

[pierzakp]

Few more issue details - limit and offset on collection is set after:
\Magento\Elasticsearch\Model\ResourceModel\Fulltext\Collection\SearchResultApplier::apply
in:
\Magento\Eav\Model\Entity\Collection\AbstractCollection::_loadEntities (line 1119).

If the $this->_pageSize is null and according to my change should be at this stage it should not be executed.

[sivaschenko]

Hi @pierzakp , thanks for the pull request!

The fix looks valid to me, however, I would encourage @kokoc to review it as well, as the implementation of SearchResultApplier is not very straight forward.

Resetting pagination looks correct approach as pagination is handled in a custom way in this case (sliceItems).
The page size and current page are actually passed from collection to the SearchResultApplier, so the only concern that I have is that they might be also needed for other components/plugins.

[gaiterjones]

When can we expect a fix to be released for this? Looking at @pierzakp PR the whole SearchResultApplier class is different to the Mage2 Open Source 2.3.3 release code on my dev server.

[pierzakp]

I have updated PR with the latest 2.3-develop branch @gaiterjones.

[magento-engcom-team]

Hi @sivaschenko, thank you for the review.
ENGCOM-6271 has been created to process this Pull Request
✳️ @sivaschenko, could you please add one of the following labels to the Pull Request?

Label	Description
Auto-Tests: Covered	All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered	Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required	Changes in Pull Request does not require coverage by auto-tests

[sivaschenko]

@pierzakp could you please cover the changed functionality with a test

[pierzakp]

@sivaschenko phpunit test added.

[sivaschenko]

Thanks for the unit test @pierzakp , can you please fix the failed static test.

[pierzakp]

@sivaschenko copyright has been added.

[magento-engcom-team]

Hi @sivaschenko, thank you for the review.
ENGCOM-6271 has been created to process this Pull Request

[engcom-Alfa]

Hi @pierzakp. Unfortunately, we are not able to reproduce this issue on the fresh 2.3-develop instance.

Actual Result:

Tested on Elasticsearch 6.2.4

@pierzakp Could you take a look?

Thanks!

[VladimirZaets]

Hi @pierzakp. Thanks for collaboration. This issue already fixed in 2.3-develop.

[m2-assistant]

Hi @pierzakp, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.