Merge lp:~openerp-dev/openobject-addons/trunk-bug-832635-nch into lp:openobject-addons

[Naresh(OpenERP)]

Hello,

This merge proposal contains fix for the linked bug as well as refactoring and improvement of the audit trail module.

please provide your feedback,

Thanks,

[Antony Lesuisse (OpenERP) (al-openerp)]

Could you update your merge proposal to the lastest trunk i committed a refactoring to
- override *_cr method to reuse the cursor.
- defer any database access after checking that auditrail is actually installed.

review: Needs Fixing

[Antony Lesuisse (OpenERP) (al-openerp)]

Please use trunk version of

check_rules
execute_cr
exec_workflow_cr

instead of

check_rule_subscription
audit_log_call

because the former use _cr, are shorter and much faster (no useless db hits).
Remove all cr.commit().

review: Needs Fixing

[Naresh(OpenERP) (nch-openerp)]

hello,

made compatible to new signatures...

Thanks,

[qdp (OpenERP) (qdp)]

Hi Naresh,

i've tested but i found a problem (Not sure i didn't break it myself during the merge, though):
*given you have an audittrail rule for res.partner and res.partner.address
*given you have a partner Agrolait with 4 existing addresses
*if you create a new address for this partner, it will create you the following logs
   - one write on contatcs fields of res.partner (correct)
   - four empty writes on res.partner.address where no changes are logged (not correct)
   - one write on res.partner.address with the data you just created (incorrect, the method logged should be 'create' instead of 'write', otherwise it's ok)

i've commited in your branch my merge with trunk, not to loose my work. You should starts from it. Also, let me know if i need to assign someone else on this task, as i'm not aware of your current load (but as you made this merge prop you'll probably be faster than anyone for it ;-) ).

Btw, i'm setting this merge prop as 'work in progress' so that it's not anymore on the pending merge we need to review. Reset its state back to 'needs review' when you're work is over.

Thanks,
Quentin

review: Needs Fixing

[Naresh(OpenERP) (nch-openerp)]

Hi Qdp,

Thanks for the review ! from your review I just found 1 bug(fixed it in the last commit) and for the rest I made gave explanation. Correct me If I missed here something.

> Hi Naresh,
>
> i've tested but i found a problem (Not sure i didn't break it myself during
> the merge, though):
> *given you have an audittrail rule for res.partner and res.partner.address
> *given you have a partner Agrolait with 4 existing addresses
> *if you create a new address for this partner, it will create you the
> following logs
> - one write on contatcs fields of res.partner (correct)

> - four empty writes on res.partner.address where no changes are logged (not
> correct)

yes, actual bug Fixed it in the last commit.

> - one write on res.partner.address with the data you just created
> (incorrect, the method logged should be 'create' instead of 'write', otherwise
> it's ok)

 according to our architecture the relational table like o2m (create,unlink,write,read) operations are handled by the server itself.i.e we pass an arg at the start of the tuple with values as '0':create, '1':write,'2':unlink etc which is used by server to perform the specific operation on the relational model.now as to log we need to hack the execution of the method before the server actually executes. So if you create a parent record along with a child the client issues a 'create' call for the parent record along with the values which will be logged as create but if you add a child record in an already saved parent record the client just issues a 'write' call for the parent record(which is correct) and the server handles the creation of the child records internally. Same is the case for *unlink*.

>
> i've commited in your branch my merge with trunk, not to loose my work. You
> should starts from it. Also, let me know if i need to assign someone else on
> this task, as i'm not aware of your current load (but as you made this merge
> prop you'll probably be faster than anyone for it ;-) ).

Nop, I managed it...:)
>
> Btw, i'm setting this merge prop as 'work in progress' so that it's not
> anymore on the pending merge we need to review. Reset its state back to 'needs
> review' when you're work is over.
>
 Needs review...work over !
> Thanks,
> Quentin

Regards,
Naresh

[qdp (OpenERP) (qdp)]

> - four empty writes on res.partner.address where no changes are logged (not
> correct)
yes but no, you didn't implemented it in the right way: what you should do is avoid creating the audittrail log if there is no log line, instead of creating it then deleting it if there is no line. It will avoid useless operations.

For that, you need to do a small change:
=> replace the function
     self.create_log_line(cr, uid, log_id, model, lines)
   by
     self.create_log(cr, uid, model, lines)

def create_log(self, cr, uid, model, lines=[]):
    if not lines:
        return True
    vals = { ... }
    vals['lines] = [(0, 0, { ... }), ...]
    return self.pool.get('audittrail.log').create(cr, uid, vals, context)

> - one write on res.partner.address with the data you just created
> (incorrect, the method logged should be 'create' instead of 'write', otherwise
> it's ok)
i know, but it's not a argument. You can force the method to use. For example, try to add those 2 lines (it seems doing the trick but i didn't test completly):
=== modified file 'audittrail/audittrail.py'
--- audittrail/audittrail.py 2011-11-08 16:55:34 +0000
+++ audittrail/audittrail.py 2011-11-09 09:43:22 +0000
@@ -354,6 +357,8 @@
                     vals = {'method': method,'object_id': model.id,'user_id': uid_orig }
                     for resource in resource_data:
                         resource_id = resource['id']
+ if resource_id not in dict_to_use.keys():
+ vals.update({'method': 'create'})
                         vals.update({'res_id': resource_id})
                         log_id = log_pool.create(cr, uid, vals)
                         lines = []

i set it back to 'work in progress' ;-)
Quentin

review: Needs Fixing

[Naresh(OpenERP) (nch-openerp)]

> For that, you need to do a small change:
> => replace the function
> self.create_log_line(cr, uid, log_id, model, lines)
> by
> self.create_log(cr, uid, model, lines)
>
> def create_log(self, cr, uid, model, lines=[]):
> if not lines:
> return True
> vals = { ... }
> vals['lines] = [(0, 0, { ... }), ...]
> return self.pool.get('audittrail.log').create(cr, uid, vals, context)
>

hmm...cannot do that because lines will always be there just the difference is whether the new values is equal to the old value or not if its equal don't create log line ignore it else create it. but of-cause there is a scope of improvement here by checking the condition before appending the lines. and also create the log entry only if there is atleast 1 line.

>
> > - one write on res.partner.address with the data you just created
> > (incorrect, the method logged should be 'create' instead of 'write',
> otherwise
> > it's ok)
> i know, but it's not a argument. You can force the method to use. For example,
> try to add those 2 lines (it seems doing the trick but i didn't test
> completly):
> === modified file 'audittrail/audittrail.py'
> --- audittrail/audittrail.py 2011-11-08 16:55:34 +0000
> +++ audittrail/audittrail.py 2011-11-09 09:43:22 +0000
> @@ -354,6 +357,8 @@
> vals = {'method': method,'object_id': model.id,'user_id':
> uid_orig }
> for resource in resource_data:
> resource_id = resource['id']
> + if resource_id not in dict_to_use.keys():
> + vals.update({'method': 'create'})
> vals.update({'res_id': resource_id})
> log_id = log_pool.create(cr, uid, vals)
> lines = []

yup, didn't think of this hack !

Regards,
Naresh

[qdp (OpenERP) (qdp)]

hi again Naresh,

before proceeding, i still have few remarks:
1) in the docstring of get_value_text() please add the meaning of the params and returned value
2) please document the function start_log_process()
3) the code of inline_process_old_data() and inline_process_new_data() seems really similar. I wonder if we couldn't factorize that code. Morover, having those functions inside another function make the code a bit unreadable.
4) a general effort of documenting/commenting is needed because the code is really difficult to understand.

