Add visibility and status filter to category product grid #12564

peterjaap · 2017-12-06T13:23:00Z

Add visibility and status filter to category product grid to be able to easier filter large amounts of products. Wading through thousands of 'Not visible individually' products is no fun.

Description

Before:

After:

& with filter applied;

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

adrian-martinez-interactiv4 · 2017-12-06T14:27:20Z

app/code/Magento/Catalog/Block/Adminhtml/Category/Tab/Product.php

@@ -159,6 +165,30 @@ protected function _prepareColumns()
        );
        $this->addColumn('name', ['header' => __('Name'), 'index' => 'name']);
        $this->addColumn('sku', ['header' => __('SKU'), 'index' => 'sku']);
+


Static tests are failing due to this: Code must not contain multiple empty lines in a row; found 2 empty lines

adrian-martinez-interactiv4 · 2017-12-06T14:45:13Z

app/code/Magento/Catalog/Block/Adminhtml/Category/Tab/Product.php

+                'header' => __('Visibility'),
+                'index' => 'visibility',
+                'type' => 'options',
+                'options' => Visibility::getOptionArray(),


Consider avoid using the method statically, injecting class in constructor to let dependency injection decide class to be used, like it's done in Magento/Catalog/Block/Adminhtml/Product/Grid.php:

Yes, I had that first but adding it to the constructor also forced me to add it to the constructor of all the classes that extend this class, otherwise it broke. This seemed to me the better solution. Also; why is the method static when we're not allowed to use it?

I cannot see any class extending \Magento\Catalog\Block\Adminhtml\Category\Tab\Product, but as third party modules may have extended this class, there's a preferred way to add a constructor parameter preserving backwards compatibility, as noted in http://devdocs.magento.com/guides/v2.2/contributor-guide/backward-compatible-development/index.html#adding-a-constructor-parameter. It works this way:

<?php /** * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ /** * Product in category grid * * @author Magento Core Team <[email protected]> */ namespace Magento\Catalog\Block\Adminhtml\Category\Tab; use Magento\Backend\Block\Widget\Grid; use Magento\Backend\Block\Widget\Grid\Column; use Magento\Backend\Block\Widget\Grid\Extended; use Magento\Catalog\Model\Product\Attribute\Source\Status; use Magento\Catalog\Model\Product\Visibility; use Magento\Framework\App\ObjectManager; class Product extends \Magento\Backend\Block\Widget\Grid\Extended { (...) /** * @var \Magento\Catalog\Model\Product\Visibility */ private $visibility; /** * @var \Magento\Catalog\Model\Product\Attribute\Source\Status */ private $status; /** * @param \Magento\Backend\Block\Template\Context $context * @param \Magento\Backend\Helper\Data $backendHelper * @param \Magento\Catalog\Model\ProductFactory $productFactory * @param \Magento\Framework\Registry $coreRegistry * @param array $data * @param Visibility $visibility * @param Status $status */ public function __construct( \Magento\Backend\Block\Template\Context $context, \Magento\Backend\Helper\Data $backendHelper, \Magento\Catalog\Model\ProductFactory $productFactory, \Magento\Framework\Registry $coreRegistry, array $data = [], \Magento\Catalog\Model\Product\Visibility $visibility = null, \Magento\Catalog\Model\Product\Attribute\Source\Status $status = null ) { $this->visibility = $visibility ?: ObjectManager::getInstance()->get(Visibility::class); $this->status = $status ?: ObjectManager::getInstance()->get(Status::class); (...) } (...) /** * @return Grid */ protected function _prepareCollection() { if ($this->getCategory()->getId()) { $this->setDefaultFilter(['in_category' => 1]); } $collection = $this->_productFactory->create()->getCollection()->addAttributeToSelect( 'name' )->addAttributeToSelect( 'sku' )->addAttributeToSelect( 'price' )->addAttributeToSelect( 'visibility' )->addAttributeToSelect( 'status' )->joinField( 'position', 'catalog_category_product', 'position', 'product_id=entity_id', 'category_id=' . (int)$this->getRequest()->getParam('id', 0), 'left' ); (...) } /** * @return Extended */ protected function _prepareColumns() { (...) $this->addColumn('name', ['header' => __('Name'), 'index' => 'name']); $this->addColumn('sku', ['header' => __('SKU'), 'index' => 'sku']); $this->addColumn( 'visibility', [ 'header' => __('Visibility'), 'index' => 'visibility', 'type' => 'options', 'options' => $this->visibility->getOptionArray(), 'header_css_class' => 'col-visibility', 'column_css_class' => 'col-visibility', ] ); $this->addColumn( 'status', [ 'header' => __('Status'), 'index' => 'status', 'type' => 'options', 'options' => $this->status->getOptionArray(), ] ); (...) } (...) }

