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

Migrate legacy import logic to adapters and hide Import page 2 #11561

Merged
merged 43 commits into from Jan 16, 2019

Conversation

Projects
None yet
6 participants
@rokaszygmantas
Copy link
Contributor

rokaszygmantas commented Nov 29, 2018

Questions Answers
Branch? develop
Description? Legacy import logic migration to adapters, in order to free and remove legacy import controller.
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? #11753
How to test? Import should work the same as before for all import types products and categories import types. A good way to test is to import the same csv file with same settings on two copies of prestashop (one with this PR and other with stable release), and then compare if the imported data is identical. It should also fix #11753. Please build the assets when testing this PR.

P.S. Noticed that when you have multistore enabled and "All shops" selected, you cannot import products - stock update fails. You must select a single shop to import it.

My internal checklist for this PR:

  • Product import logic moved to adapters
  • Category import logic moved to adapters

Futher issues to be solved in additional PRs:

  • Combinations import logic moved to adapters
  • Customers import logic moved to adapters
  • Addresses import logic moved to adapters
  • Manufacturers import logic moved to adapters
  • Suppliers import logic moved to adapters
  • Aliases import logic moved to adapters
  • Store contacts import logic moved to adapters
  • Supply orders import logic moved to adapters
  • Supply orders details import logic moved to adapters
  • Check this PR: #11715
  • Unit tests
  • Integration tests
  • Run code sniffer
  • Write doc about internal and external behavior of the import process

This change is Reviewable

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Nov 29, 2018

Hi @matks,

This is one of the two PR discussed here: #11160 (comment)
the goal is to migrate the import logic away from legacy controller into adapters.

At the moment in this PR only product import logic is migrated, but I'm working on migrating other import types as well.
A review would be handy, so I could fix the issues before proceeding with other import types 🙂

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Nov 29, 2018

Btw the PR is huge, but mostly because of the amount of logic in one adapter (ProductImportHandler), I didn't change most of it, just copied over from the legacy controller, with some minor refactoring 🙂

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 29, 2018

@rokaszygmantas nice ! I will try to get a review today or tomorrow 😄

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 3, 2018

The internal list looks good 👍 can you add a step "write doc about internal and external behavior of the import process" ? This is necessary IMO 😉

