Merge lp:~camptocamp/openerp-connector/7.0-connector-closure-functions into lp:~openerp-connector-core-editors/openerp-connector/7.0

[Guewen Baconnier @ Camptocamp]

[ADD] Added the possibility to use modifiers functions in the 'direct' mappings.

It allows to reduce the code in the @mappings methods as common modifiers can be
used. The change also include 3 modifiers: one for converting the type of the source field, one for getting the binding or openerp id of a backend field, one for getting the backend id of an openerp id or binding id.

Implements the blueprint: https://blueprints.launchpad.net/openerp-connector/+spec/functions-in-direct-bindings

Allows to use Modifiers in the direct mappings.

In the mappings, we have the possibility to defined 'direct' mappings. Example:

    direct = [('source_field', 'target_field')]

Integers, chars, etc. are copied. IDs of m2o relations are passed to their Binder which returns the external ID from the binding ID (or conversely returns the binding ID from the external ID).
I observed that we often need to apply a similar transformation on the source values, noted examples:
 - A backend sends '0000-00-00' for empty dates, we have to replace that by None
 - A backend sends dates in ISO 8601 format, we need to convert them to UTC then to a naive timestamp
 - A backend sends float as integer (float*100)
 - We want a default value when a value is empty
 - And in fact the most common operation is to find an external ID for a local ID and conversely, it was automatic but only for IDs of bindings
We could already do the changes using @mapping methods, but it was very repetitive.
That's why I introduced the "modifiers". Example:

    direct [(iso8601('source_date'), 'target_date'),
            (ifmissing('source_field', 'default value'), 'target_field')]

They also resolve an issue with the m2o: they were implicitly trying to search a Binder for the relation of the m2o, so not working if the relation was a "normal record". Now, I recommend to explicit the operation:

        # partner_id relation's 'res.partner' (normal record)
        # mag_partner_id relation's 'magento.res.partner' (binding)
        # s_partner_id is the id on the backend
    direct = [(backend_to_m2o('s_partner_id', binding='magento.res.partner'), 'partner_id'), # we needed to give the binding model so it can use it to find the normal ID
              (backend_to_m2o('s_partner_id'), 'mag_partner_id')] # we don't give the binding model if it is the same than the relation

Example of implementation:

    def a_function(field):
        ''' ``field`` is the name of the source field '''
        def transform(self, record, to_attr):
            ''' self is the current Mapper,
                record is the current record to map,
                to_attr is the target field'''
            return record[field]
        return transform

    direct = [
            (a_function('source'), 'target'),
    ]

[lp:~camptocamp/openerp-connector/7.0-connector-closure-functions]

595. By Guewen Baconnier @ Camptocamp on 2013-10-31

[ADD] changelog

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,

Good idea, but if you apply a Higher-order function to your mapping is still a "direct"
mapping ? I find it a little bit confusing, but I get the finality of the change.

Maybe we may add a new "middleware or what else" class attribute that will allows to wrap any of the transformation (direct or not) with a wrapping function. Like in ring/wsgi etc webstack handler.

Regards

Nicolas

review: Needs Information

[lp:~camptocamp/openerp-connector/7.0-connector-closure-functions]

596. By Guewen Baconnier @ Camptocamp on 2013-10-31

[CHG] rename closures in modifier

597. By Guewen Baconnier @ Camptocamp on 2013-10-31

[FIX] previous revision, forgot the return...

598. By Guewen Baconnier @ Camptocamp on 2013-10-31

[ADD] 'none' modifier to convert False-ish values to None

599. By Guewen Baconnier @ Camptocamp on 2013-11-01

[FIX] when we use a modifier for the m2o and the relation is not a binding, we need to provide the name of the binding model

600. By Guewen Baconnier @ Camptocamp on 2013-11-01

[FIX] previous change: if we specify a binding model, it means that we have to wrap / unwrap the records

601. By Guewen Baconnier @ Camptocamp on 2013-11-05

[MRG] from master branch

602. By Guewen Baconnier @ Camptocamp on 2013-11-05

[ADD] new argument in the 'backend_to_m2o' modifier to allow binding on inactive records

