Adds configurable columns to product grid #3451

empiricompany · 2023-08-19T11:38:56Z

This add new configuration to choose columns for catalog product list, i think this is an important feature to add to OpenMage.

IMPORTANT:
Original solution with trait has been replaced with a new flexible architecture that follow magento way, inspiration from massaction grid component.
Also create an interface to implement custom columns in other grids easily keeping the code well organized.
All the relevant classes that extend Mage_Adminhtml_Block_Widget_Grid have been restored to their original state.

Technical details

To modify the grid columns, two things need to be done:

Add new columns data to the collection
Add new columns to be printed in the grid

For this reason, the class Mage_Adminhtml_Block_Widget_Grid initialize an helper that must implements two methods for this purpose.
The helper is bound by a contract with the interface Mage_Adminhtml_Helper_Widget_Grid_Config_Interface, which requires the implementation of the two methods:

public function applyAdvancedGridCollection($collection);

/**
* Collection object
*
* @param Mage_Core_Model_Resource_Db_Collection_Abstract $collection
* return $this
*/
public function applyAdvancedGridCollection($collection);

This method is called by 'Mage_Adminhtml_Block_Widget_Grid' after _prepareCollection() and allows to modify the collection by accessing the $collection->addAttributeToSelect and other methods.

public function applyAdvancedGridColumn($gridBlock);

/**
 * Adminhtml grid widget block
 *
 * @param Mage_Adminhtml_Block_Widget_Grid $gridBlock
 * return $this
 */
public function applyAdvancedGridColumn($gridBlock);

This method is called after _prepareColumns and allows adding new columns to the grid by accessing the $gridBlock->addColumnAfter() and all the public methods of Mage_Adminhtml_Block_Widget_Grid .

Here, for example, I quickly implemented a new field in the orders grid:

empiricompany@b40ae34
(without system configuration)

The helper for advanced grid is initialized like this.
Here, I have made a note that a factory class would be needed.

/**
* Get Helper Advanced Grid
*
* @return Mage_Adminhtml_Helper_Widget_Grid_Config_Abstract
*/
public function getHelperAdvancedGrid(): Mage_Adminhtml_Helper_Widget_Grid_Config_Abstract
{
  if (!$this->_advancedGridHelper) {
      // TODO create factory class map block id to helper - create a new grid.xml configuration file
      switch ($this->getId()) {
          case 'productGrid':
          case 'catalog_category_products':
          case 'related_product_grid':
          case 'up_sell_product_grid':
          case 'cross_sell_product_grid':
          case 'super_product_links':
              $this->setAdvancedGridHelperName('adminhtml/widget_grid_config_catalog_product');
              break;
          case 'sales_order_grid':
              $this->setAdvancedGridHelperName('adminhtml/widget_grid_config_sales_order');
              break;
      }

      $this->_advancedGridHelper = Mage::helper($this->getAdvancedGridHelperName());

      if (!($this->_advancedGridHelper instanceof Mage_Adminhtml_Helper_Widget_Grid_Config_Abstract)) {
          Mage::throwException(
              $this->__("Helper for advanced grid config doesn't implement required interface. %s must extends Mage_Adminhtml_Helper_Widget_Grid_Config_Abstract", get_class($this->_advancedGridHelper))
          );
      }
  }
  $this->_advancedGridHelper->setGridId($this->getId());
  return $this->_advancedGridHelper;
}

The identification of the correct helper by checking block id avoids setting the helper manually in each relevant class.
I'm thinking about a new configuration file called grid.xml that maps grid IDs to helpers and a factory class.
Otherwise, and it remains an option, we should specify an helper for advanced grid like this:

class Mage_Adminhtml_Block_Catalog_Product_Grid extends Mage_Adminhtml_Block_Widget_Grid
{
    public function __construct()
    {
        parent::__construct();
        $this->setId('productGrid');
        
        // Set helperAdvancedGrid
        $this->setAdvancedGridHelperName('adminhtml/widget_grid_config_catalog_product'); 
        
        $this->setDefaultSort('entity_id');
        $this->setDefaultDir('DESC');
        $this->setSaveParametersInSession(true);
        $this->setUseAjax(true);
        $this->setVarNameFilter('product_filter');
    }

Column sorting

To manage column sorting the class Mage_Adminhtml_Block_Widget_Grid adds the block adminhtml/widget_grid_advanced (as massaction) to the 'grid.phtml' template.

The block is then called in grid.phtml, after massactionblock:

<?php if($this->getAdvancedGridBlock()->isAvailable()): ?>
    <?php echo $this->getAdvancedGridBlock()->getJavaScript() ?>
 <?php endif ?>

This block inserts the JavaScript that will handle the sorting by initializing 'varienGridAdvanced' (included in grid.js)
PS: This block can also be used to insert a reset button for column order.

For this feature a new controller Mage_Adminhtml_GridController has been created.
The columns can be sorted using drag & drop. Upon the drop event, an AJAX call to adminhtml/grid/saveColumnOrder is made, and the sequence of columns is saved in system_config advanced_grid/_id_of_grid_/order.

Then the grid reloads as usual through AJAX, similar to a search.

Features:

Enable/disable custom columns for single grids:
Catalog Product list
Catalog Category Products tab
Catalog Product View Related, Up-sells, Cross-sells and Associated Products tabs
Choose columns to display for single grids
Show Columns Created At and Updated At
Choose Image Preview Columns: Base Image, Small Image, Thumbnail
Set custom width for product image (default 64px)
Render correctly product images and field by type like: select, multiselect, date, price and text/textarea
Add a second horizontal scroll on top if the columns don't fit in the window
Set order columns by drag&drop

I have added a new column renderer for product images. The images are resized to 64px by default to reduce file size, and they are generated in media/catalog/product/cache/0/64x64/ directory.
Image previews are linked to the original url.

This is a proof of concept - Not ready for production

I have not tested yet with php 7.4, only php 8.2

It should be perfectly compatible with any existing columns added by overwriting the classes that extend Mage_Adminhtml_Block_Widget_Grid, like Mage_Adminhtml_Block_Catalog_Product_Grid.

Knowed Bugs / Issues:

Missing translations
In multi grid product view there are issues with columns sorting
Phpstan doesn't like constants in traits, even though in theory they seem to not cause any problems when called with self like: self::CONFIG_PATH_GRID_ENABLED
ref: https://wiki.php.net/rfc/constants_in_traits
Costants moved to a dedicated helper Mage_Adminhtml_Helper_Widget_Grid_Config

Improvements:

Reset columns order button
Force column drag only to x axis
Role permission to restrict the ability to sort columns
Possibility to remove default columns
Configuration field image width must show only if this columns are selected: image, small_image, thumbnail

Code Refactoring

Move css for column grid sorting to backend theme (now is injected in grid.phtml)
Create a dedicate helper that configure Mage_Adminhtml_Block_Widget_Grid and separete from helper class that retrive data from config

pquerner · 2023-08-19T11:50:19Z

Imho this should be made optional. Our instance doesnt have images, instead they are coming from CDN anyways and we've overwritten the parts, but it would make it (I think) like we have "none" images for any product - and simply add a column for no reason.

Also I think some vendors doesnt want that even if they have their product images attached to magento.

ADDISON74 · 2023-08-19T11:55:07Z

We should have the implementation of Magento 2 grids in OpenMage, but more extensive changes are needed.

There are a lot of implementations all over the Internet and for a while I considered displaying an image in the grid as useful for quick visual identification of the product, but the feedback I received was negative. First of all, the grid was getting bigger, especially when 200 products were viewed on the page. The page loading time increases, there are requests for each individual image.

I could agree with this PR only if there is also the option to disable the display of the column in product grid, which should be implicitly set in Backend to No.

ADDISON74 · 2023-08-19T11:57:40Z

... there must be the option of control in the Backend for each individual grid where images are displayed: Product Grid, Category Grid, Related/Up-sells/Cross-sells Grids.

empiricompany · 2023-08-19T11:59:55Z

We should have the implementation of Magento 2 grids in OpenMage, but more extensive changes are needed.

There are a lot of implementations all over the Internet and for a while I considered displaying an image in the grid as useful for quick visual identification of the product, but the feedback I received was negative. First of all, the grid was getting bigger, especially when 200 products were viewed on the page. The page loading time increases, there are requests for each individual image.

I could agree with this PR only if there is also the option to disable the display of the column in product grid, which should be implicitly set in Backend to No.

I agree, I will convert this PR into a draft because I want to immediately configure the choice of columns to display, which by default does nothing in order to avoid any compatibility and performance issues. I will complete the PR in the next few days while waiting for further feedback.

ADDISON74 · 2023-08-19T12:03:00Z

In addition, we must set up in the Backend, in the same section that controls each individual grid, the size of the image.

Displaying an image of 64 px height compared to 200 displayed products on the page or maybe even more if someone uses other custom values, will increase the use of the scroll, will at least triple the display.

empiricompany · 2023-08-19T14:26:09Z

What do you think about last commit be9d683?

I've used traits to add columns to the grid and i've added configuration to enable product image column and setting the width.
Trait should be the safest way to avoid compatibility issues.

If this path can be ok, I can proceed with additional features and reorganize the configuration more effectively, for which I would dedicate a separate tab.

Same questions about:

there is better location where to place the traits?
i have to implement same sort of method to configure the order of columns, now i've just placed product image after entity_id

PS: phpstan report that traits cannot have constant, but using self it's ok
i've to implement an interface like that?
https://wiki.php.net/rfc/constants_in_traits

then

interface Mage_Adminhtml_Block_Widget_Grid_Config_Product_Columns_Interface
{
    public const CONFIG_PATH_GRID_COLUMNS = 'admin/grid_catalog/columns';
    public const CONFIG_PATH_GRID_COLUMN_IMAGE_WIDTH = 'admin/grid_catalog/imagewith';
}

class Mage_Adminhtml_Block_Catalog_Product_Grid extends Mage_Adminhtml_Block_Widget_Grid implements Mage_Adminhtml_Block_Widget_Grid_Config_Product_Columns_Interface
{
    use Mage_Adminhtml_Block_Widget_Grid_Config_Product_Columns;

waiting for feedback

ADDISON74 · 2023-08-19T17:56:54Z

I would like to be able to control the display of the column in each of the 5 grids where it appears: Product, Category, Up-Sells, Cross-Sells, Related.

From the attached image it appears that it is a unitary control and not an individual one.

The idea of using multiselect is not a bad one

Catalog Grid Configuration - it is OK, it requires the translation label
Columns - OK too
Product Image - here I would control the 5 grids for the image, adding the 5 options like this

Product - Image Thumbnail
Category - Image Thumbnail
Cross-Sells - Image Thumbnail
Related - Image Thumbnail
Up-Sells - Image Thumbnail

But if he has other custom grids then he will no longer have control.

I think it would have been better to have a discussion in the dedicated section about this idea and then you could proceed to its implementation.

empiricompany · 2023-08-19T18:50:46Z

I would like to be able to control the display of the column in each of the 5 grids where it appears: Product, Category, Up-Sells, Cross-Sells, Related.

From the attached image it appears that it is a unitary control and not an individual one.

The idea of using multiselect is not a bad one
* **Catalog Grid Configuration** - it is OK, it requires the translation label

* **Columns** - OK too

* **Product Image** - here I would control the 5 grids for the image, adding the 5 options like this
Product - Image Thumbnail Category - Image Thumbnail Cross-Sells - Image Thumbnail Related - Image Thumbnail Up-Sells - Image Thumbnail

But if he has other custom grids then he will no longer have control.

I think it would have been better to have a discussion in the dedicated section about this idea and then you could proceed to its implementation.

Yes, through the trait and the way I have organized the code, I would be able to implement dedicated configurations for each type of grid (catalog_product, catalog_category/product, upsell, related, crossell, and other grids like sales), where you can set additional columns besides the product image a its order.
i want feedback about current implementation with traits

fballiano · 2023-09-20T18:21:23Z

so, I've just pushed a modifications for a first version of this kind of configuration:

at the moment:

disables loading of the doublescroll javascript file (question for @empiricompany: why is the js file named doublescroll?)
enable/disable rendering of the 2 buttons
implemented ACL in admin/system/rearrange_grids_columns

TODO

make Mage_Adminhtml_Block_Widget_Grid_Advanced_Abstract::getJavaScript() better
probably (if this implementations gets accepted) remove the "rearrange columns" button because if the combobox is enabled... rearrange should be automatically enabled. I didn't remove the button now just to see if you like this implemetation

empiricompany · 2023-09-20T22:05:39Z

@fballiano - thanks for the changes (but I can't accept or decline your implementations because they are directly made to this source and not through a separate PR to my repo)

Regarding your question: there isn't a specific reason, but I would always load the double scroll because it is useful and it comes into play at lower resolutions. In fact, we can move it to the grid.

I will test it in the next days, thanks!

fballiano · 2023-09-20T22:09:21Z

you can revert the changes if you feel so, I did only we we agreed on.

I asked about the doublescroll because it didn't feel related to this PR (the name) and I didn't read the content of the file. Now I think I get why is that. it does the horizontal scrolling of the grid.

empiricompany · 2023-09-20T22:14:39Z

you can revert the changes if you feel so, I did only we we agreed on.

Sure, should I manually revert your single commits eventually then force a push, or is there a simpler method (maybe from github ui)?

fballiano · 2023-09-20T22:15:24Z

if you git revert a commit you don't have to force push

empiricompany · 2023-09-21T07:11:02Z

Is it possible to limit specific section / field config by ACL? I haven't done it before...

i tried

<advanced_grid translate="title">
    <title>Admin Grids</title>
    <sort_order>101</sort_order>
    <children>
        <rearrange_grids_columns translate="title">
            <title>Rearrange Grids Columns</title>
        </rearrange_grids_columns>
    </children>
</advanced_grid>

want an acl like this:

fballiano · 2023-09-21T08:04:37Z

AFAIK ACLs can't be applied to system/configurations

fballiano · 2023-09-21T16:26:31Z

js/mage/adminhtml/grid-double-scroll.js

+    createDoubleScroll() {
+        let scrollbarTop = document.createElement('div');
+        scrollbarTop.classList.add('hor-scroll-top');
+        scrollbarTop.appendChild(document.createElement('div'));
+        scrollbarTop.style.overflow = 'auto';
+        scrollbarTop.style.overflowY = 'hidden';
+        scrollbarTop.firstChild.style.height = '0';
+        scrollbarTop.firstChild.style.paddingTop = '1px';
+        scrollbarTop.firstChild.appendChild(document.createTextNode('\xA0'));
+        this.wrapperScrollBar.parentNode.insertBefore(scrollbarTop, this.wrapperScrollBar);
+
+        this.scrollbarTop = scrollbarTop;
+    }


I've read this file in more detail, the only thing that I'd try to do, in order to avoid as much javascript as possible, is to have this div directly into the widget/grid.phtml it should work ;-)

I don't think it's so better only for save a few lines because if someone overwrites grid.phtml we lost it.

If you care about its size we can minify it as mage/adminhtml/grid-double-scroll.min.js and distribute also the original source mage/adminhtml/grid-double-scroll.js and can be done to others files like grid.js.

More javascript is not necessary bad if it's usefull, IMHO this is a usefull feature because if the columns don't fit in the window and I want to view the last column in a particular row, I have to scroll down to the end and then up to that row.
If we compare to other modern backends like Magento 2, our JS/Memory footprint is better

I really woulnd't compare anything to M2 and I still believe, also from a performance standpoint, there's no need for element injection in js

and it shouldn't be minified either in the source code

Okay, let's leave this decision pending for now

also, grid.phtml is already modified by this PR

have you tested if it works correctly when there are more grids like in product view (related, upsell etc..) ?

This script, as it is, is independent from the template. The only requirement is that the table is wrapped inside a div.hor-scroll, so it would also work on third-party implementations with a custom template that not include div.hor-scroll-top

at the moment the script is called by a modified version of the template so it wouldn't work. I understand that it is callable by any situation but I'll never agree that many lines of javascript are better than 1 line of html + 1 line of css, both for readability and performance.
people may have a customized grid.html (doubt it but possible) but they could also have customized layout xml so the script won't be loaded, both approaches have pros and cons but the script is not 100% better.

at the moment the script is called by a modified version of the template so it wouldn't work

Then the script should be modified to work with this external div, with all handling of the events relative to the current grid

people may have a customized grid.html (doubt it but possible) but they could also have customized layout xml so the script won't be loaded, both approaches have pros and cons but the script is not 100% better.

This is not necessarily true, the script should always be injected as grid.js unless explicitly removed in a layout update.
IMHO I prefer to keep grid.phtml clean without a div that is not clear what it does, in terms of performance it doesn't change anything.

But these are just details, the priority for me is that PR should be tested and reviewed on its implementation solution with interface contract, naming convention, etc.

sreichel · 2024-08-06T18:29:48Z

For system config ACL read https://alanastorm.com/custom_magento_system_configuration/

Maybe it helps.

Does it still work to add grid columns via observer?

sreichel · 2024-08-06T19:21:51Z

Suggestions:

add strict type declaration to all new files
add type hints to all new methods/properties
try to pass phpstan level 9 for all new classes

empiricompany · 2024-08-07T11:00:05Z

Does it still work to add grid columns via observer?

yes

empiricompany · 2024-08-07T11:01:15Z

For system config ACL read https://alanastorm.com/custom_magento_system_configuration/

Maybe it helps.

can't add an acl for system config

sreichel · 2024-08-08T02:29:22Z

can't add an acl for system config

If it does not work to set ACL "one level deeper" to config section i think its worth to raise an issue.

empiricompany · 2024-08-08T09:09:08Z

can't add an acl for system config

If it does not work to set ACL "one level deeper" to config section i think its worth to raise an issue.

no it's a different type of ACL
I needed it to limit the display of column configurations or permission to rearrange orders for example for each user, but it is not possible to do so.

sreichel · 2024-08-08T21:02:08Z

I am not sure about it, but EnhancedGrid extension has a user based grid config. (or it was a modified version of it)

empiricompany · 2024-08-09T10:41:28Z

I am not sure about it, but EnhancedGrid extension has a user based grid config. (or it was a modified version of it)

yes it has it, but it works differently it doesn't save in system config but it has two tables

empiricompany and others added 15 commits August 18, 2023 14:49

WIP

940c985

add to category product

15ab51b

add to related upsell crossell

462f182

set fixed width

4743493

clean

73398a4

clean

f74e4ee

cosmetic

716310f

css parent

41fbdd3

doc

480763b

refactor

3d96895

default width 64

b2c0b18

phpcs phpstan

852bcbd

Merge branch 'OpenMage:main' into gridproductimage

5b4f58e

phpcsfixer

43262da

phpcs

abd8a07

github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Aug 19, 2023

empiricompany marked this pull request as draft August 19, 2023 12:00

add configuration

be9d683

github-actions bot added the Component: Core Relates to Mage_Core label Aug 19, 2023

support all columns configuration

b1732aa

empiricompany changed the title ~~Adds product image column to product grid~~ Adds configurable columns to product grid Aug 20, 2023

fix issues in catalog category

3a337e6

Added configuration to enable rearrangement

4294ed3

fballiano added 2 commits September 20, 2023 19:21

Merge branch 'main' into gridproductimage

9d19518

Implemented ACL

9f3dbd6

rename acl title to admin grids

6d09657

fballiano reviewed Sep 21, 2023

View reviewed changes

empiricompany and others added 8 commits September 21, 2023 19:04

remove conditionally load doublescroll

4aefaed

Merge branch 'main' into gridproductimage

2335b5d

Merge branch 'main' into gridproductimage

e459ab6

Merge branch 'main' into gridproductimage

ea049ba

Merge branch 'main' into gridproductimage

90df199

Merge branch 'main' into gridproductimage

9c3b76c

Merge branch 'main' into gridproductimage

3792f8f

Merge branch 'main' into gridproductimage

dfae985

Merge branch 'main' into gridproductimage

c69ddb2

Merge branch 'main' into gridproductimage

07afcb9

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds configurable columns to product grid #3451

Adds configurable columns to product grid #3451

empiricompany commented Aug 19, 2023 •

edited

Loading

pquerner commented Aug 19, 2023

ADDISON74 commented Aug 19, 2023

ADDISON74 commented Aug 19, 2023

empiricompany commented Aug 19, 2023

ADDISON74 commented Aug 19, 2023

empiricompany commented Aug 19, 2023 •

edited

Loading

ADDISON74 commented Aug 19, 2023

empiricompany commented Aug 19, 2023

fballiano commented Sep 20, 2023 •

edited

Loading

empiricompany commented Sep 20, 2023

fballiano commented Sep 20, 2023

empiricompany commented Sep 20, 2023

fballiano commented Sep 20, 2023

empiricompany commented Sep 21, 2023

fballiano commented Sep 21, 2023

fballiano Sep 21, 2023

empiricompany Sep 21, 2023

fballiano Sep 21, 2023 •

edited

Loading

empiricompany Sep 21, 2023

fballiano Sep 22, 2023

empiricompany Sep 22, 2023 •

edited

Loading

empiricompany Sep 22, 2023

fballiano Sep 22, 2023

empiricompany Sep 22, 2023

sreichel commented Aug 6, 2024 •

edited

Loading

sreichel commented Aug 6, 2024 •

edited

Loading

empiricompany commented Aug 7, 2024

empiricompany commented Aug 7, 2024

sreichel commented Aug 8, 2024

empiricompany commented Aug 8, 2024

sreichel commented Aug 8, 2024

empiricompany commented Aug 9, 2024

Adds configurable columns to product grid #3451

Are you sure you want to change the base?

Adds configurable columns to product grid #3451

Conversation

empiricompany commented Aug 19, 2023 • edited Loading

Technical details

Here, for example, I quickly implemented a new field in the orders grid:

Column sorting

Features:

This is a proof of concept - Not ready for production

Knowed Bugs / Issues:

Improvements:

Code Refactoring

pquerner commented Aug 19, 2023

ADDISON74 commented Aug 19, 2023

ADDISON74 commented Aug 19, 2023

empiricompany commented Aug 19, 2023

ADDISON74 commented Aug 19, 2023

empiricompany commented Aug 19, 2023 • edited Loading

ADDISON74 commented Aug 19, 2023

empiricompany commented Aug 19, 2023

fballiano commented Sep 20, 2023 • edited Loading

empiricompany commented Sep 20, 2023

fballiano commented Sep 20, 2023

empiricompany commented Sep 20, 2023

fballiano commented Sep 20, 2023

empiricompany commented Sep 21, 2023

fballiano commented Sep 21, 2023

fballiano Sep 21, 2023

Choose a reason for hiding this comment

empiricompany Sep 21, 2023

Choose a reason for hiding this comment

fballiano Sep 21, 2023 • edited Loading

Choose a reason for hiding this comment

empiricompany Sep 21, 2023

Choose a reason for hiding this comment

fballiano Sep 22, 2023

Choose a reason for hiding this comment

empiricompany Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

empiricompany Sep 22, 2023

Choose a reason for hiding this comment

fballiano Sep 22, 2023

Choose a reason for hiding this comment

empiricompany Sep 22, 2023

Choose a reason for hiding this comment

sreichel commented Aug 6, 2024 • edited Loading

sreichel commented Aug 6, 2024 • edited Loading

empiricompany commented Aug 7, 2024

empiricompany commented Aug 7, 2024

sreichel commented Aug 8, 2024

empiricompany commented Aug 8, 2024

sreichel commented Aug 8, 2024

empiricompany commented Aug 9, 2024

empiricompany commented Aug 19, 2023 •

edited

Loading

empiricompany commented Aug 19, 2023 •

edited

Loading

fballiano commented Sep 20, 2023 •

edited

Loading

fballiano Sep 21, 2023 •

edited

Loading

empiricompany Sep 22, 2023 •

edited

Loading

sreichel commented Aug 6, 2024 •

edited

Loading

sreichel commented Aug 6, 2024 •

edited

Loading