Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

catalog:images:resize total images count calculates incorrectly #18387: #18807

Conversation

vpodorozh
Copy link
Contributor

@vpodorozh vpodorozh commented Oct 25, 2018

Description (#18387)

Assumption from issue: >>"catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue".
However, there is a problem with total images count calculation select (distinct by image path is not used in count(*) ) - functionality by itself works correctly and process all images.

Fixed Issues

#18387: catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue

  1. fixed getCountAllProductImages method that calculates total images count incorrectly;
  2. covered \Magento\Catalog\Model\ResourceModel\Product\Image with unit tests;

Manual testing scenarios

Variant I

  1. Magento 2.3-develop
  2. Sample data deployed (in order to easily have several test images in store)
Expected result

You do see total images count 801.

Actual result

You do see total images count 3422.

Variant II (synthetic case)

  1. Magento 2.3-develop
  2. Generate products iwth images:
    php bin/magento setup:performance:generate-fixtures setup/performance-toolkit/profiles/ce/medium.xml
  3. apply the same image paths for all products - so images for product will become duplicated.
    For details check catalog_product_entity_media_gallery table.
    example SQL to do this:
    create temporary table zzz as (select NULL as value_id, c.attribute_id, c.value, c.media_type, c.disabled from catalog_product_entity_media_gallery as c); insert into catalog_product_entity_media_gallery select * from zzz;
Expected result

Total images count will be equal to the value from setup/performance-toolkit/profiles/ce/medium.xml in images-count node ( 1000 ).

Actual result

Total images count will be 2 times higher that their exact value.

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)

catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix getCountAllProductImages select and cover class with unit tests.
@magento-engcom-team magento-engcom-team added Partner: ISM eCompany Pull Request is created by partner ISM eCompany partners-contribution Pull Request is created by Magento Partner Component: Catalog Release Line: 2.3 labels Oct 25, 2018
@magento-engcom-team
Copy link
Contributor

Hi @vpodorozh. 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 $VERSION instance - deploy vanilla Magento instance

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

catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix code style;
@magento-engcom-team
Copy link
Contributor

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

catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix unit test - now it can test also not only full batches, but partial as well (139 images with batch size 100);
…-resize' into 2.3-develop-18387-catalog-images-resize
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix unit test - now it can test also not only full batches, but partial as well (139 images with batch size 100);
vpodorozh pushed a commit to vpodorozh/magento2 that referenced this pull request Oct 27, 2018
@vpodorozh
Copy link
Contributor Author

vpodorozh commented Oct 27, 2018

Hi @duhon - I've found out that my unit test \Magento\Catalog\Test\Unit\Model\ResourceModel\Product\ImageTest covers only "full batches" (imgCount = 200, batchSize = 100). But the "partial batches" were not covered (imgCount = 139, batchSize = 100).
So I've adjusted it and now UnitTest cover full range of batches, could you please review it again?

Thx.

slavvka and others added 4 commits October 29, 2018 12:58
catalog:images:resize fails to process all images -> Possible underlying Magento/Framework/DB/Query/Generator issue
- fix code style;
…-resize' into 2.3-develop-18387-catalog-images-resize

# Conflicts:
#	app/code/Magento/Catalog/Test/Unit/Model/ResourceModel/Product/ImageTest.php
vpodorozh pushed a commit to vpodorozh/magento2 that referenced this pull request Oct 30, 2018
@vpodorozh
Copy link
Contributor Author

vpodorozh commented Oct 31, 2018

Okay, guys, I'm tired to fix these "quality review" issues by codacy. Do I really need to fix these problems? : https://app.codacy.com/app/antonkril/magento2/pullRequest?prid=2433664

@slavvka
Copy link
Member

slavvka commented Oct 31, 2018

@vpodorozh you may ignore the Codacy failures. You don't need to fix them

@magento-engcom-team
Copy link
Contributor

Hi @vpodorozh. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants