Merge lp:~openerp-dev/openobject-server/6.0-opw-4630-jvo into lp:openobject-server/6.0

[Jay Vora (Serpent Consulting Services)]

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

Something I missed in my first look: the search on ir.values should instead rely on the 'value' column, which will hold the reference in the form 'ir.actions.act_window,42', while the 'model' column is in fact related to the object to which that ir.values entry related too.

Here's an illustration:
=> select id,key2,model,value from ir_values where model = 'res.partner' limit 1;
 id | key2 | model | value
----+--------------------+-------------+--------------------------
 49 | client_print_multi | res.partner | ir.actions.report.xml,56

So we'll want to do something like:

ir_value_ids = pool_ir_values.search(cr, uid,
           [('value','in',['%s,%s' % (self._name,sid) for sid in sub_ids])],
           context=context)
pool_ir_values.unlink(cr, uid, ir_value_ids, context=context)

review: Needs Fixing

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

Another thing I noticed is that we don't check whether the browse_record that is returned in get_object() actually corresponds to a valid record. Indeed you can create a browse_record for an ID that does not exist, and it will only raise errors when you really access its data.

To be consistent, we need to check this as well and raise a ValueError in that case.

I'm going to add a little patch for that.

review: Needs Fixing

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

Jay, would you please double-check this branch after the latest patches?
To me it looks ok now, but I'll let you merge it after a last check.

review: Approve

[Jay Vora (Serpent Consulting Services) (jayvora)]

Olivier, Sure I will do some tests and will end this proposal into a commit :).

[Jay Vora (Serpent Consulting Services) (jayvora)]

Olivier,

Thanks for the peer review and discussion over this topic which has ended into an exemplary commit.

=== modified file 'bin/addons/base/ir/ir_model.py'
--- bin/addons/base/ir/ir_model.py	2011-01-20 16:27:06 +0000
+++ bin/addons/base/ir/ir_model.py	2011-03-23 19:21:36 +0000
@@ -608,12 +608,17 @@
         """Returns (model, res_id) corresponding to a given module and xml_id (cached) or raise ValueError if not found"""
         data_id = self._get_id(cr, uid, module, xml_id)
         res = self.read(cr, uid, data_id, ['model', 'res_id'])
+        if not res['res_id']:
+            raise ValueError('No references to %s.%s' % (module, xml_id))
         return (res['model'], res['res_id'])
 
     def get_object(self, cr, uid, module, xml_id, context=None):
         """Returns a browsable record for the given module name and xml_id or raise ValueError if not found"""
         res_model, res_id = self.get_object_reference(cr, uid, module, xml_id)
-        return self.pool.get(res_model).browse(cr, uid, res_id, context=context)
+        result = self.pool.get(res_model).browse(cr, uid, res_id, context=context)
+        if not result.exists():
+            raise ValueError('No record found for unique ID %s.%s. It may have been deleted.' % (module, xml_id))
+        return result
 
     def _update_dummy(self,cr, uid, model, module, xml_id=False, store=True):
         if not xml_id:

=== modified file 'bin/osv/orm.py'
--- bin/osv/orm.py	2011-03-11 08:27:20 +0000
+++ bin/osv/orm.py	2011-03-23 19:21:36 +0000
@@ -3227,9 +3227,26 @@
 
 
         self.check_access_rule(cr, uid, ids, 'unlink', context=context)
+        pool_model_data = self.pool.get('ir.model.data')
+        pool_ir_values = self.pool.get('ir.values')
         for sub_ids in cr.split_for_in_conditions(ids):
             cr.execute('delete from ' + self._table + ' ' \
                        'where id IN %s', (sub_ids,))
+
+            # Removing the ir_model_data reference if the record being deleted is a record created by xml/csv file,
+            # as these are not connected with real database foreign keys, and would be dangling references.
+            # Step 1. Calling unlink of ir_model_data only for the affected IDS.
+            referenced_ids = pool_model_data.search(cr, uid, [('res_id','in',list(sub_ids)),('model','=',self._name)], context=context)
+            # Step 2. Marching towards the real deletion of referenced records
+            pool_model_data.unlink(cr, uid, referenced_ids, context=context)
+
+            # For the same reason, removing the record relevant to ir_values
+            ir_value_ids = pool_ir_values.search(cr, uid,
+                    ['|',('value','in',['%s,%s' % (self._name, sid) for sid in sub_ids]),'&',('res_id','in',list(sub_ids)),('model','=',self._name)],
+                    context=context)
+            if ir_value_ids:
+                pool_ir_values.unlink(cr, uid, ir_value_ids, context=context)
+
         for order, object, store_ids, fields in result_store:
             if object != self._name:
                 obj = self.pool.get(object)
@@ -3237,6 +3254,7 @@
                 rids = map(lambda x: x[0], cr.fetchall())
                 if rids:
                     obj._store_set_values(cr, uid, rids, fields, context)
+
         return True
 
     #