Skip to content
This repository has been archived by the owner on Apr 29, 2019. It is now read-only.

#42 - Dynamically fill the image keys #94

Merged

Conversation

VincentMarmiesse
Copy link
Contributor

Hello,
Here is a fix for the issue #42, I am open to discussion if necessary.

@dmanners
Copy link
Contributor

dmanners commented Jan 9, 2018

@VincentMarmiesse thank you for this PR. Would you be able to have a look at the failing unit tests.

@dmanners dmanners self-requested a review January 9, 2018 08:06
@VincentMarmiesse
Copy link
Contributor Author

@dmanners Ok tests are failing because of the $this->_connection->select()->from() line into my _initImagesArrayKeys function.
Can you help me on this? I assume I'm supposed to use a mock to get $this->_connection working right?

Thank you for your help.

@dmanners
Copy link
Contributor

dmanners commented Jan 9, 2018

@VincentMarmiesse yup you are right about this. These methods and objects will need to be mocked correctly. I think the problem is that the select mock is only expected to be called once per test but now with your change it will need to be setup to run multiple times per test. Without knowing too much about the full example I think you will need to setup another expects such as the one found in https://github.com/magento-engcom/import-export-improvements/blob/2.3-develop/app/code/Magento/CatalogImportExport/Test/Unit/Model/Import/ProductTest.php#L526 but with the correct information passed into the with section.

Do you think you would be able to work from that? Otherwise I could always fix one test and see if you can work that fix into all the other tests?

@dmanners
Copy link
Contributor

Hi @VincentMarmiesse I have refactored the code a bit to create a new image type processor. Basically I needed to do this to make the test easier to fix. What do you think to that approach?

Also could you please double check that my changes are working as you desire.

Thanks

@PieterCappelle
Copy link
Contributor

Looks good for me. Do we have unit test with new image attribute? How will this behave in the import CSV structure? New column will be added or will it import with additional_attributes. I think we need some more explanation about this issue. What about the path of the image. Thanks!

@dmanners dmanners self-assigned this Jan 24, 2018
@dmanners dmanners added this to the January 2018 milestone Jan 24, 2018
@magento-engcom-team
Copy link
Contributor

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

@VincentMarmiesse
Copy link
Contributor Author

Hi @dmanners,

Sorry for the delay, I like this approch and it works for me. I'm kind of new to tests writing and didn't see how to test it but your solution seems nice!

@PieterCappelle, in my CSV file I want to import, I have a column for each image, so base_image, small_image, my_custom_image etc. I use FastSimpleImport2 which is based on M2 standard import.

@magento-engcom-team, ok I have accepted the invitation, should I do something more?

@magento-team magento-team merged commit c52ad59 into magento-engcom:2.3-develop Jan 25, 2018
@VincentMarmiesse VincentMarmiesse deleted the 42-imageskeys branch February 9, 2018 15:41
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants