Merge lp:~openerp-community/openobject-server/trunk-property-fields-using-get into lp:openobject-server

[qdp (OpenERP)]

Enclosed, an improvement that targets to use the get() function of ir.property object when reading a property field (using a browse). Thanks to that, it will now be possible to force the company to use for property fields when processing flows.

While developing that feature, we had to clean and refactor the code of fnct_read and _get_defaults functions that were doing pretty nasty things to compute name_get on all the needed ids.

We also have added a function in tools (maybe that's not the perfect place?) that will factorize the code later on, on addons side. Check the example of use bellow for further information:
- .. for line in self.browse(cr, uid, ids, context=context):
+ .. set_company = tools.get_and_sort_by_field(cr, uid, obj=self, ids=ids, field='company_id', context=context)
+ .. for company_id, sol_ids in set_company.items():
+ ......ctx = context.copy()
+ ......ctx.update({'force_company': company_id})
+ ......for line in self.browse(cr, uid, sol_ids, context=ctx):

Thanks,

[Olivier Dony (Odoo) (odo-openerp)]

Hello,

Thanks for the improved properties management!

Some preliminary remarks (line numbers are line numbers of the current diff):

- When adding new docstrings, we try to match the standard RST docstring format as much as possible, as we are trying to globally improve and unify our docstrings formats (therefore it is quite important in merge props). Example:

  def squares(limit):
      """Computes the square of the first positive ints, up to ``limit``.

         :param int limit: max integer to compute (excluded)
         :return: map of integers to their square
         :rtype: dict
      """
      return dict([(x, x**2) for x in xrange(limit)])

- l.40 of the diff, in _get_defaults, what is the weird loop? That's always a one-item list, so there is no reason for looping at all, nor for making the list in the first place?

- in _fnct_read(), the test `isinstance(x, orm.browse_record)` is repeated many times within the inner loops, while it should only be in the outer loop, and just once. It also seems incorrect most of the times, as it tests the default value type, but the final value is the one that is used. What if there is no default but a real value is indeed set?
Looks like this should instead be based on the property's 'type' column, shouldn't it?
The inner loops could be quite simplified too.

- l.108 in _fnct_read(): name_get() must explicitly be called as UID 1, because it is quite possible for the target record to be a record the user is not allowed to access, and seeing the name_get() is still allowed (the permission of the parent object is the one that counts). Didn't you notice the explicit comment about that in the code? ;-)

- similarly, the code had an explicit check for the existence of the target records of m2o property values, because there is not integrity management for properties done by the database. Removing that code will certainly cause regressions, unless you have taken care of it somewhere else? BTW this test must be done as UID 1 for the same reason as above. Didn't you notice the explicit comment about that stuff in the code? ;-)

- about the utility function get_and_sort_by_field(): it does not strike me as a frequent use case, so perhaps it's better to make a specific function that gives the ids of records grouped by company_id? It could be based on this generic function, but perhaps the purpose would be more clear...
  + BTW, was naming a dict value "key" a sort of joke? (l.136) ;-)
  + l.139 will not work as "set_field" is not defined, plus these 3 lines should be replaced by:
       res.setdefault(key,[]).append(item['id'])

A more complete review will probably follow with some real testing of the code.

Thanks!

-- odo, volunteer framework reviewing monkey

review: Needs Fixing

[qdp (OpenERP) (qdp)]

woops,

i've nothing to add nor any excuse. I'd better hide away... but i need to ask for a second review. How fool i am!

Thanks for your patience,
--qdp, "Monkey coding" addict against his will

review: Needs Resubmitting

[Olivier Dony (Odoo) (odo-openerp)]

After the last set of changes, I think can be merged :-)

review: Approve

=== modified file 'openerp/osv/fields.py'
--- openerp/osv/fields.py	2011-07-05 08:53:22 +0000
+++ openerp/osv/fields.py	2011-07-12 08:38:26 +0000
@@ -1212,34 +1212,30 @@
         self._symbol_get = self._deserialize_func
         super(serialized, self).__init__(string=string, **args)
 
-
 # TODO: review completly this class for speed improvement
 class property(function):
 
     def _get_default(self, obj, cr, uid, prop_name, context=None):
-        return self._get_defaults(obj, cr, uid, [prop_name], context=None)[0][prop_name]
-
-    def _get_defaults(self, obj, cr, uid, prop_name, context=None):
+        return self._get_defaults(obj, cr, uid, [prop_name], context=None)[prop_name]
+
+    def _get_defaults(self, obj, cr, uid, prop_names, context=None):
+        """Get the default values for ``prop_names´´ property fields (result of ir.property.get() function for res_id = False).
+
+           :param list of string prop_names: list of name of property fields for those we want the default value
+           :return: map of property field names to their default value
+           :rtype: dict
+        """
         prop = obj.pool.get('ir.property')
-        domain = [('fields_id.model', '=', obj._name), ('fields_id.name','in',prop_name), ('res_id','=',False)]
-        ids = prop.search(cr, uid, domain, context=context)
-        replaces = {}
-        default_value = {}.fromkeys(prop_name, False)
-        for prop_rec in prop.browse(cr, uid, ids, context=context):
-            if default_value.get(prop_rec.fields_id.name, False):
-                continue
-            value = prop.get_by_record(cr, uid, prop_rec, context=context) or False
-            default_value[prop_rec.fields_id.name] = value
-            if value and (prop_rec.type == 'many2one'):
-                replaces.setdefault(value._name, {})
-                replaces[value._name][value.id] = True
-        return default_value, replaces
+        res = {}
+        for prop_name in prop_names:
+            res[prop_name] = prop.get(cr, uid, prop_name, obj._name, context=context)
+        return res
 
     def _get_by_id(self, obj, cr, uid, prop_name, ids, context=None):
         prop = obj.pool.get('ir.property')
         vids = [obj._name + ',' + str(oid) for oid in  ids]
 
