Fix for update products via csv file (fix for 22028)

[mtwegrzycki]

Description (*)

It is fix for issue #22028

In query for selecting ids to indexing after import products, I changed "between" into "in",
Before change, when products were updated via csv file import in Admin panel, to indexing process were selected all products from range between the smallest and the biggest products id from csv file. When the ids were from wide range, it caused selecting a huge amount of data and attempt to inserting them to catalog_product_index_price_temp. If data amount was to big, it caused error
After change, for indexing process are selected only products from csv file.

Example:
Before change, when we try to update two products with id 30 and 31, only two products were selected to indexing.
When products have ids 30 and 50, it caused, that 21 products were selected to indexing (All products with ids between 30 and 50.

After change, in both cases described above, only two products will be selected to indexing.

Fixed Issues (if relevant)

No related issues found.

Manual testing scenarios (*)

The same scenario can be applied to reproduce bug and check fix:

1. Prepare csv file to update products (2 products or more). (This csv should contain products with the lowest and the highest id - 1 and 6000)
2. In Admin panel go to: System->Import and choose Product and Add/Update
3. Upload file
4. Check Data
5. Import csv file (error should be displayed now)

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 on Travis CI are green)

[m2-assistant]

Hi @mtwegrzycki. 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-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[magento-cicd2]

All committers have signed the CLA.

[mtwegrzycki]

@orlangur , it is the same pull request like in #22032 , but I corrupted my repository and PR was closed automatically.

[orlangur]

LGTM!

@mtwegrzycki please make sure commit is linked to your GitHub user and CLA is signed.

[mtwegrzycki]

Okay, I fixed issue with CLA.

[magento-engcom-team]

@mtwegrzycki thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[mtwegrzycki]

I can see, that there any integration errors in Travis. But it does not look like related to my code changes.
Please, let me know, if there are required any additional actions from me.

[orlangur]

@mtwegrzycki true, build failure seems to be random.

[magento-engcom-team]

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

[Nazar65]

Hi @mtwegrzycki thank you for Pull request, can you please explain? why you add your changes to else condition, ? because if you can see there is a validation count($entityIds) > 1 so if you have 1 entityIds then it switch in else and this looks like where e.entity_id in (1);

if (count($entityIds) > 1) {
                $select->where(sprintf('e.entity_id BETWEEN %s AND %s', min($entityIds), max($entityIds)));
            } else {
                $select->where('e.entity_id = ?', $entityIds);
                $select->where('e.entity_id IN(?)', $entityIds);
            }

[mtwegrzycki]

Hi @Nazar65,
I didn't add any changes in else condition. I added IN after BETWEEN in if condition and removed else condition.
I checked and it doesn't matter if we use IN or equals for one id(item) in MySQL.
Part of code in your message is not the same as my PR. My change looks like this:

if (count($entityIds) > 1) {
                $select->where(sprintf('e.entity_id BETWEEN %s AND %s', min($entityIds), max($entityIds)));
                $select->where('e.entity_id IN(?)', $entityIds);
            }

Please, let me know, if you need more detailed information.

[mtwegrzycki]

Okay, now I see the bug in my code.
If id will be equaled 1, SQL query will be not executed.
I should remove if condition and leave only two where queries.

[Nazar65]

@mtwegrzycki yeap. if we have one ID they not be indexed.

[mtwegrzycki]

Okay, I fixed it. :D

[Nazar65]

@orlangur can you please review ? and should we use BETWEN ? as IN works perfectly with one entity id.

[orlangur]

@Nazar65 regarding between check in comment I referenced.

[magento-engcom-team]

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

[ghost]

@mtwegrzycki unfortunately, only members of the maintainers team are allowed to assign developers to the pull request

[soleksii]

✔️ QA Passed

[m2-assistant]

Hi @mtwegrzycki, 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.

[hatimeria-artur-jewula]

If you are adding "IN" why the need to keep "BETWEEN"? I assumed the between statement was introduced in case a large number of items need reindexing. As the query itself is pretty long (from what I can see about 11k characters) it could cause fail due to batch size limit. But instead of between statement the better solution in my opinion is to chunk entity ids array and execute 2 or 3 queries.

[orlangur]

@hatimeria-artur-jewula this suggestion came from #19878 (comment)