Show resolved Hide resolved admin-dev/themes/new-theme/js/pages/import-data/Importer.js
Show resolved Hide resolved admin-dev/themes/new-theme/js/pages/import-data/Importer.js Outdated
Show resolved Hide resolved src/Adapter/Converter/ExcelToCsvFileConverter.php Outdated
Show resolved Hide resolved src/PrestaShopBundle/Resources/config/services/adapter/data_provider.yml
rewind($handle);
if (($bom = fread($handle, 3)) != "\xEF\xBB\xBF") {
rewind($handle);

This comment has been minimized.

@matks

matks Dec 4, 2018

Contributor

Does calling rewing($handle) a 2nd time really makes a difference 🤔 ?

This comment has been minimized.

@rokaszygmantas

rokaszygmantas Dec 6, 2018

Author Contributor

It's a legacy logic, but I think if we don't rewind() the second time, the file pointer would be at 3rd position when the file doesn't contain BOM, meaning it would skip 3 characters from the beginning of the file. 🤔 because fread($handle, 3) already moves the file pointer.

Show resolved Hide resolved src/Adapter/Import/Handler/ImportHandlerFinder.php Outdated
Show resolved Hide resolved src/Adapter/Import/ImportEntityDeleter.php
Show resolved Hide resolved ...Admin/Configure/AdvancedParameters/ImportDataConfigurationController.php Outdated
Show resolved Hide resolved .../Resources/config/routing/admin/configure/advanced_parameters/import.yml
Show resolved Hide resolved tests/Unit/Core/Image/Deleter/ImageFileDeleterTest.php
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 4, 2018

Hi @rokaszygmantas ! Code review is done. Sorry it was late, computer crashed on Friday 😬

While reviewing I understood a lot of things:

  1. Thank you for handling the migration of Import feature, it is VERY complex and hard to read. Must have been tough for you.
  2. The Import feature is complex and has a lot of bad code in it. RIght now we are migrating it but I can see that this is a flaw for PrestaShop. It would require a complete re-write one day (not today, and not for you though). For example there are several logic statements which are duplicated (how to create a product, how to create a category, how do images are being stored, SQL table names ...) although it should be functions reused between the Import page and the CRUD pages (there should be a single way to create a category, reused everywhere). Currently, if we modify one of these be items in PrestaShop the Import process will crash.
  3. So we are going to stay with this implementation for a while. In order to protect the field for future work, we'll need massive integration testing. See below.
  4. I reviewed the PR but it's very complex so I only mentioned small issues. The task is too big, for me and you, for a thorough review.

Integration testing

In order to allow a future rework, I would like that we setup a dedicated test folder in tests/Integration. We can call it simply tests/Integration/Import. In this folder, we must use one testing system (can be Behat, can be Phpunit, can be anything in fact) that will do the following:

  • boot/setup required PrestaShop items for the Import feature to work (might be the whole SF application if needed or if it makes things simpler, see #11560 for example)
  • take a bunch of CSV import samples, and for each of them perform the import and check the output

So this would be "black box" tests: setup system, set an initial state, apply change (here, a CSV import being processed), check final test, reset initial state, repeat with next sample.

What do you think about it ?

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Dec 6, 2018

I agree with the idea of integration tests :) added it to the list.

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented Dec 10, 2018

Hello,

I advise you to split this big tasks into multiple pull requests or the risk will be that no one will be easy to merge it.

How about split each checkbox to a dedicated issue and pull request?

Mickaël

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Dec 11, 2018

Hi @mickaelandrieu, thanks for suggestion!

I would really like to split it, because the PR is already enormous big. But should I wait for each PR to be merged before opening another, or what could be the strategy?
The problem is that a lot of the code would need to be shared across these pull requests.

@Quetzacoalt91 Quetzacoalt91 referenced this pull request Dec 11, 2018

Closed

PHP 7+ error fix #11715

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Dec 13, 2018

@matks I made some fixes starting from commit b18683c :)

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 13, 2018

OK, we're going to merge this PR and start another

  • up-to-date code review 👍
  • need to rebase first
  • then QA should check it works 😉

@rokaszygmantas rokaszygmantas force-pushed the rokaszygmantas:migration/import_logic branch from 6af08b2 to 39ea91c Dec 13, 2018

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Dec 13, 2018

rebased.

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Dec 13, 2018

btw assets are not rebuilt in this PR, but I was told that it's no longer needed to include built assets in PRs :)

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Dec 13, 2018

@rokaszygmantas yes indeed
QA team can build them, however we need to tell them in the PR description (done)
and we'll build them only after we got their approval (to avoid this PR to be blocked by a assets rebase)

@rokaszygmantas rokaszygmantas force-pushed the rokaszygmantas:migration/import_logic branch from 89a90ec to c8ec093 Jan 16, 2019

@rokaszygmantas

This comment has been minimized.

Copy link
Contributor Author

rokaszygmantas commented Jan 16, 2019

@matks, I've done another rebase and ran the CS fixer as well, so your review got dismissed. 🙂

@rokaszygmantas rokaszygmantas changed the title [WIP] Migrate legacy import logic to adapters Migrate legacy import logic to adapters Jan 16, 2019

@matks

matks approved these changes Jan 16, 2019

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 16, 2019

@rokaszygmantas got it, finally 😄

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Jan 16, 2019

@matks matks changed the title Migrate legacy import logic to adapters Migrate legacy import logic to adapters and hide Import page 2 Jan 16, 2019

@matks matks merged commit 66804e0 into PrestaShop:develop Jan 16, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rokaszygmantas rokaszygmantas deleted the rokaszygmantas:migration/import_logic branch Jan 16, 2019

@PierreRambaud PierreRambaud added this to the 1.7.6.0 milestone Jan 22, 2019

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