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

Improved Product catalogAction #8710

Merged
merged 25 commits into from Aug 20, 2018

Conversation

@mickaelandrieu
Contributor

mickaelandrieu commented Jan 28, 2018

Questions Answers
Branch? develop
Description? Refactoring of Product controller class
Type? improvement
Category? BO
BC breaks? no
Deprecations? no
Fixed ticket? http://forge.prestashop.com/browse/BOOM-2950
How to test? Product pages should work as expected, and probably better than before :)

Note this pull request start from this one => #8677 this PR is a replacement of #8677


This change is Reviewable

@mickaelandrieu mickaelandrieu changed the title from Improve catalog action to Improved Product catalogAction Jan 28, 2018

@PrestaShop PrestaShop deleted a comment from mickaelandrieu Mar 13, 2018

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Mar 20, 2018

Contributor

@eternoendless this pull request should be part of 1.7.4.x's release isn't it? It has been started before the release of 1.7.3.0.

Contributor

mickaelandrieu commented Mar 20, 2018

@eternoendless this pull request should be part of 1.7.4.x's release isn't it? It has been started before the release of 1.7.3.0.

@mickaelandrieu mickaelandrieu referenced this pull request Mar 21, 2018

Merged

Implement dynamic form customization #8368

13 of 13 tasks complete

@eternoendless eternoendless added this to the 1.7.4.0 milestone Apr 5, 2018

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Apr 17, 2018

Contributor

:lgtm:


Reviewed 4 of 14 files at r2, 12 of 16 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

Contributor

LittleBigDev commented Apr 17, 2018

:lgtm:


Reviewed 4 of 14 files at r2, 12 of 16 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

return $productProvider->getCatalogProductList($offset, $limit, $orderBy, $sortOrder, array(), true, false);
};
$headersData = array(

This comment has been minimized.

@sarjon

sarjon Apr 17, 2018

Member

hook to modify/extend headers data would be useful here, as we have to change core now to export different fields. @mickaelandrieu what do you think?

@sarjon

sarjon Apr 17, 2018

Member

hook to modify/extend headers data would be useful here, as we have to change core now to export different fields. @mickaelandrieu what do you think?

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Apr 17, 2018

Contributor

this feature have already been asked, but as a first step I want to stay iso with current behavior. Of course, extracting features into his own classes and services allows the injection of Hook Dispatcher and so on new features for community 👍

@mickaelandrieu

mickaelandrieu Apr 17, 2018

Contributor

this feature have already been asked, but as a first step I want to stay iso with current behavior. Of course, extracting features into his own classes and services allows the injection of Hook Dispatcher and so on new features for community 👍

This comment has been minimized.

@kpodemski

kpodemski Apr 24, 2018

Contributor

Prestafony FTW

@kpodemski

kpodemski Apr 24, 2018

Contributor

Prestafony FTW

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Apr 24, 2018

Contributor

Rebased :)

Contributor

mickaelandrieu commented Apr 24, 2018

Rebased :)

@eternoendless

This comment has been minimized.

Show comment
Hide comment
@eternoendless

eternoendless Apr 24, 2018

Member

Reviewed 3 of 14 files at r2, 7 of 16 files at r3, 7 of 7 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/Adapter/Product/FilterCategoriesRequestPurifier.php, line 3 at r3 (raw file):