About using the static method, you may use it that way in this particular case, but the answer about why is prefered constructor param is all about reflection used in object manager, dependency injection, and preferences defined in di.xml. It's more about what class is used than how the method is called.

Constructor params are resolved by the object manager, that may use the preference directive to instantiate one class or another as specified. If you directly include the class you need and use directly its static functions, you are bypassing preferences that may be defined for that class.

An example, let's say I wanted to add a new "dummy" visibility level. An implementation for that (excluding plugins) could be this:

In di.xml: <preference for="Magento\Catalog\Model\Product\Visibility" type="\Magento\Catalog\Model\Product\CustomVisibility" />

new class \Magento\Catalog\Model\Product\CustomVisibility :

<?php /** * Copyright © Magento, Inc. All rights reserved. * See COPYING.txt for license details. */ namespace Magento\Catalog\Model\Product; use Magento\Framework\Data\OptionSourceInterface; use Magento\Framework\DB\Ddl\Table; /** * Catalog Product visibility model and attribute source model * * @api * @since 100.0.2 */ class CustomVisibility extends Visibility implements OptionSourceInterface { const VISIBILITY_DUMMY = 5; /** * Retrieve option array * * @return array */ public static function getOptionArray() { $options = parent::getOptionArray(); $options[self::VISIBILITY_DUMMY] = __('Dummy'); return $options; } }

Using the constructor param, \Magento\Catalog\Model\Product\Visibility is replaced by \Magento\Catalog\Model\Product\CustomVisibility when it gets injected, and then you can get the options statically or not, as you prefer, but from the right class :

Using directly the class as it's done in the changes suggested in this PR makes Visibility class options irreplaceable, since you are defining directly the class to be called.

I understand why it's nicer to do it through DI, but the question was more; why is that method even static? Something like that should never be static.

And wow, that is one ugly way to add constructor parameter without breaking BC. "Never use object manager, except when you want to preserve BC." Crazy.

I guess it shouldn't be static, it's a piece of inherited code from Magento 1, and it should be refactored at some point: https://github.com/magento/magento2/blob/0849373d7636cfa17b70b8188bfc36d13ae749c1/app/code/Magento/Catalog/Model/Product/Visibility.php

About using the object manager, it's true that most of time shouldn't be used directly, but constructor params BC may be one of the few cases where it could make sense, since when the major version is increased and no BC is required, all constructors are refactored to remove this direct object manager usage. From my point of view, where using object manager is absolutely forbidden is in the main logic of any module, if object manager is strictly required to make the module work.

adrian-martinez-interactiv4 · 2017-12-06T14:45:36Z

app/code/Magento/Catalog/Block/Adminhtml/Category/Tab/Product.php

+                'header' => __('Status'),
+                'index' => 'status',
+                'type' => 'options',
+                'options' => Status::getOptionArray()


Same as Visibility

peterjaap

Updated as per review.

adrian-martinez-interactiv4

With this last changes, and squashing commits into a single clean commit with a meaningful message I think we can proceed with this PR

adrian-martinez-interactiv4 · 2017-12-06T19:34:45Z

app/code/Magento/Catalog/Block/Adminhtml/Category/Tab/Product.php

+    /**
+     * @var Status
+     */
+    protected $_status;


New class variables should be private, and should not be prefixed with an underscore to indicate visibility

adrian-martinez-interactiv4 · 2017-12-06T19:34:57Z

app/code/Magento/Catalog/Block/Adminhtml/Category/Tab/Product.php

+    /**
+     * @var Visibility
+     */
+    protected $_visibility;


New class variables should be private, and should not be prefixed with an underscore to indicate visibility

adrian-martinez-interactiv4 · 2017-12-06T19:38:20Z

