search

magento-engcom / import-export-improvements

Open Software License 3.0
31 stars 29 forks source link

#42 - Dynamically fill the image keys #94

Closed VincentMarmiesse closed 6 years ago

VincentMarmiesse commented 6 years ago

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

dmanners commented 6 years ago

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

VincentMarmiesse commented 6 years ago

@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 commented 6 years ago

@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 commented 6 years ago

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 commented 6 years ago

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!

magento-engcom-team commented 6 years ago

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

VincentMarmiesse commented 6 years ago

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?