<?php
/**
 * 2007-2017 PrestaShop

Year must be updated


src/Adapter/Product/FilterCategoriesRequestPurifier.php, line 48 at r3 (raw file):

                    $request->request->set($param, '');
                }
            }

This foreach is unnecessary, it can be replaced by:

$param = 'filter_category';
$value = $request->request->get($param);
if (null !== $value && (!is_numeric($value) || $value < 0)) {
    $request->request->set($param, '');
}

src/Adapter/Product/FilterParametersUpdater.php, line 78 at r3 (raw file):

        $filters['limit'] = $this->getLimit($limit, $filterParameters);
        $filters['orderBy'] = $this->getOrderBy($orderBy, $filterParameters);
        $filters['sortOrder'] = $this->getSortOrder($sortOrder, $filterParameters);

You can declare the array in a cleaner fashion:

$filters = array(
    'offset' => ...,
    'limit' => ...,
    ...
);

src/Core/Product/ProductExporterInterface.php, line 36 at r3 (raw file):

{
    /**
     * @param array $products

What is that array made of? Product[]?


src/PrestaShopBundle/Controller/Admin/CommonController.php, line 221 at r3 (raw file):

     * @return Response
     */
    public function renderFieldAction($formName, $formType, $fieldName, $fieldData)

I don't think I understand the intent of this method and why is it available here.


src/PrestaShopBundle/Controller/Admin/FrameworkBundleAdminController.php, line 285 at r3 (raw file):

     * @throws \LogicException
     */
    protected function shouldDenyAction($action, $object = '', $suffix = '')

I think it's better to write method names in the positive form. Otherwise you can find yourself in a position where you have to write double negations which are harder to understand than a simple positive form (eg. !shouldDenyActionactionIsAllowed).


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 863 at r3 (raw file):

     */
    public function renderFieldAction($productId, $step, $fieldName)
    {

This should issue a @trigger_error with type E_USER_DEPRECATED


src/PrestaShopBundle/Model/Product/AdminModelAdapter.php, line 441 at r4 (raw file):

    {
        $product->loadStockData();
        $this->productPricePriority = $this->adminProductWrapper->getPricePriority($product->id);

This method produces side effects. A getter should not "set".


src/PrestaShopBundle/Resources/config/services/adapters.yml, line 146 at r4 (raw file):

            - '@prestashop.adapter.legacy.configuration'

    prestashop.adapter.filter_categories.request_purifier:

Service names should follow the class namespace.


Comments from Reviewable

Member

eternoendless commented Apr 24, 2018

Reviewed 3 of 14 files at r2, 7 of 16 files at r3, 7 of 7 files at r4.
Review status: all files reviewed at latest revision, 3 unresolved discussions, some commit checks failed.


src/Adapter/Product/FilterCategoriesRequestPurifier.php, line 3 at r3 (raw file):

<?php
/**
 * 2007-2017 PrestaShop

Year must be updated


src/Adapter/Product/FilterCategoriesRequestPurifier.php, line 48 at r3 (raw file):

                    $request->request->set($param, '');
                }
            }

This foreach is unnecessary, it can be replaced by:

$param = 'filter_category';
$value = $request->request->get($param);
if (null !== $value && (!is_numeric($value) || $value < 0)) {
    $request->request->set($param, '');
}

src/Adapter/Product/FilterParametersUpdater.php, line 78 at r3 (raw file):

        $filters['limit'] = $this->getLimit($limit, $filterParameters);
        $filters['orderBy'] = $this->getOrderBy($orderBy, $filterParameters);
        $filters['sortOrder'] = $this->getSortOrder($sortOrder, $filterParameters);

You can declare the array in a cleaner fashion:

$filters = array(
    'offset' => ...,
    'limit' => ...,
    ...
);

src/Core/Product/ProductExporterInterface.php, line 36 at r3 (raw file):

{
    /**
     * @param array $products

What is that array made of? Product[]?


src/PrestaShopBundle/Controller/Admin/CommonController.php, line 221 at r3 (raw file):

     * @return Response
     */
    public function renderFieldAction($formName, $formType, $fieldName, $fieldData)

I don't think I understand the intent of this method and why is it available here.


src/PrestaShopBundle/Controller/Admin/FrameworkBundleAdminController.php, line 285 at r3 (raw file):

     * @throws \LogicException
     */
    protected function shouldDenyAction($action, $object = '', $suffix = '')

I think it's better to write method names in the positive form. Otherwise you can find yourself in a position where you have to write double negations which are harder to understand than a simple positive form (eg. !shouldDenyActionactionIsAllowed).


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 863 at r3 (raw file):

     */
    public function renderFieldAction($productId, $step, $fieldName)
    {

This should issue a @trigger_error with type E_USER_DEPRECATED


src/PrestaShopBundle/Model/Product/AdminModelAdapter.php, line 441 at r4 (raw file):

    {
        $product->loadStockData();
        $this->productPricePriority = $this->adminProductWrapper->getPricePriority($product->id);

This method produces side effects. A getter should not "set".


src/PrestaShopBundle/Resources/config/services/adapters.yml, line 146 at r4 (raw file):

            - '@prestashop.adapter.legacy.configuration'

    prestashop.adapter.filter_categories.request_purifier:

Service names should follow the class namespace.


Comments from Reviewable

@LittleBigDev

This comment has been minimized.

Show comment
Hide comment
@LittleBigDev

LittleBigDev Apr 24, 2018

Contributor

Despite numerous retries, Travis keeps failing on some unavailable selectors...

Contributor

LittleBigDev commented Apr 24, 2018

Despite numerous retries, Travis keeps failing on some unavailable selectors...

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu May 28, 2018

Contributor

Hello @eternoendless, this pull request is now up to date with develop branch.

Contributor

mickaelandrieu commented May 28, 2018

Hello @eternoendless, this pull request is now up to date with develop branch.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Jul 30, 2018

Contributor

Hello @eternoendless, I'll re-update my pull request again (since 28th of May, since 28th of January to be precise lol).
Would you mind to make it happens in develop branch this week? 👍

Contributor

mickaelandrieu commented Jul 30, 2018

Hello @eternoendless, I'll re-update my pull request again (since 28th of May, since 28th of January to be precise lol).
Would you mind to make it happens in develop branch this week? 👍

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 2, 2018

Contributor

This method produces side effects. A getter should not "set".

I can't change this API, my scope is to refactor ProductController:catalogAction, not the related AdminWrapper. Regarding the time this already take to make this pull request merged (started 8 months ago already and descoped 2 times), we must keep the scope reduced.

Contributor

mickaelandrieu commented Aug 2, 2018

This method produces side effects. A getter should not "set".

I can't change this API, my scope is to refactor ProductController:catalogAction, not the related AdminWrapper. Regarding the time this already take to make this pull request merged (started 8 months ago already and descoped 2 times), we must keep the scope reduced.

interface ProductExporterInterface
{
/**
* @param array $products

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 2, 2018

Contributor

@eternoendless to answer you, it's array of arrays that represent products.

I really like to use ProductsCollection, but we don't have this helper class for now and PrestaShopCollection is too coupled to legacy.

@mickaelandrieu

mickaelandrieu Aug 2, 2018

Contributor

@eternoendless to answer you, it's array of arrays that represent products.

I really like to use ProductsCollection, but we don't have this helper class for now and PrestaShopCollection is too coupled to legacy.

@eternoendless

Reviewed 1 of 15 files at r5, 13 of 18 files at r6, 4 of 4 files at r7.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @sarjon, @mickaelandrieu, @Quetzacoalt91, @eternoendless, and @toutantic)


src/Core/Product/ProductExporterInterface.php, line 36 at r7 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

@eternoendless to answer you, it's array of arrays that represent products.

I really like to use ProductsCollection, but we don't have this helper class for now and PrestaShopCollection is too coupled to legacy.

If it's an array of Product you can sign the parameter (in phpdoc) as Product[]


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 126 at r4 (raw file):

Previously, Quetzacoalt91 (Thomas N) wrote…

Convenient method, but will it bring issues on IDE reporting variables as undefined?

This is risky and should be avoided


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 305 at r7 (raw file):

     * @return array
     */
    private function getToolbarButtons()

Please try to keep class members in this order: public, protected, private.


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 596 at r7 (raw file):

            $maxInputVars = (int) ini_get('max_input_vars');
            $combinationsCount = count($combinations) * 25;
            $combinationsInputs = ceil($combinationsCount/1000)*1000;

What are these magic values (25 and 1000)?


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 973 at r7 (raw file):


    /**
     * @deprecated since 1.7.4.0, to be removed in 1.8 rely on CommonController::renderFieldAction

Needs update
Also, the next version is probably going to be 8, but 🤐


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 980 at r7 (raw file):

    public function renderFieldAction($productId, $step, $fieldName)
    {
        @trigger_error('This function is deprecated, use CommonController:renderFieldAction instead.', E_USER_DEPRECATED);

Missing a : in CommonController:renderFieldAction


src/PrestaShopBundle/Model/Product/AdminModelAdapter.php, line 441 at r4 (raw file):

I can't change this API, my scope is to refactor ProductController:catalogAction, not the related AdminWrapper. Regarding the time this already take to make this pull request merged (started 8 months ago already and descoped 2 times), we must keep the scope reduced.

I'm not blocking your PR for this but it goes straight to the technical debt account.


src/PrestaShopBundle/Resources/config/services/adapter/data_configuration.yml, line 175 at r7 (raw file):

            - '@prestashop.adapter.legacy.configuration'

    prestashop.adapter.filter_categories.request_purifier:

Service names should follow the class' namespace.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 6, 2018

Contributor

I'm not blocking your PR for this but it goes straight to the technical debt account.

I'm sure we agree that this PR is reducing the technical debt of Product:catalogAction like A LOT. One step at the time, a second pull request is also about reducing the technical debt of Product pages => #8690

Service names should follow the class' namespace.

Done... but none of our services respects the convention:

prestashop.adapter.product.filter_categories_request_purifier is the "better" option, but the right one is prestashop.prestashop.adapter.product.filter_categories_request_purifier. (it's easy to remember and not too long :p)

I guess we had a kind of PrestaShop convention here:

  • prestashop.core.* => start by PrestaShop\PrestaShop\Core\
  • prestashop.adapter.* => start by PrestaShop\PrestaShop\Adapter\
  • and a lot of services that doesn't respect nothing at all.

I may work on deprecating all of these services, but it sounds risky, even more since the deprecation of services in Symfony 3 is not supported. Maybe this is something to be documented here, and to clean up in the next major release: I don't know ^^

Missing a : in CommonController:renderFieldAction

Done

Contributor

mickaelandrieu commented Aug 6, 2018

I'm not blocking your PR for this but it goes straight to the technical debt account.

I'm sure we agree that this PR is reducing the technical debt of Product:catalogAction like A LOT. One step at the time, a second pull request is also about reducing the technical debt of Product pages => #8690

Service names should follow the class' namespace.

Done... but none of our services respects the convention:

prestashop.adapter.product.filter_categories_request_purifier is the "better" option, but the right one is prestashop.prestashop.adapter.product.filter_categories_request_purifier. (it's easy to remember and not too long :p)

I guess we had a kind of PrestaShop convention here:

  • prestashop.core.* => start by PrestaShop\PrestaShop\Core\
  • prestashop.adapter.* => start by PrestaShop\PrestaShop\Adapter\
  • and a lot of services that doesn't respect nothing at all.

I may work on deprecating all of these services, but it sounds risky, even more since the deprecation of services in Symfony 3 is not supported. Maybe this is something to be documented here, and to clean up in the next major release: I don't know ^^

Missing a : in CommonController:renderFieldAction

Done

@eternoendless

I guess we had a kind of PrestaShop convention here:

prestashop.core.* => start by PrestaShop\PrestaShop\Core
prestashop.adapter.* => start by PrestaShop\PrestaShop\Adapter
and a lot of services that doesn't respect nothing at all.

I'm ok with that convention, we just need to respect the namespace as it is after the second PrestaShop, that's it.

Reviewed 1 of 18 files at r8, 17 of 18 files at r9.
Dismissed @Quetzacoalt91 from a discussion.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @sarjon, @mickaelandrieu, @Quetzacoalt91, and @toutantic)


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 596 at r7 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

What are these magic values (25 and 1000)?

Re: What are these magic values (25 and 1000)?


src/PrestaShopBundle/Controller/Admin/ProductController.php, line 136 at r9 (raw file):

        );

        extract($filters, EXTR_OVERWRITE);

As said before, extract is risky and should be avoided (not to mention it's bad for readability).

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 14, 2018

Contributor

ping @eternoendless, what do you suggest in order to make this pull request merged?

Contributor

mickaelandrieu commented Aug 14, 2018

ping @eternoendless, what do you suggest in order to make this pull request merged?

*/
private function getOffset($offset, array $filterParameters)
{
return ($offset === 'last' && isset($filterParameters['last_offset'])) ? $filterParameters['last_offset'] : $offset;

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

Line 88 && 98 && 108 && 108 have the same check. Can be refactored.

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

Line 88 && 98 && 108 && 108 have the same check. Can be refactored.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 14, 2018

Contributor

this code has already been reviewed multiple times, I won't address any new comments!

Let's focus on fixing the last comment => extract stuff. You are free to improve this class again if you have 8 months (or more) to invest.

Thank you 👼

@mickaelandrieu

mickaelandrieu Aug 14, 2018

Contributor

this code has already been reviewed multiple times, I won't address any new comments!

Let's focus on fixing the last comment => extract stuff. You are free to improve this class again if you have 8 months (or more) to invest.

Thank you 👼

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

😿

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

😿

->setData($dataCallback)
->setHeadersData($headersData)
->setModeType(CsvResponse::MODE_OFFSET)
->setLimit(5000)

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

Why 5000? use const maybe and explain why 5000 :)

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

Why 5000? use const maybe and explain why 5000 :)

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 14, 2018

Contributor

I don't know why 5000, this code has just been moved outside of Product Controller.

@mickaelandrieu

mickaelandrieu Aug 14, 2018

Contributor

I don't know why 5000, this code has just been moved outside of Product Controller.

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

Weird (again)

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

Weird (again)

* @param $key
* @param $domain
* @return string
* @throws \Symfony\Component\Translation\Exception\InvalidArgumentException

This comment has been minimized.

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

This method don't really throws InvalidArgumentException but Translator::trans does =)

@PierreRambaud

PierreRambaud Aug 14, 2018

Contributor

This method don't really throws InvalidArgumentException but Translator::trans does =)

Sébastien LE BRUCHEC and others added some commits Jan 16, 2018

);
$logger->info(
'Products sorted: (' . implode(',', $productIdList) .
') with positions (' . implode(',', $productPositionList) . ').'
);
$this->addFlash(

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 16, 2018

Contributor

duplicated, I've read again all the controller: it should be fine now 👍

@mickaelandrieu

mickaelandrieu Aug 16, 2018

Contributor

duplicated, I've read again all the controller: it should be fine now 👍

@eternoendless

:lgtm:

Reviewed 17 of 18 files at r11.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @sarjon, @mickaelandrieu, @Quetzacoalt91, @PierreRambaud, and @toutantic)

@marionf marionf assigned marionf and unassigned marionf Aug 17, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Aug 20, 2018

@PierreRambaud PierreRambaud merged commit 1f63841 into PrestaShop:develop Aug 20, 2018

1 of 3 checks passed

Codacy/PR Quality Review Not up to standards. This pull request quality could be better.
Details
code-review/reviewable 8 discussions left (mickaelandrieu, PierreRambaud, Quetzacoalt91, sarjon, toutantic)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@PierreRambaud PierreRambaud deleted the mickaelandrieu:improve-catalog-action branch Aug 20, 2018

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 20, 2018

Contributor

Thanks @mickaelandrieu and everyone for reviews!

Contributor

PierreRambaud commented Aug 20, 2018

Thanks @mickaelandrieu and everyone for reviews!

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