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

Fix default values on import page #11339

Merged
merged 5 commits into from Nov 28, 2018

Conversation

@tomlev
Copy link
Member

tomlev commented Nov 9, 2018

Questions Answers
Branch? 1.7.5.x
Description? Fix empty default values on import page
Type? bug fix
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? Fixes #10430
How to test? In BO > advanced parameters > import : default values for both separators should not be blank

This change is Reviewable

@@ -86,13 +86,15 @@ public function importAction(Request $request)
$finder = $this->get('prestashop.core.import.file_finder');
$iniConfiguration = $this->get('prestashop.core.configuration.ini_configuration');
/* NO_PROD

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 9, 2018

Contributor

useless NO_PROD, remove this comment.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 9, 2018

I'm sorry I think I miss something here 😥
If the issue to to have default data on 2 fields, why not just add them in the Form Type like matks@9d05e8f ?

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Nov 20, 2018

The modification @matks suggests seems more reasonable for a dimple bug fix it it does the job
This PR introduces a lot of modifications, and I'm not fan of injecting RequestStack in the FormDataProvider, we are twisting the form system here as it was not designed to fetch data from session, request, ...

{
$this->session = $session;
$this->importFileFinder = $importFileFinder;
$this->request = $requestStack->getCurrentRequest();

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 21, 2018

Contributor

import "import_type" only (and allow null value).

The yaml declaration is the following: "@=service('request_stack').getCurrentRequest().get('import_type')"

@@ -148,7 +148,7 @@ services:
- 'LogsPage'

prestashop.admin.import.form_handler:
class: 'PrestaShop\PrestaShop\Core\Form\FormHandler'
class: 'PrestaShop\PrestaShop\Core\Form\ImportPageFormHandler'

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 21, 2018

Contributor

btw it's totally OK to write its own Form Handler!

  • Simple use cases => FormHandler
  • Specific use cases => MyOwnFormHandler
@tomlev

This comment has been minimized.

Copy link
Member

tomlev commented Nov 27, 2018

This PR aims to be a subset of this one: #10120
I know some hanges are overkill alone in this PR scope, but they will not be source to so much conflicts when merging the big one
@mickaelandrieu @matks @jolelievre

@jolelievre

This comment has been minimized.

Copy link
Contributor

jolelievre commented Nov 27, 2018

alright for me, if it's part of an already merged PR it reduces the risk of any problem
although it's overkill compared to what Mathieu proposed I guess we can merge it
This juste leaves the modif @mickaelandrieu requested, the request should never be injected (if it can be avoided)
But I guess we will have to change the develop branch as well then

Feedback will be done in develop branch as stated by @jolelievre

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 28, 2018

All right I agree with @tomlev 😃 moving to QA phase now

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 28, 2018

Also Travis wants to apply lint here 🤔
Ping @tomlev 😉

@marionf marionf self-assigned this Nov 28, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Nov 28, 2018

@marionf marionf removed their assignment Nov 28, 2018

@Quetzacoalt91 Quetzacoalt91 force-pushed the tomlev:issue-10430 branch from 20b1e95 to 40bcc2a Nov 28, 2018

@Quetzacoalt91

This comment has been minimized.

Copy link
Member

Quetzacoalt91 commented Nov 28, 2018

Fixed Travis builds by runing cs fixer

@PierreRambaud PierreRambaud added this to the 1.7.5.0 milestone Nov 28, 2018

@PierreRambaud PierreRambaud merged commit 45b0112 into PrestaShop:1.7.5.x Nov 28, 2018

1 check passed

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

This comment has been minimized.

Copy link
Contributor

PierreRambaud commented Nov 28, 2018

Thanks @tomlev @Quetzacoalt91 and @marionf for the review.

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