Refacto AdminImportControllerCore #8122

LittleBigDev · 2017-07-11T10:06:35Z

Questions	Answers
Branch?	develop
Description?	Refactoring AdminImportControllerCore code.
Type?	improvement
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?	N/A
How to test?	N/A

This change is

prestonBot · 2017-07-11T10:06:37Z

Hello LittleBigDev!

This is your first pull request on the PrestaShop project. Thank you, and welcome to this Open Source community!

LittleBigDev · 2017-07-11T10:23:21Z

To reviewers : bug was fixed by return $line_count in AdminImportController.php line 3701. Everything else is code cleaning / refactoring.

LittleBigDev · 2017-07-11T13:39:29Z

Fixed :)

Quetzacoalt91

Good idea to refacto / clean, all comments are made regarding this task.

However, some changes were not easy to understand. In my opinion, we need more tests prior merge.

Quetzacoalt91 · 2017-07-11T13:52:40Z

controllers/admin/AdminImportController.php

-            if (!(AdminImportController::copyImg($store->id, null, $store->image, 'stores', !$regenerate))) {
-                $this->warnings[] = $store->image.' '.$this->trans('cannot be copied.', array(), 'Admin.Advparameters.Notification');
+        if (!empty($store->image)) {
+            $imgWasCopied = AdminImportController::copyImg($store->id, null, $store->image, 'stores', !$regenerate);


If the value is not used on another line, no need to store it in a variable. The previous version was correct in my opinion.

It was done to enhance readability of the code. AdminImportController::copyImg() return value was unclear.

Quetzacoalt91 · 2017-07-11T13:56:28Z

controllers/admin/AdminImportController.php

+            $imgWasCopied = AdminImportController::copyImg($store->id, null, $store->image, 'stores', !$regenerate);
+            if (!$imgWasCopied) {
+                $this->warnings[] = $store->image . ' '
+                    . $this->trans('cannot be copied.', [], 'Admin.Advparameters.Notification');


ping @AlexEven

If we mix refacto and bug fix in this PR, it would be great to make $store->image as a variable in the translated message. It may be not available on the beginning of the string in some language. However, this means we cannot merge it for 1.7.2.x if 1.7.2.0 is released.

I confirm that we'd rather have
'%1 cannot be copied (or some equivalent)
than
$img . ' ' . ' cannot be copied'.

And yeah, if the PR introduces string changes, it's kinda late in the 1.7.2.x game, and might force the PR to be moved to 1.7.3.x...

I'd prefer too but it would be outside BOOM-294's scope.
I'm ok to fix this in another PR (this is not the only one that would need a fix).

Quetzacoalt91 · 2017-07-11T14:06:20Z

controllers/admin/AdminImportController.php

+        $storeId = isset($info['id']) ? (int)$info['id'] : null;
+        $store   = new Store($storeId);
+
+        $store->force_id = (bool)$forceIds;


What happen for $force_ids and Store::existsInDatabase(...)?

You should explain why you changed the tests, because after a quick look this is unclear. If we want to refacto it without modifying the test cases, I would have written i.e:

if (!empty($info['id']) && ($force_ids || Store::existsInDatabase((int)$info['id'], 'store'))) { $storeId = (int)$info['id']; } else { $storeId = null; }

All this code block is redundant with Store and ObjectModel constructors' logic :

passing empty or unknown id will lead to an unloaded Store object

$force_ids is used only when Store::add() is executed and has no meaning when we need to decide whether we pass an id to the constructor or not.

Quetzacoalt91 · 2017-07-11T14:10:55Z

controllers/admin/AdminImportController.php

+
+        // If errors, log them and stop execution
+        if (true !== $fieldsValidationResult || true !== $langFieldsValidationResult) {
+            $errorGenericMessage = $this->trans('Store is invalid', [], 'Admin.Advparameters.Notification');


[] --> array()

Quetzacoalt91 · 2017-07-11T14:12:33Z

controllers/admin/AdminImportController.php

                );
-            }
-        } else {
-            $this->errors[] = $this->trans('Store is invalid', array(), 'Admin.Advparameters.Notification').' ('.$store->name.')';


@AlexEven, aren't we supposed to add ('.$store->name.') in the translated string as a variable? This could be the occasion to update this.

Too late for 1.7.2 :(

~~I think so. Should I change this couple of translations in this PR ?~~
Edit : according to Maxime's comment, I won't fix it here.

Quetzacoalt91 · 2017-07-11T14:15:10Z

controllers/admin/AdminImportController.php

+        // If nothing worked, log an error
+        if (!$res) {
+            $this->errors[] = Db::getInstance()->getMsgError() . ' '
+                . sprintf(
                    $this->trans('%1$s (ID: %2$s) cannot be saved', array(), 'Admin.Advparameters.Notification'),


While we refacto stuff, it would be great to not use sprintf for a translated string, but insert $info['name'] and ($store->id ? $store->id : 'null') in the second parameter of trans instead

In progress...

Quetzacoalt91 · 2017-07-11T14:16:59Z

controllers/admin/AdminImportController.php

+
+        // If store doesn't exist or update failed
+        if (!$res) {
+            $store->force_id = (bool)$forceIds;


Seems to be a duplicate of line 3724

Yes I forgot to remove it here.

Quetzacoalt91 · 2017-07-11T14:19:57Z

controllers/admin/AdminImportController.php

+
+        $res = false;
+        if ($store->storeExists($store->id)) {
+            $res = $store->update();


Would save() help you? It handles both update() and add() whether it exists or not.

Yes it would. Should I fix this ?

I had a look at ObjectModel::save() method and behavior is slightly different.
ObjectModel::save() only checks if an id is present and if so, ObjectModel::update() is called.
Our code explicitely checks in DB if this is a known id. I'm ok to use ObjectModel::save(), but I need your feedback before.

aleeks · 2017-07-12T15:09:51Z

@LittleBigDev I put your PR in develop branch, we can just include this one to 1.7.2.x now.
Can you create a PR in 1.7.2.x with just the fix please ?

Edit: you need to rebase this one 😄 too

eternoendless

Thanks for this refacto! There are many little things to fix :)

eternoendless · 2017-07-18T08:52:24Z

classes/Manufacturer.php

@@ -64,6 +64,8 @@ class ManufacturerCore extends ObjectModel
    /** @var bool active */
    public $active;

+    public $shop;


Please don't forget to add PHPDoc if you know what this property contains.

Fixed in next commit.

eternoendless · 2017-07-18T08:55:55Z

controllers/admin/AdminImportController.php

+                            'Attribute (Name:Type:Position)',
+                            array(),
+                            'Admin.Advparameters.Feature'
+                        ) . '*',


Symfony standards say:

Add a single space around binary operators (==, &&, ...), with the exception of the concatenation (.) operator;

So unfortunately you need to remove the spaces around the . operator.

See: http://symfony.com/doc/current/contributing/code/standards.html#structure

eternoendless · 2017-07-18T09:01:07Z

controllers/admin/AdminImportController.php

@@ -559,12 +855,14 @@ public function __construct()
            }
        }

-        $this->separator = ($separator = Tools::substr(strval(trim(Tools::getValue('separator'))), 0, 1)) ? $separator :  ';';
+        $separator = Tools::substr(strval(trim(Tools::getValue('separator'))), 0, 1);
+        $this->separator = $separator ? $separator :  ';';


Watch it, you wrote two spaces after :

Fixed in next commit.

eternoendless · 2017-07-18T09:02:15Z

controllers/admin/AdminImportController.php

        $this->convert = false;
-        $this->multiple_value_separator = ($separator = Tools::substr(strval(trim(Tools::getValue('multiple_value_separator'))), 0, 1)) ? $separator :  ',';
+        $separator = Tools::substr(strval(trim(Tools::getValue('multiple_value_separator'))), 0, 1);
+        $this->multiple_value_separator = $separator ? $separator :  ',';


Same thing here

Fixed in next commit.

eternoendless · 2017-07-18T09:07:18Z

controllers/admin/AdminImportController.php

@@ -652,14 +959,18 @@ public function renderForm()
        $last          = strtolower($post_max_size[strlen($post_max_size) - 1]);

        switch ($last) {
+
+            /** @noinspection PhpMissingBreakStatementInspection */


You shouldn't need this if your case ends with a comment

eternoendless · 2017-07-18T12:44:41Z

controllers/admin/AdminImportController.php

-			', false);
+            $datas = Db::getInstance()->getRow(
+                'SELECT p.`id_product` '
+                . 'FROM `' . _DB_PREFIX_ . 'product` p ' . Shop::addSqlAssociation('product', 'p')


Leading space before FROM

eternoendless · 2017-07-18T13:00:12Z

controllers/admin/AdminImportController.php

+     * @param       $notUsed      : Not used anymore.
+     * @param bool  $regenerate   : If images should be regenerated
+     * @param bool  $forceIds     : If passed store id should be used for store creation
+     * @param bool  $validateOnly : If set to true, store will not be updated nor created with $info data


When writing PHPDoc, please don't use a separator between the variable name and the description

eternoendless · 2017-07-18T15:21:57Z

controllers/admin/AdminImportController.php

-                        $this->errors[] = ($field_error !== true ? $field_error : '').(isset($lang_field_error) && $lang_field_error !== true ? $lang_field_error : '').
-                            Db::getInstance()->getMsgError();
+                    if ($fieldsValidationResult !== true
+                        || isset($langFieldsValidationResult) && $langFieldsValidationResult !== true


Please wrap isset($langFieldsValidationResult) && $langFieldsValidationResult !== true with parentheses for clarity.

&& is prevalent on ||

I created a new line for this group

but ok, I'll change this in next commit.

eternoendless · 2017-07-18T15:28:15Z

controllers/admin/AdminImportController.php

@@ -4078,7 +5147,8 @@ protected function supplyOrdersDetailsImportOne($info, &$products, &$reset, $for
 					IFNULL(pa.upc, IFNULL(p.upc, \'\')) as upc
 				');
                $query->from('product', 'p');
-                $query->leftJoin('product_attribute', 'pa', 'pa.id_product = p.id_product AND id_product_attribute = '.(int)$id_product_attribute);
+                $query->leftJoin('product_attribute', 'pa', 'pa.id_product = p.id_product AND id_product_attribute = '


If the line is too long, I'd prefer if it was broken up by arguments (one per line).

eternoendless · 2017-07-18T15:30:43Z

controllers/admin/AdminImportController.php

@@ -4224,7 +5294,8 @@ protected function truncateTables($case)
                Db::getInstance()->execute('TRUNCATE TABLE `'._DB_PREFIX_.'specific_price_priority`');
                Db::getInstance()->execute('TRUNCATE TABLE `'._DB_PREFIX_.'product_carrier`');
                Db::getInstance()->execute('TRUNCATE TABLE `'._DB_PREFIX_.'cart_product`');
-                if (count(Db::getInstance()->executeS('SHOW TABLES LIKE \''._DB_PREFIX_.'favorite_product\' '))) { //check if table exist
+                //check if table exist before truncate


"table exists" 😉

aleeks · 2017-07-19T14:45:47Z

classes/Product.php

@@ -52,6 +52,9 @@ class ProductCore extends ObjectModel
    /** @var int default Category id */
    public $id_category_default;

+    /** @var array */


Hello @LittleBigDev
I've no idea of the context, just an alert, is it really array ?

Yes it is. Have a look at this line in AdminImportController.

maximebiloe · 2017-07-24T16:51:30Z

controllers/admin/AdminImportController.php

-                        'help' => $this->trans('Ignore this field if you don\'t use the Multistore tool. If you leave this field empty, the default shop will be used.', array(), 'Admin.Advparameters.Help'),
+                        'help' => $this->trans(
+                            'Ignore this field if you don\'t use the Multistore tool. If you leave this field empty, '
+                                . 'the default shop will be used.',


You can't split a translatable string like that, the parser will fail.
Please, remove the concatenation even if you did it for the line length.

maximebiloe · 2017-07-25T06:54:07Z

controllers/admin/AdminImportController.php

                    'meta_title' => array('label' => $this->trans('Meta title', array(), 'Admin.Global')),
                    'meta_keywords' => array('label' => $this->trans('Meta keywords', array(), 'Admin.Global')),
                    'meta_description' => array('label' => $this->trans('Meta description', array(), 'Admin.Global')),
                    'image' => array('label' => $this->trans('Image URL', array(), 'Admin.Advparameters.Feature')),
                    'shop' => array(
                        'label' => $this->trans('ID / Name of group shop', array(), 'Admin.Advparameters.Feature'),
-                        'help' => $this->trans('Ignore this field if you don\'t use the Multistore tool. If you leave this field empty, the default shop will be used.', array(), 'Admin.Advparameters.Help'),
+                        'help' => $this->trans(
+                            'Ignore this field if you don\'t use the Multistore tool. If you leave this field empty, '


You can't split a translatable string like that, the parser will fail.
Please, remove the concatenation even if you did it for the line length.

maximebiloe · 2017-07-25T06:54:31Z

controllers/admin/AdminImportController.php

                    'image' => array('label' => $this->trans('Image URL', array(), 'Admin.Advparameters.Feature')),
                    'shop' => array(
                        'label' => $this->trans('ID / Name of shop', array(), 'Admin.Advparameters.Feature'),
-                        'help' => $this->trans('Ignore this field if you don\'t use the Multistore tool. If you leave this field empty, the default shop will be used.', array(), 'Admin.Advparameters.Help'),
+                        'help' => $this->trans(
+                            'Ignore this field if you don\'t use the Multistore tool. If you leave this field empty, '


You can't split a translatable string like that, the parser will fail.
Please, remove the concatenation even if you did it for the line length.

maximebiloe · 2017-07-25T06:59:41Z

controllers/admin/AdminImportController.php

-					<code>php_value post_max_size 20M</code> '.
-                    $this->trans('(click to open "Generators" page)', array(), 'Admin.Advparameters.Notification').'</a>';
+                    $uploadErrorMessage = $this->trans(
+                        'The uploaded file exceeds the post_max_size directive in php.ini. If your server '


You can't split a translatable string like that, the parser will fail.
Please, remove the concatenation even if you did it for the line length.

Quetzacoalt91

It stopped during the review to avoid spam.

It would be great to give names to the placeholders in translated strings. This is important to give a context to the translator, and may help us to remove the duplicated strings (started in #7840).

Quetzacoalt91 · 2017-08-30T16:43:22Z

controllers/admin/AdminImportController.php

            'available_fields' => $this->getAvailableFields(),
-            'truncateAuthorized' => (Shop::isFeatureActive() && $this->context->employee->isSuperAdmin()) || !Shop::isFeatureActive(),
+            'truncateAuthorized' => !(Shop::isFeatureActive()) || $this->context->employee->isSuperAdmin(),


Parentheses around Shop::isFeatureActive() can be removed

Quetzacoalt91 · 2017-08-30T16:58:03Z

controllers/admin/AdminImportController.php

+                || $category->parent == $categoryId
+            ) {
+                $this->errors[] = $this->trans('The category ID must be unique. It can\'t be the same as the one for the parent category (ID: %1$s).',
+                    array($categoryId ? $categoryId : 'null'),


When using a placeholder with the Symfony translator, your must name them (https://symfony.com/doc/current/components/translation/usage.html#message-placeholders). Then you affect this name a variable to be replaced in the translated string.

Here is the correct syntax to translate this string:

$this->errors[] = $this->trans('The category ID must be unique. It can\'t be the same as the one for the parent category (ID: %id%).', array('%id%' => ($categoryId ? $categoryId : 'null')), 'Admin.Advparameters.Notification' );

Quetzacoalt91 · 2017-08-30T16:58:45Z

controllers/admin/AdminImportController.php

            }
            $category->id_parent = $category->parent;
        } elseif (isset($category->parent) && is_string($category->parent)) {
            // Validation for parenting itself
            if ($validateOnly && isset($category->name) && ($category->parent == $category->name)) {
-                $this->errors[] = $this->trans(
-                    'A category can\'t be its own parent. You should rename it (current name: %1$s).',
+                $this->errors[] = $this->trans('A category can\'t be its own parent. You should rename it (current name: %1$s).',


Same comment about translation placeholder. Please use a variable name surrounded by %% an add it in the second parameter.

Quetzacoalt91 · 2017-08-30T17:02:22Z

controllers/admin/AdminImportController.php

-                        );
+                        $this->errors[] = $this->trans('%1$s (ID: %2$s) cannot be saved', array(
+                            $categoryToCreate->name[$idLang],
+                            !empty($categoryToCreate->id) ? $categoryToCreate->id : 'null',


Same comment about translation placeholder. Please use a variable name surrounded by %% an add it in the second parameter.

Quetzacoalt91 · 2017-08-30T17:02:37Z

controllers/admin/AdminImportController.php

            if ($category->link_rewrite == '') {
                $category->link_rewrite = 'friendly-url-autogeneration-failed';
-                $this->warnings[] = sprintf($this->trans('URL rewriting failed to auto-generate a friendly URL for: %s', array(), 'Admin.Advparameters.Notification'), $category->name[$default_language_id]);
+                $this->warnings[] = $this->trans('URL rewriting failed to auto-generate a friendly URL for: %s',
+                    array($category->name[$defaultLanguageId]),


Same comment about translation placeholder. Please use a variable name surrounded by %% an add it in the second parameter.

Quetzacoalt91 · 2017-08-30T17:17:20Z

controllers/admin/AdminImportController.php

-                                    $category_to_create->name[$default_language_id],
-                                    (isset($category_to_create->id) && !empty($category_to_create->id))? $category_to_create->id : 'null'
-                                );
+                                $this->errors[] = $this->trans('%1$s (ID: %2$s) cannot be saved', array(


Same comment about translation placeholder. Please use a variable name surrounded by %% an add it in the second parameter.

Quetzacoalt91 · 2017-08-30T17:17:33Z

controllers/admin/AdminImportController.php

                    if ($category['id_category']) {
                        $product->id_category[] = (int)$category['id_category'];
                    } else {
-                        $this->errors[] = $this->trans(
-                            '%s cannot be saved',
+                        $this->errors[] = $this->trans('%s cannot be saved',


Same comment about translation placeholder. Please use a variable name surrounded by %% an add it in the second parameter.

Quetzacoalt91 · 2017-08-30T17:18:46Z

controllers/admin/AdminImportController.php

-                                'Invalid tag(s) (%s)',
+                        $isTagAdded = Tag::addTags($key, $product->id, $str, $this->multiple_value_separator);
+                        if (!$isTagAdded) {
+                            $this->addProductWarning(Tools::safeOutput($productData['name']), (int)$product->id, $this->trans('Invalid tag(s) (%s)',


Same comment about translation placeholder. Please use a variable name surrounded by %% an add it in the second parameter.

Quetzacoalt91 · 2017-08-30T17:18:58Z

controllers/admin/AdminImportController.php

                                $image->delete();
-                                $this->warnings[] = sprintf($this->trans('Error copying image: %s', array(), 'Admin.Advparameters.Notification'), $url);
+                                $this->warnings[] = $this->trans('Error copying image: %s',


Same comment about translation placeholder. Please use a variable name surrounded by %% an add it in the second parameter.

Quetzacoalt91 · 2017-08-30T17:19:03Z

controllers/admin/AdminImportController.php

@@ -2116,7 +2789,10 @@ protected function productImportOne($info, $default_language_id, $id_lang, $forc
                    }

                    if ($error) {
-                        $this->warnings[] = sprintf($this->trans('Product #%1$d: the picture (%2$s) cannot be saved.', array(), 'Admin.Advparameters.Notification'), $image->id_product, $url);
+                        $this->warnings[] = $this->trans('Product #%1$d: the picture (%2$s) cannot be saved.', array(


Same comment about translation placeholder. Please use a variable name surrounded by %% an add it in the second parameter.

AlexEven · 2017-09-05T10:19:49Z

controllers/admin/AdminImportController.php

-                    'lastname' => array('label' => $this->trans('Last name', array(), 'Admin.Global').'*'),
-                    'firstname' => array('label' => $this->trans('First name ', array(), 'Admin.Global').'*'),
-                    'address1' => array('label' => $this->trans('Address', array(), 'Admin.Global').'*'),
+                    'lastname' => array('label' => $this->trans('Last name', array(), 'Admin.Global') . '*'),


How does it behave in RTL?

Maybe @maximebiloe could answer this ?

Not as expected. I found several part of text which needs to be updated (in another PR).

AlexEven · 2017-09-05T10:30:44Z

controllers/admin/AdminImportController.php

+                    !$regenerate
+                )) {
+                    // TODO : insert translation placeholder here
+                    $this->warnings[] = $manufacturer->image . ' ' . $this->trans('cannot be copied.',


We need a variable here too

AlexEven · 2017-09-05T10:32:13Z

controllers/admin/AdminImportController.php

+            $imgWasCopied = AdminImportController::copyImg($store->id, null, $store->image, 'stores', !$regenerate);
+            if (!$imgWasCopied) {
+                $this->warnings[] = $store->image . ' '
+                    . $this->trans('cannot be copied.', array(), 'Admin.Advparameters.Notification');


This needs to be a full sentence with a variable.

LittleBigDev · 2017-09-05T13:35:40Z

Pushed fixes according to review comments.

antoin-m · 2017-09-05T13:53:33Z

xBorderie · 2017-09-05T13:55:33Z

mickaelandrieu · 2017-09-19T13:08:11Z

Hello @LittleBigDev,

this need to be rebased (and probably validated again by @Quetzacoalt91 ?)

Mickaël

Quetzacoalt91 · 2017-10-16T11:19:39Z

controllers/admin/AdminImportController.php

+                        . $generatorsPageMessage
+                        . '</a>';
+
+                    $_FILES['file']['error'] = $uploadErrorMessage . $directiveExample;


Ach, I didn't see this line. For the RTL languages, this will need to be merged in a single string to translate (in another PR).

Quetzacoalt91 · 2017-10-16T11:21:15Z

controllers/admin/AdminImportController.php

+    /**
+     * Initializes import toolbar (for import display only)
+     *
+     * @return void


Line to be removed

Quetzacoalt91 · 2017-10-18T08:26:23Z

controllers/admin/AdminImportController.php

-                    'lastname' => array('label' => $this->trans('Last name', array(), 'Admin.Global').'*'),
-                    'firstname' => array('label' => $this->trans('First name ', array(), 'Admin.Global').'*'),
-                    'address1' => array('label' => $this->trans('Address', array(), 'Admin.Global').'*'),
+                    'lastname' => array('label' => $this->trans('Last name', array(), 'Admin.Global') . '*'),


Not as expected. I found several part of text which needs to be updated (in another PR).

Quetzacoalt91 · 2017-10-19T09:33:33Z

controllers/admin/AdminImportController.php

+        // If nothing to fill, just get out ("0" string is NOT nothing).
+        if (empty($data) && $data !== '0') {
+            return true;
+        }


It seems the first check was made before to handle the multilang case, and now this is an check which returns if the value is empty or equals 0.

Did you test that part? I'm afraid that would have some consequence on the feature.

I xDebugged it, and in any case, when data is empty, nothing is filled

Quetzacoalt91 · 2017-10-19T09:40:37Z

controllers/admin/AdminImportController.php

+            Configuration::get('PS_HOME_CATEGORY'),
+            Configuration::get('PS_ROOT_CATEGORY')
+        );
+        $categoryId     = !empty($info['id']) ? (int)$info['id'] : 0;


By looking at the next lines of this function, and the calls to new Category, I wonder if you should not set null if the $info['id'] is empty.

Quetzacoalt91 · 2017-10-19T10:03:53Z

controllers/admin/AdminImportController.php


-                if (!$shop_is_feature_active) {
+                if (!$shopIsFeatureActive) {


No, that's too much. ;)
But in that case, we have:

if ($shopIsFeatureActive) { [...] if (!$shopIsFeatureActive) { } }

Which cannot happen.

Removing this test and the line after ($info['shop'] = 1;) is enough

Quetzacoalt91 · 2017-10-19T10:07:05Z

controllers/admin/AdminImportController.php

+        $moreStep = 0
+    ) {
+        if (!is_array($crossStepsVariables)) {
+            $crossStepsVariables = array();


I'm okay with the suggested change.

But I wonder if array &$crossStepsVariables = array(), would work as well and would fit better to our need.

Quetzacoalt91 · 2017-10-19T10:35:08Z

controllers/admin/AdminImportController.php

+                        ) {
+                            $categoryToCreate->link_rewrite = AdminImportController::createMultiLangField(
+                                $categoryLinkRewrite
+                            );


Why is it now in the test?

Quetzacoalt91 · 2017-10-19T13:12:18Z

controllers/admin/AdminImportController.php

+            if ($taxRate < 0 || $taxRate > 100) {
+                $this->errors[] = $this->trans('Tax rate (%rate%) is not valid (at line %line%).', array(
+                    '%rate%' => $taxRate,
+                    '%line%' => $currentLine + 1,
                    $this->trans('Format: Between 0 and 100', array(), 'Admin.Advparameters.Notification')


In both case, it seems $this->trans('Format: Between 0 and 100', array(), 'Admin.Advparameters.Notification') is never displayed

…e naming.

- "if" statements arrangement - SQL query strings arrangement - php code sniffer warnings

I didn't refacto the visible symbols (public, static...)

…rans(" and the whole translated string need to be on the same line). PSR broken rules are intended.

LittleBigDev · 2017-10-20T14:36:43Z

Hi @Quetzacoalt91 , I tried to address all your comments but after the rebase, I wasn't able to find the related code and/or to understand your suggestions. Could you please check it again ?

EDIT : and maybe we should use Reviewable for such a big review ?

LittleBigDev changed the title ~~- Fixed bug where store contact data was not entirely imported (csv import feature)~~ Fixed bug where store contact data was not entirely imported (csv import feature) Jul 11, 2017

LittleBigDev requested review from maximebiloe and aleeks July 11, 2017 10:14

PrestaShop deleted a comment from prestonBot Jul 11, 2017

LittleBigDev closed this Jul 11, 2017

LittleBigDev reopened this Jul 11, 2017

Quetzacoalt91 reviewed Jul 11, 2017

View reviewed changes

marionf added the Waiting for QA Status: action required, waiting for test feedback label Jul 11, 2017

aleeks changed the base branch from 1.7.2.x to develop July 12, 2017 15:09

prestonBot added Waiting for wording Status: action required, waiting for wording Report on StarterTheme labels Jul 12, 2017

aleeks removed Report on StarterTheme Waiting for wording Status: action required, waiting for wording labels Jul 12, 2017

prestonBot added the Report on StarterTheme label Jul 12, 2017

aleeks removed the Report on StarterTheme label Jul 12, 2017

LittleBigDev changed the title ~~Fixed bug where store contact data was not entirely imported (csv import feature)~~ Refacto AdminImportControllerCore Jul 12, 2017

prestonBot added Waiting for wording Status: action required, waiting for wording Report on StarterTheme labels Jul 12, 2017

aleeks removed Report on StarterTheme Waiting for wording Status: action required, waiting for wording labels Jul 12, 2017

LittleBigDev changed the title ~~Refacto AdminImportControllerCore~~ WIP: Refacto AdminImportControllerCore Jul 13, 2017

prestonBot added the Waiting for wording Status: action required, waiting for wording label Jul 13, 2017

eternoendless previously requested changes Jul 18, 2017

View reviewed changes

aleeks reviewed Jul 19, 2017

View reviewed changes

maximebiloe suggested changes Jul 24, 2017

View reviewed changes

maximebiloe previously requested changes Jul 25, 2017

View reviewed changes

Quetzacoalt91 requested changes Aug 30, 2017

View reviewed changes

AlexEven reviewed Sep 5, 2017

View reviewed changes

AlexEven removed the Waiting for wording Status: action required, waiting for wording label Sep 5, 2017

LittleBigDev dismissed toutantic’s stale review via 08e3e5f September 5, 2017 13:35

PrestaShop deleted a comment from eternoendless Oct 19, 2017

PrestaShop deleted a comment from LittleBigDev Oct 19, 2017

Quetzacoalt91 reviewed Oct 19, 2017

View reviewed changes

prestonBot added the Waiting for wording Status: action required, waiting for wording label Oct 19, 2017

LouiseBonnard removed the Waiting for wording Status: action required, waiting for wording label Oct 19, 2017

LittleBigDev added 12 commits October 19, 2017 17:25

BO: Some refactoring into store contacts csv import code

7ef166b

CO: PSR compliancy on AdminImportController; lines length and variabl…

0817510

…e naming.

CO: Refactored placeholders usage in translated keys (removed sprintf)

cec30bd

CO: code refactoring + fixed mistakes and refacto oversights

1c1ed9a

CO: code refactoring

4e8686e

- "if" statements arrangement - SQL query strings arrangement - php code sniffer warnings

CO: PSR compliancy (variables naming)

558a1c3

I didn't refacto the visible symbols (public, static...)

Fixed PR according to @eternoendless advice

50054ed

CO: Reverted some refactoring for the sake of translations parser ("t…

e39bcbf

…rans(" and the whole translated string need to be on the same line). PSR broken rules are intended.

BO: AdminImportController refacto fixes after review.

5094d06

BO: AdminImportController refacto fixes after review.

bd9e4b1

Fixed rebase conflicts

9d15b5b

Fixed rebase conflicts

4999fdd

LittleBigDev closed this Nov 7, 2017

LittleBigDev deleted the BOOM-294 branch November 7, 2017 16:05

Refacto AdminImportControllerCore #8122

Refacto AdminImportControllerCore #8122

Conversation

LittleBigDev commented Jul 11, 2017 • edited by eternoendless Loading

prestonBot commented Jul 11, 2017

LittleBigDev commented Jul 11, 2017

LittleBigDev commented Jul 11, 2017

Quetzacoalt91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleBigDev Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xBorderie Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleBigDev Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleBigDev Jul 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aleeks commented Jul 12, 2017 • edited Loading

eternoendless left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleBigDev Jul 19, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quetzacoalt91 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LittleBigDev commented Sep 5, 2017

antoin-m commented Sep 5, 2017

xBorderie commented Sep 5, 2017

mickaelandrieu commented Sep 19, 2017

Choose a reason for hiding this comment

LittleBigDev commented Jul 11, 2017 •

edited by eternoendless

Loading

LittleBigDev Jul 11, 2017 •

edited

Loading

xBorderie Jul 11, 2017 •

edited

Loading

LittleBigDev Jul 11, 2017 •

edited

Loading

LittleBigDev Jul 11, 2017 •

edited

Loading

aleeks commented Jul 12, 2017 •

edited

Loading

eternoendless left a comment •

edited

Loading

LittleBigDev Jul 19, 2017 •

edited

Loading