[Guewen Baconnier @ Camptocamp (gbaconnier-c2c)]

> Hello,
>
> Good idea, but if you apply a Higher-order function to your mapping is still a
> "direct"
> mapping ? I find it a little bit confusing, but I get the finality of the
> change.

Maybe the name 'direct' was unfortunate in a first place. But that's still 'direct' in a sense: from 1 attribute to 1 attribute, we just place a modifier between them.

>
> Maybe we may add a new "middleware or what else" class attribute that will
> allows to wrap any of the transformation (direct or not) with a wrapping
> function. Like in ring/wsgi etc webstack handler.

My first opinion is that I think that will make the mappings more complex, the solution proposed here is straightforward and readable, example:

    direct = [(backend_to_m2o('deal_id'), 'offer_id'),
              (backend_to_m2o('product_id', binding='qoqa.product.template'), 'product_tmpl_id'),
              (backend_to_m2o('vat_id'), 'tax_id'),
              ('lot_size', 'lot_size'),
              ('unit_price', 'unit_price'),
              ]

Though, I don't think I get what you mean. Can you write an example of how you would use it on a Mapper?

>
> Regards
>
> Nicolas

[Nicolas Bessi - Camptocamp (nbessi-c2c-deactivatedaccount)]

Hello,

I was thinking about something like in Clojure transform:
(def direct {:a [inc :a]
                      :b [dec :b]
                      :c [add :a :b]
                      :d [sub :b :c]})

direct = {'offer_id': [backend_m2o, 'deal_id'],
          'product_id': [strip_id, backend_m2o, 'product_tmpl_id'],
          'cost_price': [sum, 'key_x', 'key_y']}

After discussion with Guewen I understood some use case I have missed that make uses only introspection not working.

I got your point for the variable name, maybe we should find something like one_on_one but I can leave with that.

Regards

Nicolas

[lp:~camptocamp/openerp-connector/7.0-connector-closure-functions]

603. By Guewen Baconnier @ Camptocamp on 2013-11-09

[FIX] the backward compatibility code was outdated: binding is the name of the binding to use, to provide only when the relation is not a binding

=== modified file 'connector/CHANGES.rst'
--- connector/CHANGES.rst	2013-10-09 18:52:36 +0000
+++ connector/CHANGES.rst	2013-11-09 20:51:28 +0000
@@ -4,6 +4,9 @@
 2.0.1.dev0
 ~~~~~~~~~~
 
+* Add the possibility to use 'Modifiers' functions in the 'direct
+  mappings' (details in the documentation of the Mapper class)
+
 2.0.1 (2013-09-12)
 ~~~~~~~~~~~~~~~~~~
 

=== modified file 'connector/connector.py'
--- connector/connector.py	2013-09-13 13:22:49 +0000
+++ connector/connector.py	2013-11-09 20:51:28 +0000
@@ -283,11 +283,15 @@
         """
         raise NotImplementedError
 
-    def to_backend(self, binding_id):
+    def to_backend(self, binding_id, wrap=False):
         """ Give the external ID for an OpenERP binding ID
         (ID in a model magento.*)
 
         :param binding_id: OpenERP binding ID for which we want the backend id