Next time will be the good one, i'm pretty sure of it! (otherwise, the bug is totally fixed by now and the module seems to run perfectly!)

Regards,
Quentin

review: Needs Fixing

[Naresh(OpenERP) (nch-openerp)]

Hello Quentin,

Thanks for reviewing ! I had made the changes as required and also I have rewritten the method to process and log for the actions and workflow. So you will be required to perform again a test before merge. Fix few bugs too like useless logs were created for X2M models when a 'read' operation was performed.

hope now it want come back....Thanks again...

Regards,

[qdp (OpenERP) (qdp)]

Hi Naresh,

i finished the refactoring myself. Explain what should have been done would have taken so much time and iteration...

you can check: it's merged in revision 5754.

Thanks

=== modified file 'audittrail/audittrail.py'
--- audittrail/audittrail.py	2011-10-16 01:28:00 +0000
+++ audittrail/audittrail.py	2011-11-21 16:41:25 +0000
@@ -43,22 +43,17 @@
         "log_create": fields.boolean("Log Creates",help="Select this if you want to keep track of creation on any record of the object of this rule"),
         "log_action": fields.boolean("Log Action",help="Select this if you want to keep track of actions on the object of this rule"),
         "log_workflow": fields.boolean("Log Workflow",help="Select this if you want to keep track of workflow on any record of the object of this rule"),
-        "state": fields.selection((("draft", "Draft"),
-                                   ("subscribed", "Subscribed")),
-                                   "State", required=True),
+        "state": fields.selection((("draft", "Draft"), ("subscribed", "Subscribed")), "State", required=True),
         "action_id": fields.many2one('ir.actions.act_window', "Action ID"),
-
     }
-
     _defaults = {
-        'state': lambda *a: 'draft',
-        'log_create': lambda *a: 1,
-        'log_unlink': lambda *a: 1,
-        'log_write': lambda *a: 1,
+        'state': 'draft',
+        'log_create': 1,
+        'log_unlink': 1,
+        'log_write': 1,
     }
-
     _sql_constraints = [
-        ('model_uniq', 'unique (object_id)', """There is a rule defined on this object\n You cannot define another one the same object!""")
+        ('model_uniq', 'unique (object_id)', """There is already a rule defined on this object\n You cannot define another: please edit the existing one.""")
     ]
     __functions = {}
 
@@ -178,54 +173,33 @@
 class audittrail_objects_proxy(object_proxy):
     """ Uses Object proxy for auditing changes on object of subscribed Rules"""
 
-    def get_value_text(self, cr, uid, field_name, values, model, context=None):
-        """
-        Gets textual values for the fields
-        e.g.: For field of type many2one it gives its name value instead of id
+    def get_value_text(self, cr, uid, pool, resource_pool, method, field, value):
+        """
+        Gets textual values for the fields.
+            If the field is a many2one, it returns the name.
+            If it's a one2many or a many2many, it returns a list of name.
+            In other cases, it just returns the value.
+        :param cr: the current row, from the database cursor,
+        :param uid: the current user’s ID for security checks,
+        :param pool: current db's pooler object.
+        :param resource_pool: pooler object of the model which values are being changed.
+        :param field: for which the text value is to be returned.
+        :param value: value of the field.
+        :param recursive: True or False, True will repeat the process recursively
+        :return: string value or a list of values(for O2M/M2M)
+        """
 
-        @param cr: the current row, from the database cursor,
-        @param uid: the current user’s ID for security checks,
-        @param field_name: List of fields for text values
-        @param values: Values for field to be converted into textual values
-        @return: values: List of textual values for given fields
-        """
-        if not context:
-            context = {}
-        if field_name in('__last_update','id'):
-            return values
-        pool = pooler.get_pool(cr.dbname)
-        field_pool = pool.get('ir.model.fields')
-        model_pool = pool.get('ir.model')
-        obj_pool = pool.get(model.model)
-        if obj_pool._inherits:
-            inherits_ids = model_pool.search(cr, uid, [('model', '=', obj_pool._inherits.keys()[0])])
-            field_ids = field_pool.search(cr, uid, [('name', '=', field_name), ('model_id', 'in', (model.id, inherits_ids[0]))])
+        field_obj = (resource_pool._all_columns.get(field)).column
+        if field_obj._type in ('one2many','many2many'):
+            data = pool.get(field_obj._obj).name_get(cr, uid, value)
+            #return the modifications on x2many fields as a list of names
+            res = map(lambda x:x[1], data)
+        elif field_obj._type == 'many2one':
+            #return the modifications on a many2one field as its value returned by name_get()
+            res = value and value[1] or value
         else:
