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

Form handling improvements in import page step 1 and 2 #11160

Merged
merged 5 commits into from Nov 12, 2018

Conversation

Projects
None yet
5 participants
@rokaszygmantas
Contributor

rokaszygmantas commented Oct 26, 2018

Questions Answers
Branch? develop
Description? Form handling improvements (not visible).
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? no
How to test? No QA needed yet, since the new import page is hidden and some additional improvements will come in the near future.

This change is Reviewable

$errors[] = [
'key' => 'To proceed, please upload a file first.',
'domain' => 'Admin.Advparameters.Notification',
'parameters' => [],
];
} else {
$this->session->set('csv', $data['csv']);

This comment has been minimized.

@rokaszygmantas

rokaszygmantas Oct 26, 2018

Contributor

@matks that's the only service in which session is injected and used now, but it is required in order to preselect the configuration values if user goes back from import step 2 to step 1. Is that reasonable? :)

This comment has been minimized.

@matks

matks Nov 2, 2018

Contributor

It is reasonable 😄

However I'm curious: I see you introduced ImportFormDataProviderInterface. That is a good idea because this feature is so complex and specific. But since you created an Interface for this case, why not inject the session in getData() and setData() ?

This comment has been minimized.

@rokaszygmantas

rokaszygmantas Nov 5, 2018

Contributor

Well the interface does not limit a developer to use the session, it's only this particular implementation that requires the session.
There already are two different import form data providers, one for step 1 (ImportFormDataProvider) and one for step 2 (ImportDataConfigurationFormDataProvider) and both implement the same ImportFormDataProviderInterface, while only one of them depends on the session.

@rokaszygmantas

This comment has been minimized.

Contributor

rokaszygmantas commented Oct 26, 2018

Hi @matks!

This is the first PR with import page improvements that we discussed on #10120.

I've refactored mostly the form handling part of both import step 1 and step 2.

There will be an additional PR at some point that implements CQRS for saving import data matches, now it's using legacy logic to save it (I mean this part: https://github.com/PrestaShop/PrestaShop/pull/11160/files#diff-f7f41ac7afb236175ee151c49f7359e6R66).

Edit: Actually I see at least two additional PRs coming after this:

  1. CQRS implementation for data matches.
  2. Import logic moved away from legacy controller into adapters.
$errors[] = [
'key' => 'To proceed, please upload a file first.',
'domain' => 'Admin.Advparameters.Notification',
'parameters' => [],
];
} else {
$this->session->set('csv', $data['csv']);

This comment has been minimized.

@matks

matks Nov 2, 2018

Contributor

It is reasonable 😄

However I'm curious: I see you introduced ImportFormDataProviderInterface. That is a good idea because this feature is so complex and specific. But since you created an Interface for this case, why not inject the session in getData() and setData() ?

/**
* Interface ImportFormDataProviderInterface describes a data provider for import forms.
*/
interface ImportFormDataProviderInterface

This comment has been minimized.

@matks

matks Nov 2, 2018

Contributor

It's good to have this interface 👍 this feature is so complex, it is relevant to have this specific interface for it

*/
final class ImportDataConfigurationFormHandler implements FormHandlerInterface
class ImportFormHandler implements ImportFormHandlerInterface

This comment has been minimized.

@matks

matks Nov 2, 2018

Contributor

We might merge this with a FormHandler for CRUD pages later, but it does not exist yet. Let's keep it in mind.

defaults:
_controller: 'PrestaShopBundle:Admin\Configure\AdvancedParameters\Import:showImportData'
_legacy_controller: AdminImport
# This redirect prevents an error (redirects to import first page instead) when

This comment has been minimized.

@matks

matks Nov 2, 2018

Contributor

👍

@matks

This comment has been minimized.

Contributor

matks commented Nov 2, 2018

@rokaszygmantas Review done :) only minor issues

Agreed with the 2 incoming PRs: this is a good split of the workload 👍

@rokaszygmantas

This comment has been minimized.

Contributor

rokaszygmantas commented Nov 7, 2018

@matks thanks for review! I've left an answer to one of your comments, but other than that I didn't see any change requests, so waiting for your response or approval 🙂

@rokaszygmantas rokaszygmantas force-pushed the rokaszygmantas:migration/import_improvements branch from 350345b to 00595b8 Nov 8, 2018

@matks

matks approved these changes Nov 12, 2018

@matks

This comment has been minimized.

Contributor

matks commented Nov 12, 2018

@rokaszygmantas you're right, sorry 😅 I did not see at the end of the review that there was actually no requested change

@matks matks merged commit 81da643 into PrestaShop:develop Nov 12, 2018

2 checks passed

Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@rokaszygmantas rokaszygmantas deleted the rokaszygmantas:migration/import_improvements branch Nov 12, 2018

@rokaszygmantas rokaszygmantas referenced this pull request Nov 29, 2018

Open

[WIP] Migrate legacy import logic to adapters #11561

2 of 16 tasks complete

@eternoendless eternoendless added this to the 1.7.6.0 milestone Dec 7, 2018

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