+        :param wrap: if False, binding_id is the ID of the binding,
+                     if True, binding_id is the ID of the normal record, the
+                     method will search the corresponding binding and returns
+                     the backend id of the binding
         :return: external ID of the record
         """
         raise NotImplementedError

=== modified file 'connector/doc/guides/concepts.rst'
--- connector/doc/guides/concepts.rst	2013-11-01 09:10:27 +0000
+++ connector/doc/guides/concepts.rst	2013-11-09 20:51:28 +0000
@@ -146,8 +146,7 @@
 systems.
 
 The connector defines some base classes, which you can find below.
-Note that you can define your own ConnectorUnits as well without
-reusing them.
+Note that you can define your own ConnectorUnits as well.
 
 Mappings
 ========
@@ -159,19 +158,16 @@
 
 It supports:
 
-* direct mappings
-
+direct mappings
     Fields *a* is written in field *b*.
 
-* method mappings
-
+method mappings
     A method is used to convert one or many fields to one or many
     fields, with transformation.
     It can be filtered, for example only applied when the record is
     created or when the source fields are modified.
 
-* submapping
-
+submapping
     a sub-record (lines of a sale order) is converted using another
     Mapper
 

=== modified file 'connector/tests/test_backend.py'
--- connector/tests/test_backend.py	2013-08-23 20:34:05 +0000
+++ connector/tests/test_backend.py	2013-11-09 20:51:28 +0000
@@ -73,7 +73,7 @@
                                         self.uid)
 
     def tearDown(self):
-        super(test_backend_register, self).setUp()
+        super(test_backend_register, self).tearDown()
         BACKENDS.backends.clear()
         del self.backend._class_entries[:]
 

=== modified file 'connector/tests/test_mapper.py'
--- connector/tests/test_mapper.py	2013-04-08 11:48:59 +0000
+++ connector/tests/test_mapper.py	2013-11-09 20:51:28 +0000
@@ -2,6 +2,7 @@
 
 import unittest2
 import mock
+import openerp.tests.common as common
 
 from openerp.addons.connector.unit.mapper import (
     Mapper,
@@ -9,8 +10,16 @@
     MappingDefinition,
     changed_by,
     only_create,
+    convert,
+    m2o_to_backend,
+    backend_to_m2o,
+    none,
     mapping)
 
+from openerp.addons.connector.backend import Backend
+from openerp.addons.connector.connector import Environment
+from openerp.addons.connector.session import ConnectorSession
+
 
 class test_mapper(unittest2.TestCase):
     """ Test Mapper """
@@ -234,3 +243,112 @@
                                          'discount': .5})]
                     }
         self.assertEqual(mapper.data_for_create, expected)
+
+    def test_mapping_modifier(self):
+        """ Map a direct record with a modifier function """
+
+        def do_nothing(field):
+            def transform(self, record, to_attr):
+                return record[field]
+            return transform
+
+        class MyMapper(ImportMapper):
+            direct = [(do_nothing('name'), 'out_name')]
+
+        env = mock.MagicMock()
+        record = {'name': 'Guewen'}
+        mapper = MyMapper(env)
+        mapper.convert(record)
+        expected = {'out_name': 'Guewen'}
+        self.assertEqual(mapper.data, expected)
+        self.assertEqual(mapper.data_for_create, expected)
+
+    def test_mapping_convert(self):
+        """ Map a direct record with the convert modifier function """
+        class MyMapper(ImportMapper):
+            direct = [(convert('name', int), 'out_name')]
+
+        env = mock.MagicMock()
+        record = {'name': '300'}
+        mapper = MyMapper(env)
+        mapper.convert(record)
+        expected = {'out_name': 300}
+        self.assertEqual(mapper.data, expected)
+        self.assertEqual(mapper.data_for_create, expected)
+
+    def test_mapping_modifier_none(self):
+        """ Pipeline of modifiers """
+        class MyMapper(ImportMapper):
+            direct = [(none('in_f'), 'out_f'),
+                      (none('in_t'), 'out_t')]
+
+        env = mock.MagicMock()
+        record = {'in_f': False, 'in_t': True}
+        mapper = MyMapper(env)
+        mapper.convert(record)
+        expected = {'out_f': None, 'out_t': True}
+        self.assertEqual(mapper.data, expected)
+        self.assertEqual(mapper.data_for_create, expected)
+
+    def test_mapping_modifier_pipeline(self):
+        """ Pipeline of modifiers """
+        class MyMapper(ImportMapper):
+            direct = [(none(convert('in_f', bool)), 'out_f'),
+                      (none(convert('in_t', bool)), 'out_t')]
+
+        env = mock.MagicMock()
+        record = {'in_f': 0, 'in_t': 1}
+        mapper = MyMapper(env)
+        mapper.convert(record)
+        expected = {'out_f': None, 'out_t': True}
+        self.assertEqual(mapper.data, expected)
+        self.assertEqual(mapper.data_for_create, expected)
+
+
+class test_mapper_binding(common.TransactionCase):
+    """ Test Mapper with Bindings"""
+
+    def setUp(self):
+        super(test_mapper_binding, self).setUp()
+        self.session = ConnectorSession(self.cr, self.uid)
+        self.Partner = self.registry('res.partner')
+        self.backend = mock.Mock(wraps=Backend('x', version='y'),
+                                 name='backend')
+        backend_record = mock.Mock()
+        backend_record.get_backend.return_value = self.backend
+        self.env = Environment(backend_record, self.session, 'res.partner')
+        self.country_binder = mock.Mock(name='country_binder')
+        self.country_binder.return_value = self.country_binder
+        self.backend.get_class.return_value = self.country_binder
+
+    def test_mapping_m2o_to_backend(self):
+        """ Map a direct record with the m2o_to_backend modifier function """
+        class MyMapper(ImportMapper):
+            _model_name = 'res.partner'
+            direct = [(m2o_to_backend('country_id'), 'country')]
+
+        partner_id = self.ref('base.main_partner')
+        self.Partner.write(self.cr, self.uid, partner_id,
+                           {'country_id': self.ref('base.ch')})
+        partner = self.Partner.browse(self.cr, self.uid, partner_id)
+        self.country_binder.to_backend.return_value = 10
+
+        mapper = MyMapper(self.env)
+        mapper.convert(partner)
+        self.country_binder.to_backend.assert_called_once_with(
+            partner.country_id.id, wrap=False)
+        self.assertEqual(mapper.data, {'country': 10})
+
+    def test_mapping_backend_to_m2o(self):
+        """ Map a direct record with the backend_to_m2o modifier function """
+        class MyMapper(ImportMapper):
+            _model_name = 'res.partner'
+            direct = [(backend_to_m2o('country'), 'country_id')]
+
+        record = {'country': 10}
+        self.country_binder.to_openerp.return_value = 44
+        mapper = MyMapper(self.env)
+        mapper.convert(record)
+        self.country_binder.to_openerp.assert_called_once_with(
+            10, unwrap=False)
+        self.assertEqual(mapper.data, {'country_id': 44})

=== modified file 'connector/unit/mapper.py'
--- connector/unit/mapper.py	2013-10-28 15:17:17 +0000
+++ connector/unit/mapper.py	2013-11-09 20:51:28 +0000
@@ -60,6 +60,142 @@
     return func
 
 
+def none(field):
+    """ A modifier intended to be used on the ``direct`` mappings.
+
+    Replace the False-ish values by None.
+    It can be used in a pipeline of modifiers when .
+
+    Example::
+
+        direct = [(none('source'), 'target'),
+                  (none(m2o_to_backend('rel_id'), 'rel_id')]
+
+    :param field: name of the source field in the record
+    :param binding: True if the relation is a binding record
+    """
+    def modifier(self, record, to_attr):
+        if callable(field):
+            result = field(self, record, to_attr)
+        else:
+            result = record[field]
+        if not result:
+            return None
+        return result
+    return modifier
+
+
+def convert(field, conv_type):
+    """ A modifier intended to be used on the ``direct`` mappings.
+
+    Convert a field's value to a given type.
+
+    Example::
+
+        direct = [(convert('source', str), 'target')]
+
+    :param field: name of the source field in the record
+    :param binding: True if the relation is a binding record
+    """
+    def modifier(self, record, to_attr):
+        value = record[field]
+        if not value:
+            return False
+        return conv_type(value)
+    return modifier
+
+
+def m2o_to_backend(field, binding=None):
+    """ A modifier intended to be used on the ``direct`` mappings.
+
+    For a many2one, get the ID on the backend and returns it.
+
+    When the field's relation is not a binding (i.e. it does not point to
+    something like ``magento.*``), the binding model needs to be provided
+    in the ``binding`` keyword argument.
+
+    Example::
+
+        direct = [(m2o_to_backend('country_id', binding='magento.res.country'),
+                   'country'),
+                  (m2o_to_backend('magento_country_id'), 'country')]
+
+    :param field: name of the source field in the record
+    :param binding: name of the binding model is the relation is not a binding
+    """
+    def modifier(self, record, to_attr):
+        if not record[field]:
+            return False
+        column = self.model._all_columns[field].column
+        if column._type != 'many2one':
+            raise ValueError('The column %s should be a many2one, got %s' %
+                             field, column._type)
+        rel_id = record[field].id
+        if binding is None:
+            binding_model = column._obj
+        else:
+            binding_model = binding
+        binder = self.get_binder_for_model(binding_model)
+        # if a relation is not a binding, we wrap the record in the
+        # binding, we'll return the id of the binding
+        wrap = bool(binding)
+        value = binder.to_backend(rel_id, wrap=wrap)
+        if not value:
+            raise MappingError("Can not find an external id for record "
+                               "%s in model %s %s wrapping" %
+                               (rel_id, binding_model,
+                                'with' if wrap else 'without'))
+        return value
+    return modifier
+
+
+def backend_to_m2o(field, binding=None, with_inactive=False):
+    """ A modifier intended to be used on the ``direct`` mappings.
+
+    For a field from a backend which is an ID, search the corresponding
+    binding in OpenERP and returns its ID.
+
+    When the field's relation is not a binding (i.e. it does not point to
+    something like ``magento.*``), the binding model needs to be provided
+    in the ``binding`` keyword argument.
+
+    Example::
+
+        direct = [(backend_to_m2o('country', binding='magento.res.country'),
+                   'country_id'),
+                  (backend_to_m2o('country'), 'magento_country_id')]
+
+    :param field: name of the source field in the record
+    :param binding: name of the binding model is the relation is not a binding
+    :param with_inactive: include the inactive records in OpenERP in the search
+    """
+    def modifier(self, record, to_attr):
+        if not record[field]:
+            return False
+        column = self.model._all_columns[to_attr].column
+        if column._type != 'many2one':
+            raise ValueError('The column %s should be a many2one, got %s' %
+                             to_attr, column._type)
+        rel_id = record[field]
+        if binding is None:
+            binding_model = column._obj
+        else:
+            binding_model = binding
+        binder = self.get_binder_for_model(binding_model)
+        # if we want the ID of a normal record, not a binding,
+        # we ask the unwrapped id to the binder
+        unwrap = bool(binding)
+        with self.session.change_context({'active_test': False}):
+            value = binder.to_openerp(rel_id, unwrap=unwrap)
+        if not value:
+            raise MappingError("Can not find an existing %s for external "
+                               "record %s %s unwrapping" %
+                               (binding_model, rel_id,
+                                'with' if unwrap else 'without'))
+        return value
+    return modifier
+
+
 MappingDefinition = namedtuple('MappingDefinition',
                                ['changed_by',
                                 'only_create'])
@@ -106,7 +242,90 @@
 
 
 class Mapper(ConnectorUnit):
-    """ Transform a record to a defined output """
+    """ A Mapper translates an external record to an OpenERP record and
+    conversely. The output of a Mapper is a ``dict``.
+
+    3 types of mappings are supported:
+
+    Direct Mappings
+        Example::
+
+            direct = [('source', 'target')]
+
+        Here, the ``source`` field will be copied in the ``target`` field.
+
+        A modifier can be used in the source item.
+        The modifier will be applied to the source field before being
+        copied in the target field.
+        It should be a closure function respecting this idiom::
+
+            def a_function(field):
+                ''' ``field`` is the name of the source field '''
+                def modifier(self, record, to_attr):
+                    ''' self is the current Mapper,
+                        record is the current record to map,
+                        to_attr is the target field'''
+                    return record[field]
+                return modifier
+
+        And used like that::
+
+            direct = [
+                    (a_function('source'), 'target'),
+            ]
+
+        A more concrete example of modifier::
+
+            def convert(field, conv_type):
+                ''' Convert the source field to a defined ``conv_type``
+                (ex. str) before returning it'''
+                def modifier(self, record, to_attr):
+                    value = record[field]
+                    if not value:
+                        return None
+                    return conv_type(value)
+            return modifier
+
+        And used like that::
+
+            direct = [
+                    (convert('myfield', float), 'target_field'),
+            ]
+
+        More examples of modifiers:
+
+        * :py:func:`convert`
+        * :py:func:`m2o_to_backend`
+        * :py:func:`backend_to_m2o`
+
+    Method Mappings
+        A mapping method allows to execute arbitrary code and return one
+        or many fields::
+
+            @mapping
+            def compute_state(self, record):
+                # compute some state, using the ``record`` or not
+                state = 'pending'
+                return {'state': state}
+
+        We can also specify that a mapping methods should be applied
+        only when an object is created, and never applied on further
+        updates::
+
+            @only_create
+            @mapping
+            def default_warehouse(self, record):
+                # get default warehouse
+                warehouse_id = ...
+                return {'warehouse_id': warehouse_id}
+
+    Submappings
+        When a record contains sub-items, like the lines of a sales order,
+        we can convert the children using another Mapper::
+
+            children = [('items', 'line_ids', LineMapper)]
+
+    """
 
     __metaclass__ = MetaMapper
 
@@ -136,6 +355,14 @@
         return result
 
     def _map_direct(self, record, from_attr, to_attr):
+        """ Apply the ``direct`` mappings.
+
+        :param record: record to convert from a source to a target
+        :param from_attr: name of the source attribute or a callable
+        :type from_attr: callable | str
+        :param to_attr: name of the target attribute
+        :type to_attr: str
+        """
         raise NotImplementedError
 
     def _map_children(self, record, attr, model):
@@ -157,9 +384,6 @@
         _logger.debug('converting record %s to model %s', record, self._model_name)
         for from_attr, to_attr in self.direct:
             if (not fields or from_attr in fields):
-                # XXX not compatible with all
-                # record type (wrap
-                # records in a standard class representation?)
                 value = self._map_direct(record,
                                          from_attr,
                                          to_attr)
@@ -262,20 +486,29 @@
     """ Transform a record from a backend to an OpenERP record """
 
     def _map_direct(self, record, from_attr, to_attr):
+        """ Apply the ``direct`` mappings.
+
+        :param record: record to convert from a source to a target
+        :param from_attr: name of the source attribute or a callable
+        :type from_attr: callable | str
+        :param to_attr: name of the target attribute
+        :type to_attr: str
+        """
+        if callable(from_attr):
+            return from_attr(self, record, to_attr)
+
         value = record.get(from_attr)
         if not value:
             return False
 
+        # Backward compatibility: when a field is a relation, and a modifier is
+        # not used, we assume that the relation model is a binding.
+        # Use an explicit modifier backend_to_m2o in the 'direct' mappings to
+        # change that.
         column = self.model._all_columns[to_attr].column
         if column._type == 'many2one':
-            rel_id = record[from_attr]
-            model_name = column._obj
-            binder = self.get_binder_for_model(model_name)
-            value = binder.to_openerp(rel_id)
-
-            if not value:
-                raise MappingError("Can not find an existing %s for external "
-                                   "record %s" % (model_name, rel_id))
+            mapping = backend_to_m2o(from_attr)
+            value = mapping(self, record, to_attr)
         return value
 
     def _init_child_mapper(self, model_name):
@@ -292,20 +525,29 @@
     """ Transform a record from OpenERP to a backend record """
 
     def _map_direct(self, record, from_attr, to_attr):
+        """ Apply the ``direct`` mappings.
+
+        :param record: record to convert from a source to a target
+        :param from_attr: name of the source attribute or a callable
+        :type from_attr: callable | str
+        :param to_attr: name of the target attribute
+        :type to_attr: str
+        """
+        if callable(from_attr):
+            return from_attr(self, record, to_attr)
+
         value = record[from_attr]
         if not value:
             return False
 
+        # Backward compatibility: when a field is a relation, and a modifier is
+        # not used, we assume that the relation model is a binding.
+        # Use an explicit modifier m2o_to_backend  in the 'direct' mappings to
+        # change that.
         column = self.model._all_columns[from_attr].column
         if column._type == 'many2one':
-            rel_id = record[from_attr].id
-            model_name = column._obj
-            binder = self.get_binder_for_model(model_name)
-            value = binder.to_backend(rel_id)
-
-            if not value:
-                raise MappingError("Can not find an external id for record "
-                                   "%s in model %s" % (rel_id, model_name))
+            mapping = m2o_to_backend(from_attr)
+            value = mapping(self, record, to_attr)
         return value
 
     def _init_child_mapper(self, model_name):