Skip to content

8255: Export Products action doesn't consider hide_for_product_page value. #11926

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

Merged
merged 18 commits into from
Dec 1, 2017
Merged

Conversation

nmalevanec
Copy link
Contributor

@nmalevanec nmalevanec commented Nov 1, 2017

Description

Fix Export and Import Products action doesn't consider hide_for_product_page value.

Fixed Issues (if relevant)

  1. Export Products action doesn't consider hide_for_product_page value #8255: Export Products action doesn't consider hide_for_product_page value

Manual testing scenarios

  1. Create a simple product and upload an image.
  2. Change the scope to 1 store view.
  3. Set the image as Hide for product page in that store view.
  4. Navigate to System -> Export.
  5. Select Products and then Continue.
  6. Check that CSV files should have 2 lines. The default values at the first row of values.
  7. The second lines should contain values on sku, store_view_code and hide_from_product_page columns.
  8. Remove created simple product.
  9. Navigate to System -> Import.
  10. Import product from exported CSV with add/update behavior.
  11. Check, imported product at 1 store view has "Hide fro product page" checked for image.

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)

@magento-engcom-team magento-engcom-team added 2.2.x bugfix Component: ImportExport Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Nov 7, 2017
@dmanners dmanners self-assigned this Nov 13, 2017
@dmanners dmanners added this to the November 2017 milestone Nov 13, 2017
@dmanners
Copy link
Contributor

@nmalevanec could you have a look at the merge conflicts for me please.

# Conflicts:
#	app/code/Magento/CatalogImportExport/Model/Import/Product.php
@nmalevanec
Copy link
Contributor Author

@dmanners Done.

$data[$itemId][$storeId][self::COL_ATTR_SET] = $this->_attrSetIdToName[$attrSetId];
$data[$itemId][$storeId][self::COL_TYPE] = $item->getTypeId();
}
$attrSetId = $item->getAttributeSetId();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this also change the behavior for non-image multi-select data also? Do we have a test covering this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It covers by Magento/CatalogImportExport/Model/Export/ProductTest::testExport() for instance. Exported product has multiselect data.

* @param array $mediaGalleryData
* @return array
*/
private function restoreDisableImage(array $mediaGalleryData)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we already have a massive file here but would it make sense/be possible to extract the image processing to a separate class?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

private function filterImageInsertData(array $multiInsertData, array $imageNames)
{
//Remove image duplicates for stores.
$multiInsertData = array_map("unserialize", array_unique(array_map("serialize", $multiInsertData)));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this format json? I would be a bit worried that we are calling unserialize/serialize on data that we do not trust/have control over.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@dmanners
Copy link
Contributor

There are two test failures currently. Could you look into them please.


1) Magento\CatalogImportExport\Model\Export\ProductTest::testExportWithMedia
--
13-Nov-2017 04:15:27 | [exec] PDOException: SQLSTATE[23000]: Integrity constraint violation: 1452 Cannot add or update a child row: a foreign key constraint fails (`bamboo_integration_0`.`pfx_catalog_product_option_type_price`, CONSTRAINT `FK_0404E60DE3B8ACE3BC2CE062A74D0A97` FOREIGN KEY (`option_type_id`) REFERENCES `pfx_catalog_product_option_type_valu)


2) Magento\CatalogImportExport\Model\Export\ProductTest::testExportWithMedia
--
13-Nov-2017 04:15:27 | [exec] Failed asserting that Array &0 (
13-Nov-2017 04:15:27 | [exec]     'additional_images' => 'additional_images'
13-Nov-2017 04:15:27 | [exec]     '/m/a/magento_image.jpg' => '/m/a/magento_image.jpg'
13-Nov-2017 04:15:27 | [exec]     '' => ''
13-Nov-2017 04:15:27 | [exec] ) is identical to Array &0 (
13-Nov-2017 04:15:27 | [exec]     'hide_from_product_page' => 'hide_from_product_page'
13-Nov-2017 04:15:27 | [exec]     '' => ''
13-Nov-2017 04:15:27 | [exec]     '/m/a/magento_image.jpg' => '/m/a/magento_image.jpg'
13-Nov-2017 04:15:27 | [exec] ).

@nmalevanec
Copy link
Contributor Author

@dmanners integration test fixed.

@dmanners
Copy link
Contributor

@nmalevanec just to keep you updated there is a test that compares the installation database with sample data compared to 2.2-develop and sample data. This is showing differences after your change. I am looking into this and will get back to you about it.

@nmalevanec
Copy link
Contributor Author

@dmanners Semantic Version Checker fixed.

@devgas
Copy link

devgas commented Mar 7, 2018

@okobchenko Fix for #12356 doesn't exist in Magento 2.2.3

@dmanners
Copy link
Contributor

dmanners commented Mar 8, 2018

Hi @devgas 2.2.3 was a security patch release and as such was limited in what the contents of this release were. This should be included in the 2.2.4 release though.

@nmalevanec nmalevanec deleted the 8255 branch August 21, 2018 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Component: ImportExport Fixed in 2.2.x The issue has been fixed in 2.2 release line Progress: accept Release Line: 2.2 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants