shipperhq / module-shipper

Base ShipperHQ Repo
Open Software License 3.0
21 stars 21 forks source link

sales_order_view.xml: Commented out code should actually be uncommented by default #97

Closed audio-engineer closed 2 years ago

audio-engineer commented 3 years ago

In src/view/adminhtml/layout/sales_order_view.xml there is a section of code that has been added but commented out:

<!-- using plugin instead due to following issue -->
<!--https://github.com/magento/magento2/issues/3724 -->
<!--<referenceBlock name="default">-->
<!--<arguments>-->
<!--<argument name="columns" xsi:type="array">-->
<!--<item name="carriergroup" xsi:type="string" translate="false">col-carriergroup</item>-->
<!--<item name="carriergroup_shipping" xsi:type="string" translate="false">col-carriergroup_shipping</item>-->
<!--</argument>-->
<!--</arguments>-->
<!--</referenceBlock>-->

The code in the comment adds an array of columns to the block class, which is actually the preferred way to add columns to the order_items grid.

But by default, this module uses a plugin approach instead to add the columns after the getColumns() method in Magento\Sales\Block\Adminhtml\Order\View\Items\Renderer\DefaultRenderer. This approach was chosen due to a bug that has been fixed since it was reported back in 2016, and therefore the plugin approach is no longer necessary.

Furthermore, the referenceBlock node references the default block, but should actually reference the default_order_items_renderer block.

As the module is configured now, the plugin class creates bugs and conflicts when trying to add additional columns with other modules, and should therefore be removed.

I can work on this and create a pull request.

wsadasmit commented 3 years ago

Thanks for raising, @audio-engineer. If you have code written already we would be happy to review a PR you submit, otherwise let us know and we'll add this to our roadmap.

audio-engineer commented 3 years ago

Hello @wsadasmit. Thank you, I will create a PR later today!

wsajosh commented 2 years ago

Thanks for this @audio-engineer ! I've just merged the PR and will be creating a new release which includes this change