-            field_ids = field_pool.search(cr, uid, [('name', '=', field_name), ('model_id', '=', model.id)])
-        field_id = field_ids and field_ids[0] or False
-        assert field_id, _("'%s' field does not exist in '%s' model" %(field_name, model.model))
-
-        field = field_pool.read(cr, uid, field_id)
-        relation_model = field['relation']
-        relation_model_pool = relation_model and pool.get(relation_model) or False
-
-        if field['ttype'] == 'many2one':
-            res = False
-            relation_id = False
-            if values and type(values) == tuple:
-                relation_id = values[0]
-                if relation_id and relation_model_pool:
-                    relation_model_object = relation_model_pool.read(cr, uid, relation_id, [relation_model_pool._rec_name])
-                    res = relation_model_object[relation_model_pool._rec_name]
-            return res
-
-        elif field['ttype'] in ('many2many','one2many'):
-            res = []
-            for relation_model_object in relation_model_pool.read(cr, uid, values, [relation_model_pool._rec_name]):
-                res.append(relation_model_object[relation_model_pool._rec_name])
-            return res
-
-        return values
+            res = value
+        return res
 
     def create_log_line(self, cr, uid, log_id, model, lines=[]):
         """
@@ -233,7 +207,7 @@
 
         @param cr: the current row, from the database cursor,
         @param uid: the current user’s ID for security checks,
-        @param model: Object who's values are being changed
+        @param model: Object which values are being changed
         @param lines: List of values for line is to be created
         """
         pool = pooler.get_pool(cr.dbname)
@@ -241,216 +215,273 @@
         model_pool = pool.get('ir.model')
         field_pool = pool.get('ir.model.fields')
         log_line_pool = pool.get('audittrail.log.line')
-        #start Loop
         for line in lines:
-            if line['name'] in('__last_update','id'):
-                continue
+            field_obj = obj_pool._all_columns.get(line['name'])
+            assert field_obj, _("'%s' field does not exist in '%s' model" %(line['name'], model.model))
+            field_obj = field_obj.column
+            old_value = line.get('old_value', '')
+            new_value = line.get('new_value', '')
+            search_models = [model.id]
             if obj_pool._inherits:
-                inherits_ids = model_pool.search(cr, uid, [('model', '=', obj_pool._inherits.keys()[0])])
-                field_ids = field_pool.search(cr, uid, [('name', '=', line['name']), ('model_id', 'in', (model.id, inherits_ids[0]))])
-            else:
-                field_ids = field_pool.search(cr, uid, [('name', '=', line['name']), ('model_id', '=', model.id)])
-            field_id = field_ids and field_ids[0] or False
-            assert field_id, _("'%s' field does not exist in '%s' model" %(line['name'], model.model))
-
-            field = field_pool.read(cr, uid, field_id)
-            old_value = 'old_value' in line and  line['old_value'] or ''
-            new_value = 'new_value' in line and  line['new_value'] or ''
-            old_value_text = 'old_value_text' in line and  line['old_value_text'] or ''
-            new_value_text = 'new_value_text' in line and  line['new_value_text'] or ''
-
-            if old_value_text == new_value_text:
-                continue
-            if field['ttype'] == 'many2one':
-                if type(old_value) == tuple:
-                    old_value = old_value[0]
-                if type(new_value) == tuple:
-                    new_value = new_value[0]
+                search_models += model_pool.search(cr, uid, [('model', 'in', obj_pool._inherits.keys())])
+            field_id = field_pool.search(cr, uid, [('name', '=', line['name']), ('model_id', 'in', search_models)])
+            if field_obj._type == 'many2one':
+                old_value = old_value and old_value[0] or old_value
+                new_value = new_value and new_value[0] or new_value
             vals = {
                     "log_id": log_id,
-                    "field_id": field_id,
+                    "field_id": field_id and field_id[0] or False,
                     "old_value": old_value,
                     "new_value": new_value,
-                    "old_value_text": old_value_text,
-                    "new_value_text": new_value_text,
-                    "field_description": field['field_description']
+                    "old_value_text": line.get('old_value_text', ''),
+                    "new_value_text": line.get('new_value_text', ''),
+                    "field_description": field_obj.string
                     }
             line_id = log_line_pool.create(cr, uid, vals)
-            cr.commit()
-        #End Loop
         return True
 
-    def log_fct(self, cr, uid, model, method, fct_src, *args):
+    def log_fct(self, cr, uid_orig, model, method, fct_src, *args):
         """
-        Logging function: This function is performs logging oprations according to method
-        @param db: the current database
-        @param uid: the current user’s ID for security checks,
-        @param object: Object who's values are being changed
-        @param method: method to log: create, read, write, unlink
+        Logging function: This function is performing the logging operation
+        @param model: Object whose values are being changed
+        @param method: method to log: create, read, write, unlink, action or workflow action
         @param fct_src: execute method of Object proxy
 
         @return: Returns result as per method of Object proxy
         """
