Product RelatedItems functionality is unextendable

[vytsci]

You have made a good start with RelatedItems, but in the end everything is so fucked up and full of hard codes. Why? Why hardcode such an useful piece of functionality?

[anyt]

Hi @vytsci, Thank you for the feedback, but it's hard to understand where have you stuck.

Please, clarify what exactly is hard to customize in related products functionality, because of hardcode.

[anyt]

To understand how related products works, you can start by reading this article https://github.com/oroinc/orocommerce/blob/master/src/Oro/Bundle/ProductBundle/Resources/doc/related-items.md . Currently, you could define different strategies for finding related items and define new related items groups, out of the box we have two of them, upsell and related products. From my perspective, this looks enough for the most common customizations.

[vytsci]

I understand how it works. What i dislike is that upsell and related products have been hardcoded rather than configured via configuration. For example you have no way to add one more related item group tab within product edit page. Ill upload screens or code parts for better understanding. Ive solved my issue via event listeners. Related items has good implementation for finder and config provider, but when it comes to displaying and saving that new group it becomes a mess.

[anyt]

For example you have no way to add one more related item group tab within the product edit page

1) You could add new form field (probably you will use collection type) with the Form Type Extension. 2) And to save these items - there are events in form handler, like \Oro\Bundle\FormBundle\Event\FormHandler\Events::BEFORE_FORM_DATA_SET. In core, we haven't used these events, because it will be over-engineering when we could do everything inline.

To easier develop and understand the code we added processing of related items in \Oro\Bundle\ProductBundle\Form\Handler\ProductUpdateHandler, I don't see any drawbacks on it, you still are able to override this handler fully if you don't want the default logic, or use events if you want to extend it.

[vytsci]

ProductUpdateHandler only processes upsell and related products, but not any custom groups. Yes Ive used events. Ill check what current version has, because ive used 2.x

[dimonixx]

To add new tabs , pretty much should be changed in RelatedItemsProductEditListener , as the tabs are rendered in it.

[vytsci]

The fact that there are methods named getUpsellProductsEdidBlock and getRelatedProductsEdidBlock means that those are hard coded. And if you look closely there is no way without overriding this class to properly add new custom group. This is the issue I am talking about. Methods for upsell and related products should not exist. Everything should be handled dynamically.

[anyt]

To add new tabs , pretty much should be changed in RelatedItemsProductEditListener , as the tabs are rendered in it.

You can create one more listener that will add a new tab, no need to modify RelatedItemsProductEditListener. For that purpose check scroll data customization documentation.

And if you look closely there is no way without overriding this class to properly add new custom group.

Yes Ive used events.

Events is the way. One of them actually. Extending or overriding the handler - is another way. And there are other ways to extend. What to choose depends on your specific requirements.

I think there is no need to use events for core functionality managed in the same bundle, if something could done explicit, it's better to not use the magic.

[dimonixx]

The RelatedItemsProductEditListener adds already rendered html .

The template on that place is completely rendered and tabs data is already set.

<div {{ UI.renderPageComponentAttributes({
    module: 'oroproduct/js/app/components/related-items-tabs-component',
    options: {
        'data': relatedItemsTabsItems
    }
}) }}></div>

How is it possible to modify properly on a scroll data event to add an additional tab ?

[vytsci]

This is the problem I am talking about. Ive used scroll data, but there is no way you can add something within rendered html. This whole piece is poorly written should be reviewed. This piece is hardcode (should be dynamic):

/**
     * @param FormInterface $form
     * @param Product $entity
     * @return bool
     */
    private function saveAllRelatedItems(FormInterface $form, Product $entity)
    {
        return $this->saveRelatedProducts($form, $entity)
            && $this->saveUpsellProducts($form, $entity);
    }

This piece also (events should be used instead):

/**
     * {@inheritDoc}
     */
    protected function saveForm(FormInterface $form, $data)
    {
        return parent::saveForm($form, $data) && $this->saveAllRelatedItems($form, $data);
    }

[vytsci]

Everything above those hardcodes goes smoothly. I have not touched "view" part yet I believe there will be issues too. Above that everything works as expected by following documentation.

[anyt]

About the tab name list, agree with you @dimonixx , just debugged and there is a hardcode.

Copy-pasting the listener to extend is not the only way. Also you could override the template and extend tabs list there, this looks easier than extending the listener.

For 3.0 and 3.1 version - create a file templates/bundles/OroProductBundle/Product/RelatedItems/tabs.html.twig with the content like

{% import 'OroUIBundle::macros.html.twig' as UI %}

{% set relatedItemsTabsItems = relatedItemsTabsItems|merge([{id:'new_tab_1', label: 'New Tab 1'}, {id:'new_tab_2', label: 'New Tab 2'}]) %}
{%  dump(relatedItemsTabsItems) %}
<div {{ UI.renderPageComponentAttributes({
    module: 'oroproduct/js/app/components/related-items-tabs-component',
    options: {
        'data': relatedItemsTabsItems
    }
}) }}></div>

where {id:'new_tab_1', label: 'New Tab 1'}, {id:'new_tab_2', label: 'New Tab 2'} representing two new tab names.

for 2.* the file path will be different: app/Resources/OroProductBundle/views/Product/RelatedItems/tabs.html.twig

Adding the content to that tabs is still possible with the new independent scroll data event listener as I see.

Created an improvement to make customization of tab names easier. Internal ticket id #BB-15898.

Thank you for contribution, @dimonixx .

[vytsci]

Yes there are solution, but that does not change the fact that code needs to be reviewed and maybe refactored. Because saving is also has workarounds, but based on whole architecture that should be dynamical as everything else is.

[anyt]

@vytsci, there is always a space to refactor something, but the question is how valuable it will be for the community. Should we rewrite it or work on some bug fixes or new features?

I've created an improvement about tabs because it's not obvious how to extend them.

According to hardcode in a ProductUpdateHandler, I don't see any extensibility or an architectural issues there. So I'm closing this ticket.

[vytsci]

Your solution with tabs.html.twig file is not a solution it looks more like a hack, but Im new here maybe I dont understand something. It should be solved completely different. Maybe Ill make a PR later to show what I mean.

[anyt]

Template with tab names is easier to maintain than extended listener. But I agree, it’s a hack, same as extension of a listener.

Tab names should be proceeded by scroll data event, that’s why I’ve created an improvement.

But this is not related to the initial issue about the hardcode you are asking to move to the listener.

[vytsci]

Im not asking anything, I just saw an issue, so I reported it. Im new with ORO. When I will get used to it ill make PR. Now I just see an issue and more experienced with ORO should decide what to do.

[anyt]

Thank you for an explanation @vytsci. My fault, I understand you wrong at the first point. Looking forward to seeing your contribution with the better solution.