Code review comment for lp:~initos.com/openerp-connector-magento/7.0-magentoerpconnect_options_active

Revision history for this message
Guewen Baconnier @ Camptocamp (gbaconnier-c2c) wrote :

> from our test side we found also that 'replace' was ignored if you have a
> customer backend setup on top. Is this also a limitation or a bug?

You customer's mapper (or ConnectorUnit) has to inherit from the class that you want to extend. Here, it has to inherit from magentoerpconnect_options_active.product.ProductImportMapper (this is a normal python inheritance).

>
> As i saw that you already merge the is_active method from Jan
> https://code.launchpad.net/~jan-philipp-fischer/openerp-connector-magento/7.0
> -deactivated-products/+merge/211014 to the next release i was playing with the
> 'replace'
>
> A other solution maybe is to have a new backend typ inherits from the
> 'magento' but this also conflicts with other stuff to replace.
>
> Over all, i prefer to remove 'is_active' from the core connector and remove
> replace from my module.

By proposal was to propose a default in the core module, but using a dedicated mapper

  In the core module:

    {new}
    class IsActiveProductImportMapper(ImportMapper):
        _model_name = 'magento.product.product'
        @mapping
        def is_active(self, record):
            return {default_mapping}

    {modify}
    class ProductImportMapper(ImportMapper):
        @mapping
        def is_active(self, record):
            mapper = self.get_connector_unit_for_model(IsActiveProductImportMapper)
            return mapper.map_record(record).values()

  In the extension module:
    {inherit the specialized mapper instead of the common one}
    @magento(replacing=product.IsActiveProductImportMapper)
    class IsActiveProductImportMapper(product.IsActiveProductImportMapper):

This is the only way actually to modify a mapper "horizontally" and prevent conflicts between modules.
A way to avoid that could be to implement a mapper pipeline (but it would not resolve the issue for the other ConnectorUnits).

« Back to merge proposal