-        uid_orig = uid
-        uid = 1
-        res2 = args
         pool = pooler.get_pool(cr.dbname)
         resource_pool = pool.get(model)
-        log_pool = pool.get('audittrail.log')
         model_pool = pool.get('ir.model')
-
-        model_ids = model_pool.search(cr, uid, [('model', '=', model)])
+        model_ids = model_pool.search(cr, 1, [('model', '=', model)])
         model_id = model_ids and model_ids[0] or False
         assert model_id, _("'%s' Model does not exist..." %(model))
-        model = model_pool.browse(cr, uid, model_id)
-
-        if method in ('create'):
-            res_id = fct_src(cr, uid_orig, model.model, method, *args)
-            resource = resource_pool.read(cr, uid, res_id, args[0].keys())
-            vals = {
-                    "method": method,
-                    "object_id": model.id,
-                    "user_id": uid_orig,
-                    "res_id": resource['id'],
-            }
-            if 'id' in resource:
-                del resource['id']
-            log_id = log_pool.create(cr, uid, vals)
-            lines = []
-            for field in resource:
-                line = {
-                      'name': field,
-                      'new_value': resource[field],
-                      'new_value_text': self.get_value_text(cr, uid, field, resource[field], model)
-                      }
-                lines.append(line)
-            self.create_log_line(cr, uid, log_id, model, lines)
-
-            return res_id
-
-        elif method in ('read'):
-            res_ids = args[0]
-            old_values = {}
-            res = fct_src(cr, uid_orig, model.model, method, *args)
-            if type(res) == list:
-                for v in res:
-                    old_values[v['id']] = v
-            else:
-                old_values[res['id']] = res
-            for res_id in old_values:
-                vals = {
-                    "method": method,
-                    "object_id": model.id,
-                    "user_id": uid_orig,
-                    "res_id": res_id,
-
-                }
-                log_id = log_pool.create(cr, uid, vals)
-                lines = []
-                for field in old_values[res_id]:
-                    line = {
-                              'name': field,
-                              'old_value': old_values[res_id][field],
-                              'old_value_text': self.get_value_text(cr, uid, field, old_values[res_id][field], model)
-                              }
-                    lines.append(line)
-
-                self.create_log_line(cr, uid, log_id, model, lines)
-            return res
-
-        elif method in ('unlink'):
-            res_ids = args[0]
-            old_values = {}
-            for res_id in res_ids:
-                old_values[res_id] = resource_pool.read(cr, uid, res_id)
-
-            for res_id in res_ids:
-                vals = {
-                    "method": method,
-                    "object_id": model.id,
-                    "user_id": uid_orig,
-                    "res_id": res_id,
-
-                }
-                log_id = log_pool.create(cr, uid, vals)
-                lines = []
-                for field in old_values[res_id]:
-                    if field in ('id'):
-                        continue
-                    line = {
-                          'name': field,
-                          'old_value': old_values[res_id][field],
-                          'old_value_text': self.get_value_text(cr, uid, field, old_values[res_id][field], model)
-                          }
-                    lines.append(line)
-
-                self.create_log_line(cr, uid, log_id, model, lines)
-            res = fct_src(cr, uid_orig, model.model, method, *args)
-            return res
-        else:
-            res_ids = []
-            res = True
+        model = model_pool.browse(cr, 1, model_id)
+
+        # fields to log. currently only used by log on read()
+        field_list = []
+        old_values = new_values = {}
+
+        if method == 'create':
+            res = fct_src(cr, uid_orig, model.model, method, *args)
+            if res:
+                res_ids = [res]
+                new_values = self.get_data(cr, uid_orig, pool, res_ids, model, method)
+        elif method == 'read':
+            res = fct_src(cr, uid_orig, model.model, method, *args)
+            # build the res_ids and the old_values dict. Here we don't use get_data() to
+            # avoid performing an additional read()
+            res_ids = []
+            for record in res:
+                res_ids.append(record['id'])
+                old_values[(model.id, record['id'])] = {'value': record, 'text': record}
+            # log only the fields read
+            field_list = args[1]
+        elif method == 'unlink':
+            res_ids = args[0]
+            old_values = self.get_data(cr, uid_orig, pool, res_ids, model, method)
+            res = fct_src(cr, uid_orig, model.model, method, *args)
+        else: # method is write, action or workflow action
+            res_ids = []
             if args:
                 res_ids = args[0]
-                old_values = {}
-                fields = []
-                if len(args)>1 and type(args[1]) == dict:
-                    fields = args[1].keys()
-                if type(res_ids) in (long, int):
+                if isinstance(res_ids, (long, int)):
                     res_ids = [res_ids]
             if res_ids:
-                for resource in resource_pool.read(cr, uid, res_ids):
-                    resource_id = resource['id']
-                    if 'id' in resource:
-                        del resource['id']
-                    old_values_text = {}
-                    old_value = {}
-                    for field in resource.keys():
-                        old_value[field] = resource[field]
-                        old_values_text[field] = self.get_value_text(cr, uid, field, resource[field], model)
-                    old_values[resource_id] = {'text':old_values_text, 'value': old_value}
-
+                # store the old values into a dictionary
+                old_values = self.get_data(cr, uid_orig, pool, res_ids, model, method)
+            # process the original function, workflow trigger...
             res = fct_src(cr, uid_orig, model.model, method, *args)
-
+            if method == 'copy':
+                res_ids = [res]
             if res_ids:
-                for resource in resource_pool.read(cr, uid, res_ids):
-                    resource_id = resource['id']
-                    if 'id' in resource:
-                        del resource['id']
-                    vals = {
-                        "method": method,
-                        "object_id": model.id,
-                        "user_id": uid_orig,
-                        "res_id": resource_id,
-                    }
-
-
-                    log_id = log_pool.create(cr, uid, vals)
-                    lines = []
-                    for field in resource.keys():
-                        line = {
-                              'name': field,
-                              'new_value': resource[field],
-                              'old_value': old_values[resource_id]['value'][field],
-                              'new_value_text': self.get_value_text(cr, uid, field, resource[field], model),
-                              'old_value_text': old_values[resource_id]['text'][field]
-                              }
-                        lines.append(line)
-
-                    self.create_log_line(cr, uid, log_id, model, lines)
-            return res
+                # check the new values and store them into a dictionary
+                new_values = self.get_data(cr, uid_orig, pool, res_ids, model, method)
+        # compare the old and new values and create audittrail log if needed
+        self.process_data(cr, uid_orig, pool, res_ids, model, method, old_values, new_values, field_list)
+        return res
+
+    def get_data(self, cr, uid, pool, res_ids, model, method):
+        """
+        This function simply read all the fields of the given res_ids, and also recurisvely on
+        all records of a x2m fields read that need to be logged. Then it returns the result in
+        convenient structure that will be used as comparison basis.
+
+            :param cr: the current row, from the database cursor,
+            :param uid: the current user’s ID. This parameter is currently not used as every
+                operation to get data is made as super admin. Though, it could be usefull later.
+            :param pool: current db's pooler object.
+            :param res_ids: Id's of resource to be logged/compared.
+            :param model: Object whose values are being changed
+            :param method: method to log: create, read, unlink, write, actions, workflow actions
+            :return: dict mapping a tuple (model_id, resource_id) with its value and textual value
+                { (model_id, resource_id): { 'value': ...
+                                             'textual_value': ...
+                                           },
+                }
+        """
+        data = {}
+        resource_pool = pool.get(model.model)
+        # read all the fields of the given resources in super admin mode
+        for resource in resource_pool.read(cr, 1, res_ids):
+            values = {}
+            values_text = {}
+            resource_id = resource['id']
+            # loop on each field on the res_ids we just have read
+            for field in resource:
+                if field in ('__last_update', 'id'):
+                    continue
+                values[field] = resource[field]
+                # get the textual value of that field for this record
+                values_text[field] = self.get_value_text(cr, 1, pool, resource_pool, method, field, resource[field])
+
+                field_obj = resource_pool._all_columns.get(field).column
+                if field_obj._type in ('one2many','many2many'):
+                    # check if an audittrail rule apply in super admin mode
+                    if self.check_rules(cr, 1, field_obj._obj, method):
+                        # check if the model associated to a *2m field exists, in super admin mode
+                        x2m_model_ids = pool.get('ir.model').search(cr, 1, [('model', '=', field_obj._obj)])
+                        x2m_model_id = x2m_model_ids and x2m_model_ids[0] or False
+                        assert x2m_model_id, _("'%s' Model does not exist..." %(field_obj._obj))
+                        x2m_model = pool.get('ir.model').browse(cr, 1, x2m_model_id)
+                        #recursive call on x2m fields that need to be checked too
+                        data.update(self.get_data(cr, 1, pool, resource[field], x2m_model, method))
+            data[(model.id, resource_id)] = {'text':values_text, 'value': values}
+        return data
+
+    def prepare_audittrail_log_line(self, cr, uid, pool, model, resource_id, method, old_values, new_values, field_list=[]):
+        """
+        This function compares the old data (i.e before the method was executed) and the new data 
+        (after the method was executed) and returns a structure with all the needed information to
+        log those differences.
+
+        :param cr: the current row, from the database cursor,
+        :param uid: the current user’s ID. This parameter is currently not used as every
+            operation to get data is made as super admin. Though, it could be usefull later.
+        :param pool: current db's pooler object.
+        :param model: model object which values are being changed
+        :param resource_id: ID of record to which values are being changed
+        :param method: method to log: create, read, unlink, write, actions, workflow actions
+        :param old_values: dict of values read before execution of the method
+        :param new_values: dict of values read after execution of the method
+        :param field_list: optional argument containing the list of fields to log. Currently only
+            used when performing a read, it could be usefull later on if we want to log the write
+            on specific fields only.
+
+        :return: dictionary with
+            * keys: tuples build as ID of model object to log and ID of resource to log
+            * values: list of all the changes in field values for this couple (model, resource)
+              return {
+                (model.id, resource_id): []
+              }
+
+        The reason why the structure returned is build as above is because when modifying an existing 
+        record (res.partner, for example), we may have to log a change done in a x2many field (on 
+        res.partner.address, for example)
+        """
+        key = (model.id, resource_id)
+        lines = {
+            key: []
+        }
+        # loop on all the fields
+        for field_name, field_definition in pool.get(model.model)._all_columns.items():
+            #if the field_list param is given, skip all the fields not in that list
+            if field_list and field_name not in field_list:
+                continue
+            field_obj = field_definition.column
+            if field_obj._type in ('one2many','many2many'):
+                # checking if an audittrail rule apply in super admin mode
+                if self.check_rules(cr, 1, field_obj._obj, method):
+                    # checking if the model associated to a *2m field exists, in super admin mode
+                    x2m_model_ids = pool.get('ir.model').search(cr, 1, [('model', '=', field_obj._obj)])
+                    x2m_model_id = x2m_model_ids and x2m_model_ids[0] or False
+                    assert x2m_model_id, _("'%s' Model does not exist..." %(field_obj._obj))
+                    x2m_model = pool.get('ir.model').browse(cr, 1, x2m_model_id)
+                    # the resource_ids that need to be checked are the sum of both old and previous values (because we
+                    # need to log also creation or deletion in those lists).
+                    x2m_old_values_ids = old_values.get(key, {'value': {}})['value'].get(field_name, [])
+                    x2m_new_values_ids = new_values.get(key, {'value': {}})['value'].get(field_name, [])
+                    # We use list(set(...)) to remove duplicates.
+                    res_ids = list(set(x2m_old_values_ids + x2m_new_values_ids))
+                    for res_id in res_ids:
+                        lines.update(self.prepare_audittrail_log_line(cr, 1, pool, x2m_model, res_id, method, old_values, new_values, field_list))
+            # if the value value is different than the old value: record the change
+            if key not in old_values or key not in new_values or old_values[key]['value'][field_name] != new_values[key]['value'][field_name]:
+                data = {
+                      'name': field_name,
+                      'new_value': key in new_values and new_values[key]['value'].get(field_name),
+                      'old_value': key in old_values and old_values[key]['value'].get(field_name),
+                      'new_value_text': key in new_values and new_values[key]['text'].get(field_name),
+                      'old_value_text': key in old_values and old_values[key]['text'].get(field_name)
+                }
+                lines[key].append(data)
+        return lines
+
+    def process_data(self, cr, uid, pool, res_ids, model, method, old_values={}, new_values={}, field_list=[]):
+        """
+        This function processes and iterates recursively to log the difference between the old
+        data (i.e before the method was executed) and the new data and creates audittrail log
+        accordingly.
+
+        :param cr: the current row, from the database cursor,
+        :param uid: the current user’s ID,
+        :param pool: current db's pooler object.
+        :param res_ids: Id's of resource to be logged/compared.
+        :param model: model object which values are being changed
+        :param method: method to log: create, read, unlink, write, actions, workflow actions
+        :param old_values: dict of values read before execution of the method
+        :param new_values: dict of values read after execution of the method
+        :param field_list: optional argument containing the list of fields to log. Currently only
+            used when performing a read, it could be usefull later on if we want to log the write
+            on specific fields only.
+        :return: True
+        """
+        # loop on all the given ids
+        for res_id in res_ids:
+            # compare old and new values and get audittrail log lines accordingly
+            lines = self.prepare_audittrail_log_line(cr, uid, pool, model, res_id, method, old_values, new_values, field_list)
+
+            # if at least one modification has been found
+            for model_id, resource_id in lines:
+                vals = {
+                    'method': method,
+                    'object_id': model_id,
+                    'user_id': uid,
+                    'res_id': resource_id,
+                }
+                if (model_id, resource_id) not in old_values and method not in ('copy', 'read'):
+                    # the resource was not existing so we are forcing the method to 'create'
+                    # (because it could also come with the value 'write' if we are creating
+                    #  new record through a one2many field)
+                    vals.update({'method': 'create'})
+                if (model_id, resource_id) not in new_values and method not in ('copy', 'read'):
+                    # the resource is not existing anymore so we are forcing the method to 'unlink'
+                    # (because it could also come with the value 'write' if we are deleting the
+                    #  record through a one2many field)
+                    vals.update({'method': 'unlink'})
+                # create the audittrail log in super admin mode, only if a change has been detected
+                if lines[(model_id, resource_id)]:
+                    log_id = pool.get('audittrail.log').create(cr, 1, vals)
+                    model = pool.get('ir.model').browse(cr, uid, model_id)
+                    self.create_log_line(cr, 1, log_id, model, lines[(model_id, resource_id)])
         return True
 
     def check_rules(self, cr, uid, model, method):