-        domain = [('fields_id.model', '=', obj._name), ('fields_id.name','in',prop_name)]
+        domain = [('fields_id.model', '=', obj._name), ('fields_id.name', 'in', prop_name)]
         #domain = prop._get_domain(cr, uid, prop_name, obj._name, context)
         if vids:
             domain = [('res_id', 'in', vids)] + domain
@@ -1274,48 +1270,48 @@
             }, context=context)
         return False
 
-
-    def _fnct_read(self, obj, cr, uid, ids, prop_name, obj_dest, context=None):
-        properties = obj.pool.get('ir.property')
-        domain = [('fields_id.model', '=', obj._name), ('fields_id.name','in',prop_name)]
-        domain += [('res_id','in', [obj._name + ',' + str(oid) for oid in  ids])]
-        nids = properties.search(cr, uid, domain, context=context)
-        default_val,replaces = self._get_defaults(obj, cr, uid, prop_name, context)
-
+    def _fnct_read(self, obj, cr, uid, ids, prop_names, obj_dest, context=None):
+        prop = obj.pool.get('ir.property')
+        # get the default values (for res_id = False) for the property fields 
+        default_val = self._get_defaults(obj, cr, uid, prop_names, context)
+
+        # build the dictionary that will be returned
         res = {}
         for id in ids:
             res[id] = default_val.copy()
 
-        brs = properties.browse(cr, uid, nids, context=context)
-        for prop in brs:
-            value = properties.get_by_record(cr, uid, prop, context=context)
-            res[prop.res_id.id][prop.fields_id.name] = value or False
-            if value and (prop.type == 'many2one'):
-                # check existence as root, as seeing the name of a related
-                # object depends on access right of source document,
-                # not target, so user may not have access.
-                record_exists = obj.pool.get(value._name).exists(cr, 1, value.id)
-                if record_exists:
-                    replaces.setdefault(value._name, {})
-                    replaces[value._name][value.id] = True
-                else:
-                    res[prop.res_id.id][prop.fields_id.name] = False
-
-        for rep in replaces:
-            # search+name_get as root, as seeing the name of a related
-            # object depends on access right of source document,
-            # not target, so user may not have access.
-            nids = obj.pool.get(rep).search(cr, 1, [('id','in',replaces[rep].keys())], context=context)
-            replaces[rep] = dict(obj.pool.get(rep).name_get(cr, 1, nids, context=context))
-
-        for prop in prop_name:
+        for prop_name in prop_names:
+            property_field = obj._all_columns.get(prop_name).column
+            property_destination_obj = property_field._obj if property_field._type == 'many2one' else False
+            # If the property field is a m2o field, we will append the id of the value to name_get_ids
+            # in order to make a name_get in batch for all the ids needed.
+            name_get_ids = {}
             for id in ids:
-                if res[id][prop] and hasattr(res[id][prop], '_name'):
-                    res[id][prop] = (res[id][prop].id , replaces[res[id][prop]._name].get(res[id][prop].id, False))
-
+                # get the result of ir.property.get() for this res_id and save it in res if it's existing
+                obj_reference = obj._name + ',' + str(id)
+                value = prop.get(cr, uid, prop_name, obj._name, res_id=obj_reference, context=context)
+                if value:
+                    res[id][prop_name] = value
+                # Check existence as root (as seeing the name of a related
+                # object depends on access right of source document,
+                # not target, so user may not have access) in order to avoid
+                # pointing on an unexisting record.
+                if property_destination_obj:
+                    if res[id][prop_name] and obj.pool.get(property_destination_obj).exists(cr, 1, res[id][prop_name].id):
+                        name_get_ids[id] = res[id][prop_name].id
+                    else:
+                        res[id][prop_name] = False
+            if property_destination_obj:
+                # name_get as root (as seeing the name of a related
+                # object depends on access right of source document,
+                # not target, so user may not have access.)
+                name_get_values = dict(obj.pool.get(property_destination_obj).name_get(cr, 1, name_get_ids.values(), context=context))
+                # the property field is a m2o, we need to return a tuple with (id, name)
+                for k, v in name_get_ids.iteritems():
+                    if res[k][prop_name]:
+                        res[k][prop_name] = (v , name_get_values.get(v))
         return res
 
-
     def _field_get(self, cr, uid, model_name, prop):
         if not self.field_id.get(cr.dbname):
             cr.execute('SELECT id \

=== modified file 'openerp/tools/misc.py'
--- openerp/tools/misc.py	2011-06-23 09:03:57 +0000
+++ openerp/tools/misc.py	2011-07-12 08:38:26 +0000
@@ -1387,6 +1387,21 @@
     a.start()
     return True
 
+def get_and_group_by_field(cr, uid, obj, ids, field, context=None):
+    """ Read the values of ``field´´ for the given ``ids´´ and group ids by value.
+
+       :param string field: name of the field we want to read and group by
+       :return: mapping of field values to the list of ids that have it
+       :rtype: dict
+    """
+    res = {}
+    for record in obj.read(cr, uid, ids, [field], context=context):
+        key = record[field]
+        res.setdefault(key[0] if isinstance(key, tuple) else key, []).append(record['id'])
+    return res
+
+def get_and_group_by_company(cr, uid, obj, ids, context=None):
+    return get_and_group_by_field(cr, uid, obj, ids, field='company_id', context=context)
 
 # port of python 2.6's attrgetter with support for dotted notation
 def resolve_attr(obj, attr):