app/code/Magento/Catalog/Block/Adminhtml/Category/Tab/Product.php

    ) {
+        $this->_visibility = $visibility ?: ObjectManager::getInstance()->get(Visibility::class);
+        $this->_status = $status ?: ObjectManager::getInstance()->get(Status::class);
+


It would be nice to rearrange class variable assignations in the same order as defined in constructor, although it's not strictly needed

adrian-martinez-interactiv4 · 2017-12-06T22:26:07Z

Thank you very much @peterjaap :D , would you consider to backport this changes?

ishakhsuvarov · 2017-12-07T08:58:07Z

@peterjaap @adrian-martinez-interactiv4 I suggest adding some automated tests to this PR as it introduces changes to the behavior. This way it will be compatible with the Magento Core Definition of Done

peterjaap · 2017-12-07T15:25:17Z

@ishakhsuvarov which method would you like to see tests for?

ishakhsuvarov · 2017-12-07T15:31:19Z

I do not see to much reason to cover this class or specific method with a unit test, as it would not bring too much value.
Integration or Functional test covering this grid would be great however.

peterjaap · 2017-12-07T15:53:46Z

Ok but that feels like an entirely different issue. Besides, shouldn't we want with a functional test until MFTF is done? Seems like duplicate work if we do one now.

ishakhsuvarov · 2017-12-07T16:04:14Z

I think integration test is sufficient in this case, it is less challenging to implement and provides sufficient coverage for the specific case.

adrian-martinez-interactiv4 · 2018-01-19T02:16:29Z

@peterjaap @ishakhsuvarov @okorshenko What should we do with this PR?

On the one hand, I agree all changes should be covered with tests as possible. On the other hand, this a very simple PR will useful functionality, for an old-fashioned class using old grid system that should be fully refactored and does not seem to be covered by any kind of test yet. I mean, I don't know if given the current testing state for this class, it is worth of writing tests for it, and if tests are not written for this class, we are throwing this useful functionality away.

How should we proceed?

magento-engcom-team · 2018-02-07T22:26:14Z

@peterjaap thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

magento-engcom-team · 2018-02-07T22:27:32Z

@peterjaap thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

okorshenko · 2018-02-07T22:30:52Z

Thank you @adrian-martinez-interactiv4 @peterjaap for collaboration. We will cover the fix with the tests on our side

…ed tests

…ty-and-status-filter-to-category-product-grid

…rid #12564

adrian-martinez-interactiv4 self-assigned this Dec 6, 2017

adrian-martinez-interactiv4 self-requested a review December 6, 2017 14:12

adrian-martinez-interactiv4 suggested changes Dec 6, 2017

View reviewed changes

peterjaap commented Dec 6, 2017

View reviewed changes

adrian-martinez-interactiv4 suggested changes Dec 6, 2017

View reviewed changes

Added Visibility and Status filter to category product grid

a5bf904

peterjaap force-pushed the add-visibility-and-status-filter-to-category-product-grid branch from b157a4d to a5bf904 Compare December 6, 2017 21:12

adrian-martinez-interactiv4 approved these changes Dec 6, 2017

View reviewed changes

adrian-martinez-interactiv4 added this to the December 2017 milestone Dec 6, 2017

adrian-martinez-interactiv4 added the Release Line: 2.2 label Dec 6, 2017

magento-team added the Progress: accept label Dec 6, 2017

This was referenced Dec 7, 2017

[Backport 2.1] Added Visibility and Status filter to category product grid #12576

Closed

[Backport 2.3] Added Visibility and Status filter to category product grid #12577

Merged

magento-team added the Progress: needs update label Dec 7, 2017

okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018

okorshenko self-assigned this Feb 7, 2018

magento-engcom-team removed the Progress: needs update label Feb 7, 2018

okorshenko modified the milestones: January 2018, February 2018 Feb 7, 2018

nmalevanec and others added 2 commits February 13, 2018 11:14

Added Visibility and Status filter to category product grid - automat…

578e62c

…ed tests

Merge remote-tracking branch 'mainline/2.2-develop' into add-visibili…

5122c0a

…ty-and-status-filter-to-category-product-grid

magento-team pushed a commit that referenced this pull request Feb 23, 2018

MAGETWO-85163: Add visibility and status filter to category product g…

82f6061

…rid #12564

magento-engcom-team merged commit 5122c0a into magento:2.2-develop Feb 23, 2018

Add visibility and status filter to category product grid #12564

Add visibility and status filter to category product grid #12564

Uh oh!

Conversation

peterjaap commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Contribution checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peterjaap left a comment

Choose a reason for hiding this comment

Uh oh!

adrian-martinez-interactiv4 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrian-martinez-interactiv4 commented Dec 6, 2017

Uh oh!

ishakhsuvarov commented Dec 7, 2017

Uh oh!

peterjaap commented Dec 7, 2017

Uh oh!

ishakhsuvarov commented Dec 7, 2017

Uh oh!

peterjaap commented Dec 7, 2017

Uh oh!

ishakhsuvarov commented Dec 7, 2017

Uh oh!

adrian-martinez-interactiv4 commented Jan 19, 2018

Uh oh!

magento-engcom-team commented Feb 7, 2018

Uh oh!

magento-engcom-team commented Feb 7, 2018

Uh oh!

okorshenko commented Feb 7, 2018

Uh oh!

Uh oh!

peterjaap commented Dec 6, 2017 •

edited

Loading