+        """
+        Checks if auditrails is installed for that db and then if one rule match
+        @param cr: the current row, from the database cursor,
+        @param uid: the current user’s ID,
+        @param model: value of _name of the object which values are being changed
+        @param method: method to log: create, read, unlink,write,actions,workflow actions
+        @return: True or False
+        """
         pool = pooler.get_pool(cr.dbname)
-        # Check if auditrails is installed for that db and then if one rule match
         if 'audittrail.rule' in pool.models:
             model_ids = pool.get('ir.model').search(cr, 1, [('model', '=', model)])
             model_id = model_ids and model_ids[0] or False
             if model_id:
                 rule_ids = pool.get('audittrail.rule').search(cr, 1, [('object_id', '=', model_id), ('state', '=', 'subscribed')])
                 for rule in pool.get('audittrail.rule').read(cr, 1, rule_ids, ['user_id','log_read','log_write','log_create','log_unlink','log_action','log_workflow']):
-                    if len(rule['user_id'])==0 or uid in rule['user_id']:
+                    if len(rule['user_id']) == 0 or uid in rule['user_id']:
                         if rule.get('log_'+method,0):
                             return True
                         elif method not in ('default_get','read','fields_view_get','fields_get','search','search_count','name_search','name_get','get','request_get', 'get_sc', 'unlink', 'write', 'create'):
@@ -460,18 +491,14 @@
     def execute_cr(self, cr, uid, model, method, *args, **kw):
         fct_src = super(audittrail_objects_proxy, self).execute_cr
         if self.check_rules(cr,uid,model,method):
-            res = self.log_fct(cr, uid, model, method, fct_src, *args)
-        else:
-            res = fct_src(cr, uid, model, method, *args)
-        return res
+            return self.log_fct(cr, uid, model, method, fct_src, *args)
+        return fct_src(cr, uid, model, method, *args)
 
     def exec_workflow_cr(self, cr, uid, model, method, *args, **argv):
         fct_src = super(audittrail_objects_proxy, self).exec_workflow_cr
         if self.check_rules(cr,uid,model,'workflow'):
-            res = self.log_fct(cr, uid, model, method, fct_src, *args)
-        else:
-            res = fct_src(cr, uid, model, method, *args)
-        return res
+            return self.log_fct(cr, uid, model, method, fct_src, *args)
+        return fct_src(cr, uid, model, method, *args)
 
 audittrail_objects_proxy()